Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions .claude/skills/resolve-pr-comments/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

---
name: resolve-pr-comments
description: Resolve all unresolved PR comments interactively. Use when asked to resolve PR comments, address review feedback, handle CodeRabbit comments, or fix PR review issues. Invoked with /resolve-pr-comments <PR_NUMBER> or /resolve-pr-comments <owner/repo> <PR_NUMBER>.
description: Resolve all unresolved PR comments interactively. Makes local edits only—NEVER commits or pushes. Use when asked to resolve PR comments, address review feedback, handle CodeRabbit comments, or fix PR review issues. Invoked with /resolve-pr-comments <PR_NUMBER> or /resolve-pr-comments <owner/repo> <PR_NUMBER>.
allowed-tools: Read, Grep, Glob, Bash, Edit, Write, WebFetch, Task, AskUserQuestion, TodoWrite
---

Expand Down Expand Up @@ -143,7 +143,7 @@ gh api repos/OWNER/REPO/pulls/PR_NUMBER/comments --paginate | jq '.[] | select(.

## Step 5: Execute Actions

**CRITICAL: Do NOT reply to PR comments until changes are pushed to the remote.** The reviewer cannot verify fixes until the code is pushed. Collect all fixes locally first, then push, then reply.
**CRITICAL: Do NOT reply to PR comments until changes are pushed to the remote.** The reviewer cannot verify fixes until the code is pushed. Collect all fixes locally. This skill NEVER commits or pushes—the user handles that manually.

