From 1a6e45cbc7314075a50619fbe7c2de91bde04b29 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Sat, 11 Apr 2026 18:39:24 +0300 Subject: [PATCH] feat(mcp): advertise native args object on call_tool_* variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an `args` object parameter alongside `args_json` on call_tool_read, call_tool_write, and call_tool_destructive. The handler already accepted `args` as a fallback, but the schema did not advertise it — so LLM clients always produced pre-serialized JSON strings, costing ~350 tokens per call and producing frequent escaping bugs (see luutuankiet/mcp-proxy-shim for context). Clients can now send arguments as a native JSON object: { "name": "github:get_user", "args": { "username": "octocat" } } instead of: { "name": "github:get_user", "args_json": "{\"username\":\"octocat\"}" } `args_json` remains advertised and takes precedence when both are provided, so existing clients continue to work unchanged. The tool definitions for the default `/mcp` and `/mcp/call` routes were duplicated across mcp.go and mcp_routing.go; factored them into a shared `buildCallToolVariantTool(variant)` helper to keep the schema in sync. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/server/mcp.go | 126 +++++++++++---------- internal/server/mcp_call_tool_args_test.go | 125 ++++++++++++++++++++ internal/server/mcp_routing.go | 68 +---------- 3 files changed, 198 insertions(+), 121 deletions(-) create mode 100644 internal/server/mcp_call_tool_args_test.go diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 10729964..918dd427 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -387,6 +387,70 @@ func (p *MCPProxyServer) emitActivityInternalToolCall(internalToolName, targetSe } } +// buildCallToolVariantTool constructs the mcp.Tool definition for a single +// call_tool_* variant (read/write/destructive). Factored out so both +// registerTools and schema tests exercise the same builder. +func buildCallToolVariantTool(variant string) mcp.Tool { + var description, title, nameExample, sensitivityDesc, reasonDesc string + var opts []mcp.ToolOption + + switch variant { + case contracts.ToolVariantRead: + description = "Execute a READ-ONLY tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. Examples: search_files, get_user, list_repositories, query_database, find_issues, check_status. This is the DEFAULT choice when unsure - most tools are read-only." + title = "Call Tool (Read)" + nameExample = "github:get_user" + sensitivityDesc = "Classify data being accessed: public, internal, private, or unknown. Helps track sensitive data access patterns." + reasonDesc = "Why is this tool being called? Provide context like 'User asked to check status' or 'Gathering data for report'." + opts = []mcp.ToolOption{mcp.WithReadOnlyHintAnnotation(true)} + case contracts.ToolVariantWrite: + description = "Execute a STATE-MODIFYING tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit, assign, configure, enable, register, subscribe, publish, move, copy, rename, merge. Examples: create_issue, update_file, send_message, add_comment, set_status, edit_page. Use only when explicitly modifying state." + title = "Call Tool (Write)" + nameExample = "github:create_issue" + sensitivityDesc = "Classify data being modified: public, internal, private, or unknown. Helps track sensitive data changes." + reasonDesc = "Why is this modification needed? Provide context like 'User requested update' or 'Fixing reported issue'." + opts = []mcp.ToolOption{mcp.WithDestructiveHintAnnotation(false)} + case contracts.ToolVariantDestructive: + description = "Execute a DESTRUCTIVE tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: delete, remove, drop, revoke, disable, destroy, purge, reset, clear, unsubscribe, cancel, terminate, close, archive, ban, block, disconnect, kill, wipe, truncate, force, hard. Examples: delete_repo, remove_user, drop_table, revoke_access, clear_cache, terminate_session. Use for irreversible or high-impact operations." + title = "Call Tool (Destructive)" + nameExample = "github:delete_repo" + sensitivityDesc = "Classify data being deleted: public, internal, private, or unknown. Important for tracking destructive operations on sensitive data." + reasonDesc = "Why is this deletion needed? Provide justification like 'User confirmed cleanup' or 'Removing obsolete data'." + opts = []mcp.ToolOption{mcp.WithDestructiveHintAnnotation(true)} + default: + description = "Execute a tool." + title = "Call Tool" + nameExample = "server:tool" + sensitivityDesc = "Classify data sensitivity." + reasonDesc = "Why is this tool being called?" + } + + allOpts := []mcp.ToolOption{ + mcp.WithDescription(description), + mcp.WithTitleAnnotation(title), + } + allOpts = append(allOpts, opts...) + allOpts = append(allOpts, + mcp.WithString("name", + mcp.Required(), + mcp.Description(fmt.Sprintf("Tool name in format 'server:tool' (e.g., '%s'). CRITICAL: You MUST use exact names from retrieve_tools results - do NOT guess or invent server names. Unknown servers will fail.", nameExample)), + ), + mcp.WithObject("args", + mcp.Description("Arguments to pass to the upstream tool as a native JSON object. Refer to the tool's inputSchema from retrieve_tools for required parameters. Example: {\"path\": \"src/index.ts\", \"limit\": 20}. This is the preferred parameter — it eliminates JSON escaping overhead. Use 'args_json' only if your client cannot produce nested JSON objects."), + ), + mcp.WithString("args_json", + mcp.Description("Legacy: arguments as a pre-serialized JSON string. Prefer the 'args' parameter instead — it accepts a native JSON object and eliminates escaping overhead. If both are provided, 'args_json' wins for backward compatibility."), + ), + mcp.WithString("intent_data_sensitivity", + mcp.Description(sensitivityDesc), + ), + mcp.WithString("intent_reason", + mcp.Description(reasonDesc), + ), + ) + + return mcp.NewTool(variant, allOpts...) +} + // registerTools registers all proxy tools with the MCP server func (p *MCPProxyServer) registerTools(_ bool) { // retrieve_tools - THE PRIMARY TOOL FOR DISCOVERING TOOLS - Enhanced with clear instructions @@ -416,69 +480,13 @@ func (p *MCPProxyServer) registerTools(_ bool) { // Intent-based tool variants (Spec 018) // These replace the legacy call_tool with three operation-specific variants // that enable granular IDE permission control and require explicit intent declaration. - - // call_tool_read - Read-only operations - // NOTE: Intent parameters are flattened (not nested objects) for Gemini 3 Pro compatibility - callToolReadTool := mcp.NewTool(contracts.ToolVariantRead, - mcp.WithDescription("Execute a READ-ONLY tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. Examples: search_files, get_user, list_repositories, query_database, find_issues, check_status. This is the DEFAULT choice when unsure - most tools are read-only."), - mcp.WithTitleAnnotation("Call Tool (Read)"), - mcp.WithReadOnlyHintAnnotation(true), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:get_user'). CRITICAL: You MUST use exact names from retrieve_tools results - do NOT guess or invent server names. Unknown servers will fail."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments to pass to the tool as JSON string. Refer to the tool's inputSchema from retrieve_tools for required parameters."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data being accessed: public, internal, private, or unknown. Helps track sensitive data access patterns."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this tool being called? Provide context like 'User asked to check status' or 'Gathering data for report'."), - ), - ) + callToolReadTool := buildCallToolVariantTool(contracts.ToolVariantRead) p.server.AddTool(callToolReadTool, p.handleCallToolRead) - // call_tool_write - State-modifying operations - callToolWriteTool := mcp.NewTool(contracts.ToolVariantWrite, - mcp.WithDescription("Execute a STATE-MODIFYING tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit, assign, configure, enable, register, subscribe, publish, move, copy, rename, merge. Examples: create_issue, update_file, send_message, add_comment, set_status, edit_page. Use only when explicitly modifying state."), - mcp.WithTitleAnnotation("Call Tool (Write)"), - mcp.WithDestructiveHintAnnotation(false), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:create_issue'). CRITICAL: You MUST use exact names from retrieve_tools results - do NOT guess or invent server names. Unknown servers will fail."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments to pass to the tool as JSON string. Refer to the tool's inputSchema from retrieve_tools for required parameters."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data being modified: public, internal, private, or unknown. Helps track sensitive data changes."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this modification needed? Provide context like 'User requested update' or 'Fixing reported issue'."), - ), - ) + callToolWriteTool := buildCallToolVariantTool(contracts.ToolVariantWrite) p.server.AddTool(callToolWriteTool, p.handleCallToolWrite) - // call_tool_destructive - Irreversible operations - callToolDestructiveTool := mcp.NewTool(contracts.ToolVariantDestructive, - mcp.WithDescription("Execute a DESTRUCTIVE tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. DECISION RULE: Use this when the tool name contains: delete, remove, drop, revoke, disable, destroy, purge, reset, clear, unsubscribe, cancel, terminate, close, archive, ban, block, disconnect, kill, wipe, truncate, force, hard. Examples: delete_repo, remove_user, drop_table, revoke_access, clear_cache, terminate_session. Use for irreversible or high-impact operations."), - mcp.WithTitleAnnotation("Call Tool (Destructive)"), - mcp.WithDestructiveHintAnnotation(true), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:delete_repo'). CRITICAL: You MUST use exact names from retrieve_tools results - do NOT guess or invent server names. Unknown servers will fail."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments to pass to the tool as JSON string. Refer to the tool's inputSchema from retrieve_tools for required parameters."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data being deleted: public, internal, private, or unknown. Important for tracking destructive operations on sensitive data."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this deletion needed? Provide justification like 'User confirmed cleanup' or 'Removing obsolete data'."), - ), - ) + callToolDestructiveTool := buildCallToolVariantTool(contracts.ToolVariantDestructive) p.server.AddTool(callToolDestructiveTool, p.handleCallToolDestructive) // read_cache - Access paginated data when responses are truncated diff --git a/internal/server/mcp_call_tool_args_test.go b/internal/server/mcp_call_tool_args_test.go new file mode 100644 index 00000000..657e473c --- /dev/null +++ b/internal/server/mcp_call_tool_args_test.go @@ -0,0 +1,125 @@ +package server + +import ( + "context" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream" +) + +// TestCallToolVariantSchemaAdvertisesArgsObject verifies that the +// call_tool_read/write/destructive tool schemas advertise a native +// `args` object parameter alongside the legacy `args_json` string. +// This eliminates the escaping overhead when LLM clients produce +// tool arguments (see mcp-proxy-shim README for background). +func TestCallToolVariantSchemaAdvertisesArgsObject(t *testing.T) { + variants := []string{ + contracts.ToolVariantRead, + contracts.ToolVariantWrite, + contracts.ToolVariantDestructive, + } + + for _, variant := range variants { + t.Run(variant, func(t *testing.T) { + tool := buildCallToolVariantTool(variant) + + // Legacy string path must remain advertised for backward compat. + argsJSONProp, ok := tool.InputSchema.Properties["args_json"].(map[string]any) + require.True(t, ok, "schema must still advertise 'args_json' (backward compat)") + assert.Equal(t, "string", argsJSONProp["type"]) + + // New native object path must be advertised. + argsProp, ok := tool.InputSchema.Properties["args"].(map[string]any) + require.True(t, ok, "schema must advertise 'args' as an object property") + assert.Equal(t, "object", argsProp["type"]) + + // `args` must not be required (args_json is still accepted). + for _, req := range tool.InputSchema.Required { + assert.NotEqual(t, "args", req, "'args' must not be required") + } + }) + } +} + +// TestHandleCallToolVariantAcceptsArgsObject verifies the handler extracts a +// native `args` object from the request and routes past the argument-parsing +// phase without emitting "Invalid args_json format" — i.e. the native object +// path actually works, not just the schema. +func TestHandleCallToolVariantAcceptsArgsObject(t *testing.T) { + mockProxy := &MCPProxyServer{ + upstreamManager: upstream.NewManager(zap.NewNop(), config.DefaultConfig(), nil, secret.NewResolver(), nil), + logger: zap.NewNop(), + config: &config.Config{}, + } + ctx := context.Background() + + request := mcp.CallToolRequest{} + request.Params.Name = contracts.ToolVariantRead + request.Params.Arguments = map[string]any{ + "name": "non-existent-server:some_tool", + "args": map[string]any{ + "files": []any{ + map[string]any{"path": "src/index.ts", "head": 20}, + }, + }, + } + + result, err := mockProxy.handleCallToolVariant(ctx, request, contracts.ToolVariantRead) + require.NoError(t, err) + require.NotNil(t, result) + require.True(t, result.IsError, "non-existent server should yield an error result") + + // The error must NOT be an argument-parsing error. The handler may still + // fail later in the dispatch pipeline (because this mock proxy has no + // real runtime), but it must get past argument extraction — proving the + // native object path was accepted. + require.Len(t, result.Content, 1) + textContent, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok, "expected TextContent") + errMsg := textContent.Text + assert.NotContains(t, errMsg, "Invalid args_json format", + "handler should not attempt args_json parsing when only 'args' object is provided") + assert.NotContains(t, errMsg, "Missing required parameter", + "required parameters are present — 'args' object must be accepted") +} + +// TestHandleCallToolVariantArgsJSONWinsWhenBothProvided verifies backward +// compatibility: when a client sends BOTH args_json and args, args_json takes +// precedence. This preserves existing behavior so migration is gradual. +func TestHandleCallToolVariantArgsJSONWinsWhenBothProvided(t *testing.T) { + mockProxy := &MCPProxyServer{ + upstreamManager: upstream.NewManager(zap.NewNop(), config.DefaultConfig(), nil, secret.NewResolver(), nil), + logger: zap.NewNop(), + config: &config.Config{}, + } + ctx := context.Background() + + // Malformed args_json + well-formed args → must surface args_json parse + // error, proving args_json took precedence. + request := mcp.CallToolRequest{} + request.Params.Name = contracts.ToolVariantRead + request.Params.Arguments = map[string]any{ + "name": "non-existent-server:some_tool", + "args_json": "not valid json", + "args": map[string]any{"ok": true}, + } + + result, err := mockProxy.handleCallToolVariant(ctx, request, contracts.ToolVariantRead) + require.NoError(t, err) + require.NotNil(t, result) + require.True(t, result.IsError) + + require.Len(t, result.Content, 1) + textContent, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, textContent.Text, "Invalid args_json format", + "args_json must take precedence when both are provided (backward compat)") +} diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index d45b401f..3bcfc3db 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -342,75 +342,19 @@ func (p *MCPProxyServer) buildCallToolModeTools() []mcpserver.ServerTool { Handler: p.handleRetrieveToolsForMode(config.RoutingModeRetrieveTools), }) - // call_tool_read - callToolReadTool := mcp.NewTool(contracts.ToolVariantRead, - mcp.WithDescription("Execute a READ-ONLY tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. Use this for: search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. This is the DEFAULT choice when unsure."), - mcp.WithTitleAnnotation("Call Tool (Read)"), - mcp.WithReadOnlyHintAnnotation(true), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:get_user'). Use exact names from retrieve_tools results."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments as JSON string. Refer to the tool's inputSchema from retrieve_tools."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data: public, internal, private, or unknown."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this tool being called? Provide context."), - ), - ) + // call_tool_read / call_tool_write / call_tool_destructive — all three + // built from the shared helper in mcp.go so schema stays in sync across + // the default and retrieve_tools routing modes. tools = append(tools, mcpserver.ServerTool{ - Tool: callToolReadTool, + Tool: buildCallToolVariantTool(contracts.ToolVariantRead), Handler: p.handleCallToolRead, }) - - // call_tool_write - callToolWriteTool := mcp.NewTool(contracts.ToolVariantWrite, - mcp.WithDescription("Execute a STATE-MODIFYING tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. Use this for: create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit. Use only when explicitly modifying state."), - mcp.WithTitleAnnotation("Call Tool (Write)"), - mcp.WithDestructiveHintAnnotation(false), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:create_issue'). Use exact names from retrieve_tools results."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments as JSON string. Refer to the tool's inputSchema from retrieve_tools."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data: public, internal, private, or unknown."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this modification needed? Provide context."), - ), - ) tools = append(tools, mcpserver.ServerTool{ - Tool: callToolWriteTool, + Tool: buildCallToolVariantTool(contracts.ToolVariantWrite), Handler: p.handleCallToolWrite, }) - - // call_tool_destructive - callToolDestructiveTool := mcp.NewTool(contracts.ToolVariantDestructive, - mcp.WithDescription("Execute a DESTRUCTIVE tool. WORKFLOW: 1) Call retrieve_tools first to find tools, 2) Use the exact 'name' field from results. Use this for: delete, remove, drop, revoke, disable, destroy, purge, reset, clear, terminate. Use for irreversible or high-impact operations."), - mcp.WithTitleAnnotation("Call Tool (Destructive)"), - mcp.WithDestructiveHintAnnotation(true), - mcp.WithString("name", - mcp.Required(), - mcp.Description("Tool name in format 'server:tool' (e.g., 'github:delete_repo'). Use exact names from retrieve_tools results."), - ), - mcp.WithString("args_json", - mcp.Description("Arguments as JSON string. Refer to the tool's inputSchema from retrieve_tools."), - ), - mcp.WithString("intent_data_sensitivity", - mcp.Description("Classify data: public, internal, private, or unknown."), - ), - mcp.WithString("intent_reason", - mcp.Description("Why is this deletion needed? Provide justification."), - ), - ) tools = append(tools, mcpserver.ServerTool{ - Tool: callToolDestructiveTool, + Tool: buildCallToolVariantTool(contracts.ToolVariantDestructive), Handler: p.handleCallToolDestructive, })