fix CLI overriding environment variables#812
Conversation
CLI options with defaults (--port, --host, --simgen, --file-browser) were always overriding environment variables like ZNDRAW_SIMGEN_ENABLED because they were unconditionally added to config_kwargs. Changed these options to use None defaults, only adding them to config_kwargs when explicitly provided. This allows pydantic-settings to properly read from environment variables when CLI flags are not specified. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughCLI host/port and feature flags (simgen, file_browser) are now optional. Config builder and main only set server and feature values when provided; runtime URLs, messages, and SocketIO startup use values resolved from ZnDrawConfig. Default server bind changed to 0.0.0.0. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/test_*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🪛 Ruff (0.14.8)tests/test_config.py54-54: Possible binding to all interfaces (S104) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zndraw/cli.py (1)
561-561: Useconfig.server_hostinsocketio.run()to respect the--hostCLI flag andZNDRAW_SERVER_HOSTenvironment variable.The server binds to hardcoded
"0.0.0.0"instead ofconfig.server_host. Users who specify--host 127.0.0.1or setZNDRAW_SERVER_HOST=127.0.0.1for security (localhost-only access) will have their security intent silently bypassed—the server still binds to all interfaces. Replacesocketio.run(flask_app, debug=debug, host="0.0.0.0", port=config.server_port)withsocketio.run(flask_app, debug=debug, host=config.server_host, port=config.server_port).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/zndraw/cli.py(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/cli.py
🧬 Code graph analysis (1)
src/zndraw/cli.py (1)
src/zndraw/server_manager.py (1)
write_server_info(82-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
🔇 Additional comments (5)
src/zndraw/cli.py (5)
77-78: LGTM! Signature changes enable environment variable fallback.The parameter type changes correctly allow CLI arguments to be optional, enabling pydantic-settings to read from environment variables when values are not explicitly provided via CLI.
Also applies to: 85-86
98-102: LGTM! Conditional logic correctly prevents CLI defaults from overriding env vars.The implementation only adds server settings to config when explicitly provided, allowing pydantic-settings to read from environment variables otherwise.
136-141: LGTM! Feature flags correctly support environment variable configuration.The conditional logic ensures that feature flags are only set from CLI when explicitly provided, allowing environment variables like
ZNDRAW_SIMGEN_ENABLEDto control defaults otherwise.
174-174: LGTM! CLI option defaults correctly updated to support env vars.All four CLI options now default to
Nonewith updated help text, enabling pydantic-settings to read from environment variables when flags are not specified. The implementation is consistent across all affected options.Also applies to: 208-210, 238-241, 253-256
526-526: LGTM! Correct usage of config values ensures consistency.Using
config.server_portinstead of the raw CLIportparameter ensures the correct value is used, whether it comes from CLI, environment variables, or config defaults.Also applies to: 531-531, 553-553, 556-556
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #812 +/- ##
==========================================
+ Coverage 78.05% 78.34% +0.28%
==========================================
Files 153 153
Lines 18357 18396 +39
==========================================
+ Hits 14329 14412 +83
+ Misses 4028 3984 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/zndraw/config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: If sensible, implement collections.abc interfaces for classes, such as MutableMapping or MutableSequence
Use numpy style docstrings
Docstrings must be concise and to the point
Use type hints wherever possible. Importtyping as tif necessary, but uselist[int|float] | Noneinstead oft.Optional[t.List[int|float]]
Imports should always be at the top of the file
Files:
src/zndraw/config.py
🪛 Ruff (0.14.8)
src/zndraw/config.py
120-120: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (3.12, ubuntu-latest)
- GitHub Check: pytest (3.13, ubuntu-latest)
- GitHub Check: pytest (3.11, ubuntu-latest)
| server_host: str = Field( | ||
| default="localhost", description="Server bind host address" | ||
| ) | ||
| server_host: str = Field(default="0.0.0.0", description="Server bind host address") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check the actual code at line 120
echo "=== Checking config.py around line 120 ==="
sed -n '115,130p' src/zndraw/config.py
echo -e "\n=== Checking for admin credentials defaults ==="
sed -n '135,145p' src/zndraw/config.py
echo -e "\n=== Full server_host related lines ==="
grep -n 'server_host' src/zndraw/config.py
echo -e "\n=== Checking admin_username and admin_password ==="
grep -n 'admin_username\|admin_password' src/zndraw/config.pyRepository: zincware/ZnDraw
Length of output: 1892
🏁 Script executed:
#!/bin/bash
# 2. Check CLI code for server_host usage and context
echo "=== CLI code: server_host references ==="
rg -n -B3 -A3 'server_host' src/zndraw/cli.py || echo "Not found or file missing"
echo -e "\n=== CLI code: binding/0.0.0.0/localhost context ==="
rg -n 'bind|0\.0\.0\.0|localhost' src/zndraw/cli.py || echo "Not found"Repository: zincware/ZnDraw
Length of output: 1460
🏁 Script executed:
#!/bin/bash
# 3. Check if there are any recent git commits related to this change
echo "=== Check recent commits affecting config.py ==="
git log --oneline -20 -- src/zndraw/config.py 2>/dev/null || echo "Git info unavailable"
echo -e "\n=== Check git diff for current file status ==="
git diff src/zndraw/config.py 2>/dev/null | head -100 || echo "No staged changes"Repository: zincware/ZnDraw
Length of output: 216
🏁 Script executed:
#!/bin/bash
# 4. Check for test files that might show intended usage
echo "=== Search for config tests ==="
find . -name '*test*config*' -o -name '*config*test*' 2>/dev/null | head -10
echo -e "\n=== Check tests directory for config usage ==="
if [ -d "tests" ] || [ -d "test" ]; then
rg -n 'server_host\|0\.0\.0\.0' tests/ test/ 2>/dev/null || echo "No matches in tests"
fiRepository: zincware/ZnDraw
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Check test_config.py for hints
echo "=== test_config.py content ==="
cat tests/test_config.py | head -100
echo -e "\n=== Check for environment-specific config handling ==="
rg -n 'ZNDRAW_|environ|production|development' src/zndraw/config.py | head -20
echo -e "\n=== Check if there's any auth middleware or security defaults ==="
rg -n 'auth|security|protect' src/zndraw/config.pyRepository: zincware/ZnDraw
Length of output: 4518
🏁 Script executed:
#!/bin/bash
# Check the full context of line 188
echo "=== Context around line 188 (server_url handling) ==="
sed -n '175,195p' src/zndraw/config.py
echo -e "\n=== Check for any README or docs about deployment ==="
find . -name 'README*' -o -name '*.md' | head -5 | xargs grep -l 'server_host\|0\.0\.0\.0\|localhost' 2>/dev/null || echo "No matches in docs"Repository: zincware/ZnDraw
Length of output: 1147
Critical: Default server binding change breaks tests and creates security exposure.
The default server_host is set to "0.0.0.0" (line 120), exposing the server to all network interfaces. However, tests/test_config.py line 46 explicitly asserts config.server_host == "localhost", causing a test failure. Additionally, the CLI help text (line 210) states the default is localhost.
This indicates an incomplete or unintended change with three issues:
- Test failure: Line 46 of
test_config.pywill fail—the test expects"localhost"but the config now provides"0.0.0.0". - Security regression: Binding to
0.0.0.0exposes the server to the network by default. Since admin credentials are optional (lines 138-139, both default toNone), the server may be accessible without authentication. - Inconsistent documentation: CLI help text and all output URLs hardcoded to
localhost(lines 210, 297, 401, 423, 430, 452, 456, 553, 556) contradict the actual config default.
The code at line 188 shows awareness of this distinction (self.server_host != "0.0.0.0" → localhost for URLs), but the configuration default is misaligned.
Action required: Revert the default to "localhost" (matching the test assertion and documented behavior), allowing users to explicitly opt-in to network exposure via ZNDRAW_SERVER_HOST=0.0.0.0.
🧰 Tools
🪛 Ruff (0.14.8)
120-120: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
In src/zndraw/config.py around line 120, the default server_host should be
reverted from "0.0.0.0" to "localhost" to match tests and docs; change the Field
default to "localhost", ensure environment variable override
(ZNDRAW_SERVER_HOST) still functions so operators can opt into "0.0.0.0"
explicitly, and verify any CLI help text or URL formatting logic remains
consistent with the reverted default.
2 participants
CLI options with defaults (--port, --host, --simgen, --file-browser) were always overriding environment variables like ZNDRAW_SIMGEN_ENABLED because they were unconditionally added to config_kwargs.
Changed these options to use None defaults, only adding them to config_kwargs when explicitly provided. This allows pydantic-settings to properly read from environment variables when CLI flags are not specified.
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.