### For FIX:
1. Make the code change using Edit tool
Expand All @@ -157,12 +157,12 @@ These can be posted immediately since they don't require code verification:
gh api repos/OWNER/REPO/pulls/PR_NUMBER/comments/COMMENT_ID/replies -X POST -f body="<your reply>"
```

## Step 5b: Push and Reply to FIX comments
## Step 5b: Reply to FIX comments (after user pushes)

After ALL comments have been addressed locally:

1. Ask user to if they have pushed these changes to remote. Yes/No
2. **Only after push succeeds**, reply to FIX comments:
1. Ask user if they have pushed these changes to remote (this skill never commits or pushes). Yes/No
2. **Only after user confirms push**, reply to FIX comments:
```bash
gh api repos/OWNER/REPO/pulls/PR_NUMBER/comments/COMMENT_ID/replies -X POST -f body="Fixed - <description of change>. See updated code."
```
Comment thread
Pratham-Mishra04 marked this conversation as resolved.
Expand Down Expand Up @@ -209,13 +209,14 @@ If count is 0, report success. If comments remain:

## Important Notes

1. **NEVER reply "Fixed" until code is pushed** - The reviewer cannot verify fixes until they're on the remote. Make all fixes locally, push, THEN reply.
2. **Always read the file** before suggesting fixes - understand context
3. **Check for existing replies** in the thread before responding
4. **Wait for user approval** on each action - never auto-fix without confirmation
5. **Update tracking file** after each action
6. **Some bots are slow** - CodeRabbit may take minutes to auto-resolve after push
7. **Push code changes** before expecting auto-resolution of FIX actions
1. **NEVER commit or push changes** - This skill only makes local edits. The user handles `git add`, `git commit`, and `git push` themselves. Do not run any git commit or git push commands.
2. **NEVER reply "Fixed" until code is pushed** - The reviewer cannot verify fixes until they're on the remote. Make all fixes locally. Only reply to FIX comments after the user confirms they have pushed (the user pushes manually).
3. **Always read the file** before suggesting fixes - understand context
4. **Check for existing replies** in the thread before responding
5. **Wait for user approval** on each action - never auto-fix without confirmation
6. **Update tracking file** after each action
7. **Some bots are slow** - CodeRabbit may take minutes to auto-resolve after push
8. **User pushes manually** - This skill never commits or pushes; the user must push code changes before expecting auto-resolution of FIX actions

## Error Handling

Expand Down
25 changes: 14 additions & 11 deletions core/bifrost.go
Original file line number Diff line number Diff line change
Expand Up @@ -3347,7 +3347,7 @@ func (bifrost *Bifrost) getProviderMutex(providerKey schemas.ModelProvider) *syn
// }, toolSchema)
func (bifrost *Bifrost) RegisterMCPTool(name, description string, handler func(args any) (string, error), toolSchema schemas.ChatTool) error {
if bifrost.MCPManager == nil {
return fmt.Errorf("MCP is not configured in this Bifrost instance")
return fmt.Errorf("mcp is not configured in this bifrost instance")
}

return bifrost.MCPManager.RegisterTool(name, description, handler, toolSchema)
Expand All @@ -3365,7 +3365,7 @@ func (bifrost *Bifrost) RegisterMCPTool(name, description string, handler func(a
// - error: Any retrieval error
func (bifrost *Bifrost) GetMCPClients() ([]schemas.MCPClient, error) {
if bifrost.MCPManager == nil {
return nil, fmt.Errorf("MCP is not configured in this Bifrost instance")
return nil, fmt.Errorf("mcp is not configured in this bifrost instance")
}

clients := bifrost.MCPManager.GetClients()
Expand Down Expand Up @@ -3476,7 +3476,7 @@ func (bifrost *Bifrost) AddMCPClient(config *schemas.MCPClientConfig) error {
// }
func (bifrost *Bifrost) RemoveMCPClient(id string) error {
if bifrost.MCPManager == nil {
return fmt.Errorf("MCP is not configured in this Bifrost instance")
return fmt.Errorf("mcp is not configured in this bifrost instance")
}

return bifrost.MCPManager.RemoveClient(id)
Expand Down Expand Up @@ -3509,7 +3509,7 @@ func (bifrost *Bifrost) SetMCPManager(manager mcp.MCPManagerInterface) {
// })
func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPClientConfig) error {
if bifrost.MCPManager == nil {
return fmt.Errorf("MCP is not configured in this Bifrost instance")
return fmt.Errorf("mcp is not configured in this bifrost instance")
}

return bifrost.MCPManager.UpdateClient(id, updatedConfig)
Expand All @@ -3524,23 +3524,26 @@ func (bifrost *Bifrost) UpdateMCPClient(id string, updatedConfig *schemas.MCPCli
// - error: Any reconnection error
func (bifrost *Bifrost) ReconnectMCPClient(id string) error {
if bifrost.MCPManager == nil {
return fmt.Errorf("MCP is not configured in this Bifrost instance")
return fmt.Errorf("mcp is not configured in this bifrost instance")
}

return bifrost.MCPManager.ReconnectClient(id)
}

// UpdateToolManagerConfig updates the tool manager config for the MCP manager.
// This allows for hot-reloading of the tool manager config at runtime.
func (bifrost *Bifrost) UpdateToolManagerConfig(maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string) error {
// Pass the current value of disableAutoToolInject whenever only other fields
// change so the flag is never silently reset to its zero value.
func (bifrost *Bifrost) UpdateToolManagerConfig(maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string, disableAutoToolInject bool) error {
if bifrost.MCPManager == nil {
return fmt.Errorf("MCP is not configured in this Bifrost instance")
return fmt.Errorf("mcp is not configured in this bifrost instance")
}

bifrost.MCPManager.UpdateToolManagerConfig(&schemas.MCPToolManagerConfig{
MaxAgentDepth: maxAgentDepth,
ToolExecutionTimeout: time.Duration(toolExecutionTimeoutInSeconds) * time.Second,
CodeModeBindingLevel: schemas.CodeModeBindingLevel(codeModeBindingLevel),
MaxAgentDepth: maxAgentDepth,
ToolExecutionTimeout: time.Duration(toolExecutionTimeoutInSeconds) * time.Second,
CodeModeBindingLevel: schemas.CodeModeBindingLevel(codeModeBindingLevel),
DisableAutoToolInject: disableAutoToolInject,
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return nil
}
Expand Down Expand Up @@ -5409,7 +5412,7 @@ func (bifrost *Bifrost) handleMCPToolExecution(ctx *schemas.BifrostContext, mcpR
return nil, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Message: "MCP is not configured in this Bifrost instance",
Message: "mcp is not configured in this bifrost instance",
},
ExtraFields: schemas.BifrostErrorExtraFields{
RequestType: requestType,
Expand Down
4 changes: 3 additions & 1 deletion core/mcp/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ type MCPManagerInterface interface {
// ExecuteToolCall executes a single tool call and returns the result
ExecuteToolCall(ctx *schemas.BifrostContext, request *schemas.BifrostMCPRequest) (*schemas.BifrostMCPResponse, error)

// UpdateToolManagerConfig updates the configuration for the tool manager
// UpdateToolManagerConfig updates the configuration for the tool manager.
// DisableAutoToolInject in the config controls auto injection — pass the
// current value whenever only other fields change so it is never silently reset.
UpdateToolManagerConfig(config *schemas.MCPToolManagerConfig)

// Agent Mode Operations
Expand Down
24 changes: 19 additions & 5 deletions core/mcp/toolmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ type PluginPipeline interface {

// ToolsManager manages MCP tool execution and agent mode.
type ToolsManager struct {
toolExecutionTimeout atomic.Value
maxAgentDepth atomic.Int32
clientManager ClientManager
logger schemas.Logger
agentModeExecutor *AgentModeExecutor
toolExecutionTimeout atomic.Value
maxAgentDepth atomic.Int32
disableAutoToolInject atomic.Bool
clientManager ClientManager
logger schemas.Logger
agentModeExecutor *AgentModeExecutor

// CodeMode implementation for code execution (Starlark by default)
codeMode CodeMode
Expand Down Expand Up @@ -147,6 +148,7 @@ func NewToolsManagerWithCodeMode(
// Initialize atomic values
manager.toolExecutionTimeout.Store(config.ToolExecutionTimeout)
manager.maxAgentDepth.Store(int32(config.MaxAgentDepth))
manager.disableAutoToolInject.Store(config.DisableAutoToolInject)

manager.logger.Info("%s tool manager initialized with tool execution timeout: %v, max agent depth: %d, and code mode binding level: %s", MCPLogPrefix, config.ToolExecutionTimeout, config.MaxAgentDepth, config.CodeModeBindingLevel)
return manager
Expand Down Expand Up @@ -294,6 +296,16 @@ func (m *ToolsManager) ParseAndAddToolsToRequest(ctx context.Context, req *schem
return req
}

// When auto tool injection is disabled, only inject tools if the request
// has explicit context filters set (e.g. via x-bf-mcp-include-tools header).
if m.disableAutoToolInject.Load() {
includeTools := ctx.Value(schemas.MCPContextKeyIncludeTools)
includeClients := ctx.Value(schemas.MCPContextKeyIncludeClients)
if includeTools == nil && includeClients == nil {
return req
}
}

availableTools := m.GetAvailableTools(ctx)

if len(availableTools) == 0 {
Expand Down Expand Up @@ -668,6 +680,8 @@ func (m *ToolsManager) UpdateConfig(config *schemas.MCPToolManagerConfig) {
})
}

m.disableAutoToolInject.Store(config.DisableAutoToolInject)

Comment thread
Pratham-Mishra04 marked this conversation as resolved.
m.logger.Info("%s tool manager configuration updated with tool execution timeout: %v, max agent depth: %d, and code mode binding level: %s", MCPLogPrefix, config.ToolExecutionTimeout, config.MaxAgentDepth, config.CodeModeBindingLevel)
}

Expand Down
7 changes: 4 additions & 3 deletions core/schemas/mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ type MCPConfig struct {
}

type MCPToolManagerConfig struct {
ToolExecutionTimeout time.Duration `json:"tool_execution_timeout"`
MaxAgentDepth int `json:"max_agent_depth"`
CodeModeBindingLevel CodeModeBindingLevel `json:"code_mode_binding_level,omitempty"` // How tools are exposed in VFS: "server" or "tool"
ToolExecutionTimeout time.Duration `json:"tool_execution_timeout"`
MaxAgentDepth int `json:"max_agent_depth"`
CodeModeBindingLevel CodeModeBindingLevel `json:"code_mode_binding_level,omitempty"` // How tools are exposed in VFS: "server" or "tool"
DisableAutoToolInject bool `json:"disable_auto_tool_inject,omitempty"` // When true, MCP tools are not injected into requests by default
}

const (
Expand Down
1 change: 1 addition & 0 deletions framework/changelog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- feat: add MCPDisableAutoToolInject column to TableClientConfig
6 changes: 6 additions & 0 deletions framework/configstore/clientconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type ClientConfig struct {
MCPToolExecutionTimeout int `json:"mcp_tool_execution_timeout"` // The timeout for individual tool execution in seconds
MCPCodeModeBindingLevel string `json:"mcp_code_mode_binding_level"` // Code mode binding level: "server" or "tool"
MCPToolSyncInterval int `json:"mcp_tool_sync_interval"` // Global tool sync interval in minutes (default: 10, 0 = disabled)
MCPDisableAutoToolInject bool `json:"mcp_disable_auto_tool_inject"` // When true, MCP tools are not injected into requests by default
HeaderFilterConfig *tables.GlobalHeaderFilterConfig `json:"header_filter_config,omitempty"` // Global header filtering configuration for x-bf-eh-* headers
AsyncJobResultTTL int `json:"async_job_result_ttl"` // Default TTL for async job results in seconds (default: 3600 = 1 hour)
RequiredHeaders []string `json:"required_headers,omitempty"` // Headers that must be present on every request (case-insensitive)
Expand Down Expand Up @@ -140,6 +141,11 @@ func (c *ClientConfig) GenerateClientConfigHash() (string, error) {
hash.Write([]byte("mcpToolSyncInterval:0"))
}

// Only hash non-default value to avoid legacy config hash churn on upgrade.
if c.MCPDisableAutoToolInject {
hash.Write([]byte("mcpDisableAutoToolInject:true"))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if c.AsyncJobResultTTL > 0 {
hash.Write([]byte("asyncJobResultTTL:" + strconv.Itoa(c.AsyncJobResultTTL)))
} else {
Expand Down
33 changes: 33 additions & 0 deletions framework/configstore/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,9 @@ func triggerMigrations(ctx context.Context, db *gorm.DB) error {
if err := migrationBackfillEmptyVirtualKeyConfigs(ctx, db); err != nil {
return err
}
if err := migrationAddMCPDisableAutoToolInjectColumn(ctx, db); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -4335,6 +4338,36 @@ func migrationAddBedrockAssumeRoleColumns(ctx context.Context, db *gorm.DB) erro
return nil
}

// migrationAddMCPDisableAutoToolInjectColumn adds the mcp_disable_auto_tool_inject column to the client config table.
// When true, MCP tools are not automatically injected into requests; only explicit context filters apply.
func migrationAddMCPDisableAutoToolInjectColumn(ctx context.Context, db *gorm.DB) error {
m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
ID: "add_mcp_disable_auto_tool_inject_column",
Migrate: func(tx *gorm.DB) error {
tx = tx.WithContext(ctx)
migratorInstance := tx.Migrator()
if !migratorInstance.HasColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject") {
if err := migratorInstance.AddColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject"); err != nil {
return err
}
}
return nil
},
Rollback: func(tx *gorm.DB) error {
tx = tx.WithContext(ctx)
migratorInstance := tx.Migrator()
if err := migratorInstance.DropColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject"); err != nil {
return err
}
return nil
},
}})
if err := m.Migrate(); err != nil {
return fmt.Errorf("error while running mcp disable auto tool inject migration: %s", err.Error())
}
return nil
}

// migrationAddPricingRefactorColumns adds all new pricing columns introduced in the pricing module refactor
func migrationAddPricingRefactorColumns(ctx context.Context, db *gorm.DB) error {
m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{
Expand Down
9 changes: 6 additions & 3 deletions framework/configstore/rdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func (s *RDBConfigStore) UpdateClientConfig(ctx context.Context, config *ClientC
MCPToolExecutionTimeout: config.MCPToolExecutionTimeout,
MCPCodeModeBindingLevel: config.MCPCodeModeBindingLevel,
MCPToolSyncInterval: config.MCPToolSyncInterval,
MCPDisableAutoToolInject: config.MCPDisableAutoToolInject,
AsyncJobResultTTL: config.AsyncJobResultTTL,
RequiredHeaders: config.RequiredHeaders,
LoggingHeaders: config.LoggingHeaders,
Expand Down Expand Up @@ -223,6 +224,7 @@ func (s *RDBConfigStore) GetClientConfig(ctx context.Context) (*ClientConfig, er
MCPToolExecutionTimeout: dbConfig.MCPToolExecutionTimeout,
MCPCodeModeBindingLevel: dbConfig.MCPCodeModeBindingLevel,
MCPToolSyncInterval: dbConfig.MCPToolSyncInterval,
MCPDisableAutoToolInject: dbConfig.MCPDisableAutoToolInject,
AsyncJobResultTTL: dbConfig.AsyncJobResultTTL,
RequiredHeaders: dbConfig.RequiredHeaders,
LoggingHeaders: dbConfig.LoggingHeaders,
Expand Down Expand Up @@ -901,9 +903,10 @@ func (s *RDBConfigStore) GetMCPConfig(ctx context.Context) (*schemas.MCPConfig,
return nil, err
}
toolManagerConfig := schemas.MCPToolManagerConfig{
ToolExecutionTimeout: time.Duration(clientConfig.MCPToolExecutionTimeout) * time.Second,
MaxAgentDepth: clientConfig.MCPAgentDepth,
CodeModeBindingLevel: schemas.CodeModeBindingLevel(clientConfig.MCPCodeModeBindingLevel),
ToolExecutionTimeout: time.Duration(clientConfig.MCPToolExecutionTimeout) * time.Second,
MaxAgentDepth: clientConfig.MCPAgentDepth,
CodeModeBindingLevel: schemas.CodeModeBindingLevel(clientConfig.MCPCodeModeBindingLevel),
DisableAutoToolInject: clientConfig.MCPDisableAutoToolInject,
}
clientConfigs := make([]*schemas.MCPClientConfig, len(dbMCPClients))
for i, dbClient := range dbMCPClients {
Expand Down
1 change: 1 addition & 0 deletions framework/configstore/tables/clientconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type TableClientConfig struct {
MCPToolExecutionTimeout int `gorm:"default:30" json:"mcp_tool_execution_timeout"` // Timeout for individual tool execution in seconds (default: 30)
MCPCodeModeBindingLevel string `gorm:"default:server" json:"mcp_code_mode_binding_level"` // How tools are exposed in VFS: "server" or "tool"
MCPToolSyncInterval int `gorm:"default:10" json:"mcp_tool_sync_interval"` // Global tool sync interval in minutes (default: 10, 0 = disabled)
MCPDisableAutoToolInject bool `gorm:"default:false" json:"mcp_disable_auto_tool_inject"` // When true, MCP tools are not injected into requests by default
AsyncJobResultTTL int `gorm:"default:3600" json:"async_job_result_ttl"` // Default TTL for async job results in seconds (default: 3600 = 1 hour)
RequiredHeadersJSON string `gorm:"type:text" json:"-"` // JSON serialized []string
LoggingHeadersJSON string `gorm:"type:text" json:"-"` // JSON serialized []string
Expand Down
11 changes: 8 additions & 3 deletions transports/bifrost-http/handlers/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type ConfigManager interface {
ReloadPricingManager(ctx context.Context) error
ForceReloadPricing(ctx context.Context) error
UpdateDropExcessRequests(ctx context.Context, value bool)
UpdateMCPToolManagerConfig(ctx context.Context, maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string) error
UpdateMCPToolManagerConfig(ctx context.Context, maxAgentDepth int, toolExecutionTimeoutInSeconds int, codeModeBindingLevel string, disableAutoToolInject bool) error
ReloadPlugin(ctx context.Context, name string, path *string, pluginConfig any, placement *schemas.PluginPlacement, order *int) error
RemovePlugin(ctx context.Context, name string) error
ReloadProxyConfig(ctx context.Context, config *configstoreTables.GlobalProxyConfig) error
Expand Down Expand Up @@ -276,9 +276,14 @@ func (h *ConfigHandler) updateConfig(ctx *fasthttp.RequestCtx) {
shouldReloadMCPToolManagerConfig = true
}

// Only reload MCP tool manager config if MCP is configured
updatedConfig.MCPDisableAutoToolInject = payload.ClientConfig.MCPDisableAutoToolInject
if payload.ClientConfig.MCPDisableAutoToolInject != currentConfig.MCPDisableAutoToolInject {
shouldReloadMCPToolManagerConfig = true
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Reload MCP tool manager config with all current values in one call
if shouldReloadMCPToolManagerConfig && h.store.MCPConfig != nil {
if err := h.configManager.UpdateMCPToolManagerConfig(ctx, updatedConfig.MCPAgentDepth, updatedConfig.MCPToolExecutionTimeout, updatedConfig.MCPCodeModeBindingLevel); err != nil {
if err := h.configManager.UpdateMCPToolManagerConfig(ctx, updatedConfig.MCPAgentDepth, updatedConfig.MCPToolExecutionTimeout, updatedConfig.MCPCodeModeBindingLevel, updatedConfig.MCPDisableAutoToolInject); err != nil {
logger.Warn(fmt.Sprintf("failed to update mcp tool manager config: %v", err))
SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("failed to update mcp tool manager config: %v", err))
return
Expand Down
Loading