From 2e8c6df1e0ca3fafe03d1152bc301fa2ae0e7e4b Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sat, 1 Aug 2026 21:28:33 -0700 Subject: [PATCH 1/2] feat(providers): add the four OpenAI-compatible vendors OpenCode ships that we lacked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenCode enumerates its OpenAI-compatible providers in `packages/llm/src/providers/openai-compatible-profile.ts`: baseten, cerebras, deepinfra, deepseek, fireworks, groq, openrouter, togetherai, xai. Four of them — groq, cerebras, baseten, xai — had no row here. They are plain `/chat/completions` vendors, so this is metadata only: no production code, four `ProviderSpec` rows. The registry already synthesizes a working provider class from a row, which is why the port is this small. Model ids could not come from OpenCode, because its profile table has none to give: `OpenAICompatibleProfile` is `{provider, baseURL}` and nothing else, for all nine entries, and `providers/xai.ts` carries no ids either. Each id here was read from the vendor's own docs (2026-08-02) and pinned in `test_provider_registry.VENDOR_DEFAULTS`, the existing drift guard. Two rows needed model configs, not just registry entries: * `gpt-oss-120b` and `gpt-oss-20b` had no `MODEL_CONFIGS` row, so `get_model_config`'s prefix fallback — `key.rsplit("-", 1)[0]` — reduced them to `gpt` and matched the gpt-5.x family. Cerebras's default model is a bare `gpt-oss-120b`, so it silently inherited gpt-5.5's 272k context window, 128k output cap and $3/$15 pricing. The window is the damaging one: it sizes auto-compaction, so a session would run past the real 131k limit and die on a context-length 400 rather than compacting. * The namespaced `openai/gpt-oss-120b` that groq and baseten serve dodged that prefix but fell to the generic 200k default — also larger than the truth, so the same failure arriving later. Explicit rows for the namespaced forms are the remedy `get_model_config`'s own docstring documents. Adding rows to that table risks poisoning unrelated ids through the same prefix fallback, so every id's resolution was snapshotted before and after: only the two intended ones moved. `catalog_mode` is new on `ProviderSpec`. `dynamic_catalog` alone meant "discovered REPLACES static", which is right for the three rows that had it — sglang, vllm and ollama are local servers whose static ids are placeholder stubs — and backwards for a hosted vendor, whose curated list is deliberate and whose raw `/models` also lists speech, moderation and embedding models. Under the inherited default, Groq's curated ids vanished from the picker and ASR/TTS models took their place. The hosted rows now say `hybrid`, matching what `openrouter_provider` already passes by hand. Verified, since a table of URLs is the easy thing to get quietly wrong: every base URL answers `/models` with 401/403 rather than 404, and the real CLI run against each with a bogus key returns a distinct vendor-shaped auth error — so the URL and auth wiring reach the actual vendor, not merely a registry entry. All four declare `dynamic_catalog="openai-compatible"`, so the curated `available_models` list can be refreshed from the vendor at runtime rather than stranding users on ids that will age the same way OpenCode's did. xai takes the Chat Completions route, which is a deliberate departure: OpenCode defaults its xai facade to the Responses protocol (`providers/xai.ts`, `model: responses`), the same shape #783 gave first-party OpenAI. The reason is structural rather than a coin flip — `openai_responses` is imported only by `openai_provider`, and `_use_responses` sits behind `_is_first_party_base_url()`, which #783 scoped to api.openai.com, so Responses is not something a registry row can select at all. It would need a hand-written class and a carve-out in that gate. Chat Completions is also known-good for this model rather than assumed: OpenCode's own docs serve grok-4.5 over both protocols, routing it to `/chat/completions` in `web/src/content/docs/go.mdx` and to `/responses` in `zen.mdx`. `test_xai_requests_go_to_chat_completions` asserts the URL actually requested, so the decision is revisited deliberately, not by drift. Examined and deliberately NOT ported: * OpenRouter's `usage: {include: true}` (openrouter.ts:57-62). Probed live against the real API: the response is byte-identical with and without it — `cost` and `cost_details` come back either way. Sending it would be cargo cult. * DeepSeek. OpenCode treats it as a plain profile with no special handling; clawcodex already does more (prompt-prefix-cache usage re-mapping, and `reasoning_content` in both directions). * The profile-table architecture itself, which `ProviderSpec` already matches and exceeds — 23 rows to OpenCode's 9. While pinning the new aliases, the collision test turned out to only iterate `_SPECS`, so it could not see a new alias shadowing one of the seven hand-written providers (anthropic, deepseek, gemini, minimax, openai, openrouter, zai) — a mutant adding `deepseek` as an xai alias survived it. It is now seeded from provider ids AND `PROVIDER_ALIASES`, since names like `glm` and `z.ai` are aliases rather than ids and were shadowable through the same hole. Co-Authored-By: Claude Opus 5 --- src/models/configs.py | 52 +++++ src/providers/openai_compatible_specs.py | 93 +++++++++ tests/test_opencode_compat_providers.py | 245 +++++++++++++++++++++++ tests/test_provider_registry.py | 14 +- 4 files changed, 403 insertions(+), 1 deletion(-) create mode 100644 tests/test_opencode_compat_providers.py diff --git a/src/models/configs.py b/src/models/configs.py index 291d2419..2eeb0d8f 100644 --- a/src/models/configs.py +++ b/src/models/configs.py @@ -277,6 +277,58 @@ class ModelConfig: # stating explicitly because "GLM-5.2 is multimodal" is an easy and # costly assumption: it makes glm-5.2 look like a valid *vision* half # for a fusion model, where it would fail on every image. + # OpenAI's open-weight models, served by several hosts in this registry + # (cerebras as ``gpt-oss-120b``, groq/baseten as ``openai/gpt-oss-120b``). + # + # These rows exist to STOP a prefix match, not merely to describe a model. + # ``get_model_config`` falls back to ``key.rsplit("-", 1)[0]``, under which + # "gpt-oss-120b" reduces to "gpt" and collided with the gpt-5.x family — + # so a bare gpt-oss id silently inherited gpt-5.5's 272k window, its 128k + # output cap and its $3/$15 pricing. That window sizes auto-compaction, so + # the session would run past the real limit and die on a context-length + # 400 rather than compacting. The namespaced ``openai/gpt-oss-120b`` did + # not start with "gpt" and so got the safe generic defaults instead: one + # model behaving two ways depending on which host served it. + # + # 131,072 both ways per OpenAI's model docs (2026-08-02). Hosts may cap + # output lower; the context window is the load-bearing number here. + "gpt-oss-120b": ModelConfig( + model_id="gpt-oss-120b", + display_name="GPT-OSS 120B", + context_window=131_072, + max_output_tokens=131_072, + supports_cache=False, + supports_vision=False, + ), + "gpt-oss-20b": ModelConfig( + model_id="gpt-oss-20b", + display_name="GPT-OSS 20B", + context_window=131_072, + max_output_tokens=131_072, + supports_cache=False, + supports_vision=False, + ), + # The namespaced forms groq and baseten actually serve. Explicit rows are + # the documented remedy for a vendor-qualified id needing a real window + # (see ``get_model_config``'s docstring and ``openai/gpt-5.6-luna``) — + # without them these fall to the generic 200k default, which is LARGER + # than the true 131k, so compaction is sized past the real limit. + "openai/gpt-oss-120b": ModelConfig( + model_id="openai/gpt-oss-120b", + display_name="GPT-OSS 120B", + context_window=131_072, + max_output_tokens=131_072, + supports_cache=False, + supports_vision=False, + ), + "openai/gpt-oss-20b": ModelConfig( + model_id="openai/gpt-oss-20b", + display_name="GPT-OSS 20B", + context_window=131_072, + max_output_tokens=131_072, + supports_cache=False, + supports_vision=False, + ), "glm-5.2": ModelConfig( model_id="glm-5.2", display_name="GLM-5.2", diff --git a/src/providers/openai_compatible_specs.py b/src/providers/openai_compatible_specs.py index d96ad5b6..2df1ac3c 100644 --- a/src/providers/openai_compatible_specs.py +++ b/src/providers/openai_compatible_specs.py @@ -70,6 +70,17 @@ class ProviderSpec: #: churning hosted catalogs). None = static list only. See #: src/providers/model_discovery.py (the discoveryService port). dynamic_catalog: str | None = None + #: How a discovered list combines with ``available_models``: + #: ``"dynamic"`` (discovered REPLACES the static list) or ``"hybrid"`` + #: (static first, discovered appended). Defaults to ``"dynamic"``, which + #: is right for a LOCAL server — sglang/vllm/ollama ship placeholder + #: model ids and the endpoint is the only truth. A hosted vendor is the + #: opposite: its curated list is deliberate, while its raw ``/models`` + #: includes speech, moderation and embedding models that do not belong in + #: a coding agent's picker. ``openrouter_provider`` already passes + #: ``hybrid`` by hand for exactly this reason; this field lets a spec row + #: say the same thing. + catalog_mode: str = "dynamic" #: Generated subclass name (for repr / debugging). Derived from ``id`` when #: omitted. class_name: str = "" @@ -324,6 +335,87 @@ def resolved_class_name(self) -> str: env_vars=("META_API_KEY", "META_AI_API_KEY"), aliases=("meta-ai", "meta_ai", "muse", "muse-spark"), ), + # The four OpenAI-compatible vendors OpenCode ships that this table did + # not (reference_projects/opencode/packages/llm/src/providers/ + # openai-compatible-profile.ts). Base URLs are OpenCode's, each confirmed + # live (2026-08-02) to answer /models with a 401/403 rather than a 404, so + # the host and path are current. + # + # Model ids could NOT come from that file: ``OpenAICompatibleProfile`` is + # ``{provider, baseURL}`` and nothing else, for all nine entries, and + # ``providers/xai.ts`` carries no ids either. They were read from each + # vendor's own docs on 2026-08-02 instead. + ProviderSpec( + id="groq", + dynamic_catalog="openai-compatible", + catalog_mode="hybrid", + label="Groq", + default_base_url="https://api.groq.com/openai/v1", + default_model="openai/gpt-oss-120b", + available_models=( + "openai/gpt-oss-120b", + "openai/gpt-oss-20b", + "llama-3.3-70b-versatile", + "llama-3.1-8b-instant", + ), + env_vars=("GROQ_API_KEY",), + aliases=("groq-cloud", "groqcloud"), + ), + ProviderSpec( + id="cerebras", + dynamic_catalog="openai-compatible", + catalog_mode="hybrid", + label="Cerebras", + default_base_url="https://api.cerebras.ai/v1", + default_model="gpt-oss-120b", + available_models=("gpt-oss-120b", "zai-glm-4.7"), + env_vars=("CEREBRAS_API_KEY",), + aliases=("cerebras-cloud", "cerebras_cloud"), + ), + ProviderSpec( + id="baseten", + dynamic_catalog="openai-compatible", + catalog_mode="hybrid", + label="Baseten", + default_base_url="https://inference.baseten.co/v1", + default_model="deepseek-ai/DeepSeek-V4-Pro", + available_models=( + "deepseek-ai/DeepSeek-V4-Pro", + "deepseek-ai/DeepSeek-V4-Flash-0731", + "moonshotai/Kimi-K3", + "zai-org/GLM-5.2", + "openai/gpt-oss-120b", + ), + env_vars=("BASETEN_API_KEY",), + aliases=("base-ten", "base_ten"), + ), + # xAI also exposes an OpenAI *Responses* endpoint, and OpenCode defaults + # its xai facade to that protocol (providers/xai.ts: `model: responses`). + # This row deliberately takes the Chat Completions route instead, for a + # structural reason rather than only an absence of evidence: + # ``openai_responses`` is imported solely by ``openai_provider``, and + # ``_use_responses`` sits behind ``_is_first_party_base_url()``, which + # #783 scoped to api.openai.com. Routing xai over Responses is therefore + # not a row change at all — it needs a hand-written class plus a carve-out + # in that host gate. + # + # Chat Completions is also known-good for this model rather than merely + # assumed: OpenCode's own docs serve grok-4.5 over BOTH protocols, routing + # it to /chat/completions via @ai-sdk/openai-compatible in + # packages/web/src/content/docs/go.mdx and to /responses via @ai-sdk/openai + # in zen.mdx. A row is the right shape for the former; the latter would be + # a separate change, made by someone who can probe it with a real key. + ProviderSpec( + id="xai", + dynamic_catalog="openai-compatible", + catalog_mode="hybrid", + label="xAI (Grok)", + default_base_url="https://api.x.ai/v1", + default_model="grok-4.5", + available_models=("grok-4.5", "grok-4.3"), + env_vars=("XAI_API_KEY", "GROK_API_KEY"), + aliases=("x-ai", "x_ai", "grok"), + ), ) @@ -398,6 +490,7 @@ def get_available_models(self) -> list[str]: getattr(self, "api_key", None) or None, spec.dynamic_catalog, spec.available_models, + mode=spec.catalog_mode, ) diff --git a/tests/test_opencode_compat_providers.py b/tests/test_opencode_compat_providers.py new file mode 100644 index 00000000..8d745c51 --- /dev/null +++ b/tests/test_opencode_compat_providers.py @@ -0,0 +1,245 @@ +"""The OpenAI-compatible vendors ported from OpenCode's profile table. + +OpenCode enumerates its OpenAI-compatible providers in +``packages/llm/src/providers/openai-compatible-profile.ts``. Four of them had +no row here: groq, cerebras, baseten and xai. They are plain +``/chat/completions`` vendors, so they need no code — only correct metadata, +which is exactly what is easy to get wrong and what these tests pin. + +Model ids did not come from OpenCode, because its profile table has none to +give: ``OpenAICompatibleProfile`` is ``{provider, baseURL}`` and nothing else. +They were read from each vendor's own docs; `test_provider_registry` pins +those values as the guard against drift. +""" + +from __future__ import annotations + +import tempfile +import time +from pathlib import Path +from unittest.mock import patch + +import pytest + +from src.providers import PROVIDER_INFO, get_provider_class +from src.providers.openai_compatible_specs import SPECS_BY_ID, _SPECS + +PORTED = ("groq", "cerebras", "baseten", "xai") + + +@pytest.mark.parametrize("provider_id", PORTED) +def test_the_provider_is_registered_and_usable(provider_id: str) -> None: + assert provider_id in SPECS_BY_ID + assert provider_id in PROVIDER_INFO + provider = get_provider_class(provider_id)(api_key="sk-test") + assert provider.base_url == SPECS_BY_ID[provider_id].default_base_url + assert provider.model == SPECS_BY_ID[provider_id].default_model + + +@pytest.mark.parametrize("provider_id", PORTED) +def test_discovery_extends_the_curated_list_instead_of_replacing_it( + provider_id: str, +) -> None: + """Curation survives a live `/models` read; discovery only appends. + + These are HOSTED vendors, so the default `dynamic` mode is wrong for + them: it means "discovered REPLACES static", which is right for a local + sglang/vllm/ollama server whose static ids are placeholder stubs, and + backwards for a vendor whose raw catalog also lists speech, moderation + and embedding models. Under `dynamic` the curated ids vanished from the + picker entirely and ASR/TTS models took their place. + """ + import src.providers.model_discovery as discovery + + spec = SPECS_BY_ID[provider_id] + discovered = ["whisper-large-v3", "some-embedding-model"] + + # Driven through the provider, NOT through `_merge` directly. Asserting + # `spec.catalog_mode == "hybrid"` and `_merge("hybrid", ...)` separately + # tests both ENDS of the fix and never the wire between them: deleting + # `mode=spec.catalog_mode` from the discovery call left the whole suite + # green while curation was destroyed at runtime. + # + # The cache dir is pinned ONCE for the call. Building a fresh temp dir + # per invocation would hand every call an empty cache and fake a pass. + with tempfile.TemporaryDirectory() as cache_dir: + cache_file = Path(cache_dir) / "model-discovery-cache.json" + with patch.object(discovery, "_cache_path", lambda: cache_file): + # Seeded rather than fetched: `discovered_models` refreshes in the + # BACKGROUND and returns immediately, so a patched fetch would not + # have landed by the time the call returns. Priming the cache + # exercises the same merge path deterministically. + key = discovery._cache_key(provider_id, spec.default_base_url) + discovery._write_cache( + {key: {"models": discovered, "fetched_at": time.time()}} + ) + provider = get_provider_class(provider_id)(api_key="sk-test") + models = provider.get_available_models() + + curated = list(spec.available_models) + assert models[: len(curated)] == curated, ( + f"curation did not survive discovery: {models}" + ) + assert "whisper-large-v3" in models, "discovered models were not appended" + + +@pytest.mark.parametrize("provider_id", PORTED) +def test_the_default_model_is_one_of_the_offered_models(provider_id: str) -> None: + spec = SPECS_BY_ID[provider_id] + assert spec.default_model in spec.available_models + + +@pytest.mark.parametrize("provider_id", PORTED) +def test_the_key_is_sourced_from_the_vendor_env_var(provider_id: str) -> None: + """Each vendor's conventional variable, so an existing shell just works.""" + expected = { + "groq": "GROQ_API_KEY", + "cerebras": "CEREBRAS_API_KEY", + "baseten": "BASETEN_API_KEY", + "xai": "XAI_API_KEY", + }[provider_id] + assert expected in SPECS_BY_ID[provider_id].env_vars + + +def test_no_id_or_alias_collides_across_the_whole_registry() -> None: + """A collision would silently shadow an existing provider. + + Spans the FULL namespace, not just the spec table. Seven providers are + hand-written and absent from ``_SPECS`` — anthropic, deepseek, gemini, + minimax, openai, openrouter, zai — so a table-only check cannot see a new + alias shadowing one of them, which is precisely the collision the new + rows are most able to cause (`grok`, `base-ten`, `groqcloud`). + """ + from src.providers import PROVIDER_ALIASES + + # Seeded from ids AND the hand-written alias map: PROVIDER_ALIASES holds + # names like `glm` / `z.ai` that are aliases rather than ids, so an + # id-only seed left them shadowable (a mutant adding `glm` as an xai + # alias survived). `setdefault` means the hand-written entry wins, so + # such a collision leaves the NEW alias silently dead rather than + # hijacking the old one — still a bug, just a quiet one. + seen: dict[str, str] = {pid: pid for pid in PROVIDER_INFO} + for alias, target in PROVIDER_ALIASES.items(): + seen.setdefault(alias, target) + collisions = [] + for spec in _SPECS: + for name in (spec.id, *(spec.aliases or ())): + owner = seen.get(name) + if owner is not None and owner != spec.id: + collisions.append((name, owner, spec.id)) + seen[name] = spec.id + assert not collisions, f"name collisions: {collisions}" + + +def test_xai_requests_go_to_chat_completions() -> None: + """OpenCode defaults xai to the Responses protocol; this row does not. + + Asserts the URL actually requested, not the provider's type. An earlier + version of this test checked ``not hasattr(provider, "_use_responses")``, + which holds for 29 of the 30 registered providers — it restated "xai is + not OpenAIProvider", something ``get_provider_class`` already guarantees, + and stayed green when the row was mutated to declare Responses. + + The reason for the departure is structural, not just an absence of + evidence: ``openai_responses`` is imported only by ``openai_provider``, + and ``_use_responses`` is gated behind ``_is_first_party_base_url()``, + which #783 scoped to api.openai.com. Routing xai over Responses is not a + row change — it needs a hand-written class and a carve-out in that gate. + """ + seen: dict[str, object] = {} + + class _Resp: + status_code = 200 + + def raise_for_status(self): + return None + + def json(self): + return { + "id": "x", + "choices": [{"message": {"role": "assistant", "content": "hi"}, + "finish_reason": "stop"}], + "usage": {"prompt_tokens": 1, "completion_tokens": 1}, + } + + provider = get_provider_class("xai")(api_key="sk-test") + + import httpx + + def _spy(self, request, **kwargs): # pragma: no cover - exercised below + seen["url"] = str(request.url) + raise RuntimeError("stop-before-network") + + with patch.object(httpx.Client, "send", _spy): + try: + provider.chat([{"role": "user", "content": "hi"}]) + except Exception: + pass + + url = str(seen.get("url", "")) + assert url, "no HTTP request was attempted" + assert url.endswith("/chat/completions"), url + assert "/responses" not in url + + +# --- model-config resolution -------------------------------------------- + + +@pytest.mark.parametrize( + "model_id", ["gpt-oss-120b", "gpt-oss-20b", "openai/gpt-oss-120b", "openai/gpt-oss-20b"] +) +def test_gpt_oss_does_not_inherit_a_gpt_5_config(model_id: str) -> None: + """`gpt-oss-*` must resolve to itself, never to the gpt-5.x family. + + `get_model_config` falls back to a prefix match on `key.rsplit("-", 1)[0]`, + under which `gpt-oss-120b` reduces to `gpt` and collided with `gpt-5.5` — + silently inheriting a 272k context window, a 128k output cap and $3/$15 + pricing. The window is the dangerous one: it sizes auto-compaction, so a + session would sail past the real 131k limit and die on a context-length + 400 instead of compacting. + + Cerebras's default model is a bare `gpt-oss-120b`, which is how this + became reachable. The namespaced `openai/gpt-oss-120b` that groq and + baseten serve dodged the prefix but fell to the generic 200k default — + also larger than the truth, and so the same failure more slowly. + """ + from src.models.configs import get_model_config + + config = get_model_config(model_id) + assert config is not None, f"{model_id} has no config row" + assert "gpt-5" not in config.model_id, ( + f"{model_id} resolved to {config.model_id}" + ) + assert config.context_window == 131_072, config.context_window + + +def test_every_ported_default_model_resolves_to_itself_or_to_nothing() -> None: + """No new default may borrow another model's limits. + + A row whose default silently resolves to a different model inherits that + model's context window, which is a correctness bug rather than a cosmetic + one. `None` is fine — the caller then applies conservative defaults. + """ + from src.models.configs import get_model_config + + for provider_id in PORTED: + default = SPECS_BY_ID[provider_id].default_model + config = get_model_config(default) + if config is None: + continue + assert config.model_id == default, ( + f"{provider_id}: default {default!r} resolved to {config.model_id!r}" + ) + + +def test_local_server_rows_keep_the_replace_semantics() -> None: + """The asymmetry `catalog_mode` introduces, guarded from both sides. + + sglang/vllm/ollama ship placeholder ids (`deepseek-coder:1.3b`), and the + endpoint is the only truth — `dynamic` is what actually removes the stub + once a real list arrives. Flipping the field's default to `hybrid` would + make those stubs permanent while leaving every hosted row unchanged, so + the hosted tests alone cannot see it. + """ + for provider_id in ("ollama", "vllm", "sglang"): + assert SPECS_BY_ID[provider_id].catalog_mode == "dynamic", provider_id diff --git a/tests/test_provider_registry.py b/tests/test_provider_registry.py index 36e69482..35f3b4cc 100644 --- a/tests/test_provider_registry.py +++ b/tests/test_provider_registry.py @@ -27,7 +27,7 @@ build_provider_class, ) -# The 19 OpenAI-compatible providers added via the registry. +# The OpenAI-compatible providers added via the registry. EXPECTED_NEW_PROVIDERS = { "nvidia-nim", "atlascloud", @@ -48,6 +48,11 @@ "stepfun", "deepinfra", "meta", + # OpenCode's openai-compatible profiles that this registry was missing + "groq", + "cerebras", + "baseten", + "xai", } # A sample of (id -> (base_url, default_model)) — each vendor's published @@ -61,6 +66,13 @@ "stepfun": ("https://api.stepfun.ai/v1", "step-3.7-flash"), "siliconflow-cn": ("https://api.siliconflow.cn/v1", "deepseek-ai/DeepSeek-V4-Pro"), "meta": ("https://api.meta.ai/v1", "muse-spark-1.1"), + # Verified against each vendor's own docs 2026-08-02, and pinned here + # because model ids age. They did not come from OpenCode: the profile + # table this registry mirrors carries base URLs only, no ids at all. + "groq": ("https://api.groq.com/openai/v1", "openai/gpt-oss-120b"), + "cerebras": ("https://api.cerebras.ai/v1", "gpt-oss-120b"), + "baseten": ("https://inference.baseten.co/v1", "deepseek-ai/DeepSeek-V4-Pro"), + "xai": ("https://api.x.ai/v1", "grok-4.5"), } From fc806e46b261c8007a6d2c924992d2fa601d63ba Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Sat, 1 Aug 2026 22:24:01 -0700 Subject: [PATCH 2/2] test(git): stop test_run_git_timeout racing a real 1ms git log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test ran `git log` with `timeout=0.001` and asserted `rc != 0 or stdout == ""`. That is a race against the runner: when git finishes inside the millisecond the call succeeds with real output and the assertion fails. It failed, passed, then failed again across three CI runs of an unrelated change. Passing was no better than failing — a timeout that never fires exercises none of the branch under test, so the green runs asserted nothing. Now forces `subprocess.TimeoutExpired` and asserts what `_run_git` actually returns for it: `("", "Command timed out", -1)`. Mutating the return code or the message fails it; previously neither did. Co-Authored-By: Claude Opus 5 --- tests/test_git_utilities.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/test_git_utilities.py b/tests/test_git_utilities.py index f25297ef..d6e939a1 100644 --- a/tests/test_git_utilities.py +++ b/tests/test_git_utilities.py @@ -1,5 +1,6 @@ import os import subprocess +from unittest import mock import tempfile import pytest @@ -55,8 +56,26 @@ def test_run_git_failure(self): assert rc != 0 def test_run_git_timeout(self): - stdout, stderr, rc = _run_git(["log"], timeout=0.001) - assert rc != 0 or stdout == "" + """A timed-out git call reports failure rather than partial output. + + Driven by a forced ``TimeoutExpired`` instead of a 1ms budget on a + real ``git log``. The old form asserted ``rc != 0 or stdout == ""`` + after a genuine 0.001s timeout, which is a race: when the runner is + fast enough for git to finish inside the millisecond, the call + succeeds with real output and the assertion fails. It flaked on CI + (fail / pass / fail across three runs of an unrelated change) while + testing nothing on the runs where it passed for the wrong reason — + a timeout that never fired exercises none of this branch. + """ + with mock.patch( + "subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd=["git", "log"], timeout=0.001), + ): + stdout, stderr, rc = _run_git(["log"], timeout=0.001) + + assert rc == -1 + assert stdout == "" + assert stderr == "Command timed out" class TestGetRepoRoot: