tests: add PreRequestHook MCP tool-stamping unit tests for governance plugin - #4298
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a comprehensive test file validating GovernancePlugin.PreRequestHook MCP include-tools stamping and pruning across DisableAutoToolInject states, caller MCP filters, virtual key presence/state, grant shapes, and large-payload routing. ChangesMCP PreRequestHook test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/prerequesthookmcp_test.go`:
- Around line 251-287: Add unit tests using newLargePayloadCtx,
newPluginForMCPStamping, buildVKForMCPStamping, stampedIncludeTools and
newChatRequest to cover the missing large-payload scenarios: (1) auto-inject ON
+ no filters — assert stampedIncludeTools returns the granted tools (stamp
grant), (2) auto-inject ON + include-clients only — assert stamp grant
(include-clients result like the existing AutoInjectOff case but with
auto-inject true), and (3) both include-tools and include-clients present —
assert the pruned include-tools list wins (pruning behavior like
TestPreRequestHookMCP_LargePayload_IncludeToolsPresent_Prunes). Keep test names
consistent with existing pattern (e.g.,
TestPreRequestHookMCP_LargePayload_AutoInjectOn_NoFilters_StampsGrant,
TestPreRequestHookMCP_LargePayload_AutoInjectOn_IncludeClients_StampsGrant,
TestPreRequestHookMCP_LargePayload_BothFilters_Prunes) and reuse
buildVKForMCPStamping tool lists and newChatRequest for inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd1daf2b-eedf-4393-a8d6-b75bf8832587
📒 Files selected for processing (1)
plugins/governance/prerequesthookmcp_test.go
Confidence Score: 5/5Test-only addition that does not modify production code; all new tests follow existing helper patterns and the production behavior they exercise has been verified against the implementation. Every test case was traced against the PreRequestHook and runPreRequestRouting implementations — expected values are correct, the VK lifecycle and auto-inject toggle are exercised through the real plugin stack. Previous review concerns about t.Run subtest names, metadata.Model assertions, and explicit-empty-grant coverage are all addressed. No files require special attention — both coverage suggestions are additive and do not indicate correctness issues with the existing tests. Important Files Changed
Reviews (2): Last reviewed commit: "tests: mcp tools filtering tests for gov..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
a42f33e to
fde0b89
Compare
21ace8c to
25f7aaa
Compare
fde0b89 to
a5a841d
Compare
Merge activity
|
…ce plugin (#4298) ## Summary Adds a dedicated test file covering the MCP tool-stamping logic inside `PreRequestHook` (and the large-payload routing path). The tests verify that the governance plugin correctly enforces virtual key MCP grants — stamping allowed tools onto the request context, pruning caller-supplied tool lists to the granted ceiling, and denying access when no grants exist or the VK is inactive/absent. ## Changes - Added `prerequesthookmcp_test.go` with a full decision-matrix test suite covering: - Auto-inject enabled/disabled × include-tools present/absent × include-clients present/absent - Grant-shape edge cases: no grants, wildcard grants, ungranted client tools, and empty include-tools headers - Paths that must not stamp: no virtual key on context, inactive VK, and passthrough requests - Large-payload branch (`runPreRequestRouting`) applying the same stamping rules ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... -run TestPreRequestHookMCP ``` All tests should pass with no failures. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations These tests exercise the MCP tool grant enforcement path, which is a security boundary — ensuring callers cannot access tools beyond what their virtual key permits and that inactive or missing VKs cannot be used to bypass grant checks. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded test coverage for governance behavior around automatic tool injection and caller-provided tool filters. * Verifies stamping, pruning, and deny-all outcomes across active/inactive contexts, unrestricted grants, and ungranted tool requests. * Adds scenarios for empty filter values and large-payload routing while confirming routed model metadata remains unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary
Adds a dedicated test file covering the MCP tool-stamping logic inside
PreRequestHook(and the large-payload routing path). The tests verify that the governance plugin correctly enforces virtual key MCP grants — stamping allowed tools onto the request context, pruning caller-supplied tool lists to the granted ceiling, and denying access when no grants exist or the VK is inactive/absent.Changes
prerequesthookmcp_test.gowith a full decision-matrix test suite covering:runPreRequestRouting) applying the same stamping rulesType of change
Affected areas
How to test
go test ./plugins/governance/... -run TestPreRequestHookMCPAll tests should pass with no failures.
Breaking changes
Related issues
Security considerations
These tests exercise the MCP tool grant enforcement path, which is a security boundary — ensuring callers cannot access tools beyond what their virtual key permits and that inactive or missing VKs cannot be used to bypass grant checks.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit