Skip to content

fix: route auth hints to comfy cloud login instead of nonexistent comfy auth login#512

Open
mattmillerai wants to merge 4 commits into
mainfrom
matt/be-2975-fix-auth-login-refs
Open

fix: route auth hints to comfy cloud login instead of nonexistent comfy auth login#512
mattmillerai wants to merge 4 commits into
mainfrom
matt/be-2975-fix-auth-login-refs

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

If your Comfy Cloud session expired, several error messages and help texts told you to run comfy auth login to sign back in. That command doesn't exist — comfy auth only manages third-party model-host API tokens (Civitai, Hugging Face), and it has no login. The real sign-in command is comfy cloud login. This PR fixes all seven bad hints (plus one stale comment) so users — and agents — get sent to a command that actually works.

What changed

Corrected every user-facing reference to the nonexistent comfy auth logincomfy cloud login:

  • comfy_cli/cloud/oauth.py — 4 OAuth error hints (timeout, authorize failure, token-exchange failure, refresh failure)
  • comfy_cli/command/jobs.py — the jobs --where option help (agent-facing) and the cloud_unauthorized error hint
  • comfy_cli/command/run/__init__.py — the cloud_unauthorized error hint
  • comfy_cli/command/run_cli.py — the demo step description
  • comfy_cli/output/branding.py — a stale code comment ("shown after auth login success")

Also tightened the two signed-out-banner tests (tests/comfy_cli/output/test_branding.py) from a permissive "comfy auth login" in out or "comfy cloud login" in out into an exact comfy cloud login assertion plus a "comfy auth login" not in out guard, so this class of regression is now caught by the suite.

Verified comfy cloud login is the real command: comfy_cli/cloud/command.py:42 registers @app.command("login") under the cloud typer group (cmdline.py:1637). setup.py already used the correct command.

Acceptance

  1. ✅ Repo-wide git grep "auth login" now returns zero user-facing hits — the only remaining matches are the two negative test assertions that guard against the string reappearing.
  2. ✅ Verified against current main (audit snapshot @ a732f5c matched HEAD; edits made by content, not line number).

Out of scope — flagged follow-up (comfy auth whoami)

While auditing I found a sibling bug of the same class: 7 references route users to comfy auth whoami, which was deliberately removed as a command (see tests/comfy_cli/auth/test_auth_command.py:223test_legacy_auth_whoami_is_gone). The correct command is comfy cloud whoami. Locations:

  • comfy_cli/command/jobs.py:1097
  • comfy_cli/command/models/search.py:176, :486
  • comfy_cli/command/workflow.py:659
  • comfy_cli/command/run_cli.py:519 (step title), :522, :524/:525 (invocations + labels) — one of these sits in the same demo step as a line this PR edits

I scoped these OUT of this PR because they are a distinct correction (whoami, not login) spanning 5 files, and this ticket was explicitly the auth login set. Recommend a separate ticket to fix them (same tool-surface audit spirit; the run_cli.py invocations would actually error at runtime, though they are marked optional=True).

Testing

  • ruff check + ruff format --check on all changed files: clean
  • pytest tests/comfy_cli/output/test_branding.py tests/comfy_cli/auth/ tests/comfy_cli/command/test_run_cli.py tests/comfy_cli/command/test_run.py: 203 passed

