fix: handle missing cm-cli gracefully in comfy update#419
Merged
Conversation
Wrap update_node_id_cache() call in cmdline.update() with try/except, matching the pattern already used in install.py. ComfyUI-Manager is optional, so its absence should not crash the entire update command.
dosubot
bot
added
the
size:S
📝 WalkthroughWalkthroughThe Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #419 +/- ##
==========================================
+ Coverage 76.05% 76.07% +0.01%
==========================================
Files 35 35
Lines 4210 4213 +3
==========================================
+ Hits 3202 3205 +3
Misses 1008 1008
🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_cmdline_python_resolution.py`:
- Around line 28-40: Add a sibling test mirroring
test_update_comfy_succeeds_when_cm_cli_missing that asserts
cmdline.update(target="comfy") does not raise when a
subprocess.CalledProcessError occurs; copy the same patches
(patch("comfy_cli.cmdline.resolve_workspace_python", ...),
patch.object(cmdline.workspace_manager, "workspace_path", ...),
patch("comfy_cli.cmdline.os.chdir"), patch("comfy_cli.cmdline.subprocess.run"))
and instead of raising FileNotFoundError from
comfy_cli.cmdline.custom_nodes.command.update_node_id_cache, set that patch to
raise subprocess.CalledProcessError(returncode=1, cmd="cm-cli") so the
CalledProcessError branch in cmdline.update (function cmdline.update and the
update_node_id_cache call) is exercised.
- Around line 30-40: The test currently patches
"comfy_cli.cmdline.custom_nodes.command.update_node_id_cache" but never asserts
it was invoked, so add an assertion to ensure the guarded branch is exercised:
bind the patch to a mock variable (e.g., mock_update = patch(...).start() or use
as mock_update in the with-statement), run cmdline.update(target="comfy"), then
assert mock_update.assert_called_once() (or
mock_update.assert_called_once_with() if you want to verify arguments/exception
behavior) to guarantee update_node_id_cache was invoked.
🪄 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
Run ID: ffad5b70-288c-47a9-a6f7-dae8b2231bc7
📒 Files selected for processing (2)
comfy_cli/cmdline.pytests/comfy_cli/test_cmdline_python_resolution.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/comfy_cli/test_cmdline_python_resolution.py (1)
28-42: 🧹 Nitpick | 🔵 TrivialAdd a sibling regression test for the
CalledProcessErrorbranch.Nice guard on Line 28–Line 42 for missing
cm-cli; however, ifcmdline.update()also catchessubprocess.CalledProcessError, that branch still needs a twin test to keep coverage tight (two catches, two matches). Mirror this test withside_effect=subprocess.CalledProcessError(returncode=1, cmd="cm-cli")and assert no exception plusassert_called_once().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/comfy_cli/test_cmdline_python_resolution.py` around lines 28 - 42, Add a sibling regression test to mirror test_update_comfy_succeeds_when_cm_cli_missing that exercises the subprocess.CalledProcessError branch: create a new test (e.g., test_update_comfy_succeeds_when_cm_cli_raises_calledprocesserror) that patches resolve_workspace_python, cmdline.workspace_manager.workspace_path, os.chdir, subprocess.run, and patches comfy_cli.cmdline.custom_nodes.command.update_node_id_cache to raise subprocess.CalledProcessError(returncode=1, cmd="cm-cli") as side_effect, then call cmdline.update(target="comfy") and assert the mocked update_node_id_cache was called once (mock.assert_called_once()) to ensure the CalledProcessError path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/comfy_cli/test_cmdline_python_resolution.py`:
- Around line 28-42: Add a sibling regression test to mirror
test_update_comfy_succeeds_when_cm_cli_missing that exercises the
subprocess.CalledProcessError branch: create a new test (e.g.,
test_update_comfy_succeeds_when_cm_cli_raises_calledprocesserror) that patches
resolve_workspace_python, cmdline.workspace_manager.workspace_path, os.chdir,
subprocess.run, and patches
comfy_cli.cmdline.custom_nodes.command.update_node_id_cache to raise
subprocess.CalledProcessError(returncode=1, cmd="cm-cli") as side_effect, then
call cmdline.update(target="comfy") and assert the mocked update_node_id_cache
was called once (mock.assert_called_once()) to ensure the CalledProcessError
path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bb2b2df-e136-4527-8aee-a0ca89a1f68a
📒 Files selected for processing (1)
tests/comfy_cli/test_cmdline_python_resolution.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #403
Summary
comfy update comfycrashes withFileNotFoundErrorwhen ComfyUI-Manager (cm-cli) is not installed, even though the core update (git pull + pip install) already succeeded.update_node_id_cache()incmdline.pywith a try/except, matching the pattern already used ininstall.py(line 290-294). The function itself stays strict — callers that know cm-cli is optional handle it at the call site.Test plan
test_update_comfy_succeeds_when_cm_cli_missing— mocksupdate_node_id_cacheto raiseFileNotFoundErrorand assertscmdline.update()completes without crashingruff check+ruff format --check)