Feat/add comfy env var support#478
Conversation
- COMFY_BACKEND=host:port (one variable, colon-separated) - COMFY_HOST and COMFY_PORT (separate variables) These provide fallback when --host/--port are not passed on the command line. Resolves the missing environment variable oversight for backend configuration.
…d COMFY_BACKEND env vars - Extend _get_backend_from_env to handle COMFY_HOST_PORT (and keep COMFY_BACKEND for combined form) - Update docstrings and usage in cmdline.py and jobs.py - CLI flags still take precedence as before - This matches the requested env var naming (tracking CLI params closely)
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a shared ChangesHost/Port Resolution and Environment Fallback
ROCm Default Version Bump
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmdline as comfy_cli.cmdline
participant HostPort as comfy_cli.host_port
participant EnvBackend as comfy_cli.env_backend
participant Config as persisted config
User->>Cmdline: comfy run [--host]
Cmdline->>HostPort: parse_host_port_arg(host)
Cmdline->>HostPort: resolve_host_port(host, port)
HostPort->>EnvBackend: check env vars if not provided
HostPort->>Config: check config.background if still unset
HostPort-->>Cmdline: resolved host, port
User->>Cmdline: comfy validate
Cmdline->>EnvBackend: get_backend_from_env() if host/port unset
EnvBackend-->>Cmdline: env_h, env_p
Cmdline->>Cmdline: Graph.load(host, port)
Possibly related PRs
Suggested reviewers: No pun intended, but this host/port refactor really resolves itself nicely — and the ROCm bump? Just version-y business as usual! 🎈 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
comfy_cli/command/jobs.py (1)
38-64: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSame helper, twice the maintenance burden.
Identical to
_get_backend_from_env()incomfy_cli/cmdline.py(docstring, IPv6 rsplit gap, silentValueErrorswallow, and the localimport osshadowing the module-level import at line 23 — flagged by Pylint W0621/W0404/C0415, all included). See the detailed fix suggestion on the cmdline.py copy; the punchline here is the same: extract once, import twice, and this bug-fixing duet becomes a solo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/command/jobs.py` around lines 38 - 64, The helper _get_backend_from_env in jobs.py is a duplicate of the same logic in cmdline.py and should not be maintained in two places. Move the backend parsing into one shared helper and have both callers import/use that single implementation, keeping the existing behavior for COMFY_BACKEND/COMFY_HOST_PORT and COMFY_HOST/COMFY_PORT while preserving the same return shape. Also remove the local import os inside _get_backend_from_env so it uses the module-level import, matching the shared cleanup.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/cmdline.py`:
- Around line 912-916: The validate command’s envvar handling is inconsistent
with run() because it only uses Typer envvar bindings for COMFY_HOST/COMFY_PORT
and ignores COMFY_BACKEND/COMFY_HOST_PORT. Update the validate path in
comfy_cli/cmdline.py to use the same backend resolution logic as
_get_backend_from_env(), or a shared helper, so both validate and run honor the
same environment variables. Keep the fix anchored around validate() and
_get_backend_from_env() to preserve parity for where-aware commands.
- Around line 47-73: The env-backend parsing helper is duplicated and should be
centralized so `cmdline.py` and `command/jobs.py` both use a shared function
instead of copy-pasting `_get_backend_from_env`. Move the logic into a common
module and import it at each call site, then remove the local `import os` from
`_get_backend_from_env` since `os` is already available at module scope. While
refactoring, make the parser handle IPv6/bracketed hosts correctly and avoid
silently swallowing invalid port values; keep the shared helper consistent with
the host/port resolution behavior used elsewhere such as `_resolve_host_port`.
---
Duplicate comments:
In `@comfy_cli/command/jobs.py`:
- Around line 38-64: The helper _get_backend_from_env in jobs.py is a duplicate
of the same logic in cmdline.py and should not be maintained in two places. Move
the backend parsing into one shared helper and have both callers import/use that
single implementation, keeping the existing behavior for
COMFY_BACKEND/COMFY_HOST_PORT and COMFY_HOST/COMFY_PORT while preserving the
same return shape. Also remove the local import os inside _get_backend_from_env
so it uses the module-level import, matching the shared cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7419be01-7f4d-4455-89cc-09fe789e7847
📒 Files selected for processing (2)
comfy_cli/cmdline.pycomfy_cli/command/jobs.py
|
I have read and agree to the Contributor License Agreement |
- Extract duplicate helper from cmdline.py and jobs.py into comfy_cli/env_backend.py - Add IPv6 bracketed host support ([::1]:8188) - Log warning on invalid port values instead of silent swallow - Remove local 'import os' (was Pylint W0621/W0404/C0415) - Route validate() through same env fallback as run() for feature parity - Remove Typer envvar= bindings from validate (was COMFY_HOST/COMFY_PORT only) Addresses CodeRabbit review feedback on PR Comfy-Org#478.
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/cmdline.py`:
- Around line 885-892: The help text for the host and port options no longer
surfaces the new environment-variable fallback, so update the Typer option
descriptions in cmdline.py where the host and port parameters are defined.
Adjust the help strings for the host/port options (and any related backend
option if present) to mention that values can come from COMFY_HOST, COMFY_PORT,
or COMFY_BACKEND via _get_backend_from_env(), keeping the docs aligned with the
new manual resolution behavior.
In `@comfy_cli/env_backend.py`:
- Line 17: The _IPV6_RE regex is defined but unused, so either remove the dead
constant or update _split_host_port to use it for IPv6 host detection. Inspect
the existing manual parsing in _split_host_port and the _IPV6_RE symbol
together, then keep only the approach that is actually referenced and intended.
- Around line 39-59: The COMFY_PORT validation path in the environment backend
still uses logging.getLogger(...) without a module-level import, so an
out-of-range port can hit an unbound logging reference and crash. Move the
logging import to the top of comfy_cli/env_backend.py and remove the inline
import inside the ValueError branch in the env parsing function that reads
COMFY_HOST and COMFY_PORT. While updating that block, also simplify the
redundant port is not None check in the same parsing logic so the behavior stays
clean and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e3ad308-b330-41fa-809f-fd5f29b7f187
📒 Files selected for processing (3)
comfy_cli/cmdline.pycomfy_cli/command/jobs.pycomfy_cli/env_backend.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/cmdline.py`:
- Around line 885-892: The help text for the host and port options no longer
surfaces the new environment-variable fallback, so update the Typer option
descriptions in cmdline.py where the host and port parameters are defined.
Adjust the help strings for the host/port options (and any related backend
option if present) to mention that values can come from COMFY_HOST, COMFY_PORT,
or COMFY_BACKEND via _get_backend_from_env(), keeping the docs aligned with the
new manual resolution behavior.
In `@comfy_cli/env_backend.py`:
- Line 17: The _IPV6_RE regex is defined but unused, so either remove the dead
constant or update _split_host_port to use it for IPv6 host detection. Inspect
the existing manual parsing in _split_host_port and the _IPV6_RE symbol
together, then keep only the approach that is actually referenced and intended.
- Around line 39-59: The COMFY_PORT validation path in the environment backend
still uses logging.getLogger(...) without a module-level import, so an
out-of-range port can hit an unbound logging reference and crash. Move the
logging import to the top of comfy_cli/env_backend.py and remove the inline
import inside the ValueError branch in the env parsing function that reads
COMFY_HOST and COMFY_PORT. While updating that block, also simplify the
redundant port is not None check in the same parsing logic so the behavior stays
clean and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e3ad308-b330-41fa-809f-fd5f29b7f187
📒 Files selected for processing (3)
comfy_cli/cmdline.pycomfy_cli/command/jobs.pycomfy_cli/env_backend.py
🛑 Comments failed to post (3)
comfy_cli/cmdline.py (1)
885-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Help text doesn't mention the new env var fallback.
--host/--portpreviously relied on Typer'senvvar=binding, which auto-documents the env var in--help. Now that resolution is manual via_get_backend_from_env(), the help strings ("default 127.0.0.1"/"default 8188") no longer hint thatCOMFY_HOST/COMFY_PORT/COMFY_BACKENDcan also drive these values — a small loss of discoverability for the very feature this PR ships, punny as that irony is.✏️ Suggested help text tweak
host: Annotated[ str | None, - typer.Option(show_default=False, help="ComfyUI host (default 127.0.0.1)."), + typer.Option(show_default=False, help="ComfyUI host (default 127.0.0.1; env: COMFY_HOST/COMFY_BACKEND)."), ] = None, port: Annotated[ int | None, - typer.Option(show_default=False, help="ComfyUI port (default 8188)."), + typer.Option(show_default=False, help="ComfyUI port (default 8188; env: COMFY_PORT/COMFY_BACKEND)."), ] = None,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.host: Annotated[ str | None, typer.Option(show_default=False, help="ComfyUI host (default 127.0.0.1; env: COMFY_HOST/COMFY_BACKEND)."), ] = None, port: Annotated[ int | None, typer.Option(show_default=False, help="ComfyUI port (default 8188; env: COMFY_PORT/COMFY_BACKEND)."), ] = None,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/cmdline.py` around lines 885 - 892, The help text for the host and port options no longer surfaces the new environment-variable fallback, so update the Typer option descriptions in cmdline.py where the host and port parameters are defined. Adjust the help strings for the host/port options (and any related backend option if present) to mention that values can come from COMFY_HOST, COMFY_PORT, or COMFY_BACKEND via _get_backend_from_env(), keeping the docs aligned with the new manual resolution behavior.comfy_cli/env_backend.py (2)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Unused
_IPV6_RE— dead weight, or a regex that never gets its regex-ercise.
_IPV6_REis defined but never referenced;_split_host_portdoes its own manual bracket/colon-count parsing instead. Consider removing it, or wiring it in if it was meant to drive the IPv6 detection path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/env_backend.py` at line 17, The _IPV6_RE regex is defined but unused, so either remove the dead constant or update _split_host_port to use it for IPv6 host detection. Inspect the existing manual parsing in _split_host_port and the _IPV6_RE symbol together, then keep only the approach that is actually referenced and intended.
39-59: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the target file and show the relevant section with line numbers. git ls-files comfy_cli/env_backend.py wc -l comfy_cli/env_backend.py sed -n '1,140p' comfy_cli/env_backend.py | cat -n # Run a tiny Python repro for the scoping behavior in question. python3 - <<'PY' def repro(p): if p: try: port = int(p) except ValueError: import logging return "value error path" if port > 100: logging.getLogger(__name__).warning("oops") return None return None for value in ["999", "0", "abc", None, "42"]: try: print(value, "->", repro(value)) except Exception as e: print(value, "->", type(e).__name__, e) PYRepository: Comfy-Org/comfy-cli
Length of output: 4443
Hoist
loggingto module scopeTiny typo, big topple:
loggingis only imported on theValueErrorpath, so an out-of-rangeCOMFY_PORTreacheslogging.getLogger(...)with an unbound local and crashes the CLI. Move the import to the top of the module; the duplicateport is not Nonecheck can go too, while the imp is at it.🧰 Tools
🪛 Pylint (4.0.6)
[convention] 47-47: Import outside toplevel (logging)
(C0415)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/env_backend.py` around lines 39 - 59, The COMFY_PORT validation path in the environment backend still uses logging.getLogger(...) without a module-level import, so an out-of-range port can hit an unbound logging reference and crash. Move the logging import to the top of comfy_cli/env_backend.py and remove the inline import inside the ValueError branch in the env parsing function that reads COMFY_HOST and COMFY_PORT. While updating that block, also simplify the redundant port is not None check in the same parsing logic so the behavior stays clean and consistent.Source: Linters/SAST tools
… module Conflict resolution: - Upstream extracted host/port logic into comfy_cli/host_port.py - Our env var support now lives in env_backend.py (complementary module) - resolve_host_port() calls get_backend_from_env() internally — all 6 jobs.py call sites and cmdline.py run() get env var support automatically - validate() uses inline env fallback (doesn't go through resolve_host_port) - No duplicate code — single env_backend.py, single host_port.py - IPv6 handling from upstream's host_port.py preserved
dosubot
Bot
added
size:XXL
…var-support # Conflicts: # comfy_cli/host_port.py
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/cql/loader.py (1)
83-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBracket IPv6 hosts before building the local
object_infoURL
http://{host}:{port}/object_infobreaks forhost="::1":urlsplit()can't recover a usable hostname from the unbracketed netloc, so the loopback check sees""and rejects a valid IPv6 localhost target. Bracketing the host here keeps::1reachable, not just on paper—tiny fix, big payoff.🩹 Proposed fix
def _load_from_server(host: str, port: int, *, timeout: float) -> dict[str, Any]: - url = f"http://{host}:{port}/object_info" + host_for_url = f"[{host}]" if ":" in host else host + url = f"http://{host_for_url}:{port}/object_info"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/cql/loader.py` around lines 83 - 90, The URL construction in _load_from_server is not handling IPv6 localhost hosts correctly, since an unbracketed host like "::1" breaks urllib.parse.urlsplit and causes the loopback check to reject it. Update the host formatting in _load_from_server so IPv6 addresses are bracketed before building the object_info URL, while keeping the existing is_loopback_host validation in place for the parsed hostname.comfy_cli/cmdline.py (1)
914-923: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReuse
resolve_host_port()here
validate()still hand-rolls env fallback, so it missesconfig.backgroundand thevalidate_host()checks that the shared resolver already applies.Graph.load()brackets IPv6 later, so that part is already covered; using the shared helper keeps host resolution consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/cmdline.py` around lines 914 - 923, `validate()` is duplicating host/port fallback logic instead of using the shared resolver, so it can miss `config.background` and the `validate_host()` checks applied elsewhere. Update `validate()` to call `resolve_host_port()` and use its returned host/port before invoking `Graph.load()`, so host resolution stays consistent with `comfy run` and the rest of the command flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/host_port.py`:
- Around line 73-80: The _to_port helper currently loses the original ValueError
when int(s) fails, so update the exception handling in _to_port to chain the
caught ValueError into the typer.BadParameter raise. Keep the existing
validation and messages, but preserve the traceback by re-raising with the
original exception attached.
In `@comfy_cli/uv.py`:
- Around line 125-129: The ROCm and CUDA URLs/backends in uv.py are still
hardcoded, which duplicates version strings that already live in constants.
Update the setup in uv.py to derive rocmPytorchUrl, nvidiaPytorchUrl, and
rocmTorchBackend/cpuTorchBackend from constants.ROCmVersion and
constants.CUDAVersion instead of embedding the values directly, and make the
same source-of-truth adjustment anywhere install.py or related helpers reference
these strings so future version bumps only need constants.py.
In `@tests/comfy_cli/command/test_workflow_saved.py`:
- Around line 246-249: The test for order normalization only checks env["ok"],
so it can pass even if --order is ignored; strengthen
test_order_normalized_case_insensitively by asserting the request made through
the patched URL helper contains the normalized lowercase order value, similar to
the sibling tests that inspect calls[0]["url"]. Use the existing _run,
_patch_urlopen, and request capture pattern to verify ASC is translated to
order=asc in the actual request.
In `@tests/comfy_cli/test_host_port.py`:
- Around line 102-137: The resolve_host_port tests are still affected by ambient
COMFY_* environment variables because only ConfigManager is patched. Update the
test setup in test_host_port.py so these cases explicitly clear or isolate
COMFY_HOST, COMFY_PORT, COMFY_BACKEND, and COMFY_HOST_PORT before calling
resolve_host_port, especially in the _patch_background helper or a shared
fixture. Keep the assertions against resolve_host_port, DEFAULT_HOST, and
DEFAULT_PORT the same, but ensure the environment cannot override the intended
behavior.
---
Outside diff comments:
In `@comfy_cli/cmdline.py`:
- Around line 914-923: `validate()` is duplicating host/port fallback logic
instead of using the shared resolver, so it can miss `config.background` and the
`validate_host()` checks applied elsewhere. Update `validate()` to call
`resolve_host_port()` and use its returned host/port before invoking
`Graph.load()`, so host resolution stays consistent with `comfy run` and the
rest of the command flow.
In `@comfy_cli/cql/loader.py`:
- Around line 83-90: The URL construction in _load_from_server is not handling
IPv6 localhost hosts correctly, since an unbracketed host like "::1" breaks
urllib.parse.urlsplit and causes the loopback check to reject it. Update the
host formatting in _load_from_server so IPv6 addresses are bracketed before
building the object_info URL, while keeping the existing is_loopback_host
validation in place for the parsed hostname.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1870e95d-ba7c-4a49-bd44-1c070ce3232f
📒 Files selected for processing (31)
comfy_cli/cloud/oauth.pycomfy_cli/cmdline.pycomfy_cli/comfy_client.pycomfy_cli/command/install.pycomfy_cli/command/jobs.pycomfy_cli/command/launch.pycomfy_cli/command/models/search.pycomfy_cli/command/nodes.pycomfy_cli/command/transfer.pycomfy_cli/command/workflow.pycomfy_cli/command/workflow_fragments.pycomfy_cli/constants.pycomfy_cli/cql/_net.pycomfy_cli/cql/engine.pycomfy_cli/cql/loader.pycomfy_cli/error_codes.pycomfy_cli/host_port.pycomfy_cli/http.pycomfy_cli/uv.pycomfy_cli/where.pytests/comfy_cli/auth/test_where.pytests/comfy_cli/command/models/test_search.pytests/comfy_cli/command/test_launch_background.pytests/comfy_cli/command/test_workflow_saved.pytests/comfy_cli/cql/test_engine.pytests/comfy_cli/cql/test_loader.pytests/comfy_cli/cql/test_net.pytests/comfy_cli/test_host_port.pytests/comfy_cli/test_http.pytests/comfy_cli/test_install_python_resolution.pytests/uv/test_uv.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/cql/loader.py (1)
83-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBracket IPv6 hosts before building the local
object_infoURL
http://{host}:{port}/object_infobreaks forhost="::1":urlsplit()can't recover a usable hostname from the unbracketed netloc, so the loopback check sees""and rejects a valid IPv6 localhost target. Bracketing the host here keeps::1reachable, not just on paper—tiny fix, big payoff.🩹 Proposed fix
def _load_from_server(host: str, port: int, *, timeout: float) -> dict[str, Any]: - url = f"http://{host}:{port}/object_info" + host_for_url = f"[{host}]" if ":" in host else host + url = f"http://{host_for_url}:{port}/object_info"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/cql/loader.py` around lines 83 - 90, The URL construction in _load_from_server is not handling IPv6 localhost hosts correctly, since an unbracketed host like "::1" breaks urllib.parse.urlsplit and causes the loopback check to reject it. Update the host formatting in _load_from_server so IPv6 addresses are bracketed before building the object_info URL, while keeping the existing is_loopback_host validation in place for the parsed hostname.comfy_cli/cmdline.py (1)
914-923: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReuse
resolve_host_port()here
validate()still hand-rolls env fallback, so it missesconfig.backgroundand thevalidate_host()checks that the shared resolver already applies.Graph.load()brackets IPv6 later, so that part is already covered; using the shared helper keeps host resolution consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/cmdline.py` around lines 914 - 923, `validate()` is duplicating host/port fallback logic instead of using the shared resolver, so it can miss `config.background` and the `validate_host()` checks applied elsewhere. Update `validate()` to call `resolve_host_port()` and use its returned host/port before invoking `Graph.load()`, so host resolution stays consistent with `comfy run` and the rest of the command flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/host_port.py`:
- Around line 73-80: The _to_port helper currently loses the original ValueError
when int(s) fails, so update the exception handling in _to_port to chain the
caught ValueError into the typer.BadParameter raise. Keep the existing
validation and messages, but preserve the traceback by re-raising with the
original exception attached.
In `@comfy_cli/uv.py`:
- Around line 125-129: The ROCm and CUDA URLs/backends in uv.py are still
hardcoded, which duplicates version strings that already live in constants.
Update the setup in uv.py to derive rocmPytorchUrl, nvidiaPytorchUrl, and
rocmTorchBackend/cpuTorchBackend from constants.ROCmVersion and
constants.CUDAVersion instead of embedding the values directly, and make the
same source-of-truth adjustment anywhere install.py or related helpers reference
these strings so future version bumps only need constants.py.
In `@tests/comfy_cli/command/test_workflow_saved.py`:
- Around line 246-249: The test for order normalization only checks env["ok"],
so it can pass even if --order is ignored; strengthen
test_order_normalized_case_insensitively by asserting the request made through
the patched URL helper contains the normalized lowercase order value, similar to
the sibling tests that inspect calls[0]["url"]. Use the existing _run,
_patch_urlopen, and request capture pattern to verify ASC is translated to
order=asc in the actual request.
In `@tests/comfy_cli/test_host_port.py`:
- Around line 102-137: The resolve_host_port tests are still affected by ambient
COMFY_* environment variables because only ConfigManager is patched. Update the
test setup in test_host_port.py so these cases explicitly clear or isolate
COMFY_HOST, COMFY_PORT, COMFY_BACKEND, and COMFY_HOST_PORT before calling
resolve_host_port, especially in the _patch_background helper or a shared
fixture. Keep the assertions against resolve_host_port, DEFAULT_HOST, and
DEFAULT_PORT the same, but ensure the environment cannot override the intended
behavior.
---
Outside diff comments:
In `@comfy_cli/cmdline.py`:
- Around line 914-923: `validate()` is duplicating host/port fallback logic
instead of using the shared resolver, so it can miss `config.background` and the
`validate_host()` checks applied elsewhere. Update `validate()` to call
`resolve_host_port()` and use its returned host/port before invoking
`Graph.load()`, so host resolution stays consistent with `comfy run` and the
rest of the command flow.
In `@comfy_cli/cql/loader.py`:
- Around line 83-90: The URL construction in _load_from_server is not handling
IPv6 localhost hosts correctly, since an unbracketed host like "::1" breaks
urllib.parse.urlsplit and causes the loopback check to reject it. Update the
host formatting in _load_from_server so IPv6 addresses are bracketed before
building the object_info URL, while keeping the existing is_loopback_host
validation in place for the parsed hostname.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1870e95d-ba7c-4a49-bd44-1c070ce3232f
📒 Files selected for processing (31)
comfy_cli/cloud/oauth.pycomfy_cli/cmdline.pycomfy_cli/comfy_client.pycomfy_cli/command/install.pycomfy_cli/command/jobs.pycomfy_cli/command/launch.pycomfy_cli/command/models/search.pycomfy_cli/command/nodes.pycomfy_cli/command/transfer.pycomfy_cli/command/workflow.pycomfy_cli/command/workflow_fragments.pycomfy_cli/constants.pycomfy_cli/cql/_net.pycomfy_cli/cql/engine.pycomfy_cli/cql/loader.pycomfy_cli/error_codes.pycomfy_cli/host_port.pycomfy_cli/http.pycomfy_cli/uv.pycomfy_cli/where.pytests/comfy_cli/auth/test_where.pytests/comfy_cli/command/models/test_search.pytests/comfy_cli/command/test_launch_background.pytests/comfy_cli/command/test_workflow_saved.pytests/comfy_cli/cql/test_engine.pytests/comfy_cli/cql/test_loader.pytests/comfy_cli/cql/test_net.pytests/comfy_cli/test_host_port.pytests/comfy_cli/test_http.pytests/comfy_cli/test_install_python_resolution.pytests/uv/test_uv.py
🛑 Comments failed to post (4)
comfy_cli/host_port.py (1)
73-80: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Chain the exception for a preserved traceback.
🔗 Suggested fix
def _to_port(s: str, original: str) -> int: try: port = int(s) - except ValueError: - raise typer.BadParameter(f"invalid port in {original!r}: {s!r} is not a number") + except ValueError as exc: + raise typer.BadParameter(f"invalid port in {original!r}: {s!r} is not a number") from exc if not (1 <= port <= 65535): raise typer.BadParameter(f"invalid port in {original!r}: {port} is out of range (1-65535)") return port📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def _to_port(s: str, original: str) -> int: try: port = int(s) except ValueError as exc: raise typer.BadParameter(f"invalid port in {original!r}: {s!r} is not a number") from exc if not (1 <= port <= 65535): raise typer.BadParameter(f"invalid port in {original!r}: {port} is out of range (1-65535)") return port🧰 Tools
🪛 Pylint (4.0.6)
[convention] 78-78: Unnecessary parens after 'not' keyword
(C0325)
[warning] 77-77: Consider explicitly re-raising using 'except ValueError as exc' and 'raise typer.BadParameter(f'invalid port in {original!r}: {s!r} is not a number') from exc'
(W0707)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/host_port.py` around lines 73 - 80, The _to_port helper currently loses the original ValueError when int(s) fails, so update the exception handling in _to_port to chain the caught ValueError into the typer.BadParameter raise. Keep the existing validation and messages, but preserve the traceback by re-raising with the original exception attached.Source: Linters/SAST tools
comfy_cli/uv.py (1)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive ROCm/CUDA strings from
constantsinstead of re-hardcoding.This bump required touching
constants.py,uv.py, andinstall.pyin lockstep just to move6.3→7.2. That's three places to remember next time — a version-skew imp waiting to sneak in. Consider sourcing these fromconstants.ROCmVersion/CUDAVersionso there's a single source of truth.♻️ Suggested fix
+from comfy_cli import constants + class DependencyCompiler: cpuPytorchUrl = "https://download.pytorch.org/whl/cpu" - rocmPytorchUrl = "https://download.pytorch.org/whl/rocm7.2" - nvidiaPytorchUrl = "https://download.pytorch.org/whl/cu126" + rocmPytorchUrl = f"https://download.pytorch.org/whl/rocm{constants.ROCmVersion.v7_2.value}" + nvidiaPytorchUrl = f"https://download.pytorch.org/whl/{constants.CUDAVersion.v12_6.value.replace('cu', 'cu')}" cpuTorchBackend = "cpu" - rocmTorchBackend = "rocm7.2" - nvidiaTorchBackend = "cu126" + rocmTorchBackend = f"rocm{constants.ROCmVersion.v7_2.value}" + nvidiaTorchBackend = "cu126"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.from comfy_cli import constants class DependencyCompiler: cpuPytorchUrl = "https://download.pytorch.org/whl/cpu" rocmPytorchUrl = f"https://download.pytorch.org/whl/rocm{constants.ROCmVersion.v7_2.value}" nvidiaPytorchUrl = f"https://download.pytorch.org/whl/{constants.CUDAVersion.v12_6.value.replace('cu', 'cu')}" cpuTorchBackend = "cpu" rocmTorchBackend = f"rocm{constants.ROCmVersion.v7_2.value}" nvidiaTorchBackend = "cu126"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/uv.py` around lines 125 - 129, The ROCm and CUDA URLs/backends in uv.py are still hardcoded, which duplicates version strings that already live in constants. Update the setup in uv.py to derive rocmPytorchUrl, nvidiaPytorchUrl, and rocmTorchBackend/cpuTorchBackend from constants.ROCmVersion and constants.CUDAVersion instead of embedding the values directly, and make the same source-of-truth adjustment anywhere install.py or related helpers reference these strings so future version bumps only need constants.py.tests/comfy_cli/command/test_workflow_saved.py (1)
246-249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Strengthen case-insensitivity assertion for
--order.Only
env["ok"] is Trueis checked; the test doesn't confirmASCwas actually normalized (e.g., toorder=ascin the request URL) the way sibling tests assert oncalls[0]["url"]. As-is, a no-op or ignored--ordervalue would still pass.✏️ Proposed strengthening
def test_order_normalized_case_insensitively(self, local_target, monkeypatch, capsys): - _patch_urlopen(monkeypatch, {"/userdata": _USERDATA_LIST_RESPONSE}) + calls = _patch_urlopen(monkeypatch, {"/userdata": _USERDATA_LIST_RESPONSE}) env = _run(["list", "--where", "local", "--order", "ASC"], capsys) assert env["ok"] is True + assert "order=asc" in calls[0]["url"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_order_normalized_case_insensitively(self, local_target, monkeypatch, capsys): calls = _patch_urlopen(monkeypatch, {"/userdata": _USERDATA_LIST_RESPONSE}) env = _run(["list", "--where", "local", "--order", "ASC"], capsys) assert env["ok"] is True assert "order=asc" in calls[0]["url"]🧰 Tools
🪛 Pylint (4.0.6)
[convention] 246-246: Missing function or method docstring
(C0116)
[warning] 246-246: Redefining name 'local_target' from outer scope (line 78)
(W0621)
[warning] 246-246: Unused argument 'local_target'
(W0613)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/comfy_cli/command/test_workflow_saved.py` around lines 246 - 249, The test for order normalization only checks env["ok"], so it can pass even if --order is ignored; strengthen test_order_normalized_case_insensitively by asserting the request made through the patched URL helper contains the normalized lowercase order value, similar to the sibling tests that inspect calls[0]["url"]. Use the existing _run, _patch_urlopen, and request capture pattern to verify ASC is translated to order=asc in the actual request.tests/comfy_cli/test_host_port.py (1)
102-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unit tests aren't isolated from ambient
COMFY_*env vars.
resolve_host_port()readsCOMFY_HOST/COMFY_PORT/COMFY_BACKEND/COMFY_HOST_PORTbefore falling back toconfig.background/defaults, but these tests only patchConfigManager— they never clear those env vars. Anyone (or any CI runner) withCOMFY_HOSTset in their shell will get silently wrong/flaky results here, e.g.test_resolve_host_port_applies_defaultswould fail ifCOMFY_HOSTleaks in. A little ironic for the very feature that introduces these vars to be the one whose tests aren't insulated from them.🛡️ Suggested fix: isolate env for these tests
+@pytest.fixture(autouse=True) +def _isolate_comfy_env(monkeypatch): + """Ambient COMFY_* env vars must not leak into host/port resolution tests.""" + for var in ("COMFY_HOST", "COMFY_PORT", "COMFY_BACKEND", "COMFY_HOST_PORT"): + monkeypatch.delenv(var, raising=False) + + def _patch_background(bg): """Patch ConfigManager as seen by resolve_host_port so `.background` == bg.""" cfg = MagicMock() cfg.background = bg return patch("comfy_cli.host_port.ConfigManager", return_value=cfg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@pytest.fixture`(autouse=True) def _isolate_comfy_env(monkeypatch): """Ambient COMFY_* env vars must not leak into host/port resolution tests.""" for var in ("COMFY_HOST", "COMFY_PORT", "COMFY_BACKEND", "COMFY_HOST_PORT"): monkeypatch.delenv(var, raising=False) def _patch_background(bg): """Patch ConfigManager as seen by resolve_host_port so `.background` == bg.""" cfg = MagicMock() cfg.background = bg return patch("comfy_cli.host_port.ConfigManager", return_value=cfg) def test_resolve_host_port_brackets_ipv6(): with _patch_background(None): assert resolve_host_port("::1", None) == ("[::1]", DEFAULT_PORT) def test_resolve_host_port_does_not_double_bracket(): with _patch_background(None): assert resolve_host_port("[::1]", 8188) == ("[::1]", 8188) def test_resolve_host_port_rejects_unsafe_host(): with _patch_background(None), pytest.raises(typer.BadParameter): resolve_host_port("evil.com/@x", None) def test_resolve_host_port_applies_defaults(): with _patch_background(None): assert resolve_host_port(None, None) == (DEFAULT_HOST, DEFAULT_PORT) def test_resolve_host_port_falls_back_to_background(): # config.background is a (host, port, pid) tuple. with _patch_background(("10.0.0.5", 9001, 4242)): assert resolve_host_port(None, None) == ("10.0.0.5", 9001) def test_resolve_host_port_explicit_overrides_background(): with _patch_background(("10.0.0.5", 9001, 4242)): assert resolve_host_port("localhost", 7000) == ("localhost", 7000)🧰 Tools
🪛 Pylint (4.0.6)
[convention] 109-109: Missing function or method docstring
(C0116)
[convention] 114-114: Missing function or method docstring
(C0116)
[convention] 119-119: Missing function or method docstring
(C0116)
[convention] 124-124: Missing function or method docstring
(C0116)
[convention] 129-129: Missing function or method docstring
(C0116)
[convention] 135-135: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/comfy_cli/test_host_port.py` around lines 102 - 137, The resolve_host_port tests are still affected by ambient COMFY_* environment variables because only ConfigManager is patched. Update the test setup in test_host_port.py so these cases explicitly clear or isolate COMFY_HOST, COMFY_PORT, COMFY_BACKEND, and COMFY_HOST_PORT before calling resolve_host_port, especially in the _patch_background helper or a shared fixture. Keep the assertions against resolve_host_port, DEFAULT_HOST, and DEFAULT_PORT the same, but ensure the environment cannot override the intended behavior.
1 participant
Title: Feature support COMFY_HOST, COMFY_PORT, COMFY_HOST_PORT and COMFY_BACKEND environment variables
Body:
Plan
The goal was to add first-class support for environment variables so the comfy tool can be configured to talk to a non-local ComfyUI backend without always requiring
--host/--portflags or relying on hardcoded defaults. This was driven by the need for reliable, scriptable, non-interactive usage (e.g., in containers, CI, or multi-machine setups) while keeping CLI flags as the highest-precedence option.Spec
COMFY_prefix for clarity):COMFY_HOSTCOMFY_PORTCOMFY_HOST_PORT(combinedhost:portform)COMFY_BACKEND(combined form, kept for compatibility)--host/--port).COMFY_BACKENDorCOMFY_HOST_PORT, or separateCOMFY_HOST+COMFY_PORT)._get_backend_from_env) lives in bothcomfy_cli/cmdline.py(main entry) andcomfy_cli/command/jobs.py(job-related paths) to ensure consistent behavior.Tests performed (all on the modified source from the fork, using visible shell simulation via terminal, one or two vars at a time
pip install -e .).COMFY_HOST=172.29.112.112+COMFY_PORT=8188→ resolved to('172.29.112.112', 8188)COMFY_BACKEND=172.29.112.112:8188→ resolved correctlyCOMFY_HOST_PORT=172.29.112.112:8188→ resolved correctly (combined path)curl -I http://172.29.112.112:8188returned HTTP 200 OK.=== INPUT:):comfy --help(withCOMFY_HOST+COMFY_PORT)comfy(bare, with vars set) → showed full agent-aware banner + quick-start (no localhost fallback)comfy nodes ls(withCOMFY_HOST+COMFY_PORT) → returned liveobject_infofrom the remote server (1861 nodes, including packs like ImpactPack, KJNodes, InspirePack, partner nodes for Tripo/ByteDance/Vidu/Wan, etc.). This is the key proof: the data came from172.29.112.112:8188, not defaults.Scripts).The tests prove the env vars are not only read but are actually wired into the backend connection used by real commands.
Additional notes
Tested in the Windows git-bash environment matching the user's setup.
No localhost/127.0.0.1/0.0.0.0 references were introduced or left in the changed code.