feat: add comfy code-search (alias comfy cs) subcommand#386
Merged
Conversation
dosubot
bot
added
size:XL
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #386 +/- ##
==========================================
+ Coverage 75.00% 75.59% +0.59%
==========================================
Files 34 35 +1
Lines 4028 4127 +99
==========================================
+ Hits 3021 3120 +99
Misses 1007 1007
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new comfy code-search CLI subcommand (with hidden alias comfy cs) that queries the comfy-codesearch (Sourcegraph-backed) API and prints either human-friendly Rich output or JSON for machine consumption.
Changes:
- Introduce
comfy_cli/command/code_search.pyimplementing query building, API fetch, result normalization, stats extraction, and output rendering. - Register the new
code-searchandcscommands in the main CLI app. - Add a comprehensive new unit test module covering query building, parsing, error handling, and Typer CLI invocation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
comfy_cli/command/code_search.py |
New Typer subcommand for code search; includes API interaction, formatting, and error handling. |
comfy_cli/cmdline.py |
Wires the new subcommand into the top-level CLI (plus hidden alias). |
tests/comfy_cli/command/test_code_search.py |
New test suite validating query construction, formatting/stats parsing, error handling, and CLI behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
snomiao
requested review from
bigcat88,
christian-byrne,
Copilot and
robinjhuang
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bigcat88
reviewed
Mar 29, 2026
Contributor
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
I noticed while testing the --repo filter does substring matching rather than exact matching because Sourcegraph's repo: uses regex
For example comfy code-search --repo ComfyUI "SaveImage" returns results from ComfyUI, ComfyUI-Mirror and ComfyUI_frontend
I think most users would expect --repo ComfyUI to only search in ComfyUI
dosubot
bot
added
size:XXL
Member
Author
|
Fixed in 4b7baca — |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds NVIDIA CUDA driver auto-detection and PyTorch wheel-tag resolution; migrates ComfyUI-Manager installs from git-clone to pip-based installs; introduces unified dependency-resolution passthrough ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CmdLine as comfy_cli.cmdline
participant Resolver as _resolve_cuda
participant Detector as comfy_cli.cuda_detect
participant Installer as comfy_cli.command.install.execute
participant PipDeps as pip_install_comfyui_dependencies
participant Torch as pip (torch wheels)
User->>CmdLine: comfy install [--nvidia] [--cuda-version]
CmdLine->>Resolver: _resolve_cuda(gpu, cuda_version)
alt explicit cuda_version provided
Resolver-->>CmdLine: (cuda_version, cuda_tag)
else auto-detect
Resolver->>Detector: detect_cuda_driver_version()
Detector->>Detector: try ctypes (cuInit/cuDriverGetVersion)
alt ctypes success
Detector-->>Resolver: (major, minor)
else ctypes fails
Detector->>Detector: run nvidia-smi parse
Detector-->>Resolver: (major, minor) or None
end
Resolver->>Detector: resolve_cuda_wheel((major,minor))
Detector-->>Resolver: cuda_tag or None
Resolver-->>CmdLine: (cuda_version, cuda_tag)
end
CmdLine->>Installer: execute(..., cuda_tag=cuda_tag)
Installer->>PipDeps: pip_install_comfyui_dependencies(..., cuda_tag)
PipDeps->>Torch: pip install torch --index-url .../whl/cuXXX
Torch-->>PipDeps: success
PipDeps-->>Installer: success
Installer-->>User: installation finished
sequenceDiagram
participant User as User/CLI
participant Launch as comfy_cli.command.launch
participant Config as ConfigManager
participant Resolver as cm_cli_util.resolve_manager_gui_mode
participant Finder as cm_cli_util.find_cm_cli
participant Flags as _get_manager_flags
participant Comfy as ComfyUI process
User->>Launch: comfy launch
Launch->>Resolver: resolve_manager_gui_mode(not_installed_value=None)
Resolver->>Config: get(CONFIG_KEY_MANAGER_GUI_MODE / legacy)
alt config present
Config-->>Resolver: mode (disable/enable/legacy)
else no config
Resolver->>Finder: find_cm_cli()
Finder-->>Resolver: True/False
alt found
Resolver-->>Launch: default enable mode
else not found
Resolver-->>Launch: None
end
end
Launch->>Flags: _get_manager_flags(mode)
Flags->>Finder: find_cm_cli()
alt cm_cli found and mode enables manager
Finder-->>Flags: True
Flags-->>Launch: inject manager flags (e.g., --enable-manager, --disable-manager-ui)
else missing or disabled
Flags-->>Launch: [] or warning + fallback
end
Launch->>Comfy: start process with appended flags
Comfy-->>User: started / exit code
Possibly related PRs
A tiny rhyme to sign off: new CUDA finds the wheel in time—no cliffhanger, just a smooth install line. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/command/install.py (2)
145-163: 🧹 Nitpick | 🔵 TrivialUnused parameters:
restore,*args,**kwargsdetected.Static analysis correctly identifies these as unused. If they're for forward compatibility, consider documenting this intent or removing them to keep the signature trim—no need for phantom params to swim!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/install.py` around lines 145 - 163, The execute function signature contains unused parameters (restore, *args, **kwargs); either remove them from the signature and update any callers (remove restore and variadic forwarding) or explicitly mark them as intentionally reserved by renaming to _restore and *_args, **_kwargs and add a short comment like "reserved for forward compatibility" so linters/static analysis ignore them; update any internal references or docstring to reflect the chosen approach and keep the unique symbol execute as the locus of the change.
546-547: 🧹 Nitpick | 🔵 TrivialDead code:
response is Nonecheck is unreachable.
requests.get()never returnsNone—it either returns aResponseobject or raises arequests.RequestException. This check appears to be dead code, unless there's mocking involved in tests. None shall pass? None shall not arrive!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/install.py` around lines 546 - 547, The check "if response is None" is dead code because requests.get() raises on network errors and never returns None; remove that branch and instead wrap the requests.get(...) call in a try/except catching requests.RequestException to raise a clear Exception referencing pr_number on request failures, and after a successful call validate the HTTP status (e.g., response.status_code != 200) and raise an Exception(f"Failed to fetch PR #{pr_number}: <reason>") if the response is not OK; update the code around the response and pr_number variables accordingly.
🤖 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/command/code_search.py`:
- Around line 71-76: The generated GitHub blob URL in the code_search logic uses
raw file_path which breaks on spaces and special chars; update the code that
builds base_url (the variables clean_name, ref, file_path and the base_url
assignment) to URL-encode file_path with urllib.parse.quote (add "from
urllib.parse import quote" to imports), replacing file_path in the f-string with
quote(file_path, safe="/") so links are properly encoded before creating the
matches entries.
- Around line 27-33: The repo filter builds an anchored regex using
parts.append(f"repo:^{repo}$") but doesn't escape user-supplied repo, so
metacharacters in repo (e.g., '.', '+', '[') can change matching; update the
code to call re.escape(repo) before formatting the anchored repo filter (i.e.,
replace repo with re.escape(repo) in the parts.append call), ensure you still
prepend "Comfy-Org/" when "/" not in repo, and add import re to the module
imports.
In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py`:
- Around line 125-138: The parse_cm_output function currently compares pinned
entries against the full "name@version" string so pins that list only the base
name slip through; update parse_cm_output to derive the base name by splitting
each stripped entry at the first "@" (e.g., base = stripped.split("@", 1)[0])
and filter out any entry where either the full "name@version" or the base name
is in the pinned set (pinned_nodes) so both "name" and "name@version" pins are
respected.
In `@comfy_cli/command/custom_nodes/cm_cli_util.py`:
- Around line 28-54: find_cm_cli() is incorrectly memoized without any input key
so it returns stale results when the workspace or interpreter changes; change
the caching to key off the resolved Python interpreter (or workspace path) so
the cache reflects the environment that determines the result: call
resolve_workspace_python(workspace_manager.workspace_path) (or compute a stable
interpreter string) and add that as an explicit parameter or incorporate it into
the cache key used by `@lru_cache` on find_cm_cli (keep the function name
find_cm_cli and the lookup logic that uses resolve_workspace_python and
workspace_manager.workspace_path), then use lru_cache(maxsize=...) with that
parameter so subsequent calls for different workspaces/interpreters are cached
separately and remove test-only cache clearing. Ensure the public signature
still returns bool and behavior when no workspace remains unchanged.
In `@comfy_cli/command/custom_nodes/command.py`:
- Around line 369-373: The loop that sets existing_target by iterating
disabled_path.iterdir() runs immediately after
disabled_path.mkdir(exist_ok=True), which can create an empty directory and make
the iteration trivially empty; update the code around disabled_path.mkdir and
the for loop (the existing_target variable and the item.is_dir() /
item.name.lower() == "comfyui-manager" check) by adding a concise comment that
explains the intent: we create the directory if missing but then check for any
pre-existing "comfyui-manager" subdirectory (so an empty iterator is expected
when it was just created), clarifying that this behavior is deliberate and not a
bug.
In `@comfy_cli/command/install.py`:
- Around line 524-526: The branch parsed from pr_ref is extracted into the
variable branch but currently discarded by returning (username, default_repo,
None); change the return to include the parsed branch so callers receive it
(e.g., return username, default_repo, branch) so upstream callers like
handle_pr_checkout and find_pr_by_branch get the branch without re-parsing;
update the tuple returned from the code that splits pr_ref (the username, branch
= pr_ref.split(":", 1) block) to pass branch instead of None.
- Around line 243-288: The duplicated manager-installation-failure handling in
the skip_manager branches should be extracted into a single helper (e.g.,
_handle_manager_install_failure) and invoked wherever
pip_install_manager(repo_dir, python=python) returns False; move the import and
ConfigManager().set(constants.CONFIG_KEY_MANAGER_GUI_MODE, "disable") plus the
rprint("[yellow]Manager not installed. Launch will run without manager
flags.[/yellow]") into that helper, and replace the two inline blocks in the
non-fast_deps and fast_deps paths with calls to _handle_manager_install_failure
to keep code DRY (references: skip_manager, pip_install_manager,
DependencyCompiler, constants.CONFIG_KEY_MANAGER_GUI_MODE, rprint).
- Around line 116-142: The failure branch in pip_install_manager currently only
prints result.stderr; update the error handling in pip_install_manager (and keep
the find_cm_cli.cache_clear() behavior) to also print result.stdout so pip's
non-error output is available for diagnostics—use rprint to emit both stderr (if
present) and stdout (if present), and ensure stdout is printed even when stderr
is empty to provide full pip output for debugging.
- Around line 264-271: The cuda_tag parsing is brittle for short tags (e.g.,
"cu90"); update the logic around cuda_tag/digits/resolved_cuda to handle
variable-length digit strings: extract digits = cuda_tag[2:], then if digits has
length >=3 keep the existing two+rest split, otherwise compute major =
int(digits[:-1]) (or digits as major when only one char) and minor = digits[-1]
(default "0" if no minor) and build resolved_cuda = f"{major}.{minor}" so "cu90"
-> "9.0" and "cu130" still -> "13.0"; apply this change where cuda_tag and
resolved_cuda are defined.
- Around line 198-214: The broad except Exception that follows the
git.InvalidGitRepositoryError catch should be narrowed: replace the bare except
Exception after the git.Repo(remote) block with a catch for git.GitError (base
class for GitPython errors) and OSError to cover filesystem issues, e.g. except
(git.GitError, OSError) as e, and include the exception details in the rprint
message; locate the handling around the repo = git.Repo(repo_dir) / remote_urls
retrieval and update the exception clause and error message accordingly.
In `@comfy_cli/command/launch.py`:
- Around line 180-186: The launch() function currently calls
_get_manager_flags() and appends manager_flags to extra before deciding the
execution path, causing duplicate manager flags when background is True because
background_launch() will re-run comfy launch and append them again; move the
manager_flags injection so it only happens for the non-background path (i.e.,
append _get_manager_flags() to extra only when background is False and you're
about to execute the direct launch), leaving background_launch(extra,
frontend_pr) to receive the original extra without injected manager flags;
update references in launch(), _get_manager_flags(), background_launch(), and
any use of the extra variable to reflect this conditional injection to avoid
duplicated/overridden manager args.
In `@comfy_cli/workspace_manager.py`:
- Around line 314-327: The code instantiates a new ConfigManager (config_manager
= ConfigManager()) which can diverge from the instance-level config; replace
that local instantiation by using the existing self.config_manager throughout
(e.g., use self.config_manager.get(...) instead of config_manager.get(...));
ensure subsequent reads like the call that gets
constants.CONFIG_KEY_UV_COMPILE_DEFAULT and any other uses in this block
reference self.config_manager so the manager used is the instance's configured
manager rather than a newly created one.
In `@DEV_README.md`:
- Around line 137-143: Update the README to document that runtime resolution
uses the workspace interpreter (the command executed is effectively python -m
cm_cli in the resolved workspace), not necessarily the cm-cli executable on
PATH; revise the section around the `cm-cli`/`comfy --here install` steps to
explicitly state how Python/module resolution works, note how to verify which
interpreter/environment is being used, and give a short example command to run
the module via the workspace Python (e.g., using python -m cm_cli) so
contributors don’t mistakenly validate the wrong `cm-cli` on PATH.
In `@docs/DESIGN-uv-compile.md`:
- Around line 18-53: The fenced code blocks in the DESIGN-uv-compile document
(the ASCII flow diagram showing "User CLI" / "command.py:install()" /
"cm_cli_util.py:execute_cm_cli()" and the blocks that include
"ConfigManager.set(\"uv_compile_default\", \"True\")" and the subprocess/uv-sync
examples) are unlabeled and cause markdownlint errors; update each
triple-backtick fence to include an appropriate language tag (e.g., use "text"
or "bash" for the ASCII CLI flow, "python" for snippets referencing
command.py/ConfigManager calls, and "ini" for the config.ini example) so each
fenced block declares a language.
In `@README.md`:
- Around line 59-67: Update the README snippet around the custom Manager install
(the block that shows "comfy install --skip-manager" and the two "pip install
..." examples) to explicitly tell users to install into the workspace
interpreter by either activating the target environment first or running the
workspace Python's pip (e.g., "python -m pip" from the workspace .venv), and
replace the bare "pip install -e /path/to/your-manager-fork" and "pip install
comfyui-manager==4.1b8" examples with equivalents that use the workspace
interpreter (or instruct to activate the env) so the Manager lands in the
correct interpreter for ComfyUI.
In `@tests/comfy_cli/command/nodes/test_node_install.py`:
- Around line 253-256: The test test_install_deps_no_args_shows_error currently
only checks output text; update it to assert the command actually aborted by
checking result.exit_code is non-zero (or result.exception is set) and verify
the patched execute_cm_cli was not called (mock_execute.assert_not_called()) to
ensure no side effects; keep the existing patch target
"comfy_cli.command.custom_nodes.command.execute_cm_cli" and runner.invoke(app,
["install-deps"]) but add the exit code/exception and mock-call assertions.
In `@tests/comfy_cli/command/nodes/test_pack.py`:
- Around line 35-37: The test currently mutates
extract_node_configuration.__defaults__, which is fragile; instead mock the
function where it is imported by the CLI command so the test controls its return
value. Replace the __defaults__ patch with a mock/patch of
comfy_cli.command.nodes.command.extract_node_configuration (or use patch.object
on the module that calls it) to return the expected config dict for the test, or
alternatively adjust the CLI code to accept an explicit path/env var and call
the real extract_node_configuration with that path; ensure you reference
extract_node_configuration and the calling module
comfy_cli.command.nodes.command when applying the patch.
- Around line 50-59: The subprocess.run calls in the test use no timeout and can
hang in CI; update each subprocess.run invocation in this test to pass a
reasonable timeout (e.g., timeout=10 or timeout=30) and handle TimeoutExpired if
needed; specifically modify the calls to subprocess.run([...], cwd=tmp_path,
capture_output=True, check=True) so they include timeout=<seconds> and
(optionally) wrap them where appropriate to catch subprocess.TimeoutExpired for
clearer failure messages.
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 381-476: Add a top-level smoke test that invokes the CLI
entrypoint used by users (comfy_cli.cmdline.app) rather than the internal
command app to ensure the "code-search" command and its hidden "cs" alias are
registered: create a new test (e.g. test_top_level_entrypoints) in
TestCodeSearchCLI that patches comfy_cli.command.code_search._fetch_results to
return raw_api_response, then call runner.invoke(comfy_cli.cmdline.app,
["code-search", "LoadImage"]) and runner.invoke(comfy_cli.cmdline.app, ["cs",
"LoadImage"]) and assert both exit_code == 0 and that the expected output (e.g.
"Comfy-Org/ComfyUI" or JSON results) appears and that _fetch_results was called
with the same query string as other tests.
In `@tests/comfy_cli/test_cm_cli_python_resolution.py`:
- Around line 28-32: The current _patched_popen only handles the exact sequence
[python, "-m", "cm_cli", ...] and will miss cases where interpreter flags (e.g.,
"-u") appear; change _patched_popen to search for the "-m" token anywhere in cmd
(e.g., find index m_idx = next index where token == "-m"), verify m_idx+1 exists
and equals "cm_cli", and then rewrite the command by preserving the python
executable and any interpreter flags before "-m" (e.g., new_cmd = [cmd[0]] +
cmd[1:m_idx] + [str(stub_script)] + cmd[m_idx+2:]) before calling
original_popen(cmd, **kwargs); keep references to _patched_popen,
original_popen, and stub_script when updating the logic.
In `@tests/comfy_cli/test_config_manager.py`:
- Line 13: The test is using a fragile closure unwrap pattern to extract the
singleton class (assigning _ConfigManagerCls =
ConfigManager.__closure__[0].cell_contents); extract that logic into a shared
test helper (e.g., a utility named unwrap_singleton_from_closure) and call it
from this test and the other test that uses the same pattern (so both tests
reuse the safe helper), or alternatively add a central comment/docs entry
describing the pattern and why it’s used; update references in this file to call
the helper instead of directly accessing __closure__ and keep the symbol
ConfigManager as the input to the helper.
In `@tests/comfy_cli/test_env_checker.py`:
- Line 11: The test accesses the singleton-wrapped class by digging into
EnvChecker.__closure__[0].cell_contents which is fragile and depends on CPython
closure internals; update the test by adding a clear comment above the line
referencing EnvCheckerCls and EnvChecker.__closure__[0].cell_contents that
explains why this unwrap is necessary (to get the original class from the
singleton decorator), notes that it relies on the decorator exposing the class
in the first closure cell, and instructs maintainers what to change if the
singleton decorator implementation changes (e.g., adjust the closure index,
alter the decorator to expose a .wrapped or .original attribute, or refactor
tests to use the public API).
In `@tests/e2e/test_e2e_uv_compile.py`:
- Around line 107-108: workspace fixture currently only cleans PACK_TEST1 and
PACK_TEST2, leaving PACK_IMPACT and PACK_INSPIRE between tests; update the
fixture to fully reset installed packs before yielding by either extending
_clean_test_packs() to also remove PACK_IMPACT and PACK_INSPIRE or invoking a
full reset helper (e.g., _reset_installed_packs() or similar) so tests like
test_real_packs_simultaneous_no_conflict always run against a fresh state.
- Around line 115-122: The test runs three separate comfy commands inside a
single exec(...) string so proc.returncode only reflects the final "comfy
--skip-prompt --no-enable-telemetry env" command; split them so each command's
exit status is checked. Replace the single exec call that builds proc with
either (a) three separate exec(...) invocations for the commands "comfy
--skip-prompt --workspace {ws} install {url_flag} {install_flags}", "comfy
--skip-prompt set-default {ws}", and "comfy --skip-prompt --no-enable-telemetry
env" and assert proc.returncode == 0 after each, or (b) join the commands with
&& so the shell short-circuits on failure; update references to proc accordingly
(the variable proc and the install/set-default/env command strings) so failures
in comfy install or set-default fail the test immediately.
In `@tests/e2e/test_e2e.py`:
- Around line 174-193: test_manager_installed (and the other Manager-specific
tests in the same block) should skip when the workspace lacks Manager v4.1+
support instead of hard-failing; update the tests to perform a pre-check using
resolve_workspace_python()/ws_python to attempt importing cm_cli (or invoking
ws_python -c "import cm_cli") and if that import check fails call
pytest.skip("Manager v4.1+ not available in workspace") before running
assertions. Ensure pytest is imported in the test file and apply the same skip
logic to the other tests referenced in the block (the Manager v4.1+ cases) so
they bail out cleanly when cm_cli is unavailable.
- Around line 50-52: The workspace fixture currently runs the blocking cache
refresh via the exec call that runs "comfy --workspace {ws} node update-cache"
(proc variable) and asserts on its returncode, which makes the shared workspace
depend on Manager/cache availability; remove that exec/assert from the workspace
fixture and instead create a dedicated fixture (e.g., ensure_node_cache or
manager_cache) or move the exec call into the node-specific tests
(test_model/test_run) so only those tests run the update-cache; if you keep a
helper fixture, scope it narrowly (function or module limited to node tests) and
handle failures non-fatally (log warnings) or fail only the consuming tests
rather than aborting the shared workspace setup.
- Around line 215-243: The test test_uv_compile_default_config mutates CLI
config but only disables the setting on the happy path; wrap the enable/verify
steps in a try/finally so the setting is always restored. Specifically, in the
test_uv_compile_default_config function (and where exec and comfy_cli are used)
move the calls that verify env and subsequent assertions into a try block and
run a cleanup exec(f"{comfy_cli} manager uv-compile-default false") in a finally
block to ensure uv-compile-default is disabled regardless of failures.
---
Outside diff comments:
In `@comfy_cli/command/install.py`:
- Around line 145-163: The execute function signature contains unused parameters
(restore, *args, **kwargs); either remove them from the signature and update any
callers (remove restore and variadic forwarding) or explicitly mark them as
intentionally reserved by renaming to _restore and *_args, **_kwargs and add a
short comment like "reserved for forward compatibility" so linters/static
analysis ignore them; update any internal references or docstring to reflect the
chosen approach and keep the unique symbol execute as the locus of the change.
- Around line 546-547: The check "if response is None" is dead code because
requests.get() raises on network errors and never returns None; remove that
branch and instead wrap the requests.get(...) call in a try/except catching
requests.RequestException to raise a clear Exception referencing pr_number on
request failures, and after a successful call validate the HTTP status (e.g.,
response.status_code != 200) and raise an Exception(f"Failed to fetch PR
#{pr_number}: <reason>") if the response is not OK; update the code around the
response and pr_number variables accordingly.
🪄 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: 912117f9-9424-4b03-a1c8-59b430901e51
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/workflows/build-and-test.yml.github/workflows/publish_package.yml.github/workflows/run-on-gpu.ymlDEV_README.mdREADME.mdcomfy_cli/cmdline.pycomfy_cli/command/code_search.pycomfy_cli/command/custom_nodes/bisect_custom_nodes.pycomfy_cli/command/custom_nodes/cm_cli_util.pycomfy_cli/command/custom_nodes/command.pycomfy_cli/command/install.pycomfy_cli/command/launch.pycomfy_cli/constants.pycomfy_cli/cuda_detect.pycomfy_cli/resolve_python.pycomfy_cli/workspace_manager.pydocs/DESIGN-uv-compile.mddocs/PRD-uv-compile.mddocs/TESTING-e2e.mdtests/comfy_cli/command/github/test_pr.pytests/comfy_cli/command/nodes/test_node_install.pytests/comfy_cli/command/nodes/test_pack.pytests/comfy_cli/command/test_bisect_parse.pytests/comfy_cli/command/test_cm_cli_util.pytests/comfy_cli/command/test_code_search.pytests/comfy_cli/command/test_command.pytests/comfy_cli/command/test_frontend_pr.pytests/comfy_cli/command/test_manager_gui.pytests/comfy_cli/conftest.pytests/comfy_cli/test_cm_cli_python_resolution.pytests/comfy_cli/test_config_manager.pytests/comfy_cli/test_cuda_detect.pytests/comfy_cli/test_cuda_detect_real.pytests/comfy_cli/test_env_checker.pytests/comfy_cli/test_file_utils.pytests/comfy_cli/test_global_python_install.pytests/comfy_cli/test_install.pytests/comfy_cli/test_install_python_resolution.pytests/comfy_cli/test_launch_python_resolution.pytests/comfy_cli/test_resolve_python.pytests/comfy_cli/test_utils.pytests/e2e/test_e2e.pytests/e2e/test_e2e_uv_compile.pytests/uv/test_uv.py
💤 Files with no reviewable changes (1)
- tests/comfy_cli/test_file_utils.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/comfy_cli/command/test_code_search.py (1)
381-476: 🧹 Nitpick | 🔵 TrivialCLI tests are thorough, but consider adding a top-level smoke test.
These tests invoke
code_search.appdirectly, which won't catch a brokenapp.add_typer(...)registration incomfy_cli/cmdline.pyor verify the hiddencsalias works. A single smoke test usingcomfy_cli.cmdline.appwould close that loop — so thecsalias doesn't play hide-and-seek outside coverage!🧪 Example smoke test for top-level CLI wiring
`@patch`("comfy_cli.command.code_search._fetch_results") def test_top_level_entrypoints(self, mock_fetch, raw_api_response): """Smoke test that code-search and cs alias are registered at the top level.""" from comfy_cli.cmdline import app as main_app mock_fetch.return_value = raw_api_response # Test main command name result = runner.invoke(main_app, ["code-search", "LoadImage"]) assert result.exit_code == 0 # Test hidden alias mock_fetch.reset_mock() mock_fetch.return_value = raw_api_response result = runner.invoke(main_app, ["cs", "LoadImage"]) assert result.exit_code == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/comfy_cli/command/test_code_search.py` around lines 381 - 476, Add a top-level smoke test to ensure the code-search command and its hidden alias are wired into the main CLI: import comfy_cli.cmdline.app (symbol: app) and invoke it with both "code-search" and "cs" to confirm they route to the existing code_search app (symbol: comfy_cli.command.code_search._fetch_results mocked) — create a test function (e.g., test_top_level_entrypoints) that patches _fetch_results, calls runner.invoke(main_app, ["code-search", "LoadImage"]) and runner.invoke(main_app, ["cs", "LoadImage"]) asserting exit_code == 0 for both to catch missing app.add_typer(...) registration or alias issues.
🤖 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/command/code_search.py`:
- Around line 128-131: The f-string passed to console.print in the code_search
logic is over 120 characters; split the long line by building the message in
smaller parts (e.g., assign the formatted pieces to temporary variables like
limit_msg and main_msg or use multiple concatenated f-strings) and then call
console.print with the combined result to keep each source line under 120 chars;
locate the block using limit_msg and the console.print call in code_search.py to
apply the change.
- Around line 154-165: The exception handlers around calling _fetch_results
should explicitly chain the original exceptions when re-raising typer.Exit so
the original traceback is preserved; change each "raise typer.Exit(code=1)" in
the requests.ConnectionError, requests.Timeout and requests.HTTPError except
blocks to "raise typer.Exit(code=1) from e" (or "from None" if you intentionally
want to suppress the original context), referencing the caught exception
variable e (or name it in the other handlers) so the original exception context
is explicit.
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 125-129: The fixture limit_hit_response mutates the incoming
raw_api_response in place; change it to operate on a deep copy instead (use
copy.deepcopy) so you modify and return the copied object rather than
raw_api_response to avoid test pollution; update the fixture to import deepcopy
(or copy) and set copied["data"]["search"]["results"]["limitHit"] = True on the
copy before returning it.
---
Duplicate comments:
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 381-476: Add a top-level smoke test to ensure the code-search
command and its hidden alias are wired into the main CLI: import
comfy_cli.cmdline.app (symbol: app) and invoke it with both "code-search" and
"cs" to confirm they route to the existing code_search app (symbol:
comfy_cli.command.code_search._fetch_results mocked) — create a test function
(e.g., test_top_level_entrypoints) that patches _fetch_results, calls
runner.invoke(main_app, ["code-search", "LoadImage"]) and
runner.invoke(main_app, ["cs", "LoadImage"]) asserting exit_code == 0 for both
to catch missing app.add_typer(...) registration or alias issues.
🪄 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: 7a7e015c-9e9e-4dac-b02d-b36ab72a2ce4
📒 Files selected for processing (2)
comfy_cli/command/code_search.pytests/comfy_cli/command/test_code_search.py
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 `@comfy_cli/cmdline.py`:
- Around line 363-364: The ternary assigning cuda_version and cuda_tag is
correct but dense; refactor by extracting the conditional into a clear if/else:
check skip_torch_or_directml first and set (cuda_version, cuda_tag) accordingly,
otherwise call _resolve_cuda(gpu, cuda_version). Update the assignment site
where cuda_version, cuda_tag are set (the line using _resolve_cuda(gpu,
cuda_version) and skip_torch_or_directml) so the new if/else branches produce
the same values and preserve variable names.
In `@tests/comfy_cli/test_cuda_detect.py`:
- Around line 220-229: The test currently computes idx from required_order and
asserts it's sorted; replace that with a clearer direct comparison: compute
observed = [c for c in calls if c in required_order] and expected = [p for p in
required_order if p in calls], then assert observed == expected to verify
relative order, and keep the existing assert calls[0] == "libcuda.so.1"; use the
existing variables required_order and calls when making this change.
🪄 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: 6890078d-45c1-4ce3-987c-b007a4a44eb4
📒 Files selected for processing (2)
comfy_cli/cmdline.pytests/comfy_cli/test_cuda_detect.py
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 `@comfy_cli/cmdline.py`:
- Around line 170-174: The long rprint f-string in the CUDA warning exceeds the
line-length limit; split the message into two shorter f-strings or break the
string across lines inside the existing rprint call so the line containing the
formatted message no longer exceeds 120 chars. Locate the rprint call that
references drv, DEFAULT_CUDA_TAG and the f-string mentioning `--cuda-version`,
and rewrite it using parentheses and a line break (or two adjacent f-strings) so
the warning text is wrapped across lines while keeping the same content and
leaving the `return None, DEFAULT_CUDA_TAG` unchanged.
In `@tests/comfy_cli/test_cuda_detect.py`:
- Around line 145-170: The parameter named "val" in the test helper function
capturing_cuInit is unused; rename it to "_val" (i.e., def
capturing_cuInit(_val):) to follow the underscore convention for intentionally
unused parameters—update the function signature in the test (capturing_cuInit)
and ensure its usage as a side_effect for lib.cuInit remains unchanged.
🪄 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: 42b50db9-83f0-4c17-af55-3dc89c079ea4
📒 Files selected for processing (2)
comfy_cli/cmdline.pytests/comfy_cli/test_cuda_detect.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/comfy_cli/command/test_code_search.py (1)
125-129:⚠️ Potential issue | 🟡 MinorAvoid in-place fixture mutation (same issue previously raised).
Line 128 mutates
raw_api_responsedirectly; that can create sneaky test coupling if fixture scope changes. Clone first, then mutate the clone.Proposed fix
+import copy + `@pytest.fixture` def limit_hit_response(raw_api_response): """An API response where the limit was hit.""" - raw_api_response["data"]["search"]["results"]["limitHit"] = True - return raw_api_response + data = copy.deepcopy(raw_api_response) + data["data"]["search"]["results"]["limitHit"] = True + return data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/comfy_cli/command/test_code_search.py` around lines 125 - 129, The fixture limit_hit_response mutates the shared raw_api_response in-place; instead create a deep copy of raw_api_response (e.g., via copy.deepcopy), change the copy's ["data"]["search"]["results"]["limitHit"] to True, and return the copy so the original raw_api_response fixture is not modified; ensure you import copy at the top of the test module and only mutate the cloned object.
🤖 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/command/test_code_search.py`:
- Line 504: The local variable parsed is assigned but never used (triggering
Ruff F841); update the line "parsed = json.loads(result.output)" in the test
(tests/comfy_cli/command/test_code_search.py) to either assert something about
the parsed value or, if the decode is only for validation, change the assignment
to "_ = json.loads(result.output)" (or rename to "_parsed") so the unused-value
linter warning is suppressed.
- Around line 488-503: Patch the root callback side effects so the root app
integration tests don't call real setup/tracking: in the test class or module
where test_code_search_registered and test_cs_alias_registered live, mock or
patch comfy_cli.workspace_manager.WorkspaceManager.setup_workspace_manager and
comfy_cli.tracking.prompt_tracking_consent (e.g., with pytest fixtures or `@patch`
decorators) to no-op before invoking root_app so runner.invoke(root_app, ...)
only exercises the code-search wiring; ensure the patches target the exact
symbols WorkspaceManager.setup_workspace_manager and prompt_tracking_consent and
are applied for the scope of those tests.
---
Duplicate comments:
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 125-129: The fixture limit_hit_response mutates the shared
raw_api_response in-place; instead create a deep copy of raw_api_response (e.g.,
via copy.deepcopy), change the copy's ["data"]["search"]["results"]["limitHit"]
to True, and return the copy so the original raw_api_response fixture is not
modified; ensure you import copy at the top of the test module and only mutate
the cloned object.
🪄 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: 0d6e66e3-c58e-4353-bd42-8b57d92217bf
📒 Files selected for processing (1)
tests/comfy_cli/command/test_code_search.py
snomiao
force-pushed
the
feat/code-search
branch
from
4f7d605 to
d1e22e7
Compare
dosubot
bot
added
size:XL
snomiao
force-pushed
the
feat/code-search
branch
from
d1e22e7 to
aaa3c9f
Compare
Adds a new CLI command that searches code across ComfyUI repositories via the comfy-codesearch Vercel API (Sourcegraph-backed). - `--repo` / `-r`: filter by repository (short or full name) - `--count` / `-n`: limit results (default 20) - `--json` / `-j`: machine-readable JSON output - `cs` hidden alias for quick access Includes 35 unit tests covering query building, response parsing, error handling, CLI integration, and root-app wiring smoke tests. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
snomiao
force-pushed
the
feat/code-search
branch
from
aaa3c9f to
9dd504b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+121
to
+125
| @app.callback(invoke_without_command=True) | ||
| @tracking.track_command() | ||
| def code_search( | ||
| query: Annotated[str, typer.Argument(help="Search query (supports Sourcegraph syntax)")], | ||
| repo: Annotated[ |
There was a problem hiding this comment.
@tracking.track_command() captures and logs all CLI kwargs except ctx/context (see comfy_cli/tracking.py:69-76). For this command that includes the raw query string, which can contain proprietary code fragments or other sensitive terms; it will be included in debug logs and (when tracking is enabled) sent as event properties. Consider excluding/redacting query from tracking for this command (e.g., remove the decorator here, or extend track_command to support an allowlist/redaction and use it so only non-sensitive flags like repo/count/json_output are tracked).
Comment on lines
+106
to
+108
| def limit_hit_search(search_response): | ||
| search_response["results"]["limitHit"] = True | ||
| return search_response |
There was a problem hiding this comment.
limit_hit_search mutates the search_response fixture in-place (search_response["results"]["limitHit"] = True). If another fixture/test in the same test function also depends on search_response, this can create order-dependent coupling. Prefer returning a copied dict (e.g., deep copy) before mutation to keep fixtures isolated.
bigcat88
approved these changes
Mar 31, 2026
Contributor
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
The PR looks good!
I can not find any issues with it - LGTM!
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
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.
Summary
comfy code-searchsubcommand (aliascomfy cs) that searches code across ComfyUI repositories via the comfy-codesearch Vercel API (Sourcegraph-backed)--repo/-rto filter by repository,--count/-nto limit results, and--json/-jfor machine-readable outputUsage
Demo
Test plan
pytest tests/comfy_cli/command/test_code_search.py)comfy code-search "LoadImage"returns resultscomfy cs "LoadImage"alias workscomfy code-search --json "LoadImage"outputs valid JSONcomfy code-search --repo ComfyUI "SaveImage"filters correctly🤖 Generated with Claude Code