…comfy auth login`

The `auth` command group manages third-party model-host API tokens
(Civitai, Hugging Face) and has no `login` subcommand. Comfy Cloud
sign-in is `comfy cloud login`. Seven help/hint/error strings (plus a
stale comment) pointed users and agents at the dead `comfy auth login`,
including the agent-facing `jobs --where` option help. Correct them all
and tighten the signed-out banner tests to guard the hint.
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d2f691ea-1cf9-47cf-bd06-42cfd64756db

📥 Commits

Reviewing files that changed from the base of the PR and between 54e3713 and 605d31f.

📒 Files selected for processing (4)
  • comfy_cli/command/jobs.py
  • comfy_cli/command/models/search.py
  • comfy_cli/command/workflow.py
  • tests/comfy_cli/command/test_run_cli.py
📝 Walkthrough

Walkthrough

Cloud authentication guidance was updated across OAuth errors, jobs and cloud execution messages, run CLI examples, capability output, branding documentation, and related tests.

Changes

Cloud command guidance

Layer / File(s) Summary
Runtime authentication hints
comfy_cli/cloud/oauth.py, comfy_cli/command/jobs.py, comfy_cli/command/run/__init__.py
OAuth failures, cloud job usage, and unauthenticated cloud execution now direct users to comfy cloud login.
Cloud CLI demo surface
comfy_cli/command/run_cli.py
Demo commands and capability output now use cloud whoami instead of auth whoami.
Branding and validation
comfy_cli/output/branding.py, tests/comfy_cli/command/test_run_cli.py, tests/comfy_cli/output/test_branding.py
Banner documentation and tests now require cloud-specific login guidance and updated CLI step coverage.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-2975-fix-auth-login-refs
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-2975-fix-auth-login-refs

Comment @coderabbitai help to get the list of available commands.

@mattmillerai mattmillerai added cursor-review Request Cursor bot review agent-coded PR authored by the agent-work loop labels Jul 14, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 1 finding(s).

Severity Count
🟡 Medium 1

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

Comment thread comfy_cli/command/run_cli.py Outdated
The interactive demo's sign-in step invoked `comfy auth whoami`, which was
deliberately removed (test_legacy_auth_whoami_is_gone) — the command now lives
at `comfy cloud whoami`. Update the step title, both invocations, and the
capabilities summary so the walkthrough runs a command that actually exists.
Adjust the step-coverage test needle from "auth" to "whoami" to match.

Addresses cursor-review (gemini-3.1-pro edge-case) thread on PR #512.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Status update (automated resolver):

  • ✅ Addressed the cursor-review finding on the demo whoami step (54e3713) — resolved that thread.
  • ⚠️ The failing "Windows Specific Commands" check is a pre-existing environmental failure, not caused by this diff. It dies in comfy install --fast-deps while resolving ComfyUI's requirements.compiled (ImportError: cannot import name '__version__' from 'pydantic_core'). The same job is red on every open PR from today (be-2975/2976/2977/2978) but was green this morning on another branch, so it's an upstream dependency-resolution regression that a string-only PR can't fix. The Linux/macOS test matrices, build, ruff, and cursor-review all pass.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/comfy_cli/command/test_run_cli.py (1)

28-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the exact cloud command, not only the title.

This test would pass if the title said whoami while invocations still ran comfy auth whoami. Assert the step’s invocation argv/labels contain cloud whoami and exclude auth whoami, keeping the regression net tight and the demo out of the auth weeds.

🤖 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_run_cli.py` around lines 28 - 46, Strengthen
test_build_steps_contains_full_surface by locating the whoami step and asserting
its invocation argv or labels include “cloud whoami” and do not include “auth
whoami”; retain the existing title coverage while verifying the exact cloud
command.
🤖 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.

Outside diff comments:
In `@tests/comfy_cli/command/test_run_cli.py`:
- Around line 28-46: Strengthen test_build_steps_contains_full_surface by
locating the whoami step and asserting its invocation argv or labels include
“cloud whoami” and do not include “auth whoami”; retain the existing title
coverage while verifying the exact cloud command.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2f12adf0-e6e1-4e9f-b6c4-3fd1e5d9e8c6

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 54e3713.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • comfy_cli/cloud/oauth.py
  • comfy_cli/command/jobs.py
  • comfy_cli/command/run/__init__.py
  • comfy_cli/command/run_cli.py
  • comfy_cli/output/branding.py
  • tests/comfy_cli/command/test_run_cli.py
  • tests/comfy_cli/output/test_branding.py

Four cloud error paths (job cancel, workflow HTTP errors, cloud model
list/search) still pointed users at `comfy auth whoami`, which does not
exist — the `auth` group only has list/set/remove, and sign-in status
lives under `comfy cloud whoami`. Same class of bug this PR set out to
fix for `auth login`.

