feat(router): allow custom MCP tool names via @mcpTool directive #2433
Conversation
shamashel
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughAdds per-operation MCP tool naming via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-10-01T20:39:16.113ZApplied to files:
📚 Learning: 2025-08-20T22:13:25.222ZApplied to files:
🧬 Code graph analysis (1)router-tests/mcp_test.go (2)
🔇 Additional comments (3)
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 |
![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
🧹 Nitpick comments (1)
router/pkg/schemaloader/loader.go (1)
258-270: Consider using the constant for consistency.Line 265 uses the hardcoded string
"mcpTool"while line 205 definesmcpDirectiveNameas a byte slice. For consistency and maintainability, consider using the constant.🔎 Proposed fix
func stripMCPDirective(doc *ast.Document) { for _, ref := range doc.RootNodes { if ref.Kind == ast.NodeKindOperationDefinition { opDef := &doc.OperationDefinitions[ref.Ref] if !opDef.HasDirectives { return } - opDef.Directives.RemoveDirectiveByName(doc, "mcpTool") + opDef.Directives.RemoveDirectiveByName(doc, string(mcpDirectiveName)) opDef.HasDirectives = len(opDef.Directives.Refs) > 0 return } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
router-tests/mcp_test.gorouter-tests/testdata/mcp_operations/CustomToolName.graphqlrouter/pkg/mcpserver/server.gorouter/pkg/schemaloader/loader.gorouter/pkg/schemaloader/loader_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/mcp_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/mcp_test.go
🧬 Code graph analysis (1)
router/pkg/schemaloader/loader_test.go (1)
router/pkg/schemaloader/loader.go (2)
NewOperationLoader(39-44)Operation(19-28)
🔇 Additional comments (9)
router-tests/testdata/mcp_operations/CustomToolName.graphql (1)
1-11: LGTM!Clean test fixture demonstrating the custom MCP tool name feature. The query correctly uses the
@mcpTool(name: "get_employee_by_id")directive and will be used by integration tests to verify tool discovery and naming.router/pkg/mcpserver/server.go (1)
539-546: LGTM!The tool name resolution logic correctly prioritizes the custom
op.ToolNamefrom the@mcpTooldirective, with an appropriate fallback to the default naming convention (execute_operation_<snake_case_name>).router/pkg/schemaloader/loader_test.go (2)
169-211: LGTM!Well-structured table-driven tests covering key scenarios:
- Directive with name argument
- No directive
- Directive without name argument
- Different directive
- Directive with other arguments but no name
Good edge case coverage for the
extractMCPToolNamefunction.
213-271: LGTM!Good integration test that validates the complete loading flow with MCP directives. The test correctly verifies that:
- Operations with
@mcpTool(name: "...")haveToolNameset- Operations without the directive have an empty
ToolNamerouter-tests/mcp_test.go (2)
41-55: LGTM!The refactored assertion properly validates the
get_operation_infotool by:
- Finding it by name in the tools list
- Verifying the description
- Checking the
operationNameenum contains all three expected operations including the newCustomNamedQuery
192-220: LGTM!Excellent test coverage for the custom tool name feature:
- Verifies the custom-named tool
get_employee_by_idexists and has the correct description- Negative assertion ensures the default-generated name
execute_operation_custom_named_queryis not registered, confirming the custom name takes precedencerouter/pkg/schemaloader/loader.go (3)
208-228: LGTM!The custom validator correctly omits
KnownDirectivesvalidation to allow the@mcpTooldirective without requiring it to be defined in the schema. All other standard validation rules are appropriately included.
230-256: LGTM!The
extractMCPToolNamefunction correctly handles all edge cases:
- Returns empty string when no directives exist
- Returns empty string when
@mcpTooldirective is not found- Returns empty string when
nameargument is missing- Returns empty string for non-string argument values
- Only processes the first operation definition (appropriate given the single-operation-per-file constraint)
95-97: LGTM!Correct ordering: extracting the description and tool name before stripping the directive ensures the directive data is available for extraction, then the directive is removed before validation.
|
First contribution, let me know if there's a process I missed |
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shamashel. * #2433 (comment) The following files were modified: * `router/pkg/schemaloader/loader.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/schemaloader/loader.go (1)
258-270: Consider using the constant for directive name consistency.Line 265 uses the string literal
"mcpTool"directly, whileextractMCPToolNameuses themcpDirectiveNameconstant. Using the constant consistently would prevent potential drift if the directive name changes.🔎 Proposed fix
func stripMCPDirective(doc *ast.Document) { for _, ref := range doc.RootNodes { if ref.Kind == ast.NodeKindOperationDefinition { opDef := &doc.OperationDefinitions[ref.Ref] if !opDef.HasDirectives { return } - opDef.Directives.RemoveDirectiveByName(doc, "mcpTool") + opDef.Directives.RemoveDirectiveByName(doc, string(mcpDirectiveName)) opDef.HasDirectives = len(opDef.Directives.Refs) > 0 return } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/schemaloader/loader.go
🔇 Additional comments (5)
router/pkg/schemaloader/loader.go (5)
18-28: LGTM on struct extension.The new
ToolNamefield correctly extends theOperationstruct to carry the custom MCP tool name. Consider adding a brief doc comment to clarify its purpose and relationship to the@mcpTooldirective.
95-100: Good ordering of extraction and validation.The flow correctly extracts the tool name and description before stripping the directive, then validates the cleaned document. This ensures the directive doesn't cause validation failures while still capturing its data.
208-228: Custom validator correctly excludesKnownDirectives().The validator intentionally omits
KnownDirectives()to allow the@mcpTooldirective which isn't defined in the GraphQL schema. All other standard validation rules are appropriately included.
204-206: LGTM on directive constants.Using package-level
varfor byte slices is the correct Go idiom sinceconstdoesn't support slice types.
230-256: Fallback handling for empty tool names is correctly implemented.The function extracts the tool name correctly, and downstream code at
router/pkg/mcpserver/server.go(lines 541–546) properly falls back to defaultexecute_operation_<snake_case>naming whenToolNameis empty or missing.
The custom newMCPOperationValidator was unnecessary because stripMCPDirective removes the @mcptool directive before validation runs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router/pkg/schemaloader/loader.go (2)
205-206: Consider using string constants instead of byte slice variables.While the current approach works, declaring these as
conststrings and converting to[]byteat call sites would make them truly immutable and align with Go best practices for constant values.🔎 Optional refactoring
-var mcpDirectiveName = []byte("mcpTool") -var mcpNameArgument = []byte("name") +const mcpDirectiveName = "mcpTool" +const mcpNameArgument = "name"Then update call sites to convert when needed:
- directiveRef, exists := doc.DirectiveWithNameBytes(opDef.Directives.Refs, mcpDirectiveName) + directiveRef, exists := doc.DirectiveWithNameBytes(opDef.Directives.Refs, []byte(mcpDirectiveName))And:
- opDef.Directives.RemoveDirectiveByName(doc, string(mcpDirectiveName)) + opDef.Directives.RemoveDirectiveByName(doc, mcpDirectiveName)
209-235: Consider logging a warning for invalid @mcptool directive arguments.The function silently returns an empty string when the directive exists but has a non-string argument (line 227-231). While the fallback to default naming is safe, a warning log would help developers identify misconfigured directives during development.
🔎 Optional enhancement
value, argExists := doc.DirectiveArgumentValueByName(directiveRef, mcpNameArgument) if !argExists { return "" } if value.Kind == ast.ValueKindString { return doc.StringValueContentString(value.Ref) } + // Log warning: directive exists but argument is not a string + // (would require passing logger to this function) return ""Note: This would require passing the logger to
extractMCPToolName, which may not be worth the added complexity for this edge case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/schemaloader/loader.go
🔇 Additional comments (2)
router/pkg/schemaloader/loader.go (2)
21-21: LGTM! Clean implementation of custom tool name extraction.The new
ToolNamefield and its extraction flow are well-structured. Extracting the tool name and description before stripping the directive ensures the metadata is preserved, and the pre-validation ordering allows the default validator to be used without modification.Also applies to: 95-97, 125-125, 130-130
238-250: LGTM! Directive stripping logic is sound.The function correctly removes the
@mcpTooldirective before validation and updates theHasDirectivesflag appropriately. The early returns are safe given the single-operation constraint enforced byparseOperation.
1 participant
Closes #2308
Adds support for defining custom MCP tool names on GraphQL operations using the
@mcpTool(name: "...")directive:Without the directive, tools continue using the default
execute_operation_<snake_case_name>naming convention.Checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.