← back to Cli Printing Press
docs(cli): capture mcp handler path-vs-query positional recurrence (#626)
622e15f5684b64ce451e5eaf519d354f2f958569 · 2026-05-05 11:32:03 -0700 · Trevin Chow
makeAPIHandler in mcp_tools.go.tmpl conflated CLI positional args with
URL path placeholders, dropping query-style positionals from MCP tool
calls (e.g. movies_search lost `query` before hitting TMDb). Same bug
was fixed earlier in command_endpoint.go.tmpl per #171 WU-1; the fix
didn't propagate to the MCP-side template, so it shipped to 6+
published CLIs.
Records the symptom set, the path-template-as-source-of-truth fix, and
prevention notes covering the recurrence framing, surface-mismatch
class, and the load-bearing comment in the fixed handler.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
A docs/solutions/logic-errors/mcp-handler-conflates-path-and-query-positional-params-2026-05-05.md
Diff
commit 622e15f5684b64ce451e5eaf519d354f2f958569
Author: Trevin Chow <trevin@trevinchow.com>
Date: Tue May 5 11:32:03 2026 -0700
docs(cli): capture mcp handler path-vs-query positional recurrence (#626)
makeAPIHandler in mcp_tools.go.tmpl conflated CLI positional args with
URL path placeholders, dropping query-style positionals from MCP tool
calls (e.g. movies_search lost `query` before hitting TMDb). Same bug
was fixed earlier in command_endpoint.go.tmpl per #171 WU-1; the fix
didn't propagate to the MCP-side template, so it shipped to 6+
published CLIs.
Records the symptom set, the path-template-as-source-of-truth fix, and
prevention notes covering the recurrence framing, surface-mismatch
class, and the load-bearing comment in the fixed handler.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
...-path-and-query-positional-params-2026-05-05.md | 138 +++++++++++++++++++++
1 file changed, 138 insertions(+)
diff --git a/docs/solutions/logic-errors/mcp-handler-conflates-path-and-query-positional-params-2026-05-05.md b/docs/solutions/logic-errors/mcp-handler-conflates-path-and-query-positional-params-2026-05-05.md
new file mode 100644
index 00000000..46f6e0b3
--- /dev/null
+++ b/docs/solutions/logic-errors/mcp-handler-conflates-path-and-query-positional-params-2026-05-05.md
@@ -0,0 +1,138 @@
+---
+title: "MCP handler conflated URL path placeholders with CLI positional query args"
+date: 2026-05-05
+category: logic-errors
+module: internal/generator/templates
+problem_type: logic_error
+component: tooling
+symptoms:
+ - "Typed MCP endpoint tools silently dropped query-style positional args; e.g. movies_search called with query=The Martian sent /search/movie?year=2015 and TMDb returned an empty page"
+ - "Bug reproduced in 6+ published CLIs (Movie Goat, Shopify, Linear, Archive.is, Weather Goat, Cal.com) all generated by the same template"
+ - "Direct Cobra CLI invocation worked correctly for the same endpoint, masking the failure from CLI-only smoke checks"
+ - "No error or warning surfaced — request was well-formed, just missing a parameter"
+root_cause: logic_error
+resolution_type: code_fix
+severity: high
+tags:
+ - mcp
+ - generator-template
+ - path-params
+ - query-params
+ - positional-args
+ - printed-cli
+ - makeapihandler
+---
+
+# MCP handler conflated URL path placeholders with CLI positional query args
+
+## Problem
+
+The generated `makeAPIHandler` in `internal/generator/templates/mcp_tools.go.tmpl` had a contract mismatch with its `positionalParams []string` argument. The slice is the union of two distinct categories at the call site — real URL path placeholders (`Param.PathParam == true`, e.g. `{movieId}`) and CLI positional args that map to query params (`Param.Positional == true`, e.g. `movies search <query>` → `?query=`). The handler treated every entry as the first category: it tried to substitute each into the URL template and excluded each from the query-param map. CLI positionals that mapped to query params therefore landed nowhere — they had no `{name}` to substitute and were stripped from the query map by the `isPositional` exclusion.
+
+The CLI surface flattens many shapes (Cobra `args[0]`, Viper bindings) before reaching shared code, so the bug fired only on the typed MCP endpoint surface.
+
+## Symptoms
+
+- Movie Goat's `movies_search` MCP tool, called with `{"query":"The Martian","year":"2015"}`, issued `GET /search/movie?year=2015` to TMDb and received an empty results page.
+- Same shape reproduced in Shopify (`*_get` dropping `id`), Linear (`*_get` dropping `id`), Archive.is (`feeds_search` dropping `q`), Weather Goat (`geocoding_search` dropping `name`), Cal.com (one phone-call tool dropping `orgId`).
+- The direct Cobra CLI invocation worked for the same endpoint, masking the bug from CLI-only smoke checks.
+- No error surfaced — the request was well-formed, just missing a parameter.
+
+## What Didn't Work
+
+Nothing went down a real dead-end. The defect was diagnosed before implementation started — issue [#578](https://github.com/mvanhorn/cli-printing-press/issues/578) already carried the working patch from a Movie Goat dogfood session.
+
+One transient flake during golden regeneration: an unrelated `internal/cli/sync.go` map-iteration order surfaced in the diff on the first `scripts/golden.sh update`, then resolved on re-run. Reverted the unrelated change and proceeded; not part of this fix.
+
+## Solution
+
+Use the path template itself as the source of truth for which names are real path placeholders, and route everything else into the query-param map. Build a `pathParams` set during the substitution loop and consult it during the param-collection loop.
+
+**Before** (`internal/generator/templates/mcp_tools.go.tmpl`):
+
+```go
+// Build path by substituting positional params
+path := pathTemplate
+for _, p := range positionalParams {
+ if v, ok := args[p]; ok {
+ path = strings.Replace(path, "{"+p+"}", 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)
+ }
+}
+```
+
+**After**:
+
+```go
+// 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, placeholder, fmt.Sprintf("%v", v), 1)
+ }
+}
+
+params := make(map[string]string)
+for k, v := range args {
+ if pathParams[k] {
+ continue
+ }
+ params[k] = fmt.Sprintf("%v", v)
+}
+```
+
+Regression coverage was added as `TestGenerateMCPHandlerPreservesQueryPositionals` in `internal/generator/generator_test.go`. The test builds a spec with one query-style positional (`/search/movie` + `query`) and one real path param (`/movie/{movieId}`), generates the CLI, and asserts:
+
+1. `makeAPIHandler` is called with both names in `positionalParams` (call-site emit unchanged — fix lives in handler body).
+2. The rendered handler contains the `strings.Contains(pathTemplate, ...)` placeholder guard.
+3. The old `isPositional := false` shape is gone (regression pin).
+
+It also runs `runGoCommand("build")` to prove the generated CLI compiles. Goldens regenerated; `scripts/golden.sh verify` 11/11; `go test ./...` 2891 pass.
+
+## Why This Works
+
+`pathTemplate` is the only authoritative source for which names are placeholders. The OpenAPI/internal-spec parser populates the template with exactly the path params the URL needs; CLI positionals that map to query params are never in it. By checking `strings.Contains(pathTemplate, "{p}")` before treating `p` as a path param, the handler stops conflating the two concerns the call site already mixes together. Names that are not placeholders fall through to the param-collection loop and become query params, which is what the API contract expects.
+
+The fix is local to the template body and doesn't change the call-site contract for `positionalParams`, so no caller needs to be updated and no other generator phase is affected.
+
+## Prevention
+
+- **This is a recurrence.** [#171 WU-1](https://github.com/mvanhorn/cli-printing-press/issues/171) fixed the identical bug in the sibling template `command_endpoint.go.tmpl` (CLI surface) earlier. The fix did not propagate to `mcp_tools.go.tmpl`. Treat any future generator template that consumes `positionalParams` as a candidate for the same guard. Today's emission sites: `command_endpoint.go.tmpl`, `mcp_tools.go.tmpl`. Add the check at any new site that joins them.
+- When a generator-emitted handler branches on a parameter list, verify the list's runtime semantics match the handler's assumed category before writing the branch. If the call site composes the list from multiple `Param.*` predicates (here: `Param.Positional || Param.PathParam`), treat each predicate as a distinct category in the handler too — don't collapse them by reusing the slice as a single set.
+- Prefer the most authoritative runtime artifact (here: `pathTemplate`) as the disambiguator instead of a name list assembled at codegen time. Name lists drift from intent; the template literal does not.
+- Whenever a defect in `internal/generator/templates/mcp_tools.go.tmpl` only manifests on the MCP surface, treat it as a class of bug, not a one-off. The CLI surface flattens many shapes before they reach shared code, so MCP-only regressions are easy to ship. Add the regression test at the generator level (assert on rendered template output + `runGoCommand("build")`) rather than at the printed-CLI level.
+- (auto memory [claude]) The same template file previously shipped a separate MCP-only default bug — `newMCPClient()` reused the CLI's on-disk response cache, so MCP-driven reads saw stale snapshots after `DELETE`/`PATCH`. Both bugs share the pattern: a default that worked for the CLI surface but was wrong for the MCP surface, found via dogfood of a printed CLI. When changing this file, sweep for other defaults that silently differ between surfaces.
+- (session history) The "why" comment in the fixed handler body is load-bearing — a stash-pop conflict during the fix proposed replacing it with a generic "Build path by substituting positional params" line. The conflict was resolved by keeping the explanatory comment because the next reader needs to know `positionalParams` mixes two categories. If you later "simplify" this comment, you're recreating the conditions for the bug.
+- When iterating on a printed CLI to discover issues, label findings as systemic (retro candidate) vs specific (printed-CLI fix). A handler-template bug reproducing across 6+ generated CLIs is the systemic case; fix it in the machine, not in each printed CLI.
+
+## Related Issues
+
+- [#578](https://github.com/mvanhorn/cli-printing-press/issues/578) — original issue (this fix)
+- [#582](https://github.com/mvanhorn/cli-printing-press/pull/582) — PR that landed the fix
+- [#171 WU-1](https://github.com/mvanhorn/cli-printing-press/issues/171) — same bug, sibling template (`command_endpoint.go.tmpl`), fixed earlier; the fix did not propagate
+- [printing-press-library#232](https://github.com/mvanhorn/printing-press-library/issues/232) — Movie Goat backport (merged)
+- [printing-press-library#233](https://github.com/mvanhorn/printing-press-library/issues/233) — Shopify, Linear, Archive.is, Weather Goat, Cal.com backports (merged)
+- [#437](https://github.com/mvanhorn/cli-printing-press/issues/437) — adjacent template hygiene (URL building correctness in `client.go.tmpl`)
+- [#427](https://github.com/mvanhorn/cli-printing-press/issues/427) — adjacent template-emission gap (body-field flag plumbing for POST/PUT/PATCH)
← 57e2d13a docs(cli): capture mutator-probe dry-run pattern in solution
·
back to Cli Printing Press
·
fix(cli): harden destructive auth dogfood skips (#628) 5004e4b0 →