Fix dynamic MCP tool argument registration

[jstar0]

Summary

Fixes #19 and #20.

Dynamic tool registration can now handle downstream MCP tools whose JSON Schema argument names are not valid Python identifiers, such as Notion-Version, while still exposing the original MCP argument names to clients.

Changes

• Generate safe internal Python parameter names for dynamic tool handlers while using Pydantic aliases for the original JSON Schema property names.
• Read inputSchema.required so required arguments stay required and optional arguments receive defaults.
• Omit unset optional arguments before forwarding calls to the downstream MCP server.
• Add regression coverage for direct dynamic handler invocation and the real FastMCP tool schema/call path.

Verification

uv run pytest tests/test_dynamic_tool_registration.py -q
env -u ALL_PROXY -u all_proxy -u HTTP_PROXY -u http_proxy -u HTTPS_PROXY -u https_proxy uv run pytest -q

Summary by CodeRabbit

Release Notes

Bug Fixes

• Fixed handling of tool parameters with non-standard names (special characters, digits, reserved words) in dynamic tool registration.
• Improved parameter signature accuracy to correctly distinguish required vs optional parameters.
• Optional parameters that are not explicitly set are now automatically omitted from tool calls.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR fixes dynamic tool registration failures when MCP tool parameter names contain characters invalid in Python identifiers (e.g., hyphens). A new ToolParamDescription dataclass and _safe_parameter_name helper are added to config.py. The gateway.py handler construction is updated to use sanitized python_name values in signatures while preserving original names for proxied calls, and a new test module validates all three behaviors.

Changes

Safe Python identifier generation for dynamic tool registration

Layer / File(s)	Summary
ToolParamDescription dataclass and _safe_parameter_name helper mcp_gateway/config.py	Adds a frozen ToolParamDescription dataclass with name, python_name, type_annotation, description, and required fields plus a tuple-compatible __getitem__. Introduces _safe_parameter_name to replace non-identifier characters, prefix keyword/digit-leading/empty names, and deduplicate with numeric suffixes. Updates get_tool_params_description to return List[ToolParamDescription] and compute python_name per parameter.
Dynamic handler signature and argument forwarding mcp_gateway/gateway.py	Adds Annotated to imports. Updates create_typed_handler to use python_name as the inspect.Parameter name and attach Annotated[type, Field(alias=original_name, description=...)] metadata; optional params default to None. Adds _get_tool_argument to resolve kwargs by either name. Updates runtime assembly to build tool_kwargs from non-None values keyed by original param.name.
Registration tests tests/test_dynamic_tool_registration.py	Adds RecordingFastMCP and RecordingProxiedServer doubles. Three tests verify: non-identifier keys map to safe Python names with correct defaults; None optional args are omitted from the proxied payload; and inputSchema retains original JSON Schema property names.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A hyphen in a name? Oh no, oh my!
The gateway would stumble, tools left to die.
Now _safe_parameter_name swoops in with care,
Turning Notion-Version to something more fair.
Required stays required, optional gets None,
And original names proxy on — hop, it is done! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: fixing dynamic MCP tool argument registration to handle invalid Python identifiers.
Linked Issues check	✅ Passed	The PR fully addresses all coding objectives from issue #19: sanitizing invalid parameter names, maintaining original name mappings, handling all invalid identifier types, and omitting unset optional arguments.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the linked issue objectives; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@mcp_gateway/config.py`:
- Around line 194-196: The used_python_names set in the parameter sanitization
logic starts empty, allowing reserved parameter names like ctx (which is already
reserved in gateway.py) to be sanitized and added as duplicate parameters,
causing inspect.Signature to fail. Pre-populate the used_python_names set with
reserved/internal parameter names such as ctx before iterating through the
properties.items() loop to ensure the sanitization process respects these
reserved names and prevents duplicate parameters.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b62888ca-435f-490b-849c-259b49f1eb10

📥 Commits

Reviewing files that changed from the base of the PR and between 7e7f1f6 and 4f5a32c.

📒 Files selected for processing (3)

• mcp_gateway/config.py
• mcp_gateway/gateway.py
• tests/test_dynamic_tool_registration.py

📜 Review details

🔇 Additional comments (1)

tests/test_dynamic_tool_registration.py (1)

33-167: LGTM!

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reserve internal handler names during sanitization.

At Line 194, used_python_names starts empty, so a schema key like "ctx" sanitizes to "ctx". In mcp_gateway/gateway.py Line 57, ctx is already reserved for context; adding another ctx parameter at Line 76 causes inspect.Signature(...) to fail with a duplicate-parameter error, and tool registration breaks.

Suggested fix

 def get_tool_params_description(tool: types.Tool) -> List[ToolParamDescription]:
     param_signatures = []
@@
-        used_python_names = set()
+        # Reserve handler-internal argument names used by gateway.create_typed_handler
+        used_python_names = {"ctx"}

Also applies to: 224-237

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mcp_gateway/config.py` around lines 194 - 196, The used_python_names set in
the parameter sanitization logic starts empty, allowing reserved parameter names
like ctx (which is already reserved in gateway.py) to be sanitized and added as
duplicate parameters, causing inspect.Signature to fail. Pre-populate the
used_python_names set with reserved/internal parameter names such as ctx before
iterating through the properties.items() loop to ensure the sanitization process
respects these reserved names and prevents duplicate parameters.