Conversation
asoorm
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughResets the server's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. 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 |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/mcpserver/server.go (1)
541-549: Optional improvement: Handle operation-to-operation collisions consistently regardless of prefix setting.When
omitToolNamePrefixis false (prefixed mode), two operations with names that normalize to the same snake_case (e.g.,GetUserandGet_User→get_user) would both attempt to register asexecute_operation_get_user. The collision check at line 544 only runs whenomitToolNamePrefixis true.Consider extending the collision check to cover all cases:
♻️ Suggested improvement
toolName := operationToolName if !s.omitToolNamePrefix { toolName = fmt.Sprintf("execute_operation_%s", operationToolName) - } else if slices.Contains(s.registeredTools, operationToolName) { + } + if slices.Contains(s.registeredTools, toolName) { s.logger.Warn("Skipping operation due to tool name collision", zap.String("operation", op.Name), - zap.String("conflicting_tool", operationToolName), + zap.String("conflicting_tool", toolName), ) continue }This makes collision detection work consistently regardless of the prefix setting. However, this is a pre-existing edge case and not a regression from this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 541 - 549, The collision check is only run when s.omitToolNamePrefix is true, so when you compute toolName (using operationToolName or fmt.Sprintf("execute_operation_%s", operationToolName)) perform a membership check against s.registeredTools for the computed toolName and handle collisions consistently: after setting toolName (the variable in this block) check slices.Contains(s.registeredTools, toolName) and if true call s.logger.Warn with the same fields (use op.Name and operationToolName/conflicting_tool) and continue, ensuring the collision branch is reached regardless of s.omitToolNamePrefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/mcpserver/server.go`:
- Around line 541-549: The collision check is only run when s.omitToolNamePrefix
is true, so when you compute toolName (using operationToolName or
fmt.Sprintf("execute_operation_%s", operationToolName)) perform a membership
check against s.registeredTools for the computed toolName and handle collisions
consistently: after setting toolName (the variable in this block) check
slices.Contains(s.registeredTools, toolName) and if true call s.logger.Warn with
the same fields (use op.Name and operationToolName/conflicting_tool) and
continue, ensuring the collision branch is reached regardless of
s.omitToolNamePrefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dc43ad8-d7cc-4fee-94c8-4304f9caa4e0
📒 Files selected for processing (1)
router/pkg/mcpserver/server.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/mcp_test.go (1)
237-238: Assert the collision is skipped, not just that the fallback name disappeared.
get_schemais already present here becauseExposeSchemais enabled, soassert.Contains(t, toolNames, "get_schema")still passes if the colliding operation is incorrectly registered under the same unprefixed name. This should verify there is exactly oneget_schemaentry, or assert the full tool-name set, to prove the operation was actually skipped.Suggested tightening
- assert.Contains(t, toolNames, "get_schema") // built-in tool (ExposeSchema=true) - assert.NotContains(t, toolNames, "execute_operation_get_schema") // colliding operation is skipped, not prefixed + assert.NotContains(t, toolNames, "execute_operation_get_schema") // colliding operation is skipped, not prefixed + + getSchemaCount := 0 + for _, name := range toolNames { + if name == "get_schema" { + getSchemaCount++ + } + } + assert.Equal(t, 1, getSchemaCount, "expected only the built-in get_schema tool to be registered")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/mcp_test.go` around lines 237 - 238, The test currently only checks that "get_schema" is present and "execute_operation_get_schema" is absent, which doesn't prove the collision was skipped; change the assertions to verify there is exactly one "get_schema" entry (e.g., count occurrences of "get_schema" in toolNames and assert it equals 1) and still assert NotContains for "execute_operation_get_schema", or alternatively assert the entire toolNames equals the expected set (using assert.ElementsMatch) so you prove the colliding operation was not registered under the same unprefixed name; update the assertions referencing toolNames, "get_schema", and "execute_operation_get_schema" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/mcp_test.go`:
- Around line 237-238: The test currently only checks that "get_schema" is
present and "execute_operation_get_schema" is absent, which doesn't prove the
collision was skipped; change the assertions to verify there is exactly one
"get_schema" entry (e.g., count occurrences of "get_schema" in toolNames and
assert it equals 1) and still assert NotContains for
"execute_operation_get_schema", or alternatively assert the entire toolNames
equals the expected set (using assert.ElementsMatch) so you prove the colliding
operation was not registered under the same unprefixed name; update the
assertions referencing toolNames, "get_schema", and
"execute_operation_get_schema" accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0910158-8a5e-4386-822e-a4af3616fa17
📒 Files selected for processing (1)
router-tests/mcp_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2625 +/- ##
==========================================
+ Coverage 62.41% 62.90% +0.48%
==========================================
Files 244 244
Lines 25826 25826
==========================================
+ Hits 16119 16245 +126
+ Misses 8326 8209 -117
+ Partials 1381 1372 -9
🚀 New features to boost your workflow:
|
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
4 times, most recently
from
49755be to
b73dddc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/mcpserver/server.go (1)
541-549:⚠️ Potential issue | 🟠 Major
get_operation_infocan still collide with an operation name.This check only covers names already present in
s.registeredTools. Becauseget_operation_infois registered later in the function, an operation whose snake_case name isget_operation_infowill still be added whenomit_tool_name_prefixis enabled, leaving one real collision path unfixed.💡 Minimal fix
- } else if slices.Contains(s.registeredTools, operationToolName) { + } else if operationToolName == "get_operation_info" || slices.Contains(s.registeredTools, operationToolName) { s.logger.Error("Skipping operation due to tool name collision", zap.String("operation", op.Name), zap.String("conflicting_tool", operationToolName), ) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 541 - 549, When computing toolName in the block using operationToolName and s.omitToolNamePrefix, also detect collisions with the reserved handler name "get_operation_info" (or any other reserved/future-registered tool names) rather than only checking s.registeredTools; update the condition that currently does slices.Contains(s.registeredTools, operationToolName) to check against a combined set (e.g., registeredTools plus a small reservedNames list containing "get_operation_info") and, if found, log the same error and continue so an operation whose snake_case is "get_operation_info" is skipped when omitToolNamePrefix is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/mcpserver/server.go`:
- Around line 545-548: The current call to s.logger.Error("Skipping operation
due to tool name collision", ...) should be downgraded to a warning since the
collision is handled by skipping registration; update the call in server.go
where the collision is logged (the site using s.logger.Error with the message
"Skipping operation due to tool name collision" and fields
zap.String("operation", op.Name) and zap.String("conflicting_tool",
operationToolName)) to s.logger.Warn with the same message and fields so it logs
as a warning rather than an error.
---
Outside diff comments:
In `@router/pkg/mcpserver/server.go`:
- Around line 541-549: When computing toolName in the block using
operationToolName and s.omitToolNamePrefix, also detect collisions with the
reserved handler name "get_operation_info" (or any other
reserved/future-registered tool names) rather than only checking
s.registeredTools; update the condition that currently does
slices.Contains(s.registeredTools, operationToolName) to check against a
combined set (e.g., registeredTools plus a small reservedNames list containing
"get_operation_info") and, if found, log the same error and continue so an
operation whose snake_case is "get_operation_info" is skipped when
omitToolNamePrefix is true.
🪄 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: CHILL
Plan: Pro
Run ID: d0ae5a65-532e-4a29-9708-ca9be7fe89b0
📒 Files selected for processing (2)
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/mcp_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/mcpserver/server.go (1)
572-589: Consider potential edge case withget_operation_inforegistration order.The
get_operation_infotool is registered after the operation loop, so an operation named "GetOperationInfo" withomitToolNamePrefix: truewould bypass collision detection and potentially overwrite the built-in handler (or vice versa depending onAddToolbehavior).Pre-existing issue not introduced by this PR, but could be addressed by moving line 589's append to before the operation loop, or by adding
"get_operation_info"toregisteredToolsearlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/server.go` around lines 572 - 589, The built-in tool "get_operation_info" is appended to s.registeredTools after the operation registration loop, which can allow a user operation named "GetOperationInfo" with omitToolNamePrefix:true to collide; update the registration order so the built-in name is reserved before user operations by adding "get_operation_info" into s.registeredTools prior to the operation loop (or moving the s.registeredTools = append(s.registeredTools, "get_operation_info") call to execute before the loop that registers user operations), ensuring the built-in handler (handleGraphQLOperationInfo / mcp.NewTool("get_operation_info")) cannot be shadowed by user tools registered via AddTool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/mcpserver/server.go`:
- Around line 572-589: The built-in tool "get_operation_info" is appended to
s.registeredTools after the operation registration loop, which can allow a user
operation named "GetOperationInfo" with omitToolNamePrefix:true to collide;
update the registration order so the built-in name is reserved before user
operations by adding "get_operation_info" into s.registeredTools prior to the
operation loop (or moving the s.registeredTools = append(s.registeredTools,
"get_operation_info") call to execute before the loop that registers user
operations), ensuring the built-in handler (handleGraphQLOperationInfo /
mcp.NewTool("get_operation_info")) cannot be shadowed by user tools registered
via AddTool.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b57768dd-8d66-4bab-8e8e-9247014d4f5d
📒 Files selected for processing (2)
router-tests/mcp_test.gorouter/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/mcp_test.go
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
from
b73dddc to
1f44a5f
Compare
shamashel
left a comment
shamashel
left a comment
There was a problem hiding this comment.
Didn't realize you wanted me to submit an actual review here. LGTM from our side!
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
from
1f44a5f to
a4213a4
Compare
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
from
a4213a4 to
cfc508a
Compare
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
from
cfc508a to
5622a49
Compare
… name collisions registeredTools was never cleared between config reloads, causing it to grow unbounded. When omit_tool_name_prefix was enabled, every operation would falsely collide with its own name from the previous reload cycle. Also changes collision behavior to skip duplicate tools with a warning instead of silently falling back to a prefixed name. Fixes: ENG-9158 fix(mcp): update collision test to expect skip behavior instead of prefix fallback
- Assert full registeredTools list with ElementsMatch instead of individual Contains/counting - Rename test to accurately reflect what it verifies - Add reserved name collision scenario to prefix mode test
asoorm
force-pushed
the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
from
5622a49 to
ca84c26
Compare
asoorm
deleted the
ahmet/eng-9158-mcp-registeredtools-grows-unbounded-across-config-reloads
branch
3 participants
Summary
s.registeredTools = nilafterDeleteToolsto prevent unbounded slice growth across config reloadsomit_tool_name_prefix: true, every operation was falsely colliding with its own name from the previous reload cycleFixes ENG-9158
Closes #2623
Test plan
omit_tool_name_prefix: trueandexecution_config.file.watch: trueGetSchema→get_schema) are still detected and skipped with a warningSummary by CodeRabbit
Bug Fixes
Tests