Repository navigation
feat: audit fixes — OUTPUT_NODE, DESCRIPTION, IS_CHANGED, structlog g… - #15
Conversation
…uard, CI Python versions - Add OUTPUT_NODE=True to all 5 node classes for ComfyUI execution triggers - Add DESCRIPTION attributes for ComfyUI 0.19+ frontend info panel - Add IS_CHANGED with deterministic hash for execution caching (H100 compute savings) - Guard structlog import in grabcut_nodes.py with stdlib logging fallback - Update CI Python matrix to 3.10-3.12 (drop EOL 3.8/3.9) - Remove dead RGBA loop in GrabCutRefinement (redundant alpha multiply) - Wire all Pydantic-validated params in AutoGrabCutRemover
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughCI matrix narrowed to Python 3.10–3.12. GrabCut and transparency nodes gain deterministic change hashing, parameter validation (pydantic fallback), structured logging, per-batch error handling with CUDA memory logging, and an optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Pydantic-based parameter validation, enhanced logging, and GPU memory tracking to the GrabCut nodes, alongside a new mask inversion feature. However, the implementation contains several critical errors: the variables validate_node_params, log, and _log_gpu_memory are referenced without being defined or imported in the module. Furthermore, the invert_mask parameter is used in the logic of both remove_background and refine_mask but is missing from the method signatures and the INPUT_TYPES configuration, which will lead to runtime NameErrors.
| Tuple of (processed_image, mask, bbox_string, confidence, metrics) | ||
| """ | ||
| # --- Pydantic validation: sanitise ALL user params before GPU execution --- | ||
| if validate_node_params is not None: |
There was a problem hiding this comment.
The variable validate_node_params is used here but is not imported or defined anywhere in this module. This will result in a NameError at runtime. Additionally, the log object used on line 392 and throughout the function is also undefined. Based on the pull request description, it appears that a logging setup and validation utility were intended to be added but are missing from the current changes.
| confidence_threshold=confidence_threshold, | ||
| scaling_method=scaling_method, | ||
| edge_blur_amount=edge_blur_amount, | ||
| invert_mask=invert_mask, |
There was a problem hiding this comment.
The variable invert_mask is passed to the validation function and assigned on line 383, but it is not present in the remove_background method signature (lines 323-341) or the INPUT_TYPES for the AutoGrabCutRemover class. This will cause a NameError when the code attempts to access it on this line.
| refined_masks.append(alpha_tensor) | ||
| else: | ||
| # Return original if refinement fails | ||
| _log_gpu_memory(f"refine_batch_{i}.start") |
There was a problem hiding this comment.
The function _log_gpu_memory is called here but is not defined in this file. Furthermore, the log object used on line 804 is also undefined. These missing definitions will cause the node to fail during execution.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| output_format = validated.output_format | ||
| auto_adjust = validated.auto_adjust | ||
| except Exception as exc: | ||
| log.error("grabcut_node.validation_failed", node="AutoGrabCutRemover", error=str(exc)) |
There was a problem hiding this comment.
CRITICAL: log used but never imported/defined. Will crash with NameError at runtime.
| log.error("grabcut_node.validation_failed", node="AutoGrabCutRemover", error=str(exc)) | ||
| raise ValueError(f"[AutoGrabCutRemover] Invalid parameters: {exc}") from exc | ||
|
|
||
| log.info("grabcut_node.remove_background.start", |
There was a problem hiding this comment.
CRITICAL: log not defined (same issue as line 392).
| refined_masks.append(alpha_tensor) | ||
| else: | ||
| # Return original if refinement fails | ||
| _log_gpu_memory(f"refine_batch_{i}.start") |
There was a problem hiding this comment.
CRITICAL: _log_gpu_memory called but never defined in file.
Code Review Roast 🔥Verdict: RESOLVED | Recommendation: Merge Previous Issues - All Fixed
All previous issues resolved in this significant refactoring:
New Additions
🏆 Best part: The composed Pydantic models in src/validation.py are properly structured with separate GrabCutParams, ScalingParams, MaskParams, BBoxParams. 📊 Overall: Clean, well-organized refactoring. Ship it. Files (27 changed)
Reviewed by minimax-m2.5-20260211 · 1,859,892 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
grabcut_nodes.py (3)
731-741:⚠️ Potential issue | 🔴 CriticalBlocker:
invert_maskis referenced but never declared as a parameter (both nodes).
GrabCutRefinement.refine_mask(Line 731-741): signature has noinvert_mask; Line 794if invert_mask:raisesNameErroron the first successful batch item.AutoGrabCutRemover.remove_background(Line 323-341): signature has noinvert_maskeither; Line 368 (invert_mask=invert_mask) and Line 383 (invert_mask = validated.invert_mask) will both raise.Additionally, neither node exposes
invert_maskinINPUT_TYPES, so even after you add the parameter, ComfyUI won't pass it unless it's also declared in the schema.🔧 Proposed fix for
GrabCutRefinementAdd the input to
INPUT_TYPES(around Line 694, insiderequiredoroptional):"scaling_method": (["NEAREST", "BILINEAR", "BICUBIC", "LANCZOS"], { "default": "NEAREST", "tooltip": "Interpolation method for scaling" }), + "invert_mask": ("BOOLEAN", { + "default": False, + "tooltip": "Invert the refined alpha mask" + }),And the signature:
def refine_mask(self, image: torch.Tensor, mask: torch.Tensor, grabcut_iterations: int = 3, edge_refinement: float = 0.5, edge_blur_amount: float = 0.0, expand_margin: int = 10, bbox_safety_margin: int = 20, min_bbox_size: int = 64, output_size: str = "ORIGINAL", scaling_method: str = "NEAREST", custom_width: int = 512, - custom_height: int = 512) -> Tuple: + custom_height: int = 512, + invert_mask: bool = False) -> Tuple:Apply the analogous change in
AutoGrabCutRemover.INPUT_TYPESandremove_background(...).Also applies to: 794-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 731 - 741, Both nodes reference invert_mask but it's not declared: add an invert_mask: bool = False parameter to GrabCutRefinement.refine_mask and AutoGrabCutRemover.remove_background, add invert_mask into each class's INPUT_TYPES schema (under required/optional as a boolean) so ComfyUI will supply it, and use the validated.invert_mask value where currently referenced (e.g., the call sites that do invert_mask=invert_mask and the runtime check if invert_mask:) to avoid NameError and ensure proper default behavior.
1-21:⚠️ Potential issue | 🔴 CriticalCritical blocker: four undefined names will crash both nodes on invocation.
The imports section (lines 1–21) is missing four symbols required by downstream code:
Symbol First use Impact validate_node_paramsLine 360 Raises NameErrorbefore theis not Nonecheck—it does not act as an availability guardlogLines 392, 395, 399, 804 log.info(...)at line 395 and other log calls will raiseNameErrorunconditionally_log_gpu_memoryLine 780 Raises NameErroron first batch iteration inrefine_maskinvert_maskLine 794 (in refine_mask)Undefined in refine_masksignature (lines 731–741) but referenced at line 794 as a local variableNote:
invert_maskis correctly defined as a parameter ofremove_background(line 323), so lines 368 and 383 are safe. However, line 794 is insiderefine_mask, which lacks this parameter.Every execution path through both node methods hits at least one of these undefined names, rendering both nodes non-functional.
🔧 Suggested imports + structlog fallback + GPU helper
import numpy as np import torch from typing import Tuple, Optional import time from PIL import Image +import logging + +# Structlog with stdlib fallback (per PR objective) +try: + import structlog + log = structlog.get_logger(__name__) +except ImportError: # pragma: no cover + log = logging.getLogger(__name__) + +# Optional Pydantic-based parameter validator +try: + from .param_validation import validate_node_params # adjust path to actual module +except ImportError: + try: + from param_validation import validate_node_params + except ImportError: + validate_node_params = None + + +def _log_gpu_memory(tag: str) -> None: + """Log current CUDA allocation if available; no-op otherwise.""" + if torch.cuda.is_available(): + try: + log.debug("gpu_memory", tag=tag, + allocated_gb=round(torch.cuda.memory_allocated() / 1e9, 3)) + except Exception: + passAlso add
invert_maskas a parameter torefine_mask(line 731) and default it appropriately (e.g.,invert_mask: bool = False).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 1 - 21, The file is missing four symbols: validate_node_params, log, _log_gpu_memory and the refine_mask parameter invert_mask; add imports/fallbacks so the nodes don't crash: when COMFY_AVAILABLE is true import validate_node_params and log (or comfy.utils equivalents), otherwise define a lightweight fallback validate_node_params (pass-through), create a simple logger (or use structlog.get_logger) assigned to log, and add a safe _log_gpu_memory helper that checks torch.cuda.is_available() and logs/returns memory usage (no-op if CUDA absent); finally, add invert_mask: bool = False to the refine_mask(...) signature (refine_mask is referenced around lines 731–741/794) so refs to invert_mask inside refine_mask work (remove_background already has invert_mask). Ensure you update references to use these symbols (validate_node_params, log, _log_gpu_memory, invert_mask) used in remove_background and refine_mask.
291-294:⚠️ Potential issue | 🔴 Critical
OUTPUT_NODE,DESCRIPTION, andIS_CHANGEDattributes are missing from all node classes despite being promised in the commit message.The commit
6b86f14("feat: audit fixes — OUTPUT_NODE, DESCRIPTION, IS_CHANGED...") explicitly claims to add these attributes for ComfyUI execution triggers, frontend info panels, and execution caching — but they were never actually implemented. BothAutoGrabCutRemover(lines 291–294) andGrabCutRefinement(lines 714–717) ingrabcut_nodes.py, along with both classes innodes.py(TransparencyBackgroundRemoverandTransparencyBackgroundRemoverBatch), only declareRETURN_TYPES,RETURN_NAMES,FUNCTION, andCATEGORY. All five node classes need these three attributes added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 291 - 294, Add the missing ComfyUI node metadata attributes to each node class: add OUTPUT_NODE (set to False), DESCRIPTION (brief one-line description specific to the node) and IS_CHANGED (set to False) to AutoGrabCutRemover, GrabCutRefinement, TransparencyBackgroundRemover, and TransparencyBackgroundRemoverBatch; place them alongside the existing class-level constants (RETURN_TYPES, RETURN_NAMES, FUNCTION, CATEGORY) and make DESCRIPTION a short human-readable string matching the class purpose (e.g., "Remove background using GrabCut" or "Refine GrabCut background mask") so frontend panels and execution caching can use them.
🧹 Nitpick comments (2)
.github/workflows/comfy-ci.yml (1)
14-14: LGTM on narrowing to 3.10–3.12.Both 3.8 and 3.9 are past upstream EOL, so dropping them is appropriate.
Optional (out-of-scope nit):
actions/setup-python@v4on line 20 is a couple of majors behind; bumping to@v5can be done opportunistically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/comfy-ci.yml at line 14, Update the GitHub Actions Python matrix to target supported versions by keeping python-version: ["3.10", "3.11", "3.12"] and, optionally, bump the setup action by updating the actions/setup-python usage from actions/setup-python@v4 to actions/setup-python@v5 to stay current; ensure the workflow syntax remains valid after the change.grabcut_nodes.py (1)
803-806: Prefer specific exception types for the per-item fallback.Ruff flags BLE001 here, and the project's established error-handling pattern is to catch
cv2.error,MemoryError, andValueErrorexplicitly so genuinely unexpected exceptions (e.g.,KeyboardInterrupt-adjacent or programming errors like theNameErrordiscussed above) are not silently swallowed and masked as a "refine_error" log line.♻️ Suggested fix
- except Exception as e: + except (cv2.error, MemoryError, ValueError) as e: log.error("grabcut_node.refine_error", item=i, error=str(e)) refined_images.append(image[i]) refined_masks.append(mask[i])(Requires
import cv2at module top if not already present.)Based on learnings: "Implement comprehensive try-catch error handling in main processing functions with specific error types (cv2.error, MemoryError, ValueError) and graceful fallback for failed batch items".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 803 - 806, Replace the broad except Exception with a specific catch for image-processing/fallback errors: change the handler around the per-item refine block to "except (cv2.error, MemoryError, ValueError) as e" (and add "import cv2" at the top if missing), keep the existing log.error("grabcut_node.refine_error", item=i, error=str(e)) and the fallback appends (refined_images.append(image[i]); refined_masks.append(mask[i])), and allow other unexpected exceptions to propagate instead of being swallowed.
🤖 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 `@grabcut_nodes.py`:
- Around line 731-741: Both nodes reference invert_mask but it's not declared:
add an invert_mask: bool = False parameter to GrabCutRefinement.refine_mask and
AutoGrabCutRemover.remove_background, add invert_mask into each class's
INPUT_TYPES schema (under required/optional as a boolean) so ComfyUI will supply
it, and use the validated.invert_mask value where currently referenced (e.g.,
the call sites that do invert_mask=invert_mask and the runtime check if
invert_mask:) to avoid NameError and ensure proper default behavior.
- Around line 1-21: The file is missing four symbols: validate_node_params, log,
_log_gpu_memory and the refine_mask parameter invert_mask; add imports/fallbacks
so the nodes don't crash: when COMFY_AVAILABLE is true import
validate_node_params and log (or comfy.utils equivalents), otherwise define a
lightweight fallback validate_node_params (pass-through), create a simple logger
(or use structlog.get_logger) assigned to log, and add a safe _log_gpu_memory
helper that checks torch.cuda.is_available() and logs/returns memory usage
(no-op if CUDA absent); finally, add invert_mask: bool = False to the
refine_mask(...) signature (refine_mask is referenced around lines 731–741/794)
so refs to invert_mask inside refine_mask work (remove_background already has
invert_mask). Ensure you update references to use these symbols
(validate_node_params, log, _log_gpu_memory, invert_mask) used in
remove_background and refine_mask.
- Around line 291-294: Add the missing ComfyUI node metadata attributes to each
node class: add OUTPUT_NODE (set to False), DESCRIPTION (brief one-line
description specific to the node) and IS_CHANGED (set to False) to
AutoGrabCutRemover, GrabCutRefinement, TransparencyBackgroundRemover, and
TransparencyBackgroundRemoverBatch; place them alongside the existing
class-level constants (RETURN_TYPES, RETURN_NAMES, FUNCTION, CATEGORY) and make
DESCRIPTION a short human-readable string matching the class purpose (e.g.,
"Remove background using GrabCut" or "Refine GrabCut background mask") so
frontend panels and execution caching can use them.
---
Nitpick comments:
In @.github/workflows/comfy-ci.yml:
- Line 14: Update the GitHub Actions Python matrix to target supported versions
by keeping python-version: ["3.10", "3.11", "3.12"] and, optionally, bump the
setup action by updating the actions/setup-python usage from
actions/setup-python@v4 to actions/setup-python@v5 to stay current; ensure the
workflow syntax remains valid after the change.
In `@grabcut_nodes.py`:
- Around line 803-806: Replace the broad except Exception with a specific catch
for image-processing/fallback errors: change the handler around the per-item
refine block to "except (cv2.error, MemoryError, ValueError) as e" (and add
"import cv2" at the top if missing), keep the existing
log.error("grabcut_node.refine_error", item=i, error=str(e)) and the fallback
appends (refined_images.append(image[i]); refined_masks.append(mask[i])), and
allow other unexpected exceptions to propagate instead of being swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9ebe5e7-00ce-4527-af16-1ce9be0388c8
📒 Files selected for processing (2)
.github/workflows/comfy-ci.ymlgrabcut_nodes.py
PR #15 shipped four NameError-class bugs that crash at call time but slip past import-only CI: validate_node_params, log, _log_gpu_memory, and invert_mask were referenced without being defined. This commit: - Adds schemas.py with Pydantic v2 NodeParams (ranges mirror INPUT_TYPES) and a dataclass fallback so minimal envs still validate. - Adds a structlog-or-stdlib logging guard and _log_gpu_memory helper at the top of grabcut_nodes.py; the stdlib adapter preserves exc_info semantics. - Exposes invert_mask on both AutoGrabCutRemover and GrabCutRefinement — INPUT_TYPES, function signature, and applied to the alpha channel in every success/fallback path. - Fixes torch.stack shape mismatch in GrabCutRefinement.refine_mask: all branches now run through the same resize so batches can stack when output_size != ORIGINAL. - Narrows the validation exception handler to (ValidationError, ValueError, TypeError). - Adds OUTPUT_NODE, DESCRIPTION, and IS_CHANGED (deterministic sha256 hash over scalar inputs) to all four node classes. - Adds test_node_smoke.py with 9 tests that actually invoke each node's FUNCTION, plus parameter validation and regression tests for the invert_mask / resize-stack bugs. - Wires pytest test_node_smoke.py into CI and drops continue-on-error from flake8 so real syntax/name errors now block. - Fixes __init__.py relative imports so pytest can collect tests whose parent dir name isn't a valid Python identifier. - Adds pydantic>=2.0 and structlog>=23.0 to requirements.txt.
- Ignore .claude/, .kilo/, .memory/, kilo.json — IDE/CLI caches that shouldn't be shared. .kilo/ alone was 58MB of tool state. - Ignore node.zip — generated by `comfy node publish`, not source. Remove existing node.zip (8.6MB) from the working tree. It wasn't tracked, but it was sitting at the repo root cluttering listings.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grabcut_nodes.py (1)
853-938:⚠️ Potential issue | 🟡 MinorAsymmetric validation:
refine_maskskipsvalidate_node_params.
AutoGrabCutRemover.remove_backgroundroutes every configurable scalar throughvalidate_node_params(good), butrefine_maskpassesgrabcut_iterations,edge_refinement,edge_blur_amount,expand_margin,bbox_safety_margin,min_bbox_sizestraight to the processor. In the ComfyUI UI path this is fine because INPUT_TYPES enforces ranges, but the new smoke tests exerciserefine_maskdirectly — so a caller passinggrabcut_iterations=999here would silently propagate where it would be rejected on the sibling node.Not a blocker (defense-in-depth), but worth either validating symmetrically or adding a
# intentionally trusts UI-side INPUT_TYPES rangescomment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 853 - 938, refine_mask currently accepts numeric params (grabcut_iterations, edge_refinement, edge_blur_amount, expand_margin, bbox_safety_margin, min_bbox_size) and forwards them to self.processor without using the same validation used by AutoGrabCutRemover.remove_background; add a call to the shared validation routine (validate_node_params) at the start of refine_mask (or explicitly validate those parameters) before assigning to self.processor, referencing the same parameter names (grabcut_iterations, edge_refinement, edge_blur_amount, expand_margin, bbox_safety_margin, min_bbox_size) so out-of-range values are rejected consistently; alternatively, if you intend to rely on UI enforcement, add a clear inline comment in refine_mask stating it intentionally trusts INPUT_TYPES ranges.
🧹 Nitpick comments (7)
test_node_smoke.py (1)
233-264: Nice regression test — but tighten the resize assertion.The comment says "aspect-preserving resize" and the fixtures are square (128×128), so the output is deterministically 512×512 for that input, not merely ≤512. A stricter assertion would catch a regression where one axis stays at 128 because a branch fell through to the original-size tensor (which is exactly the bug this test is guarding against).
♻️ Suggested tightening
- # Aspect-preserving resize: output fits within 512 on both axes. - assert image_out.shape[1] <= 512 and image_out.shape[2] <= 512 - assert mask_out.shape[1] <= 512 and mask_out.shape[2] <= 512 + # Square input → square output; both items must match exactly. + assert image_out.shape[1] == image_out.shape[2] + assert mask_out.shape[1:] == image_out.shape[1:3] + assert image_out.shape[1] <= 512🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_node_smoke.py` around lines 233 - 264, The assertions that image_out and mask_out dimensions are ≤512 are too loose for the square 128×128 fixtures; tighten them to assert exact 512×512 output to catch the branch that could preserve an original-sized tensor. In test_grabcut_refinement_resize_stacks_uniformly, after calling node.refine_mask (with output_size="512x512"), replace the two checks that use <=512 on image_out.shape[1]/[2] and mask_out.shape[1]/[2] with equality checks ==512 so both height and width are asserted to be exactly 512.grabcut_nodes.py (1)
23-32: Triple-fallback is a bit defensive for a first-party module.
schemas.pyis shipped in this repo, so the outermostexcept ImportErrorshould really only fire if someone deletes the file. The intermediate relative/absolute fallback is all you need. If you want to keep the belt-and-suspenders guard, at least log a warning on the terminal fallback so silent skips of validation don't go unnoticed in prod.♻️ Suggested tweak
except ImportError: # pragma: no cover validate_node_params = None ValidationError = ValueError # type: ignore[misc,assignment] + # schemas is a first-party module; falling through here means the + # package install is broken. Surface it rather than silently skipping + # parameter validation. + import warnings + warnings.warn("schemas.py not importable; parameter validation disabled", RuntimeWarning)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grabcut_nodes.py` around lines 23 - 32, The current triple-fallback import for ValidationError and validate_node_params is overly defensive; change the import to try the relative import first and fall back to the absolute import only (remove the outermost catch that silently disables validation), and when the absolute fallback is used emit a warning (e.g., via logging.warning) so silent skips are visible; only if both imports fail should you set validate_node_params = None and ValidationError = ValueError, and in that final case also log an error/warning. Target the top-level import block that references validate_node_params and ValidationError to implement these changes.schemas.py (1)
19-20: Document that the allowedoutput_formatset is grabcut-specific.
_VALID_OUTPUT_FORMAT = {"RGBA", "MASK"}matchesAutoGrabCutRemover's dropdown, butTransparencyBackgroundRemoverinnodes.pyusesRGB_WITH_MASK(notMASK). If someone later reusesvalidate_node_paramsfrom nodes.py, valid UI values will be rejected. A brief module-/field-level comment ("Allowed values mirror the GrabCut-family INPUT_TYPES") would prevent that footgun.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas.py` around lines 19 - 20, Add a brief module- or field-level comment above _VALID_OUTPUT_FORMAT explaining that this set is GrabCut-specific (mirrors the GrabCut-family INPUT_TYPES used by AutoGrabCutRemover) and does not cover other remover types like TransparencyBackgroundRemover which uses RGB_WITH_MASK; also mention that validate_node_params (in nodes.py) may need adjustment if reused for non-GrabCut removers. This documents the scope of _VALID_OUTPUT_FORMAT and prevents future misuses without changing behavior.nodes.py (1)
10-22: DRY:_is_changed_hashis duplicated verbatim ingrabcut_nodes.py(lines 94–106).Same body, same semantics. When you tweak the hash contract (e.g., to also summarise tensor-valued kwargs — see below), you'll have to remember two places. Extract into a small helper module (e.g.
comfy_hash.pyor add toschemas.py) and import from both.Also worth noting: this helper only summarises the positional
imageby shape/dtype; any other tensor that arrives via**kwargs(e.g.,maskinGrabCutRefinement.IS_CHANGED) is fed throughrepr(), which for a large torch tensor is both non-cheap and not guaranteed stable across torch versions. Consider summarising any value with a.shapeattribute the same wayimageis summarised.♻️ Sketch of the shared helper
# e.g. in a new _hash_utils.py import hashlib def is_changed_hash(image=None, **kwargs) -> str: m = hashlib.sha256() for key in sorted(kwargs): val = kwargs[key] if hasattr(val, "shape") and hasattr(val, "dtype"): m.update(f"{key}=shape={tuple(val.shape)},dtype={val.dtype};".encode()) else: m.update(f"{key}={val!r};".encode()) if image is not None and hasattr(image, "shape"): m.update(f"image_shape={tuple(image.shape)};image_dtype={getattr(image, 'dtype', None)};".encode()) return m.hexdigest()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes.py` around lines 10 - 22, The duplicated function _is_changed_hash (present in nodes.py and grabcut_nodes.py) should be extracted into a single helper (e.g., comfy_hash.py or added to schemas.py) and both modules should import it; rename it to a clear public name like is_changed_hash and update callers (e.g., GrabCutRefinement.IS_CHANGED and any uses in nodes.py) to import that helper. Change the implementation to iterate sorted kwargs and, for each value, if it has shape and dtype summarize as shape/dtype instead of using repr(), otherwise fallback to repr(); keep the existing special handling for the positional image argument (shape/dtype) and return the sha256 hex digest. Ensure imports are updated in both modules to remove the duplicate definitions..github/workflows/comfy-ci.yml (3)
27-28: Redundant explicit installs.
scikit-learn,pydantic, andstructlogare already pinned inrequirements.txtper the relevant-snippets context. Onlypytestis actually needed here; dropping the rest keeps dependency sources single-sourced and avoids masking a future requirements-file regression.♻️ Proposed cleanup
pip install -r requirements.txt - pip install scikit-learn pydantic structlog pytest + pip install pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/comfy-ci.yml around lines 27 - 28, The workflow currently runs "pip install -r requirements.txt" and then explicitly reinstalls "scikit-learn", "pydantic", and "structlog" alongside "pytest"; remove the redundant explicit installs (scikit-learn, pydantic, structlog) from the second pip install invocation and keep only "pytest" so that dependency management remains single-sourced (look for the lines with the two pip install commands).
53-55: Consider failing CI if the smoke-test file is missing.
python -m pytest test_node_smoke.py -vexits 5 when no tests are collected (e.g., the file was renamed/deleted) and pytest ≥6 treats that as failure by default, which is what you want here. Worth adding--import-mode=importlibif the repo path ever gets Python-unfriendly (hyphenated dir) to match the__init__.pyfallback path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/comfy-ci.yml around lines 53 - 55, Update the CI step named "Run smoke tests (invocation, not just import)" to invoke pytest with explicit import-mode so the run fails if the smoke-test file is missing and handles hyphenated paths; specifically, change the run command that currently calls "python -m pytest test_node_smoke.py -v" to include "--import-mode=importlib" (i.e., "python -m pytest test_node_smoke.py -v --import-mode=importlib") so pytest ≥6 will exit non‑zero when no tests are collected and import path issues are avoided.
20-20: Bumpactions/setup-pythonto v6.
actions/setup-python@v4is outdated. v6 is the current major version and a drop-in replacement with improved .python-version parsing, PyPy support, and security updates. Requires GitHub Actions runners v2.327.1+ (standard GitHub-hosted runners already meet this requirement).♻️ Proposed bump
- uses: actions/setup-python@v4 + uses: actions/setup-python@v6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/comfy-ci.yml at line 20, Update the GitHub Actions step that references actions/setup-python by changing the version tag from v4 to v6; locate the workflow step that uses "uses: actions/setup-python@v4" and replace it with "uses: actions/setup-python@v6" to take advantage of the newer parsing, PyPy support, and security fixes (ensure runner compatibility v2.327.1+ if you have custom runners).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@schemas.py`:
- Around line 134-138: The range check loop in schemas.py can raise TypeError
for non-numeric values (e.g., iterations="bogus"); update the logic in the block
that iterates _RANGES so that for each name in cleaned you first validate that v
is a numeric type (use numbers.Real or isinstance(v, (int, float))) and raise a
ValueError with a clear message for non-numeric inputs, then perform the
existing range check (lo <= v <= hi) to raise ValueError when out of bounds; add
the necessary import (numbers) and refer to the symbols _RANGES and cleaned (and
the validate_node_params caller) when making the change.
In `@test_node_smoke.py`:
- Line 111: The assertion in test_node_smoke.py mixes and/or without parentheses
so isinstance(report, str) doesn't guard the second membership check; update the
assert that references report to parenthesize the or portion so it reads: first
ensure isinstance(report, str) and then check that either "Batch" or "Total" is
in report (i.e., assert isinstance(report, str) and ("Batch" in report or
"Total" in report)) to avoid TypeError for non-string reports.
---
Outside diff comments:
In `@grabcut_nodes.py`:
- Around line 853-938: refine_mask currently accepts numeric params
(grabcut_iterations, edge_refinement, edge_blur_amount, expand_margin,
bbox_safety_margin, min_bbox_size) and forwards them to self.processor without
using the same validation used by AutoGrabCutRemover.remove_background; add a
call to the shared validation routine (validate_node_params) at the start of
refine_mask (or explicitly validate those parameters) before assigning to
self.processor, referencing the same parameter names (grabcut_iterations,
edge_refinement, edge_blur_amount, expand_margin, bbox_safety_margin,
min_bbox_size) so out-of-range values are rejected consistently; alternatively,
if you intend to rely on UI enforcement, add a clear inline comment in
refine_mask stating it intentionally trusts INPUT_TYPES ranges.
---
Nitpick comments:
In @.github/workflows/comfy-ci.yml:
- Around line 27-28: The workflow currently runs "pip install -r
requirements.txt" and then explicitly reinstalls "scikit-learn", "pydantic", and
"structlog" alongside "pytest"; remove the redundant explicit installs
(scikit-learn, pydantic, structlog) from the second pip install invocation and
keep only "pytest" so that dependency management remains single-sourced (look
for the lines with the two pip install commands).
- Around line 53-55: Update the CI step named "Run smoke tests (invocation, not
just import)" to invoke pytest with explicit import-mode so the run fails if the
smoke-test file is missing and handles hyphenated paths; specifically, change
the run command that currently calls "python -m pytest test_node_smoke.py -v" to
include "--import-mode=importlib" (i.e., "python -m pytest test_node_smoke.py -v
--import-mode=importlib") so pytest ≥6 will exit non‑zero when no tests are
collected and import path issues are avoided.
- Line 20: Update the GitHub Actions step that references actions/setup-python
by changing the version tag from v4 to v6; locate the workflow step that uses
"uses: actions/setup-python@v4" and replace it with "uses:
actions/setup-python@v6" to take advantage of the newer parsing, PyPy support,
and security fixes (ensure runner compatibility v2.327.1+ if you have custom
runners).
In `@grabcut_nodes.py`:
- Around line 23-32: The current triple-fallback import for ValidationError and
validate_node_params is overly defensive; change the import to try the relative
import first and fall back to the absolute import only (remove the outermost
catch that silently disables validation), and when the absolute fallback is used
emit a warning (e.g., via logging.warning) so silent skips are visible; only if
both imports fail should you set validate_node_params = None and ValidationError
= ValueError, and in that final case also log an error/warning. Target the
top-level import block that references validate_node_params and ValidationError
to implement these changes.
In `@nodes.py`:
- Around line 10-22: The duplicated function _is_changed_hash (present in
nodes.py and grabcut_nodes.py) should be extracted into a single helper (e.g.,
comfy_hash.py or added to schemas.py) and both modules should import it; rename
it to a clear public name like is_changed_hash and update callers (e.g.,
GrabCutRefinement.IS_CHANGED and any uses in nodes.py) to import that helper.
Change the implementation to iterate sorted kwargs and, for each value, if it
has shape and dtype summarize as shape/dtype instead of using repr(), otherwise
fallback to repr(); keep the existing special handling for the positional image
argument (shape/dtype) and return the sha256 hex digest. Ensure imports are
updated in both modules to remove the duplicate definitions.
In `@schemas.py`:
- Around line 19-20: Add a brief module- or field-level comment above
_VALID_OUTPUT_FORMAT explaining that this set is GrabCut-specific (mirrors the
GrabCut-family INPUT_TYPES used by AutoGrabCutRemover) and does not cover other
remover types like TransparencyBackgroundRemover which uses RGB_WITH_MASK; also
mention that validate_node_params (in nodes.py) may need adjustment if reused
for non-GrabCut removers. This documents the scope of _VALID_OUTPUT_FORMAT and
prevents future misuses without changing behavior.
In `@test_node_smoke.py`:
- Around line 233-264: The assertions that image_out and mask_out dimensions are
≤512 are too loose for the square 128×128 fixtures; tighten them to assert exact
512×512 output to catch the branch that could preserve an original-sized tensor.
In test_grabcut_refinement_resize_stacks_uniformly, after calling
node.refine_mask (with output_size="512x512"), replace the two checks that use
<=512 on image_out.shape[1]/[2] and mask_out.shape[1]/[2] with equality checks
==512 so both height and width are asserted to be exactly 512.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 141b7cbb-85b9-4ab8-b9c4-1cd958a7d298
📒 Files selected for processing (8)
.github/workflows/comfy-ci.yml.gitignore__init__.pygrabcut_nodes.pynodes.pyrequirements.txtschemas.pytest_node_smoke.py
✅ Files skipped from review due to trivial changes (2)
- requirements.txt
- .gitignore
| for name, (lo, hi) in _RANGES.items(): | ||
| if name in cleaned: | ||
| v = cleaned[name] | ||
| if v < lo or v > hi: | ||
| raise ValueError(f"{name} must be in [{lo}, {hi}], got {v}") |
There was a problem hiding this comment.
Fallback range check TypeErrors on non-numeric input.
If a caller passes iterations="bogus" under the dataclass fallback, v < lo raises TypeError rather than a clean ValueError. In practice, grabcut_nodes.py catches (ValidationError, ValueError, TypeError), so it does surface as "Invalid parameters". Just calling it out — if you ever use validate_node_params from another caller that only catches ValidationError, you'll see an unintended uncaught TypeError.
🛡 Safer alternative
for name, (lo, hi) in _RANGES.items():
if name in cleaned:
v = cleaned[name]
+ if not isinstance(v, (int, float)) or isinstance(v, bool):
+ raise ValueError(f"{name} must be numeric, got {type(v).__name__}")
if v < lo or v > hi:
raise ValueError(f"{name} must be in [{lo}, {hi}], got {v}")📝 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.
| for name, (lo, hi) in _RANGES.items(): | |
| if name in cleaned: | |
| v = cleaned[name] | |
| if v < lo or v > hi: | |
| raise ValueError(f"{name} must be in [{lo}, {hi}], got {v}") | |
| for name, (lo, hi) in _RANGES.items(): | |
| if name in cleaned: | |
| v = cleaned[name] | |
| if not isinstance(v, (int, float)) or isinstance(v, bool): | |
| raise ValueError(f"{name} must be numeric, got {type(v).__name__}") | |
| if v < lo or v > hi: | |
| raise ValueError(f"{name} must be in [{lo}, {hi}], got {v}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas.py` around lines 134 - 138, The range check loop in schemas.py can
raise TypeError for non-numeric values (e.g., iterations="bogus"); update the
logic in the block that iterates _RANGES so that for each name in cleaned you
first validate that v is a numeric type (use numbers.Real or isinstance(v, (int,
float))) and raise a ValueError with a clear message for non-numeric inputs,
then perform the existing range check (lo <= v <= hi) to raise ValueError when
out of bounds; add the necessary import (numbers) and refer to the symbols
_RANGES and cleaned (and the validate_node_params caller) when making the
change.
| ) | ||
| assert image_out.shape[0] == 2 | ||
| assert mask_out.shape[0] == 2 | ||
| assert isinstance(report, str) and "Batch" in report or "Total" in report |
There was a problem hiding this comment.
Parenthesize the mixed and/or (Ruff RUF021).
As written, Python evaluates this as (isinstance(report, str) and "Batch" in report) or ("Total" in report), so the isinstance guard doesn't actually protect the second in check — a non-string report would raise TypeError instead of failing the assertion cleanly. Today batch_remove_background always returns a str so it doesn't bite, but the intent is clearly "string AND (Batch OR Total)".
🛠 Proposed fix
- assert isinstance(report, str) and "Batch" in report or "Total" in report
+ assert isinstance(report, str) and ("Batch" in report or "Total" in report)📝 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.
| assert isinstance(report, str) and "Batch" in report or "Total" in report | |
| assert isinstance(report, str) and ("Batch" in report or "Total" in report) |
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 111-111: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_node_smoke.py` at line 111, The assertion in test_node_smoke.py mixes
and/or without parentheses so isinstance(report, str) doesn't guard the second
membership check; update the assert that references report to parenthesize the
or portion so it reads: first ensure isinstance(report, str) and then check that
either "Batch" or "Total" is in report (i.e., assert isinstance(report, str) and
("Batch" in report or "Total" in report)) to avoid TypeError for non-string
reports.
- Add Pydantic parameter validation (src/validation.py) - Add structured logging with structlog fallback - Add IS_CHANGED, OUTPUT_NODE, DESCRIPTION metadata to all nodes - Add comprehensive smoke and unit tests - Fix invert_mask double-application bug - Fix create_fallback_processor()() double call - Resolve merge conflicts in .gitignore, grabcut_nodes.py, nodes.py, requirements.txt - Remove FLOYO_INTEGRATION_GUIDE.md, schemas.py (superseded) - Update CI: Python 3.10-3.12, pytest smoke tests
Limbicnation
left a comment
Limbicnation
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment — 1 critical issue, 1 warning, 2 suggestions
🔴 Critical
- schemas.py vs src/validation.py duplication —
schemas.py(143 lines) is a near-duplicate ofsrc/validation.py(148 lines). Both defineNodeParams/GrabCutNodeParamswith overlapping field sets. The PR addsschemas.pybut the repo already hassrc/validation.pywith more comprehensive composed models (GrabCutParams,ScalingParams,MaskParams,BBoxParams). The smoke test imports fromschemas, butgrabcut_nodes.pyimports fromsrc.validation. This creates two sources of truth for the same validation logic.
Fix: Removeschemas.pyand updatetest_node_smoke.pyto import fromsrc.validation.
⚠️ Warnings
- GrabCutRefinement._initialize_processor() line 829:
create_fallback_processor(iterations=3)— this passesiterationsas a keyword arg to what may be a factory function. The original code hadcreate_fallback_processor()(iterations=3)which was also wrong (double call). Need to verify this actually works.
💡 Suggestions
- CI workflow — The
Run smoke testsstep runspython -m pytest test_node_smoke.py -vbuttest_node_smoke.pyimports fromschemaswhich will fail onceschemas.pyis removed. Update the test import to usesrc.validation. - init.py import fallback — The nested
try/except ImportErrorblocks for relative→absolute imports are correct but add complexity. Consider documenting when each path is hit (ComfyUI package context vs pytest standalone).
✅ Looks Good
- All 9 smoke tests pass (
pytest test_node_smoke.py -x) OUTPUT_NODE,DESCRIPTION,IS_CHANGEDmetadata correctly added to all 4 node classesinvert_maskbug properly fixed (single inversion in RGBA buffer)- CI updated to Python 3.10-3.12 with blocking flake8 on syntax errors
- No debug prints, secrets, or conflict markers found
Reviewed by Hermes Agent
Inline Review Detailsschemas.py (new file, lines 1-143)🔴 Critical: Duplicates Fix: Remove grabcut_nodes.py:829
test_node_smoke.py:48💡 Suggestion: .github/workflows/comfy-ci.yml:49-51💡 Suggestion: The smoke test step runs |
- Remove duplicate schemas.py (near-duplicate of src/validation.py) - Update test_node_smoke.py to import from src.validation - All 9 smoke tests pass
1 participant
…uard, CI Python versions
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests