From f3602cd8cc7e01f7be77ae3b99417930b7ad14c2 Mon Sep 17 00:00:00 2001 From: Xubin Ren <52506698+Re-bin@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:14:39 +0800 Subject: [PATCH] fix(providers): harden OAuth model discovery --- docs/configuration.md | 4 +- docs/providers.md | 5 +- nanobot/providers/github_copilot_provider.py | 19 +++- nanobot/providers/oauth_model_catalog.py | 103 +++++++++++-------- nanobot/providers/registry.py | 15 ++- nanobot/providers/xai_grok_provider.py | 4 +- tests/providers/test_oauth_model_catalog.py | 76 ++++++++++---- tests/providers/test_xai_grok_provider.py | 35 ++++++- 8 files changed, 188 insertions(+), 73 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 28bd9bb84..435f54428 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -814,8 +814,8 @@ a nanobot update. GitHub Copilot uses OAuth instead of API keys. Requires a [GitHub account with a plan](https://github.com/features/copilot/plans) configured. No `providers.github_copilot` block is needed in `config.json`; `nanobot provider login` stores the OAuth session outside config. After login, the WebUI loads the account-specific Copilot model catalog online. -Only models compatible with nanobot's current Copilot chat-completions transport -are shown; Responses-only entries are intentionally omitted. +Only models compatible with nanobot's current chat-completions or Responses +transport are shown. For GitHub Enterprise / Copilot for Business, set the endpoint overrides you need before login: ```bash diff --git a/docs/providers.md b/docs/providers.md index ef012d88c..56db7211f 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -608,9 +608,8 @@ nanobot provider login github-copilot --set-main ``` The WebUI reads the models enabled for the signed-in Copilot account. nanobot -currently lists only entries that support Copilot's chat-completions endpoint; -models exposed solely through the Responses endpoint stay hidden until that -wire protocol is supported by the Copilot provider. +lists entries that support its current Copilot chat-completions or Responses +transport and hides models that it cannot route safely. Each command authenticates the selected provider and makes its current default model active. OpenAI Codex and eligible GitHub Copilot models participate in [Responses state retention](./configuration.md#responses-state-and-compaction), while native compaction remains provider-capability-specific. OAuth providers are not valid automatic fallbacks. See [`troubleshooting.md`](./troubleshooting.md#provider-and-model-problems) for proxy, headless-login, model-name, and config-key errors. diff --git a/nanobot/providers/github_copilot_provider.py b/nanobot/providers/github_copilot_provider.py index 513501613..e2925835b 100644 --- a/nanobot/providers/github_copilot_provider.py +++ b/nanobot/providers/github_copilot_provider.py @@ -314,7 +314,7 @@ def get_github_copilot_model_catalog( account_key = _catalog_account_key(getattr(token, "account_id", None)) cache_key = ( f"{storage.get_token_path()}\0{account_key}\0" - f"{_resolve('NANOBOT_COPILOT_BASE_URL', DEFAULT_COPILOT_BASE_URL)}" + f"{_resolve('NANOBOT_COPILOT_BASE_URL', DEFAULT_COPILOT_BASE_URL)}\0{proxy or ''}" ) return _GITHUB_COPILOT_MODEL_CATALOG.get(cache_key=cache_key, proxy=proxy) @@ -389,10 +389,7 @@ def _parse_github_copilot_models(payload: Any) -> tuple[ProviderModelSpec, ...]: or wire_id in seen or row.get("model_picker_enabled") is not True or policy.get("state") == "disabled" - or ( - isinstance(endpoints, list) - and "/chat/completions" not in cast(list[object], endpoints) - ) + or not _copilot_transport_supported(wire_id, endpoints) ): continue seen.add(wire_id) @@ -419,6 +416,18 @@ def _parse_github_copilot_models(payload: Any) -> tuple[ProviderModelSpec, ...]: return tuple(models) +def _copilot_transport_supported(wire_id: str, endpoints: object) -> bool: + if not isinstance(endpoints, list): + return True + supported = cast(list[object], endpoints) + if "/chat/completions" in supported: + return True + model = wire_id.lower() + return "/responses" in supported and any( + token in model for token in ("gpt-5", "o1", "o3", "o4") + ) + + def _oauth_fallback_models(provider_name: str) -> tuple[ProviderModelSpec, ...]: spec = find_by_name(provider_name) assert spec is not None diff --git a/nanobot/providers/oauth_model_catalog.py b/nanobot/providers/oauth_model_catalog.py index 8503475ae..f9359fde6 100644 --- a/nanobot/providers/oauth_model_catalog.py +++ b/nanobot/providers/oauth_model_catalog.py @@ -73,49 +73,51 @@ class OAuthModelCatalog: def get(self, *, cache_key: str, proxy: str | None = None) -> OAuthModelCatalogSnapshot: """Return a fresh catalog, sharing concurrent work and retaining a fallback.""" - while True: - with self._condition: + with self._condition: + generation = self._generation + cached = self._cached_result(cache_key) + if cached is not None: + return cached + while cache_key in self._inflight: + self._condition.wait() + if generation != self._generation: + return self._stale_or_fallback(None, self._monotonic()) cached = self._cached_result(cache_key) if cached is not None: return cached - while cache_key in self._inflight: - self._condition.wait() - cached = self._cached_result(cache_key) - if cached is not None: - return cached - generation = self._generation - self._inflight.add(cache_key) + self._inflight.add(cache_key) - try: - models = tuple(self._fetch(proxy)) - if not models: - raise ValueError("provider returned an empty model catalog") - except Exception as exc: - logger.warning("OAuth model catalog refresh failed: type={}", type(exc).__name__) - with self._condition: - invalidated = generation != self._generation - result = self._failure_result(cache_key) if not invalidated else None - else: - now = self._monotonic() - result = OAuthModelCatalogSnapshot( - models=models, - source="remote", - fetched_at=self._wall_clock(), + try: + models = tuple(self._fetch(proxy)) + if not models: + raise ValueError("provider returned an empty model catalog") + except Exception as exc: + logger.warning("OAuth model catalog refresh failed: type={}", type(exc).__name__) + with self._condition: + result = ( + self._stale_or_fallback(None, self._monotonic()) + if generation != self._generation + else self._failure_result(cache_key) ) - with self._condition: - invalidated = generation != self._generation - if not invalidated: - self._store(cache_key, _CacheEntry(snapshot=result, stored_at=now)) - self._failures.pop(cache_key, None) - finally: - with self._condition: - self._inflight.discard(cache_key) - self._condition.notify_all() + else: + now = self._monotonic() + result = OAuthModelCatalogSnapshot( + models=models, + source="remote", + fetched_at=self._wall_clock(), + ) + with self._condition: + if generation != self._generation: + result = self._stale_or_fallback(None, now) + else: + self._store(cache_key, _CacheEntry(snapshot=result, stored_at=now)) + self._failures.pop(cache_key, None) + finally: + with self._condition: + self._inflight.discard(cache_key) + self._condition.notify_all() - if invalidated: - continue - assert result is not None - return result + return result def invalidate(self) -> None: """Drop cached work and prevent an older identity refresh from being stored.""" @@ -123,18 +125,23 @@ class OAuthModelCatalog: self._generation += 1 self._entries.clear() self._failures.clear() + self._condition.notify_all() def _cached_result(self, cache_key: str) -> OAuthModelCatalogSnapshot | None: now = self._monotonic() entry = self._entries.get(cache_key) if entry is not None and now - entry.stored_at < self._fresh_ttl_s: return replace(entry.snapshot, source="cache") - if self._failures.get(cache_key, 0) > now: + failure_until = self._failures.get(cache_key) + if failure_until is not None and failure_until <= now: + self._failures.pop(cache_key, None) + elif failure_until is not None: return self._stale_or_fallback(entry, now) return None def _failure_result(self, cache_key: str) -> OAuthModelCatalogSnapshot: now = self._monotonic() + self._reserve(cache_key) self._failures[cache_key] = now + self._failure_ttl_s return self._stale_or_fallback(self._entries.get(cache_key), now) @@ -157,12 +164,24 @@ class OAuthModelCatalog: ) def _store(self, cache_key: str, entry: _CacheEntry) -> None: - if cache_key not in self._entries and len(self._entries) >= self._max_entries: - oldest = min(self._entries, key=lambda key: self._entries[key].stored_at) - self._entries.pop(oldest, None) - self._failures.pop(oldest, None) + self._reserve(cache_key) self._entries[cache_key] = entry + def _reserve(self, cache_key: str) -> None: + known = set(self._entries) | set(self._failures) + if cache_key in known or len(known) < self._max_entries: + return + oldest = min( + known, + key=lambda key: ( + self._entries[key].stored_at + if key in self._entries + else self._failures[key] - self._failure_ttl_s + ), + ) + self._entries.pop(oldest, None) + self._failures.pop(oldest, None) + def get_oauth_model_catalog( provider_name: str, diff --git a/nanobot/providers/registry.py b/nanobot/providers/registry.py index 1333e1103..bbfa65789 100644 --- a/nanobot/providers/registry.py +++ b/nanobot/providers/registry.py @@ -153,6 +153,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( backend="openai_compat", is_direct=True, ), + # === Azure OpenAI (direct API calls with API version 2024-10-21) ===== ProviderSpec( name="azure_openai", @@ -315,6 +316,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( detect_by_base_keyword="siliconflow", default_api_base="https://api.siliconflow.cn/v1", ), + # Novita AI: OpenAI-compatible gateway for hosted model APIs. ProviderSpec( name="novita", @@ -326,6 +328,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( detect_by_base_keyword="novita", default_api_base="https://api.novita.ai/openai", ), + # VolcEngine (火山引擎): OpenAI-compatible gateway, pay-per-use models ProviderSpec( name="volcengine", @@ -339,6 +342,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( thinking_style="thinking_type", supports_max_completion_tokens=True, ), + # VolcEngine Coding Plan (火山引擎 Coding Plan): same key as volcengine ProviderSpec( name="volcengine_coding_plan", @@ -352,6 +356,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( thinking_style="thinking_type", supports_max_completion_tokens=True, ), + # BytePlus: VolcEngine international, pay-per-use models ProviderSpec( name="byteplus", @@ -365,6 +370,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( strip_model_prefix=True, thinking_style="thinking_type", ), + # BytePlus Coding Plan: same key as byteplus ProviderSpec( name="byteplus_coding_plan", @@ -377,6 +383,8 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( strip_model_prefix=True, thinking_style="thinking_type", ), + + # === Standard providers (matched by model-name keywords) =============== # Anthropic: native Anthropic SDK ProviderSpec( @@ -492,6 +500,11 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( display_name="Github Copilot", model_catalog="hybrid", builtin_models=( + ProviderModelSpec( + id="github-copilot/gpt-5.4-mini", + label="GPT-5.4 Mini", + description="GitHub Copilot Responses model.", + ), ProviderModelSpec( id="github-copilot/gpt-4.1", label="GPT-4.1", @@ -768,7 +781,7 @@ PROVIDERS: tuple[ProviderSpec, ...] = ( env_key="QIANFAN_API_KEY", display_name="Qianfan", backend="openai_compat", - default_api_base="https://qianfan.baidubce.com/v2", + default_api_base="https://qianfan.baidubce.com/v2" ), ) diff --git a/nanobot/providers/xai_grok_provider.py b/nanobot/providers/xai_grok_provider.py index 12ad25e46..f25ebaae3 100644 --- a/nanobot/providers/xai_grok_provider.py +++ b/nanobot/providers/xai_grok_provider.py @@ -207,6 +207,7 @@ class XAIGrokProvider(LLMProvider): ) if on_stream_recover is not None: await on_stream_recover() + headers = _build_headers(token.access, wire_model) content, tool_calls, finish_reason, usage, reasoning_content = result usage = _combine_usage(retry_usage, usage) @@ -614,10 +615,11 @@ def _xai_error_response(exc: Exception) -> LLMResponse: ) message = str(exc).strip() or "unexpected error" retry_after = getattr(exc, "retry_after", None) + usage = getattr(exc, "usage", None) return LLMResponse( content=f"Error calling xAI ({type(exc).__name__}): {message}", finish_reason="error", - usage=getattr(exc, "usage", None), + usage=usage if isinstance(usage, LLMUsage) else None, retry_after=retry_after, error_status_code=int(status_code) if status_code is not None else None, error_kind=error_kind, diff --git a/tests/providers/test_oauth_model_catalog.py b/tests/providers/test_oauth_model_catalog.py index 82ebedc50..d9582e6a6 100644 --- a/tests/providers/test_oauth_model_catalog.py +++ b/tests/providers/test_oauth_model_catalog.py @@ -259,8 +259,14 @@ def test_github_copilot_catalog_only_lists_compatible_chat_models( }, }, { - "id": "responses-only", - "name": "Responses only", + "id": "gpt-5.4-mini", + "name": "GPT-5.4 Mini", + "model_picker_enabled": True, + "supported_endpoints": ["/responses"], + }, + { + "id": "unknown-responses-only", + "name": "Unknown Responses only", "model_picker_enabled": True, "supported_endpoints": ["/responses"], }, @@ -298,13 +304,22 @@ def test_github_copilot_catalog_only_lists_compatible_chat_models( catalog = get_oauth_model_catalog("github_copilot") assert catalog.source == "remote" - assert [model.id for model in catalog.models] == ["github-copilot/claude-sonnet"] + assert [model.id for model in catalog.models] == [ + "github-copilot/claude-sonnet", + "github-copilot/gpt-5.4-mini", + ] assert catalog.models[0].context_window == 200_000 assert catalog.models[0].reasoning_efforts == ("low", "high") assert len(captured) == 2 assert captured[0].headers["Authorization"] == "token github-secret" assert captured[1].headers["Authorization"] == "Bearer copilot-secret" assert str(captured[1].url) == "https://api.individual.githubcopilot.com/models" + assert get_oauth_model_catalog("github_copilot").source == "cache" + assert get_oauth_model_catalog( + "github_copilot", + proxy="http://proxy.example:8080", + ).source == "remote" + assert len(captured) == 4 def test_catalog_single_flights_concurrent_refreshes() -> None: @@ -337,28 +352,53 @@ def test_catalog_single_flights_concurrent_refreshes() -> None: def test_catalog_invalidation_discards_an_inflight_account_refresh() -> None: started = threading.Event() release = threading.Event() + identity = ["old-account"] + + def fetch(_proxy: str | None) -> tuple[ProviderModelSpec, ...]: + current = identity[0] + if current == "old-account": + started.set() + assert release.wait(timeout=2) + return (ProviderModelSpec(id=f"provider/{current}", label=current),) + + catalog = OAuthModelCatalog(fallback_models=(_fallback_model(),), fetch=fetch) + with ThreadPoolExecutor(max_workers=2) as pool: + old_future = pool.submit(catalog.get, cache_key="old-key") + assert started.wait(timeout=2) + identity[0] = "new-account" + catalog.invalidate() + new_future = pool.submit(catalog.get, cache_key="new-key") + new_result = new_future.result(timeout=2) + release.set() + old_result = old_future.result(timeout=2) + + assert old_result.source == "fallback" + assert new_result.models[0].id == "provider/new-account" + + identity[0] = "old-account" + assert catalog.get(cache_key="old-key").models[0].id == "provider/old-account" + + +def test_catalog_bounds_failure_only_keys() -> None: calls = 0 def fetch(_proxy: str | None) -> tuple[ProviderModelSpec, ...]: nonlocal calls calls += 1 - if calls == 1: - started.set() - assert release.wait(timeout=2) - return (ProviderModelSpec(id="provider/old-account", label="Old"),) - return (ProviderModelSpec(id="provider/new-account", label="New"),) + raise httpx.ConnectError("offline") - catalog = OAuthModelCatalog(fallback_models=(_fallback_model(),), fetch=fetch) - with ThreadPoolExecutor(max_workers=1) as pool: - future = pool.submit(catalog.get, cache_key="shared") - assert started.wait(timeout=2) - catalog.invalidate() - release.set() - result = future.result(timeout=2) + catalog = OAuthModelCatalog( + fallback_models=(_fallback_model(),), + fetch=fetch, + max_entries=2, + ) - assert calls == 2 - assert result.models[0].id == "provider/new-account" - assert catalog.get(cache_key="shared").models[0].id == "provider/new-account" + for key in ("one", "two", "three"): + assert catalog.get(cache_key=key).source == "fallback" + + assert calls == 3 + assert catalog.get(cache_key="one").source == "fallback" + assert calls == 4 def test_catalog_returns_stale_then_negative_caches_refresh_failure() -> None: diff --git a/tests/providers/test_xai_grok_provider.py b/tests/providers/test_xai_grok_provider.py index a3d07ae8f..2d1eff2b2 100644 --- a/tests/providers/test_xai_grok_provider.py +++ b/tests/providers/test_xai_grok_provider.py @@ -637,14 +637,16 @@ async def test_provider_recovers_unfinished_hosted_tool_once_and_preserves_usage _mock_token(monkeypatch) _mock_model_capabilities(monkeypatch, supports_backend_search=True) attempts = 0 + request_ids: list[str] = [] streamed: list[str] = [] recovered: list[bool] = [] first_usage = LLMUsage.reported(input_tokens=10, output_tokens=2) second_usage = LLMUsage.reported(input_tokens=11, output_tokens=4) - async def fake_request(_url, _headers, body, **kwargs): + async def fake_request(_url, headers, body, **kwargs): nonlocal attempts attempts += 1 + request_ids.append(headers["x-grok-req-id"]) assert body["max_turns"] == 5 if attempts == 1: await kwargs["on_content_delta"]("I will keep searching.") @@ -668,12 +670,43 @@ async def test_provider_recovers_unfinished_hosted_tool_once_and_preserves_usage ) assert attempts == 2 + assert len(set(request_ids)) == 2 assert recovered == [True] assert streamed == ["I will keep searching.", "Final researched answer."] assert response.content == "Final researched answer." assert response.usage == first_usage + second_usage +@pytest.mark.asyncio +async def test_provider_preserves_usage_when_hosted_tool_recovery_also_fails( + monkeypatch, +) -> None: + _mock_token(monkeypatch) + _mock_model_capabilities(monkeypatch, supports_backend_search=True) + attempts = 0 + usage = LLMUsage.reported(input_tokens=10, output_tokens=2) + + async def fake_request(*_args, **_kwargs): + nonlocal attempts + attempts += 1 + raise _XAIIncompleteHostedToolError( + [{"name": "x_search", "call_id": f"search-{attempts}"}], + usage=usage, + ) + + monkeypatch.setattr("nanobot.providers.xai_grok_provider._request_xai", fake_request) + provider = XAIGrokProvider() + + response = await provider.chat_stream_with_retry( + [{"role": "user", "content": "Search X"}], + on_stream_recover=lambda: _append([], True), + ) + + assert attempts == 2 + assert response.finish_reason == "error" + assert response.usage == usage + usage + + @pytest.mark.asyncio async def test_raw_response_error_preserves_bounded_redacted_body(monkeypatch) -> None: original_client = httpx.AsyncClient