fix: sanitize registered MCP tool names for model APIs #41

Merged

pti merged 5 commits from fix/sanitize-mcp-tool-names into main 2026-09-23 11:11:52 +02:00

Conversation 0 Commits 5 Files changed 12 +291 -141

pti commented 2026-09-23 09:50:15 +02:00

Owner

Copy link

The bug

sigil publishes its tools as sigil/lookup. The registered name was a raw <name_prefix>_<upstream_tool> join, so that slash rode through into sigil_sigil/lookup.

Gemini enforces [A-Za-z_][A-Za-z0-9_.:-]{0,127} on function-declaration names and rejects the whole GenerateContentRequest over one bad entry:

tools[54].function_declarations[0].name: [FIELD_INVALID]

So one upstream's naming convention disabled every wraptool tool in the session, not just its own.

What changed

bb2b10d — the fix. Sanitize the exposed name. The handler still calls the upstream by its unmodified name, so dispatch is untouched. Also applied on the collision-detection path, so a/b and a_b are caught as the collision they become once sanitized rather than silently registering twice.

2f3554d — cleanup, no behavior change. Single-sourced the prefix + "_" + tool formula — it was spelled twice, in the minter and the collision checker, which is the class of bug being fixed here. Exported config.FlagParamName so mcptools.FlagToParam stopped duplicating it (its "avoid an import cycle" comment went stale the moment schema.go imported config). Dropped a package-level regexp used for a one-character class test, plus an unreachable branch.

2d19952 — depth.

• mcptools.NewRegisteredTool derives Name from the sanitized Tool.Name. The struct carried two unchecked copies of one string, and the registrar adds under Tool.Name but deregisters by Name — divergence leaks a registration on every reload. Now unconstructible, and it is the choke point every Source funnels through.
• Enforced the 128-char half of the grammar, which the comment documented but nothing checked. Truncates with an 8-hex digest so long names sharing a prefix past the cut stay distinct.

e332c4f — extraction, pure move. internal/toolname now owns Sanitize, CLI, Proxy, Param, ValidParam. The rules have two caller groups that cannot import each other — config validates, mcptools/mcpproxy mint — and config cannot import mcptools without a cycle. That is the same shape that let FlagToParam and flagParamName drift apart originally, and the same reasoning behind the existing internal/yamledit extraction.

5271396 — visibility. Validate() checked a flag's derived property key but never the tools map key or subcommand tokens — only AddTool's edit path did — so config-owned names were already being silently rewritten.

This warns at the mint site rather than rejecting at load, a deliberate departure from the stricter option. A subcommand token can legitimately carry a colon: npm run build:prod, mvn clean:install. Those sanitize cleanly today; hard-erroring would turn a working config into a server that refuses to start. The choke point above already guarantees nothing invalid reaches the wire, so the only gap left was telling the operator.

Testing

gofmt, go vet and the full suite pass.

One pre-existing failure, unrelated to this branch: cmd.TestUpdateServeStateConfigLoadedAtRequiresOwnership fails with mkdir /run/user: permission denied. Verified it fails identically on untouched code — the dev container has no XDG runtime dir.

New coverage: the reported agy/Gemini failure end to end (exposed name sanitized, upstream still called by its own name), the length cap and prefix-sharing long names, the idempotence the choke point relies on, and a colon-bearing subcommand loading rather than erroring.

🤖 Generated with Claude Code

https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

