diff --git a/project-management/Roadmap/epics/llm-provider-abstraction.md b/project-management/Roadmap/epics/llm-provider-abstraction.md index 043c2c2..7b0a96a 100644 --- a/project-management/Roadmap/epics/llm-provider-abstraction.md +++ b/project-management/Roadmap/epics/llm-provider-abstraction.md @@ -89,6 +89,18 @@ correction note for full detail. Open question for the next session: does this take priority over `ux-and-install` (active, 1/4 specs shipped), since llama.cpp (issue #15) has no deadline. +**2026-07-11 — properly specced:** the deferred cutover is now fully +inventoried and planned in +`specs/007-llm-provider-abstraction/atomic-cutover-plan.md` — a full +line-by-line read of the ~125 affected references (not an estimate), the +design decisions it surfaced (cache-singleton duplication, `client == +""` equality breaking, bare-string-client backward compat removal, +and a test-layer split so the 35 `_post_json` monkeypatches land at the +right architectural seam), and a 12-step sequenced task list (T-CUT-01 … +T-CUT-12). This is now the authoritative implementation plan for the +cutover — the next BUILD session executes it directly rather than +re-deriving the approach. + `pydantic-ai`'s `StructuredDict` (raw-JSON-Schema output, no Python class) was considered as a lighter-weight alternative to `create_model()` during research and rejected: it performs no pydantic validation at all, which diff --git a/specs/007-llm-provider-abstraction/atomic-cutover-plan.md b/specs/007-llm-provider-abstraction/atomic-cutover-plan.md new file mode 100644 index 0000000..6e7ed09 --- /dev/null +++ b/specs/007-llm-provider-abstraction/atomic-cutover-plan.md @@ -0,0 +1,178 @@ +# Atomic Cutover Plan: Ollama Node Rename → Generic LLM Nodes + +Companion to `tasks.md`'s "⚠️ Correction (2026-07-11)" section and +[issue #16](https://github.com/darth-veitcher/comfydv/issues/16). This is +the properly-specced version of that deferred work — based on a full, +line-by-line inventory of every one of the ~125 affected references in +`src/comfydv/ollama.py` and `tests/test_ollama.py` (1820 lines, read in +full), not an estimate. + +The inventory surfaced that this isn't one mechanical find-and-replace — +several genuine design decisions were implicit in "rename it" and needed +resolving before any code changes. Those decisions are below, followed by +the sequenced task list that implements them. + +## Resolved decisions + +### D1 — Single source of truth for HTTP/cache infra + +`comfydv._llm.ollama_provider` owns `_post_json`, `_run_async`, +`_fetch_models`, `_TTLLRUCache`, `_cache_key`, `_MODEL_LIST_CACHE` (already +ported there — see `ollama_provider.py`). `ollama.py` stops defining its +own copies of these. The one remaining non-node use case — +`_load_default_models()` (combo-widget population at import time, before +any `OllamaClient` node exists) and the `/dv/ollama/models` refresh route — +imports `_fetch_models`/`_run_async` from `comfydv._llm.ollama_provider` +instead of duplicating them. `_CHAT_RESPONSE_CACHE` and `_post_json` +disappear from `ollama.py` entirely — nothing there needs them once +load/unload/chat delegate to `client.*`. + +**Why not keep two copies:** they were already flagged as byte-identical by +the inventory (§2.13) — the only reason `ollama.py` still has them is that +nothing has repointed the imports yet. Keeping a second copy "just in case" +is exactly the kind of duplication ADR-007 exists to eliminate. + +### D2 — `client == ""` equality is removed, not preserved + +`OllamaProvider` is a plain object, not a `str` subclass — this is a +deliberate consequence of the adapter boundary (ADR-007), not an oversight +to work around. Two tests assert string equality on `client` +(`test_client_outputs_ollama_client_type:208-209`, +`test_client_carries_headers:1233-1234`) — both get rewritten to assert +`client.host == "..."` and `isinstance(client, OllamaProvider)`. +`OllamaClientType` (the `str`-subclass, `ollama.py:97-109`) is left in +place but becomes unused by `OllamaClient.create_client()` — not deleted in +this cutover (no test depends on deleting it, and removing a class nothing +references is a separate, lower-risk cleanup, not part of this bullet's +scope). + +### D3 — Bare-string `client` backward compatibility is removed + +`test_plain_string_client_has_no_headers` (`test_ollama.py:1328-1344`) +documents and tests that wiring a plain `STRING` node directly into +`client` (skipping `OllamaClient` entirely) silently works, because +`f"{client}/..."` succeeds on any string. This was never a documented, +intended feature — it's a side effect of `OllamaClientType` being a `str` +subclass, not mentioned in ADR-005 or the original Ollama epic's spec. Once +node methods call `client.chat(...)`/`client.load_model(...)`, a bare +string raises `AttributeError`. **This test is deleted**, not rewritten — +its premise (bare-string clients are supported) is being intentionally +removed, and asserting the new failure mode would just be testing that +Python raises `AttributeError` on missing methods, which isn't +comfydv-specific behavior worth a test. + +The same bare-string pattern appears incidentally in ~8 other tests +(`ollama_host` fixture returns a plain string, used as `client=ollama_host` +in several integration tests — `test_ollama.py:220, 386, 407, 588, ...`). +These need `client=OllamaClient().create_client(ollama_host)[0]` instead of +`client=ollama_host` — a required edit, not optional, since they'll raise +`AttributeError` otherwise. See task T-CUT-08 below. + +### D4 — Test layer split (resolves all 35 relocated `_post_json` monkeypatches) + +This is the biggest structural decision. Today, `test_ollama.py` tests +ComfyUI node behavior by mocking `aiohttp` at the `ollama_mod._post_json` +seam and asserting on the exact Ollama wire payload (`keep_alive`, +`/api/generate`, tool-calling JSON shape, retry counts) *through* the node. +That seam moves — nodes no longer call `_post_json` directly, they call +`client.chat(...)` etc. Two options: (a) keep patching at whatever the new +seam is, 1:1 per test, or (b) recognize this is an architectural boundary +and split coverage accordingly. Going with **(b)**: + +- **`tests/test_ollama.py`** — ComfyUI node **contract + delegation** only. + A new `_FakeProvider` test double (implements `list_models`/ + `load_model`/`unload_model`/`chat`/`chat_structured`, records calls made + to it) stands in for `client`. Tests assert: right method called, right + arguments passed, return value flows through to the node's output tuple + correctly. **No `aiohttp`/`_post_json` mocking at this layer anymore.** + This directly matches the protocol contract's own rule ("generic nodes + MUST NOT branch on which concrete provider type they received") — if the + node tests don't need to know Ollama's wire format, they shouldn't mock + it either. +- **`tests/test_ollama_provider.py`** (new file) — `OllamaProvider`'s + actual Ollama-wire-protocol behavior: `/api/generate`+`keep_alive` int + shape, `/api/tags` parsing into `ModelInfo`, header/timeout forwarding, + response caching, cache-key composition. This is where the *substance* of + today's 35 `_post_json` monkeypatches lands — not 1:1, since several + collapse or move (see D5). +- **`tests/test_llm_chat_structured.py`** (exists, unchanged) — already + covers the shared retry/validation/error-contract mechanism. + +### D5 — Structured-output retry tests are not ported 1:1 + +`TestStructuredOutput` has 15 tests monkeypatching `_post_json` to assert +exact retry-count behavior (`test_retries_on_invalid_json_then_succeeds`, +`test_exhausts_retries_raises_runtime_error`, `test_max_retries_clamped_*`, +etc.). Once `OllamaProvider.chat_structured()` delegates to the already- +tested shared `chat_structured()` helper (`src/comfydv/_llm/chat.py`, +covered by `tests/test_llm_chat_structured.py`'s 6 tests), re-asserting +retry counts at the Ollama-node layer duplicates that coverage without +adding confidence. Replaced with: +- A handful of `test_ollama.py` delegation tests: `ChatCompletion` with + `structured_output=True` calls `client.chat_structured(model, messages, + schema, ...)` with the right schema/model/messages. +- One `test_ollama_provider.py` test: `OllamaProvider.chat_structured()` + builds `base_url=f"{self.host}/v1"` and forwards to the shared helper + with the right arguments. +- `test_structured_output_true_sends_tool_call_payload` (asserts the exact + `tools`/`tool_choice` JSON shape) is **deleted** — that's `pydantic-ai`'s + internal tool-calling mechanism now, not comfydv's; asserting on a + third-party library's internals isn't a test worth keeping. +- Pure schema-parsing tests that don't touch HTTP at all (`_parse_output_schema` + fail-fast checks, `_coerce_structured_value`, dynamic-socket + `RETURN_TYPES` mutation) are unaffected — they test code that stays in + `ollama.py` unchanged, only need the class-name rename. + +### D6 — `_client_headers()` is deleted + +Dead code once `OllamaLoadModel`/`OllamaUnloadModel`/`OllamaChatCompletion` +delegate to `client.*` (headers become internal to `OllamaProvider`, +captured once at construction). No test calls it directly. + +### D7 — Contract doc gets a small fix + +`contracts/llm_provider_protocol.md`'s illustrative code sample is missing +`timeout_secs` on `chat()`/`chat_structured()` — the actual `provider.py` +(built after the doc) has it. Fix the doc to match the real protocol; docs +follow code here, not the reverse. + +### D8 — `conftest.py`'s `_clear_ollama_caches` fixture repoints + +Per D1, there's now one cache source (`comfydv._llm.ollama_provider`). +Fixture imports `_CHAT_RESPONSE_CACHE`/`_MODEL_LIST_CACHE` from there +instead of `comfydv.ollama`, and `ChatCompletion` instead of +`OllamaChatCompletion`. This is the single highest-priority fixture change +— every `TestResponseCache` test (12) and every `structured_output`- +toggling test depends on it for isolation. + +## Sequenced task list + +Replaces `tasks.md`'s Phase 3 (US1) + Phase 5 (US3) + the deferred T014. +One coordinated PR/session, ordered so the codebase stays important at each +step even though it can't be split across separate merges (per the +2026-07-11 correction — this is genuinely atomic). + +1. **T-CUT-01** — `ollama.py`: add `from comfydv._llm.ollama_provider import OllamaProvider, _fetch_models, _run_async` (drop the local `_post_json`, `_TTLLRUCache`, `_cache_key`, `_MODEL_LIST_CACHE`, `_CHAT_RESPONSE_CACHE`, `_run_async`, `_fetch_models`, `_post_json` definitions — lines 42-194 collapse to the import). Repoint `_load_default_models()` and the `/dv/ollama/models` route to the imported `_fetch_models`. (D1) +2. **T-CUT-02** — `ollama_provider.py`: implement `OllamaProvider.list_models()` (port `_fetch_models`'s `/api/tags` logic, map to `ModelInfo`/`ModelStatus.UNLOADED`/`LOADED` — Ollama never emits `SLEEPING`/`DOWNLOADING`, per ADR-007's documented approximation), `load_model()` (port `/api/generate` + `keep_alive: -1`), `unload_model()` (port `/api/generate` + `keep_alive: 0`), `chat()` (port native `/api/chat` non-structured path), `chat_structured()` (build `base_url=f"{self.host}/v1"`, delegate to `comfydv._llm.chat.chat_structured()`). +3. **T-CUT-03** — `tests/test_ollama_provider.py` (new): tests for T-CUT-02's method bodies, mocking at `ollama_provider_mod._post_json`/`aiohttp.ClientSession` — ports the *substance* of the 35 relocated monkeypatches per D4/D5 (not 1:1 — collapses redundant retry-count tests per D5). +4. **T-CUT-04** — `ollama.py`: `OllamaClient.RETURN_TYPES = ("LLM_CLIENT",)`, `create_client()` returns `OllamaProvider(host, headers)`. (D2) +5. **T-CUT-05** — `ollama.py`: rename `OllamaModelSelector`→`LLMModelSelector`, `OllamaLoadModel`→`LLMLoadModel`, `OllamaUnloadModel`→`LLMUnloadModel`, `OllamaChatCompletion`→`ChatCompletion`; every `"OLLAMA_CLIENT"` input socket → `"LLM_CLIENT"`; rewrite the 3 method bodies (`load_model`, `unload_model`, `chat`) to delegate to `client.*` instead of `_post_json`/f-string URLs; delete `_client_headers` (D6); update the 3 `OllamaChatCompletion.*` references in the `/dv/ollama/update_structured_outputs` route body. +6. **T-CUT-06** — `src/comfydv/__init__.py`: update imports and `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS` for the 4 renamed classes. +7. **T-CUT-07** — `tests/conftest.py`: repoint `_clear_ollama_caches` (D8) and `first_generative_model`'s `_fetch_models` import (D1). +8. **T-CUT-08** — `tests/test_ollama.py`: update the import block (4 class renames); add `_FakeProvider` test double; convert every `_post_json`-monkeypatched test to use `_FakeProvider` as `client` instead (D4); replace bare-string `client=ollama_host`/`client="http://..."` usages with a constructed provider (D3); rewrite the 2 `client == ""` assertions (D2); delete `test_plain_string_client_has_no_headers` (D3) and `test_structured_output_true_sends_tool_call_payload` (D5); collapse the 15 `TestStructuredOutput` retry-count tests per D5; update `TestNodeContracts`'s `NODE_CLASSES` list (4 renames). +9. **T-CUT-09** — `contracts/llm_provider_protocol.md`: add missing `timeout_secs` params (D7). +10. **T-CUT-10** — Full suite green (`uv run pytest -m "not integration and not system"`), `ruff check --fix && ruff format`, `ty check`, `beacon doctor --strict`. +11. **T-CUT-11** — `tasks.md`: mark T007-T010/T015-T018/T014 done, referencing this plan; migration mapping (old→new names, FR-009) as a module-level constant/docstring in `ollama.py`. +12. **T-CUT-12** — Manual smoke test against a live local Ollama server per `quickstart.md`. + +## What stays exactly as originally scoped + +`OllamaHeader*`, `OllamaOption*`, `OllamaDebugHistory`, `OllamaHistoryLength` +classes and the `OLLAMA_HEADERS`/`OLLAMA_OPTIONS`/`OLLAMA_HISTORY` socket +types are **out of scope** — confirmed zero test dependencies force a +change, and ADR-007 never proposed touching them (only the +model-management/chat surface generalizes). `_parse_output_schema`, +`_comfy_types_for_schema`, `_build_structured_model`, +`_coerce_structured_value` stay in `ollama.py` unchanged — pure/local +schema logic with no network dependency, still needed by the live-preview +route. diff --git a/specs/007-llm-provider-abstraction/contracts/llm_provider_protocol.md b/specs/007-llm-provider-abstraction/contracts/llm_provider_protocol.md index ec52d3f..9b69a31 100644 --- a/specs/007-llm-provider-abstraction/contracts/llm_provider_protocol.md +++ b/specs/007-llm-provider-abstraction/contracts/llm_provider_protocol.md @@ -28,10 +28,12 @@ class LLMProvider(Protocol): async def load_model(self, model: str) -> None: ... async def unload_model(self, model: str) -> None: ... async def chat( - self, model: str, messages: list[Message], options: dict + self, model: str, messages: list[Message], options: dict | None = None, + timeout_secs: float = 300.0, ) -> str: ... async def chat_structured( - self, model: str, messages: list[Message], schema: type[BaseModel], options: dict + self, model: str, messages: list[Message], schema: type[BaseModel], + options: dict | None = None, timeout_secs: float = 300.0, max_retries: int = 2, ) -> BaseModel: ... ``` diff --git a/specs/007-llm-provider-abstraction/tasks.md b/specs/007-llm-provider-abstraction/tasks.md index 4b1f5e4..2760b7c 100644 --- a/specs/007-llm-provider-abstraction/tasks.md +++ b/specs/007-llm-provider-abstraction/tasks.md @@ -46,16 +46,25 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje **Independent Test**: Wire `OllamaClient` → `ChatCompletion`, run against a live server, confirm text output. -- [ ] T007-T [US1] Write FAILING test: `OllamaClient` node constructs and outputs an `OllamaProvider` via the `LLM_CLIENT` socket, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "Client node feeds a chat node") -- [ ] T007-I [US1] Update `OllamaClient` in `src/comfydv/ollama.py` to construct and output an `OllamaProvider` via `LLM_CLIENT` — makes T007-T pass -- [ ] T008-T [P] [US1] Write FAILING test: `OllamaProvider.chat()` returns model text via the existing `/api/chat` aiohttp call, in `tests/test_llm_provider.py` -- [ ] T008-I [US1] Implement `OllamaProvider.chat()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing non-structured path from `OllamaChatCompletion.chat`) — makes T008-T pass -- [ ] T009-T [US1] Write FAILING test: generic `ChatCompletion` node (non-structured) calls `provider.chat()` and surfaces a clear error on an unreachable host, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenarios "Client node feeds a chat node" and "Unreachable server surfaces a clear error") -- [ ] T009-I [US1] Rename `OllamaChatCompletion` → `ChatCompletion` in `src/comfydv/ollama.py`, delegate the non-structured path to `LLMProvider.chat()`, update `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS` in `src/comfydv/__init__.py` — makes T009-T pass (depends on T007-I, T008-I) -- [ ] T010-T [US1] Write FAILING test: two `ChatCompletion` nodes sharing one `OllamaClient` both pick up a host change, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "One client node configures multiple chat nodes") -- [ ] T010-I [US1] Verify/adjust that `OllamaClient` → `OllamaProvider` construction happens per node execution (not stale-cached), so a shared client's host change propagates — makes T010-T pass +**Superseded 2026-07-11 — see `atomic-cutover-plan.md`.** T007–T010 as +written below assumed US1 was independently deliverable; it isn't (see the +correction at the bottom of this file). The actual work is now +`atomic-cutover-plan.md`'s **T-CUT-04, T-CUT-05, T-CUT-06, T-CUT-08** +(`OllamaClient` output-type change, `ChatCompletion` rename+delegation, +`__init__.py` registration, and the corresponding `test_ollama.py` +rewrite using a `_FakeProvider` double). Original text kept below for +history, not as the active task list: -**Checkpoint**: US1 fully functional and independently testable (MVP). +- [ ] ~~T007-T [US1] Write FAILING test: `OllamaClient` node constructs and outputs an `OllamaProvider` via the `LLM_CLIENT` socket, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "Client node feeds a chat node")~~ +- [ ] ~~T007-I [US1] Update `OllamaClient` in `src/comfydv/ollama.py` to construct and output an `OllamaProvider` via `LLM_CLIENT` — makes T007-T pass~~ +- [ ] ~~T008-T [P] [US1] Write FAILING test: `OllamaProvider.chat()` returns model text via the existing `/api/chat` aiohttp call, in `tests/test_llm_provider.py`~~ +- [ ] ~~T008-I [US1] Implement `OllamaProvider.chat()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing non-structured path from `OllamaChatCompletion.chat`) — makes T008-T pass~~ +- [ ] ~~T009-T [US1] Write FAILING test: generic `ChatCompletion` node (non-structured) calls `provider.chat()` and surfaces a clear error on an unreachable host, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenarios "Client node feeds a chat node" and "Unreachable server surfaces a clear error")~~ +- [ ] ~~T009-I [US1] Rename `OllamaChatCompletion` → `ChatCompletion` in `src/comfydv/ollama.py`, delegate the non-structured path to `LLMProvider.chat()`, update `NODE_CLASS_MAPPINGS`/`NODE_DISPLAY_NAME_MAPPINGS` in `src/comfydv/__init__.py` — makes T009-T pass (depends on T007-I, T008-I)~~ +- [ ] ~~T010-T [US1] Write FAILING test: two `ChatCompletion` nodes sharing one `OllamaClient` both pick up a host change, in `tests/test_ollama.py` (witnesses `features/us1_connect_and_chat.feature` scenario "One client node configures multiple chat nodes")~~ +- [ ] ~~T010-I [US1] Verify/adjust that `OllamaClient` → `OllamaProvider` construction happens per node execution (not stale-cached), so a shared client's host change propagates — makes T010-T pass~~ + +**Checkpoint**: superseded — see `atomic-cutover-plan.md`'s checkpoint (T-CUT-10, full suite green). --- @@ -71,8 +80,8 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje - [x] T012-I [US2] Implement the bounded retry loop (0–5, matching ADR-006's existing contract) around the `pydantic-ai` call in `src/comfydv/_llm/chat.py`, with the Agent's own internal retries disabled (`retries=0`) so the error contract is comfydv's — makes T012-T pass (depends on T011-I) - [x] T013-T [US2] Write test: exhausted retries raise `RuntimeError` naming the model, attempt count, and a truncated last-response snippet, in `tests/test_llm_chat_structured.py` (witnesses `features/us2_structured_output.feature` scenario "Exhausted retries fail clearly instead of passing through bad data") - [x] T013-I [US2] Implement the exhausted-retry error path in `src/comfydv/_llm/chat.py`, matching ADR-006's existing error message contract (model, attempt count, truncated last response) — makes T013-T pass (depends on T012-I) -- [-] T014-T [US2] Write FAILING test: `ChatCompletion`'s `structured_output=True` path wires a schema through `chat_structured()` to per-field dynamic ComfyUI output sockets, in `tests/test_ollama.py` _Deferred — folded into the atomic node cutover (issue #16); this touches `ollama.py`, which per the 2026-07-11 correction above cannot land independently of the US1/US3 rename._ -- [-] T014-I [US2] Wire `ChatCompletion`'s existing `structured_output`/`output_schema` inputs to `LLMProvider.chat_structured()`, preserving the existing dynamic-socket (`RETURN_TYPES` mutation via `unique_id`) UX from ADR-006 — makes T014-T pass (depends on T009-I, T013-I) _Deferred — same reason as T014-T._ +- [-] T014-T [US2] _Superseded — see `atomic-cutover-plan.md` D5 and T-CUT-08. Original: write FAILING test that `ChatCompletion`'s `structured_output=True` path wires a schema through `chat_structured()` to per-field dynamic ComfyUI output sockets. D5 replaces the originally-planned retry-count-style test with a delegation test against a `_FakeProvider`, since retry behavior is already covered by `tests/test_llm_chat_structured.py`._ +- [-] T014-I [US2] _Superseded — see `atomic-cutover-plan.md` T-CUT-05. Original: wire `ChatCompletion`'s `structured_output`/`output_schema` inputs to `LLMProvider.chat_structured()`, preserving the dynamic-socket UX from ADR-006 — still the right implementation shape, just executed as part of the coordinated T-CUT-05 rename, not standalone._ **Checkpoint**: US1 + US2 both independently functional — matches today's Ollama capability, now on the shared mechanism. @@ -84,16 +93,22 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje **Independent Test**: List models via `LLMModelSelector`; load/unload one via `LLMLoadModel`/`LLMUnloadModel`; confirm status changes. -- [ ] T015-T [P] [US3] Write FAILING test: `OllamaProvider.list_models()` returns `ModelInfo` entries with status normalized into `ModelStatus` (never emitting `sleeping`/`downloading`), in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenario "List models with current status") -- [ ] T015-I [US3] Implement `OllamaProvider.list_models()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `_fetch_models`/`/api/tags` logic, reusing the existing `_TTLLRUCache`) — makes T015-T pass (depends on T006) -- [ ] T016-T [P] [US3] Write FAILING test: `OllamaProvider.load_model()`/`unload_model()` are idempotent and use `keep_alive` semantics equivalent to today's `OllamaLoadModel`/`OllamaUnloadModel`, in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenarios "Load a model into memory" and "Unload a model from memory") -- [ ] T016-I [US3] Implement `OllamaProvider.load_model()`/`unload_model()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `keep_alive` logic) — makes T016-T pass (depends on T006) -- [ ] T017-T [US3] Write FAILING test: generic `LLMModelSelector` node returns model+status pairs from any connected `LLM_CLIENT`, in `tests/test_ollama.py` -- [ ] T017-I [US3] Rename `OllamaModelSelector` → `LLMModelSelector` in `src/comfydv/ollama.py`, delegate to `LLMProvider.list_models()`, update `NODE_CLASS_MAPPINGS` — makes T017-T pass (depends on T015-I) -- [ ] T018-T [US3] Write FAILING test: generic `LLMLoadModel`/`LLMUnloadModel` nodes call `LLMProvider.load_model()`/`unload_model()` and reflect updated status, in `tests/test_ollama.py` -- [ ] T018-I [US3] Rename `OllamaLoadModel`/`OllamaUnloadModel` → `LLMLoadModel`/`LLMUnloadModel` in `src/comfydv/ollama.py`, delegate to the protocol, update `NODE_CLASS_MAPPINGS` — makes T018-T pass (depends on T016-I) +**Superseded 2026-07-11 — see `atomic-cutover-plan.md`.** Maps to +**T-CUT-02** (`OllamaProvider.list_models`/`load_model`/`unload_model` +method bodies), **T-CUT-03** (`tests/test_ollama_provider.py`, new file), +and **T-CUT-05/T-CUT-06/T-CUT-08** (the node renames + delegation + +registration + test rewrite). Original text kept for history: -**Checkpoint**: US1 + US2 + US3 independently functional. +- [ ] ~~T015-T [P] [US3] Write FAILING test: `OllamaProvider.list_models()` returns `ModelInfo` entries with status normalized into `ModelStatus` (never emitting `sleeping`/`downloading`), in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenario "List models with current status")~~ +- [ ] ~~T015-I [US3] Implement `OllamaProvider.list_models()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `_fetch_models`/`/api/tags` logic, reusing the existing `_TTLLRUCache`) — makes T015-T pass (depends on T006)~~ +- [ ] ~~T016-T [P] [US3] Write FAILING test: `OllamaProvider.load_model()`/`unload_model()` are idempotent and use `keep_alive` semantics equivalent to today's `OllamaLoadModel`/`OllamaUnloadModel`, in `tests/test_llm_provider.py` (witnesses `features/us3_model_lifecycle.feature` scenarios "Load a model into memory" and "Unload a model from memory")~~ +- [ ] ~~T016-I [US3] Implement `OllamaProvider.load_model()`/`unload_model()` in `src/comfydv/_llm/ollama_provider.py` (port of the existing `keep_alive` logic) — makes T016-T pass (depends on T006)~~ +- [ ] ~~T017-T [US3] Write FAILING test: generic `LLMModelSelector` node returns model+status pairs from any connected `LLM_CLIENT`, in `tests/test_ollama.py`~~ +- [ ] ~~T017-I [US3] Rename `OllamaModelSelector` → `LLMModelSelector` in `src/comfydv/ollama.py`, delegate to `LLMProvider.list_models()`, update `NODE_CLASS_MAPPINGS` — makes T017-T pass (depends on T015-I)~~ +- [ ] ~~T018-T [US3] Write FAILING test: generic `LLMLoadModel`/`LLMUnloadModel` nodes call `LLMProvider.load_model()`/`unload_model()` and reflect updated status, in `tests/test_ollama.py`~~ +- [ ] ~~T018-I [US3] Rename `OllamaLoadModel`/`OllamaUnloadModel` → `LLMLoadModel`/`LLMUnloadModel` in `src/comfydv/ollama.py`, delegate to the protocol, update `NODE_CLASS_MAPPINGS` — makes T018-T pass (depends on T016-I)~~ + +**Checkpoint**: superseded — see `atomic-cutover-plan.md`. --- @@ -103,10 +118,9 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje **Independent Test**: Follow the mapping to reconnect a pre-upgrade workflow; confirm equivalent output. -- [ ] T019-T [US4] Write FAILING test: every renamed/removed node and socket name (`OllamaChatCompletion`, `OllamaModelSelector`, `OllamaLoadModel`, `OllamaUnloadModel`, `OLLAMA_CLIENT`) resolves to a documented replacement via a migration-mapping constant, in `tests/test_ollama.py` (witnesses `features/us4_migration.feature` scenario "Renamed nodes are reported with a documented replacement") -- [ ] T019-I [US4] Add a migration mapping (old node/socket name → new node/socket name) as a module-level constant in `src/comfydv/ollama.py`, and surface it in the replacement nodes' `DESCRIPTION`/docstrings per FR-009 — makes T019-T pass (depends on T009-I, T017-I, T018-I) -- [ ] T020 [US4] Run the full `tests/test_ollama.py` suite against the migrated implementation and confirm every existing assertion still passes unmodified in behavior (SC-003 equivalence check; witnesses `features/us4_migration.feature` scenario "Reconnected workflow produces equivalent output") — regression pass, not a new test -- [ ] T021 [P] [US4] Update `quickstart.md`'s "Migrating an existing pre-upgrade workflow" section if the actual replacement mapping (T019-I) differs from what was drafted at plan time +- [ ] T019 [US4] _Maps to `atomic-cutover-plan.md` T-CUT-11_ — add a migration mapping (old node/socket name → new node/socket name) as a module-level constant in `src/comfydv/ollama.py`, surfaced in the replacement nodes' docstrings per FR-009 +- [ ] T020 [US4] _Maps to T-CUT-10_ — full `tests/test_ollama.py` + `tests/test_ollama_provider.py` suite green, confirming SC-003 equivalence (witnesses `features/us4_migration.feature` scenario "Reconnected workflow produces equivalent output") +- [ ] T021 [P] [US4] _Maps to T-CUT-12_ — update `quickstart.md`'s "Migrating an existing pre-upgrade workflow" section to match the actual replacement mapping, then walk it manually against a live local server **Checkpoint**: all four user stories independently functional; migration path documented. @@ -114,7 +128,7 @@ Single project: `src/comfydv/`, `tests/` at repository root (per plan.md's Proje ## Phase 7: Polish & Cross-Cutting Concerns -- [ ] T022 [P] Run `uv run ruff check --fix && uv run ruff format` across `src/comfydv/_llm/` and the modified `ollama.py`/`__init__.py` +- [ ] T022 [P] Run `uv run ruff check --fix && uv run ruff format` across `src/comfydv/_llm/`, `tests/test_ollama_provider.py`, and the modified `ollama.py`/`__init__.py` - [ ] T023 [P] Run `uv run ty check` and resolve any typing errors introduced by the `LLMProvider` `Protocol` - [ ] T024 Confirm Constitution Principle IV: `src/comfydv/_llm/` imports no `comfy`/`server` at module scope (guarded, matching `ollama.py`'s existing pattern) - [ ] T025 Run `beacon doctor --strict` and resolve any findings, including `spec-bdd-coverage` and `tdd-commit-discipline` for this spec @@ -203,3 +217,18 @@ ADR-007's own decision (breaking rename, no deprecated aliases) is **unaffected** — that call was about user-facing blast radius (small, Ollama integration shipped 2026-07-04), which this finding doesn't change. What's re-scoped is delivery sequencing, not the design decision. + +--- + +## ✅ Properly specced (2026-07-11) — see `atomic-cutover-plan.md` + +Full line-by-line inventory of every affected reference in `ollama.py` and +`tests/test_ollama.py` (1820 lines, read in full), the design decisions it +surfaced (cache-singleton duplication, `client == ""` equality +breaking, bare-string-client backward compat removal, and — the big one — +a test-layer split so the 35 relocated `_post_json` monkeypatches land at +the right architectural seam instead of being patched 1:1), and a +12-step sequenced task list (T-CUT-01 … T-CUT-12) that supersedes the +struck-through tasks above. That file is now the authoritative task list +for this remaining work; this file's Phase 3/5/6 entries are kept only for +history.