fix(validate): accept hyphenated MCP tool names in allow/deny lists#370
Conversation
mcpToolNamePattern rejected "-", but hyphenated tool names are the MCP
ecosystem norm and match the spec's SHOULD pattern
(^[a-zA-Z0-9_-]{1,128}$). Field case: an agent built with Calendly's
registry ops crash-looped at startup —
ERROR: mcp.servers[0].tools.allow[0]: invalid tool name "event_types-get_event_type"
— because the allow-list carries the server's genuine tool names, which
the platform cannot rename without breaking tools/call. The "__"
namespace-separator reservation is unchanged (separate check, now
pinned by its own test); a single "-" poses no conflict with the
"<server>__<tool>" runtime form. Length cap raised 64 → 128 to match
the spec pattern.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
initializ-mk
left a comment
There was a problem hiding this comment.
Review: accept hyphenated MCP tool names
Correct, minimal, and safe. The one security-relevant invariant — the __ namespace reservation — is verified intact at both the config and runtime boundaries. No findings. Merge-ready. CI fully green.
The fix is right
- Regex correctness:
^[a-zA-Z0-9_-]{1,128}$— the-sits at the end of the class, so it's a literal hyphen, not a range. The128cap matches the MCP spec SHOULD pattern. The class still excludes/,., spaces, quotes, so relaxing it adds no path-traversal or injection surface — a tool name stays a plain identifier. - Real problem: hyphenated names (
event_types-get_event_type) are what upstream servers expose viatools/listand must be used verbatim intools/call; the platform can't rename them, so the old pattern bricked any such agent at startup.
The __ reservation is untouched and independently enforced ✅
The regex allows _ (and thus __), so a name like otherserver__admintool must be rejected elsewhere to prevent spoofing the runtime <server>__<tool> separator. Verified:
- Config (
mcp_config.go): for both allow and deny, a regex check AND a separatestrings.Contains(name, "__")check. The regex-fail pathcontinues, so a regex-passing name likefoo__barreaches the__check and is rejected ("reserved for MCP namespacing"). The regex change doesn't touch this; the newfoo__bartest pins it. - Runtime (
validateDescriptorName): non-empty + no__, no character-class regex — consistent with config, so no "config accepts but runtime rejects" gap. - Parse is unambiguous: server names are
^[a-z][a-z0-9-]{0,30}$(no_at all) and tool names can't contain__, so the first__in<server>__<tool>is always the separator. A single-/_in either component poses no conflict.
Tests
The diff flips the old hyphen-rejection test to pin acceptance of real Calendly names (allow + deny) and adds the foo__bar-still-rejected case — exactly the two behaviors that matter. CI green across all builds, Lint, Test, Integration.
Clean fix with the tricky invariant (__ reservation + config↔runtime consistency) verified rather than assumed.
Problem
mcpToolNamePattern(forge-core/validate/mcp_config.go) was^[a-zA-Z0-9_]{1,64}$— no hyphen. Hyphenated tool names are the MCP-ecosystem norm and match the spec's SHOULD pattern (^[a-zA-Z0-9_-]{1,128}$).Field case (FanDuel): an agent built with the Calendly MCP registry entry crash-loops at startup, because its allow-list carries the server's genuine tool names:
The platform can't rename these — they're what the upstream server exposes via
tools/list, andtools/callmust use them verbatim. Any MCP server with hyphenated tool names is currently undeployable.Fix
^[a-zA-Z0-9_-]{1,128}$(adds-, raises the length cap to the spec's 128).__reservation is untouched — it's enforced by a separate check (the runtime<server>__<tool>namespace separator), and a single-/_poses no conflict with it. Now pinned by its own test case.foo__barstill-rejected case.Runtime side needs no change:
validateDescriptorName(adapter boundary, review B9) only enforces non-empty + no__, and the registry/DEFER keys use the namespaced form which is hyphen-safe.Testing
go test ./...green in forge-core; forge-cli builds. gofmt clean.🤖 Generated with Claude Code