---
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 recurring bug family.** [#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` (this learning, #578). [#1199](https://github.com/mvanhorn/cli-printing-press/issues/1199) is a third recurrence on a different axis — `args[]` indexing rather than path-vs-query disambiguation — that hit `command_endpoint.go.tmpl` and `command_promoted.go.tmpl`. Treat any future generator template that consumes `positionalParams`, or computes an index/category from `Endpoint.Params`'s ordering, as a candidate for the same guard. Known emission sites (as of #1199): `command_endpoint.go.tmpl`, `command_promoted.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
- [#1199](https://github.com/mvanhorn/cli-printing-press/issues/1199) — third recurrence in the same template family (args[] index drift in `command_endpoint.go.tmpl` and `command_promoted.go.tmpl`)
- [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)
