Skip to content

fix(MCPServer): Session tool handler not used due to variable shadowing #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 4, 2025

Conversation

cryo-zd
Copy link
Contributor

@cryo-zd cryo-zd commented May 4, 2025

Problem
  This PR fixes a bug where session-specific tool handlers were never used, and MCPServer always fell back to global tool handlers.
  The issue was caused by variable shadowing in the following block of code:

if sessionWithTools, ok := session.(SessionWithTools); ok {
    ...
    if sessionOk {
        ok = true
    }
}

  The ok declared in if sessionWithTools, ok := .... shadows the outer ok variable declared in the function:

var ok bool

  As a result, setting ok = true inside this if block had no effect on the outer variable, and the following loginc:

if !ok {
    //fallback to global tools
}

was always executed, even if a session tool was actually found.

Fix
  This fix simply avoids shadowing by renaming the inner variable:

if sessionWithTools, typeAssertOk := session.(SessionWithTools); typeAssertOk {

  This ensures that the outer ok variable reflects whether a session-specific tool was found and used.

Summary by CodeRabbit

  • Tests

    • Added a test to verify that session-specific tools correctly override global tools with the same name when called within a session context.
  • Refactor

    • Improved variable naming in session tool handling to prevent naming conflicts and enhance code clarity.

Copy link
Contributor

coderabbitai bot commented May 4, 2025

Walkthrough

This change updates the handleToolCall method in the MCP server to use a more specific variable name when performing a type assertion to the SessionWithTools interface, preventing variable shadowing. Additionally, a new test is introduced to verify that a session-specific tool correctly overrides a global tool with the same name, ensuring that the session's tool handler is invoked when expected.

Changes

File(s) Change Summary
server/server.go Renamed the variable used in the type assertion for SessionWithTools from ok to typeAssertOk in handleToolCall to avoid shadowing the outer ok variable. No logic changes.
server/session_test.go Added TestMCPServer_CallSessionTool to test that a session-specific tool overrides a global tool of the same name, confirming correct handler invocation.

Possibly related PRs


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5244489 and 12ec7be.

📒 Files selected for processing (2)
  • server/server.go (1 hunks)
  • server/session_test.go (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/server.go (1)
server/session.go (1)
  • SessionWithTools (23-31)
🔇 Additional comments (2)
server/server.go (1)

859-859: Good fix to prevent variable shadowing.

Renaming the variable from ok to typeAssertOk in the type assertion solves the variable shadowing issue that was causing session-specific tool handlers to be ignored. This change ensures that setting ok = true at line 864 properly affects the outer variable used for determining whether to fall back to global tools.

server/session_test.go (1)

299-355: Well-designed test case for the fixed issue.

This test correctly verifies that a session-specific tool can override a global tool with the same name, which directly tests the functionality fixed in server/server.go. The test creates both a global and session-specific tool with the same name but different responses, then verifies that the session-specific tool is correctly invoked.

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Collaborator

@rwjblue-glean rwjblue-glean left a comment

Choose a reason for hiding this comment

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

Nice catch!

@ezynda3 ezynda3 merged commit e40e7a7 into mark3labs:main May 4, 2025
2 checks passed
tendant pushed a commit to tendant/mcp-go that referenced this pull request May 8, 2025
adlternative pushed a commit to adlternative/mcp-go that referenced this pull request May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants