test(runtimes): port-contract suite for new adapters (#114)

[Archita-kale]

Closes #114

Summary

Adds comprehensive port-contract tests for all runtime adapters, split by
behavioral category so each adapter is tested the way it actually behaves
rather than forced into one generic parametrization:

• Universal — properties every RuntimePort must satisfy (backend
  identity, status(), default_tag_for(), category/capability lookups).
  Runs against all 7 adapters.
• Lifecycle — FakeRuntime, OllamaRuntime, LMStudioRuntime, exercised
  with fake/mocked clients (no real daemon required).
• Server-bound — LlamaCppRuntime (exactly one model per process),
  verifying it fails informatively rather than silently.
• Planned/offline — JanRuntime, Gpt4AllRuntime, VllmRuntime, verifying
  consistent RuntimeUnavailableError behavior.
• Registry coverage — a new test asserts every backend registered in
  RuntimeRegistry has contract coverage, so a future adapter can't be
  added without tests.

Also added CachePort.path() to FakeCache (previously missing) and
DownloaderPort structural-conformance tests.

No runtime behavior in src/modeldock/ is modified — tests and docs only.

Testing

• pytest tests/unit -k contract -v — 96 passed, 485 deselected
• pytest tests/unit — 581 passed
• ruff check src tests — All checks passed
• mypy src — Success: no issues found in 79 source files
• bandit -c pyproject.toml -r src — No issues identified

[himanshu231204]

Nice restructuring — splitting the suite by behavioral category (universal / lifecycle / server-bound / planned) is a real improvement over one flat parametrization, and test_registry_backend_coverage is a good guardrail. A few things worth fixing before merge:

1. test_downloader_pull_signature is a tautology and can never fail (tests/unit/test_port_contract.py):

   try:
       downloader_impl.pull(ref)
   except Exception as exc:  # noqa: BLE001 - any typed/domain error is fine
       assert isinstance(exc, Exception)

   except Exception as exc only catches things that are already Exception instances, so isinstance(exc, Exception) is true by construction — this branch can never fail, and the "success" path (no exception) is also unchecked. As written this test provides no signal at all; it'll pass even if pull() starts raising BaseException subclasses that aren't Exception, returns garbage, or a future adapter silently no-ops. If the intent is "pull() either returns without crashing the process, or raises a well-formed error," consider asserting something concrete on the exception (e.g. it's one of the domain error types, or has a non-empty message), or drop the try/except and just assert on the return value / expected exception type per-adapter.

2. Minor: tests/unit/test_port_contract.py is missing a trailing newline at EOF (same nit as #220's test_platform.py — might be worth a .editorconfig/pre-commit rule to catch this project-wide).

3. Minor: the FakeRuntime lifecycle factory does a somewhat awkward dynamic import —

   "FakeRuntime": lambda: __import__("tests.conftest", fromlist=["FakeRuntime"]).FakeRuntime(),

   — when tests.conftest is already imported directly elsewhere in the file (from tests.conftest import FakeRuntime inside _all_runtime_implementations). Could just import FakeRuntime once at module level and reference it directly; simpler and avoids the __import__ indirection.

4. Minor style: the CachePort.path() addition to FakeCache in tests/conftest.py introduces a blank line with trailing whitespace between status() and path(). Not currently flagged by the ruff config (W isn't in select), but worth a quick cleanup pass.

None of these are blocking for the overall direction, but #1 should be tightened since it's presented as real DownloaderPort contract coverage and currently isn't.

---

[Archita-kale]

Thanks for the detailed review — fixed all four points in 785dce0:

1. test_downloader_pull_signature tautology (real bug, fixed)
   You're right that except Exception as exc: assert isinstance(exc, Exception) could never fail. Replaced it with a concrete assertion:

python
with pytest.raises(DownloadError) as exc_info:
downloader_impl.pull(ref)
assert str(exc_info.value)

Verified both implementations actually raise DownloadError with a non-empty message in this environment (HttpDownloader.pull() is intentionally unsupported and always raises it; OllamaPullDownloader.pull() delegates to the real Ollama runtime, which isn't running here, so it wraps the resulting RuntimeUnavailableError as DownloadError). I also sanity-checked the new test actually catches a broken implementation — a fake downloader that silently returns None on pull() correctly fails the test now, which it didn't before.

2. Trailing newline at EOF
   Confirmed tests/unit/test_port_contract.py ends with a single \n. Agreed a .editorconfig or pre-commit hook would catch this project-wide going forward — happy to open that as a separate follow-up if useful.

3. import indirection for FakeRuntime
   Replaced with a single module-level from tests.conftest import FakeCache, FakeRuntime and removed the two now-redundant local imports in _all_runtime_implementations() and cache_factory().

4. Trailing whitespace in conftest.py
   Checked byte-for-byte — the blank line between status() and path() doesn't have trailing whitespace in what I pushed. If it's still showing up on your end let me know and I'll dig further (might have been an artifact of how it was originally viewed).

Re-ran the full gate after the fix:

pytest tests/unit -k contract -v — 96 passed, 485 deselected
pytest tests/unit — 581 passed
ruff check src tests — All checks passed
mypy src — Success: no issues found in 79 source files
bandit -c pyproject.toml -r src — No issues identified

[graphify-labs]

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

---

Graphify review — findings

Restructures the port-contract suite into capability-scoped groups—universal RuntimePort properties across all seven adapters, in-process lifecycle tests for FakeRuntime/OllamaRuntime/LMStudioRuntime via fake clients, server-bound checks that LlamaCppRuntime fails informatively on pull/remove, and planned adapters (Jan/Gpt4All/Vllm) that must raise RuntimeUnavailableError. Adds test_registry_backend_coverage so any backend registered in RuntimeRegistry without an entry in _all_runtime_implementations() fails loudly instead of shipping untested. Gives FakeCache a path() returning /fake/cache and updates the testing docs to match the new structure.

Worth a look

• Unit contract test calls live Ollama pull path — tests/unit/test_port_contract.py:477 · Escalate · medium
  • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 146 functions depend on the 123 functions this change touches.

Health — this change adds coupling hotspots:

• new: FakeRuntime — 23 callers, 9 callees
• new: FakeRegistry — 16 callers, 7 callees
• new: _manager() — 10 callers, 3 callees
• new: _all_runtime_implementations() — 2 callers, 7 callees

Verification — 146 functions in the blast radius were not formally verified this run (proofs are advisory here).

Health delta baseline: last indexed commit 50084a7 (diverged from this PR's base — delta is approximate).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

• verification_scope: 146 function(s) in the blast radius were not formally verified this run

· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).

[graphify-labs]

⚠️ Health regression — _all_runtime_implementations()

fans out to 7 callees (efferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

[Archita-kale]

Thanks for the review! It flagged the coupling in _all_runtime_implementations(), but this is intentional since the helper is meant to collect all runtime implementations for the contract tests. I also ran pytest tests/unit/test_port_contract.py locally and all 96 tests passed. So I’ve left this unchanged for now, unless you’d prefer a different approach.

[himanshu231204]

Code Review Suggestions

The overall structure — universal / lifecycle / server-bound / planned tiers plus the registry-coverage sentinel — is the right design for this kind of contract suite. A few concrete issues to address:

---

Critical

1. test_llamacpp_remove_raises_when_available likely tests the wrong code path

def test_llamacpp_remove_raises_when_available() -> None:
    runtime = _fake_llamacpp_runtime(model_id="model.gguf")
    with pytest.raises(DownloadError):
        runtime.remove(ModelRef.parse("model.gguf"))

LlamaCppRuntime.remove() raises DownloadError only in the if ref.is_cloud: branch — for non-cloud models it falls through to self._require_available() and then an HTTP call (which may or may not raise DownloadError depending on the fake client). If ModelRef.parse("model.gguf") accidentally satisfies is_cloud, the test passes because of the cloud guard, not because of the server-bound constraint it claims to test.

Fix: use a ref that clearly has is_cloud=False, assert the error message indicates the operation is unsupported (not a cloud-model issue), and confirm this is testing the "server-bound = no remove API" path, not the cloud-model guard. Consider also testing a non-GGUF ref so the two paths are clearly distinct.

---

2. params=_all_runtime_implementations() instantiates 7 adapters at module import time

The @pytest.fixture(params=...) argument is evaluated when the decorator is applied — not at test collection or at test execution time. Every adapter in _all_runtime_implementations() is constructed once during import. This means if any adapter's __init__ has side effects (environment probing, SDK imports that call home, etc.), they fire for every pytest invocation whether or not contract tests are selected. Currently OllamaRuntime.__init__ is safe (no network probe), but LMStudioRuntime, LlamaCppRuntime, and the planned runtimes have not been verified.

Fix: defer instantiation to fixture execution by using a factory approach:

_ALL_RUNTIME_FACTORIES: dict[str, Callable[[], RuntimePort]] = {
    "FakeRuntime": FakeRuntime,
    "OllamaRuntime": OllamaRuntime,
    "LMStudioRuntime": LMStudioRuntime,
    "LlamaCppRuntime": LlamaCppRuntime,
    "JanRuntime": JanRuntime,
    "Gpt4AllRuntime": Gpt4AllRuntime,
    "VllmRuntime": VllmRuntime,
}

@pytest.fixture(params=list(_ALL_RUNTIME_FACTORIES), ids=list(_ALL_RUNTIME_FACTORIES))
def any_runtime(request: pytest.FixtureRequest) -> RuntimePort:
    return _ALL_RUNTIME_FACTORIES[request.param]()

Update test_registry_backend_coverage to instantiate adapters from the factory dict for the backend-set comparison.

---

Important

3. Private-attribute patching couples the fake runtimes to implementation internals

runtime._client = _FakeOllamaClient()        # type: ignore[attr-defined]
runtime._availability = True                  # type: ignore[attr-defined]
runtime._ensure_http_client = lambda: ...     # type: ignore[assignment]

The # type: ignore comments flag this explicitly, but it's worth documenting why this is acceptable here (no other seam exists at the port boundary without spinning up real daemons) so future refactors of _client / _availability know these tests need updating. A one-line comment per patch point explaining the seam would help — e.g.:

# Bypass HTTP probe — OllamaRuntime checks availability lazily on first use.
runtime._availability = True

---

4. Planned-runtime tests don't cover get_model_client()

The planned-runtime tier tests list_installed, pull, remove, and status — all of which correctly raise RuntimeUnavailableError. But get_model_client() is conspicuously absent. If a planned runtime returns a stub client rather than raising, the lifecycle tests would expose it through the lifecycle fixture, but the planned fixture won't. Add:

def test_planned_runtime_get_client_raises(planned_runtime: RuntimePort) -> None:
    with pytest.raises(RuntimeUnavailableError):
        planned_runtime.get_model_client(ModelRef.parse("anything"))

---

5. _REAL_LIFECYCLE_FACTORIES duplicates _LIFECYCLE_FACTORIES minus FakeRuntime

_REAL_LIFECYCLE_FACTORIES is defined separately to drive real_lifecycle_runtime, which backs exactly one test (test_lifecycle_get_client_before_install_raises). This adds a second dict that's a strict subset of the first, which readers must diff manually to understand why.

Two cleaner options:

• Parametrize the test directly over [_fake_ollama_runtime, _fake_lmstudio_runtime] without a separate fixture.
• Add a pytest mark (@pytest.mark.skipif or a custom real_adapter mark) on the lifecycle_runtime fixture and filter inside the test.

The docstring on test_lifecycle_get_client_before_install_raises already explains the FakeRuntime exception clearly — surfacing that reasoning at the parametrize level would make it self-documenting.

---

Minor

6. FakeCache.path() returns a hardcoded Unix path (tests/conftest.py)

def path(self) -> str:
    return "/fake/cache"

On Windows this string is a valid str but an invalid path. If any consumer calls Path(impl.path()) on Windows, the result would resolve relative to the current drive root rather than the intended fake location. The test test_cache_path_returns_str only checks isinstance(path, str) and path, so this passes everywhere. Consider using str(Path(tempfile.gettempdir()) / "fake_cache") or simply "fake/cache" (relative) to avoid the implicit platform assumption.

---

7. PR title has 3 leading spaces

The PR title reads " test(runtimes): port-contract suite for new adapters (#114)" — three spaces before test. Minor but shows up in GitHub's commit log. Consider editing before merge.

---

Strengths

• Splitting into universal / lifecycle / server-bound / planned tiers is the correct call — a flat parametrization would either over-assert on capable adapters or under-assert on limited ones.
• test_registry_backend_coverage is an excellent sentinel: it actively prevents a registered adapter from shipping without contract coverage, which is exactly what Tests: port-contract suite for new adapters #114 asks for.
• The any_runtime/planned_runtime comment explaining why FakeRuntime is excluded from test_lifecycle_get_client_before_install_raises is clear and avoids future confusion.
• The _FakeOllamaClient streaming branch (return iter([...]) when stream=True) correctly mimics the real SDK's streaming behavior.
• The lifecycle list_installed assertion comparing by (name, tag) rather than full ModelRef equality is a thoughtful concession to real adapter behavior (backends stamping their own backend field onto returned refs).