Skip to content

Commit 496f0cb

Browse files
committed
fix(provider): align OpenAI fallback model config
Signed-off-by: King Star <mcxin.y@gmail.com>
1 parent 3f11bfa commit 496f0cb

6 files changed

Lines changed: 151 additions & 4 deletions

File tree

src/skillspector/constants.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
import logging
1919
import os
2020

21-
from skillspector.providers import get_metadata_provider
21+
from skillspector.providers import get_metadata_provider, get_model_config_provider
2222

2323
logger = logging.getLogger(__name__)
2424

@@ -68,8 +68,8 @@ def _resolve_slot_model(slot: str, provider=None) -> str:
6868

6969

7070
def build_model_config() -> dict[str, str]:
71-
"""Resolve the model map for the currently active provider."""
72-
provider = get_metadata_provider()
71+
"""Resolve the model map for the provider that will build chat models."""
72+
provider = get_model_config_provider()
7373
return {slot: _resolve_slot_model(slot, provider) for slot in _MODEL_SLOTS}
7474

7575

src/skillspector/providers/__init__.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,31 @@ def resolve_chat_model_credentials() -> tuple[str, str | None] | None:
197197
return _openai_fallback_provider().resolve_credentials()
198198

199199

200+
def get_model_config_provider() -> ModelMetadataProvider:
201+
"""Return the provider whose model defaults match graph chat-model routing.
202+
203+
Explicit bindings, CLI providers, and Bedrock's native AWS credential path
204+
remain authoritative. Unbound API-key providers use OpenAI metadata only
205+
when their own credentials are absent and the OpenAI fallback is configured.
206+
"""
207+
provider = _select_active_provider()
208+
from .bedrock import BedrockProvider
209+
210+
if (
211+
has_provider_binding()
212+
or has_cli_capability(provider)
213+
or isinstance(provider, BedrockProvider)
214+
):
215+
return provider
216+
if provider.resolve_credentials() is not None:
217+
return provider
218+
219+
fallback = _openai_fallback_provider()
220+
if fallback.resolve_credentials() is not None:
221+
return fallback
222+
return provider
223+
224+
200225
def create_chat_model(
201226
model: str,
202227
*,
@@ -248,6 +273,7 @@ def create_chat_model(
248273
"NO_LLM_API_KEY_MESSAGE",
249274
"create_chat_model",
250275
"get_active_provider",
276+
"get_model_config_provider",
251277
"get_metadata_provider",
252278
"has_cli_capability",
253279
"has_provider_binding",

tests/nodes/test_build_context.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
from skillspector.constants import MODEL_CONFIG
2929
from skillspector.nodes.build_context import build_context
3030
from skillspector.providers import reset_provider, use_provider
31+
from skillspector.providers.openai import OpenAIProvider
3132
from skillspector.state import SkillspectorState
3233

3334

@@ -133,7 +134,9 @@ def test_build_context_empty_directory_is_valid_empty_scan(tmp_path: Path) -> No
133134
assert result["model_config"] == MODEL_CONFIG
134135

135136

136-
def test_build_context_model_config_uses_bound_provider(tmp_path: Path) -> None:
137+
def test_build_context_model_config_uses_bound_provider(
138+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
139+
) -> None:
137140
class _BoundProvider:
138141
DEFAULT_MODEL = "bound-default"
139142
SLOT_DEFAULTS = {"meta_analyzer": "bound-meta"}
@@ -153,6 +156,7 @@ def resolve_credentials(self) -> tuple[str, str | None] | None:
153156
def create_chat_model(self, model: str, *, max_tokens: int, timeout: float | None = 120):
154157
return object()
155158

159+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-fallback-must-not-win")
156160
token = use_provider(_BoundProvider())
157161
try:
158162
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
163167
assert result["model_config"]["meta_analyzer"] == "bound-meta"
164168

165169

170+
def test_build_context_model_config_matches_openai_fallback(
171+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
172+
) -> None:
173+
for key in (
174+
"SKILLSPECTOR_PROVIDER",
175+
"SKILLSPECTOR_MODEL",
176+
"NVIDIA_INFERENCE_KEY",
177+
"NVIDIA_INFERENCE_METADATA_KEY",
178+
):
179+
monkeypatch.delenv(key, raising=False)
180+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only")
181+
182+
result = build_context({"skill_path": str(tmp_path)})
183+
184+
assert result["model_config"]["default"] == OpenAIProvider.DEFAULT_MODEL
185+
186+
166187
def test_build_context_skips_skip_dirs(tmp_path: Path) -> None:
167188
"""Skip dirs like __pycache__ and node_modules are not included in components."""
168189
_make_skill_spec_dir(tmp_path)

tests/unit/test_constants.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@
2323
import pytest
2424

2525
from skillspector.providers import registry
26+
from skillspector.providers.bedrock import BedrockProvider
27+
from skillspector.providers.codex_cli import CodexCLIProvider
28+
from skillspector.providers.nv_build import NvBuildProvider
29+
from skillspector.providers.openai import OpenAIProvider
2630

2731

2832
@pytest.fixture(autouse=True)
@@ -96,6 +100,53 @@ def test_whitespace_only_slot_env_is_ignored(self, monkeypatch: pytest.MonkeyPat
96100
# Whitespace-only treated as unset — falls through to provider.
97101
assert mod.MODEL_CONFIG["meta_analyzer"] != " "
98102

103+
def test_openai_fallback_uses_openai_defaults(self, monkeypatch: pytest.MonkeyPatch) -> None:
104+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only")
105+
106+
config = _reload_constants().build_model_config()
107+
108+
provider = OpenAIProvider()
109+
assert config["default"] == provider.resolve_model()
110+
assert config["meta_analyzer"] == provider.resolve_model("meta_analyzer")
111+
112+
def test_slot_override_wins_over_openai_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None:
113+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only")
114+
monkeypatch.setenv("SKILLSPECTOR_MODEL_META_ANALYZER", "custom/meta-model")
115+
116+
config = _reload_constants().build_model_config()
117+
118+
assert config["default"] == OpenAIProvider.DEFAULT_MODEL
119+
assert config["meta_analyzer"] == "custom/meta-model"
120+
121+
def test_configured_provider_precedes_openai_fallback(
122+
self, monkeypatch: pytest.MonkeyPatch
123+
) -> None:
124+
monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "nv_build")
125+
monkeypatch.setenv("NVIDIA_INFERENCE_KEY", "nvapi-test")
126+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai")
127+
128+
config = _reload_constants().build_model_config()
129+
130+
assert config["default"] == NvBuildProvider.DEFAULT_MODEL
131+
132+
def test_cli_provider_precedes_openai_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None:
133+
monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "codex_cli")
134+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai")
135+
136+
config = _reload_constants().build_model_config()
137+
138+
assert config["default"] == CodexCLIProvider.DEFAULT_MODEL
139+
140+
def test_bedrock_native_auth_precedes_openai_fallback(
141+
self, monkeypatch: pytest.MonkeyPatch
142+
) -> None:
143+
monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "bedrock")
144+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai")
145+
146+
config = _reload_constants().build_model_config()
147+
148+
assert config["default"] == BedrockProvider.DEFAULT_MODEL
149+
99150