Also strengthen the run_cli demo test per CodeRabbit: assert the whoami
step's argv/labels target `cloud whoami` and never `auth whoami`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 14, 2026
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Addressed review + self-review in e828256:

  • CodeRabbit suggestion (strengthen whoami test): test_build_steps_contains_full_surface now backed by a focused test_whoami_step_targets_cloud_not_auth that locates the whoami demo step and asserts its invocation argv/labels are cloud whoami and never auth whoami.
  • Missed call sites (self-review): found four remaining cloud error hints still pointing at the nonexistent comfy auth whoami (jobs.py cancel, workflow.py cloud HTTP errors, models/search.py cloud list/search). All are cloud-context paths, so routed to comfy cloud whoami (verified it exists via comfy cloud --help; comfy auth only has list/set/remove). Repo-wide grep now shows zero auth login/auth whoami references in source.

Verified the redirect targets are real commands, not dead-ends: comfy cloud login and comfy cloud whoami both exist in the live CLI.

Full suite: 2574 passed, 37 skipped. ruff clean.

Note: the failing "Windows Specific Commands" check is an unrelated infra failure — ComfyUI's requirements.compiled install aborts on Windows with a pydantic_core import error during comfy install, independent of these hint-string changes (the optional bench extra is not installed by pip install -e .). Re-running via the new push.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Resolver root-cause on the failing test (Windows) check — safe to merge past:

The Windows test job dies in dependency install, not in any test:

comfy --skip-prompt --workspace ./ComfyUI install --fast-deps ...
error: failed to remove file `...\venv\Lib\site-packages\pydantic_core/_pydantic_core.cp312-win_amd64.pyd`: Access is denied. (os error 5)
→ ImportError: cannot import name '__version__' from 'pydantic_core'

This is a Windows file-lock race: comfy install --fast-deps (uv) tries to reinstall pydantic-core for ComfyUI and can't remove the already-loaded .pyd. It reproduced identically on re-run and is environmental / pre-existing — it hits main equally.

This diff cannot cause it: the failing step is pip install -e . + comfy install, which install from pyproject.toml, not uv.lock; and the added anthropic dep is an optional [bench] extra that isn't installed on this path. (The uv.lock change here just re-syncs main's stale lockfile, which declares anthropic>=0.40 but never locked it.)

All review threads are resolved, Cursor/CodeRabbit findings addressed, local suite green (218 passed), and CI-pinned ruff 0.15.15 clean. mergeStateStatus is UNSTABLE (non-required check), so this red X does not block merge.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Reviews pass — closing the loop:

  • CodeRabbit (outside-diff nit on `test_run_cli.py`, "assert the exact cloud command, not only the title"): already implemented in commit e828256 as `test_whoami_step_targets_cloud_not_auth`, which asserts the whoami demo step's invocation argv/labels contain `cloud whoami` and exclude `auth whoami`. No change needed.
  • Cursor Medium (demo step still invoked `comfy auth whoami`): resolved in 54e3713.

Verified redirect targets are real: `comfy cloud login` (comfy_cli/cloud/command.py:42) and `comfy cloud whoami` (:149) are both registered commands — no dead-end. `git grep` confirms zero remaining user-facing `auth login`/`auth whoami` refs (only the negative regression guards remain).

On the one failing checkWindows Specific Commands: this is a pre-existing, repo-wide infra failure (a `pydantic_core version` ImportError during dependency install) that is currently failing on every open PR and hasn't been green on main since March 2026. It is unrelated to these string-only edits — the PR is MERGEABLE/UNSTABLE and this check is non-blocking. Fixing it belongs in a separate infra ticket.

Local run: 218 tests pass; ruff clean on changed lines; `uv lock --check` passes.

This PR is scoped to routing user-facing `comfy auth login`/`auth whoami`
hints at the real `comfy cloud login`/`cloud whoami` commands. It had also
picked up a 287-line `uv.lock` re-sync that locks `anthropic`, `pydantic`,
and `pydantic-core` — unrelated to the hint strings and not load-bearing
for any test here.

main's lockfile is genuinely stale (`uv lock --check` fails on main: the
`bench` extra declares `anthropic>=0.40` but was never locked), so the
re-sync is real work — just not this PR's. No CI job consumes `uv.lock`
(ci-cursor-review explicitly excludes it), so reverting restores main's
status quo with no regression. Tracked as a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Resolver pass — scoped the diff down, and a correction on the Windows check.

Removed an unrelated uv.lock re-sync (605d31f)