## The bug sigil publishes its tools as `sigil/lookup`. The registered name was a raw `<name_prefix>_<upstream_tool>` join, so that slash rode through into `sigil_sigil/lookup`. Gemini enforces `[A-Za-z_][A-Za-z0-9_.:-]{0,127}` on function-declaration names and rejects the **whole** `GenerateContentRequest` over one bad entry: ``` tools[54].function_declarations[0].name: [FIELD_INVALID] ``` So one upstream's naming convention disabled every wraptool tool in the session, not just its own. ## What changed **`bb2b10d` — the fix.** Sanitize the exposed name. The handler still calls the upstream by its unmodified name, so dispatch is untouched. Also applied on the collision-detection path, so `a/b` and `a_b` are caught as the collision they become once sanitized rather than silently registering twice. **`2f3554d` — cleanup, no behavior change.** Single-sourced the `prefix + "_" + tool` formula — it was spelled twice, in the minter and the collision checker, which is the class of bug being fixed here. Exported `config.FlagParamName` so `mcptools.FlagToParam` stopped duplicating it (its "avoid an import cycle" comment went stale the moment `schema.go` imported `config`). Dropped a package-level regexp used for a one-character class test, plus an unreachable branch. **`2d19952` — depth.** - `mcptools.NewRegisteredTool` derives `Name` from the sanitized `Tool.Name`. The struct carried two unchecked copies of one string, and the registrar *adds* under `Tool.Name` but *deregisters* by `Name` — divergence leaks a registration on every reload. Now unconstructible, and it is the choke point every `Source` funnels through. - Enforced the 128-char half of the grammar, which the comment documented but nothing checked. Truncates with an 8-hex digest so long names sharing a prefix past the cut stay distinct. **`e332c4f` — extraction, pure move.** `internal/toolname` now owns `Sanitize`, `CLI`, `Proxy`, `Param`, `ValidParam`. The rules have two caller groups that cannot import each other — `config` validates, `mcptools`/`mcpproxy` mint — and `config` cannot import `mcptools` without a cycle. That is the same shape that let `FlagToParam` and `flagParamName` drift apart originally, and the same reasoning behind the existing `internal/yamledit` extraction. **`5271396` — visibility.** `Validate()` checked a flag's derived property key but never the `tools` map key or subcommand tokens — only `AddTool`'s edit path did — so config-owned names were already being silently rewritten. This warns at the mint site rather than rejecting at load, a deliberate departure from the stricter option. A subcommand token can legitimately carry a colon: `npm run build:prod`, `mvn clean:install`. Those sanitize cleanly today; hard-erroring would turn a working config into a server that refuses to start. The choke point above already guarantees nothing invalid reaches the wire, so the only gap left was telling the operator. ## Testing `gofmt`, `go vet` and the full suite pass. One pre-existing failure, unrelated to this branch: `cmd.TestUpdateServeStateConfigLoadedAtRequiresOwnership` fails with `mkdir /run/user: permission denied`. Verified it fails identically on untouched code — the dev container has no XDG runtime dir. New coverage: the reported agy/Gemini failure end to end (exposed name sanitized, upstream still called by its own name), the length cap and prefix-sharing long names, the idempotence the choke point relies on, and a colon-bearing subcommand loading rather than erroring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

pti added 5 commits 2026-09-23 09:50:16 +02:00

fix(mcpproxy): sanitize registered tool names for model APIs ... bb2b10d8d6

An upstream MCP server namespaces its own tools however it likes — sigil
publishes "sigil/lookup" — and Source.Tools carried that name verbatim into
"<name_prefix>_<upstream_tool>", registering "sigil_sigil/lookup". Gemini
enforces [A-Za-z_][A-Za-z0-9_.:-]{0,127} on a function declaration name and
rejects the whole GenerateContentRequest when a single entry violates it:

  tools[54].function_declarations[0].name: [FIELD_INVALID] Invalid function
  name.

So one upstream's naming convention disabled every wraptool tool in an
Antigravity session, and the only workaround was to unhook wraptool entirely
by renaming .agents/mcp_config.json. Claude Code never surfaced this because
its own mcp__server__tool mangling rewrites the name before it reaches the
API.

SanitizeToolName maps a minted name onto that grammar. It is applied at both
minting sites — mcptools.ToolName for wrapped CLI subcommands and
Source.Tools for proxied upstream tools — and only to the EXPOSED name: the
proxy handler still calls upstream by its own unmodified ut.Name, so dispatch
is unchanged.

Validate's collision map sanitizes too. Without that, "a/b" and "a_b" under
one prefix both collapse to "sigil_a_b" at registration while validation saw
two distinct names, silently registering the same tool name twice.

It lives in config rather than mcptools because config cannot import mcptools
(cycle — the same reason flagParamName is duplicated there), while every
minting site already imports config.

Verified against a real config carrying sigil: 45 tools, none violating the
grammar, and sigil_sigil_search still dispatches to sigil/search upstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xSbKHd9CmzQW37AuvjVS6

refactor(mcptools): single-source the tool-name minting rule ... 2f3554d535

Cleanup pass over the sanitization fix in bb2b10d. No behavior change.

