From dad1b61383dba24c9ea9e8b23bd3e8b03dd73a88 Mon Sep 17 00:00:00 2001 From: zalex Date: Mon, 3 Aug 2026 12:52:24 -0700 Subject: [PATCH 1/3] Add LM Studio as a local model provider Second keyless local provider alongside Ollama, riding the same OpenAI-compatible /v1 path (OpenAIProvider with a placeholder key, default http://localhost:1234): - registry: lmstudio descriptor + builder; _normalize_ollama_url generalized to _normalize_local_url(url, default); the keyless verify branch (GET /v1/models, no auth) now covers both local providers - capabilities: lmstudio gets the same conservative local-model defaults as ollama (tools yes, parallel/vision no) - manager: _ollama_alive/_ollama_models generalized to _local_alive/_local_models driven by a LOCAL_MODEL_SERVERS table (ollama via native /api/tags, LM Studio via OpenAI-shaped /v1/models); picker gating and suggestions follow - GUI: lobe-icons LM Studio mark, gallery order, and the keyless install-help copy generalized into a LOCAL_HELP map - tests mirror the existing Ollama coverage (normalize/build/verify/ capabilities/liveness gating) plus a parse test for both list shapes Co-Authored-By: Claude Fable 5 --- README.md | 6 +- coworker/providers/capabilities.py | 7 +- coworker/providers/registry.py | 57 +++++++++--- coworker/server/manager.py | 91 ++++++++++++------- surfaces/gui/src/providers/ProviderSetup.tsx | 21 +++-- surfaces/gui/src/providers/logos.ts | 3 + surfaces/gui/src/providers/logos/lmstudio.svg | 1 + tests/test_provider_router.py | 43 +++++++-- tests/test_provider_verify.py | 8 ++ tests/test_settings.py | 57 +++++++++++- 10 files changed, 225 insertions(+), 69 deletions(-) create mode 100644 surfaces/gui/src/providers/logos/lmstudio.svg diff --git a/README.md b/README.md index b32a299d..2e3138db 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ **AI that gets your everyday tasks done.** OpenWorker is an open-source AI coworker that lives on your desktop and delivers **finished work**, not just chat: a polished document, a Slack reply with the numbers, an updated calendar, a triaged inbox. -It runs on your machine and doesn't lock you into any model: bring your own API key for OpenAI, Anthropic, Google, or an open-weight provider, or run fully local with Ollama. Your data leaves your machine only through the model and integrations *you* choose. +It runs on your machine and doesn't lock you into any model: bring your own API key for OpenAI, Anthropic, Google, or an open-weight provider, or run fully local with Ollama or LM Studio. Your data leaves your machine only through the model and integrations *you* choose. [![How OpenWorker works](docs/assets/how-it-works.png)](https://openworker.com) @@ -20,7 +20,7 @@ It runs on your machine and doesn't lock you into any model: bring your own API [**⬇ Windows 10/11 (x64)**](https://download.openworker.com/windows) builds are not yet code-signed, so SmartScreen will warn; signing is in progress -Open the app, add a model key (or point it at Ollama), and ask for something real. +Open the app, add a model key (or point it at Ollama or LM Studio), and ask for something real. ## How it works @@ -54,7 +54,7 @@ Under the hood: Model access is yours: pick a provider, paste your key, switch anytime. Supported out of the box: -**OpenAI · Anthropic · Google Gemini · Inkling (Thinking Machines) · GLM (Z.ai) · DeepSeek · Kimi (Moonshot) · Qwen · MiniMax · Mistral · Grok (xAI)** - plus open-weight models via **Together** and **Fireworks**, and fully local models via **Ollama**. +**OpenAI · Anthropic · Google Gemini · Inkling (Thinking Machines) · GLM (Z.ai) · DeepSeek · Kimi (Moonshot) · Qwen · MiniMax · Mistral · Grok (xAI)** - plus open-weight models via **Together** and **Fireworks**, and fully local models via **Ollama** and **LM Studio**. A curated model list marks what we've verified for tool-calling work. Adding any model string works at your own risk. diff --git a/coworker/providers/capabilities.py b/coworker/providers/capabilities.py index f096ff3a..e8b6c1b5 100644 --- a/coworker/providers/capabilities.py +++ b/coworker/providers/capabilities.py @@ -22,9 +22,10 @@ def capabilities_for(model: str) -> ModelCapabilities: provider = model.split(":", 1)[0].lower() if ":" in model else "" name = model.split(":", 1)[-1].lower() # strip a provider prefix if present - # Ollama (local) models vary widely and many fake/mishandle parallel tool calls — assume - # tools work (we only point at tool-capable models) but stay conservative otherwise. - if provider == "ollama": + # Local servers (Ollama, LM Studio) host models that vary widely and many fake/mishandle + # parallel tool calls — assume tools work (we only point at tool-capable models) but stay + # conservative otherwise. + if provider in ("ollama", "lmstudio"): return ModelCapabilities( tools=True, vision=False, parallel_tool_calls=False, streaming=True ) diff --git a/coworker/providers/registry.py b/coworker/providers/registry.py index 6c147705..aa44c51f 100644 --- a/coworker/providers/registry.py +++ b/coworker/providers/registry.py @@ -12,7 +12,7 @@ `AnthropicProvider`), `gemini` (native Google GenAI API via `GeminiProvider`), `bedrock` (models in the user's own AWS account — Claude natively, everything else via Converse), `vertex` (the user's own GCP project — Gemini and Claude natively, open-weight via the -MaaS endpoint), and `ollama` (local, OpenAI-compatible `/v1`). +MaaS endpoint), and the local servers `ollama` and `lmstudio` (both OpenAI-compatible `/v1`). """ from __future__ import annotations @@ -30,6 +30,12 @@ from .vertex_provider import VertexProvider DEFAULT_OLLAMA_URL = "http://localhost:11434" +DEFAULT_LMSTUDIO_URL = "http://localhost:1234" + +# Local, keyless providers reached through their OpenAI-compatible `/v1` — grouped where +# they need the same special-casing (the no-auth verify probe and its error copy). +LOCAL_PROVIDERS = ("ollama", "lmstudio") +_LOCAL_DEFAULT_URLS = {"ollama": DEFAULT_OLLAMA_URL, "lmstudio": DEFAULT_LMSTUDIO_URL} @dataclass(frozen=True) @@ -98,15 +104,15 @@ def to_dict(self) -> dict[str, Any]: } -def _normalize_ollama_url(url: Optional[str]) -> str: - """Accept `http://host:11434` or `.../v1` and return an OpenAI-compatible base URL. +def _normalize_local_url(url: Optional[str], default: str) -> str: + """Accept `http://host:port` or `.../v1` and return an OpenAI-compatible base URL. - Ollama serves its OpenAI-compatible API under `/v1`; the native API lives at the root, so we - always target `/v1`. + Ollama and LM Studio both serve their OpenAI-compatible API under `/v1` (their native + APIs live elsewhere on the same server), so we always target `/v1`. """ - base = (url or DEFAULT_OLLAMA_URL).strip().rstrip("/") + base = (url or default).strip().rstrip("/") if not base: - base = DEFAULT_OLLAMA_URL + base = default if not base.endswith("/v1"): base = base + "/v1" return base @@ -184,10 +190,17 @@ def get(key: str) -> Optional[str]: def _build_ollama(profile: dict[str, Any], secrets: Any) -> ProviderClient: # Ollama's OpenAI-compatible endpoint ignores the key but the SDK requires a non-empty # string, so we pass a placeholder. `base_url` comes from the stored profile (or the default). - base_url = _normalize_ollama_url((profile or {}).get("base_url")) + base_url = _normalize_local_url((profile or {}).get("base_url"), DEFAULT_OLLAMA_URL) return OpenAIProvider(api_key="ollama", base_url=base_url) +def _build_lmstudio(profile: dict[str, Any], secrets: Any) -> ProviderClient: + # Same placeholder-key contract as Ollama: LM Studio's local server doesn't require a + # key out of the box, but the SDK insists on a non-empty string. + base_url = _normalize_local_url((profile or {}).get("base_url"), DEFAULT_LMSTUDIO_URL) + return OpenAIProvider(api_key="lm-studio", base_url=base_url) + + def _openai_compat(vendor: str, default_base_url: str, env_key: Optional[str] = None): """Builder factory for vendors reached through their OpenAI-compatible API (Z AI, DeepSeek, Kimi, MiniMax, Qwen, xAI, Mistral). The key is resolved from the vendor's OWN profile (or its @@ -569,6 +582,26 @@ def _compat( # `ollama pull qwen3-coder:30b`. recommended_model="qwen3-coder:30b", ), + ProviderDescriptor( + name="lmstudio", + title="LM Studio (local models)", + needs_key=False, + fields=[ + ProviderField( + "base_url", + "LM Studio server URL", + secret=False, + required=False, + placeholder=DEFAULT_LMSTUDIO_URL, + help="Where LM Studio's local server is listening (the Developer tab, or " + "`lms server start`). The OpenAI-compatible /v1 path is added automatically.", + ), + ], + build=_build_lmstudio, + # Ollama's verified tool-calling pick, under LM Studio's catalog id — download it + # in-app or with `lms get qwen/qwen3-coder-30b`. + recommended_model="qwen/qwen3-coder-30b", + ), ] _BY_NAME = {d.name: d for d in DESCRIPTORS} @@ -829,8 +862,8 @@ def verify_provider_key( params={"key": key}, timeout=timeout, ) - elif name == "ollama": - base = _normalize_ollama_url(base_url) + elif name in LOCAL_PROVIDERS: # ollama / lmstudio: keyless local /v1 + base = _normalize_local_url(base_url, _LOCAL_DEFAULT_URLS[name]) resp = httpx.get(base.rstrip("/") + "/models", timeout=timeout) else: # openai + any OpenAI-compatible endpoint (Azure, OpenRouter, vendors, vLLM…) default_base = next( @@ -855,10 +888,10 @@ def verify_provider_key( if resp.status_code < 300: return {"ok": True} if resp.status_code in (401, 403): - if name == "ollama": + if name in LOCAL_PROVIDERS: return {"ok": False, "error": "Server rejected the request."} return {"ok": False, "error": "Invalid API key."} - if resp.status_code == 404 and name == "ollama": + if resp.status_code == 404 and name in LOCAL_PROVIDERS: return { "ok": False, "error": "Reached the server, but no OpenAI-compatible /v1 API there.", diff --git a/coworker/server/manager.py b/coworker/server/manager.py index ad76e996..50b0f2ae 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -80,6 +80,7 @@ provider_descriptors, verify_provider_key, ) +from ..providers.registry import DEFAULT_LMSTUDIO_URL, DEFAULT_OLLAMA_URL from ..secrets import SecretStore, state_dir from ..sessions import SessionRecord from ..skills import ( @@ -1559,10 +1560,11 @@ def _note_provider_use(self, name: str) -> None: def _suggested_models(self, name: str) -> list[str]: """Bare model-name suggestions for the 'add model' form (datalist), per provider. - Ollama → live `/api/tags` (best-effort); everyone else → the curated matrix, - topped up with the compat-vendor extras the matrix doesn't vouch for.""" - if name == "ollama": - return [m.split(":", 1)[-1] for m in self._ollama_models()] + Local servers (Ollama, LM Studio) → their live model list (best-effort); everyone + else → the curated matrix, topped up with the compat-vendor extras the matrix + doesn't vouch for.""" + if name in self.LOCAL_MODEL_SERVERS: + return [m.split(":", 1)[-1] for m in self._local_models(name)] from ..providers.matrix import models_for_provider return list( @@ -1709,48 +1711,69 @@ def set_dm_session(self, session_id: Optional[str]) -> dict[str, Any]: self._save_prefs() return {"ok": True, "dm_session": self.dm_session()} - def _ollama_alive(self) -> bool: - """Best-effort local-Ollama liveness, cached 30s (get_settings runs on every GUI - fetch — no 2s probe inline). Keyless is not the same as PRESENT: `ollama:*` picker - entries render only when an Ollama actually answers, so a machine with no Ollama - never shows phantom local models (e.g. a stray pasted string saved as a model id, - caught 2026-07-21).""" + # Local model servers (Ollama, LM Studio): keyless, probed instead of key-checked. + # Ollama is listed via its native `/api/tags`; LM Studio via its OpenAI-compatible + # `/v1/models` (its native REST API is beta and can sit behind an auth token). + LOCAL_MODEL_SERVERS: dict[str, dict[str, str]] = { + "ollama": {"default_url": DEFAULT_OLLAMA_URL, "models_path": "/api/tags"}, + "lmstudio": {"default_url": DEFAULT_LMSTUDIO_URL, "models_path": "/v1/models"}, + } + + def _local_server_root(self, name: str) -> str: + """A local server's root URL (stored base_url or the default), with any `/v1` + suffix stripped so native and OpenAI-compat paths can both be appended.""" + profile = self.secrets.get(f"provider:{name}") or {} + base = ( + (profile.get("base_url") or self.LOCAL_MODEL_SERVERS[name]["default_url"]) + .strip() + .rstrip("/") + ) + if base.endswith("/v1"): + base = base[: -len("/v1")] + return base + + def _local_models_url(self, name: str) -> str: + return self._local_server_root(name) + self.LOCAL_MODEL_SERVERS[name]["models_path"] + + def _local_alive(self, name: str) -> bool: + """Best-effort local-server liveness, cached 30s per provider (get_settings runs on + every GUI fetch — no 2s probe inline). Keyless is not the same as PRESENT: + `ollama:*` / `lmstudio:*` picker entries render only when the server actually + answers, so a machine without one never shows phantom local models (e.g. a stray + pasted string saved as a model id, caught 2026-07-21).""" import time now = time.monotonic() - cached = getattr(self, "_ollama_alive_cache", None) + cache = getattr(self, "_local_alive_cache", {}) + cached = cache.get(name) if cached and now - cached[0] < 30: return cached[1] - profile = self.secrets.get("provider:ollama") or {} - base = (profile.get("base_url") or "http://localhost:11434").strip().rstrip("/") - if base.endswith("/v1"): - base = base[: -len("/v1")] try: import httpx - alive = httpx.get(base + "/api/tags", timeout=0.8).status_code == 200 + alive = httpx.get(self._local_models_url(name), timeout=0.8).status_code == 200 except Exception: alive = False - self._ollama_alive_cache = (now, alive) + cache[name] = (now, alive) + self._local_alive_cache = cache return alive - def _ollama_models(self) -> list[str]: - """Live list of models pulled into the configured Ollama server (via its native - `/api/tags`), as `ollama:` so they're directly selectable. Empty if Ollama isn't - configured or unreachable — best-effort, never raises.""" - profile = self.secrets.get("provider:ollama") - if not profile: + def _local_models(self, name: str) -> list[str]: + """Live list of the models available on a configured local server, as + `:` so they're directly selectable. Empty if the provider isn't + configured or unreachable — best-effort, never raises. Both servers list every + *downloaded* model; each loads one on demand when it's first requested.""" + if not self.secrets.get(f"provider:{name}"): return [] - base = (profile.get("base_url") or "http://localhost:11434").strip().rstrip("/") - if base.endswith("/v1"): - base = base[: -len("/v1")] try: import httpx - data = httpx.get(base + "/api/tags", timeout=2.0).json() - return [ - f"ollama:{m['name']}" for m in data.get("models", []) if m.get("name") - ] + data = httpx.get(self._local_models_url(name), timeout=2.0).json() + # Ollama's /api/tags: {"models": [{"name": …}]}; LM Studio's /v1/models + # (OpenAI list shape): {"data": [{"id": …}]}. + entries = data.get("models") or data.get("data") or [] + ids = (m.get("name") or m.get("id") for m in entries) + return [f"{name}:{mid}" for mid in ids if mid] except Exception: return [] @@ -1816,12 +1839,12 @@ def get_settings(self) -> dict[str, Any]: # Only surface models whose provider is actually configured — the composer picker # reflects exactly what's connected. The active default is always kept selectable # (it's hidden behind the "No model" state until a provider is connected anyway). - # Ollama is keyless, so "configured" is meaningless there — its models show only - # while a local Ollama answers (cached liveness probe). + # Local servers (Ollama, LM Studio) are keyless, so "configured" is meaningless — + # their models show only while the server answers (cached liveness probe). def _selectable(m: str) -> bool: provider = self._model_provider(m) - if provider == "ollama": - return self._ollama_alive() + if provider in self.LOCAL_MODEL_SERVERS: + return self._local_alive(provider) return self._provider_configured(provider) selectable = [m for m in self._curated_models() if _selectable(m)] diff --git a/surfaces/gui/src/providers/ProviderSetup.tsx b/surfaces/gui/src/providers/ProviderSetup.tsx index 1d8dc881..b99ab493 100644 --- a/surfaces/gui/src/providers/ProviderSetup.tsx +++ b/surfaces/gui/src/providers/ProviderSetup.tsx @@ -34,6 +34,13 @@ export const KEY_HELP: Record = { xai: { url: "https://console.x.ai", label: "console.x.ai" }, }; +// Keyless local servers — the KEY_HELP counterpart: "install the app" instead of +// "create a key". +export const LOCAL_HELP: Record = { + ollama: { url: "https://ollama.com/download", name: "Ollama" }, + lmstudio: { url: "https://lmstudio.ai/download", name: "LM Studio" }, +}; + export type Verify = { state: "idle" | "testing" | "ok" | "error"; msg?: string }; /** Brand chip: always a light plate so multicolor marks read on any theme. */ @@ -102,8 +109,8 @@ export function useProviderSetup(opts?: { onSaved?: () => void }): ProviderSetup const [dirty, setDirty] = useState(false); const [showEndpoint, setShowEndpoint] = useState(false); const [verify, setVerify] = useState({ state: "idle" }); - // Keyless providers (Ollama) report configured without proving anything runs — - // a passing Detect this session is what marks them live. + // Keyless providers (Ollama, LM Studio) report configured without proving anything + // runs — a passing Detect this session is what marks them live. const [keylessOk, setKeylessOk] = useState>(new Set()); // Unsaved per-provider input survives switching cards (owner complaint 2026-07-16). const [drafts, setDrafts] = useState>>({}); @@ -337,7 +344,7 @@ export function ProviderForm({ ) : []; // Without a choice control, Test lives next to the required secret (the API key), or - // the first field for keyless providers (Ollama's Detect). + // the first field for keyless providers (Ollama's / LM Studio's Detect). const requiredSecret = fieldsAll.find((x) => x.secret && x.required); const testKey = requiredSecret ? requiredSecret.key : fieldsAll[0]?.key; if (!sel) return null; @@ -497,14 +504,14 @@ export function ProviderForm({ — takes about a minute.

)} - {info && !info.needs_key && ( + {info && !info.needs_key && LOCAL_HELP[sel] && (

- No API key needed — Ollama runs models on this computer.{" "} + No API key needed — {LOCAL_HELP[sel].name} runs models on this computer.{" "}

)} diff --git a/surfaces/gui/src/providers/logos.ts b/surfaces/gui/src/providers/logos.ts index 093c4fdd..6b06d478 100644 --- a/surfaces/gui/src/providers/logos.ts +++ b/surfaces/gui/src/providers/logos.ts @@ -9,6 +9,7 @@ import anthropic from "./logos/anthropic.svg"; import openai from "./logos/openai.svg"; import gemini from "./logos/gemini.svg"; import ollama from "./logos/ollama.svg"; +import lmstudio from "./logos/lmstudio.svg"; import bedrock from "./logos/bedrock.svg"; import vertex from "./logos/vertex.svg"; import openrouter from "./logos/openrouter.svg"; @@ -29,6 +30,7 @@ export const PROVIDER_LOGOS: Record = { gemini, meta, ollama, + lmstudio, bedrock, vertex, openrouter, @@ -49,6 +51,7 @@ export const PROVIDER_ORDER = [ "gemini", "meta", "ollama", + "lmstudio", "bedrock", "vertex", "openrouter", diff --git a/surfaces/gui/src/providers/logos/lmstudio.svg b/surfaces/gui/src/providers/logos/lmstudio.svg new file mode 100644 index 00000000..ea0816b6 --- /dev/null +++ b/surfaces/gui/src/providers/logos/lmstudio.svg @@ -0,0 +1 @@ +LM Studio \ No newline at end of file diff --git a/tests/test_provider_router.py b/tests/test_provider_router.py index d5c124a9..7954a6f4 100644 --- a/tests/test_provider_router.py +++ b/tests/test_provider_router.py @@ -14,7 +14,7 @@ StreamChunk, capabilities_for, ) -from coworker.providers.registry import _normalize_ollama_url, build_provider_client +from coworker.providers.registry import _normalize_local_url, build_provider_client from coworker.providers.openai_provider import _salvage_tool_calls_from_text @@ -45,14 +45,15 @@ def __init__(self, **kwargs): assert "base_url" not in captured -# -- ollama URL normalization --------------------------------------------------- -def test_normalize_ollama_url(): - assert _normalize_ollama_url(None) == "http://localhost:11434/v1" +# -- local-server URL normalization (Ollama, LM Studio) --------------------------- +def test_normalize_local_url(): + assert _normalize_local_url(None, "http://localhost:11434") == "http://localhost:11434/v1" assert ( - _normalize_ollama_url("http://localhost:11434") == "http://localhost:11434/v1" + _normalize_local_url("http://localhost:11434", "http://localhost:11434") + == "http://localhost:11434/v1" ) - assert _normalize_ollama_url("http://h:1/v1/") == "http://h:1/v1" - assert _normalize_ollama_url(" ") == "http://localhost:11434/v1" + assert _normalize_local_url("http://h:1/v1/", "http://localhost:11434") == "http://h:1/v1" + assert _normalize_local_url(" ", "http://localhost:1234") == "http://localhost:1234/v1" def test_build_ollama_client_uses_base_url(monkeypatch): @@ -71,6 +72,22 @@ def __init__(self, **kwargs): assert captured["api_key"] == "ollama" # placeholder, Ollama ignores it +def test_build_lmstudio_client_uses_base_url(monkeypatch): + captured: dict = {} + + class FakeOpenAI: + def __init__(self, **kwargs): + captured.update(kwargs) + + monkeypatch.setattr("openai.OpenAI", FakeOpenAI) + client = build_provider_client( + "lmstudio", {"base_url": "http://box:1234"}, secrets=None + ) + client._ensure_client() # type: ignore[attr-defined] + assert captured["base_url"] == "http://box:1234/v1" + assert captured["api_key"] == "lm-studio" # placeholder, LM Studio ignores it + + # -- router routing ------------------------------------------------------------- class _Recorder(ProviderClient): def __init__(self, name: str): @@ -136,6 +153,9 @@ def test_router_bare_only_strips_known_provider(): assert ( r._bare("ollama:qwen2.5-coder:32b") == "qwen2.5-coder:32b" ) # strip provider, keep tag + assert ( + r._bare("lmstudio:qwen/qwen3-coder-30b") == "qwen/qwen3-coder-30b" + ) # LM Studio ids carry slashes, never colons — the prefix split is unaffected assert r._bare("gpt-5.5") == "gpt-5.5" # a colon that isn't a provider (version tag) must NOT be split — else OpenAI gets "32b" assert r._bare("qwen2.5-coder:32b") == "qwen2.5-coder:32b" @@ -156,6 +176,13 @@ def test_capabilities_ollama(): assert caps.vision is False +def test_capabilities_lmstudio(): + caps = capabilities_for("lmstudio:qwen/qwen3-coder-30b") + assert caps.tools is True + assert caps.parallel_tool_calls is False + assert caps.vision is False + + # -- tool-call salvage (Ollama emits tool calls as text) ------------------------ def test_salvage_bare_json_object(): calls = _salvage_tool_calls_from_text( @@ -326,7 +353,7 @@ def test_manager_curated_models(tmp_path, monkeypatch): # covers picker mechanics only (the probe itself is covered by # test_settings.py::test_ollama_models_gated_on_liveness). Unpinned, the ollama # assertions below pass only where Ollama happens to run — green on a dev box, red in CI. - monkeypatch.setattr(SessionManager, "_ollama_alive", lambda self: True) + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: True) mgr = SessionManager(data_dir=tmp_path) # no provider keys → nothing but the always-selectable default diff --git a/tests/test_provider_verify.py b/tests/test_provider_verify.py index 8da7fe76..32d887d1 100644 --- a/tests/test_provider_verify.py +++ b/tests/test_provider_verify.py @@ -91,6 +91,14 @@ def test_verify_ollama_uses_v1_models_no_key(monkeypatch): assert "headers" not in cap # keyless +def test_verify_lmstudio_uses_v1_models_no_key(monkeypatch): + cap: dict = {} + _patch_get(monkeypatch, status=200, capture=cap) + verify_provider_key("lmstudio") # no base_url → LM Studio's default port + assert cap["url"] == "http://localhost:1234/v1/models" + assert "headers" not in cap # keyless + + def test_verify_network_error_is_clean(monkeypatch): _patch_get(monkeypatch, raise_exc=ConnectionError("boom")) res = verify_provider_key("openai", api_key="sk-x") diff --git a/tests/test_settings.py b/tests/test_settings.py index 873940e2..b10c9287 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -169,8 +169,61 @@ def test_ollama_models_gated_on_liveness(tmp_path, monkeypatch): manager = SessionManager(data_dir=tmp_path / "data") manager.add_model("ollama:llama3.3") - monkeypatch.setattr(SessionManager, "_ollama_alive", lambda self: False) + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: False) assert "ollama:llama3.3" not in manager.get_settings()["models"] - monkeypatch.setattr(SessionManager, "_ollama_alive", lambda self: True) + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: True) assert "ollama:llama3.3" in manager.get_settings()["models"] + + +def test_lmstudio_models_gated_on_liveness(tmp_path, monkeypatch): + """Same gate for `lmstudio:*` — and per-provider: only the answering server's models + render (one local server being up must not surface the other's entries).""" + from coworker.server.manager import SessionManager + + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + manager.add_model("lmstudio:qwen/qwen3-coder-30b") + manager.add_model("ollama:llama3.3") + + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: False) + assert "lmstudio:qwen/qwen3-coder-30b" not in manager.get_settings()["models"] + + monkeypatch.setattr( + SessionManager, "_local_alive", lambda self, name: name == "lmstudio" + ) + models = manager.get_settings()["models"] + assert "lmstudio:qwen/qwen3-coder-30b" in models + assert "ollama:llama3.3" not in models # the other server is still down + + +def test_local_models_parses_both_server_shapes(tmp_path, monkeypatch): + """Ollama's native /api/tags and LM Studio's OpenAI-shaped /v1/models both map to + `:` picker entries.""" + from coworker.server.manager import SessionManager + + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + manager.set_provider("ollama", {"base_url": "http://localhost:11434"}) + manager.set_provider("lmstudio", {"base_url": "http://localhost:1234"}) + + payloads = { + "http://localhost:11434/api/tags": {"models": [{"name": "llama3.3"}]}, + "http://localhost:1234/v1/models": { + "data": [{"id": "qwen/qwen3-coder-30b", "object": "model"}] + }, + } + + class _Resp: + def __init__(self, data): + self._data = data + + def json(self): + return self._data + + import httpx + + monkeypatch.setattr(httpx, "get", lambda url, timeout=None: _Resp(payloads[url])) + assert manager._local_models("ollama") == ["ollama:llama3.3"] + assert manager._local_models("lmstudio") == ["lmstudio:qwen/qwen3-coder-30b"] From 5da1a69135af6ab9d08073fddb3bae1abc88119a Mon Sep 17 00:00:00 2001 From: zalex Date: Mon, 3 Aug 2026 13:11:11 -0700 Subject: [PATCH 2/3] Address codex review: local-provider availability, probe rigor, verify semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from the gpt-5.6-sol review of the LM Studio provider addition (all pre-existing Ollama behaviors this change inherited or generalized): - keyless Detect now persists on every pass (GUI): keyless providers report configured out of the box, so the dirty/configured gate skipped the first-time save — no stored profile, and set_provider's recommended-model auto-add never ran. _local_models pairs with this by treating a stored EMPTY profile as engaged (is None check). - _provider_available: local servers must actually answer, everyone else must be configured — now drives picker gating, model_ready, and the first-working-provider default handoff (a dead local server no longer reads as ready, and no longer blocks a working provider from taking the default). - probes require the provider's list shape, not just a 200: /v1/models must serve {"data": [...]}, /api/tags {"models": [...]} — some other service answering on the port no longer passes verify or liveness. - verify_provider: non-secret fields sent explicitly blank mean 'back to the default' instead of resurrecting the stored value (a passing Test used to validate a URL the subsequent save then removed); blank secrets still fall back to the stored key. - _refresh_provider drops the per-provider liveness cache, so repointing or removing a local server re-probes immediately. - coverage: probe shape/URLs, cache invalidation, model_ready gating, first-Detect end-to-end (empty profile -> recommended auto-add -> default handoff), explicit-blank verify, the GUI hook's keyless Detect save, and an lmstudio entry in the e2e provider fixture. Co-Authored-By: Claude Fable 5 --- coworker/providers/registry.py | 13 ++ coworker/server/manager.py | 86 ++++++++--- surfaces/gui/e2e/fixtures.ts | 2 + .../gui/src/providers/ProviderSetup.test.tsx | 50 +++++- surfaces/gui/src/providers/ProviderSetup.tsx | 6 +- tests/test_provider_verify.py | 34 ++++- tests/test_settings.py | 142 +++++++++++++++++- 7 files changed, 292 insertions(+), 41 deletions(-) diff --git a/coworker/providers/registry.py b/coworker/providers/registry.py index aa44c51f..e884624d 100644 --- a/coworker/providers/registry.py +++ b/coworker/providers/registry.py @@ -886,6 +886,19 @@ def verify_provider_key( } if resp.status_code < 300: + # Local servers: a 2xx alone can't tell a model server from whatever else answers + # on that port (a dev server's HTML page, a proxy) — require the OpenAI list shape + # both serve at /v1/models. An empty model list still passes. + if name in LOCAL_PROVIDERS: + try: + shaped = isinstance(resp.json().get("data"), list) + except Exception: + shaped = False + if not shaped: + return { + "ok": False, + "error": "Reached the server, but no OpenAI-compatible /v1 API there.", + } return {"ok": True} if resp.status_code in (401, 403): if name in LOCAL_PROVIDERS: diff --git a/coworker/server/manager.py b/coworker/server/manager.py index 50b0f2ae..cba5d147 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -1613,9 +1613,10 @@ def set_provider( added = rec if name == "openai" else f"{name}:{rec}" self.add_model(added) # First working provider wins the default: if the current default model belongs to a - # provider with no usable config (the fresh-install gpt-5.6-sol case), switch the default to - # this provider's model. A default that already works is never stolen. - if added and not self._provider_configured(self._model_provider(self.model)): + # provider with no usable config (the fresh-install gpt-5.6-sol case) or a local server + # that isn't answering, switch the default to this provider's model. A default that + # already works is never stolen. + if added and not self._provider_available(self._model_provider(self.model)): self.set_default_model(added) return {"ok": True, "provider": name, "recommended_model": rec} @@ -1634,8 +1635,10 @@ def verify_provider( self, name: str, fields: Optional[dict[str, Any]] ) -> dict[str, Any]: """Test a provider's credentials with a live read-only call, WITHOUT persisting them, so - onboarding can offer a "Test" button. Falls back to stored/env values when the form left - a field blank (e.g. testing an already-configured provider).""" + onboarding can offer a "Test" button. Blank secrets fall back to stored/env values (the + form masks a saved key); non-secret fields fall back only when OMITTED — one sent + explicitly blank means "back to the default", and resurrecting the stored value would + verify a URL that a subsequent save then removes.""" import os d = get_descriptor(name) @@ -1645,7 +1648,10 @@ def verify_provider( profile = self.secrets.get(f"provider:{name}") or {} merged = {} for f in d.fields: - val = fields.get(f.key) or profile.get(f.key) or "" + if f.secret or f.key not in fields: + val = fields.get(f.key) or profile.get(f.key) or "" + else: + val = fields.get(f.key) or "" if isinstance(val, str): val = val.strip() if val: @@ -1680,6 +1686,14 @@ def _provider_configured(self, name: str) -> bool: return False return descriptor_configured(d, self.secrets.get(f"provider:{name}") or {}) + def _provider_available(self, name: str) -> bool: + """Whether a provider can serve RIGHT NOW. Local servers must actually answer + (their keyless "configured" proves nothing runs); everyone else must be + configured. Drives picker gating, `model_ready`, and default-model handoff.""" + if name in self.LOCAL_MODEL_SERVERS: + return self._local_alive(name) + return self._provider_configured(name) + # -- settings / prefs (model API key, default model, onboarding) ------------- def _prefs_path(self) -> Path: return self._data_base / "prefs.json" @@ -1712,11 +1726,22 @@ def set_dm_session(self, session_id: Optional[str]) -> dict[str, Any]: return {"ok": True, "dm_session": self.dm_session()} # Local model servers (Ollama, LM Studio): keyless, probed instead of key-checked. - # Ollama is listed via its native `/api/tags`; LM Studio via its OpenAI-compatible - # `/v1/models` (its native REST API is beta and can sit behind an auth token). + # Ollama is listed via its native `/api/tags` ({"models": [{"name": …}]}); LM Studio + # via its OpenAI-compatible `/v1/models` ({"data": [{"id": …}]}) — its native REST + # API is beta and can sit behind an auth token. LOCAL_MODEL_SERVERS: dict[str, dict[str, str]] = { - "ollama": {"default_url": DEFAULT_OLLAMA_URL, "models_path": "/api/tags"}, - "lmstudio": {"default_url": DEFAULT_LMSTUDIO_URL, "models_path": "/v1/models"}, + "ollama": { + "default_url": DEFAULT_OLLAMA_URL, + "models_path": "/api/tags", + "list_key": "models", + "id_key": "name", + }, + "lmstudio": { + "default_url": DEFAULT_LMSTUDIO_URL, + "models_path": "/v1/models", + "list_key": "data", + "id_key": "id", + }, } def _local_server_root(self, name: str) -> str: @@ -1751,7 +1776,12 @@ def _local_alive(self, name: str) -> bool: try: import httpx - alive = httpx.get(self._local_models_url(name), timeout=0.8).status_code == 200 + resp = httpx.get(self._local_models_url(name), timeout=0.8) + # Status alone can't tell a model server from whatever else answers on that + # port — require the expected list container (an empty list still counts). + alive = resp.status_code == 200 and isinstance( + resp.json().get(self.LOCAL_MODEL_SERVERS[name]["list_key"]), list + ) except Exception: alive = False cache[name] = (now, alive) @@ -1763,16 +1793,18 @@ def _local_models(self, name: str) -> list[str]: `:` so they're directly selectable. Empty if the provider isn't configured or unreachable — best-effort, never raises. Both servers list every *downloaded* model; each loads one on demand when it's first requested.""" - if not self.secrets.get(f"provider:{name}"): + # `is None` (never engaged), not falsy: a keyless Detect stores an EMPTY profile, + # which must still probe the default endpoint — otherwise the recommended-model + # auto-add and the add-model suggestions never see a default-URL server. + if self.secrets.get(f"provider:{name}") is None: return [] + spec = self.LOCAL_MODEL_SERVERS[name] try: import httpx data = httpx.get(self._local_models_url(name), timeout=2.0).json() - # Ollama's /api/tags: {"models": [{"name": …}]}; LM Studio's /v1/models - # (OpenAI list shape): {"data": [{"id": …}]}. - entries = data.get("models") or data.get("data") or [] - ids = (m.get("name") or m.get("id") for m in entries) + entries = data.get(spec["list_key"]) or [] + ids = (m.get(spec["id_key"]) for m in entries) return [f"{name}:{mid}" for mid in ids if mid] except Exception: return [] @@ -1842,10 +1874,7 @@ def get_settings(self) -> dict[str, Any]: # Local servers (Ollama, LM Studio) are keyless, so "configured" is meaningless — # their models show only while the server answers (cached liveness probe). def _selectable(m: str) -> bool: - provider = self._model_provider(m) - if provider in self.LOCAL_MODEL_SERVERS: - return self._local_alive(provider) - return self._provider_configured(provider) + return self._provider_available(self._model_provider(m)) selectable = [m for m in self._curated_models() if _selectable(m)] if self.model not in selectable: @@ -1863,10 +1892,11 @@ def _selectable(m: str) -> bool: # drives the composer's context-fill meter (absent id → meter hides). "model_context_windows": model_context_windows(), "has_key": env_key or stored, - # Provider-agnostic "can this default model actually run?" — true when the default - # model's provider is configured (any provider, not just OpenAI). Drives the GUI's - # "No model connected" composer chip and the onboarding Skip warning. - "model_ready": self._provider_configured(self._model_provider(self.model)), + # Provider-agnostic "can this default model actually run?" — the provider is + # configured, or for a local server, actually answering (a dead Ollama/LM Studio + # must not read as ready). Drives the GUI's "No model connected" composer chip + # and the onboarding Skip warning. + "model_ready": self._provider_available(self._model_provider(self.model)), "source": "env" if env_key else ("store" if stored else None), "onboarded": bool(self._prefs.get("onboarded")), "experimental_connectors": experimental_enabled(self.secrets), @@ -3778,6 +3808,14 @@ def _refresh_provider(self, name: Optional[str] = None) -> None: invalidate = getattr(self.provider, "invalidate", None) if callable(invalidate): invalidate(name) + # A repointed or removed local server must re-probe immediately — otherwise the + # picker serves up-to-30s-stale liveness for the OLD endpoint. + cache = getattr(self, "_local_alive_cache", None) + if cache: + if name is None: + cache.clear() + else: + cache.pop(name, None) # -- read models ------------------------------------------------------------ def list_sessions(self, workspace: Optional[str] = None) -> list[dict[str, Any]]: diff --git a/surfaces/gui/e2e/fixtures.ts b/surfaces/gui/e2e/fixtures.ts index e6dfa409..dba26f4f 100644 --- a/surfaces/gui/e2e/fixtures.ts +++ b/surfaces/gui/e2e/fixtures.ts @@ -339,6 +339,8 @@ const PROVIDERS = [ // ollama: keyless local provider — "configured" without proving anything runs; the // onboarding gallery shows "No key needed" and its form is endpoint + Detect (§39). { name: "ollama", title: "Ollama (local models)", needs_key: false, fields: [{ key: "base_url", label: "Endpoint", secret: false, required: false, help: "", placeholder: "http://127.0.0.1:11434", default: "http://127.0.0.1:11434" }], configured: true, values: {}, suggested_models: ["qwen3-coder:30b"], key_set_at: null, last_used_at: null }, + // lmstudio: the second keyless local provider — same Detect flow, its own install copy. + { name: "lmstudio", title: "LM Studio (local models)", needs_key: false, fields: [{ key: "base_url", label: "LM Studio server URL", secret: false, required: false, help: "", placeholder: "http://localhost:1234", default: "" }], configured: true, values: {}, suggested_models: ["qwen/qwen3-coder-30b"], key_set_at: null, last_used_at: null }, ]; /** Install the API + WebSocket mocks on a page. Returns handles for assertions/seed data. */ diff --git a/surfaces/gui/src/providers/ProviderSetup.test.tsx b/surfaces/gui/src/providers/ProviderSetup.test.tsx index 870aa7a0..0374f235 100644 --- a/surfaces/gui/src/providers/ProviderSetup.test.tsx +++ b/surfaces/gui/src/providers/ProviderSetup.test.tsx @@ -1,11 +1,19 @@ // Auth-method segmented choice + show_when field visibility (Bedrock's "Connect with"): // only the selected method's fields render, and clicking a segment switches them. +// Plus the hook's keyless Detect contract (persist even when the form is clean). import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, fireEvent, render, screen } from "@testing-library/react"; -import { ProviderForm, type ProviderSetupState } from "./ProviderSetup"; +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { ProviderForm, useProviderSetup, type ProviderSetupState } from "./ProviderSetup"; +import * as api from "../api"; import type { ProviderInfo } from "../api"; vi.mock("../tauri", () => ({ openExternal: vi.fn() })); +vi.mock("../api", () => ({ + getProviders: vi.fn(), + setProvider: vi.fn(), + verifyProvider: vi.fn(), + removeProvider: vi.fn(), +})); afterEach(cleanup); @@ -94,3 +102,41 @@ describe("ProviderForm auth-method choice", () => { expect(screen.queryByTestId("t-field-bedrock_api_key")).toBeNull(); }); }); + +const LMSTUDIO: ProviderInfo = { + name: "lmstudio", + title: "LM Studio (local models)", + needs_key: false, + configured: true, // keyless providers always report configured — even before first use + values: {}, + suggested_models: [], + recommended_model: null, + fields: [ + { key: "base_url", label: "LM Studio server URL", secret: false, required: false, help: "", placeholder: "http://localhost:1234" }, + ], +}; + +function HookHarness({ grab }: { grab: (ps: ProviderSetupState) => void }) { + grab(useProviderSetup()); + return null; +} + +describe("useProviderSetup keyless Detect", () => { + it("persists a clean keyless form on a passing Detect", async () => { + // Keyless providers report `configured` out of the box, so a dirty/configured gate + // would skip the FIRST-time save — no stored profile, and the backend's + // recommended-model auto-add would never run (codex review, 2026-08-03). + vi.mocked(api.getProviders).mockResolvedValue([LMSTUDIO]); + vi.mocked(api.verifyProvider).mockResolvedValue({ ok: true }); + vi.mocked(api.setProvider).mockResolvedValue({ ok: true }); + + let ps!: ProviderSetupState; + render( (ps = v)} />); + await waitFor(() => expect(ps.providers.length).toBe(1)); + act(() => ps.openProvider("lmstudio")); + await act(async () => { + expect(await ps.runTestAndSave()).toBe(true); + }); + expect(api.setProvider).toHaveBeenCalledWith("lmstudio", { base_url: "" }); + }); +}); diff --git a/surfaces/gui/src/providers/ProviderSetup.tsx b/surfaces/gui/src/providers/ProviderSetup.tsx index b99ab493..ad49b7cc 100644 --- a/surfaces/gui/src/providers/ProviderSetup.tsx +++ b/surfaces/gui/src/providers/ProviderSetup.tsx @@ -166,7 +166,11 @@ export function useProviderSetup(opts?: { onSaved?: () => void }): ProviderSetup setVerify({ state: "error", msg: res.error || "couldn't verify" }); return false; } - if (dirty || !info?.configured) await setProvider(sel, fields).catch(() => {}); + // Keyless providers save on every passing Detect: they report `configured` out of + // the box, so the dirty/configured gate would skip the FIRST-time save — leaving no + // stored profile, which means set_provider's recommended-model auto-add never runs. + if (dirty || !info?.configured || !info?.needs_key) + await setProvider(sel, fields).catch(() => {}); if (!info?.needs_key) setKeylessOk((s) => new Set(s).add(sel)); setVerify({ state: "ok" }); setDirty(false); diff --git a/tests/test_provider_verify.py b/tests/test_provider_verify.py index 32d887d1..86be2741 100644 --- a/tests/test_provider_verify.py +++ b/tests/test_provider_verify.py @@ -29,14 +29,22 @@ def test_detect_provider(key, expected): # -- verify_provider_key: status-code mapping + per-provider request shape ------- -def _patch_get(monkeypatch, status=200, capture=None, raise_exc=None): +def _patch_get(monkeypatch, status=200, capture=None, raise_exc=None, json_body=None): def fake_get(url, **kwargs): if capture is not None: capture["url"] = url capture.update(kwargs) if raise_exc is not None: raise raise_exc - return SimpleNamespace(status_code=status) + + def _json(): + # json_body=None mimics a 200 that isn't JSON (only the local-provider + # branch ever calls .json(); everyone else passes on status alone). + if json_body is None: + raise ValueError("not JSON") + return json_body + + return SimpleNamespace(status_code=status, json=_json) monkeypatch.setattr("httpx.get", fake_get) @@ -85,20 +93,34 @@ def test_verify_gemini_key_param(monkeypatch): def test_verify_ollama_uses_v1_models_no_key(monkeypatch): cap: dict = {} - _patch_get(monkeypatch, status=200, capture=cap) - verify_provider_key("ollama", base_url="http://localhost:11434") + _patch_get(monkeypatch, status=200, capture=cap, json_body={"data": []}) + assert verify_provider_key("ollama", base_url="http://localhost:11434") == { + "ok": True + } assert cap["url"] == "http://localhost:11434/v1/models" assert "headers" not in cap # keyless def test_verify_lmstudio_uses_v1_models_no_key(monkeypatch): cap: dict = {} - _patch_get(monkeypatch, status=200, capture=cap) - verify_provider_key("lmstudio") # no base_url → LM Studio's default port + _patch_get(monkeypatch, status=200, capture=cap, json_body={"data": []}) + assert verify_provider_key("lmstudio") == {"ok": True} # no URL → the default port assert cap["url"] == "http://localhost:1234/v1/models" assert "headers" not in cap # keyless +def test_verify_local_rejects_non_model_server(monkeypatch): + """A 200 that isn't the OpenAI list shape (some other service on the port) must not + read as a working local provider.""" + _patch_get(monkeypatch, status=200) # 200 but not JSON (an HTML page) + res = verify_provider_key("lmstudio") + assert res["ok"] is False and "/v1" in res["error"] + + _patch_get(monkeypatch, status=200, json_body={"whatever": 1}) # JSON, wrong shape + res = verify_provider_key("ollama", base_url="http://localhost:11434") + assert res["ok"] is False and "/v1" in res["error"] + + def test_verify_network_error_is_clean(monkeypatch): _patch_get(monkeypatch, raise_exc=ConnectionError("boom")) res = verify_provider_key("openai", api_key="sk-x") diff --git a/tests/test_settings.py b/tests/test_settings.py index b10c9287..0941af0a 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -198,6 +198,20 @@ def test_lmstudio_models_gated_on_liveness(tmp_path, monkeypatch): assert "ollama:llama3.3" not in models # the other server is still down +class _LocalResp: + """Minimal httpx.Response stand-in for the local-server probes (json() raises on a + None body — a 200 that isn't JSON, e.g. some other service's HTML page).""" + + def __init__(self, status=200, body=None): + self.status_code = status + self._body = body + + def json(self): + if self._body is None: + raise ValueError("not JSON") + return self._body + + def test_local_models_parses_both_server_shapes(tmp_path, monkeypatch): """Ollama's native /api/tags and LM Studio's OpenAI-shaped /v1/models both map to `:` picker entries.""" @@ -215,15 +229,127 @@ def test_local_models_parses_both_server_shapes(tmp_path, monkeypatch): }, } - class _Resp: - def __init__(self, data): - self._data = data - - def json(self): - return self._data - import httpx - monkeypatch.setattr(httpx, "get", lambda url, timeout=None: _Resp(payloads[url])) + monkeypatch.setattr( + httpx, "get", lambda url, timeout=None: _LocalResp(200, payloads[url]) + ) assert manager._local_models("ollama") == ["ollama:llama3.3"] assert manager._local_models("lmstudio") == ["lmstudio:qwen/qwen3-coder-30b"] + + +def test_local_alive_requires_model_server_shape(tmp_path, monkeypatch): + """Liveness needs the provider's expected list container, not just a 200 — another + service answering on the port must not mark a local provider alive.""" + import httpx + + from coworker.server.manager import SessionManager + + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + + seen: list[str] = [] + + def openai_shape(url, timeout=None): + seen.append(url) + return _LocalResp(200, {"data": []}) + + monkeypatch.setattr(httpx, "get", openai_shape) + assert manager._local_alive("lmstudio") is True # empty list still counts + assert seen[-1] == "http://localhost:1234/v1/models" + assert manager._local_alive("ollama") is False # /api/tags wants {"models": …} + assert seen[-1] == "http://localhost:11434/api/tags" + + manager._refresh_provider() # drop the liveness cache before re-probing + monkeypatch.setattr(httpx, "get", lambda url, timeout=None: _LocalResp(200, None)) + assert manager._local_alive("lmstudio") is False # 200 but not JSON + + +def test_local_liveness_cache_invalidated_on_config_change(tmp_path, monkeypatch): + """Repointing a local server re-probes immediately instead of serving the previous + endpoint's liveness for up to 30s.""" + import httpx + + from coworker.server.manager import SessionManager + + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + + monkeypatch.setattr( + httpx, "get", lambda url, timeout=None: _LocalResp(200, {"data": []}) + ) + assert manager._local_alive("lmstudio") is True + + def down(url, timeout=None): + raise ConnectionError("down") + + monkeypatch.setattr(httpx, "get", down) + assert manager._local_alive("lmstudio") is True # cached — no re-probe yet + manager.set_provider("lmstudio", {"base_url": "http://localhost:4321"}) + assert manager._local_alive("lmstudio") is False # cache dropped → fresh probe + + +def test_model_ready_gated_on_local_liveness(tmp_path, monkeypatch): + """A dead local server must not report the default model as ready — the composer's + "No model connected" chip relies on model_ready telling the truth.""" + from coworker.server.manager import SessionManager + + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + manager.set_default_model("lmstudio:qwen/qwen3-coder-30b") + + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: False) + assert manager.get_settings()["model_ready"] is False + + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: True) + assert manager.get_settings()["model_ready"] is True + + +def test_first_keyless_detect_stores_profile_and_recommends(tmp_path, monkeypatch): + """The GUI persists keyless providers on every passing Detect, possibly with all-blank + fields. The stored EMPTY profile still counts as engaged: the live list probes the + default endpoint, so the recommended model auto-adds and wins the unset default.""" + import httpx + + from coworker.server.manager import SessionManager + + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + + monkeypatch.setattr( + httpx, + "get", + lambda url, timeout=None: _LocalResp( + 200, {"data": [{"id": "qwen/qwen3-coder-30b"}]} + ), + ) + res = manager.set_provider("lmstudio", {"base_url": ""}) # Detect at the default URL + assert res["ok"] is True + assert manager.secrets.get("provider:lmstudio") == {} # engaged, no overrides + assert "lmstudio:qwen/qwen3-coder-30b" in manager.get_settings()["models"] + assert manager.model == "lmstudio:qwen/qwen3-coder-30b" # fresh install → wins default + + +def test_verify_provider_explicit_blank_endpoint_means_default(tmp_path, monkeypatch): + """Clearing a stored endpoint in the form verifies the DEFAULT, not the old URL — a + passing Test must validate the config that would actually be saved.""" + from coworker.server import manager as manager_mod + from coworker.server.manager import SessionManager + + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + manager.set_provider("lmstudio", {"base_url": "http://old-box:1234"}) + + cap: dict = {} + + def fake_verify(name, *, api_key=None, base_url=None, fields=None, timeout=10.0): + cap.update({"name": name, "base_url": base_url}) + return {"ok": True} + + monkeypatch.setattr(manager_mod, "verify_provider_key", fake_verify) + manager.verify_provider("lmstudio", {"base_url": ""}) # explicitly cleared + assert cap["base_url"] == "" # NOT the stored http://old-box:1234 + manager.verify_provider("lmstudio", {}) # field omitted → stored value stands + assert cap["base_url"] == "http://old-box:1234" From 838e59e5431ef66d4d9c90de3df6a0d2d67169eb Mon Sep 17 00:00:00 2001 From: zalex Date: Mon, 3 Aug 2026 13:26:03 -0700 Subject: [PATCH 3/3] Surface failed keyless saves; close review-round test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex confirmation pass (round 2) verdicts: findings 1-5 FIXED, plus three residuals addressed here: - runTestAndSave no longer swallows a rejected/not-ok setProvider: a passing Detect whose save fails now shows the error instead of '✓ Tested & saved' (affected keyed providers too; the keyless path made it likelier) - the explicit-blank verify test stubs httpx BEFORE set_provider, so its suggested-models pass can't hit the network - regression test for the dead-local-default handoff: a dead local default yields to a newly configured provider, a live one is never stolen (pins _provider_available in the handoff path) - GUI test for the failed-save path Co-Authored-By: Claude Fable 5 --- .../gui/src/providers/ProviderSetup.test.tsx | 15 +++++++++++ surfaces/gui/src/providers/ProviderSetup.tsx | 13 +++++++-- tests/test_settings.py | 27 +++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/surfaces/gui/src/providers/ProviderSetup.test.tsx b/surfaces/gui/src/providers/ProviderSetup.test.tsx index 0374f235..87e3a45f 100644 --- a/surfaces/gui/src/providers/ProviderSetup.test.tsx +++ b/surfaces/gui/src/providers/ProviderSetup.test.tsx @@ -139,4 +139,19 @@ describe("useProviderSetup keyless Detect", () => { }); expect(api.setProvider).toHaveBeenCalledWith("lmstudio", { base_url: "" }); }); + + it("a failed save surfaces as an error, not '✓ Tested & saved'", async () => { + vi.mocked(api.getProviders).mockResolvedValue([LMSTUDIO]); + vi.mocked(api.verifyProvider).mockResolvedValue({ ok: true }); + vi.mocked(api.setProvider).mockResolvedValue({ ok: false, error: "disk full" }); + + let ps!: ProviderSetupState; + render( (ps = v)} />); + await waitFor(() => expect(ps.providers.length).toBe(1)); + act(() => ps.openProvider("lmstudio")); + await act(async () => { + expect(await ps.runTestAndSave()).toBe(false); + }); + expect(ps.verify).toEqual({ state: "error", msg: "disk full" }); + }); }); diff --git a/surfaces/gui/src/providers/ProviderSetup.tsx b/surfaces/gui/src/providers/ProviderSetup.tsx index ad49b7cc..581bd735 100644 --- a/surfaces/gui/src/providers/ProviderSetup.tsx +++ b/surfaces/gui/src/providers/ProviderSetup.tsx @@ -169,8 +169,17 @@ export function useProviderSetup(opts?: { onSaved?: () => void }): ProviderSetup // Keyless providers save on every passing Detect: they report `configured` out of // the box, so the dirty/configured gate would skip the FIRST-time save — leaving no // stored profile, which means set_provider's recommended-model auto-add never runs. - if (dirty || !info?.configured || !info?.needs_key) - await setProvider(sel, fields).catch(() => {}); + // A failed save must surface, not masquerade as "✓ Tested & saved". + if (dirty || !info?.configured || !info?.needs_key) { + const saved = await setProvider(sel, fields).catch(() => ({ ok: false as const })); + if (!saved?.ok) { + setVerify({ + state: "error", + msg: ("error" in saved && saved.error) || "verified, but saving failed — try again", + }); + return false; + } + } if (!info?.needs_key) setKeylessOk((s) => new Set(s).add(sel)); setVerify({ state: "ok" }); setDirty(false); diff --git a/tests/test_settings.py b/tests/test_settings.py index 0941af0a..c6512934 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -306,6 +306,27 @@ def test_model_ready_gated_on_local_liveness(tmp_path, monkeypatch): assert manager.get_settings()["model_ready"] is True +def test_dead_local_default_yields_to_configured_provider(tmp_path, monkeypatch): + """The first-working-provider handoff runs on AVAILABILITY, not configuredness: a + dead local default yields to a newly configured provider (keyless 'configured' + would wrongly protect it), while a live one is never stolen.""" + from coworker.server.manager import SessionManager + + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) + manager = SessionManager(data_dir=tmp_path / "data") + manager.set_default_model("lmstudio:qwen/qwen3-coder-30b") + + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: False) + manager.set_provider("anthropic", {"api_key": "sk-ant-x"}) + assert manager.model == "anthropic:claude-fable-5" # dead local default yielded + + manager.set_default_model("lmstudio:qwen/qwen3-coder-30b") + monkeypatch.setattr(SessionManager, "_local_alive", lambda self, name: True) + manager.set_provider("gemini", {"api_key": "AIza-x"}) + assert manager.model == "lmstudio:qwen/qwen3-coder-30b" # a live one is never stolen + + def test_first_keyless_detect_stores_profile_and_recommends(tmp_path, monkeypatch): """The GUI persists keyless providers on every passing Detect, possibly with all-blank fields. The stored EMPTY profile still counts as engaged: the live list probes the @@ -335,11 +356,17 @@ def test_first_keyless_detect_stores_profile_and_recommends(tmp_path, monkeypatc def test_verify_provider_explicit_blank_endpoint_means_default(tmp_path, monkeypatch): """Clearing a stored endpoint in the form verifies the DEFAULT, not the old URL — a passing Test must validate the config that would actually be saved.""" + import httpx + from coworker.server import manager as manager_mod from coworker.server.manager import SessionManager monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state")) manager = SessionManager(data_dir=tmp_path / "data") + # Stub the probe BEFORE set_provider — its suggested-models pass must not hit the net. + monkeypatch.setattr( + httpx, "get", lambda url, timeout=None: _LocalResp(200, {"data": []}) + ) manager.set_provider("lmstudio", {"base_url": "http://old-box:1234"}) cap: dict = {}