mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-06-13 22:34:06 +00:00
Address PR review comments
This commit is contained in:
parent
ba3fa38e97
commit
c849ff6eec
@ -245,8 +245,8 @@ def _resolve_env_placeholders(value: str | None) -> str | None:
|
|||||||
|
|
||||||
|
|
||||||
def _provider_requires_api_key(spec: Any) -> bool:
|
def _provider_requires_api_key(spec: Any) -> bool:
|
||||||
if spec.backend == "azure_openai":
|
if spec.name == "azure_openai":
|
||||||
return True
|
return False
|
||||||
if spec.is_oauth:
|
if spec.is_oauth:
|
||||||
return False
|
return False
|
||||||
if spec.is_local or spec.is_direct:
|
if spec.is_local or spec.is_direct:
|
||||||
@ -305,6 +305,8 @@ def _oauth_provider_status(spec: Any) -> dict[str, Any]:
|
|||||||
def _provider_configured_for_settings(spec: Any, provider_config: Any) -> bool:
|
def _provider_configured_for_settings(spec: Any, provider_config: Any) -> bool:
|
||||||
if spec.is_oauth:
|
if spec.is_oauth:
|
||||||
return bool(_oauth_provider_status(spec)["configured"])
|
return bool(_oauth_provider_status(spec)["configured"])
|
||||||
|
if spec.name == "azure_openai":
|
||||||
|
return bool(provider_config.api_base)
|
||||||
if _provider_requires_api_key(spec):
|
if _provider_requires_api_key(spec):
|
||||||
return bool(provider_config.api_key)
|
return bool(provider_config.api_key)
|
||||||
return bool(
|
return bool(
|
||||||
|
|||||||
@ -93,10 +93,57 @@ def test_init_missing_key_uses_aad_token_provider(monkeypatch):
|
|||||||
assert isinstance(provider._token_provider, _AzureTokenProvider)
|
assert isinstance(provider._token_provider, _AzureTokenProvider)
|
||||||
# DefaultAzureCredential must have been instantiated exactly once
|
# DefaultAzureCredential must have been instantiated exactly once
|
||||||
credential_factory.assert_called_once_with()
|
credential_factory.assert_called_once_with()
|
||||||
# The SDK client must have received the token provider as its api_key
|
|
||||||
# (the SDK stores it on the auth wrapper, not directly accessible — so
|
|
||||||
# we assert the callable was wired in via the provider attribute).
|
|
||||||
assert provider._token_provider._credential is credential_instance
|
assert provider._token_provider._credential is credential_instance
|
||||||
|
# The token provider must be wired into the OpenAI SDK as its
|
||||||
|
# ``_api_key_provider`` callable — that's what the SDK invokes per
|
||||||
|
# request to refresh the bearer token.
|
||||||
|
assert provider._client._api_key_provider is provider._token_provider
|
||||||
|
# Static api_key starts empty until the first refresh.
|
||||||
|
assert provider._client.api_key == ""
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_aad_token_provider_wires_into_sdk_auth_headers(monkeypatch):
|
||||||
|
"""End-to-end: SDK ``_refresh_api_key`` invokes our callable and the
|
||||||
|
resulting bearer token shows up in ``auth_headers``.
|
||||||
|
|
||||||
|
This is a regression guard against a future refactor that constructs
|
||||||
|
``_AzureTokenProvider`` but forgets to pass it to ``AsyncOpenAI`` (in
|
||||||
|
which case the outgoing request would carry no Authorization header).
|
||||||
|
"""
|
||||||
|
access_token = SimpleNamespace(token="token-A", expires_on=time.time() + 3600)
|
||||||
|
credential_instance = MagicMock()
|
||||||
|
credential_instance.get_token = AsyncMock(return_value=access_token)
|
||||||
|
credential_factory = MagicMock(return_value=credential_instance)
|
||||||
|
_install_fake_azure_identity(monkeypatch, credential_factory)
|
||||||
|
|
||||||
|
provider = AzureOpenAIProvider(
|
||||||
|
api_key="", api_base="https://res.openai.azure.com",
|
||||||
|
)
|
||||||
|
|
||||||
|
# Before any refresh, the SDK has no key yet -> no auth header.
|
||||||
|
assert provider._client.api_key == ""
|
||||||
|
assert provider._client.auth_headers == {}
|
||||||
|
|
||||||
|
# Trigger the SDK's refresh path; it must call our async callable.
|
||||||
|
refreshed = await provider._client._refresh_api_key()
|
||||||
|
|
||||||
|
assert refreshed == "token-A"
|
||||||
|
assert provider._client.api_key == "token-A"
|
||||||
|
assert provider._client.auth_headers == {"Authorization": "Bearer token-A"}
|
||||||
|
credential_instance.get_token.assert_awaited_with(
|
||||||
|
"https://cognitiveservices.azure.com/.default"
|
||||||
|
)
|
||||||
|
|
||||||
|
# A second refresh picks up a rotated token without re-instantiating
|
||||||
|
# the credential — proves we delegate per-request rather than caching
|
||||||
|
# the first value.
|
||||||
|
credential_instance.get_token = AsyncMock(
|
||||||
|
return_value=SimpleNamespace(token="token-B", expires_on=time.time() + 3600)
|
||||||
|
)
|
||||||
|
await provider._client._refresh_api_key()
|
||||||
|
assert provider._client.auth_headers == {"Authorization": "Bearer token-B"}
|
||||||
|
credential_factory.assert_called_once_with()
|
||||||
|
|
||||||
|
|
||||||
def test_init_explicit_key_does_not_construct_credential(monkeypatch):
|
def test_init_explicit_key_does_not_construct_credential(monkeypatch):
|
||||||
|
|||||||
@ -7,6 +7,7 @@ import pytest
|
|||||||
|
|
||||||
from nanobot.config.loader import load_config, save_config
|
from nanobot.config.loader import load_config, save_config
|
||||||
from nanobot.config.schema import Config, ModelPresetConfig
|
from nanobot.config.schema import Config, ModelPresetConfig
|
||||||
|
from nanobot.providers.registry import find_by_name
|
||||||
from nanobot.webui.settings_api import (
|
from nanobot.webui.settings_api import (
|
||||||
WebUISettingsError,
|
WebUISettingsError,
|
||||||
_oauth_provider_status,
|
_oauth_provider_status,
|
||||||
@ -17,7 +18,6 @@ from nanobot.webui.settings_api import (
|
|||||||
update_model_configuration,
|
update_model_configuration,
|
||||||
update_network_safety_settings,
|
update_network_safety_settings,
|
||||||
)
|
)
|
||||||
from nanobot.providers.registry import find_by_name
|
|
||||||
|
|
||||||
|
|
||||||
def test_create_model_configuration_writes_label_and_selects(
|
def test_create_model_configuration_writes_label_and_selects(
|
||||||
@ -461,3 +461,119 @@ def test_create_model_configuration_accepts_configured_oauth_provider(
|
|||||||
assert payload["agent"]["model_preset"] == "codex"
|
assert payload["agent"]["model_preset"] == "codex"
|
||||||
saved = load_config(config_path)
|
saved = load_config(config_path)
|
||||||
assert saved.model_presets["codex"].provider == "openai_codex"
|
assert saved.model_presets["codex"].provider == "openai_codex"
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Azure OpenAI: settings contract for static-key vs AAD (DefaultAzureCredential)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_settings_payload_azure_openai_with_api_key_is_configured(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""Static-key mode: api_key + api_base both set -> configured."""
|
||||||
|
config_path = tmp_path / "config.json"
|
||||||
|
config = Config()
|
||||||
|
config.providers.azure_openai.api_key = "k"
|
||||||
|
config.providers.azure_openai.api_base = "https://r.openai.azure.com"
|
||||||
|
save_config(config, config_path)
|
||||||
|
monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path)
|
||||||
|
|
||||||
|
payload = settings_payload()
|
||||||
|
azure = next(row for row in payload["providers"] if row["name"] == "azure_openai")
|
||||||
|
|
||||||
|
assert azure["configured"] is True
|
||||||
|
assert azure["api_key_required"] is False
|
||||||
|
assert azure["auth_type"] == "api_key"
|
||||||
|
assert azure["api_base"] == "https://r.openai.azure.com"
|
||||||
|
|
||||||
|
|
||||||
|
def test_settings_payload_azure_openai_aad_mode_is_configured(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""AAD mode: only api_base set (no api_key) -> still configured."""
|
||||||
|
config_path = tmp_path / "config.json"
|
||||||
|
config = Config()
|
||||||
|
config.providers.azure_openai.api_base = "https://r.openai.azure.com"
|
||||||
|
save_config(config, config_path)
|
||||||
|
monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path)
|
||||||
|
|
||||||
|
payload = settings_payload()
|
||||||
|
azure = next(row for row in payload["providers"] if row["name"] == "azure_openai")
|
||||||
|
|
||||||
|
assert azure["configured"] is True
|
||||||
|
assert azure["api_key_required"] is False
|
||||||
|
assert azure["api_base"] == "https://r.openai.azure.com"
|
||||||
|
assert azure["api_key_hint"] is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_settings_payload_azure_openai_missing_base_not_configured(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""api_key alone (no api_base) is NOT a working config -> not configured."""
|
||||||
|
config_path = tmp_path / "config.json"
|
||||||
|
config = Config()
|
||||||
|
config.providers.azure_openai.api_key = "k"
|
||||||
|
save_config(config, config_path)
|
||||||
|
monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path)
|
||||||
|
|
||||||
|
payload = settings_payload()
|
||||||
|
azure = next(row for row in payload["providers"] if row["name"] == "azure_openai")
|
||||||
|
|
||||||
|
assert azure["configured"] is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_create_model_configuration_accepts_azure_openai_aad_mode(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""Provider-validation accepts azure_openai with only api_base (AAD mode)."""
|
||||||
|
config_path = tmp_path / "config.json"
|
||||||
|
config = Config()
|
||||||
|
config.providers.azure_openai.api_base = "https://r.openai.azure.com"
|
||||||
|
save_config(config, config_path)
|
||||||
|
monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path)
|
||||||
|
|
||||||
|
payload = create_model_configuration(
|
||||||
|
{
|
||||||
|
"label": ["Azure AAD"],
|
||||||
|
"provider": ["azure_openai"],
|
||||||
|
"model": ["my-deployment"],
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
assert payload["agent"]["model_preset"] == "azure-aad"
|
||||||
|
saved = load_config(config_path)
|
||||||
|
assert saved.model_presets["azure-aad"].provider == "azure_openai"
|
||||||
|
assert saved.model_presets["azure-aad"].model == "my-deployment"
|
||||||
|
|
||||||
|
|
||||||
|
def test_create_model_configuration_rejects_azure_openai_without_base(
|
||||||
|
tmp_path,
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""azure_openai without api_base must still be rejected as not configured."""
|
||||||
|
config_path = tmp_path / "config.json"
|
||||||
|
save_config(Config(), config_path)
|
||||||
|
monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path)
|
||||||
|
|
||||||
|
with pytest.raises(WebUISettingsError, match="provider is not configured"):
|
||||||
|
create_model_configuration(
|
||||||
|
{
|
||||||
|
"label": ["Azure"],
|
||||||
|
"provider": ["azure_openai"],
|
||||||
|
"model": ["my-deployment"],
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_azure_openai_spec_no_longer_requires_api_key() -> None:
|
||||||
|
"""Contract guard: api_key is optional for azure_openai (AAD fallback)."""
|
||||||
|
from nanobot.webui.settings_api import _provider_requires_api_key
|
||||||
|
|
||||||
|
spec = find_by_name("azure_openai")
|
||||||
|
assert spec is not None
|
||||||
|
assert _provider_requires_api_key(spec) is False
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user