Skip to content

Conversation

@shamashel
Copy link

@shamashel shamashel commented Jan 5, 2026

Closes #2308

Adds a configuration option omit_tool_name_prefix to remove the execute_operation_ prefix from MCP tool names:

mcp:
  enabled: true
  omit_tool_name_prefix: true  # GetUser → get_user (instead of execute_operation_get_user)

Environment variable: MCP_OMIT_TOOL_NAME_PREFIX

This is an alternative approach to #2433 (directive-based), suggested by ahmed in Slack. The config-based approach is simpler and provides backwards compatibility via the false default.

Collision Detection: If an operation name would collide with a built-in MCP tool (get_schema, execute_graphql, get_operation_info), the prefix is automatically retained and a warning is logged.

Changes

File Change
router/pkg/config/config.go Add OmitToolNamePrefix field to MCPConfiguration
router/pkg/config/config.schema.json Add omit_tool_name_prefix schema property
router/pkg/config/fixtures/full.yaml Add example
router/pkg/mcpserver/server.go Add option, With func, conditional tool name logic, collision detection
router/core/router.go Wire config to mcpserver
router-tests/mcp_test.go Add integration tests including collision detection
router-tests/testenv/testenv.go Add MCPOperationsPath config for custom test fixtures
router-tests/testdata/mcp_operations_collision/ Test fixtures for collision detection
testdata/*.json Update golden fixtures

Checklist

Summary by CodeRabbit

  • New Features

    • Option to omit the "execute_operation_" prefix from generated MCP tool names; collisions fallback to prefixed names.
  • Configuration

    • Configuration schema and defaults updated to expose the omit-tool-name-prefix option.
    • MCP operations storage path made configurable with a sensible default.
  • Tests

    • Added tests covering listing, executing, and collision behavior when short tool names are enabled; new test operations included.

✏️ Tip: You can customize this high-level summary in your review settings.

Add a boolean configuration option to omit the 'execute_operation_' prefix
from MCP tool names. When enabled, an operation named 'GetUser' generates
tool name 'get_user' instead of 'execute_operation_get_user'.

Closes wundergraph#2308
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Walkthrough

Adds an MCP option to omit the "execute_operation_" prefix, wires it through config, schema, fixtures, and router bootstrap, implements prefix-omission with collision fallback in the MCP server, adds tests for short-name listing/execution and collision, and makes testenv MCP operations path configurable.

Changes

Cohort / File(s) Summary
Tests
router-tests/mcp_test.go
Add subtests for OmitToolNamePrefix: verify listing omits execute_operation_ prefix, executing by short name, and collision handling where built-in names force prefixed fallback.
Router bootstrap
router/core/router.go
Wire new MCP option via WithOmitToolNamePrefix(r.mcp.OmitToolNamePrefix) into Router.bootstrap.
MCP server implementation
router/pkg/mcpserver/server.go
Add OmitToolNamePrefix option and omitToolNamePrefix state, WithOmitToolNamePrefix constructor, prefer snake_cased short tool names when enabled, detect collisions with built-ins and fall back to execute_operation_ prefixed names (logs warning).
Configuration & schema
router/pkg/config/config.go, router/pkg/config/config.schema.json, router/pkg/config/fixtures/full.yaml
Add OmitToolNamePrefix boolean in MCP config, update JSON schema and fixture entry omit_tool_name_prefix.
Config testdata / generated defaults
router/pkg/config/testdata/config_defaults.json, router/pkg/config/testdata/config_full.json
Add MCP.OmitToolNamePrefix: false to test defaults/full config files.
Testenv / MCP operations path
router-tests/testenv/testenv.go
Add MCPOperationsPath to test Config and use it (with default) to compute MCP storage provider path for tests instead of a hard-coded path.
MCP test fixtures
router-tests/testdata/mcp_operations_collision/*
Add GraphQL operation files (GetSchema.graphql, MyMutation.graphql, MyQuery.graphql) used to trigger name-collision and short-name execution scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a configuration option to omit the tool name prefix for MCP tools.
Linked Issues check ✅ Passed The PR implements issue #2308's requirement to allow control over MCP tool names through a configurable option with sensible defaults and collision handling.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the omit_tool_name_prefix feature; no unrelated modifications detected.

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

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20d5a0f and ab918f0.

📒 Files selected for processing (1)
  • router-tests/mcp_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • router-tests/mcp_test.go

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.

❤️ Share

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

Add tests verifying:
- Tool names are generated without 'execute_operation_' prefix when enabled
- Tools can be executed using the short names
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
router/pkg/config/config.go (1)

1000-1000: Consider adding a documentation comment.

The field definition is correct and follows all naming conventions. However, adding a brief comment explaining the field's purpose would improve maintainability:

// OmitOperationPrefix controls whether to omit the "execute_operation_" prefix from MCP tool names.
// When true, GetUser → get_user. When false (default), GetUser → execute_operation_get_user.
OmitOperationPrefix       bool             `yaml:"omit_operation_prefix" envDefault:"false" env:"MCP_OMIT_OPERATION_PREFIX"`

This is optional since other fields in MCPConfiguration also lack comments, but it would help future maintainers understand the feature without consulting external documentation.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98bb8e9 and 1c7397a.

📒 Files selected for processing (8)
  • router-tests/mcp_test.go
  • router/core/router.go
  • router/pkg/config/config.go
  • router/pkg/config/config.schema.json
  • router/pkg/config/fixtures/full.yaml
  • router/pkg/config/testdata/config_defaults.json
  • router/pkg/config/testdata/config_full.json
  • router/pkg/mcpserver/server.go
🧰 Additional context used
🧠 Learnings (4)
📚 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
  • router/pkg/config/testdata/config_defaults.json
📚 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
  • router/core/router.go
📚 Learning: 2025-07-30T09:29:46.660Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.

Applied to files:

  • router/pkg/config/config.schema.json
📚 Learning: 2025-09-29T10:28:07.122Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2192
File: router/pkg/config/config.go:1028-1029
Timestamp: 2025-09-29T10:28:07.122Z
Learning: The deprecation strategy for IntrospectionEnabled field in router/pkg/config/config.go is to first mark it as deprecated, then migrate all call sites to use IntrospectionConfig.Enabled, and finally remove the deprecated field. The envDefault tag should be kept during the deprecation period for backward compatibility.

Applied to files:

  • router/pkg/config/config.go
🧬 Code graph analysis (2)
router-tests/mcp_test.go (2)
router-tests/testenv/testenv.go (3)
  • Run (105-122)
  • Config (292-349)
  • Environment (1735-1771)
router/pkg/config/config.go (2)
  • Config (1032-1107)
  • MCPConfiguration (991-1002)
router/core/router.go (1)
router/pkg/mcpserver/server.go (1)
  • WithOmitOperationPrefix (315-319)
🔇 Additional comments (10)
router/pkg/config/testdata/config_defaults.json (1)

140-140: LGTM! Default value correctly added.

The new OmitOperationPrefix field is properly added to the MCP configuration block with the default value false, which matches the envDefault tag in the struct definition and preserves backward compatibility.

router/pkg/config/testdata/config_full.json (1)

175-175: LGTM! Test fixture properly updated.

The OmitOperationPrefix field is correctly added to the full configuration test fixture, maintaining consistency with config_defaults.json.

router/pkg/config/fixtures/full.yaml (1)

48-48: LGTM! YAML fixture correctly updated.

The omit_operation_prefix field follows the correct YAML naming convention and is properly placed in the MCP configuration section with the appropriate default value.

router/core/router.go (1)

942-942: LGTM! Configuration properly wired.

The new OmitOperationPrefix configuration is correctly wired into the MCP server initialization using the standard option pattern, consistent with other MCP options in this block.

router/pkg/config/config.schema.json (1)

2135-2139: LGTM! Schema definition is clear and well-documented.

The new omit_operation_prefix property is properly defined with:

  • Correct default value (false) for backward compatibility
  • Clear description with concrete examples
  • Appropriate warning about breaking existing integrations
  • Proper placement alongside other MCP configuration options
router-tests/mcp_test.go (2)

164-199: LGTM! Comprehensive test coverage for the new flag.

The test properly verifies:

  • Short tool names (my_employees, update_mood) are registered when the flag is enabled
  • Prefixed names (execute_operation_*) are NOT registered
  • Clear assertions with descriptive failure messages

201-227: LGTM! End-to-end validation of short name execution.

This test ensures that tools registered with short names can be successfully invoked and return expected results. Good complement to the listing test.

router/pkg/mcpserver/server.go (3)

93-94: LGTM! Clean addition to Options struct.

The new field is well-documented and follows existing patterns for boolean configuration flags.


314-319: LGTM! Option builder follows established patterns.

The WithOmitOperationPrefix function correctly implements the functional options pattern used throughout this codebase.


561-566: LGTM! Clean conditional logic for tool naming.

The implementation correctly generates tool names based on the configuration flag:

  • When enabled: uses snake_case operation name directly (e.g., my_employees)
  • When disabled: adds execute_operation_ prefix (e.g., execute_operation_my_employees)

The logic is clear and maintains backward compatibility with the default behavior.

shamashel added a commit to shamashel/cosmo-docs that referenced this pull request Jan 5, 2026
Add documentation for the new omit_operation_prefix configuration option
that allows cleaner MCP tool names without the execute_operation_ prefix.

Related: wundergraph/cosmo#2441
Rename MCP config option per maintainer feedback:
- omit_operation_prefix -> strip_tool_name_prefix
- OmitOperationPrefix -> StripToolNamePrefix
- MCP_OMIT_OPERATION_PREFIX -> MCP_STRIP_TOOL_NAME_PREFIX

Updated description to clarify the option removes the
'execute_operation_' prefix from MCP tool names.
@shamashel shamashel changed the title feat(router): add omit_operation_prefix config for MCP tool names feat(router): add strip_tool_name_prefix config for MCP tool names Jan 5, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
router/pkg/mcpserver/server.go (1)

561-575: Verify tool title consistency with stripped names.

The tool naming logic correctly applies the prefix based on stripToolNamePrefix. However, the tool's Title annotation (line 575) always includes "Execute operation" regardless of the prefix setting:

  • With stripToolNamePrefix=true: tool name = "get_user", title = "Execute operation GetUser"
  • With stripToolNamePrefix=false: tool name = "execute_operation_get_user", title = "Execute operation GetUser"

This might be intentional to keep the title descriptive for display purposes, but please verify this is the desired behavior. If the goal is full consistency, consider making the title conditional as well:

Optional: Make title consistent with tool name
+	var toolTitle string
+	if s.stripToolNamePrefix {
+		toolTitle = op.Name
+	} else {
+		toolTitle = fmt.Sprintf("Execute operation %s", op.Name)
+	}
 	tool.Annotations = mcp.ToolAnnotation{
 		IdempotentHint: mcp.ToBoolPtr(op.OperationType != "mutation"),
-		Title:          fmt.Sprintf("Execute operation %s", op.Name),
+		Title:          toolTitle,
 		ReadOnlyHint:   mcp.ToBoolPtr(op.OperationType == "query"),
 		OpenWorldHint:  mcp.ToBoolPtr(true),
 	}
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 141bcdf and 2c5e315.

📒 Files selected for processing (8)
  • router-tests/mcp_test.go
  • router/core/router.go
  • router/pkg/config/config.go
  • router/pkg/config/config.schema.json
  • router/pkg/config/fixtures/full.yaml
  • router/pkg/config/testdata/config_defaults.json
  • router/pkg/config/testdata/config_full.json
  • router/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • router/pkg/config/config.go
  • router-tests/mcp_test.go
  • router/pkg/config/config.schema.json
🧰 Additional context used
🧠 Learnings (7)
📚 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/pkg/config/fixtures/full.yaml
  • router/core/router.go
  • router/pkg/config/testdata/config_defaults.json
📚 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/pkg/config/fixtures/full.yaml
📚 Learning: 2025-09-17T18:13:31.021Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/jwks_token_decoder.go:176-199
Timestamp: 2025-09-17T18:13:31.021Z
Learning: In router/pkg/authentication/jwks_token_decoder.go, when AllowEmptyAlgorithm is true, the allowedAlgorithms slice is populated by getSupportedAlgorithms() from validation_store.go, ensuring it's never empty and preventing unintended token rejections.

Applied to files:

  • router/pkg/config/testdata/config_full.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.

Applied to files:

  • router/pkg/config/testdata/config_defaults.json
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.

Applied to files:

  • router/pkg/config/testdata/config_defaults.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Applied to files:

  • router/pkg/config/testdata/config_defaults.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.

Applied to files:

  • router/pkg/config/testdata/config_defaults.json
🧬 Code graph analysis (1)
router/core/router.go (1)
router/pkg/mcpserver/server.go (1)
  • WithStripToolNamePrefix (315-319)
🔇 Additional comments (7)
router/pkg/config/fixtures/full.yaml (1)

48-48: LGTM! Configuration option added correctly.

The new strip_tool_name_prefix field is added with a sensible default value of false to maintain backward compatibility.

router/pkg/config/testdata/config_full.json (1)

165-165: LGTM! Test data updated correctly.

The StripToolNamePrefix field is properly added to the MCP configuration test data with the correct default value.

router/core/router.go (1)

942-942: LGTM! Configuration wiring is correct.

The new StripToolNamePrefix option is properly wired from the router's MCP configuration to the MCP server options.

router/pkg/config/testdata/config_defaults.json (1)

130-130: LGTM! Default configuration updated correctly.

The StripToolNamePrefix field is properly added to the default configuration test data.

router/pkg/mcpserver/server.go (3)

93-94: LGTM! Option field added correctly.

The StripToolNamePrefix field is properly added to the Options struct with clear documentation.


115-115: LGTM! Server field properly initialized.

The stripToolNamePrefix field is correctly added to the server struct and initialized from the options.

Also applies to: 246-246


314-319: LGTM! Functional option implemented correctly.

The WithStripToolNamePrefix function follows the established functional options pattern and is properly implemented.

shamashel added a commit to shamashel/cosmo-docs that referenced this pull request Jan 5, 2026
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.82%. Comparing base (2a2cd52) to head (ab918f0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2441       +/-   ##
===========================================
+ Coverage   36.27%   54.82%   +18.55%     
===========================================
  Files         944      213      -731     
  Lines      123298    23256   -100042     
  Branches     4944        0     -4944     
===========================================
- Hits        44731    12751    -31980     
+ Misses      77025     9248    -67777     
+ Partials     1542     1257      -285     
Files with missing lines Coverage Δ
router/core/router.go 68.56% <100.00%> (-1.86%) ⬇️
router/pkg/config/config.go 20.77% <ø> (-59.75%) ⬇️
router/pkg/mcpserver/server.go 79.48% <100.00%> (+1.25%) ⬆️

... and 822 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shamashel shamashel requested a review from asoorm January 7, 2026 16:23
@shamashel shamashel changed the title feat(router): add strip_tool_name_prefix config for MCP tool names feat(router): add omit_tool_name_prefix config for MCP tool names Jan 8, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c5e315 and 4f43b85.

📒 Files selected for processing (8)
  • router-tests/mcp_test.go
  • router/core/router.go
  • router/pkg/config/config.go
  • router/pkg/config/config.schema.json
  • router/pkg/config/fixtures/full.yaml
  • router/pkg/config/testdata/config_defaults.json
  • router/pkg/config/testdata/config_full.json
  • router/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • router/core/router.go
  • router/pkg/config/testdata/config_defaults.json
  • router/pkg/config/config.go
  • router/pkg/config/testdata/config_full.json
  • router-tests/mcp_test.go
  • router/pkg/config/config.schema.json
  • router/pkg/config/fixtures/full.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T12:37:21.521Z
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.

Applied to files:

  • router/pkg/mcpserver/server.go
🔇 Additional comments (4)
router/pkg/mcpserver/server.go (4)

93-94: LGTM! Clear configuration option.

The new OmitToolNamePrefix field is well-documented and follows the existing options pattern. The boolean defaults to false, preserving backwards compatibility.


115-115: LGTM! Proper field placement.

The internal field follows the established pattern for storing server configuration.


246-246: LGTM! Correct option wiring.

The option is properly transferred from the configuration to the server instance.


314-319: LGTM! Functional option follows established pattern.

The implementation is consistent with other configuration options in the codebase.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @router-tests/testdata/mcp_operations_collision/MyMutation.graphql:
- Around line 1-2: The GraphQL mutation UpdateMood has missing commas: add a
comma between variables in the signature (`$employeeID: Int!, $mood: Mood!`) and
add a comma between arguments in the call (`employeeID: $employeeID, mood:
$mood`) so the mutation variables and arguments are properly comma-separated.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f43b85 and 8a828de.

📒 Files selected for processing (6)
  • router-tests/mcp_test.go
  • router-tests/testdata/mcp_operations_collision/GetSchema.graphql
  • router-tests/testdata/mcp_operations_collision/MyMutation.graphql
  • router-tests/testdata/mcp_operations_collision/MyQuery.graphql
  • router-tests/testenv/testenv.go
  • router/pkg/mcpserver/server.go
✅ Files skipped from review due to trivial changes (1)
  • router-tests/testdata/mcp_operations_collision/GetSchema.graphql
🧰 Additional context used
🧠 Learnings (4)
📚 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/testenv/testenv.go
  • router-tests/mcp_test.go
📚 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/testenv/testenv.go
  • router-tests/mcp_test.go
📚 Learning: 2025-09-29T10:28:07.122Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2192
File: router/pkg/config/config.go:1028-1029
Timestamp: 2025-09-29T10:28:07.122Z
Learning: The deprecation strategy for IntrospectionEnabled field in router/pkg/config/config.go is to first mark it as deprecated, then migrate all call sites to use IntrospectionConfig.Enabled, and finally remove the deprecated field. The envDefault tag should be kept during the deprecation period for backward compatibility.

Applied to files:

  • router-tests/testenv/testenv.go
📚 Learning: 2026-01-06T12:37:21.521Z
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.

Applied to files:

  • router-tests/testenv/testenv.go
  • router/pkg/mcpserver/server.go
  • router-tests/mcp_test.go
🧬 Code graph analysis (1)
router-tests/testenv/testenv.go (2)
router/core/router.go (1)
  • WithStorageProviders (2137-2141)
router/pkg/config/config.go (2)
  • StorageProviders (800-805)
  • FileSystemStorageProvider (832-835)
🔇 Additional comments (11)
router-tests/testdata/mcp_operations_collision/MyQuery.graphql (1)

1-12: LGTM!

The query syntax is correct and follows GraphQL conventions.

router-tests/testenv/testenv.go (2)

344-344: LGTM!

The MCPOperationsPath field addition enables test-specific MCP operations directories, supporting the collision test scenarios.


1442-1450: LGTM!

The implementation correctly applies the default path "testdata/mcp_operations" and allows override via MCPOperationsPath. This enables tests to specify custom operation directories for collision scenarios.

router-tests/mcp_test.go (3)

164-199: LGTM!

This test correctly verifies that when OmitToolNamePrefix is enabled, tool names appear without the execute_operation_ prefix. The boolean flag logic properly checks for short names and ensures prefixed names are absent.


201-227: LGTM!

This test validates that operations can be executed using short tool names when OmitToolNamePrefix is enabled. The test correctly invokes my_employees and verifies the response content.


229-257: LGTM!

This test properly validates collision handling: when a user operation name collides with a built-in tool (e.g., get_schema), the implementation correctly falls back to the prefixed name execute_operation_get_schema for the user operation while preserving the built-in tool at its canonical name.

router/pkg/mcpserver/server.go (5)

11-11: LGTM!

The slices import is necessary for the collision detection logic at line 565.


94-95: LGTM!

The OmitToolNamePrefix field is well-documented and follows the existing pattern for configuration options.


116-116: LGTM!

The omitToolNamePrefix field is correctly initialized from options and stored in the server instance.

Also applies to: 247-247


315-320: LGTM!

The WithOmitToolNamePrefix constructor follows the established functional options pattern used throughout the file.


562-572: LGTM!

The collision detection logic is well-implemented:

  1. Default toolName is the snake_cased operation name (line 562)
  2. If prefix is not omitted, apply execute_operation_ prefix (lines 563-565)
  3. If prefix is omitted BUT there's a collision with registered tools (built-in or previous operations), fall back to prefixed name with a warning (lines 565-572)

The collision check against s.registeredTools correctly detects conflicts with both built-in tools (registered earlier at lines 450-512) and previously registered user operations (added at line 591).

shamashel and others added 2 commits January 9, 2026 05:05
- Fix GraphQL syntax errors in MyMutation.graphql (add commas)
- Use assert.ElementsMatch for cleaner test assertions
- Embed struct values directly in CallToolRequest initialization
- Reorder MCPConfiguration fields (RouterURL before OmitToolNamePrefix)
- Trim description in config.schema.json
@shamashel shamashel requested a review from endigma January 9, 2026 14:40
Copy link
Member

@endigma endigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small thing!

@shamashel shamashel requested a review from endigma January 9, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow users to define the MCP tool's name

3 participants