Skip to content

fix(server): bind localhost by default#337

Merged
Thump604 merged 1 commit intowaybarrios:mainfrom
Thump604:codex/issue68-default-bind-localhost
Apr 18, 2026
Merged

fix(server): bind localhost by default#337
Thump604 merged 1 commit intowaybarrios:mainfrom
Thump604:codex/issue68-default-bind-localhost

Conversation

@Thump604
Copy link
Copy Markdown
Collaborator

Summary

  • change both serve entrypoints to bind 127.0.0.1 by default instead of 0.0.0.0
  • keep external exposure explicit via --host 0.0.0.0 and update the docs/help text accordingly
  • add a regression test covering the CLI and standalone server parser defaults

Test plan

  • PYTHONPATH=/private/tmp/vllm-mlx-issue68-bind-local /opt/ai-runtime/venv-live/bin/python -m pytest tests/test_server.py -q
  • /opt/ai-runtime/venv-live/bin/python -m black --check --fast vllm_mlx/server.py vllm_mlx/cli.py tests/test_server.py
  • /opt/ai-runtime/venv-live/bin/python -m compileall vllm_mlx/server.py vllm_mlx/cli.py tests/test_server.py

Copy link
Copy Markdown
Collaborator

@janhilgard janhilgard left a comment

Choose a reason for hiding this comment

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

Review: fix(server): bind localhost by default

Addresses: Issue #68, finding #13

Summary: Changes the default --host from 0.0.0.0 to 127.0.0.1 in both the cli.py serve parser and the standalone server.py parser. Docs and help text updated accordingly.

Strengths

  • Correct security default. Binding to all interfaces with no auth is an easy footgun, and localhost-by-default follows the principle of least privilege.
  • Good refactor extracting create_parser() from main() in server.py so the test can inspect parser defaults directly.
  • Test coverage is clean and covers both entrypoints.
  • Docs consistently updated across all three reference pages.

Issues / Suggestions

  1. Breaking change for existing users. Anyone currently relying on the implicit 0.0.0.0 binding (e.g. Docker containers, LAN-accessible setups, systemd services) will silently stop accepting remote connections after upgrading. This deserves a mention in the PR description and ideally a note in the changelog or migration guide. Consider whether a deprecation warning (log a warning when --host is not explicitly set) would smooth the transition, though I acknowledge a clean default change is also defensible.

  2. Minor: create_parser() docstring. The new function says "Create the standalone server argument parser." but it is also now importable and used from tests. A slightly broader docstring like "Create the argument parser for the standalone server entrypoint." would be more accurate since the word "standalone" might confuse readers into thinking it cannot be reused.

Overall this is a clean, well-scoped change. The only real concern is the silent behavioral change for existing deployments.

Verdict: LGTM with the caveat that the breaking change should be documented.

@Thump604 Thump604 force-pushed the codex/issue68-default-bind-localhost branch from 3eb29ef to 52a162e Compare April 18, 2026 02:03
@Thump604 Thump604 merged commit 826cb97 into waybarrios:main Apr 18, 2026
9 checks passed
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