Skip to content

fix: replace or with is not None checks for config/override merging#3833

Merged
jlowin merged 2 commits intomainfrom
fix/or-pattern-falsy-values
Apr 11, 2026
Merged

fix: replace or with is not None checks for config/override merging#3833
jlowin merged 2 commits intomainfrom
fix/or-pattern-falsy-values

Conversation

@strawgate
Copy link
Copy Markdown
Collaborator

@strawgate strawgate commented Apr 11, 2026

Several config merging paths used x or default which silently drops falsy-but-valid values. port=0 (OS-assigned port), host="" (all interfaces), and description="" (explicitly cleared) were all replaced by their defaults.

# Before: port=0 silently becomes 8000
server.run_http(port=0)

# After: port=0 is respected (OS assigns a free port)
server.run_http(port=0)
# Before: description="" falls through to the docstring
@mcp.tool(description="")
def my_tool() -> str:
    """This docstring should NOT be the description"""
    return "hello"

# After: description stays ""

Changed x or default to x if x is not None else default in:

  • cli/cli.py — CLI run command config merging + subprocess/reload command builders
  • cli/run.py — programmatic run_command() config merging + kwargs builder (with stdio guard)
  • server/mixins/transport.pyrun_http host/port/path/log_level
  • tools/function_tool.py — tool description
  • resources/function_resource.py — resource description
  • prompts/function_prompt.py — prompt description
  • server/sampling/sampling_tool.py — sampling tool description

Fixes #3832

🤖 Generated with Claude Code

@marvin-context-protocol marvin-context-protocol Bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. cli Related to FastMCP CLI commands (run, dev, install) or CLI functionality. labels Apr 11, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8993d50605

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/fastmcp/cli/cli.py Outdated
Comment on lines +613 to +615
final_server_args = (
server_args if server_args is not None else config.deployment.args
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fall back to config args when CLI varargs are absent

In run, server_args comes from *server_args, so it is always a tuple (empty when the user passes no trailing args) and never None. With this is not None check, final_server_args now always takes the tuple and never falls back to config.deployment.args, which means fastmcp.json deployment.args are silently dropped unless the user re-specifies args on the CLI. This breaks existing config-driven launches (including reload/subprocess paths) that depend on configured server arguments.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — *server_args is variadic so it's always a tuple, never None. Reverted to or for this specific case. The other parameters all default to None so is not None is correct for them.

@strawgate strawgate force-pushed the fix/or-pattern-falsy-values branch from 8993d50 to b5b0e51 Compare April 11, 2026 00:31
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b5b0e5193f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/fastmcp/cli/run.py
transport if transport is not None else config.deployment.transport
)
host = host if host is not None else config.deployment.host
port = port if port is not None else config.deployment.port
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pass port=0 through to run_async kwargs

This merge now preserves port=0 from CLI/config (port if port is not None else ...), but run_command still only forwards port when it is truthy (if port: later in the same function). As a result, run_command(..., port=0) with a fastmcp.json file drops the port entirely and server.run_async falls back to global settings, so the override is not honored and even a configured deployment.port can be bypassed.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — changed if final_port: to if final_port is not None: in both reload and uv subprocess command builders (lines 645-649 and 698-703). Also fixed the module-mode ignored options warning (lines 524-531).

Comment thread src/fastmcp/cli/cli.py
Comment on lines +608 to +609
final_port = port if port is not None else config.deployment.port
final_path = path if path is not None else config.deployment.path
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve falsy HTTP overrides in reload/subprocess commands

The new is not None merge keeps falsy overrides like --port 0 and --path "", but the reload/uv command builders still append these flags only when truthy (if final_port, if final_path). In fastmcp run --reload (and uv-wrapped runs), those overrides are silently omitted from the child command, so the child process starts with default/config values instead of the requested override.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the same commit — both command builder paths now use is not None.

@strawgate strawgate force-pushed the fix/or-pattern-falsy-values branch from b5b0e51 to a7f8051 Compare April 11, 2026 02:06
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7f80514b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +340 to +342
streamable_http_path=path
if path is not None
else fastmcp.settings.streamable_http_path,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat empty path as unset before building HTTP routes

Switching to path is not None means path="" now gets forwarded to create_streamable_http_app/create_sse_app instead of falling back to configured defaults. Starlette routes require paths to start with /, so FastMCP.http_app(path="", transport="http"|"sse") now raises AssertionError: Routed paths must start with '/' during startup. Before this change, an empty string flowed through or and safely used the default MCP path, so this introduces a new crash path for callers that pass empty values.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — path="" would crash Starlette's route validation. In practice nobody passes empty path (the CLI param defaults to None, not ""), but this is a minor behavior change. An explicit empty string for a URL path is a caller error regardless.

@strawgate strawgate force-pushed the fix/or-pattern-falsy-values branch from a7f8051 to 0df35be Compare April 11, 2026 03:46
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0df35be968

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/fastmcp/cli/run.py Outdated
Comment on lines 245 to 246
if port is not None:
kwargs["port"] = port
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip port kwarg when transport is stdio

run_command now always forwards port when it is non-None, so calls like run_command(..., transport="stdio", port=0) (or equivalent CLI/config flows) pass port into server.run_async. In the stdio branch, TransportMixin.run_async forwards kwargs to run_stdio_async, which does not accept a port parameter, causing TypeError and process exit instead of starting the server. This regression is triggered specifically by falsy-but-valid overrides such as port=0 that were previously filtered out.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — HTTP-specific kwargs (host, port, path) are now guarded by if transport != "stdio" in the kwargs builder so they don't leak into run_stdio_async.

Falsy-but-valid values like port=0 (OS-assigned), host="" (all interfaces),
and description="" (explicitly cleared) were silently dropped by `x or default`
patterns across CLI, transport, and component registration.

Fixes #3832

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@strawgate strawgate force-pushed the fix/or-pattern-falsy-values branch from 0df35be to 0729948 Compare April 11, 2026 04:35
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0729948c42

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/fastmcp/cli/run.py
Comment on lines +244 to +248
if transport != "stdio":
if host is not None:
kwargs["host"] = host
if port is not None:
kwargs["port"] = port
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve default transport before appending HTTP kwargs

run_command now filters HTTP-only kwargs with if transport != "stdio", but transport can be None here and server.run_async() resolves that to fastmcp.settings.transport (default "stdio"). With this patch’s is not None checks, calls like run_command(..., transport=None, port=0) now pass port through, and the stdio path raises TypeError because run_stdio_async does not accept port (same for host/path). Fresh evidence in this commit is the combination of transport != "stdio" and port is not None, which still treats None as HTTP-capable.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in a follow-up PR. The guard now uses transport is not None and transport != "stdio" so HTTP kwargs aren't passed when transport is unspecified (defaults to stdio).

…rceTemplate

🤖 Generated with Claude Code

Co-authored-by: Jeremiah Lowin <[email protected]>
Copy link
Copy Markdown
Member

@jlowin jlowin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct fix for falsy-but-valid values. Added the missing template.py location — scanned the rest of the codebase and remaining or uses are intentional.

@jlowin jlowin merged commit 790f0bc into main Apr 11, 2026
9 checks passed
@jlowin jlowin deleted the fix/or-pattern-falsy-values branch April 11, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. cli Related to FastMCP CLI commands (run, dev, install) or CLI functionality. server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Falsy-but-valid values silently dropped by or operator in config merging

2 participants