feat: add Use() method for attaching tool middleware at runtime#767
Conversation
The existing WithToolHandlerMiddleware option only works at server construction time. Use() lets callers attach middleware after the server is created, matching the net/http pattern. Also apply the middleware chain in executeRegularToolAsTask so that hybrid-mode (TaskSupportOptional) tool calls go through the same middleware as synchronous ones. Signed-off-by: majiayu000 <1835304752@qq.com>
Verifies that Use() middleware runs when a TaskSupportOptional tool is called with a task param, routing through executeRegularToolAsTask. Signed-off-by: majiayu000 <1835304752@qq.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a runtime middleware registration method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
|
@majiayu000 Can you add documentation in the www dir that demonstrates this? |
Signed-off-by: majiayu000 <1835304752@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
www/docs/pages/servers/middleware.mdx (3)
162-170: Consider adding a brief explanation ofWithRecovery().The section mentions
server.WithRecovery()but doesn't explain what it does. Adding a brief note would help readers understand its purpose:-MCP-Go includes built-in recovery middleware that catches panics in tool handlers: +MCP-Go includes built-in recovery middleware that catches panics in tool handlers and returns an error response instead of crashing the server:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/docs/pages/servers/middleware.mdx` around lines 162 - 170, Add a one-sentence explanation of server.WithRecovery() next to the example so readers know its purpose: state that WithRecovery() installs built-in recovery middleware on the MCP-Go server to catch panics in tool handlers and convert them into safe error responses (preventing process crashes and logging the error); place this sentence near the server.NewMCPServer(...) snippet referencing WithRecovery() so the intent and behavior are clear.
103-103: Optional: Consider hyphenating compound adjective.The static analysis tool suggests using "Rate-Limiting Middleware" (hyphenated) when "Rate Limiting" functions as a compound adjective modifying "Middleware." This follows standard English grammar conventions for compound adjectives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/docs/pages/servers/middleware.mdx` at line 103, Update the header text "Rate Limiting Middleware" to use a hyphenated compound adjective by changing it to "Rate-Limiting Middleware" in the document; locate the header string "### Rate Limiting Middleware" and replace it with "### Rate-Limiting Middleware".
47-132: Consider adding import statements and helper definitions for clarity.The middleware examples are technically correct but could be enhanced:
- The rate limiting example (lines 108-132) uses
server.GetSessionID(ctx)andrate.NewLimiterwithout showing imports (golang.org/x/time/rate)- The auth example (lines 85-100) references
extractToken,validateToken, anduserKeywithout definitionsWhile these are acceptable for documentation, adding brief comments or a note about required imports would improve clarity for readers copying the examples.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/docs/pages/servers/middleware.mdx` around lines 47 - 132, Add missing import notes and small helper definitions referenced by the examples: mention the required imports (e.g., "context", "log", "fmt", and "golang.org/x/time/rate") and clarify helper symbols used in the auth and rate-limit examples; specifically document or stub extractToken, validateToken, and userKey referenced by authMiddleware, and call out server.GetSessionID and rate.NewLimiter used by rateLimiter/newRateLimiter/middleware so readers know to provide or import those helpers before copying the examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@www/docs/pages/servers/middleware.mdx`:
- Around line 147-160: Clarify middleware scope: update the docs to state that
middleware registered via Use() is applied to calls handled by handleCallTool
and executeRegularToolAsTask (i.e., regular tools invoked via the task path) but
is NOT applied to native task tools handled by executeTaskTool or
TaskToolHandlerFunc; explicitly mention this distinction in the paragraph around
executeRegularToolAsTask and replace the blanket "applies to both regular and
task-routed calls" line with a note that only regular tools routed through the
task executor receive middleware, while native task tools do not.
---
Nitpick comments:
In `@www/docs/pages/servers/middleware.mdx`:
- Around line 162-170: Add a one-sentence explanation of server.WithRecovery()
next to the example so readers know its purpose: state that WithRecovery()
installs built-in recovery middleware on the MCP-Go server to catch panics in
tool handlers and convert them into safe error responses (preventing process
crashes and logging the error); place this sentence near the
server.NewMCPServer(...) snippet referencing WithRecovery() so the intent and
behavior are clear.
- Line 103: Update the header text "Rate Limiting Middleware" to use a
hyphenated compound adjective by changing it to "Rate-Limiting Middleware" in
the document; locate the header string "### Rate Limiting Middleware" and
replace it with "### Rate-Limiting Middleware".
- Around line 47-132: Add missing import notes and small helper definitions
referenced by the examples: mention the required imports (e.g., "context",
"log", "fmt", and "golang.org/x/time/rate") and clarify helper symbols used in
the auth and rate-limit examples; specifically document or stub extractToken,
validateToken, and userKey referenced by authMiddleware, and call out
server.GetSessionID and rate.NewLimiter used by
rateLimiter/newRateLimiter/middleware so readers know to provide or import those
helpers before copying the examples.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16ade903-c597-4753-897a-095a2e6b919f
📒 Files selected for processing (2)
www/docs/pages/servers/middleware.mdxwww/vocs.config.ts
✅ Files skipped from review due to trivial changes (1)
- www/vocs.config.ts
- Clarify middleware scope: Use() applies to regular tools and regular tools executed via the task path, but not to native task tools - Expand WithRecovery() description - Add import statements and helper notes to examples - Hyphenate "Rate-Limiting Middleware" Signed-off-by: majiayu000 <1835304752@qq.com>
Description
Fixes #729
Adds
Use()toMCPServerso middleware can be attached after server construction. The existingWithToolHandlerMiddlewareoption only works at init time, which doesn't fit use cases where middleware needs to be registered dynamically (e.g., per-tenant auth, conditional logging).Two changes:
Use(mw ...ToolHandlerMiddleware)onMCPServer— acquires the write lock, appends totoolHandlerMiddlewares, done. Variadic so you can chain multiple in one call.executeRegularToolAsTaskwas bypassing middleware entirely for tools withTaskSupportOptional. Applied the same middleware chain thathandleToolCallalready uses, so hybrid-mode task execution isn't a silent gap.Type of Change
Checklist
Additional Information
Tests added:
TestMCPServer_Use_AppliesMiddlewareToToolCall— middleware runs on a normal tool callTestMCPServer_Use_MultipleMiddlewaresExecuteInOrder— ordering matchesnet/httpconvention (outermost first)TestMCPServer_Use_MiddlewareAppliedToRegularToolAsTask— covers theexecuteRegularToolAsTaskpathTestMCPServer_Use_ConcurrentSafe— pass-race, no data racesSummary by CodeRabbit