fix: add file-based fallback to check_comfy_repo for non-git installs#401
Merged
fix: add file-based fallback to check_comfy_repo for non-git installs#401
Conversation
📝 WalkthroughWalkthroughRefactors repository detection to return resolved filesystem paths (str) instead of git.Repo objects and adds marker-file-based detection for non-git/portable ComfyUI installs. CLI callers now accept and use resolved path strings. A little rhyme: "If git won't stick, markers do the trick." Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (comfy)"
participant WM as "workspace_manager"
participant FS as "Filesystem"
participant GIT as "git.Repo"
CLI->>WM: check_comfy_repo(path)
WM->>GIT: try git.Repo(parent_of_path)
alt Git repo valid and ComfyUI markers exist
GIT-->>WM: git.Repo (valid)
WM->>FS: resolve repo.working_dir -> resolved_path
WM-->>CLI: (True, resolved_path)
else Git invalid or no comfy remote
GIT--x WM: raises InvalidGitRepositoryError / no comfy remote
WM->>FS: _find_comfyui_root(path) (walk up, check markers)
alt markers found (threshold met)
FS-->>WM: marker_root (absolute path)
WM-->>CLI: (True, marker_root)
else markers not found
FS-->>WM: None
WM-->>CLI: (False, None)
end
end
🚥 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❌ Patch coverage is
@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 75.59% 75.66% +0.07%
==========================================
Files 35 35
Lines 4127 4143 +16
==========================================
+ Hits 3120 3135 +15
- Misses 1007 1008 +1
🚀 New features to boost your workflow:
|
dosubot
bot
added
the
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/workspace_manager.py (1)
79-86:⚠️ Potential issue | 🟡 MinorHarden
custom_nodesparent derivation to avoid empty-path git resolution.When
parentbecomes"",git.Repo(parent, ...)resolves from current working directory, which can produce false positives.🔧 Proposed guard
if not path_is_comfy_repo and "custom_nodes" in path: parts = path.split(os.sep) try: index = parts.index("custom_nodes") - parent = os.sep.join(parts[:index]) + parent = os.sep.join(parts[:index]) or os.sep repo = git.Repo(parent, search_parent_directories=True) path_is_comfy_repo = any(remote.url in constants.COMFY_ORIGIN_URL_CHOICES for remote in repo.remotes) except ValueError: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/workspace_manager.py` around lines 79 - 86, The parent path derived for "custom_nodes" can be empty which lets git.Repo("") resolve from CWD and give false positives; before calling git.Repo(parent, search_parent_directories=True) in workspace_manager (where path, parts, index, parent, repo and path_is_comfy_repo are used), add a guard to skip git.Repo when parent is empty or not an absolute/valid directory (e.g., check if parent and os.path.isdir(parent) or normalize with os.path.abspath(parent) and only then construct repo and evaluate remotes), otherwise leave path_is_comfy_repo False.tests/comfy_cli/test_workspace_manager.py (1)
213-224: 🛠️ Refactor suggestion | 🟠 MajorAdd regression tests for marker fallback paths (non-git root + non-git subdir).
This PR’s core behavior is marker fallback, but the suite still mostly mocks
check_comfy_repo. Add direct tests that exercise real filesystem markers so future changes don’t quietly break this fix. A tiny test now avoids a big oops later.🧪 Suggested test additions
+def test_check_comfy_repo_non_git_marker_root(tmp_path): + from comfy_cli.workspace_manager import check_comfy_repo + root = tmp_path / "ComfyUI" + root.mkdir() + (root / "main.py").write_text("#") + (root / "nodes.py").write_text("#") + (root / "comfy").mkdir() + ok, resolved = check_comfy_repo(str(root)) + assert ok is True + assert resolved == str(root) + +def test_check_comfy_repo_non_git_marker_subdir(tmp_path): + from comfy_cli.workspace_manager import check_comfy_repo + root = tmp_path / "ComfyUI" + sub = root / "custom_nodes" / "x" + sub.mkdir(parents=True) + (root / "main.py").write_text("#") + (root / "nodes.py").write_text("#") + (root / "comfy_extras").mkdir() + ok, resolved = check_comfy_repo(str(sub)) + assert ok is True + assert resolved == str(root)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/comfy_cli/test_workspace_manager.py` around lines 213 - 224, Add regression tests that exercise real filesystem marker fallback instead of mocking check_comfy_repo: create a temporary directory structure with a marker file (e.g., a Comfy marker used by workspace_manager like ".comfy-repo" or the repository marker your code checks), create a subdirectory (non-git subdir), monkeypatch os.getcwd to the subdir, instantiate the manager with _make_manager(use_here=None) and _mock_config as needed, call mgr.get_workspace_path() without patching check_comfy_repo/_paths_match, and assert the returned path matches the marker-rooted resolved path; reference helpers _make_manager, get_workspace_path, and check_comfy_repo/_paths_match to locate where to integrate the new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/workspace_manager.py`:
- Around line 97-103: The fallback detection only checks the provided absolute
path (abs_path) for ComfyUI markers via _has_comfyui_markers, so paths that are
inside a ComfyUI tree (e.g. custom_nodes/...) are missed; update the logic to
walk up parent directories from abs_path until filesystem root, calling
_has_comfyui_markers on each parent and returning (True, root_path) when found
(otherwise return (False, None)); reference the existing abs_path variable and
_has_comfyui_markers function to implement the upward traversal and return the
detected ComfyUI root.
---
Outside diff comments:
In `@comfy_cli/workspace_manager.py`:
- Around line 79-86: The parent path derived for "custom_nodes" can be empty
which lets git.Repo("") resolve from CWD and give false positives; before
calling git.Repo(parent, search_parent_directories=True) in workspace_manager
(where path, parts, index, parent, repo and path_is_comfy_repo are used), add a
guard to skip git.Repo when parent is empty or not an absolute/valid directory
(e.g., check if parent and os.path.isdir(parent) or normalize with
os.path.abspath(parent) and only then construct repo and evaluate remotes),
otherwise leave path_is_comfy_repo False.
In `@tests/comfy_cli/test_workspace_manager.py`:
- Around line 213-224: Add regression tests that exercise real filesystem marker
fallback instead of mocking check_comfy_repo: create a temporary directory
structure with a marker file (e.g., a Comfy marker used by workspace_manager
like ".comfy-repo" or the repository marker your code checks), create a
subdirectory (non-git subdir), monkeypatch os.getcwd to the subdir, instantiate
the manager with _make_manager(use_here=None) and _mock_config as needed, call
mgr.get_workspace_path() without patching check_comfy_repo/_paths_match, and
assert the returned path matches the marker-rooted resolved path; reference
helpers _make_manager, get_workspace_path, and check_comfy_repo/_paths_match to
locate where to integrate the new tests.
🪄 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: 8d66d251-f2cc-4df5-9469-6de0e3d3b45b
📒 Files selected for processing (3)
comfy_cli/cmdline.pycomfy_cli/workspace_manager.pytests/comfy_cli/test_workspace_manager.py
- Add _find_comfyui_root() that walks up parent directories looking for ComfyUI markers, mirroring git's search_parent_directories behavior for non-git installs (zip downloads, forks, portable builds). - Add comfy_api to marker list, bump threshold from 3/4 to 4/5 to reduce false positives. - Add comprehensive tests for _has_comfyui_markers, _find_comfyui_root, check_comfy_repo fallback paths, and non-git integration scenarios.
bigcat88
force-pushed
the
fix/check-comfy-repo-fallback-markers
branch
from
0f5a1be to
3d210d0
Compare
dosubot
bot
added
size:L
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy_cli/workspace_manager.py (1)
93-102:⚠️ Potential issue | 🟡 MinorEdge case: path starting with
custom_nodesproduces empty parent.If
pathis a relative path likecustom_nodes/MyNode, thenindex=0andparts[:0]yields an empty list, makingparentan empty string. Passing an empty string togit.Repo()may produce unexpected behavior (it would search from current working directory).This is likely a rare edge case, but worth considering. The marker-based fallback might recover gracefully, but the git call could still produce confusing errors.
🛡️ Proposed defensive fix
try: index = parts.index("custom_nodes") - parent = os.sep.join(parts[:index]) + parent = os.sep.join(parts[:index]) or "." repo = git.Repo(parent, search_parent_directories=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/workspace_manager.py` around lines 93 - 102, If path_is_comfy_repo is False and "custom_nodes" is at the start of a relative path (e.g., "custom_nodes/MyNode"), parts.index("custom_nodes") will be 0 and parent becomes an empty string which should not be passed to git.Repo; update the block around parts = path.split(os.sep) to guard before calling git.Repo(parent, ...): after computing index and parent, check that parent is truthy (non-empty) and/or os.path.exists(parent) and only then attempt git.Repo; if parent is empty or does not exist, skip the git lookup and leave path_is_comfy_repo False (or fallback to the existing marker-based logic). Ensure this change references the same symbols: path, parts, index, parent, git.Repo, repo.remotes, and constants.COMFY_ORIGIN_URL_CHOICES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@comfy_cli/workspace_manager.py`:
- Around line 93-102: If path_is_comfy_repo is False and "custom_nodes" is at
the start of a relative path (e.g., "custom_nodes/MyNode"),
parts.index("custom_nodes") will be 0 and parent becomes an empty string which
should not be passed to git.Repo; update the block around parts =
path.split(os.sep) to guard before calling git.Repo(parent, ...): after
computing index and parent, check that parent is truthy (non-empty) and/or
os.path.exists(parent) and only then attempt git.Repo; if parent is empty or
does not exist, skip the git lookup and leave path_is_comfy_repo False (or
fallback to the existing marker-based logic). Ensure this change references the
same symbols: path, parts, index, parent, git.Repo, repo.remotes, and
constants.COMFY_ORIGIN_URL_CHOICES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc361d6d-abd7-4e37-a571-0136655c7589
📒 Files selected for processing (3)
comfy_cli/cmdline.pycomfy_cli/workspace_manager.pytests/comfy_cli/test_workspace_manager.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.
Fixes #205
check_comfy_repo()only validated ComfyUI installations by matching git remote URLs against a hardcoded allowlist. This causedset-defaultand workspace detection to reject perfectly valid ComfyUI installations that were set up via zip download, the Windows portable build, or cloned from a fork/mirror with a non-listed remote URL.This adds a file-based fallback that checks for ComfyUI marker files (
main.py,comfy/,nodes.py,comfy_extras/— at least 3 of 4 must be present) when the git-based check fails. The git check still runs first, so existing behavior is preserved for standard clones.The return type of
check_comfy_repo()changes fromtuple[bool, git.Repo | None]totuple[bool, str | None], returning the resolved path string directly instead of a repo object. All callers only usedrepo.working_diranyway, so this is a straightforward simplification.