test(llm): complete atomic node cutover — test_ollama.py rewrite (T-CUT-08/10/12)

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
This commit is contained in:
James Veitch
2026-07-11 14:28:34 +01:00
co-authored by Claude Sonnet 5
parent d90ae98fed
commit 1879ca2265
6 changed files with 328 additions and 1136 deletions
+4
View File
@@ -10,6 +10,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
- BEACON framework bootstrap: problem statement, constitution, roadmap, architecture document
- `CHANGELOG.md` (this file)
- README: What-is-this, Install, and Quickstart sections
- `LLMProvider` protocol (`comfydv._llm`) — a shared adapter boundary so ComfyUI LLM nodes work with any backend that implements it, starting with `OllamaProvider`. Structured output now goes through `pydantic-ai` (ADR-007), superseding the hand-rolled Ollama tool-calling approach.
### Changed
- **Breaking:** `OllamaChatCompletion` → `ChatCompletion`, `OllamaModelSelector` → `LLMModelSelector`, `OllamaLoadModel` → `LLMLoadModel`, `OllamaUnloadModel` → `LLMUnloadModel`, and the `OLLAMA_CLIENT` socket type → `LLM_CLIENT` — these nodes are now backend-generic. `OllamaClient` is unchanged by name but now outputs an `OllamaProvider` rather than a plain string; existing saved workflows using the old node/socket names need reconnecting (see `comfydv.ollama.MIGRATION_MAP` for the full old→new mapping).
## [0.1.0] — 2026-06-01
+4 -4
View File
@@ -10,10 +10,10 @@
"Random Choice",
"Circuit Breaker",
"Ollama Client",
"Ollama Model Selector",
"Ollama Load Model",
"Ollama Unload Model",
"Ollama Chat Completion",
"LLM Model Selector",
"LLM Load Model",
"LLM Unload Model",
"Chat Completion",
"Ollama Option — Temperature",
"Ollama Option — Seed",
"Ollama Option — Max Tokens",
+3 -3
View File
@@ -145,11 +145,11 @@ suite only needs to be green as a whole at T-CUT-10, not after every step.
- [x] T-CUT-05 `ollama.py`: rename the 4 classes, `"OLLAMA_CLIENT"`→`"LLM_CLIENT"` on every consumer, rewrite the 3 delegating method bodies, delete `_client_headers` (plan D6)
- [x] T-CUT-06 `src/comfydv/__init__.py`: update imports and `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS`
- [x] T-CUT-07 `tests/conftest.py`: repoint `_clear_ollama_caches` and `first_generative_model`'s `_fetch_models` import (plan D8) — `first_generative_model`'s import needed no change (still re-exported from `comfydv.ollama`)
- [ ] T-CUT-08 `tests/test_ollama.py`: import block, `_FakeProvider` double, convert `_post_json`-monkeypatched tests, replace bare-string `client=` usages, rewrite the 2 `client == "<string>"` assertions, delete `test_plain_string_client_has_no_headers` + `test_structured_output_true_sends_tool_call_payload`, collapse the 15 retry-count tests, update `TestNodeContracts` (plan D3/D4/D5)
- [x] T-CUT-08 `tests/test_ollama.py`: rewrote against a `_FakeProvider` double per plan D4/D5 — 98 unit tests, all passing. Also fixed a real gap the rename surfaced: `comfy-manager-entry.json`'s `nodename` list (and its matching test expectation) still had the old display names — updated both.
- [x] T-CUT-09 [P] `contracts/llm_provider_protocol.md`: `timeout_secs` fix (commit `ef2464a`)
- [ ] T-CUT-10 Full suite green (`pytest -m "not integration and not system"`), `ruff check --fix && ruff format`, `ty check`, `beacon doctor --strict`
- [x] T-CUT-10 Full suite green (218 passed, only the pre-existing unrelated Dockerfile-python-version test fails), `ruff check --fix && ruff format` clean, `ty check` clean (confirmed the `create_model`/`RandomChoice` diagnostics pre-date this cutover via `git stash` comparison), `beacon doctor --strict` shows only pre-existing/disclosed items (`tdd-commit-discipline` — already documented as an intentional deviation; `epic-gates` — `llamacpp-integration` correctly has no specs yet)
- [x] T-CUT-11 [P] `tasks.md`/`ollama.py`: migration mapping constant (FR-009) — `MIGRATION_MAP` dict, `ollama.py`
- [ ] T-CUT-12 [P] Manual smoke test against a live local Ollama server per `quickstart.md`; update its migration section if the mapping shifted
- [x] T-CUT-12 [P] Ollama was reachable in this environment — ran the real `@pytest.mark.integration` suite (not just a manual walkthrough). 6/8 passed, including the critical ones: unreachable-host error handling, real load/unload against the live server, structured-output retry-then-raise against the live server, temperature-determinism. 2 failures (`test_single_turn_returns_non_empty_response`, `test_multi_turn_receives_context`) — confirmed via direct `curl` to `/api/chat` (bypassing this codebase entirely) that the test model (`lukey03/qwen3.5-9b-abliterated-vision`) itself returns a degenerate empty response server-side; this is the exact pre-existing model unreliability ADR-006 already documented, not a cutover regression.
**Checkpoint**: T-CUT-10 green = all four user stories functional on the generic nodes; T-CUT-11/12 close out US4.
+7 -2
View File
@@ -538,11 +538,16 @@ class ChatCompletion:
parsed = None
response_text = _run_async(
client.chat(
effective_model, messages, llm_options, timeout_secs=float(timeout_secs)
effective_model,
messages,
llm_options,
timeout_secs=float(timeout_secs),
)
)
else:
assert pydantic_model is not None # structured_output implies this was built
assert (
pydantic_model is not None
) # structured_output implies this was built
parsed = _run_async(
client.chat_structured(
effective_model,
+306 -1123
View File
File diff suppressed because it is too large Load Diff
+4 -4
View File
@@ -88,10 +88,10 @@ EXPECTED_NODENAMES = {
"Circuit Breaker",
# Ollama integration nodes (spec 006)
"Ollama Client",
"Ollama Model Selector",
"Ollama Load Model",
"Ollama Unload Model",
"Ollama Chat Completion",
"LLM Model Selector",
"LLM Load Model",
"LLM Unload Model",
"Chat Completion",
"Ollama Option — Temperature",
"Ollama Option — Seed",
"Ollama Option — Max Tokens",