[object Object]

← back to Cli Printing Press

fix(cli): preserve query params in generated MCP handlers (#582)

e9696c9b84fa77863e66acb165c2603d12513612 · 2026-05-04 16:27:56 -0700 · Trevin Chow

* fix(cli): preserve query params in generated MCP handlers

The generated makeAPIHandler conflated CLI positional args with URL
path params: any name in positionalParams was dropped from the query
map, even when the path template had no matching {placeholder}. For
endpoints whose CLI surface uses a positional for a query param (e.g.
movies search <query> -> /search/movie?query=...), the typed MCP tool
silently sent the request without the argument.

Filter positionalParams by whether the path template actually contains
{name}. Real path placeholders still substitute and stay out of the
query map; everything else flows through as a query param.

Closes #578

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(cli): tighten mcp handler comment and test assertions

Trim the WHY-only sentence from makeAPIHandler's pre-loop comment and
loosen the test's handler-body checks to a behavioral regex on the
placeholder guard, so a benign rename of pathParams or its set type
won't break the regression coverage. The NotContains guard on the old
isPositional shape is preserved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Files touched

Diff

commit e9696c9b84fa77863e66acb165c2603d12513612
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Mon May 4 16:27:56 2026 -0700

    fix(cli): preserve query params in generated MCP handlers (#582)
    
    * fix(cli): preserve query params in generated MCP handlers
    
    The generated makeAPIHandler conflated CLI positional args with URL
    path params: any name in positionalParams was dropped from the query
    map, even when the path template had no matching {placeholder}. For
    endpoints whose CLI surface uses a positional for a query param (e.g.
    movies search <query> -> /search/movie?query=...), the typed MCP tool
    silently sent the request without the argument.
    
    Filter positionalParams by whether the path template actually contains
    {name}. Real path placeholders still substitute and stay out of the
    query map; everything else flows through as a query param.
    
    Closes #578
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * refactor(cli): tighten mcp handler comment and test assertions
    
    Trim the WHY-only sentence from makeAPIHandler's pre-loop comment and
    loosen the test's handler-body checks to a behavioral regex on the
    placeholder guard, so a benign rename of pathParams or its set type
    won't break the regression coverage. The NotContains guard on the old
    isPositional shape is preserved.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 internal/generator/generator_test.go               | 69 ++++++++++++++++++++++
 internal/generator/templates/mcp_tools.go.tmpl     | 25 ++++----
 .../printing-press-golden/internal/mcp/tools.go    | 25 ++++----
 .../tier-routing-golden/internal/mcp/tools.go      | 25 ++++----
 4 files changed, 108 insertions(+), 36 deletions(-)

diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 3d44d5da..b3efd4b8 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -5950,6 +5950,75 @@ func assertNewMCPClientSkipsCache(t *testing.T, specSource string) {
 		"newMCPClient must disable the response cache so MCP-driven reads see fresh state across mutations")
 }
 
+// TestGenerateMCPHandlerPreservesQueryPositionals proves the makeAPIHandler
+// body in generated MCP tools.go distinguishes real URL path placeholders
+// (e.g. /movie/{movieId}) from CLI positional args that map to query
+// params (e.g. `search <query>` -> ?query=). The bug was that the handler
+// dropped every positionalParams entry from the query map, so a query-style
+// positional like `query` silently disappeared from the request — TMDb
+// returned an empty page for movies_search even with a non-empty query.
+func TestGenerateMCPHandlerPreservesQueryPositionals(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := minimalSpec("mcphandler")
+	apiSpec.Resources = map[string]spec.Resource{
+		"movies": {
+			Description: "Movies",
+			Endpoints: map[string]spec.Endpoint{
+				// Query-style positional: `query` is a CLI positional but
+				// must be sent as ?query=… because the path has no {query}.
+				"search": {
+					Method:      "GET",
+					Path:        "/search/movie",
+					Description: "Search movies",
+					Params: []spec.Param{
+						{Name: "query", Type: "string", Required: true, Positional: true, Description: "Search query"},
+						{Name: "year", Type: "string", Description: "Release year"},
+					},
+				},
+				// Real path param: `movieId` is substituted into the URL.
+				"get": {
+					Method:      "GET",
+					Path:        "/movie/{movieId}",
+					Description: "Get a movie",
+					Params: []spec.Param{
+						{Name: "movieId", Type: "string", Required: true, PathParam: true, Description: "Movie ID"},
+					},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	require.NoError(t, New(apiSpec, outputDir).Generate())
+
+	toolsData, err := os.ReadFile(filepath.Join(outputDir, "internal", "mcp", "tools.go"))
+	require.NoError(t, err)
+	tools := string(toolsData)
+
+	// Call sites still pass both names — the upstream emit is unchanged;
+	// the fix lives entirely inside the handler body.
+	assert.Regexp(t,
+		regexp.MustCompile(`makeAPIHandler\("GET",\s*"/search/movie",\s*\[]string\{[^}]*"query"`),
+		tools,
+		"search call site must still pass `query` in positionalParams (handler decides path vs query at runtime)")
+	assert.Regexp(t,
+		regexp.MustCompile(`makeAPIHandler\("GET",\s*"/movie/\{movieId\}",\s*\[]string\{[^}]*"movieId"`),
+		tools,
+		"get-by-id call site must pass `movieId` in positionalParams")
+
+	assert.Regexp(t,
+		regexp.MustCompile(`strings\.Contains\(pathTemplate,`),
+		tools,
+		"makeAPIHandler must guard the path-substitution loop with a placeholder-presence check so query-style positionals stay in the query map")
+	assert.NotContains(t, tools, "isPositional := false",
+		"old handler shape skipped every positionalParams entry from query — must not regress")
+
+	// Generated CLI must still compile.
+	runGoCommand(t, outputDir, "mod", "tidy")
+	runGoCommand(t, outputDir, "build", "./...")
+}
+
 // TestGenerateMCPCodeOrchKeywordsHasStopwordFilter proves the keyword
 // extractor in the code-orchestration thin surface filters short tokens
 // and a stopword set. Without this, two-letter substrings like "is"/"in"
diff --git a/internal/generator/templates/mcp_tools.go.tmpl b/internal/generator/templates/mcp_tools.go.tmpl
index 06036158..faee9c70 100644
--- a/internal/generator/templates/mcp_tools.go.tmpl
+++ b/internal/generator/templates/mcp_tools.go.tmpl
@@ -176,27 +176,28 @@ func makeAPIHandler(method, pathTemplate string, positionalParams []string) serv
 		// we rely on here (or an empty map when the payload is something else).
 		args := req.GetArguments()
 
-		// Build path by substituting positional params
+		// positionalParams mixes real URL path params with CLI positional
+		// args that map to query params (e.g. `search <query>` -> ?query=);
+		// the placeholder check below disambiguates them at runtime.
 		path := pathTemplate
+		pathParams := make(map[string]bool, len(positionalParams))
 		for _, p := range positionalParams {
+			placeholder := "{" + p + "}"
+			if !strings.Contains(pathTemplate, placeholder) {
+				continue
+			}
+			pathParams[p] = true
 			if v, ok := args[p]; ok {
-				path = strings.Replace(path, "{"+p+"}", fmt.Sprintf("%v", v), 1)
+				path = strings.Replace(path, placeholder, fmt.Sprintf("%v", v), 1)
 			}
 		}
 
-		// Collect non-positional params as query params
 		params := make(map[string]string)
 		for k, v := range args {
-			isPositional := false
-			for _, p := range positionalParams {
-				if k == p {
-					isPositional = true
-					break
-				}
-			}
-			if !isPositional {
-				params[k] = fmt.Sprintf("%v", v)
+			if pathParams[k] {
+				continue
 			}
+			params[k] = fmt.Sprintf("%v", v)
 		}
 
 		var data json.RawMessage
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
index cb9965bc..b4e547e7 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
@@ -155,27 +155,28 @@ func makeAPIHandler(method, pathTemplate string, positionalParams []string) serv
 		// we rely on here (or an empty map when the payload is something else).
 		args := req.GetArguments()
 
-		// Build path by substituting positional params
+		// positionalParams mixes real URL path params with CLI positional
+		// args that map to query params (e.g. `search <query>` -> ?query=);
+		// the placeholder check below disambiguates them at runtime.
 		path := pathTemplate
+		pathParams := make(map[string]bool, len(positionalParams))
 		for _, p := range positionalParams {
+			placeholder := "{" + p + "}"
+			if !strings.Contains(pathTemplate, placeholder) {
+				continue
+			}
+			pathParams[p] = true
 			if v, ok := args[p]; ok {
-				path = strings.Replace(path, "{"+p+"}", fmt.Sprintf("%v", v), 1)
+				path = strings.Replace(path, placeholder, fmt.Sprintf("%v", v), 1)
 			}
 		}
 
-		// Collect non-positional params as query params
 		params := make(map[string]string)
 		for k, v := range args {
-			isPositional := false
-			for _, p := range positionalParams {
-				if k == p {
-					isPositional = true
-					break
-				}
-			}
-			if !isPositional {
-				params[k] = fmt.Sprintf("%v", v)
+			if pathParams[k] {
+				continue
 			}
+			params[k] = fmt.Sprintf("%v", v)
 		}
 
 		var data json.RawMessage
diff --git a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
index 4c0f61c1..2d4f27df 100644
--- a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
@@ -92,27 +92,28 @@ func makeAPIHandler(method, pathTemplate, tier string, positionalParams []string
 		// we rely on here (or an empty map when the payload is something else).
 		args := req.GetArguments()
 
-		// Build path by substituting positional params
+		// positionalParams mixes real URL path params with CLI positional
+		// args that map to query params (e.g. `search <query>` -> ?query=);
+		// the placeholder check below disambiguates them at runtime.
 		path := pathTemplate
+		pathParams := make(map[string]bool, len(positionalParams))
 		for _, p := range positionalParams {
+			placeholder := "{" + p + "}"
+			if !strings.Contains(pathTemplate, placeholder) {
+				continue
+			}
+			pathParams[p] = true
 			if v, ok := args[p]; ok {
-				path = strings.Replace(path, "{"+p+"}", fmt.Sprintf("%v", v), 1)
+				path = strings.Replace(path, placeholder, fmt.Sprintf("%v", v), 1)
 			}
 		}
 
-		// Collect non-positional params as query params
 		params := make(map[string]string)
 		for k, v := range args {
-			isPositional := false
-			for _, p := range positionalParams {
-				if k == p {
-					isPositional = true
-					break
-				}
-			}
-			if !isPositional {
-				params[k] = fmt.Sprintf("%v", v)
+			if pathParams[k] {
+				continue
 			}
+			params[k] = fmt.Sprintf("%v", v)
 		}
 
 		var data json.RawMessage

← 7572b1e0 fix(cli): live-dogfood matrix accuracy (WU-2, F4+F5) (#577)  ·  back to Cli Printing Press  ·  test(cli): live-dogfood resolve-success and search error_pat 67c977d8 →