Pr 7#8
Conversation
Phase 1 — Foundation Hardening:
- Extract ollama_client.py adapter from PromptGeneratorNode
- Add pytest suite (29 tests, 70% coverage gate)
- Unify style sources into config/styles.yaml
- Extend CI with pytest + coverage
Phase 2 — Prompt Chain Nodes:
- PromptRefinerNode: iterative LLM refinement (1-3 passes)
- NegativePromptNode: style-aware negative prompt generation
- PromptCombinerNode: blend/concat/weighted_average modes
New graph target:
PromptGenerator -> PromptRefiner -> PromptCombiner -> CLIP
^
NegativePrompt ---------+
Closes roadmap phases 1.1-2.3
- Remove dead imports from prompt_generator_node.py and ollama_client.py - Switch optional imports to importlib.util.find_spec pattern - Remove orphaned _cached_models/_cache_time from PromptGeneratorNode - Remove unused seed param from PromptRefinerNode - Add top_p input to NegativePromptNode with proper wiring - Fix pytest.ini to not omit adapters from coverage - Remove unused pytest imports from test files - Run ruff check + format (all clean now) - All 29 tests passing
📝 WalkthroughWalkthroughAdds an Ollama adapter, three new ComfyUI nodes (refiner, negative-prompt, combiner), centralizes style presets in YAML, refactors the prompt generator to use the adapter, introduces unit tests and pytest configuration, and adds a CI job enforcing 70% coverage. Changes
Sequence DiagramsequenceDiagram
participant UI as ComfyUI (Executor)
participant PG as PromptGeneratorNode
participant OC as OllamaClient
participant OS as Ollama Server
UI->>PG: generate(prompt, style, model, ...)
PG->>OC: create_progress_bar(unique_id)
OC-->>PG: pbar?
PG->>OC: check_health(model)
OC->>OS: ollama.ps()
OS-->>OC: loaded_models
OC-->>PG: (healthy, msg, is_loaded)
alt Ollama available & model loaded
PG->>OC: generate_streaming(model, prompt, ...)
OC->>OS: ollama.generate(..., stream=True)
loop chunk
OS-->>OC: chunk
OC->>OC: accumulate text
OC->>pbar: update progress
end
OC-->>PG: generated_text
else Streaming timeout/error or not loaded
PG->>OC: generate_subprocess(model, prompt, ...)
OC->>OC: subprocess.run(['ollama', 'run', ...])
OC-->>PG: (success, output_text)
end
PG->>PG: extract_final_prompt(output)
PG-->>UI: final_prompt
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. 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 the PromptRefiner, NegativePrompt, and PromptCombiner nodes, refactors Ollama integration into a dedicated adapter, and adds a YAML-based style system and unit tests. Feedback highlights a resource inefficiency in the streaming implementation due to excessive thread creation and provides suggestions to simplify redundant import logic and improve weighting granularity in the prompt combiner.
| t = threading.Thread(target=_iter_next, args=(it,), daemon=True) | ||
| t.start() |
There was a problem hiding this comment.
Creating a new thread for every single chunk in the streaming loop is highly inefficient and can lead to significant resource overhead or exhaustion, especially with long-form LLM responses that generate hundreds of tokens. A better approach would be to use a single background worker thread and a queue to handle the stream, allowing you to enforce timeouts on the queue consumption rather than spawning threads repeatedly.
| try: | ||
| import ollama | ||
| import importlib.util | ||
|
|
||
| OLLAMA_API_AVAILABLE = True | ||
| except ImportError: | ||
| OLLAMA_API_AVAILABLE = False | ||
| if importlib.util.find_spec("ollama") is not None: | ||
| OLLAMA_API_AVAILABLE = True | ||
| except Exception: | ||
| pass | ||
|
|
||
| try: | ||
| import comfy.utils | ||
| import importlib.util | ||
|
|
||
| COMFY_PROGRESS_AVAILABLE = True | ||
| except ImportError: | ||
| COMFY_PROGRESS_AVAILABLE = False | ||
| if importlib.util.find_spec("comfy") is not None: | ||
| COMFY_PROGRESS_AVAILABLE = True | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The logic for checking the availability of ollama and comfy is redundant and can be simplified. You are importing importlib.util twice and performing identical checks in separate blocks.
| try: | |
| import ollama | |
| import importlib.util | |
| OLLAMA_API_AVAILABLE = True | |
| except ImportError: | |
| OLLAMA_API_AVAILABLE = False | |
| if importlib.util.find_spec("ollama") is not None: | |
| OLLAMA_API_AVAILABLE = True | |
| except Exception: | |
| pass | |
| try: | |
| import comfy.utils | |
| import importlib.util | |
| COMFY_PROGRESS_AVAILABLE = True | |
| except ImportError: | |
| COMFY_PROGRESS_AVAILABLE = False | |
| if importlib.util.find_spec("comfy") is not None: | |
| COMFY_PROGRESS_AVAILABLE = True | |
| except Exception: | |
| pass | |
| try: | |
| import importlib.util | |
| OLLAMA_API_AVAILABLE = importlib.util.find_spec("ollama") is not None | |
| COMFY_PROGRESS_AVAILABLE = importlib.util.find_spec("comfy") is not None | |
| except Exception: | |
| pass |
Code Review Roast 🔥Verdict: All Previous Issues Resolved | Recommendation: Merge Well, would you look at that. A follow-up commit actually addressed the issues raised in the original review. I was fully prepared to write another scathing roast about nothing changing, but here we are. Issues From Previous Review — All Fixed ✅
Incremental Changes (commit
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
.github/workflows/test.yml (1)
43-49: Optional: enable pip caching to speed up CI.
actions/setup-python@v5supports built-in pip caching via thecache: 'pip'input, which would noticeably reduce the install time ofpytest,pytest-cov,pyyaml, andjinja2on each run.♻️ Proposed change
- name: Set up Python uses: actions/setup-python@v5 with: python-version: '3.10' + cache: 'pip' - name: Install dependencies run: | pip install pytest pytest-cov pyyaml jinja2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 43 - 49, Update the "Install dependencies" job step to enable pip caching via the existing setup action: when using actions/setup-python@v5 add the input cache: 'pip' (or configure the setup-python step that precedes the "Install dependencies" step to include cache: 'pip') so pip dependencies (pytest, pytest-cov, pyyaml, jinja2) are cached between runs and CI install time is reduced; ensure the "Install dependencies" and "Run unit tests" steps remain unchanged except for enabling the cache on the setup action.__init__.py (1)
6-18: Broadexcept ImportErrorcan mask real import bugs in sub-modules.If any of the imported modules (e.g.,
prompt_refiner_node) raises anImportErrorinternally — say, due to a missing optional dep, a typo, or a transitive failure — thisexceptwill swallow the original error, retry the same import via the absolute path, and surface a less helpful "no module namednodes.*" or a duplicate failure. Consider narrowing toImportErrorraised specifically by the relative form, e.g. by checking__package__, so genuine errors propagate:♻️ Proposed refactor
-try: - from .nodes.prompt_generator_node import PromptGeneratorNode - from .nodes.style_applier_node import StyleApplierNode - from .nodes.prompt_refiner_node import PromptRefinerNode - from .nodes.negative_prompt_node import NegativePromptNode - from .nodes.prompt_combiner_node import PromptCombinerNode -except ImportError: - # Fallback for test environments where ComfyUI isn't present - from nodes.prompt_generator_node import PromptGeneratorNode - from nodes.style_applier_node import StyleApplierNode - from nodes.prompt_refiner_node import PromptRefinerNode - from nodes.negative_prompt_node import NegativePromptNode - from nodes.prompt_combiner_node import PromptCombinerNode +if __package__: + from .nodes.prompt_generator_node import PromptGeneratorNode + from .nodes.style_applier_node import StyleApplierNode + from .nodes.prompt_refiner_node import PromptRefinerNode + from .nodes.negative_prompt_node import NegativePromptNode + from .nodes.prompt_combiner_node import PromptCombinerNode +else: + # Fallback for test environments where this is loaded as a top-level module. + from nodes.prompt_generator_node import PromptGeneratorNode + from nodes.style_applier_node import StyleApplierNode + from nodes.prompt_refiner_node import PromptRefinerNode + from nodes.negative_prompt_node import NegativePromptNode + from nodes.prompt_combiner_node import PromptCombinerNode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__init__.py` around lines 6 - 18, The broad except ImportError in __init__.py around importing PromptGeneratorNode, StyleApplierNode, PromptRefinerNode, NegativePromptNode, and PromptCombinerNode can hide real import errors from those modules; change the handler to catch the ImportError as e and only perform the fallback absolute imports when the error is specifically about the relative package (e.g., inspect e.name or the exception message for the 'nodes' package or check __package__), otherwise re-raise the original ImportError so genuine internal import problems in prompt_refiner_node (or others) surface.nodes/prompt_generator_node.py (2)
27-44: Refactor: deduplicateimport importlib.utiland consolidate the availability probes.
importlib.utilis the same stdlib module being imported twice in adjacenttryblocks. It can be imported once at module scope, then bothfind_specchecks reduced to a single straight-line block — easier to read and one fewer pyflakes/ruff false-positive trigger surface.♻️ Proposed cleanup
-OLLAMA_API_AVAILABLE = False -COMFY_PROGRESS_AVAILABLE = False - -try: - import importlib.util - - if importlib.util.find_spec("ollama") is not None: - OLLAMA_API_AVAILABLE = True -except Exception: - pass - -try: - import importlib.util - - if importlib.util.find_spec("comfy") is not None: - COMFY_PROGRESS_AVAILABLE = True -except Exception: - pass +import importlib.util + + +def _module_available(name: str) -> bool: + try: + return importlib.util.find_spec(name) is not None + except (ImportError, ValueError): + return False + + +OLLAMA_API_AVAILABLE = _module_available("ollama") +COMFY_PROGRESS_AVAILABLE = _module_available("comfy")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/prompt_generator_node.py` around lines 27 - 44, Import importlib.util once at module scope and remove the duplicate try blocks; then set OLLAMA_API_AVAILABLE and COMFY_PROGRESS_AVAILABLE by calling importlib.util.find_spec("ollama") and importlib.util.find_spec("comfy") in a single, simple block (wrap the checks in one try/except if you still want to suppress import errors). Update the existing symbols OLLAMA_API_AVAILABLE and COMFY_PROGRESS_AVAILABLE accordingly and remove the second importlib.util import and its try/except to eliminate duplication.
84-84: Remove unused constant —CHUNK_TIMEOUThas been moved toOllamaClient.
CHUNK_TIMEOUTis defined inOllamaClientand used internally there, but the duplicate definition inPromptGeneratorNodeis unreferenced. Remove it to avoid maintenance confusion.♻️ Proposed cleanup
- CHUNK_TIMEOUT = 30 - # Default style templates (fallback when YAML not available) DEFAULT_STYLES = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/prompt_generator_node.py` at line 84, Remove the unused duplicate constant CHUNK_TIMEOUT from PromptGeneratorNode: delete the CHUNK_TIMEOUT declaration in the PromptGeneratorNode module (it is now defined and used inside OllamaClient) so the code no longer contains an unreferenced constant and avoids maintenance confusion with the OllamaClient.CHUNK_TIMEOUT definition.nodes/prompt_combiner_node.py (1)
169-187:_blenduses coarse bracket tiers and ignores theseparatorparameter; consider ComfyUI's native(text:weight)syntax for higher fidelity.The current tier mapping (
>=1.5 → ((..)),>=1.2 → (..),<=0.5 → [..], else passthrough) discards weight precision: e.g.,weight=0.6,0.9, and1.0all collapse to bare text, and1.21and1.49both produce(text). ComfyUI natively supports(text:weight)which preserves the exact weight value. Additionally, the user-suppliedseparatorparameter (line 128) is intentionally not used by_blend; it always joins with hard-coded", "while_concatrespects the separator. Consider updating the docstring to clarify this behavior.♻️ Optional refactor
def _blend(self, prompts: List[Tuple[str, float]]) -> str: """ - Blend prompts using ComfyUI-style emphasis markers. - Higher weight = more parentheses emphasis. + Blend prompts using ComfyUI's (text:weight) emphasis syntax. + Note: always joins with ", "; the `separator` argument is not used here. """ parts = [] for text, weight in prompts: if weight <= 0: continue - # Map weight to emphasis levels - if weight >= 1.5: - parts.append(f"(({text}))") - elif weight >= 1.2: - parts.append(f"({text})") - elif weight <= 0.5: - parts.append(f"[{text}]") - else: - parts.append(text) + if abs(weight - 1.0) < 1e-3: + parts.append(text) + else: + parts.append(f"({text}:{weight:.2f})") return ", ".join(parts)🤖 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 169 - 187, The _blend method currently collapses continuous weights into coarse bracket tiers and ignores the user-supplied separator; update _blend(prompts: List[Tuple[str, float]]) to emit ComfyUI-style precision by formatting each non‑zero weight as "(text:weight)" (preserving the exact float) and join the parts using the existing separator parameter (used by _concat) instead of the hard-coded ", ". Also update the _blend docstring to state it uses ComfyUI "(text:weight)" syntax and that weights <= 0 are skipped.nodes/negative_prompt_node.py (2)
115-127: Docstring is missingtop_p.
top_pis in the signature but not in the Args section.📝 Proposed doc fix
Args: prompt: Positive prompt string style: Style category for style-aware negative hints model: Ollama model to use temperature: Generation temperature (lower = more conservative) + top_p: Nucleus sampling parameter timeout: Maximum generation time🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/negative_prompt_node.py` around lines 115 - 127, The docstring for the "Generate a negative prompt from a positive prompt" function is missing the top_p parameter description; update the Args section in the function's docstring (the one whose signature includes top_p, temperature, timeout, etc.) to add a brief line for top_p (e.g., "top_p: Nucleus sampling probability for generation, between 0 and 1 (lower = more conservative).") so it documents the parameter meaning and expected range alongside temperature and timeout.
33-43: Style key duplication withPromptGeneratorNode.DEFAULT_STYLESinvites drift.
STYLE_HINTSkeys must stay in sync with the styles defined innodes/prompt_generator_node.py:DEFAULT_STYLES(and the upcomingconfig/styles.yaml). When a new style is added there, this dict will silently fall through to the generic default at line 132 and produce style-blind negatives. Consider sourcing the style list from the same YAML/registry, or at least adding a one-time consistency assertion at import / via a unit test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/negative_prompt_node.py` around lines 33 - 43, STYLE_HINTS currently hardcodes style keys that can drift from PromptGeneratorNode.DEFAULT_STYLES; instead, either build STYLE_HINTS programmatically from PromptGeneratorNode.DEFAULT_STYLES (or from the shared styles YAML/registry) so the keys stay in sync, or add a one-time import-time assertion that set(STYLE_HINTS.keys()) == set(PromptGeneratorNode.DEFAULT_STYLES) and raise a clear error if they differ; update the module to reference PromptGeneratorNode.DEFAULT_STYLES when constructing or validating STYLE_HINTS and add a targeted unit test to catch future mismatches.tests/unit/test_ollama_client.py (1)
13-72: Use a pytest fixture for shared setup/teardown to prevent state bleed.Three issues with the current pattern:
Test isolation gap in
test_default_models_when_api_unavailable(lines 13–18): It does not resetOllamaClient._cached_models/_cache_timebefore running. The implementation checks the cache beforeOLLAMA_API_AVAILABLE(seeollama_client.pylines 63–67), so if a previously-run test left a recent cache populated, this test would return cached models instead ofDEFAULT_MODELS, causing a flaky failure depending on test execution order.Direct module-attribute mutation (e.g.,
client_module.ollama = mock_ollamaat lines 34, 60, 98, 121) is brittle. Preferunittest.mock.patch.object(client_module, "ollama", mock_ollama)andpatch.object(client_module, "OLLAMA_API_AVAILABLE", True)so cleanup is automatic on exception too.Heavy boilerplate duplicated across 4 tests — extract an autouse fixture that resets the class-level cache and patches the ollama module/availability flag.
♻️ Sketch of fixture-based refactor
import pytest from unittest.mock import patch, MagicMock import nodes.adapters.ollama_client as client_module from nodes.adapters.ollama_client import OllamaClient `@pytest.fixture`(autouse=True) def reset_cache(): OllamaClient._cached_models = None OllamaClient._cache_time = 0 yield OllamaClient._cached_models = None OllamaClient._cache_time = 0 `@pytest.fixture` def mock_ollama_available(): mock_ollama = MagicMock() with patch.object(client_module, "ollama", mock_ollama), \ patch.object(client_module, "OLLAMA_API_AVAILABLE", True): yield mock_ollama🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_ollama_client.py` around lines 13 - 72, Add an autouse pytest fixture to reset OllamaClient._cached_models and _cache_time before and after each test and replace manual try/finally mutations with patch.object usage: ensure tests stop directly assigning client_module.ollama or client_module.OLLAMA_API_AVAILABLE and instead use a fixture (e.g., mock_ollama_available) that patches client_module.ollama to a MagicMock and client_module.OLLAMA_API_AVAILABLE to True for the test scope; also ensure the default-models test explicitly clears the cache (via the autouse reset_cache) so discover_models will return DEFAULT_MODELS when OLLAMA_API_AVAILABLE is False.nodes/prompt_refiner_node.py (1)
121-152: Subprocess failure on pass N>1 discards earlier successful refinement.If pass 1 succeeds and pass 2's streaming returns
Noneand the subprocess fallback fails, line 140 returns a generic failure string and throws away the already-improvedcurrent_promptfrom pass 1. Consider returning the best-so-far prompt with a logged warning instead, since the user is better served by the partially-refined output than by an error string that breaks the downstream graph.♻️ Suggested behavior
if output is None: # Fallback to subprocess success, output = client.generate_subprocess(model, refinement, timeout) if not success: - return (f"[PromptRefiner] Pass {i + 1} failed: {output}",) + print( + f"[PromptRefiner] Pass {i + 1} failed ({output}); " + f"returning best-so-far after {i} pass(es)" + ) + return (current_prompt,)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/prompt_refiner_node.py` around lines 121 - 152, The loop in the PromptRefiner (inside the method that runs passes) discards earlier successful refinements when a later pass's streaming output is None and the subprocess fallback (client.generate_subprocess) fails; instead of returning a generic failure tuple there, change the failure path to log a warning and return the best-so-far current_prompt (i.e., keep current_prompt from previous successful passes) so downstream graph receives the partial refinement; update the failure branch that currently does "if not success: return (f'[PromptRefiner] Pass {i + 1} failed: {output}',)" to log the error (using your logger/print) and return (current_prompt,) while preserving existing variables like current_prompt, client.generate_streaming, client.generate_subprocess, and extract_final_prompt.nodes/adapters/ollama_client.py (1)
174-220: Daemon thread can outlivegenerate_streamingand keep consuming the upstream iterator.When the chunk-wait times out (line 190–193) the function returns
None, but the daemon threadtkeeps blocking onnext(it)against the underlying ollama HTTP stream. It will eventually finish or die at process exit, but until then it holds the connection and a reference toresult_holder. Under repeated timeouts (e.g., a stuck Ollama server) this can pile up threads/connections within a single ComfyUI process.Two pragmatic mitigations: (a) attempt to close the stream when it exposes a
close()method before returning, and/or (b) add a shorttime.sleepand re-checkt.is_alive()once before bailing. Acceptable to defer, but worth noting since this runs on the request thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/adapters/ollama_client.py` around lines 174 - 220, In generate_streaming, when the waiter thread t (target _iter_next reading from iterator it) times out and t.is_alive() is true, close or drain the upstream iterator before returning: check if the iterator object it has a close() or aaclose() attribute and call it inside a try/except to avoid raising; if no close exists, sleep a short interval (e.g., time.sleep(0.1)) and attempt to join again for a brief timeout to let the thread finish; only then proceed to log and return None. Ensure you reference the existing names (generate_streaming, _iter_next, it, t, result_holder) and keep the thread daemonization as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/styles.yaml`:
- Around line 1-3: Both _get_style_list() and _load_templates() in
nodes/prompt_generator_node.py currently hardcode reading config/templates.yaml,
so the new config/styles.yaml (with 9 style presets and the category field) is
never loaded; update both functions to open and parse config/styles.yaml instead
of config/templates.yaml and ensure they read the style entries (including the
new category metadata) so those styles are returned by _get_style_list() and
included by _load_templates() for UI exposure.
In `@docs/ROADMAP.md`:
- Around line 27-31: Add language identifiers to the two fenced code blocks that
are triggering markdownlint MD040: update the block that begins with
"[PromptGenerator] → [PromptRefiner] → [PromptCombiner] → CLIP Text Encode" to
start with ```text and similarly update the block that begins with "Phase 1
(foundation) ──→ Phase 2 (chain nodes)" to start with ```text so both fenced
code blocks include a language tag and satisfy the linter.
In `@nodes/adapters/ollama_client.py`:
- Around line 110-126: The health-check logic around the ollama.ps() response
uses model.startswith(rm.split(":")[0]) which causes false positives for
different-size siblings (e.g., "qwen3:8b" vs "qwen3:4b"); update the is_loaded
check to compare base names by equality instead of startswith: compute
model_base = model.split(":")[0] and rm_base = rm.split(":")[0] and use
any(model == rm or model_base == rm_base for rm in running_models) when
determining is_loaded so you only match on exact base-name equality.
- Around line 48-93: The class-level cache is accidentally being shadowed by
instance attributes because discover_models assigns to self._cached_models and
self._cache_time, so the cache is effectively per-instance; either make the
cache truly class-scoped by assigning to the class attributes (use
OllamaClient._cached_models and OllamaClient._cache_time when updating) inside
discover_models (and keep the class-level declaration and comment), or
explicitly convert the design to an instance cache by removing the class-level
declarations and initializing self._cached_models = None and self._cache_time =
0.0 inside __init__ (and update the comment); update the logging in
_log/discover_models as needed to reflect which scope you chose.
In `@nodes/prompt_combiner_node.py`:
- Around line 194-219: The current _weighted_average builds repeated entries
then deduplicates them (seen/result), which collapses repeats and makes it
behave exactly like _concat; fix by emitting an explicit weighted token instead
of deduping: in _weighted_average produce entries in parts as
f"({text}:{weight})" (or similar ComfyUI weight syntax) and skip the dedup logic
so higher-weight prompts appear with their weight preserved; keep the
total_weight==0 fallback to join raw texts, and update any tests that compare
_weighted_average vs _concat to expect the weighted syntax.
In `@pytest.ini`:
- Around line 9-18: The coverage sections in pytest.ini ([coverage:run] and
[coverage:report]) are ignored by coverage.py; move the configuration into a
proper coverage config file or explicitly tell pytest-cov to load pytest.ini.
Fix by either creating a .coveragerc containing the [run] and [report] keys and
values (source, omit, fail_under, show_missing, skip_covered) and removing the
coverage sections from pytest.ini, or update the test invocation (CI/workflow)
to pass --cov-config=pytest.ini so show_missing and omit patterns are honored;
reference the pytest.ini coverage sections, the show_missing/omit settings, and
fail_under when applying the change.
In `@tests/unit/test_extract_final_prompt.py`:
- Around line 15-17: The test asserts None->None but extract_final_prompt is
typed as def extract_final_prompt(text: str) -> str; fix by removing or changing
the test in tests/unit/test_extract_final_prompt.py (delete test_none_input) and
update behavior in nodes/prompt_generator_node.py: keep the signature
extract_final_prompt(text: str) -> str and add a simple defensive guard at the
top of extract_final_prompt that returns "" for falsy or non-str inputs (so
callers and type hints remain consistent while the function safely handles None
in practice).
In `@tests/unit/test_style_presets.py`:
- Around line 82-88: The test test_render_template_no_emphasis_mood in
PromptGeneratorNode should assert both unwanted fragments are absent rather than
using an or; update the assertion against node._render_template("cinematic", "a
mountain") so it requires that "Focus particularly on:" is not in result AND
"None" is not in result (or use two separate assert statements) to ensure
neither substring appears in the rendered template.
---
Nitpick comments:
In `@__init__.py`:
- Around line 6-18: The broad except ImportError in __init__.py around importing
PromptGeneratorNode, StyleApplierNode, PromptRefinerNode, NegativePromptNode,
and PromptCombinerNode can hide real import errors from those modules; change
the handler to catch the ImportError as e and only perform the fallback absolute
imports when the error is specifically about the relative package (e.g., inspect
e.name or the exception message for the 'nodes' package or check __package__),
otherwise re-raise the original ImportError so genuine internal import problems
in prompt_refiner_node (or others) surface.
In @.github/workflows/test.yml:
- Around line 43-49: Update the "Install dependencies" job step to enable pip
caching via the existing setup action: when using actions/setup-python@v5 add
the input cache: 'pip' (or configure the setup-python step that precedes the
"Install dependencies" step to include cache: 'pip') so pip dependencies
(pytest, pytest-cov, pyyaml, jinja2) are cached between runs and CI install time
is reduced; ensure the "Install dependencies" and "Run unit tests" steps remain
unchanged except for enabling the cache on the setup action.
In `@nodes/adapters/ollama_client.py`:
- Around line 174-220: In generate_streaming, when the waiter thread t (target
_iter_next reading from iterator it) times out and t.is_alive() is true, close
or drain the upstream iterator before returning: check if the iterator object it
has a close() or aaclose() attribute and call it inside a try/except to avoid
raising; if no close exists, sleep a short interval (e.g., time.sleep(0.1)) and
attempt to join again for a brief timeout to let the thread finish; only then
proceed to log and return None. Ensure you reference the existing names
(generate_streaming, _iter_next, it, t, result_holder) and keep the thread
daemonization as-is.
In `@nodes/negative_prompt_node.py`:
- Around line 115-127: The docstring for the "Generate a negative prompt from a
positive prompt" function is missing the top_p parameter description; update the
Args section in the function's docstring (the one whose signature includes
top_p, temperature, timeout, etc.) to add a brief line for top_p (e.g., "top_p:
Nucleus sampling probability for generation, between 0 and 1 (lower = more
conservative).") so it documents the parameter meaning and expected range
alongside temperature and timeout.
- Around line 33-43: STYLE_HINTS currently hardcodes style keys that can drift
from PromptGeneratorNode.DEFAULT_STYLES; instead, either build STYLE_HINTS
programmatically from PromptGeneratorNode.DEFAULT_STYLES (or from the shared
styles YAML/registry) so the keys stay in sync, or add a one-time import-time
assertion that set(STYLE_HINTS.keys()) ==
set(PromptGeneratorNode.DEFAULT_STYLES) and raise a clear error if they differ;
update the module to reference PromptGeneratorNode.DEFAULT_STYLES when
constructing or validating STYLE_HINTS and add a targeted unit test to catch
future mismatches.
In `@nodes/prompt_combiner_node.py`:
- Around line 169-187: The _blend method currently collapses continuous weights
into coarse bracket tiers and ignores the user-supplied separator; update
_blend(prompts: List[Tuple[str, float]]) to emit ComfyUI-style precision by
formatting each non‑zero weight as "(text:weight)" (preserving the exact float)
and join the parts using the existing separator parameter (used by _concat)
instead of the hard-coded ", ". Also update the _blend docstring to state it
uses ComfyUI "(text:weight)" syntax and that weights <= 0 are skipped.
In `@nodes/prompt_generator_node.py`:
- Around line 27-44: Import importlib.util once at module scope and remove the
duplicate try blocks; then set OLLAMA_API_AVAILABLE and COMFY_PROGRESS_AVAILABLE
by calling importlib.util.find_spec("ollama") and
importlib.util.find_spec("comfy") in a single, simple block (wrap the checks in
one try/except if you still want to suppress import errors). Update the existing
symbols OLLAMA_API_AVAILABLE and COMFY_PROGRESS_AVAILABLE accordingly and remove
the second importlib.util import and its try/except to eliminate duplication.
- Line 84: Remove the unused duplicate constant CHUNK_TIMEOUT from
PromptGeneratorNode: delete the CHUNK_TIMEOUT declaration in the
PromptGeneratorNode module (it is now defined and used inside OllamaClient) so
the code no longer contains an unreferenced constant and avoids maintenance
confusion with the OllamaClient.CHUNK_TIMEOUT definition.
In `@nodes/prompt_refiner_node.py`:
- Around line 121-152: The loop in the PromptRefiner (inside the method that
runs passes) discards earlier successful refinements when a later pass's
streaming output is None and the subprocess fallback
(client.generate_subprocess) fails; instead of returning a generic failure tuple
there, change the failure path to log a warning and return the best-so-far
current_prompt (i.e., keep current_prompt from previous successful passes) so
downstream graph receives the partial refinement; update the failure branch that
currently does "if not success: return (f'[PromptRefiner] Pass {i + 1} failed:
{output}',)" to log the error (using your logger/print) and return
(current_prompt,) while preserving existing variables like current_prompt,
client.generate_streaming, client.generate_subprocess, and extract_final_prompt.
In `@tests/unit/test_ollama_client.py`:
- Around line 13-72: Add an autouse pytest fixture to reset
OllamaClient._cached_models and _cache_time before and after each test and
replace manual try/finally mutations with patch.object usage: ensure tests stop
directly assigning client_module.ollama or client_module.OLLAMA_API_AVAILABLE
and instead use a fixture (e.g., mock_ollama_available) that patches
client_module.ollama to a MagicMock and client_module.OLLAMA_API_AVAILABLE to
True for the test scope; also ensure the default-models test explicitly clears
the cache (via the autouse reset_cache) so discover_models will return
DEFAULT_MODELS when OLLAMA_API_AVAILABLE is False.
🪄 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: 3cf2b2c1-eba8-48a1-bbe2-cc738cd9a518
📒 Files selected for processing (14)
.github/workflows/test.yml.gitignore__init__.pyconfig/styles.yamldocs/ROADMAP.mdnodes/adapters/ollama_client.pynodes/negative_prompt_node.pynodes/prompt_combiner_node.pynodes/prompt_generator_node.pynodes/prompt_refiner_node.pypytest.initests/unit/test_extract_final_prompt.pytests/unit/test_ollama_client.pytests/unit/test_style_presets.py
| # ComfyUI-PromptGenerator Style Presets | ||
| # Single source of truth for all style definitions | ||
| # Format: YAML with Jinja2 template syntax |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git mv config/styles.yaml config/templates.yamlRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Confirm which YAML filename the node reads, and what's actually committed under config/.
fd -e yaml -e yml . config 2>/dev/null
echo "--- references in code ---"
rg -nP '(templates|styles)\.yaml' --type=pyRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 1008
🏁 Script executed:
cat config/styles.yamlRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 6168
🏁 Script executed:
cat config/templates.yaml | head -50Repository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 1866
🏁 Script executed:
sed -n '248-280p;365-385p' nodes/prompt_generator_node.pyRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 132
🏁 Script executed:
sed -n '248,280p' nodes/prompt_generator_node.pyRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 1340
🏁 Script executed:
sed -n '365,385p' nodes/prompt_generator_node.pyRepository: Limbicnation/ComfyUI-PromptGenerator
Length of output: 950
Critical: config/styles.yaml is never loaded—the node only reads config/templates.yaml, causing 6 of the 9 new style presets to be completely ignored at runtime.
Both _get_style_list() (line 252) and _load_templates() (line 367) in nodes/prompt_generator_node.py hardcode the path to config/templates.yaml. The new styles.yaml file adds 9 complete style definitions with a new category field, but only 3 of those styles exist in templates.yaml (cinematic, video_wan, still_image). The other 6 styles (anime, photorealistic, fantasy, abstract, cyberpunk, sci-fi) are silently dropped, and the category metadata is never exposed to the UI.
Align the loader with the file: update both functions to read config/styles.yaml instead:
Fix: Update loader paths in both functions
- template_path = Path(__file__).parent.parent / "config" / "templates.yaml"
+ template_path = Path(__file__).parent.parent / "config" / "styles.yaml"Apply in both _get_style_list() and _load_templates().
(Renaming the file is not feasible—templates.yaml already exists and cannot be overwritten.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/styles.yaml` around lines 1 - 3, Both _get_style_list() and
_load_templates() in nodes/prompt_generator_node.py currently hardcode reading
config/templates.yaml, so the new config/styles.yaml (with 9 style presets and
the category field) is never loaded; update both functions to open and parse
config/styles.yaml instead of config/templates.yaml and ensure they read the
style entries (including the new category metadata) so those styles are returned
by _get_style_list() and included by _load_templates() for UI exposure.
| ``` | ||
| [PromptGenerator] → [PromptRefiner] → [PromptCombiner] → CLIP Text Encode | ||
| ↑ | ||
| [NegativePrompt] ───────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint (MD040).
Both fences are missing a language tag, which will keep lint warnings active.
💡 Suggested fix
-```
+```text
[PromptGenerator] → [PromptRefiner] → [PromptCombiner] → CLIP Text Encode
↑
[NegativePrompt] ───────────┘@@
- +text
Phase 1 (foundation) ──→ Phase 2 (chain nodes)
│
Phase 3 (multi-backend) ──────┘
│
Phase 4 (intelligence) ←── requires Phase 3 protocol abstraction
│
Phase 5 (video/multimodal) ←── requires Phase 4 analyzer for quality scoring
Also applies to: 80-88
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ROADMAP.md` around lines 27 - 31, Add language identifiers to the two
fenced code blocks that are triggering markdownlint MD040: update the block that
begins with "[PromptGenerator] → [PromptRefiner] → [PromptCombiner] → CLIP Text
Encode" to start with ```text and similarly update the block that begins with
"Phase 1 (foundation) ──→ Phase 2 (chain nodes)" to start with ```text so both
fenced code blocks include a language tag and satisfy the linter.
| try: | ||
| ps_response = ollama.ps() | ||
| running_models = [m.model for m in ps_response.models] | ||
| is_loaded = any( | ||
| model == rm or model.startswith(rm.split(":")[0]) | ||
| for rm in running_models | ||
| ) | ||
| if is_loaded: | ||
| return (True, f"Model '{model}' is loaded in VRAM", True) | ||
| else: | ||
| return ( | ||
| True, | ||
| f"Model '{model}' not loaded (cold start expected)", | ||
| False, | ||
| ) | ||
| except Exception: | ||
| return (True, "Ollama running, model status unknown", False) |
There was a problem hiding this comment.
startswith match yields false positives for same-family different-size models.
is_loaded = any(
model == rm or model.startswith(rm.split(":")[0])
for rm in running_models
)For model="qwen3:8b" with a loaded model rm="qwen3:4b", rm.split(":")[0] == "qwen3" and "qwen3:8b".startswith("qwen3") is True — so the health check reports the 8b is "loaded in VRAM" when only the 4b is loaded, suppressing the cold-start warning the user actually needs. Compare base names with equality instead.
🐛 Proposed fix
- is_loaded = any(
- model == rm or model.startswith(rm.split(":")[0])
- for rm in running_models
- )
+ model_base = model.split(":")[0]
+ is_loaded = any(
+ model == rm or model_base == rm.split(":")[0]
+ 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` around lines 110 - 126, The health-check
logic around the ollama.ps() response uses model.startswith(rm.split(":")[0])
which causes false positives for different-size siblings (e.g., "qwen3:8b" vs
"qwen3:4b"); update the is_loaded check to compare base names by equality
instead of startswith: compute model_base = model.split(":")[0] and rm_base =
rm.split(":")[0] and use any(model == rm or model_base == rm_base for rm in
running_models) when determining is_loaded so you only match on exact base-name
equality.
| [coverage:run] | ||
| source = nodes | ||
| omit = | ||
| */tests/* | ||
| */__pycache__/* | ||
|
|
||
| [coverage:report] | ||
| fail_under = 70 | ||
| show_missing = True | ||
| skip_covered = False |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does coverage.py read configuration from pytest.ini by default, or only from .coveragerc, setup.cfg, tox.ini, and pyproject.toml?
💡 Result:
coverage.py does not read pytest.ini by default. By default it looks for coverage configuration in the .coveragerc file name (in the directory coverage.py is run from); if that doesn’t exist (and no --rcfile/ COV_RCFILE is provided), it searches other common coverage config files in this order: setup.cfg, tox.ini, or pyproject.toml (and it can also read pyproject.toml when TOML support is available).
Citations:
- 1: https://coverage.readthedocs.io/en/latest/config.html
- 2: https://github.com/nedbat/coveragepy/blob/7.5.1/doc/config.rst
- 3: https://coverage.readthedocs.io/en/6.5.0/config.html
Coverage configuration in pytest.ini will be silently ignored.
coverage.py does not read pytest.ini by default. It only auto-discovers configuration from .coveragerc, setup.cfg, tox.ini, or pyproject.toml. The [coverage:run] and [coverage:report] sections here have no effect unless you explicitly pass --cov-config=pytest.ini to pytest-cov. As a result, show_missing = True won't take effect and the omit patterns won't apply. The fail_under = 70 works only because it's duplicated on the CLI.
Choose one:
♻️ Option A — move coverage config to a dedicated .coveragerc
# .coveragerc
[run]
source = nodes
omit =
*/tests/*
*/__pycache__/*
[report]
fail_under = 70
show_missing = True
skip_covered = FalseRemove the coverage sections from pytest.ini:
[pytest]
minversion = 7.0
testpaths = tests
python_files = test_*.py
python_classes = Test*
python_functions = test_*
addopts = -v --tb=short
-
-[coverage:run]
-source = nodes
-omit =
- */tests/*
- */__pycache__/*
-
-[coverage:report]
-fail_under = 70
-show_missing = True
-skip_covered = False♻️ Option B — tell coverage to use pytest.ini explicitly
Update the workflow command:
- PYTHONPATH=. pytest tests/unit/ -v --cov=nodes --cov-fail-under=70
+ PYTHONPATH=. pytest tests/unit/ -v --cov=nodes --cov-config=pytest.ini --cov-report=term-missing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pytest.ini` around lines 9 - 18, The coverage sections in pytest.ini
([coverage:run] and [coverage:report]) are ignored by coverage.py; move the
configuration into a proper coverage config file or explicitly tell pytest-cov
to load pytest.ini. Fix by either creating a .coveragerc containing the [run]
and [report] keys and values (source, omit, fail_under, show_missing,
skip_covered) and removing the coverage sections from pytest.ini, or update the
test invocation (CI/workflow) to pass --cov-config=pytest.ini so show_missing
and omit patterns are honored; reference the pytest.ini coverage sections, the
show_missing/omit settings, and fail_under when applying the change.
| def test_none_input(self): | ||
| """None should return None.""" | ||
| assert extract_final_prompt(None) is None |
There was a problem hiding this comment.
Test contradicts the declared type contract of extract_final_prompt.
extract_final_prompt is annotated as def extract_final_prompt(text: str) -> str (see nodes/prompt_generator_node.py line 47), but this test pins None → None as expected behavior. Type checkers and downstream callers (e.g., nodes/negative_prompt_node.py already calls it as extract_final_prompt(output.strip()), always passing a str) will be misled. Pick one direction:
♻️ Option A — relax the annotation to reflect the implementation
In nodes/prompt_generator_node.py:
-def extract_final_prompt(text: str) -> str:
+def extract_final_prompt(text: Optional[str]) -> Optional[str]:♻️ Option B — drop the None-input test and have the function defend against falsy-non-string by returning ""
- def test_none_input(self):
- """None should return None."""
- assert extract_final_prompt(None) is None
+ def test_falsy_input_returns_empty(self):
+ """Falsy/empty inputs should normalize to an empty string."""
+ assert extract_final_prompt("") == ""…and in nodes/prompt_generator_node.py:
- if not text:
- return text
+ if not text:
+ return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_extract_final_prompt.py` around lines 15 - 17, The test
asserts None->None but extract_final_prompt is typed as def
extract_final_prompt(text: str) -> str; fix by removing or changing the test in
tests/unit/test_extract_final_prompt.py (delete test_none_input) and update
behavior in nodes/prompt_generator_node.py: keep the signature
extract_final_prompt(text: str) -> str and add a simple defensive guard at the
top of extract_final_prompt that returns "" for falsy or non-str inputs (so
callers and type hints remain consistent while the function safely handles None
in practice).
| def test_render_template_no_emphasis_mood(self): | ||
| """Template should render cleanly without emphasis or mood.""" | ||
| node = PromptGeneratorNode() | ||
| result = node._render_template("cinematic", "a mountain") | ||
| assert "a mountain" in result | ||
| # Emphasis/mood conditional blocks should be absent or minimal | ||
| assert "Focus particularly on:" not in result or "None" not in result |
There was a problem hiding this comment.
Weak assertion: or allows the failure case to pass.
assert A or B only fails when both A and B are False. Here it would fail only if "Focus particularly on:" is in result and "None" is in result. The test would silently pass even if "Focus particularly on: None" appeared just "Focus particularly on:" without "None" (e.g., if someone changes the template to render the literal "None" as ""). Use two assertions (or and) to actually express "render cleanly".
🐛 Proposed fix
- # Emphasis/mood conditional blocks should be absent or minimal
- assert "Focus particularly on:" not in result or "None" not in result
+ # Emphasis/mood conditional blocks should be absent
+ assert "Focus particularly on:" not in result
+ assert "None" not in result📝 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_render_template_no_emphasis_mood(self): | |
| """Template should render cleanly without emphasis or mood.""" | |
| node = PromptGeneratorNode() | |
| result = node._render_template("cinematic", "a mountain") | |
| assert "a mountain" in result | |
| # Emphasis/mood conditional blocks should be absent or minimal | |
| assert "Focus particularly on:" not in result or "None" not in result | |
| def test_render_template_no_emphasis_mood(self): | |
| """Template should render cleanly without emphasis or mood.""" | |
| node = PromptGeneratorNode() | |
| result = node._render_template("cinematic", "a mountain") | |
| assert "a mountain" in result | |
| # Emphasis/mood conditional blocks should be absent | |
| assert "Focus particularly on:" not in result | |
| assert "None" not in result |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_style_presets.py` around lines 82 - 88, The test
test_render_template_no_emphasis_mood in PromptGeneratorNode should assert both
unwanted fragments are absent rather than using an or; update the assertion
against node._render_template("cinematic", "a mountain") so it requires that
"Focus particularly on:" is not in result AND "None" is not in result (or use
two separate assert statements) to ensure neither substring appears in the
rendered template.
- Fix _weighted_average deduplication bug (now uses weight-ratio emphasis markers) - Replace bare except Exception with specific exception handling in OllamaClient - Propagate API contract mismatches (TypeError/AttributeError) as RuntimeError - Replace print() with logging.getLogger(__name__) in all new nodes - Expose top_p in PromptRefinerNode INPUT_TYPES for consistency - Update PR-8-REVIEW.md with fix log and merge recommendation
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 (2)
nodes/prompt_combiner_node.py (1)
117-227:⚠️ Potential issue | 🟠 MajorAdd direct unit tests for this node’s branch logic to unblock CI.
This class adds many decision paths (mode dispatch + weighting thresholds), and current pipeline reports 22% coverage. Please add focused tests for: no prompts, single prompt, each mode, unknown mode, and zero-weight edge cases.
I can draft a compact pytest table-driven suite for
combine,_blend, and_weighted_averageif you want.🤖 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 117 - 227, Add pytest unit tests exercising PromptCombiner.combine and its branches: write parametric tests that call combine with (1) no non-empty prompts to assert it returns the error tuple, (2) a single prompt to assert it returns the prompt unchanged, (3) mode="blend" with varying weights to cover parentheses, square-bracket and neutral cases (use _blend or call combine with mode="blend"), (4) mode="concat" to assert separator behavior, (5) mode="weighted_average" with zero total weight and with differing weights to hit triple/double/single/neutral/de-emphasis thresholds (call _weighted_average or combine with mode="weighted_average"), and (6) an unknown mode to assert the unknown-mode message; reference the methods combine, _blend, _concat, and _weighted_average when locating code and use parametrized pytest cases to compactly cover all branches including zero-weight edge cases.nodes/negative_prompt_node.py (1)
109-172:⚠️ Potential issue | 🟠 MajorAdd unit tests for
generate_negativebranch behavior to unblock CI.The pipeline is currently failing coverage threshold (40%). This method adds several branches (empty input, streaming fallback to subprocess, empty-cleaned-output fallback) that should be covered with targeted tests.
I can draft a compact pytest table for these branches if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/negative_prompt_node.py` around lines 109 - 172, Add pytest unit tests for NegativePromptNode.generate_negative covering: (1) empty prompt string returns the “[NegativePrompt] Please provide a positive prompt.” tuple; (2) successful streaming path where OllamaClient.generate_streaming returns a full string and extract_final_prompt returns cleaned text (assert returned tuple contains that text and logger path exercised); (3) streaming returns None and OllamaClient.generate_subprocess returns (True, output) to exercise subprocess fallback; and (4) LLM returns empty after cleaning so extract_final_prompt yields falsy and the method falls back to STYLE_HINTS for the given style. Use monkeypatch or unittest.mock to stub OllamaClient.generate_streaming, OllamaClient.generate_subprocess, and extract_final_prompt, and assert outputs reference NEGATIVE_PROMPT_TEMPLATE/STYLE_HINTS where applicable.
♻️ Duplicate comments (1)
nodes/adapters/ollama_client.py (1)
132-135:⚠️ Potential issue | 🟡 MinorUse base-name equality instead of
startswithfor loaded-model checks.Line 133 can mark
qwen3:8bas loaded when onlyqwen3:4bis running. Compare base names with equality to avoid false positives.🐛 Proposed fix
- is_loaded = any( - model == rm or model.startswith(rm.split(":")[0]) - for rm in running_models - ) + model_base = model.split(":")[0] + is_loaded = any( + model == rm or model_base == rm.split(":")[0] + 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` around lines 132 - 135, The is_loaded check currently treats a model as loaded if model == rm or model.startswith(rm.split(":")[0]), which causes false positives like marking "qwen3:8b" loaded when "qwen3:4b" is running; change the logic in the is_loaded comprehension to compare base names explicitly (split both model and rm on ":" and compare their base names for equality) so only models with the same base name are considered loaded, while still preserving the exact match fallback.
🧹 Nitpick comments (1)
nodes/prompt_refiner_node.py (1)
155-170: Avoid backward progress updates across refinement passes.Per-pass updates (Line 156) and chunk-level updates inside
generate_streamingcompete, so progress can jump backward between passes. Consider using only one progress strategy (either pass-level only, or pass-scaled chunk updates).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/prompt_refiner_node.py` around lines 155 - 170, The current code updates pbar twice causing backward jumps: once per pass (using i/passes and pbar.update_absolute) and again inside generate_streaming's chunk-level updates; pick one strategy and remove the conflicting update. Either remove the pass-level update (the block computing progress with i, passes and calling pbar.update_absolute) and let client.generate_streaming drive progress, or keep the pass-level scaling and modify generate_streaming calls to accept a per-pass progress range so it only reports within that segment; update references to pbar, generate_streaming, passes, i, and pbar.update_absolute 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 `@nodes/adapters/ollama_client.py`:
- Around line 159-167: The docstring for generate_streaming promises returning
None on failure but the implementation raises RuntimeError on errors (see the
exception block around ollama.generate() and timeout handling); update
generate_streaming to catch all operational exceptions (including timeouts and
any exceptions from ollama.generate()), log the error/context using the node's
logger, and return None instead of raising so callers relying on a None fallback
behave correctly; ensure the per-chunk and total timeout enforcement code still
cancels/cleans up resources before returning None.
In `@nodes/prompt_combiner_node.py`:
- Around line 176-187: The current loop drops entries where weight <= 0 which
causes an empty return when all weights are zero; modify the logic in the block
using variables weight, text, parts and the final return(", ".join(parts)) so
that either (a) only negative weights are skipped (keep weight == 0 as neutral
and append text normally) or (b) after the loop, if parts is empty, fall back to
appending all original texts (unmodified) before returning; update the condition
and/or add the fallback to ensure a non-empty string is returned when all blend
weights are 0.0.
---
Outside diff comments:
In `@nodes/negative_prompt_node.py`:
- Around line 109-172: Add pytest unit tests for
NegativePromptNode.generate_negative covering: (1) empty prompt string returns
the “[NegativePrompt] Please provide a positive prompt.” tuple; (2) successful
streaming path where OllamaClient.generate_streaming returns a full string and
extract_final_prompt returns cleaned text (assert returned tuple contains that
text and logger path exercised); (3) streaming returns None and
OllamaClient.generate_subprocess returns (True, output) to exercise subprocess
fallback; and (4) LLM returns empty after cleaning so extract_final_prompt
yields falsy and the method falls back to STYLE_HINTS for the given style. Use
monkeypatch or unittest.mock to stub OllamaClient.generate_streaming,
OllamaClient.generate_subprocess, and extract_final_prompt, and assert outputs
reference NEGATIVE_PROMPT_TEMPLATE/STYLE_HINTS where applicable.
In `@nodes/prompt_combiner_node.py`:
- Around line 117-227: Add pytest unit tests exercising PromptCombiner.combine
and its branches: write parametric tests that call combine with (1) no non-empty
prompts to assert it returns the error tuple, (2) a single prompt to assert it
returns the prompt unchanged, (3) mode="blend" with varying weights to cover
parentheses, square-bracket and neutral cases (use _blend or call combine with
mode="blend"), (4) mode="concat" to assert separator behavior, (5)
mode="weighted_average" with zero total weight and with differing weights to hit
triple/double/single/neutral/de-emphasis thresholds (call _weighted_average or
combine with mode="weighted_average"), and (6) an unknown mode to assert the
unknown-mode message; reference the methods combine, _blend, _concat, and
_weighted_average when locating code and use parametrized pytest cases to
compactly cover all branches including zero-weight edge cases.
---
Duplicate comments:
In `@nodes/adapters/ollama_client.py`:
- Around line 132-135: The is_loaded check currently treats a model as loaded if
model == rm or model.startswith(rm.split(":")[0]), which causes false positives
like marking "qwen3:8b" loaded when "qwen3:4b" is running; change the logic in
the is_loaded comprehension to compare base names explicitly (split both model
and rm on ":" and compare their base names for equality) so only models with the
same base name are considered loaded, while still preserving the exact match
fallback.
---
Nitpick comments:
In `@nodes/prompt_refiner_node.py`:
- Around line 155-170: The current code updates pbar twice causing backward
jumps: once per pass (using i/passes and pbar.update_absolute) and again inside
generate_streaming's chunk-level updates; pick one strategy and remove the
conflicting update. Either remove the pass-level update (the block computing
progress with i, passes and calling pbar.update_absolute) and let
client.generate_streaming drive progress, or keep the pass-level scaling and
modify generate_streaming calls to accept a per-pass progress range so it only
reports within that segment; update references to pbar, generate_streaming,
passes, i, and pbar.update_absolute 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0eb81b19-bc06-4103-a63b-f1cf5cb2dc68
📒 Files selected for processing (5)
docs/PR-8-REVIEW.mdnodes/adapters/ollama_client.pynodes/negative_prompt_node.pynodes/prompt_combiner_node.pynodes/prompt_refiner_node.py
✅ Files skipped from review due to trivial changes (1)
- docs/PR-8-REVIEW.md
| """ | ||
| Stream ollama.generate() with per-chunk and total timeout enforcement. | ||
|
|
||
| Args: | ||
| seed: Optional seed for deterministic generation (e.g. PromptRefiner) | ||
|
|
||
| Returns: | ||
| Full response text, or None on failure. | ||
| """ |
There was a problem hiding this comment.
Align generate_streaming failure contract with caller behavior.
Line 166 documents “None on failure”, but Lines 250-255 raise RuntimeError. Current node callers use None to trigger subprocess fallback, so these exceptions can bypass fallback and fail node execution.
💡 Proposed fix (keep `None`-based contract)
- except (TypeError, AttributeError) as e:
- # API contract mismatch — propagate so caller knows the integration broke
- raise RuntimeError(f"Ollama API contract mismatch: {e}") from e
- except Exception as e:
- # Unknown failure — log and propagate with context
- raise RuntimeError(f"Streaming generation failed: {e}") from e
+ except (TypeError, AttributeError) as e:
+ self._log(f"Ollama API contract mismatch: {e}")
+ return None
+ except Exception as e:
+ self._log(f"Streaming generation failed: {e}")
+ return NoneAlso applies to: 250-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nodes/adapters/ollama_client.py` around lines 159 - 167, The docstring for
generate_streaming promises returning None on failure but the implementation
raises RuntimeError on errors (see the exception block around ollama.generate()
and timeout handling); update generate_streaming to catch all operational
exceptions (including timeouts and any exceptions from ollama.generate()), log
the error/context using the node's logger, and return None instead of raising so
callers relying on a None fallback behave correctly; ensure the per-chunk and
total timeout enforcement code still cancels/cleans up resources before
returning None.
| if weight <= 0: | ||
| continue | ||
| # Map weight to emphasis levels | ||
| if weight >= 1.5: | ||
| parts.append(f"(({text}))") | ||
| elif weight >= 1.2: | ||
| parts.append(f"({text})") | ||
| elif weight <= 0.5: | ||
| parts.append(f"[{text}]") | ||
| else: | ||
| parts.append(text) | ||
| return ", ".join(parts) |
There was a problem hiding this comment.
Avoid blank output when all blend weights are zero.
On Line 176, weight <= 0 entries are dropped. If all prompts use 0.0, Line 187 returns an empty string even though valid prompt text was provided.
💡 Proposed fix
parts = []
for text, weight in prompts:
- if weight <= 0:
+ if weight < 0:
continue
# Map weight to emphasis levels
if weight >= 1.5:
parts.append(f"(({text}))")
elif weight >= 1.2:
parts.append(f"({text})")
elif weight <= 0.5:
parts.append(f"[{text}]")
else:
parts.append(text)🤖 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 176 - 187, The current loop drops
entries where weight <= 0 which causes an empty return when all weights are
zero; modify the logic in the block using variables weight, text, parts and the
final return(", ".join(parts)) so that either (a) only negative weights are
skipped (keep weight == 0 as neutral and append text normally) or (b) after the
loop, if parts is empty, fall back to appending all original texts (unmodified)
before returning; update the condition and/or add the fallback to ensure a
non-empty string is returned when all blend weights are 0.0.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
Documentation