Add "ComfyUI CoolB Nodes" to custom nodes list - #3292
CoolBreeze164 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe custom-node list adds ComfyUI CoolB Nodes with git-clone installation, its GitHub repository, author information, and a description of its utility nodes. ChangesCustom Node Registration
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The manifest entry satisfies the documented registration and git-clone integration contracts, so it is ready to merge. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Comment |
|
Thanks for the contribution! I took a look at ComfyUI-CoolB-Nodes and ran into a couple of issues I'd like to see addressed before this can be merged.
Both stem from the same broken-containment issue at the path-construction layer. Please resolve each of these against a fixed base directory: use realpath and check with commonpath that the result stays inside that base, and reject absolute paths and any I'll re-evaluate once these are addressed. |
|
Thank you for your review! I'm completely new to all this coding stuff, this was my first repo. You were right, after testing, the original writer and reader could indeed operate outside comfy root folder. I added a fix using Qwen:
After testing the fix it seems to resolve the issues. If only comfy had text managers natively :( |
|
Thanks for the quick fix, and nice work — especially for a first repo! I re-checked the updated code, and the path handling itself is solid now: absolute paths and ".." are rejected, and the realpath + commonpath check keeps both nodes inside their base directory. The remaining issue is which directory that base is. Both nodes use the whole ComfyUI root (nodes.py:1494 for the reader, nodes.py:1541 for the writer), and that is still too wide:
Your _safe_resolve_under_base helper can stay exactly as it is — only the base_dir it is given needs to change. I'll re-evaluate once these are addressed. |
Adds a pack of utility nodes for ComfyUI featuring saving/loading speakers with transcripts (useful alongside TTS packs), execution flow control (muting/bypassing nodes and groups on trigger to actively change the workflow state from run to run), some useful text file utilities (including save/load text file) and data type conversion.