From 496f0cb43a1a7eca4b433f8ce51abcbde67bbc57 Mon Sep 17 00:00:00 2001 From: King Star Date: Fri, 31 Jul 2026 18:55:00 +0800 Subject: [PATCH] fix(provider): align OpenAI fallback model config Signed-off-by: King Star --- src/skillspector/constants.py | 6 +-- src/skillspector/providers/__init__.py | 26 +++++++++++++ tests/nodes/test_build_context.py | 23 +++++++++++- tests/unit/test_constants.py | 51 ++++++++++++++++++++++++++ tests/unit/test_llm_utils.py | 12 ++++++ tests/unit/test_mcp_server.py | 37 +++++++++++++++++++ 6 files changed, 151 insertions(+), 4 deletions(-) diff --git a/src/skillspector/constants.py b/src/skillspector/constants.py index eae0ee52..e73c24d2 100644 --- a/src/skillspector/constants.py +++ b/src/skillspector/constants.py @@ -18,7 +18,7 @@ import logging import os -from skillspector.providers import get_metadata_provider +from skillspector.providers import get_metadata_provider, get_model_config_provider logger = logging.getLogger(__name__) @@ -68,8 +68,8 @@ def _resolve_slot_model(slot: str, provider=None) -> str: def build_model_config() -> dict[str, str]: - """Resolve the model map for the currently active provider.""" - provider = get_metadata_provider() + """Resolve the model map for the provider that will build chat models.""" + provider = get_model_config_provider() return {slot: _resolve_slot_model(slot, provider) for slot in _MODEL_SLOTS} diff --git a/src/skillspector/providers/__init__.py b/src/skillspector/providers/__init__.py index a4c0d709..57e2512e 100644 --- a/src/skillspector/providers/__init__.py +++ b/src/skillspector/providers/__init__.py @@ -197,6 +197,31 @@ def resolve_chat_model_credentials() -> tuple[str, str | None] | None: return _openai_fallback_provider().resolve_credentials() +def get_model_config_provider() -> ModelMetadataProvider: + """Return the provider whose model defaults match graph chat-model routing. + + Explicit bindings, CLI providers, and Bedrock's native AWS credential path + remain authoritative. Unbound API-key providers use OpenAI metadata only + when their own credentials are absent and the OpenAI fallback is configured. + """ + provider = _select_active_provider() + from .bedrock import BedrockProvider + + if ( + has_provider_binding() + or has_cli_capability(provider) + or isinstance(provider, BedrockProvider) + ): + return provider + if provider.resolve_credentials() is not None: + return provider + + fallback = _openai_fallback_provider() + if fallback.resolve_credentials() is not None: + return fallback + return provider + + def create_chat_model( model: str, *, @@ -248,6 +273,7 @@ def create_chat_model( "NO_LLM_API_KEY_MESSAGE", "create_chat_model", "get_active_provider", + "get_model_config_provider", "get_metadata_provider", "has_cli_capability", "has_provider_binding", diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index 1a267720..759c84f3 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -28,6 +28,7 @@ from skillspector.constants import MODEL_CONFIG from skillspector.nodes.build_context import build_context from skillspector.providers import reset_provider, use_provider +from skillspector.providers.openai import OpenAIProvider from skillspector.state import SkillspectorState @@ -133,7 +134,9 @@ def test_build_context_empty_directory_is_valid_empty_scan(tmp_path: Path) -> No assert result["model_config"] == MODEL_CONFIG -def test_build_context_model_config_uses_bound_provider(tmp_path: Path) -> None: +def test_build_context_model_config_uses_bound_provider( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: class _BoundProvider: DEFAULT_MODEL = "bound-default" SLOT_DEFAULTS = {"meta_analyzer": "bound-meta"} @@ -153,6 +156,7 @@ def resolve_credentials(self) -> tuple[str, str | None] | None: def create_chat_model(self, model: str, *, max_tokens: int, timeout: float | None = 120): return object() + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-fallback-must-not-win") token = use_provider(_BoundProvider()) try: result = build_context({"skill_path": str(tmp_path)}) @@ -163,6 +167,23 @@ def create_chat_model(self, model: str, *, max_tokens: int, timeout: float | Non assert result["model_config"]["meta_analyzer"] == "bound-meta" +def test_build_context_model_config_matches_openai_fallback( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + for key in ( + "SKILLSPECTOR_PROVIDER", + "SKILLSPECTOR_MODEL", + "NVIDIA_INFERENCE_KEY", + "NVIDIA_INFERENCE_METADATA_KEY", + ): + monkeypatch.delenv(key, raising=False) + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only") + + result = build_context({"skill_path": str(tmp_path)}) + + assert result["model_config"]["default"] == OpenAIProvider.DEFAULT_MODEL + + def test_build_context_skips_skip_dirs(tmp_path: Path) -> None: """Skip dirs like __pycache__ and node_modules are not included in components.""" _make_skill_spec_dir(tmp_path) diff --git a/tests/unit/test_constants.py b/tests/unit/test_constants.py index 6cfdabc6..bf0a2ba9 100644 --- a/tests/unit/test_constants.py +++ b/tests/unit/test_constants.py @@ -23,6 +23,10 @@ import pytest from skillspector.providers import registry +from skillspector.providers.bedrock import BedrockProvider +from skillspector.providers.codex_cli import CodexCLIProvider +from skillspector.providers.nv_build import NvBuildProvider +from skillspector.providers.openai import OpenAIProvider @pytest.fixture(autouse=True) @@ -96,6 +100,53 @@ def test_whitespace_only_slot_env_is_ignored(self, monkeypatch: pytest.MonkeyPat # Whitespace-only treated as unset — falls through to provider. assert mod.MODEL_CONFIG["meta_analyzer"] != " " + def test_openai_fallback_uses_openai_defaults(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only") + + config = _reload_constants().build_model_config() + + provider = OpenAIProvider() + assert config["default"] == provider.resolve_model() + assert config["meta_analyzer"] == provider.resolve_model("meta_analyzer") + + def test_slot_override_wins_over_openai_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only") + monkeypatch.setenv("SKILLSPECTOR_MODEL_META_ANALYZER", "custom/meta-model") + + config = _reload_constants().build_model_config() + + assert config["default"] == OpenAIProvider.DEFAULT_MODEL + assert config["meta_analyzer"] == "custom/meta-model" + + def test_configured_provider_precedes_openai_fallback( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "nv_build") + monkeypatch.setenv("NVIDIA_INFERENCE_KEY", "nvapi-test") + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai") + + config = _reload_constants().build_model_config() + + assert config["default"] == NvBuildProvider.DEFAULT_MODEL + + def test_cli_provider_precedes_openai_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "codex_cli") + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai") + + config = _reload_constants().build_model_config() + + assert config["default"] == CodexCLIProvider.DEFAULT_MODEL + + def test_bedrock_native_auth_precedes_openai_fallback( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "bedrock") + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai") + + config = _reload_constants().build_model_config() + + assert config["default"] == BedrockProvider.DEFAULT_MODEL + class TestModelValidation: """_validate_model_config warns or raises on unknown model IDs.""" diff --git a/tests/unit/test_llm_utils.py b/tests/unit/test_llm_utils.py index 92609337..c2e57c16 100644 --- a/tests/unit/test_llm_utils.py +++ b/tests/unit/test_llm_utils.py @@ -31,6 +31,7 @@ from pydantic import BaseModel from skillspector import llm_utils +from skillspector.constants import build_model_config from skillspector.llm_utils import ( AgentCLIChatModel, _extract_json_object, @@ -456,6 +457,17 @@ def test_openai_fallback_uses_openai_default_model( assert _chat_model_name(llm) == OpenAIProvider.DEFAULT_MODEL + def test_graph_model_config_matches_openai_fallback_client( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only") + + model = build_model_config()["default"] + llm = get_chat_model(model=model) + + assert model == OpenAIProvider.DEFAULT_MODEL + assert _chat_model_name(llm) == OpenAIProvider.DEFAULT_MODEL + def test_explicit_model_still_overrides_openai_fallback( self, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 12149095..9cac8a72 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -24,7 +24,9 @@ from skillspector import mcp_server from skillspector.mcp_server import run_scan +from skillspector.nodes.build_context import build_context from skillspector.providers import reset_provider, use_provider +from skillspector.providers.openai import OpenAIProvider def _write_skill(tmp_path: Path, body: str = "# Safe skill") -> Path: @@ -80,6 +82,41 @@ async def test_run_scan_reports_llm_available_with_credentials( assert result["scan_mode"] == "static-only" +async def test_run_scan_openai_fallback_builds_matching_graph_model_config( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + for key in ( + "SKILLSPECTOR_PROVIDER", + "SKILLSPECTOR_MODEL", + "NVIDIA_INFERENCE_KEY", + "NVIDIA_INFERENCE_METADATA_KEY", + ): + monkeypatch.delenv(key, raising=False) + monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only") + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (True, None)) + _write_skill(tmp_path) + captured: dict[str, str] = {} + + class _Graph: + async def ainvoke(self, state, config): + context = build_context({"skill_path": state["input_path"]}) + captured.update(context["model_config"]) + return { + "filtered_findings": [], + "risk_score": 0, + "risk_severity": "LOW", + "risk_recommendation": "OK", + "report_body": "report", + } + + monkeypatch.setattr(mcp_server, "graph", _Graph()) + + result = await run_scan(str(tmp_path), use_llm=True, output_format="json") + + assert result["llm_used"] is True + assert captured["default"] == OpenAIProvider.DEFAULT_MODEL + + async def test_run_scan_uses_bound_provider_without_credentials( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: