Fixes a real gap discovered this session: ChatCompletion/FormatString's
"ui": {"text": [...]} return value was never rendered anywhere — no JS in
this repo ever implemented an onExecuted handler to read it, so the
retry-status banner from the previous commit was invisible in practice.
RandomChoice never returned a ui payload at all (OUTPUT_NODE was False).
- src/js/preview_text.js: read-only STRING preview widget for
ChatCompletion/FormatString/RandomChoice, mirroring ComfyUI core's own
PreviewAny node/JS extension (confirmed via the actual bundled frontend
source, not guessed) rather than inventing a new convention.
- RandomChoice: OUTPUT_NODE=True + a ui.text preview of its arbitrary-typed
output (str/number passthrough, else JSON, else str() — same fallback
chain as core's PreviewAny). IS_CHANGED still returns the raw picked
value, not the new ui-wrapped dict, so change-detection is unaffected.
- Live retry feedback: chat()/chat_structured() gain an on_status
callback, invoked at each retry boundary with a human-readable status
(_llm/retry.py's format_retry_status/format_recovered_status).
ChatCompletion wires this to ComfyUI's own PromptServer.send_progress_text
(the same first-party mechanism core's gaussian-splat/image nodes use)
rather than a hand-rolled custom event — the frontend already renders a
live "progressText" widget for it, so no additional JS was needed for
the live half of this feature.
Verified live against a real Ollama backend (docker-compose.dev.yml +
Playwright): forced a genuine refusal/retry via custom_phrases, confirmed
both the final preview widget and the live progress widget render
correctly on ChatCompletion, and confirmed FormatString/RandomChoice's
preview widgets render too. 410 tests passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three follow-ups to the refusal-retry feature:
- Each retry's request timeout now escalates (next_timeout_secs: base *
attempt) across every chat()/chat_structured() retry loop, instead of
reusing the same budget that just ran out.
- LLMProvider.chat()/chat_structured() gain an optional attempt_info
out-param (record_attempt_info), populated with the seed/timeout
actually used and the attempt/refusal counts — backward compatible,
no return-type change.
- ChatCompletion exposes a new seed_used output and prepends a status
line to its existing text preview when a retry/refusal happened, so
both are visible without any frontend/JS changes.
seed_used is appended as the LAST output (after any dynamic
structured-output fields), not inserted after model_name — dynamic
fields must keep starting at their existing index (3) so already-wired
workflows (workflows/ltx-i2v-pipeline.json, and the still-open PR #35
canvas-format file) that link a FormatString input to a ChatCompletion
field by output index aren't silently repointed at the wrong socket.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ship a comma-separated custom_phrases field on OllamaOptionRefusalRetry
so a new deflection phrasing a specific model uses can be added at
runtime, without waiting on a code change/release. Each phrase is
checked as a free, case-insensitive substring match (no embedding
model required) and, when embedding_model is set, folded in as extra
exemplars for the similarity check too.
Also ship two more default patterns spotted this session: bare
"cannot generate" (no leading "I") and "I am/I'm restricted from" —
both verified against the existing ordinary-content regression cases
to confirm no new false positives.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The lexical refusal pattern required an "I" before cannot/can't/won't
so a refusal fragment like "Cannot generate this image due to the
content" (no leading pronoun — the shape it takes as a JSON string
value or a mid-sentence fragment) went undetected. Made the "I"
prefix optional; the cannot/can't/won't-plus-nearby-generate/create/
etc. shape is specific enough that dropping the pronoun requirement
doesn't pick up ordinary content (verified against the existing
"ordinary content" regression cases).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An explicitly configured embedding_model is an opt-in to pay for the
similarity check, but is_refusal still gated it on is_ambiguous — a
length/hedge-keyword heuristic that skips anything long or plainly
worded. For chat_structured() this checks the full serialized JSON
output, whose braces/field-name overhead alone often pushed responses
past the 400-char cutoff, so the embedding call silently never fired
for exactly the subtle, on-topic-looking deflections this feature was
built to catch.
Now embed_fn, once supplied, always runs on any non-blank response that
didn't already match the lexical pass. Also widened is_ambiguous's own
threshold/keyword list, since it remains a standalone tested heuristic
for other callers even though is_refusal no longer wires it in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The lexical pattern only matched the "I'm unable to" contraction, so
spelled-out refusals like "I am unable to comply with your instruction"
fell through undetected without an embedding model configured.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Some models (observed with an abliterated Qwen variant) answer a request
they judge "politically sensitive" with hedging refusal language instead of
erroring or returning blank — neither of which the existing blank-response
or schema-validation retry triggers catch, so the refusal just passed
through as a normal result.
Adds a hybrid detector to _llm/retry.py: a free lexical/regex pass catches
blatant refusals ("I cannot generate...") without any network call; a
response that's short and/or hedge-y enough to be ambiguous additionally
gets an embedding-similarity check against canonical refusal exemplars, via
a new optional LLMProvider.embed() (Ollama's /api/embed, llama-server's
/v1/embeddings) — deliberately model-agnostic to the backend, since this is
a model-behavior concern, not a backend one. A detected refusal is treated
exactly like a blank response or validation failure: retried with a bumped
seed (existing next_seed()/RETRY_BACKOFF_SECS machinery), never returned to
the caller.
Wired into all four retry loops: OllamaProvider.chat()/chat_structured(),
LlamaCppProvider.chat(), and the shared _llm/chat.py chat_structured() used
by LlamaCppProvider.chat_structured(). New OllamaOptionRefusalRetry node
(same composable OLLAMA_OPTIONS chain as OllamaOptionDisableThinking)
configures it: enabled toggle, optional embedding_model (blank = lexical-
only), similarity threshold.
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Adds OllamaOptionDisableThinking, a composable node chaining into the
same OLLAMA_OPTIONS socket every other OllamaOption* node uses. Unlike
those (Ollama-native sampling params passed through verbatim), the
"think" key it emits is a comfydv-level convention: every LLMProvider
implementation pops it out of options and translates it to its own wire
shape before building a request, since neither backend recognizes a
literal "think" key nested inside a generic options object.
Confirmed live: Ollama's native /api/chat and OpenAI-compatible
/v1/chat/completions both silently ignore "think" nested in options —
it must be a top-level request field, or the model burns its whole
token budget on chain-of-thought reasoning before ever responding
(eval_count: 223 vs 2 in a direct comparison). llama.cpp's translation
(chat_template_kwargs/reasoning_effort) is sourced from llama-server's
documented request-body fields, not live-verified against a running
instance.
This also fixes two existing live integration tests that already passed
options={"think": False} under the mistaken assumption it worked — it
was a silent no-op until now.
See ADR-010 for the full design discussion, including why this ended up
as a composable option node rather than a new ChatCompletion input.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
Ollama's OpenAI-compatible endpoint silently reloads the model at its
default context size on every call, discarding any options.num_ctx
override — confirmed live even when the same options are included in
that request. OllamaProvider.chat_structured() now hand-rolls a call to
Ollama's native /api/chat + "format" instead of routing through the
shared pydantic-ai path, since that endpoint correctly preserves and
applies context-size options. LlamaCppProvider keeps the shared path
(llama-server's context is fixed at process launch, not per-request),
switched to pydantic-ai's NativeOutput mode so a thinking-capable model's
reasoning no longer competes with the structured response for token
budget.
Also fixes _build_structured_model typing non-required schema fields as
bare `py_type` with a None default, which only tolerates the field being
omitted — an explicit `null` (which models routinely emit) failed
pydantic validation until the field was typed `py_type | None`.
See ADR-009 for the full investigation and rejected alternatives.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
ComfyUI has no native list/array socket type — FormatString's dynamic
inputs are always STRING (see update_widget). Passing a list of values
(e.g. extraction hints) into a {% for %} loop therefore requires either
manual comma-splitting or a way to carry structured data through a
STRING socket as JSON text.
Jinja2 ships `tojson` but not its inverse. Register `fromjson` (a thin
wrapper over json.loads) on FormatString.jinja_env so a STRING input
containing a JSON array/object can be parsed back into real Python data:
{% for hint in extraction_hints | fromjson %}
- {{ hint }}
{% endfor %}
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
uv run pytest was failing every single test with ModuleNotFoundError:
No module named 'comfydv._llm' — reproduces identically on a clean
checkout, unrelated to any test content.
Root cause: the repo root's own __init__.py (ComfyUI's custom-node
entry point) is also a valid "comfydv" package from a sys.path state
pytest transiently constructs during fixture setup, and it doesn't
expose the _llm submodule. A bare `import comfydv` issued from inside
a fixture body (e.g. _clear_ollama_caches) could resolve to that root
package instead of src/comfydv.
Forcing an explicit `import comfydv` in pytest_configure — while our
sys.path.insert(0, src) is still the definitive answer — caches the
correct module in sys.modules before anything else gets a chance to
resolve it ambiguously.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
FormatString._extract_keys used hand-rolled regexes to detect variables
in Jinja2 templates, which broke on anything beyond bare {{ var }} and
{{ var | filter }}:
- {% for hint in extraction_hints %}{{ hint }}{% endfor %} incorrectly
surfaced the loop-local `hint` as a required input and never detected
`extraction_hints` itself, since the control-structure regex only
matched filter syntax (word followed by `|`).
- {% if extraction_hints is defined %} never matched at all.
- Filters called with arguments, e.g. {{ x | tojson(indent=2) }}, broke
the regex's anchor to the closing `}}` and silently extracted nothing.
Replaced with jinja2.meta.find_undeclared_variables() over the parsed
AST, which handles all of Jinja2's syntax correctly and excludes names
bound within the template (loop targets, {% set %}) by construction.
Since that call returns an unordered set, sort by first textual
occurrence to keep extraction order deterministic for callers that rely
on positional outputs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
Fixes#27. Live-observed against a real Ollama server under model-swap
load: /api/chat can answer HTTP 200 with model/created_at/message all
blank and done: false, before the target model has actually finished
loading. chat()'s existing blank-response retry (PR #23) couldn't tell
this apart from a genuine blank generation, so once retries were
exhausted it silently returned "" — success, with a wrong answer.
chat() now tracks whether the last attempt's response was this explicit
done: false stub. If retries are exhausted and the last response was one,
it raises a clear RuntimeError instead of returning blank. A genuine
blank generation (done: true, or no `done` field at all) keeps the
pre-existing silent-return behavior unchanged — only the stub signal
escalates.
3 new tests cover: raises when every attempt is a stub, recovers cleanly
when a later attempt is a real response, and the regression guard that a
genuine done:true blank still returns silently as before.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
FR-006/SC-005 require a clear, specific error when an image is wired to
a model that can't process it. Live testing against a real Ollama server
found this unimplemented for the Ollama backend: /api/chat silently
accepts an unsupported `images` field and answers HTTP 200 with a blank
message instead of erroring, which the existing blank-response retry
(PR #23) swallows as an ordinary blank generation rather than surfacing.
OllamaProvider.chat()/chat_structured() now check /api/show's
`capabilities` list before sending a request that carries an image, and
raise a clear ValueError if `vision` is absent. The check is skipped
entirely for image-less requests (FR-003) and fails open on any lookup
problem (older Ollama, network hiccup) so it never blocks a request that
would otherwise have worked.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
docker/Dockerfile was bumped from 3.11 to 3.13 in 50076d7 without
updating the assertion, breaking the suite on main. Verified 3.13
compatibility by building the image and booting ComfyUI 0.27.0 in it
(custom node loads, DB migrations run, no errors) before updating the
pinned version the test enforces.
Fixes#20
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YArD9ZjBWKsAvazmS48amA
Images on the current turn ride on Agent.run user_prompt as BinaryContent;
images on a history user turn ride on its UserPromptPart; text-only calls
stay plain-string. Red: chat.py passes only .content today. Witnesses both
us3_structured_image.feature scenarios.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvS9TFMCFYNHC4MMvzJaZS
llama.cpp's OpenAI endpoint takes images as image_url parts inside content;
text-only messages must keep a plain-string content (byte-identical). Red:
mapping absent and model_dump() leaks images:None. Witnesses both
us2_both_backends.feature scenarios.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvS9TFMCFYNHC4MMvzJaZS
Ollama /api/chat carries images as a flat per-message base64 array; a
text-only request must omit the images key (byte-identical to today,
FR-003/SC-004). Red: model_dump() currently leaks images:None into the
payload. Witnesses us1_describe_image.feature "Describe a wired image".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvS9TFMCFYNHC4MMvzJaZS
Optional image input; wired image attaches to the last user turn only
(history untouched, FR-007); un-wired path stays text-only; output
positions unchanged. Red: chat() has no image parameter yet. Witnesses
both us1_describe_image.feature scenarios.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvS9TFMCFYNHC4MMvzJaZS
Message gains an optional images field carrying base64 payloads per turn;
text-only messages must dump without an images key (FR-003/SC-004). Red:
current Message has no images attribute.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvS9TFMCFYNHC4MMvzJaZS
beacon-reviewer caught a real gap: when a caller pins options["seed"],
the retry loop set the new seed on ModelSettings' top-level "seed"
field but left the same request's extra_body.options.seed (the
Ollama-native passthrough) at the original pinned value. A backend
that honors the nested field over the top-level OpenAI one would keep
sending the identical effective seed on every retry, silently
defeating this fix for exactly the pinned-seed case it needs to cover.
Now both fields are kept in sync on every retry attempt, built via
fresh copies rather than mutating the caller's options dict in place
(that dict is shared across every attempt, and possibly across other
calls). Added a regression test asserting the caller's options dict is
untouched after a multi-attempt retry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Reported live on a fresh runpod: structured_output extraction on
ChatCompletion would fail with
RuntimeError: chat_structured: response failed validation against
schema after 3 attempt(s) ... Last error: Exceeded maximum output
retries (0)
...but succeed if a plain (non-structured) chat call with the same
prompt was run first. Root cause: pydantic-ai's Agent is built with
retries=0 (comfydv drives its own outer retry loop, not pydantic-ai's
internal one), so a single bad/empty tool-call response instantly
exhausts one attempt. comfydv's own retry loop then retried with the
*exact same request* every time — if the model is genuinely stuck
(e.g. still warming up on a freshly-started backend) or produces a
degenerate response for a given seed, every attempt fails identically.
Confirmed the seed was unset in the reporting user's workflow (Ollama
picks its own per call) and it still failed 3/3 in a row, which rules
out "stuck on one bad seed" and points at request-level determinism
plus zero backoff between attempts as the real gap.
Plain chat() had zero retry/validation logic at all before this change
— it silently returned whatever content came back, even blank. Given
the underlying failure mode (a fresh model's first response sometimes
being blank) is identical for both chat modes, this fix applies to
both, not just structured output.
Adds src/comfydv/_llm/retry.py: next_seed() (deterministic, starting
from any user-pinned options["seed"], incrementing per retry — attempt
1 is always the caller's original, untouched request) and
RETRY_BACKOFF_SECS (a flat delay between retries, giving a still-
loading model time to finish rather than hammering it with identical
requests back-to-back).
Both OllamaProvider.chat() and LlamaCppProvider.chat() now retry (per
the node's existing max_retries widget) when a response comes back
blank, injecting the incremented seed each retry — Ollama via the
native options.seed field, llama.cpp via the OpenAI spec's top-level
"seed" field (nesting it under "options" like the rest of the
Ollama-native passthrough would silently not work — llama-server's
OpenAI-compatible endpoint doesn't read seed from there, a pre-existing,
documented non-goal for general options passthrough that doesn't apply
to this internal retry mechanism). Neither raises if every retry stays
blank — matches chat()'s existing never-validates contract.
chat_structured() in _llm/chat.py now injects the same incrementing
seed via pydantic-ai's ModelSettings["seed"] (which correctly maps to
the OpenAI API's top-level seed param for both backends) and adds the
same backoff between attempts, on top of its existing validation-retry
loop.
Live-verified against real Ollama: the happy path (model responds
normally first try) still makes exactly one network call for chat()
and completes chat_structured() normally — no regression from the new
retry scaffolding. The specific cold/stuck-model failure itself is
runpod-cold-start-dependent and wasn't independently reproducible in
this session's environment (a second live attempt succeeded on the
very first try) — unit tests directly exercise the retry/seed/backoff
logic instead (test_llm_retry.py, and new cases in
test_ollama_provider.py, test_llamacpp_provider.py, and
test_llm_chat_structured.py), verified against both providers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
beacon-reviewer caught it: reconstructing the old buggy _run_async and
running it against test_run_async_works_when_called_from_within_a_running_loop
and test_run_async_propagates_exceptions_from_within_a_running_loop shows
both pass against the old code too. asyncio.get_running_loop() never
spuriously raises under plain CPython/pytest, so the old conditional also
takes the safe worker-thread branch there — the actual reported bug is not
reproducible outside real ComfyUI's execution engine, and only the live
end-to-end run against it proved the fix.
Corrects the block comment to say what the tests actually do: lock in the
documented contract so a regression to a no-thread asyncio.run(coro) call
is caught immediately, without claiming they reproduce the specific
environment-dependent failure.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Found by running a genuine end-to-end test — real docker-compose
ComfyUI harness, real Ollama, real llama-server (router mode),
actual workflow graphs submitted via POST /prompt and polled via
/history, not direct Python calls to provider classes. Every
structured_output=True ChatCompletion call failed with:
RuntimeError: asyncio.run() cannot be called from a running event loop
Root cause: _run_async() tried asyncio.get_running_loop() first and
only spun up an isolated worker thread if that succeeded; otherwise
it called asyncio.run(coro) directly on the current thread. Under
pytest or a standalone script, get_running_loop() never spuriously
raises, so this worked and the worker-thread path was rarely
exercised for real. Under ComfyUI's actual async execution engine
(Python 3.13, node functions invoked synchronously from inside an
already-running event loop), get_running_loop() sometimes raised
anyway — routing straight into asyncio.run(coro) on the one thread
guaranteed to already have a loop running, reproducing the crash
every time. Confirmed via a live-instrumented debug run against the
real container: the coroutine executed correctly whenever the
worker-thread path was taken (get_running_loop() succeeding), and
crashed only via the direct-call fallback.
Fixed by removing the conditional entirely: always run the coroutine
in a freshly spawned worker thread. A new thread never has an
ambient loop, so asyncio.run() is safe there unconditionally,
regardless of what the calling thread's loop state actually is.
Also traced down a second symptom from the same end-to-end run:
non-structured chat() calls sometimes returned an empty response
with no error. Verified via direct curl calls to Ollama and
llama-server (bypassing comfydv entirely) that this was a real,
transient GPU/Metal memory-pressure issue on the test machine
(`ggml_metal_synchronize: error: Insufficient Memory`, Ollama's own
backend left in a wedged state returning HTTP 200 with an empty
zero-value body) — not a comfydv bug. Restarting the backend
processes resolved it; no code change was needed for that part.
Added tests/test_ollama_provider.py coverage for the specific shape
that broke: _run_async called synchronously from within code that is
itself already executing inside asyncio.run() — the actual pattern
ComfyUI's execution engine uses, which no prior test exercised (every
existing call site invoked _run_async from a plain synchronous pytest
function with no ambient loop).
Re-verified end-to-end after the fix: 5/5 real workflows passing
through actual ComfyUI execution — Ollama basic chat, Ollama full
lifecycle (Load->Chat->Unload), Ollama structured output, llama.cpp
basic chat, llama.cpp structured output — all with real generated
content, not mocks.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
beacon-reviewer caught a real gap: tests/test_comfyui_import_compat.py
ERRORed (not passed) when run in isolation or under a file-sharded
runner — the autouse _clear_ollama_caches fixture from conftest.py
does `from comfydv._llm.ollama_provider import ...`, and this file
was the one case where nothing had already bound `comfydv` in
sys.modules correctly first, so it resolved to the wrong package.
Only the full single-process suite happened to be green.
Fixed by shadowing that fixture with a local no-op override scoped
to this module: these tests exercise package import resolution
itself, not OllamaProvider/ChatCompletion state, so there is nothing
for the original fixture to reset here — and depending on it made
the test order-dependent on an unrelated file's import order, which
is exactly the kind of fragility this file exists to guard against.
Verified: passes standalone (`pytest tests/test_comfyui_import_compat.py`)
and as part of the full suite.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Every internal `from comfydv._llm.X import Y`-style absolute
self-import broke the entire plugin the moment ComfyUI actually
loaded it — every node, not just the LLM ones, since the whole
src/comfydv/__init__.py chain aborted on the first such import.
ComfyUI's custom_nodes loader imports this plugin via a *relative*
chain (repo-root __init__.py does `from .src.comfydv import ...`),
nesting comfydv under whatever top-level name the folder gets —
never `comfydv` itself. An absolute `from comfydv...` self-import
only resolves if `src/` has separately been placed on sys.path,
which conftest.py does for every test — masking this completely.
No test ever exercised the real loading shape.
Confirmed via git bisection against the actual docker-compose dev
harness (built and ran real ComfyUI): this predates spec 008
entirely — checking out the commit right after PR #17 merged,
before llamacpp.py existed, reproduces the identical failure at
ollama.py's own absolute import. Fixed by converting every internal
self-import across src/comfydv/ to a relative import, which resolves
correctly under both loading shapes. Verified fixed by rebuilding
the harness and confirming all 21 nodes register via /object_info.
Added tests/test_comfyui_import_compat.py: a subprocess-based test
reproducing ComfyUI's exact nested-relative-import shape (not
conftest.py's sys.path-patched shape), plus a static AST guard
against any future absolute self-import creeping back in.
Also fixed, found via the same live-harness investigation:
- src/js/ollama.js matched on pre-rename node names
(OllamaModelSelector/OllamaLoadModel/OllamaChatCompletion), so the
"Refresh models" button and live structured-output socket preview
were silently absent from every node in the real UI.
- The /dv/ollama/models route always spoke Ollama's wire protocol
regardless of which backend was actually connected — pointing it
at a llama.cpp host could never populate real models. Route now
takes a `backend` param; getHostAndBackendFromNode() in ollama.js
determines it from the connected client node's registered type;
LlamaCppProvider gained a matching _fetch_models() name-only view
(mirrors OllamaProvider's, same graceful-degradation contract).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Ran a second live smoke test — load_model() then chat_structured()
with a real pydantic.BaseModel schema — against the same router-mode
llama-server used for the earlier fix. Confirmed pydantic-ai's
Agent/OpenAIProvider mechanism works against llama-server's
OpenAI-compatible endpoint, not just Ollama's (the only one verified
live in the prerequisite epic). No gap found; every LlamaCppProvider
method has now been exercised against a real server, not just mocks.
Also strengthened the two idempotency regression tests added in the
previous commit: they previously asserted only "does not raise", which
would also pass if the method silently no-op'd for an unrelated bug.
Now assert the mocked _post_json was actually called with the expected
request, so the test verifies real behavior, not just absence of a
crash.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Ran the live smoke test (T017) that had been wrongly marked deferred
for "no llama-server reachable" — llama-server was installed via
Homebrew the whole time and a router-mode server was launched against
a real local GGUF model to verify.
This caught a real gap the mocked suite couldn't: router mode's
/models/load and /models/unload are NOT idempotent at the wire level.
Calling load on an already-loaded model returns HTTP 400 "model is
already running" (and unload/"model is not running" symmetrically),
not the {"success": true} the conformance contract assumed without
live verification. LlamaCppProvider now absorbs exactly those two
error messages as the desired end-state already reached; any other
error still propagates. Contract doc corrected to describe the real
behavior; regression tests added (mocked, run in CI).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
LlamaCppProvider.list_models() caught every exception and returned [],
indistinguishable from "no models installed". Fixed _get_json to raise
on HTTP error status (matching _post_json's existing behavior — its
docstring already claimed this), and list_models() to distinguish
OSError (genuinely unreachable, degrades to []) from RuntimeError
(server responded with an error — surfaced with a router-mode hint).
Found by beacon-reviewer ahead of PR open.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Second LLMProvider (ADR-007), backed by llama-server's router mode. Only
one new ComfyUI node — LlamaCppClient, emitting the same LLM_CLIENT socket
Ollama Client does. ChatCompletion, LLMModelSelector, LLMLoadModel,
LLMUnloadModel work unmodified once wired to it — the actual proof the
provider abstraction generalizes, not just Ollama-shaped in practice.
API shape verified live against ggml-org/llama.cpp's tools/server/README.md
(research.md) rather than assumed: model identifier field is "id" (not
"name"), status is a nested {"value": "..."} object. list_models() needs
no normalization — llama.cpp's status vocabulary is exactly ModelStatus's
full set, unlike Ollama's narrower one.
38 new tests (test_llamacpp_provider.py, test_llamacpp.py), including a
dedicated US4 test proving no generic node branches on provider type —
the same call sequence succeeds against an Ollama-shaped or llama.cpp-
shaped fake provider.
README/docs updated for the new node this time, not left stale.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
chat_structured() silently dropped the options dict (temperature, seed,
num_predict, repeat_penalty, etc. — set via the OllamaOption* nodes) in
structured_output mode: OllamaProvider.chat_structured() only used it for
the cache key, and the shared comfydv._llm.chat.chat_structured() helper
had no options parameter at all. On main, the hand-rolled implementation
sent options verbatim in the request body; this migration silently lost it,
contradicting FR-008 (structured-output behavior must be unchanged).
Fixed by forwarding options as pydantic-ai's model_settings.extra_body,
matching the original payload shape exactly rather than lossily remapping
onto ModelSettings' own standardized field names (which don't cover
Ollama-native params like num_predict/repeat_penalty/top_k anyway).
Verified against the live local Ollama server: structured output with
temperature=0.0/seed=42 succeeds on first attempt.
Caught by an independent beacon-reviewer pass on the diff before opening a
PR — exactly what that review step is for.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Rewrote tests/test_ollama.py against a _FakeProvider double per
atomic-cutover-plan.md's D4/D5 test-layer split: node-contract/delegation
tests only, no aiohttp/_post_json mocking (that lives in
test_ollama_provider.py now). 98 unit tests, collapsed from the original
~125 largely mechanical monkeypatch-per-scenario tests where the retry/
validation coverage they duplicated already lives in
test_llm_chat_structured.py.
Fixed a real gap the rename surfaced along the way: comfy-manager-entry.json
(ComfyUI Manager registry listing) still had the old Ollama-specific
display names — updated it and its matching test expectation.
Full validation (T-CUT-10): 218 tests green (only the pre-existing,
unrelated Dockerfile-python-version test fails); ruff/ty clean (confirmed
via git stash comparison that the remaining ty diagnostics pre-date this
work); beacon doctor --strict shows only already-disclosed items.
Manual smoke test (T-CUT-12): Ollama was reachable in this environment, so
ran the real integration suite rather than a walkthrough. 6/8 passed
including the critical paths (real load/unload, real structured-output
retry-then-raise, real error handling). 2 failures traced via direct curl
to Ollama's /api/chat (bypassing this codebase) to a pre-existing,
ADR-006-documented unreliability in the specific test model, not a
regression.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
OllamaClient now emits an OllamaProvider via the generic LLM_CLIENT socket
(was OllamaClientType/OLLAMA_CLIENT). OllamaChatCompletion, OllamaModelSelector,
OllamaLoadModel, OllamaUnloadModel renamed to ChatCompletion, LLMModelSelector,
LLMLoadModel, LLMUnloadModel — all delegate to client.* (LLMProvider protocol)
instead of building Ollama URLs/payloads inline. Structured output now goes
through client.chat_structured() (pydantic-ai), not hand-rolled tool-calling.
ollama.py's own duplicate HTTP/cache infra (_post_json, _TTLLRUCache,
_MODEL_LIST_CACHE, _CHAT_RESPONSE_CACHE) is gone — single source of truth in
comfydv._llm.ollama_provider (plan D1); the combo-widget path
(_load_default_models, /dv/ollama/models) imports _fetch_models/_run_async
from there instead of duplicating them. _client_headers deleted (dead code
once nodes delegate to client.*).
Added MIGRATION_MAP (old->new node/socket names, FR-009). NODE_CLASS_MAPPINGS
keys changed to match (breaking rename, confirmed acceptable per ADR-007).
conftest.py's autouse cache-clearing fixture repointed to the single cache
source and ChatCompletion (plan D8).
tests/test_ollama.py itself is NOT yet updated (T-CUT-08, next commit) — it
will fail to collect until then; this is the expected atomic-cutover
intermediate state, not a regression to fix independently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
list_models()/load_model()/unload_model()/chat()/chat_structured() —
behavior-preserving ports of the existing inline ollama.py logic, plus one
genuinely new capability: list_models() also queries /api/ps to report
real loaded/unloaded status (the old OllamaModelSelector only ever showed
names via /api/tags, never live status).
15 new tests in tests/test_ollama_provider.py, mocking at the module's own
_post_json/_get_json seam per atomic-cutover-plan.md's D4 test-layer split.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Provider-layer only — src/comfydv/_llm/chat.py doesn't touch ollama.py, so
it's safe to land ahead of the deferred atomic node cutover (issue #16).
Ports ADR-006's exact retry/validation contract: bounded retries (0-5,
clamped), RuntimeError naming model/attempt-count/truncated-last-response
on exhaustion. Agent's own internal retries disabled so the error contract
is comfydv's, not pydantic-ai's.
Required an API-discovery spike (Agent/OpenAIProvider/OpenAIChatModel
constructors and exception surface aren't reliably knowable from training
data for a fast-moving library) before tests could be written meaningfully
— tests and implementation were validated together rather than strictly
red-first; noted in tasks.md rather than presented as pure TDD.
Also fixes a pre-existing gap in test_packaging.py's requirements.txt/
pyproject.toml consistency check: it stripped `[extras]` on one side of the
comparison but not the other, which this change's pydantic-ai-slim[openai]
entry exposed as the first dependency with an extras marker.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Adds pydantic-ai-slim[openai] per ADR-007, the Message/ModelStatus/
ModelInfo/LLMProvider protocol, and an OllamaProvider skeleton carrying
host/headers as instance state — ported _post_json/_fetch_models/_run_async/
_TTLLRUCache infra from comfydv.ollama unchanged. Method bodies land per
user story in specs/007-llm-provider-abstraction/tasks.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Adds a POST /dv/ollama/update_structured_outputs route and a matching JS
extension so editing structured_output/output_schema on
OllamaChatCompletion updates the node's output sockets immediately in the
UI — one per schema property — mirroring FormatString's existing live
dynamic-output pattern. No need to run the graph first to see the extracted
fields appear. Invalid/incomplete JSON while typing falls back to the base
3-output shape rather than erroring the request.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds structured_output/output_schema/max_retries inputs to
OllamaChatCompletion, fixing three real reliability problems: models
prepending commentary, wrapping responses in code fences, and occasionally
returning blank output. When enabled, requests go through Ollama's
OpenAI-compatible tool-calling endpoint (a single forced tool call matching
output_schema) instead of native /api/chat, and each response is validated
against a dynamically-built pydantic model — invalid/blank responses retry
with fresh network calls, exhausting into a clear RuntimeError rather than
silently passing bad data downstream. One additional ComfyUI output socket
is exposed per schema property, mirroring FormatString's existing
dynamic-output pattern. Fully backward compatible: structured_output
defaults off and non-structured behavior is untouched.
Ollama's native /api/chat "format" field (JSON-Schema-constrained decoding)
was tried first but proved unreliable against a real local model during
implementation — see ADR-006 for the full investigation and why
tool-calling was chosen over both native format and pydantic-ai (the latter
would reintroduce httpx/openai-sdk, reversing ADR-004).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FormatString is now OUTPUT_NODE=True and returns {"ui": {"text": [...]},
"result": (...)} instead of a bare tuple, so the rendered output shows
directly on the node — no separate Show Text node needed to inspect it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds an OLLAMA_HEADERS socket type (Basic Auth, Bearer Token, Custom) that
composes onto OllamaClient, so requests to a proxied/authenticated Ollama
server carry the right Authorization header through model discovery,
load/unload, and chat.
Also adds a bounded local cache: a short-TTL LRU for model discovery
(avoids duplicate /api/tags round-trips from repeated node creation/refresh)
and an unbounded-TTL LRU for chat responses, keyed on the full request
signature — needed because OllamaChatCompletion is OUTPUT_NODE=True and so
is re-executed by ComfyUI on every queue run even when nothing changed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- OllamaChatCompletion.model changed from COMBO to plain STRING so it can
receive a wired value from OllamaLoadModel without a separate model_name
socket (removes dual-input UX confusion and COMBO validation errors)
- Removed model_name optional input — model STRING is the single entry point
- _load_default_models() tries OLLAMA_HOST env var → host.docker.internal →
localhost so COMBO nodes pre-populate on server start inside Docker
- docker-compose.dev.yml: OLLAMA_HOST=http://host.docker.internal:11434 so
model dropdowns are populated at startup without a manual Refresh click
- JS: refreshModelWidget handles both COMBO and STRING widget types; keeps
user-typed/wired value when it is already a known model
- Tests: first_generative_model fixture dynamically selects the first
installed model instead of hardcoding embeddinggemma:latest (which is an
embedding-only model that rejects /api/generate with HTTP 400)
- Tests: test_multi_turn_receives_context passes think=False to prevent
Qwen3 thinking-mode models from returning empty content fields
- Screenshots retaken: ollama_chat.png, ollama_lifecycle.png, ollama_workflow.png
All 80 unit + 9 integration tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWysE6jYn4YjNorLBQ1cXb
OllamaChatCompletion now sets OUTPUT_NODE = True and returns
{"ui": {"text": [...]}, "result": (...)} so ComfyUI renders the
response text directly in the node body after each run.
On first turn, the full response is displayed.
On subsequent turns, the response is shown with a compact history
summary (last 6 messages, 100-char snippets) so multi-turn context
is visible at a glance without a separate display node.
_history_preview() helper added for the summary rendering.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWysE6jYn4YjNorLBQ1cXb
5 failing unit tests:
- test_chat_is_output_node
- test_chat_returns_ui_result_dict
- test_chat_ui_contains_response_text
- test_chat_result_is_3_tuple
- test_wired_model_name_overrides_combo (updated to unpack from ["result"])
Integration tests updated to unpack from ["result"] for API consistency.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWysE6jYn4YjNorLBQ1cXb
9 new tests cover:
- _run_async: event-loop safety when called from a running loop
- _post_json: raises RuntimeError on 4xx/5xx; timeout kwarg forwarded
- LoadModel/UnloadModel: must use /api/generate, keep_alive must be int
- ChatCompletion: must expose timeout_secs widget and forward it
58 pre-existing unit tests remain green. Integration tests excluded.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWysE6jYn4YjNorLBQ1cXb
OllamaUnloadModel now works as a pass-through node: wire
OllamaChatCompletion.response into the optional `passthrough` input and
OllamaChatCompletion.model_name into `model` — ComfyUI's data-flow graph
then guarantees Load → Chat → Unload execution order.
OllamaChatCompletion gains an optional `model_name` forceInput so
OllamaLoadModel.model_name can wire in (creating Load → Chat ordering) and
overrides the COMBO dropdown. It also returns `model_name` as a 3rd output
so the model can flow downstream to UnloadModel without duplication.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWysE6jYn4YjNorLBQ1cXb
