mcp tool groups#1464
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces a concrete MCP manager with a new exported interface and updates all usages; adds three new React UI selectors (entity, MCP server, MCP tool); adjusts MCP agent auto-execution set and readToolFile behavior; adds MCP-related types and a new API tag. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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: 4
🤖 Fix all issues with AI agents
In `@core/bifrost.go`:
- Around line 2801-2808: The SetMCPManager method currently assigns whatever is
passed into bifrost.McpManager; add a simple nil guard so that if the incoming
manager is nil (or an untyped nil interface) you explicitly set
bifrost.McpManager = nil, otherwise assign the provided manager; locate the
SetMCPManager function and the MCPManagerInterface type to implement this check
and update the assignment accordingly and optionally add a short comment
documenting that callers must pass a properly initialized MCPManagerInterface.
In `@ui/components/ui/entityAssociationSelect.tsx`:
- Around line 110-135: The equality check in the selectedValues computation
(inside the value.map and options.find) can miss matches when ids are different
types; update the lookup to normalize types before comparing (e.g., compare
String(opt.id) to String(id) or otherwise cast both sides) so existingOption is
found and label/meta from options are preserved for Option<EntityOptionMeta>;
ensure the same normalization is used consistently in selectedValues and
anywhere else you match ids in this component.
In `@ui/components/ui/mcpServerSelector.tsx`:
- Around line 229-232: The href for the registry link is building a query param
with raw server.clientId which can break the URL for special characters; update
the anchor in the mcpServerSelector component to URL-encode the clientId (use
encodeURIComponent on server.clientId) when composing
`${registryPath}?server=...` so it matches the pattern used elsewhere (e.g.,
encodeURIComponent(provider)/encodeURIComponent(token)); ensure target and rel
attributes remain unchanged.
In `@ui/components/ui/mcpToolSelector.tsx`:
- Around line 232-307: The Collapsible is using asChild but its direct child is
a fragment (<>...</>), which prevents props/refs from being forwarded; either
remove the asChild prop on Collapsible so it renders its internal wrapper
normally, or replace the fragment with a single DOM element (e.g., a <tbody> or
<div>) so Collapsible(asChild) can forward props—update the JSX where
Collapsible is declared (the Collapsible open/onOpenChange block) and keep
CollapsibleTrigger and CollapsibleContent usage unchanged.
🧹 Nitpick comments (1)
ui/components/ui/mcpToolSelector.tsx (1)
13-35: Prefer reusing the sharedSelectedTooltype to avoid drift.Now that
SelectedToolis exported fromui/lib/types/mcp.ts, import/re‑export it here instead of redefining.♻️ Suggested refactor
import { ChevronDown, ChevronRight, X } from "lucide-react" import { useCallback, useMemo, useState } from "react" +import type { SelectedTool } from "../../lib/types/mcp" +export type { SelectedTool } from "../../lib/types/mcp" @@ -// Types - -export interface SelectedTool { - mcpClientId: string - toolName: string -} +// Types
| // SetMCPManager sets the MCP manager for this Bifrost instance. | ||
| // This is primarily used for testing purposes to inject a custom MCP manager. | ||
| // This allows injecting a custom MCP manager implementation (e.g., for enterprise features). | ||
| // | ||
| // Parameters: | ||
| // - manager: The MCP manager to set | ||
| func (bifrost *Bifrost) SetMCPManager(manager *mcp.MCPManager) { | ||
| bifrost.mcpManager = manager | ||
| // - manager: The MCP manager to set (must implement MCPManagerInterface) | ||
| func (bifrost *Bifrost) SetMCPManager(manager mcp.MCPManagerInterface) { | ||
| bifrost.McpManager = manager | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if there's information about a PR stack or related changes
git log --oneline -20Repository: maximhq/bifrost
Length of output: 83
🏁 Script executed:
# Find the SetMCPManager method to see the actual implementation
fd core/bifrost.go -x cat -n {} | head -100Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Get more context around lines 2801-2808 in core/bifrost.go
wc -l core/bifrost.goRepository: maximhq/bifrost
Length of output: 79
🏁 Script executed:
# Search for how McpManager is used in the codebase
rg "McpManager" --type go -A 2 -B 2 | head -100Repository: maximhq/bifrost
Length of output: 4959
🏁 Script executed:
# Check the MCPManagerInterface definition
rg "MCPManagerInterface" --type go -A 5 -B 2 | head -150Repository: maximhq/bifrost
Length of output: 2341
🏁 Script executed:
# Search for actual calls to SetMCPManager
rg "SetMCPManager" --type go -B 3 -A 3Repository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
# Check if there are any typed-nil patterns or reflect usage in the codebase
rg "reflect\." --type go | head -30Repository: maximhq/bifrost
Length of output: 2613
🏁 Script executed:
# Get the exact location and context of SetMCPManager in bifrost.go
sed -n '2795,2815p' core/bifrost.goRepository: maximhq/bifrost
Length of output: 756
🏁 Script executed:
# Check the MCPManagerInterface method signatures to understand what methods are called
fd core/mcp/interface.go -x cat -n | head -200Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Search for nil checks patterns in the codebase related to McpManager
rg "McpManager.*!= nil|McpManager.*== nil" --type go -B 2 -A 5Repository: maximhq/bifrost
Length of output: 6262
🏁 Script executed:
# Check if there are any existing patterns in Go standard library or common patterns for validating interface implementations
rg "reflect.ValueOf.*Kind.*Ptr.*IsNil" --type go || echo "No match found in codebase"Repository: maximhq/bifrost
Length of output: 85
🏁 Script executed:
# Look at how other setters in the codebase handle similar interface validation
rg "func.*Set.*interface\{}" --type go -A 3 | head -50Repository: maximhq/bifrost
Length of output: 708
🏁 Script executed:
# Check if there are any other MCPManager validation patterns
fd core/mcp/ -name "*.go" -x cat -n {} | grep -A 10 -B 5 "nil" | head -80Repository: maximhq/bifrost
Length of output: 289
🏁 Script executed:
# Check the MCPManager implementation to see if it's likely a typed-nil could exist
fd "manager.go" core/mcp/ -x head -100 {}Repository: maximhq/bifrost
Length of output: 7046
🏁 Script executed:
# Search for how MCPManager is instantiated
rg "NewMCPManager" --type go -B 3 -A 5Repository: maximhq/bifrost
Length of output: 3504
🏁 Script executed:
# Check if there are any tests or patterns showing typed-nil issues
rg "typed.nil|typed nil" --type goRepository: maximhq/bifrost
Length of output: 41
Consider a simpler safeguard for SetMCPManager against nil or invalid implementations.
While a nil pointer stored in an interface value can pass != nil checks in Go, a more practical concern is ensuring the manager is properly initialized before use. However, the reflect-based approach is unconventional. A simpler alternative would be to document this expectation or add a basic nil check that converts to nil explicitly:
func (bifrost *Bifrost) SetMCPManager(manager mcp.MCPManagerInterface) {
+ if manager == nil {
+ bifrost.McpManager = nil
+ return
+ }
bifrost.McpManager = manager
}This covers the common case without requiring reflection. If stricter typed-nil validation is needed, that's better handled through code review and documentation of what valid implementations must provide.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SetMCPManager sets the MCP manager for this Bifrost instance. | |
| // This is primarily used for testing purposes to inject a custom MCP manager. | |
| // This allows injecting a custom MCP manager implementation (e.g., for enterprise features). | |
| // | |
| // Parameters: | |
| // - manager: The MCP manager to set | |
| func (bifrost *Bifrost) SetMCPManager(manager *mcp.MCPManager) { | |
| bifrost.mcpManager = manager | |
| // - manager: The MCP manager to set (must implement MCPManagerInterface) | |
| func (bifrost *Bifrost) SetMCPManager(manager mcp.MCPManagerInterface) { | |
| bifrost.McpManager = manager | |
| } | |
| // SetMCPManager sets the MCP manager for this Bifrost instance. | |
| // This allows injecting a custom MCP manager implementation (e.g., for enterprise features). | |
| // | |
| // Parameters: | |
| // - manager: The MCP manager to set (must implement MCPManagerInterface) | |
| func (bifrost *Bifrost) SetMCPManager(manager mcp.MCPManagerInterface) { | |
| if manager == nil { | |
| bifrost.McpManager = nil | |
| return | |
| } | |
| bifrost.McpManager = manager | |
| } |
🤖 Prompt for AI Agents
In `@core/bifrost.go` around lines 2801 - 2808, The SetMCPManager method currently
assigns whatever is passed into bifrost.McpManager; add a simple nil guard so
that if the incoming manager is nil (or an untyped nil interface) you explicitly
set bifrost.McpManager = nil, otherwise assign the provided manager; locate the
SetMCPManager function and the MCPManagerInterface type to implement this check
and update the assignment accordingly and optionally add a short comment
documenting that callers must pass a properly initialized MCPManagerInterface.
| // Convert selected IDs to Option format | ||
| const selectedValues = useMemo((): Option<EntityOptionMeta>[] => { | ||
| return value.map((id) => { | ||
| // Try to find the option in the provided options | ||
| const existingOption = options.find((opt) => opt.id === id) | ||
| if (existingOption) { | ||
| return { | ||
| label: existingOption.label, | ||
| value: String(existingOption.id), | ||
| meta: { | ||
| id: existingOption.id, | ||
| description: existingOption.description, | ||
| metadata: existingOption.metadata, | ||
| }, | ||
| } | ||
| } | ||
| // If not found, create a basic option | ||
| return { | ||
| label: String(id), | ||
| value: String(id), | ||
| meta: { | ||
| id, | ||
| }, | ||
| } | ||
| }) | ||
| }, [value, options]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 103
🏁 Script executed:
wc -l ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 107
🏁 Script executed:
head -50 ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 1605
🏁 Script executed:
sed -n '100,150p' ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 1474
🏁 Script executed:
rg "entityAssociationSelect|EntityAssociationSelect" --type tsx --type ts -A 3 -B 3Repository: maximhq/bifrost
Length of output: 86
🏁 Script executed:
rg "entityAssociationSelect|EntityAssociationSelect" --type ts -A 3 -B 3 | head -100Repository: maximhq/bifrost
Length of output: 1583
🏁 Script executed:
rg "EntityAssociationSelect" -A 5 -B 2 | grep -E "value=|EntityAssociationSelect|<EntityAssociationSelect" | head -50Repository: maximhq/bifrost
Length of output: 311
🏁 Script executed:
rg "import.*EntityAssociationSelect|from.*entityAssociationSelect" -A 10 -B 2Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
rg "<EntityAssociationSelect" -A 5Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
sed -n '1,100p' ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 2915
🏁 Script executed:
sed -n '150,246p' ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 3119
🏁 Script executed:
fd "\.tsx$\|\.ts$" | xargs rg "EntityAssociationSelect" | grep -v "entityAssociationSelect.tsx"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
git log --oneline -n 20 ui/components/ui/entityAssociationSelect.tsx 2>/dev/null || echo "No git history available"Repository: maximhq/bifrost
Length of output: 83
🏁 Script executed:
git log --oneline -n 50 | head -20Repository: maximhq/bifrost
Length of output: 83
🏁 Script executed:
rg "id.*string.*number|string.*number.*id" ui/components/ui/entityAssociationSelect.tsxRepository: maximhq/bifrost
Length of output: 318
🏁 Script executed:
python3 << 'EOF'
# Analyzing the type mismatch issue
# The types allow:
# - value: (string | number)[]
# - options[].id: string | number
# Strict equality comparison behavior:
test_cases = [
("string '123'", "number 123", "'123' === 123", False),
("string '123'", "string '123'", "'123' === '123'", True),
("number 123", "number 123", "123 === 123", True),
("number 123", "string '123'", "123 === '123'", False),
]
print("Strict Equality (===) Behavior for ID Matching:")
print("=" * 60)
for type1, type2, expr, result in test_cases:
print(f"{type1:20} vs {type2:20} | {expr:20} → {result}")
print("\n" + "=" * 60)
print("Type system allows both types, but strict === will fail on mismatches")
print("This means selectedValues could miss label/metadata lookups if types differ")
EOFRepository: maximhq/bifrost
Length of output: 145
Normalize ID matching to avoid losing labels/metadata.
The component accepts both strings and numbers for ids (value: (string | number)[] and options[].id: string | number). Using strict equality at line 113 will fail when comparing mismatched types—for example, when value contains string "123" while options contains number 123. This causes the lookup to fail and fall back to a basic option without label or metadata.
Suggested fix
- const existingOption = options.find((opt) => opt.id === id)
+ const existingOption = options.find((opt) => String(opt.id) === String(id))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Convert selected IDs to Option format | |
| const selectedValues = useMemo((): Option<EntityOptionMeta>[] => { | |
| return value.map((id) => { | |
| // Try to find the option in the provided options | |
| const existingOption = options.find((opt) => opt.id === id) | |
| if (existingOption) { | |
| return { | |
| label: existingOption.label, | |
| value: String(existingOption.id), | |
| meta: { | |
| id: existingOption.id, | |
| description: existingOption.description, | |
| metadata: existingOption.metadata, | |
| }, | |
| } | |
| } | |
| // If not found, create a basic option | |
| return { | |
| label: String(id), | |
| value: String(id), | |
| meta: { | |
| id, | |
| }, | |
| } | |
| }) | |
| }, [value, options]) | |
| // Convert selected IDs to Option format | |
| const selectedValues = useMemo((): Option<EntityOptionMeta>[] => { | |
| return value.map((id) => { | |
| // Try to find the option in the provided options | |
| const existingOption = options.find((opt) => String(opt.id) === String(id)) | |
| if (existingOption) { | |
| return { | |
| label: existingOption.label, | |
| value: String(existingOption.id), | |
| meta: { | |
| id: existingOption.id, | |
| description: existingOption.description, | |
| metadata: existingOption.metadata, | |
| }, | |
| } | |
| } | |
| // If not found, create a basic option | |
| return { | |
| label: String(id), | |
| value: String(id), | |
| meta: { | |
| id, | |
| }, | |
| } | |
| }) | |
| }, [value, options]) |
🤖 Prompt for AI Agents
In `@ui/components/ui/entityAssociationSelect.tsx` around lines 110 - 135, The
equality check in the selectedValues computation (inside the value.map and
options.find) can miss matches when ids are different types; update the lookup
to normalize types before comparing (e.g., compare String(opt.id) to String(id)
or otherwise cast both sides) so existingOption is found and label/meta from
options are preserved for Option<EntityOptionMeta>; ensure the same
normalization is used consistently in selectedValues and anywhere else you match
ids in this component.
| <a | ||
| href={`${registryPath}?server=${server.clientId}`} | ||
| target="_blank" | ||
| rel="noopener noreferrer" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the specific file
find . -type f -name "mcpServerSelector.tsx" 2>/dev/nullRepository: maximhq/bifrost
Length of output: 99
🏁 Script executed:
# Check the file size and examine the relevant lines
if [ -f "ui/components/ui/mcpServerSelector.tsx" ]; then
wc -l "ui/components/ui/mcpServerSelector.tsx"
echo "---"
sed -n '220,240p' "ui/components/ui/mcpServerSelector.tsx"
fiRepository: maximhq/bifrost
Length of output: 835
🏁 Script executed:
# Also search for other instances of similar patterns where clientId might be used in URLs
rg "href=.*\?.*server=" -A 2 -B 2 ui/components/Repository: maximhq/bifrost
Length of output: 445
🏁 Script executed:
# Search for any encodeURIComponent usage to see if it's used elsewhere
rg "encodeURIComponent" ui/ || echo "No instances found"Repository: maximhq/bifrost
Length of output: 374
🏁 Script executed:
# Check if there are related files or configurations about URL encoding patterns
rg "registryPath" --type tsx --type tsRepository: maximhq/bifrost
Length of output: 86
🏁 Script executed:
# Let me look at more context around the line and understand what registryPath is
sed -n '200,240p' "ui/components/ui/mcpServerSelector.tsx"Repository: maximhq/bifrost
Length of output: 1600
🏁 Script executed:
# Search for the definition of registryPath in the same file
rg "registryPath" "ui/components/ui/mcpServerSelector.tsx" -B 5 -A 2Repository: maximhq/bifrost
Length of output: 807
🏁 Script executed:
# Check where server.clientId comes from and what its structure might be
rg "clientId" "ui/components/ui/mcpServerSelector.tsx" -B 3 -A 1Repository: maximhq/bifrost
Length of output: 1321
🏁 Script executed:
# Look at git log to see if there's a PR stack or related commits
git log --oneline -20Repository: maximhq/bifrost
Length of output: 83
🏁 Script executed:
# Check if there are similar URL parameter patterns elsewhere in the same file
rg "href=.*\$" "ui/components/ui/mcpServerSelector.tsx"Repository: maximhq/bifrost
Length of output: 129
Encode server IDs in the registry URL to prevent query string breakage.
The server.clientId parameter should be URL-encoded to handle special characters. The codebase already follows this pattern elsewhere (e.g., encodeURIComponent(provider) in governanceApi.ts, encodeURIComponent(token) in useWebSocket.tsx).
✅ Suggested fix
- href={`${registryPath}?server=${server.clientId}`}
+ href={`${registryPath}?server=${encodeURIComponent(server.clientId)}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a | |
| href={`${registryPath}?server=${server.clientId}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| <a | |
| href={`${registryPath}?server=${encodeURIComponent(server.clientId)}`} | |
| target="_blank" | |
| rel="noopener noreferrer" |
🤖 Prompt for AI Agents
In `@ui/components/ui/mcpServerSelector.tsx` around lines 229 - 232, The href for
the registry link is building a query param with raw server.clientId which can
break the URL for special characters; update the anchor in the mcpServerSelector
component to URL-encode the clientId (use encodeURIComponent on server.clientId)
when composing `${registryPath}?server=...` so it matches the pattern used
elsewhere (e.g., encodeURIComponent(provider)/encodeURIComponent(token)); ensure
target and rel attributes remain unchanged.
| <Collapsible | ||
| key={key} | ||
| open={isExpanded} | ||
| onOpenChange={() => toggleExpanded(key)} | ||
| asChild | ||
| > | ||
| <> | ||
| <TableRow className="group"> | ||
| <TableCell className="p-2"> | ||
| <CollapsibleTrigger asChild> | ||
| <button | ||
| type="button" | ||
| className="hover:bg-muted flex h-8 w-8 items-center justify-center rounded-md transition-colors" | ||
| disabled={!tool.parameters} | ||
| > | ||
| {tool.parameters ? ( | ||
| isExpanded ? ( | ||
| <ChevronDown className="h-4 w-4" /> | ||
| ) : ( | ||
| <ChevronRight className="h-4 w-4" /> | ||
| ) | ||
| ) : ( | ||
| <span className="h-4 w-4" /> | ||
| )} | ||
| </button> | ||
| </CollapsibleTrigger> | ||
| </TableCell> | ||
| <TableCell> | ||
| <div className="min-w-0"> | ||
| <div className="text-foreground truncate text-sm font-medium"> | ||
| {tool.toolName} | ||
| </div> | ||
| {tool.description && ( | ||
| <p className="text-muted-foreground mt-0.5 line-clamp-1 text-xs"> | ||
| {tool.description} | ||
| </p> | ||
| )} | ||
| </div> | ||
| </TableCell> | ||
| <TableCell className="hidden md:table-cell"> | ||
| <Badge variant="outline">{tool.mcpClientName}</Badge> | ||
| </TableCell> | ||
| <TableCell className="p-2"> | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-8 w-8 p-0" | ||
| onClick={() => handleRemoveTool(tool.mcpClientId, tool.toolName)} | ||
| disabled={disabled} | ||
| > | ||
| <X className="h-4 w-4" /> | ||
| </Button> | ||
| </TableCell> | ||
| </TableRow> | ||
| <CollapsibleContent asChild> | ||
| <tr> | ||
| <td colSpan={4} className="p-0"> | ||
| <div className="bg-muted/30 border-t px-4 py-3"> | ||
| <div className="text-muted-foreground mb-2 text-xs font-medium"> | ||
| Parameters Schema | ||
| </div> | ||
| {tool.parameters ? ( | ||
| <pre className="bg-muted overflow-auto rounded-md p-3 text-xs"> | ||
| <code>{JSON.stringify(tool.parameters, null, 2)}</code> | ||
| </pre> | ||
| ) : ( | ||
| <div className="text-muted-foreground text-sm"> | ||
| No parameters defined | ||
| </div> | ||
| )} | ||
| </div> | ||
| </td> | ||
| </tr> | ||
| </CollapsibleContent> | ||
| </> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Collapsible and Table component implementations
echo "=== Inspecting collapsible.tsx ==="
cat ui/components/ui/collapsible.tsx
echo -e "\n=== Inspecting table.tsx ==="
cat ui/components/ui/table.tsxRepository: maximhq/bifrost
Length of output: 3168
Remove asChild from Collapsible or remove the fragment wrapper.
The asChild prop on Collapsible (from Radix UI) merges props/refs into its first child via the Slot pattern. A fragment <> cannot receive props or refs, blocking this mechanism. Either remove asChild to let Collapsible render normally, or restructure to pass a single DOM element (not wrapped in a fragment) as the direct child when using asChild.
🤖 Prompt for AI Agents
In `@ui/components/ui/mcpToolSelector.tsx` around lines 232 - 307, The Collapsible
is using asChild but its direct child is a fragment (<>...</>), which prevents
props/refs from being forwarded; either remove the asChild prop on Collapsible
so it renders its internal wrapper normally, or replace the fragment with a
single DOM element (e.g., a <tbody> or <div>) so Collapsible(asChild) can
forward props—update the JSX where Collapsible is declared (the Collapsible
open/onOpenChange block) and keep CollapsibleTrigger and CollapsibleContent
usage unchanged.
akshaydeo
force-pushed
the
01-28-mcp_tool_groups
branch
from
933ba82 to
8a3e7d8
Compare
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)
core/mcp/codemode/starlark/readfile.go (1)
204-252: Avoid “complete file” headers on partial reads.Because the header is inserted before slicing, any request with
startLine/endLinecan return only a subset while still saying “this is the complete file.” That can cause callers to stop paginating early. Update the header after clamping so it reflects whether the full range was returned (e.g., “showing lines X–Y of Z”), or only add the “complete file” suffix when the final range is full.🛠️ Suggested fix (adjust header when returning a subset)
if start > end { // If start > end after clamping, just return the start line end = start } + if start == 1 && end < totalLines { + lines[0] = fmt.Sprintf("# Total lines: %d (showing lines %d-%d)", totalLines, start, end) + } // Slice lines (convert to 0-based indexing) selectedLines := lines[start-1 : end] fileContent = strings.Join(selectedLines, "\n") }
🧹 Nitpick comments (1)
ui/components/ui/mcpToolSelector.tsx (1)
15-38: Prefer the sharedSelectedTooltype to avoid drift.
SelectedToolnow exists inui/lib/types/mcp.ts; consider reusing it here instead of redefining it locally to keep types consistent across UI surfaces.♻️ Possible refactor
+import type { SelectedTool } from "@/lib/types/mcp"; +export type { SelectedTool } from "@/lib/types/mcp"; // Types -export interface SelectedTool { - mcpClientId: string - toolName: string -}
akshaydeo
changed the base branch from
01-27-mcp_flow_refactoring
to
graphite-base/1464
1 participant
Summary
Refactor the MCP (Model Control Protocol) manager to use an interface instead of a concrete implementation, enabling enterprise extensions and custom implementations. Also add new UI components for MCP server and tool selection.
Changes
mcpManagerfield in Bifrost struct toMcpManager(exported) with interface typeMCPManagerInterfaceincore/mcp/interface.goto define the contractEntityAssociationSelect: Generic component for selecting entities with metadataMCPServerSelector: Component for selecting MCP servers with status indicatorsMCPToolSelector: Component for selecting and viewing MCP tools with parameter schemasType of change
Affected areas
How to test
Test the interface implementation by verifying core functionality works as before:
For UI components, create a test page that uses the new selectors with mock data.
Breaking changes
Related issues
Enables enterprise extensions to the MCP system while maintaining compatibility.
Security considerations
No security implications as this is a refactoring of existing functionality with new UI components.
Checklist