Skip to content

refactor: make CallToolRequest.Arguments more flexible (Breaking Change) #287

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 13 commits into from
May 17, 2025

Conversation

ezynda3
Copy link
Contributor

@ezynda3 ezynda3 commented May 14, 2025

This change allows for more flexible argument types in CallToolRequest.
Arguments field is now of type 'any' instead of 'map[string]any'.
Added GetArguments() and GetRawArguments() methods for backward compatibility.

Fixes #104

Summary by CodeRabbit

  • Refactor

    • Updated argument access in tool handlers and utilities to use a unified method for retrieving argument values, enhancing consistency and flexibility.
  • New Features

    • Added flexible and typed methods for safer, more versatile access to tool request arguments, including support for primitives, slices, and structured binding.
    • Introduced typed tool handlers enabling automatic binding of request arguments to strongly typed structs with built-in validation.
    • Added a new example demonstrating typed tools with detailed argument schemas and personalized responses.
  • Tests

    • Added extensive tests covering argument binding, typed retrieval, error handling, serialization, and typed handler functionality with complex input data.

This change allows for more flexible argument types in CallToolRequest.
Arguments field is now of type 'any' instead of 'map[string]any'.
Added GetArguments() and GetRawArguments() methods for backward compatibility.

Fixes #104
Copy link
Contributor

coderabbitai bot commented May 14, 2025

