Hardening/v1.3.1#9
Conversation
… mode handling Changes: - Add [tool.ruff] config with target-version py310 and a curated rule set (E/F/W, I, UP, B, SIM, RUF) so future drift is caught in CI lint. - PEP 585 sweep across all node modules: drop legacy typing.Dict / List / Tuple / Optional in favor of dict / list / tuple / `X | None`. Annotate class-level mutable defaults as ClassVar to satisfy RUF012. - PromptCombinerNode: replace the magic-string mode chain with a CombineMode StrEnum + match statement. The dropdown choices in INPUT_TYPES are now derived from the same Literal alias used in the function signature, so the UI and the type contract can't drift apart. - Smoke tests for PromptCombinerNode (14 tests) covering enum mapping, all three modes, edge cases, and the unknown-mode error path. Brings combiner coverage from 28% to 96%. - Tidy preexisting issues surfaced by the new lint rules: B904 except chaining in style_presets, RUF013 implicit Optional, RUF059 unused unpack, SIM117 nested-with consolidation in tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ests)
Direct response to the v1.1.6 packaging gap, where the prompt-chain nodes
(combiner, refiner, negative) were authored under nodes/ but never wired
into NODE_CLASS_MAPPINGS, so they silently failed to ship.
- __init__.py now raises RuntimeError at import time if NODE_CLASS_MAPPINGS
and NODE_DISPLAY_NAME_MAPPINGS drift apart. ComfyUI startup logs surface
the mismatch instead of dropping the node from the menu silently.
- New tests/unit/test_node_registration.py contract suite (8 tests):
* Expected 5 keys are registered.
* Display names cover every class.
* Each registered class satisfies the ComfyUI node interface
(INPUT_TYPES classmethod, RETURN_TYPES, FUNCTION, CATEGORY,
and the FUNCTION attribute resolves to a callable method).
* Every nodes/*_node.py module on disk is reachable from the mapping —
this is the regression guard that would have caught v1.1.6.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Direct response to the v1.1.x → v1.3.0 silent-skip pattern, where pyproject.toml was bumped four times without ever pushing the matching git tag, so .github/workflows/publish.yml never fired and the registry stayed on v1.1.6. - scripts/check_version_tag.py: stdlib-only (tomllib) script that fails if pyproject.toml's version has no matching git tag locally. - .pre-commit-config.yaml: wires ruff (check + format) and the local version-tag-guard hook. The guard fires only on pyproject.toml edits. - test.yml: new verify-tag-exists job that fails the main-branch build when pyproject.toml's version has no matching tag in the repo. Belt and suspenders for the pre-commit hook. - test.yml validate job: same comfy-cli telemetry fix already applied to publish.yml; suppresses the interactive opt-in prompt in CI. - pytest.ini + test.yml: lower coverage gate from 70 (never enforced; failed on every main push since chain nodes were added) to 50 (the actual current floor). Comment marks it as a starting point for future test growth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Hardening release. No public API changes — same 5 NODE_CLASS_MAPPINGS keys as v1.3.0. The bump exists so the registry can serve a build that includes the version-tag guard, the registration contract tests, the mapping drift self-check, the modernized typing, and the CI fixes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request modernizes type annotations across the codebase to use PEP 604 builtin unions and generics instead of the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -297,9 +290,9 @@ def generate_subprocess( | |||
| except FileNotFoundError: | |||
There was a problem hiding this comment.
🔥 The Roast: {e!s} is Python 2 carryover. The !s format specifier was necessary in Python 2 to force string representation, but in Python 3 every value is already a string when formatted. You're essentially writing str(str(e)) with extra steps.
🩹 The Fix:
| except FileNotFoundError: | |
| return (False, f"Error: {e}") |
📏 Severity: nitpick
Code Review Roast 🔥Verdict: 1 Issue Found | Recommendation: Merge (fix the nitpick first) Overview
🏆 Best part: This PR is doing real work. The node registration contract tests ( 💀 Worst part: 📊 Overall: Like finding a $100 bill in your winter coat — the coverage drop was embarrassing, but this PR is mostly a net positive. Merge it, but clean up that one nitpick on the way out. Files Reviewed (18 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m2.5-20260211 · 139,318 tokens |
![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 Ruff for linting and formatting, adds a pre-commit hook to ensure git tags match version bumps, and refactors several nodes to use modern Python type hints (PEP 585/604). It also includes contract tests for node registration and a refactored PromptCombinerNode. Feedback identifies critical compatibility issues for Python 3.10 users due to the use of StrEnum and tomllib, which were introduced in Python 3.11. Additionally, the move to instance-level caching in OllamaClient is noted as ineffective because the client is re-instantiated on every call, preventing the cache from persisting.
| from enum import StrEnum | ||
| from typing import Any, Literal, get_args | ||
|
|
||
|
|
||
| class CombineMode(StrEnum): | ||
| """Supported strategies for combining multiple prompts.""" | ||
|
|
||
| BLEND = "blend" | ||
| CONCAT = "concat" | ||
| WEIGHTED_AVERAGE = "weighted_average" |
There was a problem hiding this comment.
StrEnum was introduced in Python 3.11. Since the project's pyproject.toml specifies requires-python = ">=3.10", using StrEnum will cause an ImportError at runtime for users on Python 3.10. To maintain compatibility with Python 3.10, you can inherit from both str and Enum instead.
| from enum import StrEnum | |
| from typing import Any, Literal, get_args | |
| class CombineMode(StrEnum): | |
| """Supported strategies for combining multiple prompts.""" | |
| BLEND = "blend" | |
| CONCAT = "concat" | |
| WEIGHTED_AVERAGE = "weighted_average" | |
| from enum import Enum | |
| from typing import Any, Literal, get_args | |
| class CombineMode(str, Enum): | |
| """Supported strategies for combining multiple prompts.""" | |
| BLEND = "blend" | |
| CONCAT = "concat" | |
| WEIGHTED_AVERAGE = "weighted_average" |
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import tomllib |
There was a problem hiding this comment.
tomllib is only available in the standard library starting with Python 3.11. As the project targets Python 3.10, this script will fail with an ImportError when run in a 3.10 environment. Consider using a fallback to tomli (the backport of tomllib) or an alternative parsing method to ensure the pre-commit hook remains functional for all supported Python versions.
| import tomllib | |
| try: | |
| import tomllib | |
| except ImportError: | |
| import tomli as tomllib |
| self.logger_prefix = logger_prefix | ||
| # Instance-level cache (was class-level, causing race conditions) | ||
| self._cached_models: Optional[List[str]] = None | ||
| self._cached_models: list[str] | None = None |
There was a problem hiding this comment.
The model cache is currently instance-level (self._cached_models), but OllamaClient is instantiated fresh inside the generate and discover_models methods of the nodes (e.g., PromptGeneratorNode.generate). This means the cache will not persist between node executions, effectively disabling the caching mechanism. To fix this while avoiding race conditions, consider using class-level attributes protected by a class-level threading.Lock.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nodes/prompt_combiner_node.py (1)
18-20: ⚡ Quick winGuard against drift between
CombineModeandModeLiteral
CombineModeandModeLiteralcurrently duplicate the same mode values. A small import-time consistency check would prevent subtle UI/type drift if one side changes later.Suggested patch
# Single source of truth for the choices exposed in INPUT_TYPES and accepted by combine(). ModeLiteral = Literal["blend", "concat", "weighted_average"] +MODE_CHOICES = tuple(m.value for m in CombineMode) +assert set(get_args(ModeLiteral)) == set(MODE_CHOICES) @@ "mode": ( - list(get_args(ModeLiteral)), + list(MODE_CHOICES), {"default": CombineMode.BLEND.value}, ),Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/prompt_combiner_node.py` around lines 18 - 20, Add an import-time consistency assertion to ensure CombineMode and ModeLiteral remain in sync: compare the set/list of mode values from the CombineMode enum (or class) with the tuple/list used to build ModeLiteral and raise/assert a clear error if they differ; update the check near where ModeLiteral is defined (and repeat for the other duplicate at the 45-46 region) so that INPUT_TYPES and the combine() method rely on a single canonical source and fail fast if the two definitions diverge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nodes/adapters/ollama_client.py`:
- Line 130: The current is_loaded check in ollama_client.py (variable is_loaded)
treats any model that startswith the base family as loaded, causing false
positives across tagged variants; change the logic so entries in running_models
that include an explicit tag (contain ":") require exact equality (model == rm),
while only untagged running_models (no ":") may match any variant by comparing
base names (compare model.split(":")[0] to rm). Update the is_loaded expression
to first detect whether rm contains ":" and branch to exact match for tagged
entries and base-name match only for untagged entries.
In `@nodes/prompt_combiner_node.py`:
- Around line 6-15: The file imports StrEnum which is only available in Python
3.11; replace it with the Python 3.10-compatible pattern by importing Enum from
enum and change the CombineMode declaration to use the str + Enum mixin (i.e.,
class CombineMode(str, Enum):) while keeping the existing members BLEND, CONCAT,
WEIGHTED_AVERAGE and the docstring; remove the StrEnum import and leave other
imports (Any, Literal, get_args) as-is so behavior remains identical on Python
3.10.
---
Nitpick comments:
In `@nodes/prompt_combiner_node.py`:
- Around line 18-20: Add an import-time consistency assertion to ensure
CombineMode and ModeLiteral remain in sync: compare the set/list of mode values
from the CombineMode enum (or class) with the tuple/list used to build
ModeLiteral and raise/assert a clear error if they differ; update the check near
where ModeLiteral is defined (and repeat for the other duplicate at the 45-46
region) so that INPUT_TYPES and the combine() method rely on a single canonical
source and fail fast if the two definitions diverge.
🪄 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: 981620e0-b4f3-49f4-9070-2b1892ed40c1
📒 Files selected for processing (18)
.github/workflows/test.yml.pre-commit-config.yaml__init__.pynodes/adapters/ollama_client.pynodes/negative_prompt_node.pynodes/prompt_combiner_node.pynodes/prompt_generator_node.pynodes/prompt_refiner_node.pynodes/style_applier_node.pypyproject.tomlpytest.iniscripts/check_version_tag.pystyle_presets.pytests/unit/test_extract_final_prompt.pytests/unit/test_node_registration.pytests/unit/test_ollama_client.pytests/unit/test_prompt_combiner.pytests/unit/test_style_presets.py
| model == rm or model.startswith(rm.split(":")[0]) | ||
| for rm in running_models | ||
| ) | ||
| is_loaded = any(model == rm or model.startswith(rm.split(":")[0]) for rm in running_models) |
There was a problem hiding this comment.
Model-loaded detection can return false positives across tags
model.startswith(rm.split(":")[0]) marks different variants of the same base family as “loaded” (e.g., qwen3:8b vs qwen3:14b), which can misreport health status.
Suggested fix
- is_loaded = any(model == rm or model.startswith(rm.split(":")[0]) for rm in running_models)
+ requested_name, _, requested_tag = model.partition(":")
+ is_loaded = any(
+ rm == model
+ or (
+ not requested_tag
+ and rm.partition(":")[0] == requested_name
+ )
+ for rm in running_models
+ )📝 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.
| is_loaded = any(model == rm or model.startswith(rm.split(":")[0]) for rm in running_models) | |
| requested_name, _, requested_tag = model.partition(":") | |
| is_loaded = any( | |
| rm == model | |
| or ( | |
| not requested_tag | |
| and rm.partition(":")[0] == requested_name | |
| ) | |
| for rm in running_models | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/adapters/ollama_client.py` at line 130, The current is_loaded check in
ollama_client.py (variable is_loaded) treats any model that startswith the base
family as loaded, causing false positives across tagged variants; change the
logic so entries in running_models that include an explicit tag (contain ":")
require exact equality (model == rm), while only untagged running_models (no
":") may match any variant by comparing base names (compare model.split(":")[0]
to rm). Update the is_loaded expression to first detect whether rm contains ":"
and branch to exact match for tagged entries and base-name match only for
untagged entries.
| from enum import StrEnum | ||
| from typing import Any, Literal, get_args | ||
|
|
||
|
|
||
| class CombineMode(StrEnum): | ||
| """Supported strategies for combining multiple prompts.""" | ||
|
|
||
| BLEND = "blend" | ||
| CONCAT = "concat" | ||
| WEIGHTED_AVERAGE = "weighted_average" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow Python versions:"
fd -H -I '.*\.ya?ml' .github/workflows | xargs -r rg -n 'python-version|matrix|3\.10' -C2
echo
echo "Remaining StrEnum usages in Python files:"
rg -n '\bStrEnum\b' --type pyRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 1701
Replace StrEnum with str + Enum mixin for Python 3.10 compatibility
StrEnum was introduced in Python 3.11 and is not available in Python 3.10, which is the target version in your CI workflows. This causes an ImportError when running tests. Use the str mixin with Enum instead to maintain equivalent behavior while supporting Python 3.10.
Suggested patch
-from enum import StrEnum
+from enum import Enum
from typing import Any, Literal, get_args
-class CombineMode(StrEnum):
+class CombineMode(str, Enum):
"""Supported strategies for combining multiple prompts."""🧰 Tools
🪛 GitHub Actions: Test
[error] 6-6: Test collection failed: ImportError: cannot import name 'StrEnum' from 'enum' (Python 3.10). The file attempts from enum import StrEnum, but StrEnum is not available in this Python version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/prompt_combiner_node.py` around lines 6 - 15, The file imports StrEnum
which is only available in Python 3.11; replace it with the Python
3.10-compatible pattern by importing Enum from enum and change the CombineMode
declaration to use the str + Enum mixin (i.e., class CombineMode(str, Enum):)
while keeping the existing members BLEND, CONCAT, WEIGHTED_AVERAGE and the
docstring; remove the StrEnum import and leave other imports (Any, Literal,
get_args) as-is so behavior remains identical on Python 3.10.
- fix(scripts): tomllib fallback for Python 3.10 compat - fix(ci): remove verify-tag-exists from test.yml (publish.yml already guards) - fix(tests): ensure local nodes/ import in test_node_registration.py - fix(adapter): shared module-level model cache across OllamaClient instances - fix(adapter): exact model tag match in check_health() to avoid prefix conflation - fix(refiner): per-pass seed increment so multi-pass refinement varies - feat(generator): strip DeepSeek <think> and markdown <details> blocks - test: add prompt_refiner seed tests, cache sharing tests, exact match tests - chore(deps): add dev extras with tomli fallback
- fix(scripts): tomllib fallback for Python 3.10 compat - fix(ci): remove verify-tag-exists from test.yml (publish.yml already guards) - fix(tests): ensure local nodes/ import in test_node_registration.py - fix(adapter): shared module-level model cache across OllamaClient instances - fix(adapter): exact model tag match in check_health() to avoid prefix conflation - fix(refiner): per-pass seed increment so multi-pass refinement varies - feat(generator): strip DeepSeek <think> and markdown <details> blocks - test: add prompt_refiner seed tests, cache sharing tests, exact match tests - chore(deps): add dev extras with tomli fallback
- fix(scripts): tomllib fallback for Python 3.10 compat - fix(ci): remove verify-tag-exists from test.yml (publish.yml already guards) - fix(tests): ensure local nodes/ import in test_node_registration.py - fix(adapter): shared module-level model cache across OllamaClient instances - fix(adapter): exact model tag match in check_health() to avoid prefix conflation - fix(refiner): per-pass seed increment so multi-pass refinement varies - feat(generator): strip DeepSeek <think> and markdown <details> blocks - test: add prompt_refiner seed tests, cache sharing tests, exact match tests - chore(deps): add dev extras with tomli fallback
- fix(scripts): tomllib fallback for Python 3.10 compat - fix(ci): remove verify-tag-exists from test.yml (publish.yml already guards) - fix(tests): ensure local nodes/ import in test_node_registration.py - fix(adapter): shared module-level model cache across OllamaClient instances - fix(adapter): exact model tag match in check_health() to avoid prefix conflation - fix(refiner): per-pass seed increment so multi-pass refinement varies - fix(combiner): validate mode before single-prompt fast path - feat(generator): strip DeepSeek <think> and markdown <details> blocks - test: add prompt_refiner seed tests, cache sharing tests, exact match tests - chore(deps): add dev extras with tomli fallback
1 participant
Summary by CodeRabbit
New Features
Tests
Chores