feat(mcp): filter allowed tools by readOnlyHint#1425
Conversation
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
RohanSogani
requested review from
CatherineSue,
key4ng,
slin1237,
zhaowenzi and
zhoug9127
as code owners
github-actions
Bot
added
mcp
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant MCP Utils
participant Session
Client->>Router: Request with McpAllowedTools
Router->>MCP Utils: project_allowed_tools(allowed_tools)
MCP Utils-->>Router: McpToolExposureFilter {tool_names?, read_only?}
Router->>Session: McpServerInput {allowed_tools: Filter}
Session->>Session: Index filters by server<br/>HashMap<&str, &McpToolExposureFilter>
Session->>Session: matches_allowed_tool_filter()<br/>for each tool from server
Note over Session: Validate read_only status<br/>and tool name membership
Session-->>Router: Filtered tool inventory
Router-->>Client: Response with filtered tools
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 41 minutes and 6 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585718fbd0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the basic tool allowlist in MCP server bindings with a more flexible McpToolExposureFilter, enabling tool filtering based on both names and read_only annotations. Changes span the core MCP session logic, gateway projection utilities, and associated tests. Feedback highlights a performance regression in the tool matching logic where HashSet allocations occur repeatedly within a loop; it is recommended to pre-calculate these sets to improve efficiency.
![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 the current code and only fix it if needed.
Inline comments:
In `@crates/mcp/src/core/session.rs`:
- Around line 1397-1476: Add a unit test that covers alias entries when both
name filters and read_only annotations are applied: create a ToolEntry alias
(e.g., via whatever helper creates alias entries) whose target tool is annotated
read_only(true) and also include that alias name in McpToolExposureFilter names;
then construct a McpToolSession (as in tests using McpOrchestrator and
McpServerBinding) and assert that
matches_allowed_tool_filter()/session.mcp_tools()/session.has_exposed_tool()
allow the alias only when the target’s read_only annotation permits it. This
ensures the alias inventory path correctly consults ToolAnnotations.read_only
and the name-matching logic in matches_allowed_tool_filter().
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 969e74e6-ad27-48f8-bd01-900894fe236c
📒 Files selected for processing (6)
crates/mcp/src/core/mod.rscrates/mcp/src/core/session.rscrates/mcp/src/lib.rsmodel_gateway/src/routers/anthropic/router.rsmodel_gateway/src/routers/common/mcp_utils.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Hi @RohanSogani, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
1 participant
Description
Problem
The Responses API
allowed_toolsfield on MCP tools was not fully applied before exposing MCP tools to the model.The array form should restrict model-visible MCP tools by name:
{ "type": "mcp", "server_url": "...", "allowed_tools": ["tool_a", "tool_b"] }The Responses API also supports the filter object form:
{ "allowed_tools": { "read_only": true, "tool_names": ["tool_a"] } }Before this change, SMG could parse the object form, but the runtime MCP session only carried a flat list of tool names. Any read_only restriction was projected to an empty allowlist because readOnlyHint-
backed filtering was not implemented.
Resolves #163.
Solution
Add a generic MCP tool exposure filter that preserves both tool_names and read_only.
McpToolSession now filters the MCP tool inventory using both constraints:
Missing or false readOnlyHint is treated conservatively as not read-only.
This keeps the implementation generic and does not add any provider-specific or OCI-specific behavior.
Changes
Test Plan
Repro from repo root:
cargo +nightly fmt --all -- --check
cargo check -p smg-mcp -p smg
cargo test -p smg-mcp allowed_tools -- --nocapture
cargo test -p smg project_allowed_tools -- --nocapture
Verified:
Checklist
Summary by CodeRabbit
New Features
Chores