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
Full spec/plan/tasks/BDD scaffold for the llamacpp-integration epic, now
that its dependency (llm-provider-abstraction, PR #17) is satisfied.
Researched llama-server's router-mode API shape live (postdates training
data) rather than assuming it — two details that would have been wrong by
assumption: the model identifier field is "id" not "name" (differs from
Ollama's /api/tags), and "status" is a nested object ({"value": "..."}) not
a flat string.
Unlike the prerequisite epic, this decomposition genuinely holds as 4
independent user stories — LlamaCppClient is a new node, not a changed one,
so there's no shared "output type" migration forcing an atomic cutover.
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
Struck-through historical tasks (- [ ] ~~...~~) still read as open
checkboxes to beacon's tooling — bullet status was picking one as "active"
instead of the real next step. Marked all superseded Phase 3/5/T014/T019-21
entries [-] (deferred, per the project's own convention), and added a real
Phase 8 with T-CUT-01..12 as trackable checkboxes matching
atomic-cutover-plan.md, so tooling and humans see the same source of truth.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Full line-by-line inventory of every reference in src/comfydv/ollama.py
and tests/test_ollama.py (1820 lines, read in full — not estimated)
affected by the OllamaClient/OllamaChatCompletion/OllamaModelSelector/
OllamaLoadModel/OllamaUnloadModel rename, plus the design decisions it
surfaced that "just rename it" was hiding:
- Single source of truth for HTTP/cache infra (comfydv._llm.ollama_provider)
- client == "<host string>" equality is removed, not preserved (OllamaProvider
isn't a str subclass)
- Bare-string client backward compatibility is removed (undocumented side
effect of the old str-subclass trick, never an intended feature)
- Test-layer split: node-contract/delegation tests stay in test_ollama.py
using a _FakeProvider double; Ollama-wire-protocol tests move to a new
test_ollama_provider.py — this is what actually resolves the 35 relocated
_post_json monkeypatches, rather than patching them 1:1 at a seam that no
longer exists
- Structured-output retry tests are not ported 1:1 — already covered by
tests/test_llm_chat_structured.py's existing 6 tests
Produces a 12-step sequenced task list (T-CUT-01..12) in
atomic-cutover-plan.md that supersedes tasks.md's original US1/US3
decomposition (struck through, kept for history). This is now the
authoritative plan for the next BUILD session to execute.
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
Discovered mid-implementation: OllamaClient is a single shared producer for
every downstream Ollama node, so changing its output type per ADR-007
breaks all consumers simultaneously, and the class rename breaks
tests/test_ollama.py's 125 references atomically, not per-story. Confirmed
by independent product + engineering review (agent-trio deliberation,
aligned). Re-scoped the node-layer cutover as its own follow-up session,
tracked in issue #16. ADR-007's decision itself is unaffected — only
delivery sequencing changes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
Adopt a shared LLMProvider protocol (list/load/unload/chat/structured-chat)
as the adapter pattern answer to GitHub issue #15 — Ollama and llama.cpp
share the chat/structured-output mechanism via pydantic-ai while model
lifecycle stays backend-specific per provider. Supersedes ADR-006, narrows
ADR-004's scope to non-chat REST calls.
Adds epic 007 (llm-provider-abstraction, prerequisite) and the
llamacpp-integration epic it unblocks, plus the full spec/plan/tasks/BDD
scaffold for the prerequisite epic.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj
18 stub tests pass; 4 Gherkin feature files cover US1–US4 acceptance
scenarios verbatim from spec.md. No assertions yet — TDD pairs T010-T
through T041-T will fill these in next.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore: initialise BEACON framework with all bootstrap artefacts
- Problem statement, constitution, architecture doc, roadmap populated
- CHANGELOG.md created (Keep a Changelog); README expanded with
What-is-this, Install, and Quickstart sections
- pyproject.toml gains [project.urls] (repository + documentation)
- beacon doctor: 32 pass, 2 pre-commit warns, 0 failures
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore: Phase 1 — add NullHandler to package root, remove hardcoded setLevel
T001: logging.getLogger("comfydv").addHandler(NullHandler()) in __init__.py
T002: remove logger.setLevel(logging.DEBUG) from format_string.py
Also fix pyproject.toml TOML structure (project.urls was inside [project] block)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test: failing tests for logging modernisation (T010-T through T030-T)
RED phase — 5 tests fail for the correct reasons before implementation:
T010-T: format_string produces stdout (print block)
T011-T: update_widget emits INFO records on hot path
T012-T: random_choice produces stdout (colorama/rich prints)
T020-T: load_node_state uses print() on error instead of logger.error
T021-T: circuit_breaker uses print() instead of logger
T001/T002/T030-T already green: NullHandler registered, setLevel removed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat: standardise logging across all nodes (T010-I through T044)
GREEN phase — all 11 logging tests pass:
T010-I: Remove 8-line diagnostic print block from format_string()
T011-I: Downgrade all hot-path logger.info() calls to logger.debug();
switch all logger calls to %-style formatting; remove rich import
T012-I: Replace colorama/termcolor/rich print calls in random_choice with
logger.debug(); add logger = logging.getLogger(__name__)
T020-I: Convert print() on load_node_state error to logger.error()
T021-I: Add logger to circuit_breaker; replace print() with logger.debug();
fix logic so status=False triggers the interrupt (per BDD spec)
T040: Remove colorama, rich, termcolor from pyproject.toml dependencies
T042: ruff check + format clean
T044: beacon doctor --strict passes (34/34)
Also adds ADR-001 and ADR-002 capturing the stdlib logging and NullHandler
decisions, linked from the logging-modernisation epic.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore: mark all 001-standardise-logging tasks complete in tasks.md
All [x] checkboxes flipped after 11/11 tests pass and beacon doctor --strict
reports 0 failures.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore: mark logging-modernisation epic success criteria complete
All success criteria verified: NullHandler added, setLevel removed, print()
calls converted, colorama/rich/termcolor removed from deps, zero stdout in
normal operation, errors surface at ERROR level, all tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore: commit spec artefacts and dependency lock for 001-standardise-logging
Includes spec.md, plan.md, research.md, BDD feature files, contracts, .beacon.toml
backlink, and uv.lock after removing colorama/rich/termcolor.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: James Veitch <darthveitcher@office-mac-mini.local>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
