feat: add metadata support to MCP tools and refactor metadata tests#602
feat: add metadata support to MCP tools and refactor metadata tests#602alexshopee wants to merge 20 commits intogoogle:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the MCP toolset by integrating metadata forwarding capabilities, allowing request-scoped information to be passed to downstream MCP servers. Concurrently, it streamlines the associated test suite through refactoring, making it more concise and easier to maintain. These changes collectively improve the toolset's extensibility and the robustness of its testing infrastructure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces metadata support for MCP tools, allowing request-scoped data to be forwarded to tool calls. The implementation is clean and follows the description. The changes also include a nice refactoring of the metadata tests to use a generic helper function, which improves maintainability. I've added a couple of suggestions to further improve the robustness of the new tests by ensuring the tool handlers are always executed as expected.
tool/mcptoolset/set_test.go
Outdated
| var receivedMeta map[string]any | ||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| receivedMeta = req.Params.Meta | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
|
|
||
| testMetadata := map[string]any{ | ||
| "request_id": "test-123", | ||
| "user_id": "user-456", | ||
| "nested_data": map[string]any{"key": "value"}, | ||
| } | ||
| metadataProvider := func(ctx tool.Context) map[string]any { | ||
| return testMetadata | ||
| } | ||
|
|
||
| result := runMetadataTest(t, metadataProvider, echoToolFunc) | ||
| if result == nil { | ||
| t.Fatal("Expected non-nil result") | ||
| } | ||
|
|
||
| if diff := cmp.Diff(testMetadata, receivedMeta); diff != "" { | ||
| t.Errorf("metadata mismatch (-want +got):\n%s", diff) | ||
| } |
There was a problem hiding this comment.
For improved test robustness, it's a good practice to explicitly verify that the tool handler was actually executed. This can be done by using a boolean flag, similar to the pattern used in TestMetadataProviderReturnsNil. This ensures the test doesn't pass silently if the tool handler is never called.
var receivedMeta map[string]any
var toolCalled bool
echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) {
toolCalled = true
receivedMeta = req.Params.Meta
return nil, struct{ Message string }{Message: "ok"}, nil
}
testMetadata := map[string]any{
"request_id": "test-123",
"user_id": "user-456",
"nested_data": map[string]any{"key": "value"},
}
metadataProvider := func(ctx tool.Context) map[string]any {
return testMetadata
}
result := runMetadataTest(t, metadataProvider, echoToolFunc)
if result == nil {
t.Fatal("Expected non-nil result")
}
if !toolCalled {
t.Fatal("Tool was not called")
}
if diff := cmp.Diff(testMetadata, receivedMeta); diff != "" {
t.Errorf("metadata mismatch (-want +got):\n%s", diff)
}
tool/mcptoolset/set_test.go
Outdated
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| if req.Params.Meta != nil { | ||
| t.Errorf("Expected nil metadata, got %v", req.Params.Meta) | ||
| } | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
| _ = runMetadataTest(t, nil, echoToolFunc) |
There was a problem hiding this comment.
To make this test more robust, it's good practice to confirm that the tool handler was actually executed. Without this check, the test could pass silently if the tool handler is never called. You can add a boolean flag that is set within the handler and checked after the test run, similar to the pattern in TestMetadataProviderReturnsNil.
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | |
| if req.Params.Meta != nil { | |
| t.Errorf("Expected nil metadata, got %v", req.Params.Meta) | |
| } | |
| return nil, struct{ Message string }{Message: "ok"}, nil | |
| } | |
| _ = runMetadataTest(t, nil, echoToolFunc) | |
| var toolCalled bool | |
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | |
| toolCalled = true | |
| if req.Params.Meta != nil { | |
| t.Errorf("Expected nil metadata, got %v", req.Params.Meta) | |
| } | |
| return nil, struct{ Message string }{Message: "ok"}, nil | |
| } | |
| _ = runMetadataTest(t, nil, echoToolFunc) | |
| if !toolCalled { | |
| t.Fatal("Tool was not called") | |
| } |
alexshopee
force-pushed
the
alex.zhang/mcp-metadata-improvements
branch
from
9e52313 to
f5b9a91
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces metadata support for MCP tool calls, allowing request-scoped data to be forwarded to tools. The implementation is clean, adding a MetadataProvider to the mcptoolset.Config and correctly plumbing it through to the tool execution logic. The changes also include a nice refactoring of the metadata tests into a shared helper function, which improves maintainability. The test coverage for the new functionality is comprehensive, covering cases where a provider is present, absent, or returns nil metadata. I have a minor suggestion to improve consistency in the test code.
tool/mcptoolset/set_test.go
Outdated
| var metaCalled bool | ||
|
|
||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| metaCalled = true | ||
| receivedMeta = req.Params.Meta | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
|
|
||
| metadataProvider := func(ctx tool.Context) map[string]any { | ||
| return nil | ||
| } | ||
|
|
||
| _ = runMetadataTest(t, metadataProvider, echoToolFunc) | ||
|
|
||
| if !metaCalled { | ||
| t.Fatal("Tool was not called") | ||
| } |
There was a problem hiding this comment.
For consistency with the other metadata tests (TestMetadataProvider and TestMetadataProviderNil), please rename the metaCalled flag to toolCalled.
| var metaCalled bool | |
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | |
| metaCalled = true | |
| receivedMeta = req.Params.Meta | |
| return nil, struct{ Message string }{Message: "ok"}, nil | |
| } | |
| metadataProvider := func(ctx tool.Context) map[string]any { | |
| return nil | |
| } | |
| _ = runMetadataTest(t, metadataProvider, echoToolFunc) | |
| if !metaCalled { | |
| t.Fatal("Tool was not called") | |
| } | |
| var toolCalled bool | |
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | |
| toolCalled = true | |
| receivedMeta = req.Params.Meta | |
| return nil, struct{ Message string }{Message: "ok"}, nil | |
| } | |
| metadataProvider := func(ctx tool.Context) map[string]any { | |
| return nil | |
| } | |
| _ = runMetadataTest(t, metadataProvider, echoToolFunc) | |
| if !toolCalled { | |
| t.Fatal("Tool was not called") | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively adds metadata support to the MCP toolset and refactors the tests for better maintainability. The implementation is solid, and the test refactoring is a great improvement. I have a couple of suggestions to further improve code clarity and test consistency.
tool/mcptoolset/set_test.go
Outdated
| func TestMetadataProviderNil(t *testing.T) { | ||
| var toolCalled bool | ||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| toolCalled = true | ||
| if req.Params.Meta != nil { | ||
| t.Errorf("Expected nil metadata, got %v", req.Params.Meta) | ||
| } | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
| _ = runMetadataTest(t, nil, echoToolFunc) | ||
| if !toolCalled { | ||
| t.Fatal("Tool was not called") | ||
| } | ||
| } |
There was a problem hiding this comment.
For consistency with TestMetadataProvider and TestMetadataProviderReturnsNil, it's better to move the assertion out of the echoToolFunc. This keeps the tool handler's responsibility to a minimum (capturing results) and makes the test's assertion logic clearer by separating the 'act' and 'assert' phases of the test.
func TestMetadataProviderNil(t *testing.T) {
var receivedMeta map[string]any
var toolCalled bool
echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) {
toolCalled = true
receivedMeta = req.Params.Meta
return nil, struct{ Message string }{Message: "ok"}, nil
}
_ = runMetadataTest(t, nil, echoToolFunc)
if !toolCalled {
t.Fatal("Tool was not called")
}
if receivedMeta != nil {
t.Errorf("Expected nil metadata, got %v", receivedMeta)
}
}
tool/mcptoolset/tool.go
Outdated
| if meta := t.metadataProvider(ctx); meta != nil { | ||
| params.Meta = meta | ||
| } |
There was a problem hiding this comment.
The nested if statement is redundant. Assigning the result of t.metadataProvider(ctx) directly to params.Meta is sufficient, as params.Meta will correctly be nil if the provider returns nil. This simplifies the code and reduces nesting.
params.Meta = t.metadataProvider(ctx)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces metadata support for MCP tools, which is a valuable feature for passing request-scoped data. The implementation is clean and follows existing patterns. The refactoring of the metadata tests into a helper function is a good step towards better maintainability. I've suggested a further refactoring of the tests to use a table-driven approach, which would reduce duplication even more and make them easier to extend.
| func TestMetadataProvider(t *testing.T) { | ||
| var receivedMeta map[string]any | ||
| var toolCalled bool | ||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| toolCalled = true | ||
| receivedMeta = req.Params.Meta | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
|
|
||
| testMetadata := map[string]any{ | ||
| "request_id": "test-123", | ||
| "user_id": "user-456", | ||
| "nested_data": map[string]any{"key": "value"}, | ||
| } | ||
| metadataProvider := func(ctx tool.Context) map[string]any { | ||
| return testMetadata | ||
| } | ||
|
|
||
| result := runMetadataTest(t, metadataProvider, echoToolFunc) | ||
| if result == nil { | ||
| t.Fatal("Expected non-nil result") | ||
| } | ||
|
|
||
| if !toolCalled { | ||
| t.Fatal("Tool was not called") | ||
| } | ||
|
|
||
| if diff := cmp.Diff(testMetadata, receivedMeta); diff != "" { | ||
| t.Errorf("metadata mismatch (-want +got):\n%s", diff) | ||
| } | ||
| } | ||
|
|
||
| func TestMetadataProviderNil(t *testing.T) { | ||
| var receivedMeta map[string]any | ||
| var toolCalled bool | ||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| toolCalled = true | ||
| receivedMeta = req.Params.Meta | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
| _ = runMetadataTest(t, nil, echoToolFunc) | ||
| if !toolCalled { | ||
| t.Fatal("Tool was not called") | ||
| } | ||
| if receivedMeta != nil { | ||
| t.Errorf("Expected nil metadata, got %v", receivedMeta) | ||
| } | ||
| } | ||
|
|
||
| func TestMetadataProviderReturnsNil(t *testing.T) { | ||
| var receivedMeta map[string]any | ||
| var toolCalled bool | ||
|
|
||
| echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) { | ||
| toolCalled = true | ||
| receivedMeta = req.Params.Meta | ||
| return nil, struct{ Message string }{Message: "ok"}, nil | ||
| } | ||
|
|
||
| metadataProvider := func(ctx tool.Context) map[string]any { | ||
| return nil | ||
| } | ||
|
|
||
| _ = runMetadataTest(t, metadataProvider, echoToolFunc) | ||
|
|
||
| if !toolCalled { | ||
| t.Fatal("Tool was not called") | ||
| } | ||
|
|
||
| if receivedMeta != nil { | ||
| t.Errorf("Expected nil metadata when provider returns nil, got %v", receivedMeta) | ||
| } | ||
| } |
There was a problem hiding this comment.
These three tests for metadata handling are very similar and contain duplicated setup for the echoToolFunc and the variables to capture its side effects. You can combine them into a single table-driven test to improve maintainability and reduce boilerplate. This also makes it easier to add new metadata-related test cases in the future.
func TestMetadata(t *testing.T) {
testMetadata := map[string]any{
"request_id": "test-123",
"user_id": "user-456",
"nested_data": map[string]any{"key": "value"},
}
testCases := []struct {
name string
provider mcptoolset.MetadataProvider
wantMetadata map[string]any
}{
{
name: "provider returns metadata",
provider: func(ctx tool.Context) map[string]any {
return testMetadata
},
wantMetadata: testMetadata,
},
{
name: "provider is nil",
provider: nil,
wantMetadata: nil,
},
{
name: "provider returns nil",
provider: func(ctx tool.Context) map[string]any {
return nil
},
wantMetadata: nil,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
var receivedMeta map[string]any
var toolCalled bool
echoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, input struct{}) (*mcp.CallToolResult, struct{ Message string }, error) {
toolCalled = true
receivedMeta = req.Params.Meta
return nil, struct{ Message string }{Message: "ok"}, nil
}
result := runMetadataTest(t, tc.provider, echoToolFunc)
if result == nil {
t.Fatal("Expected non-nil result")
}
if !toolCalled {
t.Fatal("Tool was not called")
}
if diff := cmp.Diff(tc.wantMetadata, receivedMeta); diff != "" {
t.Errorf("metadata mismatch (-want +got):\n%s", diff)
}
})
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully adds metadata support to the MCP toolset and refactors the associated tests for improved maintainability. The implementation is clean and the test coverage is good. I have a couple of suggestions to further enhance the clarity and structure of the new tests.
tool/mcptoolset/set_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestMetadata(t *testing.T) { |
There was a problem hiding this comment.
For better clarity and consistency, consider renaming this test function from TestMetadata to TestMetadataProvider. This name more accurately reflects that the test is focused on the behavior of the MetadataProvider in different scenarios (when it's nil, returns nil, or returns data).
| func TestMetadata(t *testing.T) { | |
| func TestMetadataProvider(t *testing.T) { |
tool/mcptoolset/set_test.go
Outdated
| func runMetadataTest[In, Out any](t *testing.T, provider mcptoolset.MetadataProvider, toolFunc mcp.ToolHandlerFor[In, Out]) map[string]any { | ||
| t.Helper() | ||
|
|
||
| clientTransport, serverTransport := mcp.NewInMemoryTransports() | ||
| server := mcp.NewServer(&mcp.Implementation{Name: "test_server", Version: "v1.0.0"}, nil) | ||
| mcp.AddTool(server, &mcp.Tool{Name: "echo_tool", Description: "echoes input"}, toolFunc) | ||
| _, err := server.Connect(t.Context(), serverTransport, nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| ts, err := mcptoolset.New(mcptoolset.Config{ | ||
| Transport: clientTransport, | ||
| MetadataProvider: provider, | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create MCP tool set: %v", err) | ||
| } | ||
|
|
||
| invCtx := icontext.NewInvocationContext(t.Context(), icontext.InvocationContextParams{}) | ||
| readonlyCtx := icontext.NewReadonlyContext(invCtx) | ||
| tools, err := ts.Tools(readonlyCtx) | ||
| if err != nil { | ||
| t.Fatalf("Failed to get tools: %v", err) | ||
| } | ||
|
|
||
| if len(tools) != 1 { | ||
| t.Fatalf("Expected 1 tool, got %d", len(tools)) | ||
| } | ||
|
|
||
| fnTool, ok := tools[0].(toolinternal.FunctionTool) | ||
| if !ok { | ||
| t.Fatal("Tool does not implement FunctionTool interface") | ||
| } | ||
|
|
||
| toolCtx := toolinternal.NewToolContext(invCtx, "", nil, nil) | ||
| result, err := fnTool.Run(toolCtx, map[string]any{}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to run tool: %v", err) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The runMetadataTest helper function can be improved by making it self-contained in its validation and not returning a value. Currently, it returns the tool's result, which is only used for a nil check in the calling test. By moving this nil check inside the helper and changing its signature to not return anything, you can make the main test logic cleaner and more focused on its specific assertions. This improves the separation of concerns, where the helper is responsible for all setup and generic validation.
After applying this change, you should also update the call site in TestMetadata (lines 837-840) to runMetadataTest(t, tc.provider, echoToolFunc).
func runMetadataTest[In, Out any](t *testing.T, provider mcptoolset.MetadataProvider, toolFunc mcp.ToolHandlerFor[In, Out]) {
t.Helper()
clientTransport, serverTransport := mcp.NewInMemoryTransports()
server := mcp.NewServer(&mcp.Implementation{Name: "test_server", Version: "v1.0.0"}, nil)
mcp.AddTool(server, &mcp.Tool{Name: "echo_tool", Description: "echoes input"}, toolFunc)
_, err := server.Connect(t.Context(), serverTransport, nil)
if err != nil {
t.Fatal(err)
}
ts, err := mcptoolset.New(mcptoolset.Config{
Transport: clientTransport,
MetadataProvider: provider,
})
if err != nil {
t.Fatalf("Failed to create MCP tool set: %v", err)
}
invCtx := icontext.NewInvocationContext(t.Context(), icontext.InvocationContextParams{})
readonlyCtx := icontext.NewReadonlyContext(invCtx)
tools, err := ts.Tools(readonlyCtx)
if err != nil {
t.Fatalf("Failed to get tools: %v", err)
}
if len(tools) != 1 {
t.Fatalf("Expected 1 tool, got %d", len(tools))
}
fnTool, ok := tools[0].(toolinternal.FunctionTool)
if !ok {
t.Fatal("Tool does not implement FunctionTool interface")
}
toolCtx := toolinternal.NewToolContext(invCtx, "", nil, nil)
result, err := fnTool.Run(toolCtx, map[string]any{})
if err != nil {
t.Fatalf("Failed to run tool: %v", err)
}
if result == nil {
t.Fatal("Expected non-nil result from tool run")
}
}…nstead of return the result to calling functions
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully adds metadata support to the MCP toolset, enabling the forwarding of request-scoped metadata to MCP tool calls. The changes are well-integrated across the set.go and tool.go files, introducing a MetadataProvider type and incorporating it into the Config and mcpTool structures. The Run method in mcpTool correctly utilizes this provider to attach metadata to CallToolParams.
Additionally, the refactoring of metadata tests in set_test.go into a generic helper function runMetadataTest significantly improves maintainability and reduces duplication, as described in the PR body. The test cases cover various scenarios for the MetadataProvider, including when it returns metadata, is nil, or returns nil. Overall, the changes are clear, functional, and enhance the toolset's capabilities.
tool/mcptoolset/set_test.go
Outdated
| if result == nil { | ||
| t.Fatal("Expected non-nil result from tool run") | ||
| } |
There was a problem hiding this comment.
The check if result == nil is redundant and potentially misleading here. The echoToolFunc returns a structured output, which mcpTool.Run wraps into a map[string]any. Therefore, result will never be nil in this scenario. The t.Fatalf("Failed to run tool: %v", err) already handles actual errors during tool execution. Removing this check would make the test clearer.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces metadata support for MCP tools, allowing request-scoped data to be forwarded to tool calls. The changes are well-implemented across the toolset configuration, tool conversion, and tool execution logic. Additionally, the tests for metadata handling have been refactored into a reusable helper function, which improves maintainability. My main feedback is to enhance the robustness of the mcpTool.Run method by adding panic recovery, similar to how other tools handle user-provided code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a MetadataProvider to forward request-scoped metadata to MCP tool calls, a valuable feature for tracing and authentication. The implementation is clean and correctly handles the different provider states (present, nil, or returning nil). The associated tests have been effectively refactored into a single, table-driven test with a generic helper function, which greatly improves maintainability and follows best practices. Additionally, a panic recovery mechanism has been added to the tool's Run method, enhancing the toolset's robustness. I've suggested a small improvement to make the panic error more structured and programmatically inspectable, but overall the changes are excellent.
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("panic in tool %q: %v\nstack: %s", t.Name(), r, debug.Stack()) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The panic recovery is a great addition for robustness. To make these panic errors more programmatically accessible and inspectable by callers, consider wrapping the panic information in a dedicated structured error type instead of a formatted string. This allows consumers of the toolset to type-assert the error and handle tool panics differently from other errors, for example, by logging the stack trace separately or extracting panic details for monitoring.
For example, you could define a custom error type:
// ToolPanicError represents an error caused by a panic within a tool's execution.
type ToolPanicError struct {
ToolName string
PanicValue any
Stack []byte
}
func (e *ToolPanicError) Error() string {
return fmt.Sprintf("panic in tool %q: %v", e.ToolName, e.PanicValue)
}And then use it in the defer block:
ddefer func() {
if r := recover(); r != nil {
err = &ToolPanicError{
ToolName: t.Name(),
PanicValue: r,
Stack: debug.Stack(),
}
}
}()This makes the error much more useful for upstream callers.
|
hi @dpasiukevich i suppose the codes have been improved with the instructions of gemini, could u help take a look at this pr? In addition, i just follow the panic recover mechanism regarding the functionTool as below: and i suppose there is no need to follow the gemini's latest comment, if there is no other problems, pls merge this pr, thank u ! |
|
hi @baptmont could u help take a look at this pr, i appreciate it |
2 participants
Summary
This PR is based on PR #475 (“feat: add metadata support to MCP tools”) by @philippslang (Philipp Lang). It implements the same feature for Issue #469, and adds test refactoring and robustness improvements from code review.
Issue & related PR
Purpose (from #475)
MetadataProvideronmcptoolset.Config; when set and returning non-nil metadata, attach it tomcp.CallToolParams.Metabefore calling the MCP tool.Detailed comparison: what this PR changes on top of #475
Functionality (production code)
if t.metadataProvider != nil { if meta := t.metadataProvider(ctx); meta != nil { params.Meta = meta } }if t.metadataProvider != nil { params.Meta = t.metadataProvider(ctx) }— direct assignment (nil is fine forparams.Meta).defer recover()inmcpTool.Run, converting panic to an error with tool name and stack (same pattern asfunctiontool.Run), so a panickingMetadataProviderdoes not crash the process.No other API or config changes;
MetadataProvidertype andConfig/setwiring match #475.Unit tests (set_test.go)
TestMetadataProvider,TestMetadataProviderNil,TestMetadataProviderReturnsNil. Each duplicated: in-memory transport, server,AddTool,mcptoolset.New,Tools(), get first tool,Run(toolCtx, map[string]any{}).TestMetadataProviderwith three cases:"provider returns metadata","provider is nil","provider returns nil". Shared setup and run moved into a generic helperrunMetadataTest[In, Out any](t, provider, toolFunc).runMetadataTest: builds in-memory transport & server, registers one tool withtoolFunc, creates toolset with givenprovider, callsTools(), runs the single tool withmap[string]any{}. No return value; failures viat.Fatal/t.Fatalf(e.g. tool run error).TestMetadataProviderReturnsNilused ametaCalledflag and asserted the tool was called.toolCalledflag set in the handler and asserttoolCalledafterrunMetadataTest, so no case passes if the handler is never invoked.metaCalledin one test.toolCalledeverywhere; test nameTestMetadata→TestMetadataProviderto reflect metadata-provider behavior.TestMetadataProviderNilcould assert inside the handler (req.Params.Meta != nil→t.Errorf).toolCalled = true,receivedMeta = req.Params.Meta); assertion outside the handler:if receivedMeta != nil { t.Errorf(...) }, keeping act vs assert separate.result := runMetadataTest(...); if result == nil { t.Fatal(...) }.result == nilcheck in helper was removed (per review); callers only asserttoolCalledandreceivedMeta(or nil).Coverage: Same three scenarios as #475 (provider returns metadata; provider is nil; provider returns nil), with added assurance that the tool handler is actually run in each case.
Code follows the Google Go Style Guide. Changes are limited to
tool/mcptoolset(set.go, tool.go, set_test.go).Testing plan
Unit tests
tool/mcptoolsetmetadata behavior and refactored tests.toolCalled).go test ./tool/mcptoolset/ -run 'TestMetadataProvider' -v