feat!: configure the base URL via COMFY_BASE_URL instead of a constructor argument - #43
Conversation
…rgument `Comfy`/`AsyncComfy` now always target Comfy Cloud unless the `COMFY_BASE_URL` environment variable names another deployment. The base-URL constructor parameter is gone, so an arbitrary endpoint is no longer part of the call surface. `api_key` is keyword-only now: the old positional `Comfy(url, key)` form would otherwise read a URL as a key silently. The variable is read per construction (not at import), must be an http(s) URL, and unset-or-blank means Comfy Cloud. The unit suite's `server` fixture sets it, so tests construct a bare `Comfy()`, and an autouse fixture scrubs an ambient value so a developer's own shell cannot point the suite at a real deployment.
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe SDK now selects deployment targets through ChangesDeployment targeting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant Comfy
participant Environment
participant ComfyLow
Application->>Comfy: construct client
Comfy->>Environment: read COMFY_BASE_URL
Environment-->>Comfy: configured URL or blank value
Comfy->>Comfy: validate URL or select Comfy Cloud
Comfy->>ComfyLow: initialize resolved deployment client
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 4
🤖 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 `@README.md`:
- Around line 76-90: Add a migration note near the Comfy/AsyncComfy constructor
examples explicitly stating that api_key must now be passed by keyword, the
former positional base-URL constructor is unsupported, and COMFY_BASE_URL must
be configured before constructing either client.
In `@src/comfy_sdk/client.py`:
- Around line 58-63: Update _resolve_base_url to access parsed.port after
validating the URL components, causing malformed or out-of-range ports to raise
the existing COMFY_BASE_URL ValueError before returning the URL to ComfyLow.
In `@tests/integration/test_gateway_e2e.py`:
- Around line 96-102: Update the client fixture function client to accept the
pytest monkeypatch fixture and replace the direct os.environ assignment with
monkeypatch.setenv(BASE_URL_ENV_VAR, BASE_URL), ensuring COMFY_BASE_URL is
automatically restored after the fixture while preserving the existing client
setup and cleanup.
In `@tests/test_base_url_env.py`:
- Around line 80-93: Add asynchronous tests alongside
test_async_env_var_selects_the_deployment covering blank and whitespace-only
COMFY_BASE_URL values, whitespace trimming, and malformed URL rejection,
mirroring the synchronous cases from lines 45-71. Exercise AsyncComfy’s actual
URL resolution and validation behavior, asserting defaults or normalized URLs
where appropriate and the expected exception for malformed values.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f3b6669-93c5-4acc-962f-452fe6d7c514
📒 Files selected for processing (17)
README.mdsrc/comfy_sdk/__init__.pysrc/comfy_sdk/client.pytests/conftest.pytests/integration/test_gateway_e2e.pytests/test_assets.pytests/test_async.pytests/test_auth_headers.pytests/test_base_url_env.pytests/test_content_redirect_security.pytests/test_default_base_url.pytests/test_download_and_workflows.pytests/test_events.pytests/test_get_download_url.pytests/test_jobs.pytests/test_sse_idle.pytests/test_user_agent.py
💤 Files with no reviewable changes (1)
- tests/test_default_base_url.py
urlsplit defers the port check, so `http://host:bad` passed validation and only failed later inside httpx. Reading `.port` in the resolver turns it into the same clear COMFY_BASE_URL error. README: blank means whitespace-only too, plus a migration line for the keyword-only `api_key`.
The transport builds request URLs by appending the API path to this string, so `https://host?x=1` put `/api/v2/...` inside the query and the request never reached the endpoint. Also cover AsyncComfy in the malformed-value tests.
The e2e fixture set COMFY_BASE_URL directly and never unset it. Scope the unit-suite scrub to skip tests/integration instead, so that suite simply inherits the variable it is configured with and monkeypatch leaves the environment as it was found. Also fixes the latent trap that the variable was absent during the module's later tests, so a client built inside a test body would have targeted Comfy Cloud.
|
@coderabbitai review All four findings are addressed (replies on each thread): the migration note, port validation, the async rejection test, and the gateway fixture no longer mutating |
|
|
3 participants
Comfy()/AsyncComfy()target Comfy Cloud; another deployment is selected with theCOMFY_BASE_URLenvironment variable. The base-URL constructor parameter is removed, so an arbitrary endpoint is no longer part of the call surface.Breaking:
Comfy(base_url, api_key)→Comfy(api_key=...)+COMFY_BASE_URL.api_keyis keyword-only now, so the old positional form raisesTypeErrorinstead of quietly reading a URL as a key.The variable is read per construction (not at import), must be an
http(s)URL, and unset-or-blank means Comfy Cloud.Test suite: the
serverfixture points the SDK at the stub by setting the variable, so tests construct a bareComfy(); an autouse fixture scrubs an ambient value so a developer's own shell can't aim the suite at a real deployment. That is most of the diff.Scope: the public clients only.
comfy_low(the documented escape hatch) still takes a base URL — it is what the clients build on.Verified:
ruff check,ruff format --check,mypy src,pytest(120 pass, 4 skipped).Matching change in
comfy-typescript-sdkkeeps the two SDKs in lockstep.Summary by CodeRabbit
New Features
COMFY_BASE_URLenvironment variable.Documentation