Skip to content

refactor: remove 8 dead symbols + vestigial where.py re-export (BE-3268)#525

Open
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3268-dead-code-cleanup
Open

refactor: remove 8 dead symbols + vestigial where.py re-export (BE-3268)#525
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3268-dead-code-cleanup

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

There were 8 functions/classes in the codebase that nothing ever called, plus one leftover "re-export" line — like a light switch wired to nothing. This PR deletes them. Because nothing referenced them, nothing changes at runtime; the code just gets smaller and less confusing. Matches the merged precedent of #499/#504.

What & why

Deletes 8 zero-reference symbols plus a vestigial where.py re-export line. Each was verified dead by full-repo grep (definition-only hits — no decorators, no string/dynamic references, no __all__ exports):

Symbol File
get_version_from_pyproject comfy_cli/update.py
install_conda_package comfy_cli/utils.py
known_providers comfy_cli/auth/store.py (1-line wrapper; real callers use SUPPORTED_PROVIDERS directly)
get_os_details comfy_cli/command/install.py
prompt_autocomplete comfy_cli/ui.py
active_spec_path comfy_cli/command/generate/spec.py (unused wrapper over _select_spec_path)
validate_comfyui comfy_cli/cmdline.py (undecorated — not a typer callback; distinct from custom_nodes validate_comfyui_manager)
OAuthCancelled comfy_cli/cloud/oauth.py (never raised or caught anywhere)
get_token re-export comfy_cli/where.py:27 (all callers use comfy_cli.cancellation directly)

Coupled cleanups (kept the tree honest / green)

  • Orphaned imports removed as a direct consequence of the deletions (ruff-enforced): metadata (update.py), Iterable (store.py), and subprocess / typer / the rprint as print shim (utils.py — all three were used only by install_conda_package).
  • oauth_cancelled registry entry removed from comfy_cli/error_codes.py. The OAuthCancelled class attribute code = "oauth_cancelled" was the only source that made that code count as "raised"; deleting the class left it dead, and the repo's own test_every_registered_code_is_raised requires dead entries be deleted. This also drops it from the comfy --json discover contract — safe, because the code was never actually raised, so no consumer could ever have received it.

Judgment calls / flags

  • OAuthCancelled (flagged per ticket): never raised or caught today. If a branch in flight plans to use it for user-abort during login, it (and the oauth_cancelled error code) can be re-added at that point.
  • Two deleted symbols live in auth modules (known_providers, OAuthCancelled), but deleting unreferenced code changes no auth behavior.

Testing

  • ruff check + ruff format --check: clean.
  • Full pytest suite: 2573 passed, 37 skipped.
  • Zero behavioral risk by construction (no references).

Delete zero-reference symbols confirmed dead by full-repo grep (no
decorators, no dynamic/string refs, no __all__ exports):

- update.py: get_version_from_pyproject (+ orphaned metadata import)
- utils.py: install_conda_package (+ orphaned subprocess/typer/print-shim imports)
- auth/store.py: known_providers (+ orphaned Iterable import); callers use
  SUPPORTED_PROVIDERS directly
- command/install.py: get_os_details
- ui.py: prompt_autocomplete
- command/generate/spec.py: active_spec_path (unused wrapper over _select_spec_path)
- cmdline.py: validate_comfyui (undecorated; distinct from custom_nodes validate_comfyui_manager)
- cloud/oauth.py: OAuthCancelled (never raised or caught)
- where.py: vestigial get_token re-export line

Also removes the coupled oauth_cancelled entry from error_codes.REGISTRY:
the OAuthCancelled class attribute was the only source of that code, so
deleting the class made it dead and the registry's own
test_every_registered_code_is_raised requires it gone.
@mattmillerai mattmillerai added the agent-coded PR authored by the agent-work loop label Jul 17, 2026
@mattmillerai
mattmillerai marked this pull request as ready for review July 17, 2026 05:31
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

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: 57 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: d8e3e721-d3de-41c7-bccc-0fd7f4089cde

📥 Commits

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

📒 Files selected for processing (10)
  • comfy_cli/auth/store.py
  • comfy_cli/cloud/oauth.py
  • comfy_cli/cmdline.py
  • comfy_cli/command/generate/spec.py
  • comfy_cli/command/install.py
  • comfy_cli/error_codes.py
  • comfy_cli/ui.py
  • comfy_cli/update.py
  • comfy_cli/utils.py
  • comfy_cli/where.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3268-dead-code-cleanup
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3268-dead-code-cleanup

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