- Add config.ProxyToolName(prefix, upstreamTool). The
  `prefix + "_" + tool` formula was spelled twice, in the mcpproxy
  minter and in validateMCPServers' collision check. Those two must
  agree by construction -- a name-forming rule living in one place
  only is the class of bug bb2b10d fixed -- so both now mint through
  one function, mirroring mcptools.ToolName on the wrapped-CLI side.

- Export config.FlagParamName and make mcptools.FlagToParam delegate
  to it. Its "duplicated here to avoid a config -> mcptools import
  cycle" comment went stale the moment schema.go imported config for
  the sanitizer; only the reverse direction cycles.

- Drop toolNameFirstRe (a package-level regexp, and an init-time
  MustCompile, for a one-character class test) and the unreachable
  empty-string branch: the leading-character guard already yields "_"
  for "". The character replacement stays a regexp deliberately -- it
  substitutes per RUNE, where a byte loop would emit one "_" per byte.

- Tell the incident story once, on the exported function, instead of
  three times; drop the enumeration of minting call sites, which rots
  at the next one.

- Tests: drop the `valid` regexp, which re-derived the grammar to
  check a table of literal wants against itself, and the redundant
  git_push case; add ".hidden" and "café/x" to pin the leading-dot
  and per-rune behaviors.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

refactor(mcptools): enforce tool-name sanitization at the registration seam ... 2d19952535

Sanitizing at each minting site leaves every future site free to
forget, and left half the documented grammar unenforced. Close both.

- Add mcptools.NewRegisteredTool(tool, handler), which derives Name
  from the sanitized Tool.Name. RegisteredTool carried two unchecked
  copies of one string: the registrar adds under Tool.Name but
  deregisters by Name, so a divergent pair silently leaks a
  registration across every reload. Deriving both from one value makes
  that unconstructible rather than merely unlikely, and gives the
  Source seam a backstop that holds for an implementation which
  forgets to sanitize. cliSource, the mcpproxy source, and the
  registrar's test fake now build through it.

