Skip to content

fix(tools): let optional MCP tool arguments accept null - #261

Merged
Zongwei9888 merged 1 commit into
HKUDS:mainfrom
vishalhabib99:fix/optional-args-accept-null
Oct 21, 2026
Merged

Zongwei9888 merged 1 commit into
HKUDS:mainfrom
vishalhabib99:fix/optional-args-accept-null

Conversation

@vishalhabib99

Copy link
Copy Markdown
Contributor

Description

Three tools in the bundled MCP servers declare optional arguments as T = None without None in the type. FastMCP then advertises them as, for example, {"type": "integer", "default": null}: the default is null, but the type doesn't allow null. A client that sends the advertised default back explicitly is rejected before the tool runs. For example, read_file with {"file_path": "a.py", "start_line": null} fails with:

1 validation error for read_fileArguments
start_line
  Input should be a valid integer [type=int_type, input_value=None, input_type=NoneType]

This PR annotates those arguments as T | None. Behavior for callers that leave the arguments out doesn't change.

Related Issues

None found. I searched issues and PRs for null, Optional, nullable and default None.

Changes Made

  • Changes annotations only: 5 parameters in 3 tools, with no logic changes.
    • tools/code_implementation_server.py: read_file (start_line, end_line) and search_code (search_directory)
    • tools/document_segmentation_server.py: read_document_segments (keywords, max_total_chars)
  • Adds tests/test_bundled_mcp_optional_args.py. For both servers, it lists the tools through the real FastMCP instance and checks that every argument whose schema default is null also accepts null. It checks both the advertised schema and pydantic argument validation. It doesn't call any tools and doesn't start a backend.

Checklist

  • Changes tested locally
  • Code reviewed
  • Documentation updated (if necessary): not needed
  • Unit tests added (if applicable)

Additional Notes

  • The new test fails on main for all 5 arguments and passes with this change.
  • Full suite on macOS, Python 3.13, installed from scripts/ci/requirements.lock:
    • main: 9 failed, 2774 passed, 14 skipped
    • this branch: 9 failed, 2776 passed, 14 skipped (the 2 new test cases pass)
    • The same 9 tests fail on main and on this branch. They cover the workbench and verification test runners (the system python3 here has no pytest, plus npm runs), the ci-scope git action, and the bash large-output spill test. None of them touch these servers.
  • ruff 0.15.21 (the pre-commit pin) check and format both pass on the changed files.
  • The package requires Python >= 3.12 (setup.py), so T | None works on every supported version. Other modules in tools/ already use it.

read_file, search_code and read_document_segments declared optional
arguments as `int = None` / `str = None` / `List[str] = None`. FastMCP
advertises those as {"type": "integer", "default": null}, so a client
that sends the advertised default back explicitly gets a validation error
before the tool runs. Annotate the five arguments as `T | None`.
@Zongwei9888
Zongwei9888 merged commit 7d5c251 into HKUDS:main Oct 21, 2026
@Zongwei9888

Copy link
Copy Markdown
Collaborator

Merged into main on 2026-10-21 as 7d5c251 ("Merge PR #261: let optional MCP tool arguments accept null"). Thank you for a precise first contribution.

We confirmed the bug: our own argument validator rejected null for {"type": "integer", "default": null} before the call reached the server, and with T | None the schema becomes nullable, the same shape git_command's optional arguments already use. Required arguments still reject null, and your test fails without the change and passes with it.

A small nit we may tidy later: the module docstring in tools/document_segmentation_server.py still says List[str] = None. It's in the README news for 2026-10-21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants