feat: add MCP auto tool injection toggle#1933
Conversation
14 tasks
Collaborator
Author
|
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. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request adds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
framework/configstore/migrations.go (1)
4195-4200: Make rollback idempotent with a column-existence guard.
Rollbackcurrently drops the column unconditionally. Guarding withHasColumnimproves resilience across partial rollback or branch-skew scenarios.♻️ Suggested hardening
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 + if migratorInstance.HasColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject") { + if err := migratorInstance.DropColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject"); err != nil { + return err + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 4195 - 4200, The Rollback closure currently calls migratorInstance.DropColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject") unconditionally; make it idempotent by first checking migratorInstance.HasColumn(&tables.TableClientConfig{}, "mcp_disable_auto_tool_inject") and only calling DropColumn when HasColumn returns true, ensuring you still propagate errors from HasColumn and DropColumn and preserving the existing tx.WithContext(ctx) and migratorInstance usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 3378-3380: Change the error string returned in the nil check for
bifrost.MCPManager (the block that returns fmt.Errorf when bifrost.MCPManager ==
nil) to follow Go conventions: make the message all lowercase with no trailing
punctuation (e.g., "mcp is not configured in this bifrost instance") so
fmt.Errorf produces a compliant error string.
In `@ui/app/workspace/config/views/mcpView.tsx`:
- Around line 188-193: Add a data-testid to the new Switch control so it follows
the workspace UI selector convention; in the Switch component with id
"mcp-disable-auto-tool-inject" (the one using
checked={localConfig.mcp_disable_auto_tool_inject ?? false},
onCheckedChange={handleDisableAutoToolInjectChange},
disabled={!hasSettingsUpdateAccess}), add a prop data-testid using the pattern
"<entity>-<element>-<qualifier>" (e.g., "mcp-switch-disable-auto-tool-inject" or
"mcp-disable-auto-tool-inject-switch") to make the element targetable by tests.
---
Nitpick comments:
In `@framework/configstore/migrations.go`:
- Around line 4195-4200: The Rollback closure currently calls
migratorInstance.DropColumn(&tables.TableClientConfig{},
"mcp_disable_auto_tool_inject") unconditionally; make it idempotent by first
checking migratorInstance.HasColumn(&tables.TableClientConfig{},
"mcp_disable_auto_tool_inject") and only calling DropColumn when HasColumn
returns true, ensuring you still propagate errors from HasColumn and DropColumn
and preserving the existing tx.WithContext(ctx) and migratorInstance usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4db42427-dfea-4af6-9e93-224a715157fe
📒 Files selected for processing (20)
core/bifrost.gocore/changelog.mdcore/mcp/interface.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mcp.goframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/changelog.mdtransports/config.schema.jsontransports/schema_test/config_schema_test.goui/app/workspace/config/views/mcpView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
Pratham-Mishra04
force-pushed
the
03-05-feat_add_option_to_disable_automatic_mcp_tool_injection_per_request
branch
from
a26bb7f to
b515efc
Compare
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
9ee898b to
8789ae3
Compare
18 tasks
Pratham-Mishra04
force-pushed
the
03-05-feat_add_option_to_disable_automatic_mcp_tool_injection_per_request
branch
from
b515efc to
1ee6d71
Compare
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
8789ae3 to
ea84c03
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_add_option_to_disable_automatic_mcp_tool_injection_per_request
branch
from
1ee6d71 to
57671fd
Compare
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
ea84c03 to
2d9d2e9
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_add_option_to_disable_automatic_mcp_tool_injection_per_request
branch
from
57671fd to
dae2e01
Compare
Pratham-Mishra04
force-pushed
the
02-18-refactor_standardize_empty_array_conventions_in_virtual_key
branch
from
2d9d2e9 to
c0dfed9
Compare
Pratham-Mishra04
force-pushed
the
03-05-feat_add_option_to_disable_automatic_mcp_tool_injection_per_request
branch
from
dae2e01 to
fc9336c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/mcp/toolmanager.go`:
- Around line 674-675: UpdateConfig is unconditionally calling
m.disableAutoToolInject.Store(config.DisableAutoToolInject), which allows
partial/patch updates to inadvertently flip DisableAutoToolInject; change the
config contract so presence is explicit (make DisableAutoToolInject a *bool or
add a DisableAutoToolInjectSet bool) and then in UpdateConfig only call
m.disableAutoToolInject.Store(...) when that presence flag/pointer is
non-nil/true; adjust callers to supply the new presence marker or pointer as
needed so partial updates won't reset m.disableAutoToolInject inadvertently.
- Around line 301-302: The context key lookups in toolmanager.go are using
undefined bare identifiers MCPContextKeyIncludeTools and
MCPContextKeyIncludeClients; update the read paths to use the same fully
qualified keys used by writers by calling
ctx.Value(schemas.MCPContextKeyIncludeTools) and
ctx.Value(schemas.MCPContextKeyIncludeClients) where the current ctx.Value(...)
calls occur (locate the ctx.Value usages in the function/method that references
includeTools/includeClients) so the keys match the schemas package constants
used by transports, handlers, and tests.
In `@transports/changelog.md`:
- Line 6: The changelog contains a duplicate entry "fix: preserve original audio
filename in transcription requests" — remove the repeated line (the duplicate
changelog item) so the file only contains a single instance of that entry;
search for the exact string "fix: preserve original audio filename in
transcription requests" and delete the redundant occurrence to keep release
notes unique.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5645c5d-afdd-4cc0-a6ac-24865582ee10
📒 Files selected for processing (21)
core/bifrost.gocore/changelog.mdcore/mcp/interface.gocore/mcp/mcp.gocore/mcp/toolmanager.gocore/schemas/mcp.goframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/changelog.mdtransports/config.schema.jsontransports/schema_test/config_schema_test.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/logs/sheets/logDetailsSheet.tsxui/lib/types/config.tsui/lib/types/schemas.ts
Comment on lines
+301
to
+302
| includeTools := ctx.Value(MCPContextKeyIncludeTools) | ||
| includeClients := ctx.Value(MCPContextKeyIncludeClients) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Key definitions =="
rg -n 'MCPContextKeyInclude(Tools|Clients)' --type go -B2 -A2
echo
echo "== context.WithValue write sites =="
rg -n 'context\.WithValue.*MCPContextKeyInclude' --type go -B2 -A2
echo
echo "== ctx.Value read sites =="
rg -n '\.Value\(.*MCPContextKeyInclude' --type go -B2 -A2Repository: maximhq/bifrost
Length of output: 19317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check toolmanager.go for key definitions =="
rg -n '^const|^var|MCPContextKey' core/mcp/toolmanager.go | head -30
echo
echo "== Full context around toolmanager.go lines 301-302 =="
sed -n '295,310p' core/mcp/toolmanager.goRepository: maximhq/bifrost
Length of output: 755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check toolmanager.go imports =="
head -50 core/mcp/toolmanager.go | grep -E '^import|^const|^var'
echo
echo "== Search for where these unqualified identifiers should come from =="
rg -n 'const.*MCPContextKey' core/mcp/
echo
echo "== Check if toolmanager.go has any const block at package level =="
sed -n '1,100p' core/mcp/toolmanager.go | grep -A20 'const ('
</parameter>Repository: maximhq/bifrost
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full imports section in toolmanager.go =="
sed -n '1,30p' core/mcp/toolmanager.go
echo
echo "== Check if schemas is imported =="
rg -n 'import|schemas' core/mcp/toolmanager.go | head -20
</parameter>Repository: maximhq/bifrost
Length of output: 2528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify the symbols are undefined in toolmanager.go =="
# Check if these unqualified symbols appear anywhere else in toolmanager.go
rg -c 'MCPContextKeyIncludeTools|MCPContextKeyIncludeClients' core/mcp/toolmanager.go
echo
echo "== Confirm key definitions are only in schemas =="
rg -n 'const.*MCPContextKeyIncludeTools|const.*MCPContextKeyIncludeClients' core/schemas/
</parameter>Repository: maximhq/bifrost
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find exact key definition lines =="
rg -n 'MCPContextKeyInclude' core/schemas/bifrost.go
echo
echo "== Verify no const definition of these unqualified names exist =="
grep -r 'const.*MCPContextKeyInclude' core/mcp/
echo
echo "== Check if there's a package-level const block in toolmanager.go =="
sed -n '30,150p' core/mcp/toolmanager.go | grep -A50 'const'
</parameter>Repository: maximhq/bifrost
Length of output: 515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if toolmanager.go has build constraints =="
head -5 core/mcp/toolmanager.go
echo
echo "== Verify these unqualified names are truly undefined by checking all occurrences =="
rg -n 'MCPContextKeyInclude(Tools|Clients)' core/mcp/toolmanager.go -B3 -A3
echo
echo "== Search entire codebase for const definitions of unqualified MCPContextKey =="
rg -n '^const.*MCPContextKey' core/
</parameter>Repository: maximhq/bifrost
Length of output: 735
Fix context key references in toolmanager.go to use fully qualified names matching write paths.
Lines 301-302 use undefined identifiers MCPContextKeyIncludeTools and MCPContextKeyIncludeClients. All write paths (transports, handlers, tests) set these via schemas.MCPContextKeyIncludeTools and schemas.MCPContextKeyIncludeClients, but the read path here uses unqualified names that won't match. This breaks context filter detection when auto-inject is disabled.
Change to:
ctx.Value(schemas.MCPContextKeyIncludeTools)ctx.Value(schemas.MCPContextKeyIncludeClients)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/mcp/toolmanager.go` around lines 301 - 302, The context key lookups in
toolmanager.go are using undefined bare identifiers MCPContextKeyIncludeTools
and MCPContextKeyIncludeClients; update the read paths to use the same fully
qualified keys used by writers by calling
ctx.Value(schemas.MCPContextKeyIncludeTools) and
ctx.Value(schemas.MCPContextKeyIncludeClients) where the current ctx.Value(...)
calls occur (locate the ctx.Value usages in the function/method that references
includeTools/includeClients) so the keys match the schemas package constants
used by transports, handlers, and tests.
Comment on lines
+674
to
+675
| m.disableAutoToolInject.Store(config.DisableAutoToolInject) | ||
|
|
Contributor
There was a problem hiding this comment.
UpdateConfig can unintentionally reset DisableAutoToolInject.
Line 674 always writes config.DisableAutoToolInject, while the rest of UpdateConfig behaves like a partial/patch update. A partial call that only intends to change timeout/depth can silently flip this flag to false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/mcp/toolmanager.go` around lines 674 - 675, UpdateConfig is
unconditionally calling
m.disableAutoToolInject.Store(config.DisableAutoToolInject), which allows
partial/patch updates to inadvertently flip DisableAutoToolInject; change the
config contract so presence is explicit (make DisableAutoToolInject a *bool or
add a DisableAutoToolInjectSet bool) and then in UpdateConfig only call
m.disableAutoToolInject.Store(...) when that presence flag/pointer is
non-nil/true; adjust callers to supply the new presence marker or pointer as
needed so partial updates won't reset m.disableAutoToolInject inadvertently.
| - fix: async jobs stuck in "processing" on marshal failure now correctly transition to "failed" | ||
| - feat: adds attachment support in Maxim plugin | ||
| - feat: add option to disable automatic MCP tool injection per request | ||
| - fix: preserve original audio filename in transcription requests |
Contributor
There was a problem hiding this comment.
Remove duplicate changelog item.
Line 6 repeats an existing changelog entry already present earlier in this file, so it should be removed to avoid duplicate release notes.
✂️ Proposed fix
- feat: add option to disable automatic MCP tool injection per request
-- fix: preserve original audio filename in transcription requests📝 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.
Suggested change
| - fix: preserve original audio filename in transcription requests |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/changelog.md` at line 6, The changelog contains a duplicate entry
"fix: preserve original audio filename in transcription requests" — remove the
repeated line (the duplicate changelog item) so the file only contains a single
instance of that entry; search for the exact string "fix: preserve original
audio filename in transcription requests" and delete the redundant occurrence to
keep release notes unique.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new configuration option
DisableAutoToolInjectto the MCP (Model Context Protocol) system that allows disabling automatic tool injection into requests. When enabled, MCP tools are only included when explicitly requested via context headers or filters, providing more granular control over tool availability.Changes
DisableAutoToolInjectfield toMCPToolManagerConfigschema with runtime update supportToolsManagerto safely handle concurrent accessParseAndAddToolsToRequestto respect the disable flag and only inject tools when explicit context filters are presentUpdateMCPDisableAutoToolInjectmethods across the stackType of change
Affected areas
How to test
Validate the new MCP auto tool injection toggle:
Test the feature:
x-bf-mcp-include-toolsheader - tools should be injectedScreenshots/Recordings
UI changes include a new toggle switch in the MCP configuration view with descriptive text explaining when tools are injected based on explicit headers.
Breaking changes
This is a backward-compatible addition with a default value of
false(auto injection enabled).Related issues
This addresses the need for more granular control over MCP tool injection behavior in request processing.
Security considerations
The feature provides better control over tool exposure by allowing administrators to require explicit opt-in for tool injection, potentially reducing unintended tool access.
Checklist
docs/contributing/README.mdand followed the guidelines