- Enforce the 128-character half of the grammar the sanitizer's own
  comment documents. An over-long name kills the whole request exactly
  as an invalid character does, and arrives from the same untrusted
  place (an upstream's ut.Name). Truncate to MaxToolNameLen with an
  8-hex-char digest of the pre-truncation name, so two names sharing a
  prefix past the cut stay distinct; the collision check in
  validateMCPServers sanitizes identically, so any clash truncation
  does create is still caught. Every surviving byte is ASCII, so the
  cut cannot split a rune.

- Log the mapping when an upstream tool's name is rewritten. The
  operator reads the rewritten name in their client and then greps
  their config for the upstream's own spelling; mcp_servers are absent
  from wraptool_discover, so nothing else pairs the two.

Tests cover the cap, prefix-sharing long names, and the idempotence
the choke point relies on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

refactor(toolname): extract the MCP naming rules into their own package ... e332c4f70b

The rules governing what wraptool may call a tool had two kinds of
caller that cannot import each other: internal/config validates names
at load and checks for collisions; internal/mcptools and
internal/mcpproxy mint and register them. config cannot import
mcptools without a cycle, so the rules had been living in config with
mcptools reaching back for them -- and before that, copied into both,
which is how FlagToParam and flagParamName drifted apart in the first
place.

Give them a home neither side owns, the way internal/yamledit was
extracted for the same reason:

  toolname.Sanitize    the model-API function-name grammar
  toolname.CLI         mint for a wrapped CLI subcommand
  toolname.Proxy       mint for a proxied upstream tool
  toolname.Param       schema property key from a CLI flag
  toolname.ValidParam  the property-key grammar

mcptools.ToolName and mcptools.FlagToParam stay as the names their
callers already use, now one-line delegates. config keeps no copy at
all. Pure move plus the delegation -- no behavior change; the tests
move with the rules and gain coverage for CLI and for Proxy agreeing
with the join its own doc comment promises.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

feat(mcptools): report when a config-owned tool name is rewritten ... 52713965cd

Validate() checks a flag's derived schema property key but never
checked the tool map key or the subcommand tokens against the
tool-name grammar, so a config-owned name that needed sanitizing was
rewritten with nothing said. The operator writes `subcommand: [run,
build:prod]` and then reads `npm_run_build_prod` in their client, with
no surface pairing the two.

Warn at the mint site rather than rejecting at load. A subcommand
token can legitimately carry a character the grammar forbids -- npm
run build:prod, mvn clean:install -- and those configs work today;
turning them into a server that refuses to start would be a
regression, not a fix. The choke point added in 2d19952 already
guarantees nothing invalid reaches the wire, so the only gap left was
telling the operator, which is what this does.

The message mirrors the upstream-rewrite log in mcpproxy, and the
warning lives at the mint site rather than in Validate() so the config
package stays free of logging -- the same split as the
allowed_cwd_roots warning, which is emitted from cmd/serve.go.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

pti added 1 commit 2026-09-23 10:22:48 +02:00

style(toolname): make the sanitize table gofmt-stable across Go versions ... 7f26fbe1cc

CI (Go 1.27) failed the gofmt gate on this file while Go 1.25 and 1.26
considered it clean. The two disagree about comment alignment in the
test's case table: 1.25/1.26 align the trailing comment on
{"9lives", ...} with the two longer rows above it, 1.27 breaks that
alignment group instead. Neither formatting satisfies the other, so
running either version's gofmt -w just moves the failure.

Put the comments on their own lines. With no trailing comments there
is no alignment group to disagree about, and gofmt from 1.25 through
1.27 all report the file clean.

Also write the non-ASCII case as "café/x" so the source stays
pure ASCII. It is the same string -- the test still pins that the
unsafe-character pass replaces one "_" per RUNE rather than per byte
-- but it no longer depends on how a given gofmt measures the display
width of a multi-byte rune.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

pti added 1 commit 2026-09-23 10:28:34 +02:00

build(ci): bump golangci-lint to 2.13.2 for Go 1.27 ... c140093552

CI started failing on every branch with golangci-lint's own typecheck
unable to read the standard library it was handed:

  go-1.27.1/lib/go/src/math/rand/v2/rand.go:213:17:
    method must have no type parameters (typecheck)

golangci-lint 2.12.2 was built with go1.26.2, so its bundled go/types
does not know Go 1.27 language features and chokes on 1.27's own
stdlib sources. 2.13.2 is built with go1.27.0.

Reproduced locally by pointing GOROOT at a 1.27.0 toolchain: 2.12.2
fails the same way -- a different stdlib file, same cause -- and
2.13.2 reports 0 issues across ./... under that toolchain.

The base32 hash was computed with a nix-base32 encoder self-tested by
re-deriving 2.12.2's existing manifest hash from its tarball first, so
it is verified rather than transcribed.

NOTE, not fixed here: the trigger was the Go version moving on its own.
channels.scm pins only the snamellit channel and leaves guix itself on
%default-channels, so `guix time-machine` tracks guix master and the
toolchain drifts between runs -- which is how a pinned linter and an
unpinned compiler drifted apart. Pinning the guix channel is the
reproducibility fix and is worth doing separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

pti added 1 commit 2026-09-23 10:33:47 +02:00

fix(cmd): isolate the serve-state test's runtime dir properly ... 514342af9f

TestUpdateServeStateConfigLoadedAtRequiresOwnership set XDG_RUNTIME_DIR
with a bare t.Setenv, which does not isolate anything: adrg/xdg
resolves xdg.RuntimeDir once at package init, so paths.RuntimeDir()
never saw the temp dir and the test wrote to the real per-user runtime
directory.

That passes only where /run/user/<uid> already exists and is writable
-- a desktop session -- and fails everywhere else. On the CI runner as
uid 980:

  server_test.go:386: writeServeState: creating runtime dir:
    mkdir /run/user/980: permission denied

Use withIsolatedRuntime, the helper two dozen lines above it that every
other runtime-dir test in this file already uses. It does the same
Setenv plus the xdg.Reload() that makes it take effect, and restores
both on cleanup.

No production code changes -- the test was asserting against the wrong
directory, not finding a real defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017WJrdw1xwqL2xjAWhdzuVY

pti force-pushed fix/sanitize-mcp-tool-names from 514342af9f to 0968e700fb 2026-09-23 10:40:58 +02:00 Compare

pti merged commit 061708b298 into main 2026-09-23 11:11:52 +02:00

pti referenced this pull request from a commit 2026-09-23 11:11:53 +02:00

Merge pull request 'fix: sanitize registered MCP tool names for model APIs' (#41) from fix/sanitize-mcp-tool-names into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

pti/wraptool!41

No description provided.