fix: filter FETCH progress lines from bisect node list#400
Conversation
📝 WalkthroughWalkthroughAdded 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❌ Patch coverage is
@@ Coverage Diff @@
## main #400 +/- ##
==========================================
+ Coverage 74.98% 75.00% +0.01%
==========================================
Files 34 34
Lines 4025 4028 +3
==========================================
+ Hits 3018 3021 +3
Misses 1007 1007
🚀 New features to boost your workflow:
|
dosubot
bot
added
size:L
![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 (1)
comfy_cli/command/custom_nodes/bisect_custom_nodes.py (1)
157-162:⚠️ Potential issue | 🟠 MajorPass
mode="local"tosimple-showto stop fetching the registry and nip the latency in the bud.The
execute_cm_clicall at line 157 doesn't specify a mode parameter, so it defaults to fetching from the remote registry even though bisect only needs locally enabled nodes. Thecm-cliinfrastructure supports--mode localto avoid network requests, which other commands in the codebase already use. Sinceset_custom_node_enabled_states()at lines 112–114 also callsenableanddisablewithout mode, those should usemode="local"as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py` around lines 157 - 162, Call cm-cli in local mode to avoid remote registry fetches: update the execute_cm_cli invocation that runs "simple-show" (the call to execute_cm_cli(["simple-show", "enabled"])) to pass mode="local" so it becomes a local query, and also update the enable/disable invocations inside set_custom_node_enabled_states (the calls that run "enable" and "disable" via execute_cm_cli) to include mode="local" so both the initial nodes list and the subsequent enable/disable operations use the local mode and avoid network latency.
🤖 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/custom_nodes/bisect_custom_nodes.py`:
- Around line 125-136: The parser parse_cm_output currently filters by blacklist
but should strictly whitelist node-shaped lines to avoid future ghost entries;
update parse_cm_output to only accept lines matching a node pattern (e.g.,
"name@version") using a regex like r'^[^@\s]+@[^@\s]+$' (or equivalent) and then
apply the existing pinned_nodes exclusion, so only regex-matching lines that are
not in pinned are returned; keep trimming and handle None pinned_nodes as
before.
---
Outside diff comments:
In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py`:
- Around line 157-162: Call cm-cli in local mode to avoid remote registry
fetches: update the execute_cm_cli invocation that runs "simple-show" (the call
to execute_cm_cli(["simple-show", "enabled"])) to pass mode="local" so it
becomes a local query, and also update the enable/disable invocations inside
set_custom_node_enabled_states (the calls that run "enable" and "disable" via
execute_cm_cli) to include mode="local" so both the initial nodes list and the
subsequent enable/disable operations use the local mode and avoid network
latency.
🪄 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: 4aec7667-2525-43f2-8be2-29402c814920
📒 Files selected for processing (2)
comfy_cli/command/custom_nodes/bisect_custom_nodes.pytests/comfy_cli/command/test_bisect_parse.py
| def parse_cm_output(cm_output: str, pinned_nodes: set[str] | None = None) -> list[str]: | ||
| """Parse cm_cli simple-show output into a list of node names. | ||
|
|
||
| Filters out progress/status lines (FETCH ...) and blank lines, | ||
| keeping only actual node entries. | ||
| """ | ||
| pinned = pinned_nodes or set() | ||
| return [ | ||
| stripped | ||
| for line in cm_output.strip().split("\n") | ||
| if (stripped := line.strip()) and not stripped.startswith("FETCH") and stripped not in pinned | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden parsing by whitelisting node-shaped lines (name@version) to avoid future ghost entries.
Nice fix for FETCH leaks, but this is still blacklist-based. If cm_cli emits other status lines later, phantom nodes can sneak back in. A tiny whitelist keeps this imp from coming back to rhyme.
♻️ Proposed parser hardening
def parse_cm_output(cm_output: str, pinned_nodes: set[str] | None = None) -> list[str]:
@@
- return [
- stripped
- for line in cm_output.strip().split("\n")
- if (stripped := line.strip()) and not stripped.startswith("FETCH") and stripped not in pinned
- ]
+ return [
+ stripped
+ for raw in cm_output.splitlines()
+ if (stripped := raw.strip())
+ and "@" in stripped
+ and not stripped.startswith("FETCH")
+ and stripped not in pinned
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py` around lines 125 -
136, The parser parse_cm_output currently filters by blacklist but should
strictly whitelist node-shaped lines to avoid future ghost entries; update
parse_cm_output to only accept lines matching a node pattern (e.g.,
"name@version") using a regex like r'^[^@\s]+@[^@\s]+$' (or equivalent) and then
apply the existing pinned_nodes exclusion, so only regex-matching lines that are
not in pinned are returned; keep trimming and handle None pinned_nodes as
before.
bigcat88
force-pushed
the
fix/bisect-fetch-lines
branch
from
b7dfa19 to
f816e0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
comfy_cli/command/custom_nodes/bisect_custom_nodes.py (1)
125-138: 🧹 Nitpick | 🔵 TrivialHarden parsing to strict
name@versionshape.Line 137 still accepts any line containing
@; that can let future status lines sneak in as ghost nodes again. A strict pattern keeps this bisect from playing hide-and-seek with phantom entries.♻️ Proposed parser tightening
+import re + +NODE_LINE_RE = re.compile(r"^[^@\s].*?@[^@\s]+$") + def parse_cm_output(cm_output: str, pinned_nodes: set[str] | None = None) -> list[str]: @@ pinned = pinned_nodes or set() return [ stripped - for line in cm_output.strip().split("\n") - if (stripped := line.strip()) and "@" in stripped and stripped not in pinned + for line in cm_output.splitlines() + if (stripped := line.strip()) + and NODE_LINE_RE.fullmatch(stripped) + and stripped not in pinned ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py` around lines 125 - 138, The parser parse_cm_output currently accepts any line containing "@", which can falsely include status lines; tighten it to only accept strict name@version tokens by validating each stripped line against a pattern like "one non-space/non-@ segment, then @, then one non-space/non-@ segment" (use a regex test) before filtering against pinned_nodes; update references in the function (parameters: cm_output, pinned_nodes; local: pinned) to use that validation so only exact "name@version" strings are returned and pinned comparisons remain correct.
🤖 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_bisect_parse.py`:
- Around line 140-142: Add a regression test to ensure parse_cm_output ignores
status lines that contain "@" but are not node entries: update or add to
test_arbitrary_status_lines_filtered to include a line like "INFO: user@host"
(or similar imposter lines) in the output string and assert that
parse_cm_output(output) still returns only the legitimate node token(s) (e.g.,
["NodeA@1.0"]). This ensures parse_cm_output's filtering logic (used in
test_arbitrary_status_lines_filtered) rejects non-node lines containing "@" and
prevents false positives.
---
Duplicate comments:
In `@comfy_cli/command/custom_nodes/bisect_custom_nodes.py`:
- Around line 125-138: The parser parse_cm_output currently accepts any line
containing "@", which can falsely include status lines; tighten it to only
accept strict name@version tokens by validating each stripped line against a
pattern like "one non-space/non-@ segment, then @, then one non-space/non-@
segment" (use a regex test) before filtering against pinned_nodes; update
references in the function (parameters: cm_output, pinned_nodes; local: pinned)
to use that validation so only exact "name@version" strings are returned and
pinned comparisons remain correct.
🪄 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: c472121d-b0d6-4278-b1c8-8c5b64b8fa18
📒 Files selected for processing (2)
comfy_cli/command/custom_nodes/bisect_custom_nodes.pytests/comfy_cli/command/test_bisect_parse.py
| def test_arbitrary_status_lines_filtered(self): | ||
| output = "some random status line\nINFO: loading\nNodeA@1.0\nDone.\n" | ||
| assert parse_cm_output(output) == ["NodeA@1.0"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression case for status lines that contain @.
Nice coverage overall, but this suite should also assert that non-node lines with @ are rejected (e.g., INFO: user@host) so the filter doesn’t get tricked by imposter lines.
🧪 Suggested test addition
class TestParseCmOutput:
@@
def test_arbitrary_status_lines_filtered(self):
output = "some random status line\nINFO: loading\nNodeA@1.0\nDone.\n"
assert parse_cm_output(output) == ["NodeA@1.0"]
+
+ def test_status_lines_with_at_symbol_filtered(self):
+ output = "INFO: user@host\nNodeA@1.0\n"
+ assert parse_cm_output(output) == ["NodeA@1.0"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_arbitrary_status_lines_filtered(self): | |
| output = "some random status line\nINFO: loading\nNodeA@1.0\nDone.\n" | |
| assert parse_cm_output(output) == ["NodeA@1.0"] | |
| def test_arbitrary_status_lines_filtered(self): | |
| output = "some random status line\nINFO: loading\nNodeA@1.0\nDone.\n" | |
| assert parse_cm_output(output) == ["NodeA@1.0"] | |
| def test_status_lines_with_at_symbol_filtered(self): | |
| output = "INFO: user@host\nNodeA@1.0\n" | |
| assert parse_cm_output(output) == ["NodeA@1.0"] |
🧰 Tools
🪛 Pylint (4.0.5)
[convention] 140-140: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/comfy_cli/command/test_bisect_parse.py` around lines 140 - 142, Add a
regression test to ensure parse_cm_output ignores status lines that contain "@"
but are not node entries: update or add to test_arbitrary_status_lines_filtered
to include a line like "INFO: user@host" (or similar imposter lines) in the
output string and assert that parse_cm_output(output) still returns only the
legitimate node token(s) (e.g., ["NodeA@1.0"]). This ensures parse_cm_output's
filtering logic (used in test_arbitrary_status_lines_filtered) rejects non-node
lines containing "@" and prevents false positives.
1 participant
Fixes #271.
comfy node bisect startcallscm_cli simple-show enabledwhich printsFETCH ComfyRegistry Data: X/Yprogress lines to stdout before the actual node list. The output parser only filtered lines starting withFETCH DATA(catchingFETCH DATA from: ...) but missedFETCH ComfyRegistry Data: ...lines. These leaked into the node list as 18 phantom entries, causing the bisect to waste time on nonexistent nodes.Fixed by extracting the parsing into a
parse_cm_output()function and broadening the filter to exclude all lines starting withFETCH. Test uses the exact real-world output from the issue to verify only real nodes pass through.