This PR had picked up a 287-line uv.lock diff that locks anthropic 0.116.0, pydantic, and pydantic-core — unrelated to the hint strings, and not load-bearing for anything here. It would have landed a dependency change under a PR titled "fix auth hints", where no reviewer is evaluating it as one. Reverted to main's lockfile; the diff is now 9 files / +39−21, purely the hint fix plus its regression guards.

To be clear, the re-sync is real work — just not this PR's. main's lockfile is genuinely stale:

$ uv lock --check          # on a clean main checkout
The lockfile at `uv.lock` needs to be updated, but `--check` was provided.

pyproject.toml:59 declares optional-dependencies.bench = [ "anthropic>=0.40" ] but it was never locked, so any uv sync silently regenerates the lock into whatever branch you're on — which is how it got here. No CI job consumes uv.lock (ci-cursor-review.yml:48 excludes it), so reverting restores main's status quo with no regression. Filed as a follow-up to fix on its own branch where it's reviewable.

Correction: the failing Windows check is a real bug, not "environmental"

An earlier comment of mine called it environmental and unfixable. That was wrong, and worth correcting: it's a genuine product bug with a dedicated open PR — #535 (fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289)). The eager tracking.pymixpanelpydantic import at CLI startup holds _pydantic_core.*.pyd open, so comfy install --fast-deps can't replace it on Windows (Access is denied. (os error 5)ImportError: cannot import name '__version__' from 'pydantic_core').

Evidence it's independent of this diff: Windows Specific Commands is red across essentially every branch today, and the only green runs are on matt/be-3289-lazy-telemetry-import — the branch that removes that exact import chain. Still out of scope for a string-only PR; it unblocks via #535.

Verification

  • Full suite on this branch: 2565 passed. The 9 failures (test_config_parser.py ×8, test_node_init.py ×1) reproduce identically on origin/main and are local-env artifacts — both files are untouched by this diff and both pass in CI's Linux/macOS matrices.
  • PR-touched areas (test_branding, test_run_cli, auth/, cloud/): 198 passed.
  • ruff check + ruff format --check: clean (235 files).
  • Redirect targets confirmed real, not dead-ends: @app.command("login") at comfy_cli/cloud/command.py:40 and @app.command("whoami") at :147, both registered under the cloud group; comfy auth (cmdline.py:1638) is the model-host token group only. git grep shows zero remaining user-facing auth login/auth whoami refs — only the negative regression guards.

All review threads addressed. MERGEABLE.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Correction to my previous comment, and the follow-up is already covered.

I was wrong that the 9 failures are "local-env artifacts"

I claimed those 9 tests "pass in CI's Linux/macOS matrices". CI has since falsified that — the build job now fails with exactly the same 9 (test_config_parser.py ×8 + test_node_init.py ×1, ValueError: Comment cannot contain line breaks). They are real, not environmental.

They are still not caused by this PR, and specifically not by my uv.lock revert. The cause is tomlkit 0.15: pyproject.toml:49 pins tomlkit as bare "tomlkit" (unpinned), and CI installs from pyproject.toml rather than the lockfile, so it now resolves 0.15.1, whose comment handling rejects the multi-line hint blocks the registry config parser emits. Evidence it isn't the revert:

  • tomlkit is locked at 0.13.3 identically in origin/main, in the pre-revert commit e828256, and in my post-revert 605d31f — the revert did not move it.
  • No CI workflow consumes uv.lock at all, so the lockfile can't influence what CI resolves.
  • The same 9 failures reproduce on a clean origin/main checkout locally.
  • build was green at 05:06 and red at 11:57 on the same code path — a time-based dependency release, not a diff.

This is already tracked by #536 (fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15) and #533. #537 (ci: install test dependencies from uv.lock instead of resolving fresh) would prevent this class of drift.

No follow-up filed for the uv.lock re-sync — it already exists

I was about to file one, but the work is already open in #534 (build(deps): re-sync uv.lock with the pyproject bench extra (BE-3291), uv.lock-only), plus #543 and #545. That makes dropping the lock diff from this PR the right call regardless of scope: had it stayed, #512 would have collided with #534 over the same 287 lines.

Net state of this PR

The diff is now 9 files / +39−21, string fixes plus regression guards only. Every red check is a pre-existing repo-wide breakage with its own dedicated PR — test (Windows) → #535, build#536/#533 — and none of them touch the files this PR changes. MERGEABLE, all review threads resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant