From c070e2fc09926983ec9d7f6cf2f6327d6be18b89 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 23:44:22 +0000 Subject: [PATCH 1/5] Initial plan From d13d9008673daf73704c113e451a60ee95805a4d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 00:14:24 +0000 Subject: [PATCH 2/5] feat: transform MCP "additional properties" validation errors into helpful messages with Did you mean suggestions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b4192ef2-4d95-44d3-bcc9-d4c22a63131d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_argument_validation.go | 266 ++++++++++++++++++++ pkg/cli/mcp_argument_validation_test.go | 307 ++++++++++++++++++++++++ pkg/cli/mcp_server.go | 4 + 3 files changed, 577 insertions(+) create mode 100644 pkg/cli/mcp_argument_validation.go create mode 100644 pkg/cli/mcp_argument_validation_test.go diff --git a/pkg/cli/mcp_argument_validation.go b/pkg/cli/mcp_argument_validation.go new file mode 100644 index 00000000000..cd0a0174ade --- /dev/null +++ b/pkg/cli/mcp_argument_validation.go @@ -0,0 +1,266 @@ +package cli + +// This file implements an MCP receiving middleware that transforms raw JSON schema +// "additional properties" validation errors into helpful, user-friendly messages +// with "Did you mean?" suggestions. +// +// Background: When the MCP SDK validates tool arguments against the input schema +// (which uses additionalProperties: false), it emits a raw message like: +// +// validating "arguments": validating root: unexpected additional properties ["workflow-name"] +// +// This is surfaced directly to users, leaking internal validation details without +// any guidance on the correct parameter name. The middleware here intercepts +// those tool-error results and replaces the message with a helpful alternative. + +import ( + "context" + "fmt" + "regexp" + "sort" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// toolParamEntry holds the valid parameter names for a single MCP tool. +type toolParamEntry = []string + +// argumentValidationMiddleware returns a mcp.Middleware that intercepts tool-call +// results containing "unexpected additional properties" validation errors and +// replaces them with a helpful message that names the unknown parameter, suggests +// a close match, and points to the tool's --help output. +// +// toolParams maps tool names to their list of valid JSON parameter names. It is +// built from the tool input schemas during server construction. +func argumentValidationMiddleware(toolParams map[string]toolParamEntry) mcp.Middleware { + return func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) { + result, err := next(ctx, method, req) + if err != nil || method != "tools/call" { + return result, err + } + + // Check whether the result is a tool error containing a schema + // "additional properties" validation message. + toolResult, ok := result.(*mcp.CallToolResult) + if !ok || !toolResult.IsError { + return result, err + } + + // Extract the error text from the first TextContent element. + if len(toolResult.Content) == 0 { + return result, err + } + textContent, ok := toolResult.Content[0].(*mcp.TextContent) + if !ok { + return result, err + } + errMsg := textContent.Text + + if !strings.Contains(errMsg, "unexpected additional properties") { + return result, err + } + + // Parse the unknown parameter names from the error text. + unknownParams := extractUnknownParams(errMsg) + if len(unknownParams) == 0 { + return result, err + } + + // Determine the tool name from the request so we can look up valid params. + toolName := extractMCPToolName(req) + validParams := toolParams[toolName] + + // Build a helpful replacement message. + helpMsg := buildHelpfulParamError(toolName, unknownParams, validParams) + + // Return a modified tool result with the helpful message, preserving IsError. + replaced := *toolResult + replaced.Content = []mcp.Content{&mcp.TextContent{Text: helpMsg}} + return &replaced, nil + } + } +} + +// extractMCPToolName retrieves the tool name from a MCP Request by casting the +// request params to *mcp.CallToolParamsRaw. Returns an empty string if the cast +// fails. +func extractMCPToolName(req mcp.Request) string { + if p, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok { + return p.Name + } + return "" +} + +// extractUnknownParams parses the JSON-schema validation error string to extract +// the list of unknown parameter names. +// +// The expected format (from jsonschema-go) is: +// +// unexpected additional properties ["name1" "name2"] +// +// which uses %q-style quoting for a []string. +var additionalPropsRE = regexp.MustCompile(`unexpected additional properties (.+)$`) +var quotedStringRE = regexp.MustCompile(`"([^"]+)"`) + +func extractUnknownParams(errMsg string) []string { + m := additionalPropsRE.FindStringSubmatch(errMsg) + if m == nil { + return nil + } + raw := m[1] + matches := quotedStringRE.FindAllStringSubmatch(raw, -1) + var params []string + for _, sm := range matches { + if sm[1] != "" { + params = append(params, sm[1]) + } + } + return params +} + +// buildHelpfulParamError constructs a human-readable error message that: +// - names each unknown parameter +// - suggests the closest valid parameter (if a good match is found) +// - directs the user to the tool's --help output +func buildHelpfulParamError(toolName string, unknownParams []string, validParams []string) string { + var sb strings.Builder + + for i, param := range unknownParams { + if i > 0 { + sb.WriteString("\n") + } + sb.WriteString(fmt.Sprintf("Unknown parameter '%s'.", param)) + if suggestion := findSimilarParam(param, validParams); suggestion != "" { + sb.WriteString(fmt.Sprintf(" Did you mean '%s'?", suggestion)) + } + } + + if toolName != "" { + sb.WriteString(fmt.Sprintf("\nRun 'agenticworkflows %s --help' for usage.", toolName)) + } + + return sb.String() +} + +// findSimilarParam returns the valid parameter name most similar to unknown, or +// an empty string if no parameter is close enough. +// +// Similarity is measured by the ratio of the longest-common-prefix length to the +// shorter of the two normalized strings. A threshold of 0.7 (70%) is required. +// +// Normalization: lowercase, hyphens and underscores removed. +func findSimilarParam(unknown string, validParams []string) string { + if len(validParams) == 0 { + return "" + } + + normUnknown := normalizeParamName(unknown) + + type candidate struct { + name string + score float64 + } + var best candidate + + for _, p := range validParams { + normP := normalizeParamName(p) + + // Exact normalized match wins immediately. + if normP == normUnknown { + return p + } + + lcp := longestCommonPrefixLen(normUnknown, normP) + shorter := min(len(normP), len(normUnknown)) + if shorter == 0 { + continue + } + score := float64(lcp) / float64(shorter) + if score > best.score { + best = candidate{name: p, score: score} + } + } + + const threshold = 0.7 + if best.score >= threshold { + return best.name + } + return "" +} + +// normalizeParamName lowercases name and removes hyphens and underscores, so +// that "workflow-name", "workflow_name", and "workflowname" all compare equal. +func normalizeParamName(name string) string { + name = strings.ToLower(name) + name = strings.ReplaceAll(name, "-", "") + name = strings.ReplaceAll(name, "_", "") + return name +} + +// longestCommonPrefixLen returns the length of the longest common prefix of a +// and b. +func longestCommonPrefixLen(a, b string) int { + n := min(len(a), len(b)) + for i := range n { + if a[i] != b[i] { + return i + } + } + return n +} + +// mcpToolParams returns the registry of valid parameter names for every tool +// registered in the MCP server. This is called once during server construction +// and the result is passed to argumentValidationMiddleware. +// +// The map key is the MCP tool name; the value is the sorted list of valid JSON +// parameter names taken from the corresponding *Args struct json tags. +func mcpToolParams() map[string]toolParamEntry { + params := map[string]toolParamEntry{ + "status": { + "pattern", + }, + "compile": { + "workflows", "strict", "zizmor", "poutine", "actionlint", + "runner-guard", "fix", "max_tokens", + }, + "logs": { + "workflow_name", "count", "start_date", "end_date", "engine", + "firewall", "no_firewall", "filtered_integrity", "branch", + "after_run_id", "before_run_id", "timeout", "max_tokens", "artifacts", + }, + "audit": { + "run_id_or_url", "run_ids_or_urls", "artifacts", "max_tokens", + }, + "audit-diff": { + "base_run_id", "compare_run_ids", "artifacts", + }, + "checks": { + "pr_number", "repo", + }, + "mcp-inspect": { + "workflow_file", "server", "tool", + }, + "add": { + "workflows", "number", "name", + }, + "update": { + "workflows", "major", "force", + }, + "fix": { + "workflows", "write", "list_codemods", + }, + } + + // Sort each list for deterministic output in suggestions. + for k, v := range params { + sorted := make([]string, len(v)) + copy(sorted, v) + sort.Strings(sorted) + params[k] = sorted + } + + return params +} diff --git a/pkg/cli/mcp_argument_validation_test.go b/pkg/cli/mcp_argument_validation_test.go new file mode 100644 index 00000000000..c404ac4864a --- /dev/null +++ b/pkg/cli/mcp_argument_validation_test.go @@ -0,0 +1,307 @@ +//go:build !integration + +package cli + +import ( + "context" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestExtractUnknownParams verifies that the error-message parser correctly +// extracts unknown parameter names from the jsonschema-go validation error. +func TestExtractUnknownParams(t *testing.T) { + tests := []struct { + name string + errMsg string + expected []string + }{ + { + name: "single unknown param", + errMsg: `validating "arguments": validating root: unexpected additional properties ["workflow-name"]`, + expected: []string{"workflow-name"}, + }, + { + name: "multiple unknown params", + errMsg: `validating "arguments": validating root: unexpected additional properties ["workflow-name" "invalid-param"]`, + expected: []string{"workflow-name", "invalid-param"}, + }, + { + name: "underscore param", + errMsg: `validating "arguments": validating root: unexpected additional properties ["workflow_name"]`, + expected: []string{"workflow_name"}, + }, + { + name: "no match — different error", + errMsg: "some other validation error", + expected: nil, + }, + { + name: "empty string", + errMsg: "", + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractUnknownParams(tt.errMsg) + assert.Equal(t, tt.expected, got, "extracted unknown params should match") + }) + } +} + +// TestFindSimilarParam verifies the fuzzy matching of parameter names. +func TestFindSimilarParam(t *testing.T) { + compileParams := []string{"actionlint", "fix", "max_tokens", "poutine", "runner-guard", "strict", "workflows", "zizmor"} + + tests := []struct { + name string + unknown string + validParams []string + expected string + }{ + { + name: "workflow-name → workflows (compile)", + unknown: "workflow-name", + validParams: compileParams, + expected: "workflows", + }, + { + name: "workflow_name → workflows (compile)", + unknown: "workflow_name", + validParams: compileParams, + expected: "workflows", + }, + { + name: "workflowname → workflows (compile)", + unknown: "workflowname", + validParams: compileParams, + expected: "workflows", + }, + { + name: "max-tokens → max_tokens (compile)", + unknown: "max-tokens", + validParams: compileParams, + expected: "max_tokens", + }, + { + name: "runnerguard → runner-guard (compile)", + unknown: "runnerguard", + validParams: compileParams, + expected: "runner-guard", + }, + { + name: "completely unrelated — no match", + unknown: "banana", + validParams: compileParams, + expected: "", + }, + { + name: "empty validParams — no match", + unknown: "workflow-name", + validParams: []string{}, + expected: "", + }, + { + name: "exact match after normalization", + unknown: "strict", + validParams: compileParams, + expected: "strict", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := findSimilarParam(tt.unknown, tt.validParams) + assert.Equal(t, tt.expected, got, "similar param suggestion should match") + }) + } +} + +// TestBuildHelpfulParamError verifies the structure of the helpful error message. +func TestBuildHelpfulParamError(t *testing.T) { + t.Run("single unknown param with suggestion", func(t *testing.T) { + msg := buildHelpfulParamError( + "compile", + []string{"workflow-name"}, + []string{"workflows", "strict"}, + ) + assert.Contains(t, msg, "Unknown parameter 'workflow-name'", "should mention unknown param") + assert.Contains(t, msg, "Did you mean 'workflows'?", "should suggest workflows") + assert.Contains(t, msg, "agenticworkflows compile --help", "should point to help") + }) + + t.Run("single unknown param without suggestion", func(t *testing.T) { + msg := buildHelpfulParamError( + "compile", + []string{"banana"}, + []string{"workflows", "strict"}, + ) + assert.Contains(t, msg, "Unknown parameter 'banana'", "should mention unknown param") + assert.NotContains(t, msg, "Did you mean", "should not suggest for unrelated param") + assert.Contains(t, msg, "agenticworkflows compile --help", "should point to help") + }) + + t.Run("multiple unknown params", func(t *testing.T) { + msg := buildHelpfulParamError( + "compile", + []string{"workflow-name", "abc"}, + []string{"workflows", "strict"}, + ) + assert.Contains(t, msg, "Unknown parameter 'workflow-name'", "should mention first unknown param") + assert.Contains(t, msg, "Unknown parameter 'abc'", "should mention second unknown param") + assert.Contains(t, msg, "agenticworkflows compile --help", "should point to help") + }) + + t.Run("empty tool name omits help line", func(t *testing.T) { + msg := buildHelpfulParamError("", []string{"workflow-name"}, []string{"workflows"}) + assert.NotContains(t, msg, "--help", "no tool name means no help line") + }) +} + +// TestArgumentValidationMiddleware_TransformsAdditionalPropertiesError verifies +// that the middleware replaces raw schema validation errors with helpful messages. +func TestArgumentValidationMiddleware_TransformsAdditionalPropertiesError(t *testing.T) { + toolParams := map[string]toolParamEntry{ + "compile": {"actionlint", "fix", "max_tokens", "poutine", "runner-guard", "strict", "workflows", "zizmor"}, + } + + middleware := argumentValidationMiddleware(toolParams) + + // Build a fake "additional properties" tool error result. + rawErrMsg := `validating "arguments": validating root: unexpected additional properties ["workflow-name"]` + fakeResult := &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + &mcp.TextContent{Text: rawErrMsg}, + }, + } + + // Wrap a handler that returns the fake error result. + handler := middleware(func(_ context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + return fakeResult, nil + }) + + // Use a minimal fake request for the tools/call method. + // Since extractMCPToolName uses a type assertion, we pass nil params + // and accept an empty toolName (which skips the help line). + result, err := handler(context.Background(), "tools/call", fakeToolCallRequest("compile")) + require.NoError(t, err, "middleware should not return an error") + + toolResult, ok := result.(*mcp.CallToolResult) + require.True(t, ok, "result should be *mcp.CallToolResult") + assert.True(t, toolResult.IsError, "IsError should remain true") + + require.Len(t, toolResult.Content, 1, "should have one content element") + text, ok := toolResult.Content[0].(*mcp.TextContent) + require.True(t, ok, "content should be *mcp.TextContent") + + assert.NotContains(t, text.Text, "validating root", "raw schema error should be gone") + assert.Contains(t, text.Text, "Unknown parameter 'workflow-name'", "should name the bad param") + assert.Contains(t, text.Text, "Did you mean 'workflows'?", "should suggest workflows") +} + +// TestArgumentValidationMiddleware_PassesThroughNonValidationErrors verifies +// that the middleware does not modify tool results that are unrelated to schema +// validation. +func TestArgumentValidationMiddleware_PassesThroughNonValidationErrors(t *testing.T) { + toolParams := mcpToolParams() + middleware := argumentValidationMiddleware(toolParams) + + // A regular tool error (not a schema validation error). + regularErr := &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + &mcp.TextContent{Text: "workflow 'nonexistent' not found"}, + }, + } + + handler := middleware(func(_ context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + return regularErr, nil + }) + + result, err := handler(context.Background(), "tools/call", fakeToolCallRequest("compile")) + require.NoError(t, err) + + toolResult, ok := result.(*mcp.CallToolResult) + require.True(t, ok) + text := toolResult.Content[0].(*mcp.TextContent) + assert.Equal(t, "workflow 'nonexistent' not found", text.Text, "non-validation errors should be unchanged") +} + +// TestArgumentValidationMiddleware_PassesThroughSuccessResults verifies that +// successful tool results are not modified. +func TestArgumentValidationMiddleware_PassesThroughSuccessResults(t *testing.T) { + toolParams := mcpToolParams() + middleware := argumentValidationMiddleware(toolParams) + + successResult := &mcp.CallToolResult{ + IsError: false, + Content: []mcp.Content{ + &mcp.TextContent{Text: `[{"workflow":"test.md","valid":true}]`}, + }, + } + + handler := middleware(func(_ context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + return successResult, nil + }) + + result, err := handler(context.Background(), "tools/call", fakeToolCallRequest("compile")) + require.NoError(t, err) + + toolResult, ok := result.(*mcp.CallToolResult) + require.True(t, ok) + assert.False(t, toolResult.IsError) +} + +// TestArgumentValidationMiddleware_PassesThroughNonToolCallMethods verifies +// that the middleware ignores methods other than "tools/call". +func TestArgumentValidationMiddleware_PassesThroughNonToolCallMethods(t *testing.T) { + toolParams := mcpToolParams() + middleware := argumentValidationMiddleware(toolParams) + + // A tools/list response would never contain IsError, but even if it did + // the middleware should leave it alone. + called := false + handler := middleware(func(_ context.Context, method string, _ mcp.Request) (mcp.Result, error) { + called = true + assert.Equal(t, "tools/list", method) + return nil, nil + }) + + _, err := handler(context.Background(), "tools/list", fakeToolCallRequest("")) + require.NoError(t, err) + assert.True(t, called) +} + +// TestMCPToolParams verifies that the tool parameter registry is populated and +// consistent with the known tools. +func TestMCPToolParams(t *testing.T) { + params := mcpToolParams() + + expectedTools := []string{"status", "compile", "logs", "audit", "audit-diff", "checks", "mcp-inspect", "add", "update", "fix"} + for _, tool := range expectedTools { + t.Run(tool, func(t *testing.T) { + toolParams, ok := params[tool] + require.True(t, ok, "tool '%s' should be in the parameter registry", tool) + assert.NotEmpty(t, toolParams, "tool '%s' should have at least one parameter", tool) + + // Verify that the compile tool includes the parameters mentioned in the issue. + if tool == "compile" { + assert.Contains(t, toolParams, "workflows", "compile tool must include 'workflows' param") + } + }) + } +} + +// fakeToolCallRequest returns a minimal mcp.Request whose GetParams() type-assertion +// to *mcp.CallToolParamsRaw will succeed and return the given tool name. +func fakeToolCallRequest(toolName string) mcp.Request { + return &mcp.ServerRequest[*mcp.CallToolParamsRaw]{ + Params: &mcp.CallToolParamsRaw{Name: toolName}, + } +} diff --git a/pkg/cli/mcp_server.go b/pkg/cli/mcp_server.go index 3126c1b7313..f6abb492ff1 100644 --- a/pkg/cli/mcp_server.go +++ b/pkg/cli/mcp_server.go @@ -82,5 +82,9 @@ func createMCPServer(cmdPath string, actor string, validateActor bool, manifestC registerUpdateTool(server, execCmd) registerFixTool(server, execCmd) + // Add receiving middleware to transform raw JSON-schema "additional properties" + // validation errors into helpful messages with "Did you mean?" suggestions. + server.AddReceivingMiddleware(argumentValidationMiddleware(mcpToolParams())) + return server } From acb37456fa3a86e029507e8c41f108c6099c5f43 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 00:20:23 +0000 Subject: [PATCH 3/5] refactor: add maintenance comments to mcpToolParams linking to source-of-truth register functions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b4192ef2-4d95-44d3-bcc9-d4c22a63131d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_argument_validation.go | 23 +++++++++++++++++++++++ pkg/cli/mcp_argument_validation_test.go | 3 +++ 2 files changed, 26 insertions(+) diff --git a/pkg/cli/mcp_argument_validation.go b/pkg/cli/mcp_argument_validation.go index cd0a0174ade..9bf4d442cb0 100644 --- a/pkg/cli/mcp_argument_validation.go +++ b/pkg/cli/mcp_argument_validation.go @@ -217,38 +217,61 @@ func longestCommonPrefixLen(a, b string) int { // // The map key is the MCP tool name; the value is the sorted list of valid JSON // parameter names taken from the corresponding *Args struct json tags. +// +// MAINTENANCE: When tool parameters change, update the corresponding entry here +// to match the json tags on the *Args struct in register*Tool functions: +// - status → registerStatusTool in mcp_tools_readonly.go (statusArgs) +// - compile → registerCompileTool in mcp_tools_readonly.go (compileArgs) +// - logs → registerLogsTool in mcp_tools_privileged.go (logsArgs) +// - audit → registerAuditTool in mcp_tools_privileged.go (auditArgs) +// - audit-diff → registerAuditDiffTool in mcp_tools_privileged.go (auditDiffArgs) +// - checks → registerChecksTool in mcp_tools_readonly.go (checksArgs) +// - mcp-inspect → registerMCPInspectTool in mcp_tools_readonly.go (mcpInspectArgs) +// - add → registerAddTool in mcp_tools_management.go (addArgs) +// - update → registerUpdateTool in mcp_tools_management.go (updateArgs) +// - fix → registerFixTool in mcp_tools_management.go (fixArgs) func mcpToolParams() map[string]toolParamEntry { params := map[string]toolParamEntry{ + // statusArgs in mcp_tools_readonly.go "status": { "pattern", }, + // compileArgs in mcp_tools_readonly.go "compile": { "workflows", "strict", "zizmor", "poutine", "actionlint", "runner-guard", "fix", "max_tokens", }, + // logsArgs in mcp_tools_privileged.go "logs": { "workflow_name", "count", "start_date", "end_date", "engine", "firewall", "no_firewall", "filtered_integrity", "branch", "after_run_id", "before_run_id", "timeout", "max_tokens", "artifacts", }, + // auditArgs in mcp_tools_privileged.go "audit": { "run_id_or_url", "run_ids_or_urls", "artifacts", "max_tokens", }, + // auditDiffArgs in mcp_tools_privileged.go "audit-diff": { "base_run_id", "compare_run_ids", "artifacts", }, + // checksArgs in mcp_tools_readonly.go "checks": { "pr_number", "repo", }, + // mcpInspectArgs in mcp_tools_readonly.go "mcp-inspect": { "workflow_file", "server", "tool", }, + // addArgs in mcp_tools_management.go "add": { "workflows", "number", "name", }, + // updateArgs in mcp_tools_management.go "update": { "workflows", "major", "force", }, + // fixArgs in mcp_tools_management.go "fix": { "workflows", "write", "list_codemods", }, diff --git a/pkg/cli/mcp_argument_validation_test.go b/pkg/cli/mcp_argument_validation_test.go index c404ac4864a..813b633eca7 100644 --- a/pkg/cli/mcp_argument_validation_test.go +++ b/pkg/cli/mcp_argument_validation_test.go @@ -280,6 +280,9 @@ func TestArgumentValidationMiddleware_PassesThroughNonToolCallMethods(t *testing // TestMCPToolParams verifies that the tool parameter registry is populated and // consistent with the known tools. +// +// MAINTENANCE: This list must mirror createMCPServer in mcp_server.go — +// add an entry here whenever a new register*Tool call is added there. func TestMCPToolParams(t *testing.T) { params := mcpToolParams() From 363993193ff31fcd136d75d2ef85fefbb9f1d362 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 1 May 2026 00:30:08 +0000 Subject: [PATCH 4/5] docs(adr): add draft ADR-29406 for MCP receiving middleware validation error transformation Co-Authored-By: Claude Sonnet 4.6 --- ...iddleware-for-validation-error-messages.md | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 docs/adr/29406-mcp-receiving-middleware-for-validation-error-messages.md diff --git a/docs/adr/29406-mcp-receiving-middleware-for-validation-error-messages.md b/docs/adr/29406-mcp-receiving-middleware-for-validation-error-messages.md new file mode 100644 index 00000000000..619e13d8de5 --- /dev/null +++ b/docs/adr/29406-mcp-receiving-middleware-for-validation-error-messages.md @@ -0,0 +1,88 @@ +# ADR-29406: MCP Receiving Middleware for Helpful Validation Error Messages + +**Date**: 2026-05-01 +**Status**: Draft +**Deciders**: pelikhan + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The MCP server enforces strict JSON schema validation on tool call arguments using `additionalProperties: false`. When a caller passes an unknown parameter (e.g., `workflow-name` instead of `workflows`), the underlying `jsonschema-go` library produces a raw, internal error message such as `validating "arguments": validating root: unexpected additional properties ["workflow-name"]`. This opaque message is surfaced directly to the MCP client with no guidance on the correct parameter name, degrading the developer experience for all 10 registered tools. The MCP Go SDK provides a middleware hook (`AddReceivingMiddleware`) that can intercept tool results at the server boundary before they are returned to the caller. + +### Decision + +We will implement a generic MCP receiving middleware (`argumentValidationMiddleware`) that intercepts `CallToolResult` errors containing "unexpected additional properties" and replaces them with a human-readable message including the unknown parameter name, a "Did you mean?" suggestion derived from a longest-common-prefix fuzzy matcher, and a pointer to the tool's `--help` output. A hardcoded per-tool parameter registry (`mcpToolParams()`) maintains the list of valid parameter names for each tool, linked by comment to the source-of-truth `*Args` struct in each `register*Tool` function. + +### Alternatives Considered + +#### Alternative 1: Patch the upstream JSON schema or MCP SDK + +The `jsonschema-go` validation error format is controlled by the upstream library; customizing it would require forking or monkey-patching a third-party dependency. This was rejected because it introduces maintenance overhead for upstream changes and violates the project's policy of minimizing fork-based patches to dependencies. + +#### Alternative 2: Per-tool error handling at each registration site + +Each `register*Tool` function could wrap its handler to detect and reformat validation errors locally. While this avoids a central registry, it means duplicating the detection and formatting logic across ten tools and increases the risk of inconsistent messaging. The single middleware with a shared registry was preferred for uniformity and locality of change. + +#### Alternative 3: Levenshtein edit-distance fuzzy matching + +Full edit-distance similarity (e.g., via `golang.org/x/text` or a dedicated library) would handle transpositions and mid-string substitutions that longest-common-prefix cannot. It was considered but rejected for the typical case: MCP callers most often pass hyphen/underscore variants of a correct name (e.g., `workflow-name` for `workflows`), which normalization plus LCP handles reliably without an additional dependency. + +### Consequences + +#### Positive +- All ten MCP tools immediately get helpful error messages without changes to their individual implementations. +- Normalization (strip hyphens and underscores, lowercase) correctly resolves the most common error pattern — delimiter mismatch — before prefix comparison. +- The middleware is transparent: non-validation errors and successful results pass through unchanged, so no existing behavior is affected. + +#### Negative +- `mcpToolParams()` is a manually maintained registry; if a parameter is added or renamed in a `*Args` struct without updating this file, the suggestion will become stale or absent. +- LCP-based similarity does not catch mid-string differences or transpositions (e.g., `workflwo` → `workflows`); callers who mistype in those ways receive no suggestion. + +#### Neutral +- The middleware is registered once at server construction time, so its cost is one closure allocation; per-call overhead is limited to a single string comparison in the non-matching fast path. +- A maintenance comment block in `mcpToolParams()` links each entry to its source-of-truth registration function to reduce the risk of the registry drifting. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Middleware Registration + +1. The MCP server **MUST** register `argumentValidationMiddleware` as a receiving middleware via `AddReceivingMiddleware` after all tools have been registered. +2. The middleware **MUST** be initialized with the result of `mcpToolParams()` at server construction time. +3. The middleware **MUST NOT** modify results for MCP methods other than `tools/call`. +4. The middleware **MUST NOT** modify `CallToolResult` values where `IsError` is `false`. + +### Error Interception and Transformation + +1. The middleware **MUST** detect tool-call errors whose text content contains the substring `unexpected additional properties`. +2. The middleware **MUST** extract all unknown parameter names from the error message using the `extractUnknownParams` parser. +3. For each unknown parameter, the middleware **MUST** emit an `Unknown parameter ''.` prefix in the replacement message. +4. If a valid parameter whose normalized form shares a longest-common-prefix ratio ≥ 0.70 with the unknown parameter exists, the middleware **MUST** append `Did you mean ''?` to that parameter's line. +5. If the tool name is known, the middleware **MUST** append a `Run 'agenticworkflows --help' for usage.` line. +6. The replacement `CallToolResult` **MUST** preserve `IsError: true` and **MUST** contain exactly one `TextContent` element with the replacement message. + +### Parameter Registry (`mcpToolParams`) + +1. The `mcpToolParams()` registry **MUST** contain an entry for every tool registered in `createMCPServer`. +2. Each entry **MUST** list all valid JSON parameter names derived from the corresponding `*Args` struct's `json` tags. +3. Parameter lists **MUST** be kept in sorted order to produce deterministic suggestion output. +4. When a tool's `*Args` struct gains, removes, or renames a parameter, the corresponding `mcpToolParams()` entry **MUST** be updated in the same commit. + +### Normalization + +1. Parameter name normalization **MUST** lowercase the name and remove all hyphen (`-`) and underscore (`_`) characters before comparison. +2. An exact match after normalization **SHALL** be preferred over any prefix-score comparison and **MUST** return the matching valid parameter immediately. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25196075860) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From 075b20f68259ae5458f6aef5c5c366db4cb47373 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 01:08:45 +0000 Subject: [PATCH 5/5] fix: correct misleading comments in mcp_argument_validation Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7260c95f-5cc6-49dd-a709-962c00ca130a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_argument_validation.go | 2 +- pkg/cli/mcp_argument_validation_test.go | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/cli/mcp_argument_validation.go b/pkg/cli/mcp_argument_validation.go index 9bf4d442cb0..5422d1c0a99 100644 --- a/pkg/cli/mcp_argument_validation.go +++ b/pkg/cli/mcp_argument_validation.go @@ -32,7 +32,7 @@ type toolParamEntry = []string // a close match, and points to the tool's --help output. // // toolParams maps tool names to their list of valid JSON parameter names. It is -// built from the tool input schemas during server construction. +// provided by the caller as a hardcoded registry (see mcpToolParams). func argumentValidationMiddleware(toolParams map[string]toolParamEntry) mcp.Middleware { return func(next mcp.MethodHandler) mcp.MethodHandler { return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) { diff --git a/pkg/cli/mcp_argument_validation_test.go b/pkg/cli/mcp_argument_validation_test.go index 813b633eca7..3d795a729bb 100644 --- a/pkg/cli/mcp_argument_validation_test.go +++ b/pkg/cli/mcp_argument_validation_test.go @@ -186,9 +186,8 @@ func TestArgumentValidationMiddleware_TransformsAdditionalPropertiesError(t *tes return fakeResult, nil }) - // Use a minimal fake request for the tools/call method. - // Since extractMCPToolName uses a type assertion, we pass nil params - // and accept an empty toolName (which skips the help line). + // Call the middleware with a request carrying tool name "compile" so + // extractMCPToolName resolves it and the help line is included in the output. result, err := handler(context.Background(), "tools/call", fakeToolCallRequest("compile")) require.NoError(t, err, "middleware should not return an error")