Skip to content

refactor(install): dedupe manager-install fallback and normalize exit conventions (BE-4356)#594

Open
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-4356-dedupe-manager-fallback
Open

refactor(install): dedupe manager-install fallback and normalize exit conventions (BE-4356)#594
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-4356-dedupe-manager-fallback

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

When you run comfy install, the tool installs ComfyUI-Manager and, if that
install fails, quietly turns the manager off so a later launch doesn't try to
use something that isn't there. That little "install-or-degrade" dance was
written out twice — once for the normal pip path and once for the
--fast-deps path — and the two copies had already started to drift. This PR
folds them into one shared helper so they can't drift again.

While in the file, it also makes the module exit the way the rest of the
codebase does: raise typer.Exit(code=1) instead of sys.exit(...).

What changed

  1. Dedupe the manager-install fallback. New module-private helper
    _install_manager_with_fallback(repo_dir, python, *, bootstrap_pip: bool)
    holds the single copy of "install the manager; on failure disable the
    manager GUI mode and warn." Both call sites in execute() (the pip path and
    the fast_deps path) now call it. The only behavioral difference between the
    old copies — the fast_deps path calls ensure_pip(python) first because
    its uv-managed venv may ship no pip, while the pip path already bootstrapped
    pip earlier — is preserved via the bootstrap_pip flag (False for the pip
    path, True for fast_deps). Pure code motion; no behavior change.

  2. Normalize exit conventions. The module's six sys.exit(...) calls are
    now raise typer.Exit(code=1), matching the convention used broadly
    elsewhere in the codebase. All of these already propagate straight up to
    main(), which re-raises both typer.Exit and SystemExit identically, so
    the exit code is unchanged (1) for five of them.

⚠️ One intentional observable change

sys.exit(-1) on the "path exists but isn't a recognized ComfyUI repo" branch
previously produced shell exit code 255 (a negative arg wraps to 255); it
now produces 1, like every other failure in the module. This is the only
user-visible change and is a deliberate normalization.

Notes / judgment calls

  • Deferred as ticketed: the full _install_deps_pip / _install_deps_fast
    two-strategy extraction is intentionally not done here — install is the
    highest-blast-radius command and the groom verdict downgraded scope to just
    the concrete dedupe + exit normalization. Defer that split until install next
    changes for feature reasons.
  • Test update: typer.Exit is not a subclass of SystemExit, so the
    one existing test asserting pytest.raises(SystemExit) on
    checkout_stable_comfyui was updated to pytest.raises(typer.Exit). I
    verified none of the six exit sites sit inside a broad except Exception: in
    their call path (which would newly swallow a typer.Exit where SystemExit
    escaped) — they all propagate to main() unchanged.
  • New coverage: added TestInstallManagerWithFallback locking the helper's
    bootstrap-vs-not behavior and the degrade-on-failure config write.

Testing

  • Full unit suite green: 2670 passed, 13 skipped (uv run pytest tests/comfy_cli).
  • ruff check + ruff format --check clean on the touched files.

… conventions (BE-4356)

Extract the twice-duplicated manager-install-and-degrade sequence into a
single helper _install_manager_with_fallback(repo_dir, python, *,
bootstrap_pip: bool), called from both the pip and fast_deps paths of
execute(). The only prior difference between the two copies — the fast_deps
path bootstrapping pip first — is preserved via the bootstrap_pip flag.

Normalize the module's sys.exit(1)/sys.exit(-1) calls to raise
typer.Exit(code=1), matching the convention used broadly elsewhere. Note
sys.exit(-1) previously yielded shell exit code 255; it now yields 1 (a
minor observable change on the invalid-ComfyUI-repo path).
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5cbc462c-32c0-4528-ac82-bb20c8941851

📥 Commits

Reviewing files that changed from the base of the PR and between 85b62da and 6ad55d7.

📒 Files selected for processing (3)
  • comfy_cli/command/install.py
  • tests/comfy_cli/command/github/test_pr.py
  • tests/comfy_cli/test_install.py

📝 Walkthrough

Walkthrough

Installation failures now consistently raise typer.Exit, while ComfyUI-Manager setup uses a shared fallback that can disable manager GUI mode after installation failure. Tests cover both behaviors.

Changes

Install reliability

Layer / File(s) Summary
ComfyUI-Manager fallback installation
comfy_cli/command/install.py, tests/comfy_cli/test_install.py
Manager setup is centralized in a helper supporting optional pip bootstrapping, installation, GUI-mode disabling on failure, and branch coverage.
Typer exit handling
comfy_cli/command/install.py, tests/comfy_cli/command/github/test_pr.py
Dependency, repository, release, and checkout failures raise typer.Exit(code=1); the related test expects typer.Exit.

Sequence Diagram(s)

sequenceDiagram
  participant InstallCommand
  participant ManagerFallback
  participant Pip
  participant ConfigManager
  InstallCommand->>ManagerFallback: install ComfyUI-Manager
  ManagerFallback->>Pip: bootstrap pip when enabled
  ManagerFallback->>Pip: install manager
  Pip-->>ManagerFallback: success or failure
  ManagerFallback->>ConfigManager: disable manager GUI mode on failure
Loading

Suggested reviewers: bigcat88

🚥 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-4356-dedupe-manager-fallback
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-4356-dedupe-manager-fallback

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

@mattmillerai
mattmillerai marked this pull request as ready for review July 24, 2026 07:02
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 24, 2026
@coderabbitai
coderabbitai Bot requested a review from bigcat88 July 24, 2026 07:03

@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.

✅ No high-signal findings.

Panel: 6/8 reviewers contributed findings.

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

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 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