fix: replace hardcoded mcpToolParams() with reflection-based extraction

docs/adr/30166-reflection-based-mcp-tool-parameter-extraction.md

@@ -0,0 +1,70 @@
+# ADR-30166: Reflection-Based MCP Tool Parameter Extraction
+
+**Date**: 2026-05-04
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The `mcpToolParams()` function in `pkg/cli/mcp_argument_validation.go` maintained a hardcoded map from MCP tool name to the list of valid JSON parameter names. This map fed the "Did you mean?" suggestion system for unrecognised parameters. The map was already out of sync: the `audit` tool's `experiment` and `variant` fields existed in `auditArgs` but were absent from the hardcoded map. A `// MAINTENANCE:` comment warned contributors to keep the map aligned with the `*Args` structs, but that approach had already failed in practice. The codebase has ten MCP tools, each backed by a dedicated `*Args` struct; the mismatch was invisible until a user hit a confusing suggestion gap.
+
+### Decision
+
+We will replace the hardcoded parameter map in `mcpToolParams()` with a reflection-based approach: each `*Args` struct is elevated from function-local to package scope, and a new `jsonFieldNames(v any) []string` helper uses `reflect` to extract JSON tag names from exported struct fields at runtime. This eliminates the need for any manual map maintenance and ensures the "Did you mean?" suggestions are always derived from the live struct definitions.
+
+### Alternatives Considered
+
+#### Alternative 1: Improve the Hardcoded Map with Stronger Test Coverage
+
+The existing hardcoded map could be kept, but supplemented with a generated "golden file" test that would fail if a struct field appeared in an `*Args` struct but not in the map. This would catch future drift at CI time. However, it still requires a two-step authoring process (add the field, then update the map and/or golden file), and it addresses the symptom rather than the root cause. The current test already failed to catch the `experiment`/`variant` gap, suggesting the feedback loop is not tight enough.
+
+#### Alternative 2: Code Generation (go generate)
+
+A `go generate` script could introspect the `*Args` structs and emit a static Go file containing the map. This avoids reflection at runtime and provides a checked-in artifact that shows exactly what parameters are registered. The downside is build complexity: the generated file must be re-run whenever an `*Args` struct changes, contributors must remember to commit the generated output, and CI must verify it is up to date. For a map this small (10 tools, ~60 parameters total), the overhead of a generation pipeline outweighs the runtime-inspection cost.
+
+### Consequences
+
+#### Positive
+- Adding a field to any `*Args` struct automatically includes it in "Did you mean?" suggestions; no secondary change is required.
+- Fixes an existing correctness bug where `experiment` and `variant` were silently absent from `audit` tool suggestions.
+- Removes the `// MAINTENANCE:` comment and its associated cognitive burden.
+
+#### Negative
+- All `*Args` structs are now at package scope in their respective files (`mcp_tools_readonly.go`, `mcp_tools_privileged.go`, `mcp_tools_management.go`), meaning they are technically exported-visible within the `cli` package and accessible from test files — types that were previously encapsulated in function bodies.
+- The `reflect` package is imported; `mcpToolParams()` now has an implicit runtime dependency on struct tag formatting conventions (e.g., the `json:"-"` sentinel).
+
+#### Neutral
+- The `jsonFieldNames` helper sorts its output; the earlier code sorted the map values in a post-processing loop. Semantics are unchanged, but the sorting is now done inside the helper rather than in the caller.
+- Tests have been updated to verify the reflection path, including a new `TestJSONFieldNames` unit test and an extended `TestMCPToolParams` spot-check for `experiment` and `variant`.
+
+---
+
+## 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).
+
+### Args Struct Scope
+
+1. Every `*Args` struct that backs an MCP tool **MUST** be declared at package scope in its respective source file so it is accessible for reflection by `mcpToolParams()`.
+2. `*Args` structs **MUST NOT** be declared inside the body of their `register*Tool` function, as function-local types cannot be referenced by `mcpToolParams()`.
+3. Each exported field of an `*Args` struct that represents a valid MCP parameter **MUST** carry a `json` struct tag with a non-empty, non-`"-"` name.
+
+### Parameter Extraction
+
+1. The `jsonFieldNames` helper **MUST** use `reflect` to extract JSON tag names and **MUST NOT** rely on any manually maintained list or generated artifact.
+2. `jsonFieldNames` **MUST** skip unexported fields, fields with no `json` tag, and fields whose `json` tag name is `"-"`.
+3. `jsonFieldNames` **MUST** return a sorted slice to ensure deterministic output in "Did you mean?" suggestions.
+4. `mcpToolParams()` **MUST** derive each tool's parameter list by calling `jsonFieldNames` with a zero-value instance of the corresponding `*Args` struct.
+5. `mcpToolParams()` **MUST NOT** contain any hardcoded parameter name strings.
+
+### 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/25332888181) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/mcp_argument_validation.go