"""

Walkthrough

The changes refactor how tool handler functions and tests access tool call arguments, replacing direct field access with the new GetArguments() method. The Arguments field in CallToolRequest.Params is changed from a strict map type to a flexible any type, and new accessor methods are introduced. Additional tests verify the new behavior and serialization.

Changes

Files/Paths Change Summary
mcp/tools.go Changed Arguments field type from map[string]any to any; added GetArguments(), GetRawArguments(), and typed getters.
mcp/tools_test.go Added tests for CallToolRequest argument handling and JSON marshaling/unmarshaling with various argument types.
mcp/typed_tools.go, mcp/typed_tools_test.go Added typed tool handler support with generic binding and tests for typed handlers and validation.
client/inprocess_test.go, client/sse_test.go, mcptest/mcptest_test.go Updated argument access in handlers to use GetArguments() instead of direct field access.
examples/custom_context/main.go, examples/dynamic_path/main.go, examples/everything/main.go Updated tool handlers to use GetArguments() for argument retrieval.
examples/typed_tools/main.go Added example demonstrating typed tool handlers with strong argument typing and validation.
mcp/utils.go Updated ParseArgument to use GetArguments() for argument retrieval.
README.md Updated documentation to reflect new argument access patterns.

Assessment against linked issues

Objective (Issue #) Addressed Explanation
Change Params.Arguments type to any (or interface{}) for flexibility (#104)
Provide backward-compatible access for arguments as a map (#104)
Ensure correct handling of arguments in handlers and utilities (#104)
Add or update tests to verify new argument type and accessors (#104)

Possibly related PRs

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (1.64.8)

Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2
Failed executing command with error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


Note

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.

✨ 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.
    • Explain this complex logic.
    • 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 explain this code block.
    • @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 explain its main purpose.
    • @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.

@ezynda3 ezynda3 force-pushed the update-arguments-parsing branch from a06d4ac to 27e3a49 Compare May 14, 2025 13:58
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 (2)
mcp/tools_test.go (2)

378-400: Thorough JSON serialization test with helpful note

This test properly verifies JSON marshaling and unmarshaling behavior. The comment about JSON numbers being unmarshaled as float64 is especially valuable to prevent confusion when working with numeric values.

Consider adding similar tests for JSON marshaling/unmarshaling with non-map argument types (string, struct) to ensure consistent behavior across all supported types.


312-400: Consider additional edge cases for robustness

The tests provide good coverage for the main use cases, but could be strengthened by adding tests for:

  1. nil arguments
  2. Empty map arguments
  3. Nested complex structures (maps containing maps, arrays, etc.)

These additional tests would ensure comprehensive handling of all possible input types.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between a5ac0b3 and 27e3a49.

📒 Files selected for processing (1)
  • mcp/tools_test.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/tools_test.go (2)
mcp/tools.go (1)
  • CallToolRequest (44-59)
mcp/types.go (1)
  • Params (118-118)
🔇 Additional comments (3)
mcp/tools_test.go (3)

312-332: Clean implementation of map argument tests

This test effectively verifies that both GetArguments() and GetRawArguments() correctly handle the common case where Arguments is a map. Good use of type assertion with the ok variable to ensure proper error handling.


334-349: Appropriate handling of non-map type cases

This test correctly verifies the expected behavior when Arguments is a string (non-map type): GetArguments() returns an empty map while GetRawArguments() preserves the original string value. The clear comment about returning an empty map helps explain the intended behavior.


351-376: Well-structured struct argument test case

The test thoroughly checks the behavior with custom struct arguments and properly verifies both accessor methods. Good use of JSON tags in the test struct to simulate real-world usage.

Copy link
Collaborator

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

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

I think this is a good first step but it does not necessarily address all the concerns in the issue like a function to unmarshal the arguments into a struct or helper functions to get the arguments to avoid manually type checking each argument.

Maybe we can address it in this PR itself if others can also jump in and share inputs on how this must be implemented.

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: 2

🧹 Nitpick comments (2)
mcp/tools.go (2)

48-50: Breaking-change is fine, but please document accepted JSON shapes

Changing Arguments from map[string]any to any is what enables the new flexibility, but it also introduces ambiguity for consumers:

  • Callers that always expected a map now compile, but will silently receive an empty map when the producer sends a non-map payload – this may hide bugs.
  • Nothing in the comment explains which concrete JSON shapes are actually accepted/expected (map, array, scalar, struct‐like object, raw JSON, …).

Consider extending the doc-comment (or the project’s CHANGELOG) to specify the supported shapes and how each helper behaves when the shape does not match.


88-274: Large block of almost-identical getters can be consolidated

The five Get*/Require* pairs share ~90 % identical control flow. This bloats the file and makes future changes error-prone.

You can remove duplication by extracting a small internal helper, e.g.:

func (r CallToolRequest) coerce(key string) (any, bool) {
	args := r.GetArguments()
	v, ok := args[key]
	return v, ok
}

…and then factor the conversion logic into generic functions or switches that return the desired type. A sketch:

func getNumber[T constraints.Integer | constraints.Float](v any, def T) T { … }

Not only does this cut LOC, it centralises edge-case handling (e.g. json.Number, float32) and keeps the public API unchanged.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 27e3a49 and 57b579d.

📒 Files selected for processing (1)
  • mcp/tools.go (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
mcp/tools.go (3)
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in mark3labs/mcp-go handles both InputSchema and RawInputSchema formats. When unmarshaling JSON, it first tries to parse into a structured ToolInputSchema format, and if that fails, it falls back to using the raw schema format, providing symmetry with the MarshalJSON method.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
🧬 Code Graph Analysis (1)
mcp/tools.go (1)
mcp/types.go (1)
  • Params (118-118)
🔇 Additional comments (1)
mcp/tools.go (1)

112-148: json.Number and overflow cases are not handled

encoding/json unmarshals numbers into float64 when the destination is any, unless UseNumber is enabled on the decoder, in which case you receive json.Number.

If upstream ever enables UseNumber, GetInt/RequireInt and friends will start returning the default/error even though the value is convertible.

Recommend adding a branch:

case json.Number:
    if i, err := v.Int64(); err == nil {
        return int(i) // or overflow-check first
    }

Similarly guard against float64 values that exceed math.MaxInt.

Copy link
Collaborator

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

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

LGTM.

We should add tests that use the new Bind and helper functions. Other than that I think we are good to go.

We can maybe wait until one of the issue openers review this and see if it is as the expected.

IMO, this is exactly how we should implement this but maybe others have a different idea and maybe more opinionated.

ezynda3 and others added 5 commits May 14, 2025 17:31
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@ezynda3 ezynda3 requested a review from rwjblue-glean May 14, 2025 14:39
Comment on lines +78 to +81
// BindArguments unmarshals the Arguments into the provided struct
// This is useful for working with strongly-typed arguments
func (r CallToolRequest) BindArguments(target any) error {
if target == nil || reflect.ValueOf(target).Kind() != reflect.Ptr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a great start, but I think we should probably extend further so that when you register a tool you can 100% control the args parsing. I'm not sure what the best API for taht might be, but I could imagine AddTool (or a AddToolWithCustomArgs or some other better name) that takes two functions (one parses args into your custom struct, and the other would be like todays tool handler func but directly receive the return value of the parsed args).

To be clear, I think this API is a good improvement. I just think we can make it even better.

@ezynda3 / @pottekkat - What do y'all think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sounds good. Will try to add some stuff a bit later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

TypedToolHandlerFunc looks good 👍🏽

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

♻️ Duplicate comments (2)
mcp/tools.go (2)

63-70: Using nil instead of empty map improves error detection.

The method now returns nil instead of an empty map when Arguments is not a map type, which allows callers to distinguish between "no arguments" and "wrong argument type" scenarios.


78-96: LGTM: Implemented BindArguments with suggested optimizations.

The implementation properly includes:

  1. Validation that the target is a non-nil pointer
  2. Fast-path for raw JSON to avoid double marshaling
  3. Clear error messages

This follows the feedback from the previous review comment.

🧹 Nitpick comments (1)
mcp/tools.go (1)

63-468: Consider adding helper functions for custom argument parsing.

The current implementation is a great improvement, but as discussed in previous comments, extending the API to better support custom argument parsing at tool registration time would be valuable.

// Example addition to consider (pseudocode):
+// RegisterToolWithArgsParser registers a tool with a custom argument parser and handler
+func (s *Server) RegisterToolWithArgsParser(
+    tool Tool,
+    parser func(CallToolRequest) (any, error),
+    handler func(context.Context, any) (CallToolResult, error),
+) {
+    // Wrap the handler in a function that parses arguments first
+    wrapperHandler := func(ctx context.Context, req CallToolRequest) (CallToolResult, error) {
+        parsedArgs, err := parser(req)
+        if err != nil {
+            return CallToolResult{IsError: true}, err
+        }
+        return handler(ctx, parsedArgs)
+    }
+    s.RegisterTool(tool, wrapperHandler)
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 57b579d and a8474c7.

📒 Files selected for processing (6)
  • README.md (3 hunks)
  • examples/typed_tools/main.go (1 hunks)
  • mcp/tools.go (3 hunks)
  • mcp/tools_test.go (1 hunks)
  • mcp/typed_tools.go (1 hunks)
  • mcp/typed_tools_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
mcp/typed_tools.go (2)
mcp/tools.go (2)
  • CallToolRequest (46-61)
  • CallToolResult (36-43)
mcp/utils.go (1)
  • NewToolResultError (300-310)
mcp/tools.go (1)
mcp/types.go (1)
  • Params (118-118)
🔇 Additional comments (23)
mcp/typed_tools.go (1)

1-20: Well-designed type-safe handler implementation

The implementation of TypedToolHandlerFunc and NewTypedToolHandler introduces a clean, type-safe pattern for handling tool calls. The use of generics provides compile-time type checking while maintaining flexibility.

This approach eliminates the need for manual type assertions and conversions in each handler, making code more robust and maintainable.

examples/typed_tools/main.go (4)

11-21: Good type structure for arguments

The GreetingArgs struct provides a clear, strongly-typed representation of tool arguments with proper JSON field tags and a well-organized nested structure.


31-64: Comprehensive tool schema definition

The tool schema definition is detailed and properly aligned with the GreetingArgs struct. It includes appropriate descriptions, validation constraints, and nested object definitions.


66-67: Excellent example of typed handler registration

This line demonstrates the clean integration of the new typed handler pattern with the existing MCP server framework. The use of NewTypedToolHandler creates a seamless bridge between type-safe handlers and the MCP infrastructure.


75-105: Well-implemented typed handler function

The handler effectively demonstrates proper validation, conditional processing based on input fields, and handling of nested data. The incremental building of the response shows a clear pattern for working with typed arguments.

mcp/tools_test.go (4)

312-336: Good test coverage for argument binding

This test validates that the BindArguments method correctly binds map arguments to typed structs, which is a fundamental capability for the typed tool handlers.


338-440: Comprehensive test suite for helper functions

The test covers all getter methods (with defaults) and "require" methods for different types, including handling of missing keys and error conditions. This thorough validation ensures the robustness of the argument access API.


442-462: Tests verify flexibility with different argument types

These tests validate that the new flexible argument handling works correctly with various input types (maps, strings, and structs), confirming that GetArguments and GetRawArguments behave as expected for each type.

Also applies to: 464-479, 481-506


508-530: JSON serialization test handles numeric type conversion

This test correctly validates JSON marshaling/unmarshaling of arguments and acknowledges the standard Go behavior where JSON numbers are unmarshaled as float64. This is an important consideration for consumers of the API.

mcp/typed_tools_test.go (3)

12-76: Robust testing of basic typed handler functionality

This test thoroughly validates the core functionality of typed handlers, including proper argument binding and error handling scenarios such as type mismatches and missing fields.


78-145: Good validation logic test cases

The test effectively verifies that typed handlers can implement validation logic and return appropriate error results, including domain-specific validation like preventing division by zero.


147-304: Excellent coverage for complex nested structures

This comprehensive test verifies the handler's ability to process complex nested objects with multiple levels, different field types, and various edge cases including partial data and raw JSON input. The assertions thoroughly check that all aspects of the nested structure are correctly processed.

mcp/tools.go (11)

7-8: Added imports for new argument handling functionality.

The addition of the reflect and strconv packages appropriately supports the new type conversion and validation functionality in the helper methods.


49-50: LGTM: Type change supports flexible argument formats.

Changing Arguments from map[string]any to any enables more flexible argument types, aligning with the PR objective to make CallToolRequest.Arguments more flexible.


72-76: LGTM: Added raw argument access.

The GetRawArguments() method provides a clean way to access the arguments without type conversion, supporting the need for flexible argument types.


98-119: LGTM: String argument helpers are well-implemented.

The String helpers provide a convenient way to extract string arguments with appropriate default values or errors, handling type assertions correctly.


121-158: LGTM: Integer helpers with robust type conversion.

The Int helpers correctly handle various types that can be converted to integers, including native ints, float64 values, and strings that can be parsed as numbers.


160-197: LGTM: Float helpers with proper type conversion and error handling.

The Float helpers follow the same pattern as the Int helpers, with comprehensive type conversions and clear error messages.


199-240: LGTM: Bool helpers with extensive type conversion.

The Bool helpers handle a wide range of valid boolean representations, including:

  • Native booleans
  • Strings that can be parsed as booleans
  • Numeric values (treating non-zero as true)

This provides flexible, user-friendly boolean argument handling.


242-284: LGTM: String slice helpers with proper type handling.

The String slice helpers correctly handle both native string slices and slices of interface values, with appropriate error messages for type mismatches.


286-344: LGTM: Int slice helpers with type conversion.

The Int slice implementations properly convert compatible types and provide clear error messages when conversion fails.


346-404: LGTM: Float slice helpers with robust type handling.

The Float slice helpers implement the same pattern as the Int slice helpers, with proper type conversion and error handling.


406-468: LGTM: Bool slice helpers with extensive type support.

The Bool slice implementations support various boolean representations, consistent with the single boolean helpers.

@pottekkat pottekkat added type: enhancement New feature or enhancement request area: sdk SDK improvements unrelated to MCP specification labels May 16, 2025
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

🔭 Outside diff range comments (1)
mcp/tools.go (1)

50-50: ⚠️ Potential issue

Critical issue: Type change needed for flexible arguments

According to the PR objectives and failures in static analysis, the Arguments field should be changed from map[string]any to any type to support the more flexible argument structure.

The type assertions throughout the new methods won't work unless this change is made.

-        Arguments map[string]any `json:"arguments,omitempty"`
+        Arguments any `json:"arguments,omitempty"`
🧹 Nitpick comments (1)
mcp/tools.go (1)

55-62: Consider adding error feedback for invalid argument types

While returning nil when Arguments isn't a map is better than returning an empty map, consider adding additional methods that would help diagnose type issues, such as:

  1. A method that returns both the map and a boolean indicating success
  2. A method that returns an error when Arguments isn't a map
// Additional helper method to provide more feedback
+func (r CallToolRequest) GetArgumentsWithOk() (map[string]any, bool) {
+    args, ok := r.Params.Arguments.(map[string]any)
+    return args, ok
+}
🧰 Tools
🪛 GitHub Check: test

[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🪛 GitHub Check: lint

[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🪛 GitHub Actions: go

[error] 58-58: invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between a8474c7 and ac5c2ef.

📒 Files selected for processing (1)
  • mcp/tools.go (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/tools.go (1)
mcp/types.go (1)
  • Params (154-154)
🪛 GitHub Check: test
mcp/tools.go

[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🪛 GitHub Check: lint
mcp/tools.go

[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


[failure] 58-58:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🪛 GitHub Actions: go
mcp/tools.go

[error] 58-58: invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🔇 Additional comments (11)
mcp/tools.go (11)

64-68: Looks good: GetRawArguments() provides access to raw argument data

This method is a good addition that allows access to the raw arguments in any format, supporting the flexible argument handling described in the PR objective.


70-88: Good implementation: BindArguments() includes validation and optimization

The implementation includes:

  • Validation that the target is a non-nil pointer
  • Fast-path optimization for when Arguments is already a json.RawMessage
  • Error wrapping with descriptive messages

These improvements align with previous review feedback.

🧰 Tools
🪛 GitHub Check: test

[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface

🪛 GitHub Check: lint

[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


[failure] 78-78:
invalid operation: r.Params.Arguments (variable of type map[string]any) is not an interface) (typecheck)


90-111: Looks good: String accessor methods are well-implemented

The implementation for string type access is thorough with both default and required versions.


113-150: Looks good: Integer accessor methods handle type conversions

The implementation correctly handles different source types (int, float64, string) with proper conversion logic.


152-189: Looks good: Float accessor methods handle type conversions

Similar to the integer methods, these correctly handle conversion from various source types.


191-232: Looks good: Boolean accessor methods handle multiple types

The boolean conversion is comprehensive, handling various input types (bool, string, int, float64) with proper conversion logic.


234-276: Looks good: String slice accessor methods properly handle type conversions

The implementation correctly distinguishes between native string slices and slices of interfaces that need conversion.


278-336: Looks good: Integer slice accessor methods handle multiple source types

The implementation correctly handles conversion from various numeric and string types.


338-396: Looks good: Float slice accessor methods handle multiple source types

Similar to the integer slice methods, these correctly handle conversion from various types.


398-460: Looks good: Boolean slice accessor methods handle multiple types

The implementation correctly handles conversion from bool, string, int, and float64 values.


7-8: Looks good: New imports support added functionality

The addition of reflect and strconv packages supports the new type conversions and validations.

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 (2)
mcp/tools.go (2)

73-88: Consider adding a JSON format validation.

The BindArguments method assumes that the target struct matches the structure of the JSON data, but doesn't validate this before attempting to unmarshal. You might want to add schema validation for more complex use cases.

 func (r CallToolRequest) BindArguments(target any) error {
 	if target == nil || reflect.ValueOf(target).Kind() != reflect.Ptr {
 		return fmt.Errorf("target must be a non-nil pointer")
 	}
 
 	// Fast-path: already raw JSON
 	if raw, ok := r.Params.Arguments.(json.RawMessage); ok {
 		return json.Unmarshal(raw, target)
 	}
 
 	data, err := json.Marshal(r.Params.Arguments)
 	if err != nil {
 		return fmt.Errorf("failed to marshal arguments: %w", err)
 	}
+
+	// Optional validation could go here:
+	// if v, ok := target.(json.Validator); ok {
+	//     // Pre-validate the JSON data before fully unmarshaling
+	//     if err := json.Unmarshal(data, json.RawMessage{}); err != nil {
+	//         return fmt.Errorf("invalid JSON format for target type: %w", err)
+	//     }
+	// }
 
 	return json.Unmarshal(data, target)
 }

90-460: Consider adding more examples to the documentation.

The implementation of all these accessor methods is comprehensive, but it would be helpful to include some usage examples in the documentation to guide users on how to effectively use these methods.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between ac5c2ef and 70e6056.

📒 Files selected for processing (1)
  • mcp/tools.go (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/tools.go (1)
mcp/types.go (2)
  • Meta (112-124)
  • Params (154-154)
🔇 Additional comments (12)
mcp/tools.go (12)

49-51: LGTM! Field type change achieves the PR objective.

Changing the Arguments field from map[string]any to any enables more flexible argument types as intended, which will allow for both structured and unstructured data.


57-62: Return nil for non-map arguments is the correct approach.

Good implementation that now correctly returns nil when the arguments aren't a map instead of an empty map. This allows callers to distinguish between "no args" and "wrong type" scenarios, facilitating better error handling.


66-68: LGTM! Simple, effective raw data accessor.

This method provides direct access to the raw arguments without type conversion, completing the access pattern options for consumers.


72-88: BindArguments implementation follows best practices.

Excellent implementation that:

  1. Properly validates the target is a non-nil pointer
  2. Includes a fast-path for already marshaled JSON
  3. Uses clear error messages with proper wrapping

This matches the suggestions in previous review comments and provides a clean API for type-safe argument binding.


90-111: LGTM! String accessor methods are well-implemented.

The string accessor methods provide both default and required variants with appropriate error handling and type checking.


113-150: Integer accessor methods handle type conversions properly.

The implementation correctly handles various numeric types and string-to-int conversions, with proper error messaging for required accessors.


152-189: Float accessor methods handle type conversions properly.

The implementation correctly handles various numeric types and string-to-float conversions, with proper error messaging for required accessors.


191-232: Boolean accessor methods handle multiple representations.

Comprehensive implementation that converts from various types (string, int, float) to boolean values with appropriate error handling.


234-276: String slice accessor methods handle different array representations.

The implementation properly handles both native string slices and arrays of interfaces, with appropriate type checking in the required variant.


278-336: Int slice accessor methods thoroughly handle conversions.

Comprehensive handling of various source types, including proper error messages for conversion failures in the required variant.


338-396: Float slice accessor methods are well-implemented.

Consistent with the pattern established for other slice types, with proper error handling and type conversions.


398-460: Boolean slice accessor methods provide robust type handling.

The implementation properly converts from various source types to boolean values, maintaining consistency with other slice accessors.

@ezynda3 ezynda3 merged commit 28c9cc3 into main May 17, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: sdk SDK improvements unrelated to MCP specification type: enhancement New feature or enhancement request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arguments type is too much strict
3 participants