fix: stamp virtual key tool allowlist when include-clients filter bypasses autoInjectDisabled

[Pratham-Mishra04]

Summary

When a request includes an include-clients filter, ParseAndAddToolsToRequest in core/mcp opts the request into tool injection even if auto-injection is globally disabled. Previously, the virtual key's tool allowlist was only stamped onto the context when auto-injection was enabled, meaning requests that bypassed the auto-injection gate via include-clients could run without the key's tool restrictions applied.

Changes

• Renamed callerProvided to includeToolsProvided in both runPreRequestRouting and PreRequestHook for clarity.
• Added a check for MCPContextKeyIncludeClients in the context. When this key is present, the virtual key's tool allowlist (computeMCPIncludeTools) is stamped onto the context regardless of whether auto-injection is disabled, ensuring the key's grants are enforced on every code path where injection can occur.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

Send a request with an include-clients filter set and disableAutoToolInject enabled on the governance plugin. Verify that the virtual key's tool allowlist is applied and that tools outside the key's grant are not injected.

go test ./plugins/governance/...

Breaking changes

• Yes
• No

Security considerations

This fix closes a gap where a caller could use an include-clients filter to trigger tool injection while bypassing the virtual key's tool allowlist. The key's grants are now enforced on all injection paths, preventing potential privilege escalation through MCP tool access.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

Summary by CodeRabbit

• Bug Fixes
  • Fixed tool injection so filters that request client-specific tools still trigger tool availability even when automatic injection is disabled.
  • Ensured previous pruning/default behavior remains unchanged while correctly honoring explicit client-filter requests for tool inclusion.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Pratham-Mishra04]

• tests: add PreRequestHook MCP tool-stamping unit tests for governance plugin #4298
• fix: stamp virtual key tool allowlist when include-clients filter bypasses autoInjectDisabled #4297 👈 (View in Graphite)
• feat: enforce VK tool-grant boundary on caller-provided x-bf-mcp-include-tools lists via pruneMCPIncludeToolsFromContext #4296
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: eee79104-b3d1-4e6e-83c7-f3d4113f4133

📥 Commits

Reviewing files that changed from the base of the PR and between 25f7aaa and 7099ba4.

📒 Files selected for processing (1)

• plugins/governance/main.go

---

📝 Walkthrough

Walkthrough

Two pre-request hook functions record whether include-tools were caller-provided and update the auto-injection gate so include-clients filters can trigger stamping of schemas.MCPContextKeyIncludeTools even when disable_auto_tool_inject is set; otherwise stamping still requires auto-injection and absent caller include-tools.

Changes

MCP Include-Tools Auto-Injection Logic

Layer / File(s)	Summary
Include-tools auto-injection with include-clients fallback plugins/governance/main.go	runPreRequestRouting and PreRequestHook now track whether include-tools was caller-provided and allow stamping schemas.MCPContextKeyIncludeTools when MCPContextKeyIncludeClients is present even if disable_auto_tool_inject is enabled; otherwise stamping requires absent caller include-tools and auto-injection enabled.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant GovernancePlugin
  participant Context
  Client->>GovernancePlugin: send request (with/without include-tools)
  GovernancePlugin->>GovernancePlugin: prune include-tools -> set includeToolsProvided
  GovernancePlugin->>Context: read disableAutoToolInject
  GovernancePlugin->>Context: read MCPContextKeyIncludeClients
  GovernancePlugin->>Context: stamp MCPContextKeyIncludeTools (if !includeToolsProvided && (autoInjectAllowed || includeClientsPresent))

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• maximhq/bifrost#4296: Modifies the same runPreRequestRouting and PreRequestHook include-tools/stamping and pruning logic.
• maximhq/bifrost#4176: Related refactor computing and stamping MCP include-tools in PreRequestHook.

Suggested reviewers

• akshaydeo
• danpiths

Poem

🐰
I pruned the tools, then took a hop,
If clients call, I won't stop,
Auto-inject may sleep or hide,
But client filters still decide,
Hooray — stamped keys for every ride!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the specific bug fix: stamping the virtual key tool allowlist when the include-clients filter bypasses autoInjectDisabled, which matches the main change.
Description check	✅ Passed	The description includes all required sections with substantive content: Summary explains the bug, Changes details the modifications, Type of change is marked, Affected areas are checked, How to test is provided, Breaking changes are addressed, and Security considerations explain the fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 06-11-fix_mcp_allowed_clients_governance_fixes

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the change is a minimal two-line guard fix applied consistently in both code paths that call computeMCPIncludeTools.

The compound condition !includeToolsProvided && (!autoInjectDisabled || includeClientsPresent) is correct across all four case combinations: auto-inject enabled (unchanged), auto-inject disabled with no include-clients (unchanged), auto-inject disabled with include-clients present (newly stamped), and caller-provided tool list (unchanged via the leading negation). The computeMCPIncludeTools empty-slice return is non-nil in Go, so it is correctly stamped as a deny-all grant when the VK has no configured tools, which is the right fail-closed behavior. The fix is applied symmetrically in both runPreRequestRouting and PreRequestHook, which cover the large-payload and normal request paths respectively.

No files require special attention.

Important Files Changed

Filename	Overview
plugins/governance/main.go	Corrects the allowlist-stamping guard in both runPreRequestRouting and PreRequestHook to enforce VK tool grants when an include-clients filter is present; logic is correct across all condition combinations.

_{Reviews (3): Last reviewed commit: "fix: mcp allowed clients governance fixe..." | Re-trigger Greptile}

[Pratham-Mishra04]

@greptile

Re P1 (empty slice bypasses the allowlist stamp): This is based on a misreading of the guard. computeMCPIncludeTools initializes its result with make([]string, 0) and only appends — it never returns nil. A non-nil empty slice satisfies tools != nil, so the zero-grant case is stamped onto MCPContextKeyIncludeTools, and downstream shouldSkipToolForRequest treats an empty list as deny-all (len == 0 → every tool skipped). The scenario described (zero-grant VK + include-clients + disableAutoToolInject=true) injects zero tools, not all of them. This is covered by TestPreRequestHookMCP_NoGrants_IncludeClients_StampsDenyAll, which asserts the stamped value is a non-nil empty list on exactly this path. The suggested remediation ("stamp on an empty return value too") is what the code already does.

Re P2 (missing test coverage): Covered in prerequesthookmcp_test.go. Both requested cases exist — TestPreRequestHookMCP_AutoInjectOff_IncludeClientsOnly_StampsGrant (a) and TestPreRequestHookMCP_AutoInjectOff_NoFilters_StampsNothing (b) — plus the same pair for the large-payload runPreRequestRouting site and the full include-tools × include-clients × toggle matrix for both call sites.

[greptile-apps]

You're right on both counts, and I appreciate the thorough walkthrough.

P1: The nil-vs-empty-slice distinction is the crux, and I got it wrong. Since computeMCPIncludeTools initialises with make([]string, 0) and only appends, the return value is always non-nil. A non-nil empty slice satisfies tools != nil, so the stamp happens, and shouldSkipToolForRequest correctly interprets len == 0 as deny-all. My concern about the zero-grant case silently bypassing the stamp was based on a misread of the function's return type contract. The zero-grant path is handled correctly as-is.

P2: I'll take your word on the test coverage — naming specific test functions (TestPreRequestHookMCP_AutoInjectOff_IncludeClientsOnly_StampsGrant, TestPreRequestHookMCP_AutoInjectOff_NoFilters_StampsNothing, and the deny-all case) plus both call-site variants is a concrete enough description that I'm satisfied the matrix is covered. The search index on the PR branch simply didn't surface prerequesthookmcp_test.go.

Both original comments were incorrect. The implementation and test coverage are solid — happy to see this merge.

[akshaydeo]

Merge activity

• Jun 11, 4:21 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jun 11, 4:24 PM UTC: Graphite rebased this pull request as part of a merge.
• Jun 11, 4:25 PM UTC: @akshaydeo merged this pull request with Graphite.