@@ -16,6 +16,7 @@ package cli
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"regexp"
 	"sort"
 	"strings"
@@ -220,79 +221,57 @@ func longestCommonPrefixLen(a, b string) int {
 	return n
 }
 
+// jsonFieldNames extracts the JSON field names from the exported fields of a
+// struct value using reflection.  The json tag name (the part before the first
+// comma) is used; fields whose tag is "-" or that have no json tag are skipped.
+// The returned slice is sorted for deterministic output.
+func jsonFieldNames(v any) []string {
+	t := reflect.TypeOf(v)
+	if t.Kind() == reflect.Pointer {
+		t = t.Elem()
+	}
+	if t.Kind() != reflect.Struct {
+		return nil
+	}
+	var names []string
+	for i := range t.NumField() {
+		field := t.Field(i)
+		if !field.IsExported() {
+			continue
+		}
+		tag := field.Tag.Get("json")
+		if tag == "" || tag == "-" {
+			continue
+		}
+		name, _, _ := strings.Cut(tag, ",")
+		if name != "" && name != "-" {
+			names = append(names, name)
+		}
+	}
+	sort.Strings(names)
+	return names
+}
+
 // 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.
+// registered in the MCP server.  The map key is the MCP tool name; the value
+// is the sorted list of valid JSON parameter names derived automatically from
+// the corresponding *Args struct's json tags via reflection.
 //
-// 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)
+// Each *Args struct is declared at package scope (rather than locally inside its
+// register*Tool function) precisely so it can be referenced here for reflection.
+// Adding a new field to any *Args struct automatically includes it here —
+// no manual update is required.
 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",
-		},
+	return map[string]toolParamEntry{
+		"status":      jsonFieldNames(statusArgs{}),
+		"compile":     jsonFieldNames(compileArgs{}),
+		"logs":        jsonFieldNames(logsArgs{}),
+		"audit":       jsonFieldNames(auditArgs{}),
+		"audit-diff":  jsonFieldNames(auditDiffArgs{}),
+		"checks":      jsonFieldNames(checksArgs{}),
+		"mcp-inspect": jsonFieldNames(mcpInspectArgs{}),
+		"add":         jsonFieldNames(addArgs{}),
+		"update":      jsonFieldNames(updateArgs{}),
+		"fix":         jsonFieldNames(fixArgs{}),
 	}
-
-	// 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
 }

pkg/cli/mcp_argument_validation_test.go

@@ -11,6 +11,23 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// TestJSONFieldNames verifies that jsonFieldNames extracts the correct JSON tag
+// names from a struct via reflection.
+func TestJSONFieldNames(t *testing.T) {
+	type sampleArgs struct {
+		Alpha   string   `json:"alpha,omitempty"`
+		Beta    int      `json:"beta"`
+		Gamma   []string `json:"gamma,omitempty"`
+		ignored string   //nolint:unused // unexported — must be excluded
+		Skip    string   `json:"-"`
+		NoTag   string
+	}
+
+	got := jsonFieldNames(sampleArgs{})
+	// sorted result
+	assert.Equal(t, []string{"alpha", "beta", "gamma"}, got, "should include only tagged exported fields, sorted")
+}
+
 // TestExtractUnknownParams verifies that the error-message parser correctly
 // extracts unknown parameter names from the jsonschema-go validation error.
 func TestExtractUnknownParams(t *testing.T) {
@@ -278,23 +295,38 @@ 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.
+// consistent with the known tools.  Parameter names are derived automatically
+// from the *Args struct json tags via reflection, so this test validates that
+// every tool is present, has non-empty params, and that no unexpected tools
+// have been added to mcpToolParams() without also being listed here.
 func TestMCPToolParams(t *testing.T) {
 	params := mcpToolParams()
 
+	// This list must mirror createMCPServer in mcp_server.go.  If a new tool is
+	// registered there, add it here as well — the len check below will fail and
+	// catch any discrepancy at test time.
 	expectedTools := []string{"status", "compile", "logs", "audit", "audit-diff", "checks", "mcp-inspect", "add", "update", "fix"}
+
+	// Verify the registry has exactly the expected number of entries so that a
+	// new tool added to mcpToolParams() without also adding it to expectedTools
+	// (or vice-versa) is caught immediately.
+	assert.Len(t, params, len(expectedTools), "mcpToolParams() must contain exactly the expected tools; update expectedTools if a new tool was added")
+
 	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" {
+			// Spot-check a known parameter for each tool to confirm reflection is working.
+			switch tool {
+			case "compile":
 				assert.Contains(t, toolParams, "workflows", "compile tool must include 'workflows' param")
+			case "audit":
+				// experiment and variant were previously missing from the hardcoded map;
+				// reflection must now include them automatically.
+				assert.Contains(t, toolParams, "experiment", "audit tool must include 'experiment' param")
+				assert.Contains(t, toolParams, "variant", "audit tool must include 'variant' param")
 			}
 		})
 	}