100151
class TestModelValidation:
101152
"""_validate_model_config warns or raises on unknown model IDs."""

tests/unit/test_llm_utils.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
from pydantic import BaseModel
3232

3333
from skillspector import llm_utils
34+
from skillspector.constants import build_model_config
3435
from skillspector.llm_utils import (
3536
AgentCLIChatModel,
3637
_extract_json_object,
@@ -456,6 +457,17 @@ def test_openai_fallback_uses_openai_default_model(
456457

457458
assert _chat_model_name(llm) == OpenAIProvider.DEFAULT_MODEL
458459

460+
def test_graph_model_config_matches_openai_fallback_client(
461+
self, monkeypatch: pytest.MonkeyPatch
462+
) -> None:
463+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only")
464+
465+
model = build_model_config()["default"]
466+
llm = get_chat_model(model=model)
467+
468+
assert model == OpenAIProvider.DEFAULT_MODEL
469+
assert _chat_model_name(llm) == OpenAIProvider.DEFAULT_MODEL
470+
459471
def test_explicit_model_still_overrides_openai_fallback(
460472
self, monkeypatch: pytest.MonkeyPatch
461473
) -> None:

tests/unit/test_mcp_server.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424

2525
from skillspector import mcp_server
2626
from skillspector.mcp_server import run_scan
27+
from skillspector.nodes.build_context import build_context
2728
from skillspector.providers import reset_provider, use_provider
29+
from skillspector.providers.openai import OpenAIProvider
2830

2931

3032
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(
8082
assert result["scan_mode"] == "static-only"
8183

8284

85+
async def test_run_scan_openai_fallback_builds_matching_graph_model_config(
86+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
87+
) -> None:
88+
for key in (
89+
"SKILLSPECTOR_PROVIDER",
90+
"SKILLSPECTOR_MODEL",
91+
"NVIDIA_INFERENCE_KEY",
92+
"NVIDIA_INFERENCE_METADATA_KEY",
93+
):
94+
monkeypatch.delenv(key, raising=False)
95+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test-openai-only")
96+
monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (True, None))
97+
_write_skill(tmp_path)
98+
captured: dict[str, str] = {}
99+
100+
class _Graph:
101+
async def ainvoke(self, state, config):
102+
context = build_context({"skill_path": state["input_path"]})
103+
captured.update(context["model_config"])
104+
return {
105+
"filtered_findings": [],
106+
"risk_score": 0,
107+
"risk_severity": "LOW",
108+
"risk_recommendation": "OK",
109+
"report_body": "report",
110+
}
111+
112+
monkeypatch.setattr(mcp_server, "graph", _Graph())
113+
114+
result = await run_scan(str(tmp_path), use_llm=True, output_format="json")
115+
116+
assert result["llm_used"] is True
117+
assert captured["default"] == OpenAIProvider.DEFAULT_MODEL
118+
119+
83120
async def test_run_scan_uses_bound_provider_without_credentials(
84121
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
85122
) -> None:

0 commit comments

Comments
 (0)