@mattmillerai mattmillerai added the cursor-review Request Cursor bot review label Jul 17, 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.

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

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Resolver pass: no code changes needed — both red checks are pre-existing repo-wide breaks

Went through this PR for conflicts, CI, and review feedback. Summary: nothing actionable on the diff itself; the two failures are upstream/infra problems that also break main.

Merge conflict

None — mergeable: MERGEABLE against main.

Review feedback

No unresolved review threads. Cursor consolidated panel: "✅ No high-signal findings" (6/8 reviewers contributed). CodeRabbit: skipped (no matching review label).

build — pre-existing, caused by an upstream tomlkit release

9 failures, all ValueError: Comment cannot contain line breaks, raised from tomlkit/items.py:504 inside comfy_cli/registry/config_parser.pya file this PR does not touch.

Root cause: tomlkit is unpinned (pyproject.toml:49), and CI now resolves tomlkit 0.15.1, which forbids \n in .comment(). Two call sites pass multi-line hint blocks (config_parser.py:75 and :250).

Reproduced on a clean origin/main worktree, with no part of this PR present:

$ uv run --extra dev --with 'tomlkit==0.15.1' pytest tests/comfy_cli/registry/test_config_parser.py -q
8 failed, 73 passed

With the locally-resolved tomlkit 0.13.3, the same tests pass (81 passed). So: upstream dependency regression, not this diff.

test (Windows Specific Commands) — pre-existing infra failure

Fails on 10 of the last 10 runs across many unrelated branches. It dies in Install Dependencies, not in any test:

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' (unknown location)

comfy install shells out to uv pip install against the same venv the running interpreter is loaded from; Windows locks the loaded .pyd, the partial removal corrupts the venv, and the next comfy call fails at import.

Verification of this PR

  • Full suite in an isolated worktree: 2573 passed, 37 skipped — matches the PR description.
  • ruff check / ruff format --check: clean in CI (ruff_check ✅).
  • Re-verified every deleted symbol still has zero references repo-wide (incl. non-Python files). The only validate_comfyui hits are validate_comfyui_manager, a genuinely distinct symbol, as described.
  • The oauth_cancelled registry deletion is required by the repo's own test_every_registered_code_is_raised, which mandates dead entries be removed.

Follow-ups proposed

Both CI breaks are out of scope here (this is a pure dead-code deletion) and block many branches at once, so they should land independently on main rather than be conflated into this diff. I've recorded both for follow-up tickets: the tomlkit pin/rework (well-specified) and the Windows workflow venv-locking issue (needs a workflow-design decision first).

This PR is ready to merge on its own merits once the tomlkit break is fixed on main.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Resolver re-check: still no changes needed — and both CI breaks now have fixes in flight

Re-verified this PR from scratch in an isolated worktree. Conclusion is unchanged: the diff is clean and nothing here is actionable. Updating my earlier comment with one new fact — the two red checks are already being fixed by other PRs, so no follow-up tickets are needed.

Verification of this diff (re-run, not carried over)

  • Full suite: 2573 passed, 37 skipped — matches the PR description.
  • Re-grepped every deleted symbol repo-wide: get_version_from_pyproject, install_conda_package, known_providers, get_os_details, prompt_autocomplete, active_spec_path, OAuthCancelled, oauth_cancelled0 hits each. A word-boundary search for validate_comfyui also returns 0, confirming validate_comfyui_manager is a genuinely distinct symbol and is untouched.
  • No merge conflict (MERGEABLE). No unresolved review threads. Cursor panel: no high-signal findings.

Both red checks are pre-existing and reproduce on PRs that don't touch the code

Confirmed identical failures on #543, #544, #545, #547.

build — upstream tomlkit 0.15.1 forbids \n in .comment(); tomlkit is unpinned at pyproject.toml:49. Reproduced against an unmodified tree:

--with 'tomlkit==0.15.1'  -> 8 failed, 73 passed
lockfile tomlkit 0.13.3   -> 81 passed

Already addressed by → #536 (BE-3290) / #533 (BE-3285), with #537 (BE-3292) pinning CI to uv.lock.

test (Windows) — dies in Install Dependencies, before any test: comfy install shells out to uv pip install against the venv its own interpreter is loaded from, Windows locks the in-use _pydantic_core\*.pyd, and the partial removal corrupts the venv. Already addressed by → #535 (BE-3289) / #529 (UV_LINK_MODE=copy).

Net

No follow-ups filed — filing any would duplicate the five PRs above. This PR needs no changes; it should go green on its own once the tomlkit fix lands on main and this branch is updated.

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