pkg/cli/mcp_tools_management.go

@@ -11,14 +11,15 @@ import (
 
 var mcpToolsManagementLog = logger.New("cli:mcp_tools_management")
 
+// addArgs holds the input parameters for the add tool.
+type addArgs struct {
+	Workflows []string `json:"workflows" jsonschema:"Workflows to add (e.g., 'owner/repo/workflow-name' or 'owner/repo/workflow-name@version')"`
+	Number    int      `json:"number,omitempty" jsonschema:"Create multiple numbered copies (corresponds to -c flag, default: 1)"`
+	Name      string   `json:"name,omitempty" jsonschema:"Specify name for the added workflow - without .md extension (corresponds to -n flag)"`
+}
+
 // registerAddTool registers the add tool with the MCP server.
 func registerAddTool(server *mcp.Server, execCmd execCmdFunc) {
-	type addArgs struct {
-		Workflows []string `json:"workflows" jsonschema:"Workflows to add (e.g., 'owner/repo/workflow-name' or 'owner/repo/workflow-name@version')"`
-		Number    int      `json:"number,omitempty" jsonschema:"Create multiple numbered copies (corresponds to -c flag, default: 1)"`
-		Name      string   `json:"name,omitempty" jsonschema:"Specify name for the added workflow - without .md extension (corresponds to -n flag)"`
-	}
-
 	mcp.AddTool(server, &mcp.Tool{
 		Name: "add",
 		Annotations: &mcp.ToolAnnotations{
@@ -73,14 +74,15 @@ func registerAddTool(server *mcp.Server, execCmd execCmdFunc) {
 	})
 }
 
+// updateArgs holds the input parameters for the update tool.
+type updateArgs struct {
+	Workflows []string `json:"workflows,omitempty" jsonschema:"Workflow IDs to update (empty for all workflows)"`
+	Major     bool     `json:"major,omitempty" jsonschema:"Allow major version updates when updating tagged releases"`
+	Force     bool     `json:"force,omitempty" jsonschema:"Force update even if no changes detected"`
+}
+
 // registerUpdateTool registers the update tool with the MCP server.
 func registerUpdateTool(server *mcp.Server, execCmd execCmdFunc) {
-	type updateArgs struct {
-		Workflows []string `json:"workflows,omitempty" jsonschema:"Workflow IDs to update (empty for all workflows)"`
-		Major     bool     `json:"major,omitempty" jsonschema:"Allow major version updates when updating tagged releases"`
-		Force     bool     `json:"force,omitempty" jsonschema:"Force update even if no changes detected"`
-	}
-
 	mcp.AddTool(server, &mcp.Tool{
 		Name: "update",
 		Annotations: &mcp.ToolAnnotations{
@@ -145,14 +147,15 @@ Returns formatted text output showing:
 	})
 }
 
+// fixArgs holds the input parameters for the fix tool.
+type fixArgs struct {
+	Workflows    []string `json:"workflows,omitempty" jsonschema:"Workflow IDs to fix (empty for all workflows)"`
+	Write        bool     `json:"write,omitempty" jsonschema:"Write changes to files (default is dry-run)"`
+	ListCodemods bool     `json:"list_codemods,omitempty" jsonschema:"List all available codemods and exit"`
+}
+
 // registerFixTool registers the fix tool with the MCP server.
 func registerFixTool(server *mcp.Server, execCmd execCmdFunc) {
-	type fixArgs struct {
-		Workflows    []string `json:"workflows,omitempty" jsonschema:"Workflow IDs to fix (empty for all workflows)"`
-		Write        bool     `json:"write,omitempty" jsonschema:"Write changes to files (default is dry-run)"`
-		ListCodemods bool     `json:"list_codemods,omitempty" jsonschema:"List all available codemods and exit"`
-	}
-
 	mcp.AddTool(server, &mcp.Tool{
 		Name: "fix",
 		Annotations: &mcp.ToolAnnotations{