fix: replace or with is not None checks for config/override merging#3833
fix: replace or with is not None checks for config/override merging#3833
or with is not None checks for config/override merging#3833Conversation
There was a problem hiding this comment.
💡 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".
| final_server_args = ( | ||
| server_args if server_args is not None else config.deployment.args | ||
| ) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
8993d50 to
b5b0e51
Compare
There was a problem hiding this comment.
💡 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".
| 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 |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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).
| final_port = port if port is not None else config.deployment.port | ||
| final_path = path if path is not None else config.deployment.path |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed in the same commit — both command builder paths now use is not None.
b5b0e51 to
a7f8051
Compare
There was a problem hiding this comment.
💡 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".
| streamable_http_path=path | ||
| if path is not None | ||
| else fastmcp.settings.streamable_http_path, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
a7f8051 to
0df35be
Compare
There was a problem hiding this comment.
💡 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".
| if port is not None: | ||
| kwargs["port"] = port |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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]>
0df35be to
0729948
Compare
There was a problem hiding this comment.
💡 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".
| if transport != "stdio": | ||
| if host is not None: | ||
| kwargs["host"] = host | ||
| if port is not None: | ||
| kwargs["port"] = port |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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]>
jlowin
left a comment
There was a problem hiding this comment.
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.
Several config merging paths used
x or defaultwhich silently drops falsy-but-valid values.port=0(OS-assigned port),host=""(all interfaces), anddescription=""(explicitly cleared) were all replaced by their defaults.Changed
x or defaulttox if x is not None else defaultin:cli/cli.py— CLIruncommand config merging + subprocess/reload command builderscli/run.py— programmaticrun_command()config merging + kwargs builder (with stdio guard)server/mixins/transport.py—run_httphost/port/path/log_leveltools/function_tool.py— tool descriptionresources/function_resource.py— resource descriptionprompts/function_prompt.py— prompt descriptionserver/sampling/sampling_tool.py— sampling tool descriptionFixes #3832
🤖 Generated with Claude Code