[Bicheng (Kenneth) (BichengWang)]

Passing --http --port 0 currently starts Uvicorn on port 3001 instead of letting it select an available port. The same happens with the --sse alias. Also, --port 0 without an HTTP transport bypasses the existing transport-option validation and starts STDIO.

Distinguish an omitted port (None) from an explicit zero in both checks. Add focused CLI regressions covering HTTP and SSE with default, zero, and nonzero ports, explicit ports without HTTP, and default STDIO. The tests exercise the real argument parser and mock the server-running boundary.

Validation:

• The initial regression suite reproduced three failures before the fix.
• The full MCP test suite passes after the fix (30 tests).
• Repository pre-commit checks and the new test file's Black check pass.

Uvicorn documents port zero as selecting an available port.

[Bicheng (Kenneth) (BichengWang)]

Bicheng (Kenneth) (Bicheng (Kenneth) (@BichengWang)) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

[XU (kokokoXUY)]

Review: honor port zero in the MCP HTTP transports (#2595)

Summary

This patch fixes two falsy-zero bugs in the markitdown-mcp CLI. --port 0 is
the documented way to ask the OS for a free port, but the CLI treated 0 as
"port not provided": with --http/--sse it silently started uvicorn on the
default 3001, and without a HTTP transport it skipped the argument validation
entirely and fell through to STDIO. Both sites are now is not None checks.
I reproduced both behaviors at the base commit, verified the fix end to end
with a real uvicorn server, and ran the full MCP test suite.

What the patch does

packages/markitdown-mcp/src/markitdown_mcp/__main__.py (2 lines changed):

• Line 108, the transport validation:

  if not use_http and (args.host or args.port):
  

  becomes

  if not use_http and (args.host or args.port is not None):
  

• Line 132, the port passed to uvicorn:

  port=args.port if args.port else 3001,
  

  becomes

  port=args.port if args.port is not None else 3001,
  

packages/markitdown-mcp/tests/test_cli.py (new file, 49 lines) adds three
tests: test_default_transport_uses_stdio (patches mcp.run and asserts it is
called with no arguments), test_http_port_reaches_uvicorn (parametrized over
--http/--sse and over omitted port / --port 0 / --port 4321, asserting
the host and port that reach uvicorn.run), and
test_explicit_port_requires_http_transport (parametrized over 0/4321,
expecting SystemExit with code 2, the argparse message on stderr, and that
mcp.run is never called).

What I verified locally

1. Scope. The diff is exactly the two files reported by the API (+51/-2), and
   git diff --stat upstream-main pr2595 over the current main tip
   (4cc9fa17653d695d64fb9eee5b33d4de55ff84e8) shows the same two files only,
   so the head tree already contains all of main and nothing needs
   reconciling. (git merge-tree is not usable in my clone because the fetched
   PR commit is shallow, which reports a bogus "unrelated histories" error; the
   diff --stat above is the stronger check anyway.)

2. Head test suite. pytest packages/markitdown-mcp/tests -q at head
   2e6968c57ecca3bcdf2b208d0255e51c570776ff:
   30 passed in 12.01s. test_cli.py alone: 9 passed in 1.57s.

3. Red test. With the head test file copied onto the base source
   (4cc9fa1), the suite gives 3 failed, 27 passed in 11.42s:
   test_http_port_reaches_uvicorn[port_args1-0---http] and
   [port_args1-0---sse] both fail with assert 3001 == 0
   (test_cli.py:35), and test_explicit_port_requires_http_transport[0] fails
   with DID NOT RAISE SystemExit (test_cli.py:44). The two nonzero-port
   cases pass on both trees, so the tests pin exactly the regression.

4. End-to-end. I drove the real argparse code path and started a real uvicorn
   server (patching only uvicorn.run to inspect the socket the server
   actually bound, then POSTing an initialize request to
   http://127.0.0.1:<bound>/mcp). Base vs head:

   argv	base: requested / bound / HTTP	head: requested / bound / HTTP
   --http --port 0	3001 / 3001 / 200	0 / 51700 / 200
   --sse --port 0	3001 / 3001 / 200	0 / 51704 / 200
   --http	3001 / 3001 / 200	3001 / 3001 / 200
   --http --port 4321	4321 / 4321 / 200	4321 / 4321 / 200
   --http --host 127.0.0.1 --port 0	3001 / 3001 / 200	0 / 64440 / 200
   --port 0 (no transport)	STDIO, rc 0	exit code 2, argparse error
   --port 4321 (no transport)	exit code 2	exit code 2

   The head responses to initialize were valid MCP responses
   (capabilities returned), so the ephemeral port really serves the MCP
   endpoint.

5. Startup log. Running python -m markitdown_mcp --http --port 0 as a real
   subprocess prints INFO: Uvicorn running on http://127.0.0.1:56285 (Press CTRL+C to quit), i.e. the banner shows the port that was actually
   chosen rather than 0 or a stale 3001, so there is no user-facing
   confusion about where the server ended up.

6. Unchanged paths. --host 127.0.0.1 without --http/--sse still exits 2
   on both trees, and --http without --port still uses 3001, so the default
   behavior and the host validation are untouched.

7. No sibling sites. Grepping the repository for falsy checks on this option
   finds only __main__.py:108 and :132; host = args.host if args.host else "127.0.0.1" on line 115 is fine because an empty host string is not
   meaningful. I did not find a third place with the same bug.

8. Formatting. black --check (23.7.0) on both changed files: unchanged.
   .pre-commit-config.yaml contains black as its only hook, so the
   pre-commit job should be clean once it is approved to run.

9. CI coverage. In .github/workflows/tests.yml the mcp-tests job installs
   ./packages/markitdown-mcp and runs pytest with
   working-directory: packages/markitdown-mcp, and the tests directory has an
   __init__.py with no custom testpaths, so the new test_cli.py is
   collected on all ten matrix entries (ubuntu 3.10-3.14, windows-latest 3.12,
   windows-11-vs2026-arm 3.12).

10. Style. The other test modules in this package also omit return annotations
    (def test_legacy_initialize_handshake(fixture_uri):), so the new file is
    consistent with its neighbors. Note this differs from the main
    markitdown package, which uses -> None, but that convention does not
    apply to markitdown-mcp.

Non-blocking observations

A. --port 0 is not mentioned in packages/markitdown-mcp/README.md (line 35
shows a fixed --port 3001 example). Now that the CLI honors it, a short
note such as "pass --port 0 to let the OS pick a free port; the chosen
port is printed at startup" would make the feature discoverable. Optional
and easy to do in a follow-up.

B. The second half of the fix is a small behavior change: --port 0 without
--http/--sse used to silently start STDIO and now exits with an
argparse error, consistent with any other explicit --port value. I think
that is the correct contract, but it is worth one line in the PR
description so it is not mistaken for a regression by users who passed
--port 0 while using STDIO.

C. The test asserts the port that reaches uvicorn.run, which is exactly the
boundary where the bug lived; asserting the OS-assigned port would not be
deterministic in CI, so the current granularity is appropriate. My local
end-to-end run covers the runtime behavior.

How this was produced

Reviewed with the help of an AI assistant (as disclosed on my other reviews in
this repository). All checks were executed locally against a clean worktree at
head 2e6968c57ecca3bcdf2b208d0255e51c570776ff and at base
4cc9fa17653d695d64fb9eee5b33d4de55ff84e8, using CPython 3.13.12, pytest, and
black 23.7.0; mcp 2.3.0 and httpx2 2.13.1 were installed in a throwaway
virtual environment for the MCP tests. No network requests were made by the
reviewed code during these runs.

Verdict

I would merge this. The change is minimal, it fixes a real bug that the new
tests reproduce on the base commit, the default port and the host validation
are unchanged, and the end-to-end behavior is what the uvicorn documentation
promises. The README note in (A) is the only thing I would consider adding,
and it can be a follow-up.

[Bicheng (Kenneth) (BichengWang)]

Thank you for the review, XU (@kokokoXUY) any chance to get approval?

[XU (kokokoXUY)]

Thanks for the CLA and for the kind words. I should be clear about one thing: I am
not a maintainer of markitdown -- I am an outside contributor -- so an approval
from me would not help this land. The review above is a verification report, not a
gate, and the decision belongs to the maintainers.

For what it is worth, it is a positive report: the falsy-zero bug is real and I
reproduced both behaviours on the base commit, the patch is minimal, and my
end-to-end runs against a live uvicorn server matched the documented behaviour.
The only suggestions I had were two documentation notes -- one for the README and
one for the PR description -- about --port 0 with STDIO now being rejected like
any other explicit port. Both are optional and can be follow-ups.

Nothing else is needed from my side. If a maintainer asks for changes, I am happy
to re-verify the updated revision locally.

[Bicheng (Kenneth) (BichengWang)]

Adam Fourney (@afourney), could you approve the test and pre-commit workflows and take a look at this PR and #2676, #2677, and #2685? All four are mergeable against current main, with passing CLA checks. #2685 has a community approval; is anything else needed before merge?