code mode compact signatures#1453
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces the TypeScript/Goja code-mode with a Starlark/Python-based CodeMode, introduces a CodeMode abstraction and Starlark implementation, adds per-client tool synchronization and related config/DB/UI changes, and updates dependencies to include go.starlark.net while removing goja/go-typescript. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Assistant
participant ToolMgr as ToolsManager
participant CodeMode as StarlarkCodeMode
participant Pipeline as PluginPipeline
participant Starlark as Starlark Runtime
participant MCP as MCP Client
Client->>ToolMgr: ExecuteToolInternal(toolCall)
alt Is CodeMode Tool?
ToolMgr->>CodeMode: ExecuteTool(ctx, toolCall)
CodeMode->>CodeMode: Parse tool name & args
alt executeToolCode
CodeMode->>Starlark: Run code with bindings (timeout)
Starlark->>CodeMode: print()/logs captured
Starlark->>CodeMode: callMCPTool(...) when invoking MCP tool
CodeMode->>Pipeline: RunMCPPreHooks(request)
Pipeline->>MCP: Invoke MCP tool
MCP-->>Pipeline: Tool response
Pipeline->>CodeMode: RunMCPPostHooks(response)
CodeMode->>Starlark: Return tool result to script
Starlark-->>CodeMode: Final result + logs + errors
else list/read/docs
CodeMode->>CodeMode: Generate/fetch virtual .pyi or docs
end
CodeMode->>ToolMgr: Return ChatMessage response
else Regular MCP Tool
ToolMgr->>MCP: Regular MCP call flow
MCP-->>ToolMgr: Response
end
ToolMgr-->>Client: ChatMessage / Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/mcp/utils.go (1)
151-170: Fix undefined tool-name mapping variables.Line 170 assigns
toolNameMapping[sanitizedToolName] = originalMCPName, but neither identifier is declared in this scope, so the file won’t compile. Define the sanitized key and map it to the original MCP name.🐛 Proposed fix
- validationName := strings.ReplaceAll(mcpTool.Name, "-", "_") - if err := validateNormalizedToolName(validationName); err != nil { + sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_") + if err := validateNormalizedToolName(sanitizedToolName); err != nil { - toolNameMapping[sanitizedToolName] = originalMCPName + toolNameMapping[sanitizedToolName] = mcpTool.Namecore/mcp/codemodeexecutecode.go (1)
695-701: UndefinedoriginalMCPToolNamebreaks build.Line 699 references
originalMCPToolName, which isn’t in scope. UseoriginalToolName(ortoolNameToCall) instead.🐛 Proposed fix
- ToolName: originalMCPToolName, + ToolName: originalToolName,
🤖 Fix all issues with AI agents
In `@core/mcp/codemodeexecutecode.go`:
- Around line 345-362: The timeout context created as
toolExecutionTimeout/timeoutCtx isn't observed by starlark.ExecFile; replace
context-based cancellation with a timer that calls thread.Cancel() to enforce
the execution timeout: create the starlark.Thread (as currently done), remove or
stop using timeoutCtx/SetLocal, start a time.AfterFunc(toolExecutionTimeout,
func(){ thread.Cancel() }) before calling starlark.ExecFile, capture the
returned timer and stop it (timer.Stop()) immediately after ExecFile returns to
avoid leaks, and ensure you still call defer cancel() or remove unused cancel
variables so no unused context remains; reference symbols: toolExecutionTimeout,
timeoutCtx, thread, starlark.ExecFile, thread.Cancel(), time.AfterFunc.
🧹 Nitpick comments (6)
core/mcp/codemodelistfiles.go (1)
101-113: Keep stub filenames aligned with bound tool identifiers.Runtime bindings normalize with
parseToolName(lowercase + identifier rules), but the list output only replaces hyphens. Tools with spaces/case can yield .pyi filenames that don’t match the callable name. Consider reusing the same normalization path.♻️ Suggested alignment
- toolName := stripClientPrefix(tool.Function.Name, clientName) - // Replace any remaining hyphens with underscores for Python compatibility - toolName = strings.ReplaceAll(toolName, "-", "_") + toolName := stripClientPrefix(tool.Function.Name, clientName) + toolName = strings.ReplaceAll(toolName, "-", "_") + toolName = parseToolName(toolName)core/mcp/codemodegetdocs.go (2)
86-114: Consider handling multiple matching servers for consistency.
handleReadToolFileincodemodereadfile.gotracksmatchCountand returns an error when multiple servers match case-insensitively (lines 119-130). This function breaks on the first match without similar ambiguity detection, which could lead to inconsistent behavior between the two tools.♻️ Suggested approach
+ matchCount := 0 + for clientName, tools := range availableToolsPerClient { client := m.clientManager.GetClientByName(clientName) if client == nil { logger.Warn("%s Client %s not found, skipping", MCPLogPrefix, clientName) continue } if !client.ExecutionConfig.IsCodeModeClient || len(tools) == 0 { continue } clientNameLower := strings.ToLower(clientName) if clientNameLower == serverNameLower { + matchCount++ + if matchCount > 1 { + errorMsg := fmt.Sprintf("Multiple servers match '%s':\n", serverName) + for name := range availableToolsPerClient { + if strings.ToLower(name) == serverNameLower { + errorMsg += fmt.Sprintf(" - %s\n", name) + } + } + errorMsg += "\nPlease use the exact server name from listToolFiles." + return createToolResponseMessage(toolCall, errorMsg), nil + } matchedClientName = clientName // ... rest of matching logic - break } }
116-148: Consider sorting available servers/tools for consistent error messages.The lists of available servers (line 122) and tools (line 140) are built by iterating over maps, which have non-deterministic order in Go. Sorting these lists would provide more consistent, user-friendly error messages.
core/mcp/codemodereadfile.go (3)
396-409: Usesort.Strings()instead of manual bubble sort.The manual bubble sort implementation works but is non-idiomatic Go. Using
sort.Strings()from the standard library is cleaner, more efficient (O(n log n) vs O(n²)), and reduces code complexity. This pattern is also repeated ingenerateTypeDefinitions(lines 568-574).♻️ Proposed fix
Add to imports:
import ( // ... existing imports "sort" )Then replace the manual sorts:
- // Simple alphabetical sort for each group - for i := 0; i < len(requiredNames)-1; i++ { - for j := i + 1; j < len(requiredNames); j++ { - if requiredNames[i] > requiredNames[j] { - requiredNames[i], requiredNames[j] = requiredNames[j], requiredNames[i] - } - } - } - for i := 0; i < len(optionalNames)-1; i++ { - for j := i + 1; j < len(optionalNames); j++ { - if optionalNames[i] > optionalNames[j] { - optionalNames[i], optionalNames[j] = optionalNames[j], optionalNames[i] - } - } - } + sort.Strings(requiredNames) + sort.Strings(optionalNames)Apply the same fix in
generateTypeDefinitions(lines 568-574).
445-479: Consider handlinganyOf/oneOf/allOfcomposition types.The function handles basic JSON Schema types and enums well, but doesn't handle composition types (
anyOf,oneOf,allOf) which are defined inToolFunctionParameters(per the relevant code snippets). These would currently return"Any", which may be acceptable but loses type information.If these composition types are used by MCP tools, consider adding support:
♻️ Example enhancement for union types
// Check for anyOf/oneOf (union types) if anyOf, ok := prop["anyOf"].([]interface{}); ok && len(anyOf) > 0 { types := make([]string, 0, len(anyOf)) for _, item := range anyOf { if itemMap, ok := item.(map[string]interface{}); ok { types = append(types, jsonSchemaToPython(itemMap)) } } return "Union[" + strings.Join(types, ", ") + "]" }
636-649: Non-deterministic example parameter selection.The function iterates over a map to find the first parameter for examples. Since Go map iteration order is non-deterministic, the example shown could vary between runs. Consider sorting parameter names first for consistent documentation output.
♻️ Proposed fix for deterministic examples
+import "sort" func getExampleParams(params *schemas.ToolFunctionParameters) string { if params == nil || params.Properties == nil || len(*params.Properties) == 0 { return "" } props := *params.Properties required := make(map[string]bool) if params.Required != nil { for _, req := range params.Required { required[req] = true } } - // Get first required param as example - for name := range props { - if required[name] { - return fmt.Sprintf("%s=\"...\"", name) - } - } - - // If no required, get first param - for name := range props { - return fmt.Sprintf("%s=\"...\"", name) - } + // Sort names for deterministic output + names := make([]string, 0, len(props)) + for name := range props { + names = append(names, name) + } + sort.Strings(names) + + // Get first required param as example + for _, name := range names { + if required[name] { + return fmt.Sprintf("%s=\"...\"", name) + } + } + + // If no required, get first param alphabetically + if len(names) > 0 { + return fmt.Sprintf("%s=\"...\"", names[0]) + } return "" }
| defer cancel() | ||
|
|
||
| // Try to convert to object to check if it's a promise | ||
| // Use recover to safely handle null values that can't be converted to objects | ||
| var valObj *goja.Object | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Value is null or can't be converted to object, just export it | ||
| valObj = nil | ||
| } | ||
| }() | ||
| valObj = val.ToObject(vm) | ||
| }() | ||
|
|
||
| if valObj != nil { | ||
| // Check if it has a 'then' method (Promise-like) | ||
| if then := valObj.Get("then"); then != nil && then != goja.Undefined() { | ||
| // It's a promise, we need to await it | ||
| // Use buffered channels to prevent blocking if handlers are called after timeout | ||
| resultChan := make(chan interface{}, 1) | ||
| errChan := make(chan error, 1) | ||
|
|
||
| // Set up promise handlers | ||
| thenFunc, ok := goja.AssertFunction(then) | ||
| if ok { | ||
| // Call then with resolve and reject handlers | ||
| _, err := thenFunc(val, | ||
| vm.ToValue(func(res goja.Value) { | ||
| select { | ||
| case resultChan <- res.Export(): | ||
| case <-timeoutCtx.Done(): | ||
| // Timeout already occurred, ignore result | ||
| } | ||
| }), | ||
| vm.ToValue(func(err goja.Value) { | ||
| var errMsg string | ||
| if err == nil || err == goja.Undefined() { | ||
| errMsg = "unknown error" | ||
| } else { | ||
| // Try to get error message from Error object | ||
| if errObj := err.ToObject(vm); errObj != nil { | ||
| if msg := errObj.Get("message"); msg != nil && msg != goja.Undefined() { | ||
| errMsg = msg.String() | ||
| } else if name := errObj.Get("name"); name != nil && name != goja.Undefined() { | ||
| errMsg = name.String() | ||
| } else { | ||
| errMsg = err.String() | ||
| } | ||
| } else { | ||
| // Fallback to string conversion | ||
| errMsg = err.String() | ||
| } | ||
| } | ||
| select { | ||
| case errChan <- fmt.Errorf("%s", errMsg): | ||
| case <-timeoutCtx.Done(): | ||
| // Timeout already occurred, ignore error | ||
| } | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| executionErr = err | ||
| return | ||
| } | ||
| thread := &starlark.Thread{ | ||
| Name: "codemode", | ||
| Print: func(_ *starlark.Thread, msg string) { | ||
| appendLog(msg) | ||
| }, | ||
| } | ||
|
|
||
| // Wait for result or error with timeout | ||
| select { | ||
| case res := <-resultChan: | ||
| result = res | ||
| case err := <-errChan: | ||
| logger.Debug(fmt.Sprintf("%s Promise rejected: %v", CodeModeLogPrefix, err)) | ||
| executionErr = err | ||
| case <-timeoutCtx.Done(): | ||
| logger.Debug(fmt.Sprintf("%s Promise timeout while waiting for result", CodeModeLogPrefix)) | ||
| executionErr = fmt.Errorf("execution timeout") | ||
| } | ||
| } else { | ||
| result = val.Export() | ||
| } | ||
| } else { | ||
| result = val.Export() | ||
| } | ||
| } else { | ||
| // Not an object (or null/undefined), just export the value | ||
| result = val.Export() | ||
| } | ||
| }() | ||
| // Set up cancellation check | ||
| thread.SetLocal("context", timeoutCtx) | ||
|
|
||
| // Step 5: Execute the code | ||
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SetInterrupt or ErrInterrupted are used elsewhere
rg -n "SetInterrupt|ErrInterrupted" --type goRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Get the full context around lines 345-362 in the target file
wc -l core/mcp/codemodeexecutecode.goRepository: maximhq/bifrost
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Look at the code context - broader range to understand the function
sed -n '330,380p' core/mcp/codemodeexecutecode.go | cat -nRepository: maximhq/bifrost
Length of output: 2048
🏁 Script executed:
#!/bin/bash
# Search for other timeout or context usage patterns in the file
rg -n "WithTimeout|context\.Done|cancellation" core/mcp/codemodeexecutecode.goRepository: maximhq/bifrost
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Search for Starlark imports to verify API
rg -n "go.starlark.io\|\"starlark\"" core/mcp/codemodeexecutecode.go | head -20Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there's any use of SetLocal with "context" elsewhere
rg -n "SetLocal.*context|context.*SetLocal" --type goRepository: maximhq/bifrost
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check git for PR stack/branch info
git log --oneline -10 HEADRepository: maximhq/bifrost
Length of output: 96
🌐 Web query:
Starlark Go SetInterrupt channel timeout
💡 Result:
In current starlark-go (go.starlark.net/starlark), there isn’t a Thread.SetInterrupt API anymore; cancellation is done via (*starlark.Thread).Cancel(...), which is safe to call from another goroutine while the thread is executing. [1]
To enforce a timeout, schedule Cancel:
thread := &starlark.Thread{Name: "main"}
timer := time.AfterFunc(2*time.Second, func() {
thread.Cancel("timeout")
})
defer timer.Stop()
globals, err := starlark.ExecFileOptions(nil, thread, "script.star", nil, predeclared)
if err != nil {
// err will be an *starlark.EvalError with reason "timeout"
}If you want a “budget” style limit rather than wall-clock time, you can also cap execution with thread.SetMaxExecutionSteps(...) (which triggers Cancel("too many steps") by default). [1]
Source: starlark-go Thread.Cancel / SetMaxExecutionSteps docs. [1]
[1] (pkg.go.dev)
Citations:
Timeout context isn't wired to Starlark execution.
Line 347 creates timeoutCtx, but starlark.ExecFile won't observe it; an infinite loop can run unbounded. Use thread.Cancel() with time.AfterFunc() to enforce the timeout instead. Note: The current starlark-go library uses Cancel() for cancellation, not SetInterrupt.
⏱️ Proposed fix to enforce timeout
toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration)
timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout)
defer cancel()
thread := &starlark.Thread{
Name: "codemode",
Print: func(_ *starlark.Thread, msg string) {
appendLog(msg)
},
}
- // Set up cancellation check
- thread.SetLocal("context", timeoutCtx)
+
+ timer := time.AfterFunc(toolExecutionTimeout, func() {
+ thread.Cancel("timeout")
+ })
+ defer timer.Stop()📝 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.
| // Step 4: Create Starlark thread with print function and timeout | |
| toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration) | |
| timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) | |
| defer cancel() | |
| // Try to convert to object to check if it's a promise | |
| // Use recover to safely handle null values that can't be converted to objects | |
| var valObj *goja.Object | |
| func() { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| // Value is null or can't be converted to object, just export it | |
| valObj = nil | |
| } | |
| }() | |
| valObj = val.ToObject(vm) | |
| }() | |
| if valObj != nil { | |
| // Check if it has a 'then' method (Promise-like) | |
| if then := valObj.Get("then"); then != nil && then != goja.Undefined() { | |
| // It's a promise, we need to await it | |
| // Use buffered channels to prevent blocking if handlers are called after timeout | |
| resultChan := make(chan interface{}, 1) | |
| errChan := make(chan error, 1) | |
| // Set up promise handlers | |
| thenFunc, ok := goja.AssertFunction(then) | |
| if ok { | |
| // Call then with resolve and reject handlers | |
| _, err := thenFunc(val, | |
| vm.ToValue(func(res goja.Value) { | |
| select { | |
| case resultChan <- res.Export(): | |
| case <-timeoutCtx.Done(): | |
| // Timeout already occurred, ignore result | |
| } | |
| }), | |
| vm.ToValue(func(err goja.Value) { | |
| var errMsg string | |
| if err == nil || err == goja.Undefined() { | |
| errMsg = "unknown error" | |
| } else { | |
| // Try to get error message from Error object | |
| if errObj := err.ToObject(vm); errObj != nil { | |
| if msg := errObj.Get("message"); msg != nil && msg != goja.Undefined() { | |
| errMsg = msg.String() | |
| } else if name := errObj.Get("name"); name != nil && name != goja.Undefined() { | |
| errMsg = name.String() | |
| } else { | |
| errMsg = err.String() | |
| } | |
| } else { | |
| // Fallback to string conversion | |
| errMsg = err.String() | |
| } | |
| } | |
| select { | |
| case errChan <- fmt.Errorf("%s", errMsg): | |
| case <-timeoutCtx.Done(): | |
| // Timeout already occurred, ignore error | |
| } | |
| }), | |
| ) | |
| if err != nil { | |
| executionErr = err | |
| return | |
| } | |
| thread := &starlark.Thread{ | |
| Name: "codemode", | |
| Print: func(_ *starlark.Thread, msg string) { | |
| appendLog(msg) | |
| }, | |
| } | |
| // Wait for result or error with timeout | |
| select { | |
| case res := <-resultChan: | |
| result = res | |
| case err := <-errChan: | |
| logger.Debug(fmt.Sprintf("%s Promise rejected: %v", CodeModeLogPrefix, err)) | |
| executionErr = err | |
| case <-timeoutCtx.Done(): | |
| logger.Debug(fmt.Sprintf("%s Promise timeout while waiting for result", CodeModeLogPrefix)) | |
| executionErr = fmt.Errorf("execution timeout") | |
| } | |
| } else { | |
| result = val.Export() | |
| } | |
| } else { | |
| result = val.Export() | |
| } | |
| } else { | |
| // Not an object (or null/undefined), just export the value | |
| result = val.Export() | |
| } | |
| }() | |
| // Set up cancellation check | |
| thread.SetLocal("context", timeoutCtx) | |
| // Step 5: Execute the code | |
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) | |
| // Step 4: Create Starlark thread with print function and timeout | |
| toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration) | |
| timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) | |
| defer cancel() | |
| thread := &starlark.Thread{ | |
| Name: "codemode", | |
| Print: func(_ *starlark.Thread, msg string) { | |
| appendLog(msg) | |
| }, | |
| } | |
| timer := time.AfterFunc(toolExecutionTimeout, func() { | |
| thread.Cancel("timeout") | |
| }) | |
| defer timer.Stop() | |
| // Step 5: Execute the code | |
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) |
🤖 Prompt for AI Agents
In `@core/mcp/codemodeexecutecode.go` around lines 345 - 362, The timeout context
created as toolExecutionTimeout/timeoutCtx isn't observed by starlark.ExecFile;
replace context-based cancellation with a timer that calls thread.Cancel() to
enforce the execution timeout: create the starlark.Thread (as currently done),
remove or stop using timeoutCtx/SetLocal, start a
time.AfterFunc(toolExecutionTimeout, func(){ thread.Cancel() }) before calling
starlark.ExecFile, capture the returned timer and stop it (timer.Stop())
immediately after ExecFile returns to avoid leaks, and ensure you still call
defer cancel() or remove unused cancel variables so no unused context remains;
reference symbols: toolExecutionTimeout, timeoutCtx, thread, starlark.ExecFile,
thread.Cancel(), time.AfterFunc.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/mcp/utils.go (1)
150-171: Undefined variables in tool name mapping assignment.Line 170 references
sanitizedToolNameandoriginalMCPNamewhich are not defined in this function. The available variables arevalidationName(the sanitized tool name defined on line 152) andmcpTool.Name(the original MCP tool name). This will cause a compile error.🐛 Proposed fix
// Store the tool with the prefixed name tools[prefixedToolName] = bifrostTool // Store the mapping from sanitized name to original MCP name for later lookup during execution - toolNameMapping[sanitizedToolName] = originalMCPName + toolNameMapping[validationName] = mcpTool.Name }core/mcp/codemodeexecutecode.go (1)
694-702: Critical: Undefined variableoriginalMCPToolNamewill cause compile error.Line 699 references
originalMCPToolNamewhich is never defined in this function. Based on line 543, this should beoriginalToolName.🐛 Fix the undefined variable
mcpResp = &schemas.BifrostMCPResponse{ ChatMessage: createToolResponseMessage(toolCall, rawResult), ExtraFields: schemas.BifrostMCPResponseExtraFields{ ClientName: clientName, - ToolName: originalMCPToolName, + ToolName: originalToolName, Latency: latency, }, }
🤖 Fix all issues with AI agents
In `@core/mcp/codemodeexecutecode.go`:
- Around line 484-489: The map-to-Starlark conversion in goToStarlark currently
ignores the error returned by dict.SetKey which can drop values; update the case
handling in goToStarlark to capture the error from
dict.SetKey(starlark.String(k), goToStarlark(v)) and handle it (e.g., return the
error up the call chain or propagate a wrapped error) instead of discarding
it—ensure goToStarlark’s signature supports returning an error or convert the
function to return (starlark.Value, error) so callers (and any callers of
goToStarlark) are updated accordingly to propagate failures.
- Around line 345-361: The Starlark thread isn't being cancelled when the
timeout context expires because setting the context in thread locals doesn't
enforce cancellation; after creating thread (the variable thread) and
timeoutCtx/toolExecutionTimeout, schedule a time.AfterFunc using
toolExecutionTimeout (or watch timeoutCtx) to call thread.Cancel() when the
timeout fires, keep the returned timer so you can Stop() it after
starlark.ExecFile returns, and still defer cancel() on timeoutCtx; this wires
the cancellation into the Starlark execution and prevents timer leaks.
In `@core/mcp/codemodereadfile.go`:
- Around line 353-358: The generated Python signature in the else branch is
missing the trailing colon; update the code that writes the signature (the
sb.WriteString calls for toolName, params, desc) so that both branches emit a
colon at the end of the signature (i.e., change the else branch signature to
include " -> dict:"). Locate the block that formats the signature using
sb.WriteString and ensure the format string in the branch without desc mirrors
the colon from the branch with desc.
🧹 Nitpick comments (6)
core/mcp/codemodereadfile.go (3)
396-409: Consider usingsort.Stringsfor cleaner sorting.The manual bubble sort works but could be simplified with
sort.Strings. This is a minor readability improvement.♻️ Optional refactor
+import "sort" + // Simple alphabetical sort for each group - for i := 0; i < len(requiredNames)-1; i++ { - for j := i + 1; j < len(requiredNames); j++ { - if requiredNames[i] > requiredNames[j] { - requiredNames[i], requiredNames[j] = requiredNames[j], requiredNames[i] - } - } - } - for i := 0; i < len(optionalNames)-1; i++ { - for j := i + 1; j < len(optionalNames); j++ { - if optionalNames[i] > optionalNames[j] { - optionalNames[i], optionalNames[j] = optionalNames[j], optionalNames[i] - } - } - } + sort.Strings(requiredNames) + sort.Strings(optionalNames)
636-646: Non-deterministic parameter selection in examples.Map iteration order in Go is not guaranteed, so the example parameter selected may vary between runs. Consider sorting parameter names first for consistent output, especially since this affects LLM-facing documentation.
♻️ Optional: Deterministic parameter selection
+ // Sort names for deterministic selection + var names []string + for name := range props { + names = append(names, name) + } + sort.Strings(names) + // Get first required param as example - for name := range props { + for _, name := range names { if required[name] { return fmt.Sprintf("%s=\"...\"", name) } } // If no required, get first param - for name := range props { + if len(names) > 0 { + return fmt.Sprintf("%s=\"...\"", names[0]) + } - return fmt.Sprintf("%s=\"...\"", name) - } return ""
160-164: Inconsistent indentation in error message construction.The error message building appears to have inconsistent indentation relative to the surrounding
ifblock. While this doesn't affect functionality, it reduces readability.core/mcp/codemodeexecutecode_test.go (2)
216-237: Consider using standard library for case-insensitive string matching.The recursive
containsIgnoreCaseimplementation is complex. Standard library functions would be cleaner and more maintainable for test helpers.♻️ Simpler implementation using standard library
+import "strings" + func containsIgnoreCase(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && (containsIgnoreCase(s[1:], substr) || (len(s) >= len(substr) && equalFold(s[:len(substr)], substr)))) + return strings.Contains(strings.ToLower(s), strings.ToLower(substr)) } - -func equalFold(a, b string) bool { - if len(a) != len(b) { - return false - } - for i := 0; i < len(a); i++ { - ca, cb := a[i], b[i] - if ca >= 'A' && ca <= 'Z' { - ca += 'a' - 'A' - } - if cb >= 'A' && cb <= 'Z' { - cb += 'a' - 'A' - } - if ca != cb { - return false - } - } - return true -}
477-488: Truncation test assertion could be more precise.The test logs the result but doesn't make a concrete assertion about the truncation behavior. Consider adding an explicit assertion.
♻️ More explicit truncation assertion
t.Run("Truncate long result", func(t *testing.T) { longString := "" for i := 0; i < 300; i++ { longString += "a" } result := formatResultForLog(longString) - if len(result) > 200 { - // Should be truncated to around 200 chars (plus quotes and ellipsis) - t.Logf("Result length: %d (truncated as expected)", len(result)) + // Should be truncated (original is 300 chars + 2 quotes = 302) + // After truncation, should be around 200 chars + if len(result) >= 300 { + t.Errorf("Expected truncation, but result length is %d", len(result)) } })core/mcp/codemodeexecutecode.go (1)
298-331: Consider adding type assertion safety check for consistency.Line 303 uses a direct type assertion without checking the ok value, while line 313 properly checks it. Although Starlark guarantees kwargs keys are strings, adding the check would be more defensive and consistent:
♻️ Optional: Add defensive type check
for _, kwarg := range kwargs { if len(kwarg) == 2 { - key := string(kwarg[0].(starlark.String)) - value := starlarkToGo(kwarg[1]) - goArgs[key] = value + if keyStr, ok := kwarg[0].(starlark.String); ok { + goArgs[string(keyStr)] = starlarkToGo(kwarg[1]) + } } }
| timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) | ||
| defer cancel() | ||
|
|
||
| // Try to convert to object to check if it's a promise | ||
| // Use recover to safely handle null values that can't be converted to objects | ||
| var valObj *goja.Object | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Value is null or can't be converted to object, just export it | ||
| valObj = nil | ||
| } | ||
| }() | ||
| valObj = val.ToObject(vm) | ||
| }() | ||
|
|
||
| if valObj != nil { | ||
| // Check if it has a 'then' method (Promise-like) | ||
| if then := valObj.Get("then"); then != nil && then != goja.Undefined() { | ||
| // It's a promise, we need to await it | ||
| // Use buffered channels to prevent blocking if handlers are called after timeout | ||
| resultChan := make(chan interface{}, 1) | ||
| errChan := make(chan error, 1) | ||
|
|
||
| // Set up promise handlers | ||
| thenFunc, ok := goja.AssertFunction(then) | ||
| if ok { | ||
| // Call then with resolve and reject handlers | ||
| _, err := thenFunc(val, | ||
| vm.ToValue(func(res goja.Value) { | ||
| select { | ||
| case resultChan <- res.Export(): | ||
| case <-timeoutCtx.Done(): | ||
| // Timeout already occurred, ignore result | ||
| } | ||
| }), | ||
| vm.ToValue(func(err goja.Value) { | ||
| var errMsg string | ||
| if err == nil || err == goja.Undefined() { | ||
| errMsg = "unknown error" | ||
| } else { | ||
| // Try to get error message from Error object | ||
| if errObj := err.ToObject(vm); errObj != nil { | ||
| if msg := errObj.Get("message"); msg != nil && msg != goja.Undefined() { | ||
| errMsg = msg.String() | ||
| } else if name := errObj.Get("name"); name != nil && name != goja.Undefined() { | ||
| errMsg = name.String() | ||
| } else { | ||
| errMsg = err.String() | ||
| } | ||
| } else { | ||
| // Fallback to string conversion | ||
| errMsg = err.String() | ||
| } | ||
| } | ||
| select { | ||
| case errChan <- fmt.Errorf("%s", errMsg): | ||
| case <-timeoutCtx.Done(): | ||
| // Timeout already occurred, ignore error | ||
| } | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| executionErr = err | ||
| return | ||
| } | ||
| thread := &starlark.Thread{ | ||
| Name: "codemode", | ||
| Print: func(_ *starlark.Thread, msg string) { | ||
| appendLog(msg) | ||
| }, | ||
| } | ||
|
|
||
| // Wait for result or error with timeout | ||
| select { | ||
| case res := <-resultChan: | ||
| result = res | ||
| case err := <-errChan: | ||
| logger.Debug(fmt.Sprintf("%s Promise rejected: %v", CodeModeLogPrefix, err)) | ||
| executionErr = err | ||
| case <-timeoutCtx.Done(): | ||
| logger.Debug(fmt.Sprintf("%s Promise timeout while waiting for result", CodeModeLogPrefix)) | ||
| executionErr = fmt.Errorf("execution timeout") | ||
| } | ||
| } else { | ||
| result = val.Export() | ||
| } | ||
| } else { | ||
| result = val.Export() | ||
| } | ||
| } else { | ||
| // Not an object (or null/undefined), just export the value | ||
| result = val.Export() | ||
| } | ||
| }() | ||
| // Set up cancellation check | ||
| thread.SetLocal("context", timeoutCtx) | ||
|
|
||
| // Step 5: Execute the code | ||
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if there's a stack of PRs by looking at common stack metadata files
find . -type f -name "*.md" -o -name "*.txt" | head -20Repository: maximhq/bifrost
Length of output: 828
🏁 Script executed:
# Check the full executeCode function to understand the timeout context usage
fd -e go -x grep -l "executeCode" {} \;Repository: maximhq/bifrost
Length of output: 184
🏁 Script executed:
# Look at the specific file and context around the lines mentioned
head -n 450 core/mcp/codemodeexecutecode.go | tail -n 120Repository: maximhq/bifrost
Length of output: 3435
🏁 Script executed:
# Check starlark imports to understand version and available methods
rg "starlark" core/mcp/codemodeexecutecode.go -A 2 -B 2Repository: maximhq/bifrost
Length of output: 4387
🏁 Script executed:
# Check starlark.Thread method signatures and Cancel availability
rg "Thread\.(Cancel|SetLocal)" --type go -A 3 -B 1Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Search if timeoutCtx is ever read/used elsewhere in the executeCode function
rg "context\|timeoutCtx\|GetLocal" core/mcp/codemodeexecutecode.go -B 2 -A 2Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check starlark library documentation or any usage patterns for timeout/cancellation
rg "starlark\.ExecFile\|ExecuteFile" --type go -B 5 -A 5Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Look for any goroutine cancellation patterns with starlark threads
rg "thread\.Cancel\|Done\(\)" core/mcp/codemodeexecutecode.go -B 3 -A 3Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check if there's any PR stack context or other related changes
git log --oneline -20 -- core/mcp/codemodeexecutecode.go 2>/dev/null || echo "No git history available"Repository: maximhq/bifrost
Length of output: 96
🏁 Script executed:
# Search for any starlark timeout/cancel related code more broadly
rg "starlark" core/mcp/codemodeexecutecode.go | head -20Repository: maximhq/bifrost
Length of output: 1181
🏁 Script executed:
# Check if go.mod references starlark version
rg "go.starlark.net" . --type modRepository: maximhq/bifrost
Length of output: 86
🏁 Script executed:
# Let's examine the full executeCode function to understand context flow
sed -n '250,410p' core/mcp/codemodeexecutecode.goRepository: maximhq/bifrost
Length of output: 5387
🏁 Script executed:
# Search for Thread usage patterns in the codebase
rg "starlark\.Thread\{" --type go -A 10Repository: maximhq/bifrost
Length of output: 634
🏁 Script executed:
# Check if there are any callback or pre/post exec hooks
rg "ExecFile\|Thread\|Cancel" core/mcp/ --type go -B 2 -A 2 | head -50Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check go.mod for starlark version
find . -name "go.mod" -exec cat {} \; | grep starlarkRepository: maximhq/bifrost
Length of output: 110
🏁 Script executed:
# Search starlark documentation or type definitions in vendor if available
fd -e go starlark | xargs grep -l "Thread" | head -5Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Let's check if starlark has Cancel in its API - search for common timeout patterns
python3 << 'EOF'
import subprocess
import json
# Try to get starlark.net/starlark documentation via web search equivalent
# For now, check if there are any hints in error messages or test files
result = subprocess.run(['rg', 'starlark.*Thread', '-A', '5', '--type', 'go'],
capture_output=True, text=True)
print("Starlark Thread patterns:")
print(result.stdout[:2000])
EOFRepository: maximhq/bifrost
Length of output: 145
🏁 Script executed:
# Check if the tool invocation in Starlark callbacks has any timeout handling
sed -n '300,340p' core/mcp/codemodeexecutecode.goRepository: maximhq/bifrost
Length of output: 1423
🏁 Script executed:
# Search for how callMCPTool is implemented to see if timeout is enforced there
rg "func.*callMCPTool" core/mcp/ -A 20Repository: maximhq/bifrost
Length of output: 3030
🌐 Web query:
go.starlark.net Thread Cancel method API timeout
💡 Result:
In go.starlark.net/starlark, the Thread cancellation API is:
func (thread *Thread) Cancel(reason string)— causes Starlark execution in that thread to fail with anEvalErrorcontainingreason. It’s safe to call from any goroutine, but cancellation may be observed with a delay if the interpreter is currently inside a built-in function. Callthread.Uncancel()to reset the cancellation state. [1]func (thread *Thread) SetMaxExecutionSteps(max uint64)— sets a step limit; when exceeded, the interpreter callsthread.OnMaxStepsif set, otherwise it callsthread.Cancel("too many steps"). [1]
Implementing a wall-clock timeout
A common pattern is to start a timer that calls Cancel, then stop it if evaluation finishes first:
thread := &starlark.Thread{Name: "main"}
timer := time.AfterFunc(2*time.Second, func() {
thread.Cancel("timeout")
})
defer timer.Stop()
globals, err := starlark.ExecFile(thread, "script.star", src, predeclared)
// err will be an *starlark.EvalError if cancelledThis works because Cancel is explicitly goroutine-safe. [1]
Source: go.starlark.net/starlark API docs on pkg.go.dev. [1]
References:
[1] pkg.go.dev — go.starlark.net/starlark (Thread.Cancel, Thread.SetMaxExecutionSteps, Thread.Uncancel) (pkg.go.dev)
Citations:
🌐 Web query:
starlark.Thread API documentation methods
💡 Result:
In the Go Starlark implementation (go.starlark.net/starlark), starlark.Thread has these documented methods: (pkg.go.dev)
CallFrame(depth int) CallFrameCallStack() CallStackCallStackDepth() intCancel(reason string)DebugFrame(depth int) DebugFrameExecutionSteps() uint64Local(key string) interface{}SetLocal(key string, value interface{})SetMaxExecutionSteps(max uint64)Uncancel()
Citations:
🏁 Script executed:
# Let's check if there's any usage of context.Done() or similar patterns in tool callbacks
rg "context\.Done\|Select\|context\." core/mcp/codemodeexecutecode.go -B 2 -A 2Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check the actual starlark package to understand what methods Thread has
python3 << 'EOF'
# Let's look at the actual behavior - search for any timeout-related comments or patterns
import subprocess
# Search for documentation or hints about starlark thread cancellation
result = subprocess.run(['rg', 'Cancel|Done|context|timeout', 'core/mcp/codemodeexecutecode.go', '-i'],
capture_output=True, text=True)
lines = result.stdout.split('\n')
for line in lines[:40]:
if 'timeout' in line.lower() or 'cancel' in line.lower() or 'context' in line.lower():
print(line)
EOFRepository: maximhq/bifrost
Length of output: 145
Starlark execution may not respect timeout — cancellation not wired to thread.
The timeout context is created (line 347) but storing it in thread local (line 358) doesn't enforce timeout on Starlark script execution. The starlark.Thread.SetLocal() method only makes the context available to user code; Starlark doesn't automatically check context cancellation. Long-running scripts could run indefinitely despite the timeout.
To properly enforce timeouts, use time.AfterFunc() to call thread.Cancel() when the context times out, following the documented Starlark pattern:
🔧 Proposed fix to wire timeout cancellation
// Step 4: Create Starlark thread with print function and timeout
toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration)
timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout)
defer cancel()
thread := &starlark.Thread{
Name: "codemode",
Print: func(_ *starlark.Thread, msg string) {
appendLog(msg)
},
}
-// Set up cancellation check
-thread.SetLocal("context", timeoutCtx)
+// Set up timeout cancellation - cancel thread when timeout expires
+timer := time.AfterFunc(toolExecutionTimeout, func() {
+ thread.Cancel("execution timeout")
+})
+defer timer.Stop()📝 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.
| // Step 4: Create Starlark thread with print function and timeout | |
| toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration) | |
| timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) | |
| defer cancel() | |
| // Try to convert to object to check if it's a promise | |
| // Use recover to safely handle null values that can't be converted to objects | |
| var valObj *goja.Object | |
| func() { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| // Value is null or can't be converted to object, just export it | |
| valObj = nil | |
| } | |
| }() | |
| valObj = val.ToObject(vm) | |
| }() | |
| if valObj != nil { | |
| // Check if it has a 'then' method (Promise-like) | |
| if then := valObj.Get("then"); then != nil && then != goja.Undefined() { | |
| // It's a promise, we need to await it | |
| // Use buffered channels to prevent blocking if handlers are called after timeout | |
| resultChan := make(chan interface{}, 1) | |
| errChan := make(chan error, 1) | |
| // Set up promise handlers | |
| thenFunc, ok := goja.AssertFunction(then) | |
| if ok { | |
| // Call then with resolve and reject handlers | |
| _, err := thenFunc(val, | |
| vm.ToValue(func(res goja.Value) { | |
| select { | |
| case resultChan <- res.Export(): | |
| case <-timeoutCtx.Done(): | |
| // Timeout already occurred, ignore result | |
| } | |
| }), | |
| vm.ToValue(func(err goja.Value) { | |
| var errMsg string | |
| if err == nil || err == goja.Undefined() { | |
| errMsg = "unknown error" | |
| } else { | |
| // Try to get error message from Error object | |
| if errObj := err.ToObject(vm); errObj != nil { | |
| if msg := errObj.Get("message"); msg != nil && msg != goja.Undefined() { | |
| errMsg = msg.String() | |
| } else if name := errObj.Get("name"); name != nil && name != goja.Undefined() { | |
| errMsg = name.String() | |
| } else { | |
| errMsg = err.String() | |
| } | |
| } else { | |
| // Fallback to string conversion | |
| errMsg = err.String() | |
| } | |
| } | |
| select { | |
| case errChan <- fmt.Errorf("%s", errMsg): | |
| case <-timeoutCtx.Done(): | |
| // Timeout already occurred, ignore error | |
| } | |
| }), | |
| ) | |
| if err != nil { | |
| executionErr = err | |
| return | |
| } | |
| thread := &starlark.Thread{ | |
| Name: "codemode", | |
| Print: func(_ *starlark.Thread, msg string) { | |
| appendLog(msg) | |
| }, | |
| } | |
| // Wait for result or error with timeout | |
| select { | |
| case res := <-resultChan: | |
| result = res | |
| case err := <-errChan: | |
| logger.Debug(fmt.Sprintf("%s Promise rejected: %v", CodeModeLogPrefix, err)) | |
| executionErr = err | |
| case <-timeoutCtx.Done(): | |
| logger.Debug(fmt.Sprintf("%s Promise timeout while waiting for result", CodeModeLogPrefix)) | |
| executionErr = fmt.Errorf("execution timeout") | |
| } | |
| } else { | |
| result = val.Export() | |
| } | |
| } else { | |
| result = val.Export() | |
| } | |
| } else { | |
| // Not an object (or null/undefined), just export the value | |
| result = val.Export() | |
| } | |
| }() | |
| // Set up cancellation check | |
| thread.SetLocal("context", timeoutCtx) | |
| // Step 5: Execute the code | |
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) | |
| // Step 4: Create Starlark thread with print function and timeout | |
| toolExecutionTimeout := m.toolExecutionTimeout.Load().(time.Duration) | |
| timeoutCtx, cancel := context.WithTimeout(ctx, toolExecutionTimeout) | |
| defer cancel() | |
| thread := &starlark.Thread{ | |
| Name: "codemode", | |
| Print: func(_ *starlark.Thread, msg string) { | |
| appendLog(msg) | |
| }, | |
| } | |
| // Set up timeout cancellation - cancel thread when timeout expires | |
| timer := time.AfterFunc(toolExecutionTimeout, func() { | |
| thread.Cancel("execution timeout") | |
| }) | |
| defer timer.Stop() | |
| // Step 5: Execute the code | |
| globals, err := starlark.ExecFile(thread, "code.star", trimmedCode, predeclared) |
🤖 Prompt for AI Agents
In `@core/mcp/codemodeexecutecode.go` around lines 345 - 361, The Starlark thread
isn't being cancelled when the timeout context expires because setting the
context in thread locals doesn't enforce cancellation; after creating thread
(the variable thread) and timeoutCtx/toolExecutionTimeout, schedule a
time.AfterFunc using toolExecutionTimeout (or watch timeoutCtx) to call
thread.Cancel() when the timeout fires, keep the returned timer so you can
Stop() it after starlark.ExecFile returns, and still defer cancel() on
timeoutCtx; this wires the cancellation into the Starlark execution and prevents
timer leaks.
| case map[string]interface{}: | ||
| dict := starlark.NewDict(len(val)) | ||
| for k, v := range val { | ||
| dict.SetKey(starlark.String(k), goToStarlark(v)) | ||
| } | ||
| return dict |
There was a problem hiding this comment.
Ignoring error from dict.SetKey could silently drop values.
starlark.Dict.SetKey returns an error (e.g., for frozen dicts or hash collisions). While unlikely in this context, ignoring it could lead to silently incomplete conversions.
🔧 Proposed fix
case map[string]interface{}:
dict := starlark.NewDict(len(val))
for k, v := range val {
- dict.SetKey(starlark.String(k), goToStarlark(v))
+ if err := dict.SetKey(starlark.String(k), goToStarlark(v)); err != nil {
+ // Log but continue - this shouldn't happen with fresh dicts
+ logger.Debug(fmt.Sprintf("%s Failed to set dict key %s: %v", CodeModeLogPrefix, k, err))
+ }
}
return dict🤖 Prompt for AI Agents
In `@core/mcp/codemodeexecutecode.go` around lines 484 - 489, The map-to-Starlark
conversion in goToStarlark currently ignores the error returned by dict.SetKey
which can drop values; update the case handling in goToStarlark to capture the
error from dict.SetKey(starlark.String(k), goToStarlark(v)) and handle it (e.g.,
return the error up the call chain or propagate a wrapped error) instead of
discarding it—ensure goToStarlark’s signature supports returning an error or
convert the function to return (starlark.Value, error) so callers (and any
callers of goToStarlark) are updated accordingly to propagate failures.
| // Write Python signature: def tool_name(param: type, param: type = None) -> dict: # description | ||
| if desc != "" { | ||
| sb.WriteString(fmt.Sprintf("def %s(%s) -> dict: # %s\n", toolName, params, desc)) | ||
| } else { | ||
| sb.WriteString(fmt.Sprintf("def %s(%s) -> dict\n", toolName, params)) | ||
| } |
There was a problem hiding this comment.
Missing colon in function signature syntax.
When there's no description, the generated signature is def tool_name(params) -> dict without a trailing colon, which is invalid Python syntax. The colon should be present regardless of whether there's a comment.
🐛 Proposed fix
// Write Python signature: def tool_name(param: type, param: type = None) -> dict: # description
if desc != "" {
sb.WriteString(fmt.Sprintf("def %s(%s) -> dict: # %s\n", toolName, params, desc))
} else {
- sb.WriteString(fmt.Sprintf("def %s(%s) -> dict\n", toolName, params))
+ sb.WriteString(fmt.Sprintf("def %s(%s) -> dict:\n", toolName, params))
}🤖 Prompt for AI Agents
In `@core/mcp/codemodereadfile.go` around lines 353 - 358, The generated Python
signature in the else branch is missing the trailing colon; update the code that
writes the signature (the sb.WriteString calls for toolName, params, desc) so
that both branches emit a colon at the end of the signature (i.e., change the
else branch signature to include " -> dict:"). Locate the block that formats the
signature using sb.WriteString and ensure the format string in the branch
without desc mirrors the colon from the branch with desc.
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
44c643f to
9d20b32
Compare
akshaydeo
force-pushed
the
01-26-code_mode_compact_signatures
branch
2 times, most recently
from
0781a36 to
db30ffa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/internal/mcptests/agent_request_id_test.go (1)
20-38: Guardstarlark.SetLogger()from concurrent mutation in parallel tests.
starlark.SetLogger()directly assigns to an unsynchronized package-level variable without any locking mechanism. Since multiple tests in this file run witht.Parallel()and each callssetupMCPManagerWithRequestIDFunc(), concurrent writes to the sharedloggervariable create a data race. Usesync.Onceper test or synchronize access with a mutex to prevent cross-test logger overrides and race detector violations.
🤖 Fix all issues with AI agents
In `@core/mcp/codemode/starlark/getdocs.go`:
- Around line 150-156: The header currently uses tools[0].Function.Name which
already contains the client prefix and can produce duplicated names; update the
block that writes the header (the isToolLevel branch in getdocs.go) to derive an
unprefixed tool name (e.g., compute unprefixedName by removing the clientName +
"." prefix if present or by splitting on '.' and taking the last segment) and
use that in the fmt.Sprintf call instead of tools[0].Function.Name; reference
variables/functions: isToolLevel, tools, tools[0].Function.Name, clientName, and
the sb.WriteString(fmt.Sprintf(...)) line to locate and replace the value.
In `@core/mcp/codemode/starlark/utils.go`:
- Around line 343-355: The validateNormalizedToolName function currently only
checks for '/' and '..' but misses Windows backslash separators; update
validateNormalizedToolName to also reject '\' and any occurrences of
filepath.Separator (or explicitly check for '\\'), and optionally compare
normalizedName to filepath.Clean(normalizedName) to detect traversal constructs;
ensure error messages reference the offending input and use the function name
validateNormalizedToolName in the log/return to make the check explicit.
In `@core/mcp/utils.go`:
- Around line 151-172: The current logic constructs prefixedToolName using the
original MCP name (with hyphens) and stores a mapping from sanitizedToolName to
the original name, which breaks the repo invariant that tool identifiers are
sanitized (underscores) end-to-end; change the code to build and use a sanitized
prefixed name by replacing hyphens with underscores on mcpTool.Name before
prefixing (use the same sanitized string for bifrostTool.Function.Name when
non-nil), store tools using that sanitized prefixed name, and update
toolNameMapping to map the sanitized prefixed name (or sanitized base name) to
the original MCP name so lookups and allow-lists that expect underscores will
work; adjust references around convertMCPToolToBifrostSchema,
bifrostTool.Function.Name, prefixedToolName, sanitizedToolName, and
toolNameMapping accordingly.
🧹 Nitpick comments (2)
core/mcp/toolmanager.go (2)
36-38: Consider usingatomic.Valuefor thecodeModefield for thread-safety consistency.The
toolExecutionTimeoutandmaxAgentDepthfields use atomic types, butcodeModeis a plain interface field. IfSetCodeModeis called while other goroutines read viaGetAvailableTools,executeToolInternal,UpdateConfig, orGetCodeModeBindingLevel, this could cause a data race.If
SetCodeModeis guaranteed to be called only during single-threaded initialization, consider documenting this constraint. Otherwise, wrapping inatomic.Valuewould provide consistency with other fields.
638-645: CodeMode config update condition may miss timeout-only updates.The update is conditional on
config.CodeModeBindingLevel != "". If onlyToolExecutionTimeoutchanges whileCodeModeBindingLevelis empty, the CodeMode won't receive the updated timeout. Consider whether this is the intended behavior.♻️ Alternative approach to update CodeMode for timeout changes
- if m.codeMode != nil && config.CodeModeBindingLevel != "" { + if m.codeMode != nil && (config.CodeModeBindingLevel != "" || config.ToolExecutionTimeout > 0) { m.codeMode.UpdateConfig(&CodeModeConfig{ BindingLevel: config.CodeModeBindingLevel, ToolExecutionTimeout: config.ToolExecutionTimeout, }) }
| // Write comprehensive header | ||
| sb.WriteString("# ============================================================================\n") | ||
| if isToolLevel && len(tools) == 1 && tools[0].Function != nil { | ||
| sb.WriteString(fmt.Sprintf("# Documentation for %s.%s tool\n", clientName, tools[0].Function.Name)) | ||
| } else { | ||
| sb.WriteString(fmt.Sprintf("# Documentation for %s MCP server\n", clientName)) | ||
| } |
There was a problem hiding this comment.
Header uses prefixed tool name; can duplicate server name.
tools[0].Function.Name already includes the client prefix, so the header can render as server.server-tool. Use the unprefixed name for display.
🔧 Proposed fix
- sb.WriteString(fmt.Sprintf("# Documentation for %s.%s tool\n", clientName, tools[0].Function.Name))
+ unprefixedName := stripClientPrefix(tools[0].Function.Name, clientName)
+ unprefixedName = strings.ReplaceAll(unprefixedName, "-", "_")
+ sb.WriteString(fmt.Sprintf("# Documentation for %s.%s tool\n", clientName, unprefixedName))🤖 Prompt for AI Agents
In `@core/mcp/codemode/starlark/getdocs.go` around lines 150 - 156, The header
currently uses tools[0].Function.Name which already contains the client prefix
and can produce duplicated names; update the block that writes the header (the
isToolLevel branch in getdocs.go) to derive an unprefixed tool name (e.g.,
compute unprefixedName by removing the clientName + "." prefix if present or by
splitting on '.' and taking the last segment) and use that in the fmt.Sprintf
call instead of tools[0].Function.Name; reference variables/functions:
isToolLevel, tools, tools[0].Function.Name, clientName, and the
sb.WriteString(fmt.Sprintf(...)) line to locate and replace the value.
| // validateNormalizedToolName validates a normalized tool name to prevent path traversal. | ||
| func validateNormalizedToolName(normalizedName string) error { | ||
| if normalizedName == "" { | ||
| return fmt.Errorf("tool name cannot be empty after normalization") | ||
| } | ||
| if strings.Contains(normalizedName, "/") { | ||
| return fmt.Errorf("tool name cannot contain '/' (path separator) after normalization: %s", normalizedName) | ||
| } | ||
| if strings.Contains(normalizedName, "..") { | ||
| return fmt.Errorf("tool name cannot contain '..' (path traversal) after normalization: %s", normalizedName) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Harden path traversal guard for Windows separators.
validateNormalizedToolName blocks / and .. but allows \, which can bypass checks on Windows. Consider rejecting both separators (and optionally validating against filepath.Clean).
🔒 Proposed hardening
- if strings.Contains(normalizedName, "/") {
- return fmt.Errorf("tool name cannot contain '/' (path separator) after normalization: %s", normalizedName)
- }
+ if strings.ContainsAny(normalizedName, `/\`) {
+ return fmt.Errorf("tool name cannot contain path separators after normalization: %s", normalizedName)
+ }📝 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.
| // validateNormalizedToolName validates a normalized tool name to prevent path traversal. | |
| func validateNormalizedToolName(normalizedName string) error { | |
| if normalizedName == "" { | |
| return fmt.Errorf("tool name cannot be empty after normalization") | |
| } | |
| if strings.Contains(normalizedName, "/") { | |
| return fmt.Errorf("tool name cannot contain '/' (path separator) after normalization: %s", normalizedName) | |
| } | |
| if strings.Contains(normalizedName, "..") { | |
| return fmt.Errorf("tool name cannot contain '..' (path traversal) after normalization: %s", normalizedName) | |
| } | |
| return nil | |
| } | |
| // validateNormalizedToolName validates a normalized tool name to prevent path traversal. | |
| func validateNormalizedToolName(normalizedName string) error { | |
| if normalizedName == "" { | |
| return fmt.Errorf("tool name cannot be empty after normalization") | |
| } | |
| if strings.ContainsAny(normalizedName, "/\\") { | |
| return fmt.Errorf("tool name cannot contain path separators after normalization: %s", normalizedName) | |
| } | |
| if strings.Contains(normalizedName, "..") { | |
| return fmt.Errorf("tool name cannot contain '..' (path traversal) after normalization: %s", normalizedName) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@core/mcp/codemode/starlark/utils.go` around lines 343 - 355, The
validateNormalizedToolName function currently only checks for '/' and '..' but
misses Windows backslash separators; update validateNormalizedToolName to also
reject '\' and any occurrences of filepath.Separator (or explicitly check for
'\\'), and optionally compare normalizedName to filepath.Clean(normalizedName)
to detect traversal constructs; ensure error messages reference the offending
input and use the function name validateNormalizedToolName in the log/return to
make the check explicit.
| // Validate the original tool name (with hyphens replaced by underscores for validation only) | ||
| validationName := strings.ReplaceAll(mcpTool.Name, "-", "_") | ||
| if err := validateNormalizedToolName(validationName); err != nil { | ||
| logger.Warn(fmt.Sprintf("%s Skipping MCP tool %q: %v", MCPLogPrefix, mcpTool.Name, err)) | ||
| continue | ||
| } | ||
|
|
||
| // Convert MCP tool schema to Bifrost format | ||
| bifrostTool := convertMCPToolToBifrostSchema(&mcpTool) | ||
| // Prefix tool name with client name to make it permanent (using '-' as separator) | ||
| prefixedToolName := fmt.Sprintf("%s-%s", clientName, sanitizedToolName) | ||
| // Keep the original tool name (don't sanitize) so we can call the MCP server correctly | ||
| prefixedToolName := fmt.Sprintf("%s-%s", clientName, mcpTool.Name) | ||
| // Update the tool's function name to match the prefixed name | ||
| if bifrostTool.Function != nil { | ||
| bifrostTool.Function.Name = prefixedToolName | ||
| } | ||
| // Store the tool with the prefixed name | ||
| tools[prefixedToolName] = bifrostTool | ||
| // Store the mapping from sanitized name to original MCP name for later lookup during execution | ||
| toolNameMapping[sanitizedToolName] = originalMCPName | ||
| sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_") | ||
| toolNameMapping[sanitizedToolName] = mcpTool.Name | ||
| } |
There was a problem hiding this comment.
Keep MCP tool names sanitized to avoid lookup/execution mismatches.
Using original MCP names (with hyphens) in the prefixed tool name and storing a reverse mapping conflicts with the repo’s sanitized-name invariant, which can break tool filtering/allow-lists and call routing that expect underscores. Prefer keeping sanitized names end-to-end. Based on learnings, tool names should remain sanitized throughout.
🔧 Proposed fix
- // Validate the original tool name (with hyphens replaced by underscores for validation only)
- validationName := strings.ReplaceAll(mcpTool.Name, "-", "_")
- if err := validateNormalizedToolName(validationName); err != nil {
+ // Validate the sanitized tool name (hyphens replaced by underscores)
+ sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_")
+ if err := validateNormalizedToolName(sanitizedToolName); err != nil {
logger.Warn(fmt.Sprintf("%s Skipping MCP tool %q: %v", MCPLogPrefix, mcpTool.Name, err))
continue
}
@@
- // Prefix tool name with client name to make it permanent (using '-' as separator)
- // Keep the original tool name (don't sanitize) so we can call the MCP server correctly
- prefixedToolName := fmt.Sprintf("%s-%s", clientName, mcpTool.Name)
+ // Prefix tool name with client name to make it permanent (using '-' as separator)
+ // Use sanitized tool name to keep lookups consistent across MCP flows
+ prefixedToolName := fmt.Sprintf("%s-%s", clientName, sanitizedToolName)
@@
- // Store the mapping from sanitized name to original MCP name for later lookup during execution
- sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_")
- toolNameMapping[sanitizedToolName] = mcpTool.Name
+ // Store sanitized name mapping (identity) for lookup compatibility
+ toolNameMapping[sanitizedToolName] = sanitizedToolName📝 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.
| // Validate the original tool name (with hyphens replaced by underscores for validation only) | |
| validationName := strings.ReplaceAll(mcpTool.Name, "-", "_") | |
| if err := validateNormalizedToolName(validationName); err != nil { | |
| logger.Warn(fmt.Sprintf("%s Skipping MCP tool %q: %v", MCPLogPrefix, mcpTool.Name, err)) | |
| continue | |
| } | |
| // Convert MCP tool schema to Bifrost format | |
| bifrostTool := convertMCPToolToBifrostSchema(&mcpTool) | |
| // Prefix tool name with client name to make it permanent (using '-' as separator) | |
| prefixedToolName := fmt.Sprintf("%s-%s", clientName, sanitizedToolName) | |
| // Keep the original tool name (don't sanitize) so we can call the MCP server correctly | |
| prefixedToolName := fmt.Sprintf("%s-%s", clientName, mcpTool.Name) | |
| // Update the tool's function name to match the prefixed name | |
| if bifrostTool.Function != nil { | |
| bifrostTool.Function.Name = prefixedToolName | |
| } | |
| // Store the tool with the prefixed name | |
| tools[prefixedToolName] = bifrostTool | |
| // Store the mapping from sanitized name to original MCP name for later lookup during execution | |
| toolNameMapping[sanitizedToolName] = originalMCPName | |
| sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_") | |
| toolNameMapping[sanitizedToolName] = mcpTool.Name | |
| } | |
| // Validate the sanitized tool name (hyphens replaced by underscores) | |
| sanitizedToolName := strings.ReplaceAll(mcpTool.Name, "-", "_") | |
| if err := validateNormalizedToolName(sanitizedToolName); err != nil { | |
| logger.Warn(fmt.Sprintf("%s Skipping MCP tool %q: %v", MCPLogPrefix, mcpTool.Name, err)) | |
| continue | |
| } | |
| // Convert MCP tool schema to Bifrost format | |
| bifrostTool := convertMCPToolToBifrostSchema(&mcpTool) | |
| // Prefix tool name with client name to make it permanent (using '-' as separator) | |
| // Use sanitized tool name to keep lookups consistent across MCP flows | |
| prefixedToolName := fmt.Sprintf("%s-%s", clientName, sanitizedToolName) | |
| // Update the tool's function name to match the prefixed name | |
| if bifrostTool.Function != nil { | |
| bifrostTool.Function.Name = prefixedToolName | |
| } | |
| // Store the tool with the prefixed name | |
| tools[prefixedToolName] = bifrostTool | |
| // Store sanitized name mapping (identity) for lookup compatibility | |
| toolNameMapping[sanitizedToolName] = sanitizedToolName |
🤖 Prompt for AI Agents
In `@core/mcp/utils.go` around lines 151 - 172, The current logic constructs
prefixedToolName using the original MCP name (with hyphens) and stores a mapping
from sanitizedToolName to the original name, which breaks the repo invariant
that tool identifiers are sanitized (underscores) end-to-end; change the code to
build and use a sanitized prefixed name by replacing hyphens with underscores on
mcpTool.Name before prefixing (use the same sanitized string for
bifrostTool.Function.Name when non-nil), store tools using that sanitized
prefixed name, and update toolNameMapping to map the sanitized prefixed name (or
sanitized base name) to the original MCP name so lookups and allow-lists that
expect underscores will work; adjust references around
convertMCPToolToBifrostSchema, bifrostTool.Function.Name, prefixedToolName,
sanitizedToolName, and toolNameMapping accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/mcp/codemode/starlark/utils.go`:
- Around line 99-104: The call to starlark.Dict.SetKey in the
map[string]interface{} case ignores its returned error; update the block around
starlark.NewDict and goToStarlark to capture the error from dict.SetKey (e.g.,
err := dict.SetKey(starlark.String(k), goToStarlark(v))) and handle it
appropriately for this package—either return the error to the caller, log and
continue, or panic if freezing should never occur; adjust the surrounding
function signature/return path if needed so that goToStarlark (or its caller)
can propagate or react to SetKey failures.
In `@framework/oauth2/main.go`:
- Around line 599-621: The parsed token fields (e.g., tokenResponse.AccessToken,
tokenResponse.RefreshToken, tokenResponse.TokenType, tokenResponse.Scope) must
be normalized by trimming whitespace before validation so whitespace-only values
don't pass; after parsing (both JSON and form branches) call strings.TrimSpace
on those fields and assign the trimmed values back to tokenResponse, then
perform the existing AccessToken empty-check (and any other validations) — also
ensure downstream callers like GetAccessToken and RefreshAccessToken consume
trimmed values by keeping the normalized tokenResponse values.
🧹 Nitpick comments (7)
core/mcp/utils.go (1)
510-522: Comment updated but regex pattern may need review for Starlark.The comment now references "Python/Starlark code" but the regex pattern still matches
awaitkeyword which doesn't exist in Starlark. While the pattern will still match non-await calls correctly (sinceawaitis optional), the comment about "Optional await keyword" is misleading for Starlark.Consider updating the comment to clarify backward compatibility
-// extractToolCallsFromCode extracts tool calls from Python/Starlark code -// Tool calls are in the format: server_name.tool_name(...) +// extractToolCallsFromCode extracts tool calls from Python/Starlark code +// Tool calls are in the format: server_name.tool_name(...) +// Note: The regex still accepts optional "await" prefix for backward compatibility +// with any existing TypeScript code, though Starlark doesn't use await. func extractToolCallsFromCode(code string) ([]toolCallInfo, error) {core/mcp/codemode/starlark/starlark_test.go (1)
218-239: Helper functions work butcontainsIgnoreCaseis inefficient.The recursive implementation of
containsIgnoreCasehas O(n²) complexity due to creating substrings on each recursive call. This is acceptable for test code with small strings, but could be slow for large inputs.Consider using strings.Contains with strings.ToLower for simplicity
func containsIgnoreCase(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && (containsIgnoreCase(s[1:], substr) || (len(s) >= len(substr) && equalFold(s[:len(substr)], substr)))) + return strings.Contains(strings.ToLower(s), strings.ToLower(substr)) }This would require adding
"strings"to the imports.core/internal/mcptests/agent_request_id_test.go (1)
3-14: Preferbifrost.Ptr(...)for pointer creation.
Keeps pointer creation consistent with repository conventions.♻️ Suggested change
import ( "context" "fmt" "sync" "testing" + "github.com/maximhq/bifrost/core/bifrost" "github.com/maximhq/bifrost/core/mcp" "github.com/maximhq/bifrost/core/mcp/codemode/starlark" "github.com/maximhq/bifrost/core/schemas" @@ - logger := &testLogger{t: t} + logger := bifrost.Ptr(testLogger{t: t})Based on learnings, prefer the Ptr helper over the address operator for pointer creation.
Also applies to: 24-37
core/bifrost.go (1)
273-281: Use the Ptr helper forCodeModeConfigcreation.
Aligns with the repository’s pointer creation convention.♻️ Suggested change
- codeModeConfig = &mcp.CodeModeConfig{ + codeModeConfig = Ptr(mcp.CodeModeConfig{ BindingLevel: mcpConfig.ToolManagerConfig.CodeModeBindingLevel, ToolExecutionTimeout: mcpConfig.ToolManagerConfig.ToolExecutionTimeout, - } + })Based on learnings, prefer Ptr over
&for pointer creation.core/mcp/codemode/starlark/starlark.go (1)
74-81: Note:LogMutexfrom dependencies is unused.The
CodeModeDependenciesstruct includes aLogMutexfield, butSetDependenciesdoesn't use it—instead, the struct maintains its own internallogMu. This is fine if the mutex is intentionally private to this implementation, but you may want to removeLogMutexfromCodeModeDependenciesif it's not needed elsewhere.core/mcp/codemode/starlark/readfile.go (1)
355-368: Consider usingsort.Stringsfor cleaner sorting.The manual bubble sort implementation works but Go's standard library provides a cleaner alternative.
♻️ Suggested refactor
+import "sort" + // Simple alphabetical sort for each group -for i := 0; i < len(requiredNames)-1; i++ { - for j := i + 1; j < len(requiredNames); j++ { - if requiredNames[i] > requiredNames[j] { - requiredNames[i], requiredNames[j] = requiredNames[j], requiredNames[i] - } - } -} -for i := 0; i < len(optionalNames)-1; i++ { - for j := i + 1; j < len(optionalNames); j++ { - if optionalNames[i] > optionalNames[j] { - optionalNames[i], optionalNames[j] = optionalNames[j], optionalNames[i] - } - } -} +sort.Strings(requiredNames) +sort.Strings(optionalNames)core/mcp/codemode/starlark/executecode.go (1)
274-280: Type assertion on kwarg key could panic in edge cases.Line 276 uses
kwarg[0].(starlark.String)without a type check. While Starlark kwargs should always have string keys, a defensive check would be safer.♻️ Suggested defensive check
for _, kwarg := range kwargs { if len(kwarg) == 2 { - key := string(kwarg[0].(starlark.String)) - value := starlarkToGo(kwarg[1]) - goArgs[key] = value + if keyStr, ok := kwarg[0].(starlark.String); ok { + goArgs[string(keyStr)] = starlarkToGo(kwarg[1]) + } } }
| case map[string]interface{}: | ||
| dict := starlark.NewDict(len(val)) | ||
| for k, v := range val { | ||
| dict.SetKey(starlark.String(k), goToStarlark(v)) | ||
| } | ||
| return dict |
There was a problem hiding this comment.
Ignored error from Dict.SetKey.
starlark.Dict.SetKey returns an error (e.g., when the dict is frozen), which is currently ignored. While unlikely in this context, handling it improves robustness.
🐛 Proposed fix
case map[string]interface{}:
dict := starlark.NewDict(len(val))
for k, v := range val {
- dict.SetKey(starlark.String(k), goToStarlark(v))
+ _ = dict.SetKey(starlark.String(k), goToStarlark(v)) // Error ignored: dict is newly created and unfrozen
}
return dictOr handle the error explicitly if you want stricter behavior.
📝 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.
| case map[string]interface{}: | |
| dict := starlark.NewDict(len(val)) | |
| for k, v := range val { | |
| dict.SetKey(starlark.String(k), goToStarlark(v)) | |
| } | |
| return dict | |
| case map[string]interface{}: | |
| dict := starlark.NewDict(len(val)) | |
| for k, v := range val { | |
| _ = dict.SetKey(starlark.String(k), goToStarlark(v)) // Error ignored: dict is newly created and unfrozen | |
| } | |
| return dict |
🤖 Prompt for AI Agents
In `@core/mcp/codemode/starlark/utils.go` around lines 99 - 104, The call to
starlark.Dict.SetKey in the map[string]interface{} case ignores its returned
error; update the block around starlark.NewDict and goToStarlark to capture the
error from dict.SetKey (e.g., err := dict.SetKey(starlark.String(k),
goToStarlark(v))) and handle it appropriately for this package—either return the
error to the caller, log and continue, or panic if freezing should never occur;
adjust the surrounding function signature/return path if needed so that
goToStarlark (or its caller) can propagate or react to SetKey failures.
db30ffa to
a02d6fb
Compare
akshaydeo
force-pushed
the
01-26-code_mode_compact_signatures
branch
2 times, most recently
from
b75dd1a to
1f01461
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/config/views/mcpView.tsx (1)
34-39: Preserve0when hydrating tool sync interval.
|| "10"treats0as falsy, so a valid zero value becomes “10” in the UI and can block correct save behavior. Use nullish coalescing instead.🛠️ Proposed fix
- mcp_tool_sync_interval: config?.mcp_tool_sync_interval?.toString() || "10", + mcp_tool_sync_interval: config?.mcp_tool_sync_interval?.toString() ?? "10",
🤖 Fix all issues with AI agents
In `@core/mcp/codemode/starlark/readfile.go`:
- Around line 23-38: Update the example filename strings in the readfile.go
branching that sets fileNameDescription and toolDescription so they include the
required "servers/" prefix; specifically modify the examples in the
server-binding branch and the tool-binding branch (where bindingLevel is
compared to schemas.CodeModeBindingLevelServer and where
fileNameDescription/toolDescription are assigned) to show
"servers/<serverName>.pyi" and "servers/<serverName>/<toolName>.pyi"
respectively, and ensure any inline example text like 'calculator.pyi' or
'calculator/add.pyi' is changed to 'servers/calculator.pyi' and
'servers/calculator/add.pyi' to match the stated format.
In `@core/mcp/toolmanager.go`:
- Around line 514-528: The code currently reverses sanitized tool names by
calling getOriginalToolName and sending originalMCPToolName to the MCP server;
instead keep tool names sanitized end‑to‑end. Replace usage of
originalMCPToolName in the mcp.CallToolRequest Params.Name with
sanitizedToolName, remove or stop calling getOriginalToolName in this path (and
any related comment implying hyphen restoration), and update the surrounding
comment to state that sanitized names (underscores) are sent to the MCP server
to preserve consistency (symbols: stripClientPrefix, sanitizedToolName,
getOriginalToolName, mcp.CallToolRequest, Params.Name).
- Around line 639-645: The code currently skips calling m.codeMode.UpdateConfig
when config.CodeModeBindingLevel is empty, so timeout-only changes are not
propagated; change the logic to always call m.codeMode.UpdateConfig when
m.codeMode != nil, computing an effective binding level first: if
config.CodeModeBindingLevel is non-empty use it, otherwise read the current
binding level from the existing CodeMode instance (e.g., via a getter or its
current config) and pass that plus config.ToolExecutionTimeout in the
CodeModeConfig to m.codeMode.UpdateConfig so timeout updates are applied even
when the binding level is unchanged.
In `@framework/oauth2/main.go`:
- Around line 486-491: The code creates a TableOauthToken using
strings.TrimSpace on tokenResponse values but doesn't validate the trimmed
values; replicate the same fix used in RefreshAccessToken: first assign trimmed
values to local vars (e.g., trimmedAccess :=
strings.TrimSpace(tokenResponse.AccessToken), trimmedRefresh :=
strings.TrimSpace(tokenResponse.RefreshToken)), then check that trimmedAccess
(and trimmedRefresh if required) are not empty and return/log an error instead
of inserting when they are empty; update the tokenRecord to use these trimmed
vars and ensure the surrounding function returns an error path when validation
fails.
In `@ui/app/workspace/config/views/mcpView.tsx`:
- Around line 164-166: Update the help text in mcpView.tsx (the paragraph under
the MCP tool sync interval control) to correctly reflect backend semantics:
state that setting the global mcp_tool_sync_interval to 0 (or any value ≤ 0)
uses DefaultToolSyncInterval (10 minutes) rather than disabling sync, and note
that disabling sync is only supported per-client via -1; reference the symbols
mcp_tool_sync_interval and DefaultToolSyncInterval in the copy so users
understand the behavior and where to disable sync.
🧹 Nitpick comments (9)
framework/oauth2/main.go (1)
613-616:fmt.Sscanfsilently ignores parse errors.If
expires_incontains a non-numeric value (e.g.,"abc"), parsing fails silently andExpiresInremains0, causing immediate token expiration. This is arguably acceptable (triggers refresh), but explicit error logging would aid debugging.🔧 Optional: Add logging for malformed expires_in
// Parse expires_in if present if expiresIn := formValues.Get("expires_in"); expiresIn != "" { - fmt.Sscanf(expiresIn, "%d", &tokenResponse.ExpiresIn) + if _, err := fmt.Sscanf(expiresIn, "%d", &tokenResponse.ExpiresIn); err != nil { + logger.Warn("Failed to parse expires_in from form response", "value", expiresIn, "error", err) + } }core/mcp/codemode.go (1)
60-76: Consider documenting the thread-safety expectations for CodeModeDependencies.The
LogMutex *sync.Mutexfield suggests concurrent access concerns. Consider adding a brief comment clarifying whether callers are expected to use this mutex or if the CodeMode implementation handles synchronization internally.📝 Suggested documentation enhancement
// CodeModeDependencies holds the dependencies required by CodeMode implementations. type CodeModeDependencies struct { // ClientManager provides access to MCP clients and their tools ClientManager ClientManager // PluginPipelineProvider returns a plugin pipeline for running MCP hooks PluginPipelineProvider func() PluginPipeline // ReleasePluginPipeline releases a plugin pipeline back to the pool ReleasePluginPipeline func(pipeline PluginPipeline) // FetchNewRequestIDFunc generates unique request IDs for nested tool calls FetchNewRequestIDFunc func(ctx *schemas.BifrostContext) string // LogMutex protects concurrent access to logs during code execution + // The CodeMode implementation is responsible for acquiring this lock when writing logs LogMutex *sync.Mutex }ui/lib/types/schemas.ts (1)
663-663: Consider adding validation for tool_sync_interval values.The field accepts any number, but based on the comment semantics (-1 = disabled, 0 = use global, >0 = custom interval), consider adding validation to catch invalid inputs early.
🔧 Suggested validation enhancement
- tool_sync_interval: z.number().optional(), // -1 = disabled, 0 = use global, >0 = custom interval in minutes + tool_sync_interval: z + .number() + .int("Sync interval must be an integer") + .min(-1, "Sync interval must be -1 (disabled), 0 (use global), or a positive number of minutes") + .optional(), // -1 = disabled, 0 = use global, >0 = custom interval in minutescore/mcp/codemode/starlark/starlark_test.go (1)
218-239: Consider using standard library string functions.The custom
containsIgnoreCaseimplementation has O(n²) complexity due to recursion. Since these are test helpers, performance isn't critical, but usingstrings.Containswithstrings.ToLowerorstrings.EqualFoldwould be cleaner and more efficient.Suggested simplification using standard library
+import "strings" + func containsAny(s string, substrs ...string) bool { for _, sub := range substrs { - if containsIgnoreCase(s, sub) { + if strings.Contains(strings.ToLower(s), strings.ToLower(sub)) { return true } } return false } - -func containsIgnoreCase(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && (containsIgnoreCase(s[1:], substr) || (len(s) >= len(substr) && equalFold(s[:len(substr)], substr)))) -} - -func equalFold(a, b string) bool { - if len(a) != len(b) { - return false - } - for i := 0; i < len(a); i++ { - ca, cb := a[i], b[i] - if ca >= 'A' && ca <= 'Z' { - ca += 'a' - 'A' - } - if cb >= 'A' && cb <= 'Z' { - cb += 'a' - 'A' - } - if ca != cb { - return false - } - } - return true -}framework/configstore/migrations.go (1)
3067-3107: Migration implementation looks good, with a minor robustness suggestion.The migration correctly adds both global (
mcp_tool_sync_interval) and per-client (tool_sync_interval) columns with proper idempotency checks in the Migrate function.For consistency with other migrations in this file (e.g.,
migrationAddAllowedHeadersJSONColumnat line 2921), consider addingHasColumnchecks in the Rollback function to make it more defensive.Optional: Add defensive checks in rollback
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() - if err := migrator.DropColumn(&tables.TableClientConfig{}, "mcp_tool_sync_interval"); err != nil { - return err + if migrator.HasColumn(&tables.TableClientConfig{}, "mcp_tool_sync_interval") { + if err := migrator.DropColumn(&tables.TableClientConfig{}, "mcp_tool_sync_interval"); err != nil { + return err + } } - if err := migrator.DropColumn(&tables.TableMCPClient{}, "tool_sync_interval"); err != nil { - return err + if migrator.HasColumn(&tables.TableMCPClient{}, "tool_sync_interval") { + if err := migrator.DropColumn(&tables.TableMCPClient{}, "tool_sync_interval"); err != nil { + return err + } } return nil },core/internal/mcptests/fixtures.go (1)
1467-1469: Preferbifrost.Ptr(...)for pointer creation.The repo convention favors
bifrost.Ptr(...)over&valueeven when&is valid.♻️ Suggested change
- logger := &testLogger{t: t} + logger := bifrost.Ptr(testLogger{t: t})Based on learnings, prefer
bifrost.Ptr(...)for pointer creation.ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
338-346: Guard againstNaNin the tool sync interval input.
parseIntcan yieldNaNduring partial edits (e.g., “-”), which then renders as “NaN”. Consider normalizing invalid parses toundefined.🛠️ Possible tweak
- onChange={(e) => { - const val = e.target.value === "" ? undefined : parseInt(e.target.value); - field.onChange(val); - }} + onChange={(e) => { + if (e.target.value === "") { + field.onChange(undefined); + return; + } + const parsed = Number.parseInt(e.target.value, 10); + field.onChange(Number.isNaN(parsed) ? undefined : parsed); + }}core/bifrost.go (1)
273-283: Prefer the Ptr helper for CodeModeConfig allocation.If the
Ptrhelper is available inpackage bifrost, use it instead of&for consistency with the repo convention.♻️ Proposed refactor
- codeModeConfig = &mcp.CodeModeConfig{ + codeModeConfig = Ptr(mcp.CodeModeConfig{ BindingLevel: mcpConfig.ToolManagerConfig.CodeModeBindingLevel, ToolExecutionTimeout: mcpConfig.ToolManagerConfig.ToolExecutionTimeout, - } + })Based on learnings, keep pointer creation consistent with
bifrost.Ptr.core/mcp/codemode/starlark/executecode.go (1)
19-24: Remove unusedtoolBindingstruct.The
toolBindingstruct (lines 20-24) is defined but never used anywhere in the codebase. Remove it to improve code cleanliness.
| if bindingLevel == schemas.CodeModeBindingLevelServer { | ||
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>.pyi (e.g., 'calculator.pyi')" | ||
| toolDescription = "Reads a virtual .pyi stub file for a specific MCP server, returning compact Python function signatures " + | ||
| "for all tools available on that server. The fileName should be in format servers/<serverName>.pyi as listed by listToolFiles. " + | ||
| "The function performs case-insensitive matching and removes the .pyi extension. " + | ||
| "Each tool can be accessed in code via: serverName.tool_name(param=value). " + | ||
| "If the compact signature is not enough to understand a tool, use getToolDocs for detailed documentation. " + | ||
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." | ||
| } else { | ||
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>/<toolName>.pyi (e.g., 'calculator/add.pyi')" | ||
| toolDescription = "Reads a virtual .pyi stub file for a specific tool, returning its compact Python function signature. " + | ||
| "The fileName should be in format servers/<serverName>/<toolName>.pyi as listed by listToolFiles. " + | ||
| "The function performs case-insensitive matching and removes the .pyi extension. " + | ||
| "The tool can be accessed in code via: serverName.tool_name(param=value). " + | ||
| "If the compact signature is not enough to understand the tool, use getToolDocs for detailed documentation. " + | ||
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." |
There was a problem hiding this comment.
Align filename examples with the servers/ prefix.
The examples omit the servers/ prefix even though the format requires it, which can confuse users.
✏️ Suggested fix
- fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>.pyi (e.g., 'calculator.pyi')"
+ fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>.pyi (e.g., 'servers/calculator.pyi')"
...
- fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>/<toolName>.pyi (e.g., 'calculator/add.pyi')"
+ fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>/<toolName>.pyi (e.g., 'servers/calculator/add.pyi')"📝 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.
| if bindingLevel == schemas.CodeModeBindingLevelServer { | |
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>.pyi (e.g., 'calculator.pyi')" | |
| toolDescription = "Reads a virtual .pyi stub file for a specific MCP server, returning compact Python function signatures " + | |
| "for all tools available on that server. The fileName should be in format servers/<serverName>.pyi as listed by listToolFiles. " + | |
| "The function performs case-insensitive matching and removes the .pyi extension. " + | |
| "Each tool can be accessed in code via: serverName.tool_name(param=value). " + | |
| "If the compact signature is not enough to understand a tool, use getToolDocs for detailed documentation. " + | |
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." | |
| } else { | |
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>/<toolName>.pyi (e.g., 'calculator/add.pyi')" | |
| toolDescription = "Reads a virtual .pyi stub file for a specific tool, returning its compact Python function signature. " + | |
| "The fileName should be in format servers/<serverName>/<toolName>.pyi as listed by listToolFiles. " + | |
| "The function performs case-insensitive matching and removes the .pyi extension. " + | |
| "The tool can be accessed in code via: serverName.tool_name(param=value). " + | |
| "If the compact signature is not enough to understand the tool, use getToolDocs for detailed documentation. " + | |
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." | |
| if bindingLevel == schemas.CodeModeBindingLevelServer { | |
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>.pyi (e.g., 'servers/calculator.pyi')" | |
| toolDescription = "Reads a virtual .pyi stub file for a specific MCP server, returning compact Python function signatures " + | |
| "for all tools available on that server. The fileName should be in format servers/<serverName>.pyi as listed by listToolFiles. " + | |
| "The function performs case-insensitive matching and removes the .pyi extension. " + | |
| "Each tool can be accessed in code via: serverName.tool_name(param=value). " + | |
| "If the compact signature is not enough to understand a tool, use getToolDocs for detailed documentation. " + | |
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." | |
| } else { | |
| fileNameDescription = "The virtual filename from listToolFiles in format: servers/<serverName>/<toolName>.pyi (e.g., 'servers/calculator/add.pyi')" | |
| toolDescription = "Reads a virtual .pyi stub file for a specific tool, returning its compact Python function signature. " + | |
| "The fileName should be in format servers/<serverName>/<toolName>.pyi as listed by listToolFiles. " + | |
| "The function performs case-insensitive matching and removes the .pyi extension. " + | |
| "The tool can be accessed in code via: serverName.tool_name(param=value). " + | |
| "If the compact signature is not enough to understand the tool, use getToolDocs for detailed documentation. " + | |
| "Workflow: listToolFiles -> readToolFile -> (optional) getToolDocs -> executeToolCode." |
🤖 Prompt for AI Agents
In `@core/mcp/codemode/starlark/readfile.go` around lines 23 - 38, Update the
example filename strings in the readfile.go branching that sets
fileNameDescription and toolDescription so they include the required "servers/"
prefix; specifically modify the examples in the server-binding branch and the
tool-binding branch (where bindingLevel is compared to
schemas.CodeModeBindingLevelServer and where fileNameDescription/toolDescription
are assigned) to show "servers/<serverName>.pyi" and
"servers/<serverName>/<toolName>.pyi" respectively, and ensure any inline
example text like 'calculator.pyi' or 'calculator/add.pyi' is changed to
'servers/calculator.pyi' and 'servers/calculator/add.pyi' to match the stated
format.
| // Strip the client name prefix from tool name before calling MCP server | ||
| // The MCP server expects the original tool name (with hyphens), not the sanitized version | ||
| sanitizedToolName := stripClientPrefix(toolName, client.ExecutionConfig.Name) | ||
| originalMCPToolName := getOriginalToolName(sanitizedToolName, client) | ||
|
|
||
| // Call the tool via MCP client -> MCP server | ||
| callRequest := mcp.CallToolRequest{ | ||
| Request: mcp.Request{ | ||
| Method: string(mcp.MethodToolsCall), | ||
| }, | ||
| Params: mcp.CallToolParams{ | ||
| Name: originalMCPToolName, | ||
| Arguments: arguments, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Avoid reversing sanitized tool names when calling MCP.
This reintroduces original names even though the system standardizes tool names by replacing - with _. Send the sanitized name to the server to keep names consistent end‑to‑end.
🐛 Proposed fix
- // Strip the client name prefix from tool name before calling MCP server
- // The MCP server expects the original tool name (with hyphens), not the sanitized version
- sanitizedToolName := stripClientPrefix(toolName, client.ExecutionConfig.Name)
- originalMCPToolName := getOriginalToolName(sanitizedToolName, client)
+ // Strip the client name prefix; keep sanitized names end-to-end
+ sanitizedToolName := stripClientPrefix(toolName, client.ExecutionConfig.Name)
...
Params: mcp.CallToolParams{
- Name: originalMCPToolName,
+ Name: sanitizedToolName,
Arguments: arguments,
},Based on learnings, tool names should remain sanitized end‑to‑end.
📝 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.
| // Strip the client name prefix from tool name before calling MCP server | |
| // The MCP server expects the original tool name (with hyphens), not the sanitized version | |
| sanitizedToolName := stripClientPrefix(toolName, client.ExecutionConfig.Name) | |
| originalMCPToolName := getOriginalToolName(sanitizedToolName, client) | |
| // Call the tool via MCP client -> MCP server | |
| callRequest := mcp.CallToolRequest{ | |
| Request: mcp.Request{ | |
| Method: string(mcp.MethodToolsCall), | |
| }, | |
| Params: mcp.CallToolParams{ | |
| Name: originalMCPToolName, | |
| Arguments: arguments, | |
| }, | |
| } | |
| // Strip the client name prefix; keep sanitized names end-to-end | |
| sanitizedToolName := stripClientPrefix(toolName, client.ExecutionConfig.Name) | |
| // Call the tool via MCP client -> MCP server | |
| callRequest := mcp.CallToolRequest{ | |
| Request: mcp.Request{ | |
| Method: string(mcp.MethodToolsCall), | |
| }, | |
| Params: mcp.CallToolParams{ | |
| Name: sanitizedToolName, | |
| Arguments: arguments, | |
| }, | |
| } |
🤖 Prompt for AI Agents
In `@core/mcp/toolmanager.go` around lines 514 - 528, The code currently reverses
sanitized tool names by calling getOriginalToolName and sending
originalMCPToolName to the MCP server; instead keep tool names sanitized
end‑to‑end. Replace usage of originalMCPToolName in the mcp.CallToolRequest
Params.Name with sanitizedToolName, remove or stop calling getOriginalToolName
in this path (and any related comment implying hyphen restoration), and update
the surrounding comment to state that sanitized names (underscores) are sent to
the MCP server to preserve consistency (symbols: stripClientPrefix,
sanitizedToolName, getOriginalToolName, mcp.CallToolRequest, Params.Name).
| // Update CodeMode configuration if present | ||
| if m.codeMode != nil && config.CodeModeBindingLevel != "" { | ||
| m.codeMode.UpdateConfig(&CodeModeConfig{ | ||
| BindingLevel: config.CodeModeBindingLevel, | ||
| ToolExecutionTimeout: config.ToolExecutionTimeout, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Propagate timeout updates to CodeMode even when binding level is unchanged.
UpdateConfig only calls CodeMode.UpdateConfig when CodeModeBindingLevel is non-empty, so timeout-only updates may be skipped. Consider passing effective binding level + timeout.
🐛 Proposed fix
- if m.codeMode != nil && config.CodeModeBindingLevel != "" {
- m.codeMode.UpdateConfig(&CodeModeConfig{
- BindingLevel: config.CodeModeBindingLevel,
- ToolExecutionTimeout: config.ToolExecutionTimeout,
- })
- }
+ if m.codeMode != nil {
+ bindingLevel := config.CodeModeBindingLevel
+ if bindingLevel == "" {
+ bindingLevel = m.codeMode.GetBindingLevel()
+ }
+ timeout := config.ToolExecutionTimeout
+ if timeout <= 0 {
+ timeout = m.toolExecutionTimeout.Load().(time.Duration)
+ }
+ m.codeMode.UpdateConfig(&CodeModeConfig{
+ BindingLevel: bindingLevel,
+ ToolExecutionTimeout: timeout,
+ })
+ }🤖 Prompt for AI Agents
In `@core/mcp/toolmanager.go` around lines 639 - 645, The code currently skips
calling m.codeMode.UpdateConfig when config.CodeModeBindingLevel is empty, so
timeout-only changes are not propagated; change the logic to always call
m.codeMode.UpdateConfig when m.codeMode != nil, computing an effective binding
level first: if config.CodeModeBindingLevel is non-empty use it, otherwise read
the current binding level from the existing CodeMode instance (e.g., via a
getter or its current config) and pass that plus config.ToolExecutionTimeout in
the CodeModeConfig to m.codeMode.UpdateConfig so timeout updates are applied
even when the binding level is unchanged.
| // Create oauth_token record (sanitize tokens to prevent header formatting issues) | ||
| tokenID := uuid.New().String() | ||
| tokenRecord := &tables.TableOauthToken{ | ||
| ID: tokenID, | ||
| AccessToken: tokenResponse.AccessToken, | ||
| RefreshToken: tokenResponse.RefreshToken, | ||
| AccessToken: strings.TrimSpace(tokenResponse.AccessToken), | ||
| RefreshToken: strings.TrimSpace(tokenResponse.RefreshToken), |
There was a problem hiding this comment.
Same validation gap as RefreshAccessToken—empty tokens could be stored.
Same concern applies here: whitespace-only access tokens would be trimmed to empty and stored. The OAuth flow would appear to complete successfully, but subsequent token retrieval would fail.
🔧 Suggested fix (validate after trim)
// Create oauth_token record (sanitize tokens to prevent header formatting issues)
tokenID := uuid.New().String()
+ sanitizedAccessToken := strings.TrimSpace(tokenResponse.AccessToken)
+ if sanitizedAccessToken == "" {
+ return fmt.Errorf("access token is empty after sanitization")
+ }
tokenRecord := &tables.TableOauthToken{
ID: tokenID,
- AccessToken: strings.TrimSpace(tokenResponse.AccessToken),
+ AccessToken: sanitizedAccessToken,
RefreshToken: strings.TrimSpace(tokenResponse.RefreshToken),
TokenType: tokenResponse.TokenType,
ExpiresAt: time.Now().Add(time.Duration(tokenResponse.ExpiresIn) * time.Second),
Scopes: string(scopesJSON),
}🤖 Prompt for AI Agents
In `@framework/oauth2/main.go` around lines 486 - 491, The code creates a
TableOauthToken using strings.TrimSpace on tokenResponse values but doesn't
validate the trimmed values; replicate the same fix used in RefreshAccessToken:
first assign trimmed values to local vars (e.g., trimmedAccess :=
strings.TrimSpace(tokenResponse.AccessToken), trimmedRefresh :=
strings.TrimSpace(tokenResponse.RefreshToken)), then check that trimmedAccess
(and trimmedRefresh if required) are not empty and return/log an error instead
of inserting when they are empty; update the tokenRecord to use these trimmed
vars and ensure the surrounding function returns an error path when validation
fails.
| <p className="text-muted-foreground text-sm"> | ||
| How often to refresh tool lists from MCP servers. Set to 0 to disable. | ||
| </p> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ToolSyncInterval|tool_sync_interval|ToolSyncManager|ClientToolSyncer|DefaultToolSyncInterval" core/mcp frameworkRepository: maximhq/bifrost
Length of output: 5983
🏁 Script executed:
# Also search more broadly for tool sync and MCP configuration
rg -n "tool.*sync" -i core/mcp framework --type ts --type tsx --type py --type go --type javaRepository: maximhq/bifrost
Length of output: 86
🏁 Script executed:
# Look for schema definitions related to tool sync interval
fd -e json -e schema -e proto core/mcp framework | head -20Repository: maximhq/bifrost
Length of output: 396
🏁 Script executed:
# Search for references to "0" and "-1" in MCP configuration/schema contexts
rg -B3 -A3 "disable.*tool|tool.*disable" core/mcp framework -iRepository: maximhq/bifrost
Length of output: 3275
🏁 Script executed:
# Read the mcpView.tsx file to see which setting is being configured
cat -n ui/app/workspace/config/views/mcpView.tsx | sed -n '150,180p'Repository: maximhq/bifrost
Length of output: 1281
🏁 Script executed:
# Read the full ResolveToolSyncInterval function to understand 0 and -1 handling
cat -n core/mcp/toolsync.go | sed -n '216,245p'Repository: maximhq/bifrost
Length of output: 1105
🏁 Script executed:
# Check the NewClientToolSyncer function to see how interval 0 is handled
cat -n core/mcp/toolsync.go | sed -n '31,50p'Repository: maximhq/bifrost
Length of output: 640
🏁 Script executed:
# Check the NewToolSyncManager function
cat -n core/mcp/toolsync.go | sed -n '163,175p'Repository: maximhq/bifrost
Length of output: 528
Correct UI copy for tool sync interval—0 does not disable.
The UI says "Set to 0 to disable," but backend code shows setting the global mcp_tool_sync_interval to 0 or any value ≤ 0 defaults to DefaultToolSyncInterval (10 minutes). Update the help text to reflect the actual semantics: 0 uses the default interval, not disabled. For consistent behavior with per-client settings (which use -1 to disable), either support -1 for the global setting or clarify the minimum allowed value.
🤖 Prompt for AI Agents
In `@ui/app/workspace/config/views/mcpView.tsx` around lines 164 - 166, Update the
help text in mcpView.tsx (the paragraph under the MCP tool sync interval
control) to correctly reflect backend semantics: state that setting the global
mcp_tool_sync_interval to 0 (or any value ≤ 0) uses DefaultToolSyncInterval (10
minutes) rather than disabling sync, and note that disabling sync is only
supported per-client via -1; reference the symbols mcp_tool_sync_interval and
DefaultToolSyncInterval in the copy so users understand the behavior and where
to disable sync.
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
9d20b32 to
cec3f0d
Compare
akshaydeo
force-pushed
the
01-26-code_mode_compact_signatures
branch
from
1f01461 to
7318932
Compare
akshaydeo
changed the base branch from
01-24-mcp_auth_server_setup
to
graphite-base/1453
1 participant
Summary
Replace JavaScript/TypeScript code execution with Python (Starlark) for code mode, providing a more accessible and deterministic environment for tool orchestration.
Changes
getToolDocstool for detailed documentation.pyistubs instead of.d.tsfilesType of change
Affected areas
How to test
Test the new Python-based code mode with various tools:
Breaking changes
This changes the syntax for code mode from TypeScript to Python (Starlark). Users will need to update their code mode scripts to use Python syntax:
Old:
const result = await server.tool_name({ param: "value" });New:
result = server.tool_name(param="value")Security considerations
The Starlark interpreter provides a more deterministic and sandboxed environment than the previous JavaScript VM, with no access to imports, network, or file system operations outside of MCP tools.
Checklist