From fe2bf2083ee9ca8f7061d51189eed78d9d933fbb Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Wed, 24 Jun 2026 14:18:19 +0530 Subject: [PATCH 1/2] fix: address non-blocking reviewer nits from #178 and #179 Add DEFAULT_MODEL and SLOT_DEFAULTS as ClassVar declarations to ModelMetadataProvider Protocol so callers no longer need type: ignore[attr-defined] when accessing provider attributes. Add validate_base_url() helper that warns on malformed (non-http/https or missing host) base URLs at model creation time, catching operator misconfigurations early without raising. --- src/skillspector/providers/base.py | 8 ++- src/skillspector/providers/chat_models.py | 30 ++++++++ tests/unit/test_reviewer_nits.py | 88 +++++++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_reviewer_nits.py diff --git a/src/skillspector/providers/base.py b/src/skillspector/providers/base.py index a18858e..48b8db6 100644 --- a/src/skillspector/providers/base.py +++ b/src/skillspector/providers/base.py @@ -17,7 +17,7 @@ from __future__ import annotations -from typing import Protocol +from typing import ClassVar, Protocol from langchain_core.language_models.chat_models import BaseChatModel @@ -31,8 +31,14 @@ class ModelMetadataProvider(Protocol): ``resolve_model`` runs the per-provider waterfall: ``SKILLSPECTOR_MODEL`` env var → provider's slot-specific default → provider's general default. Always returns a non-empty string. + + ``DEFAULT_MODEL`` is the provider's general default model label. + ``SLOT_DEFAULTS`` maps specific slots to their preferred models. """ + DEFAULT_MODEL: ClassVar[str] + SLOT_DEFAULTS: ClassVar[dict[str, str]] + def get_context_length(self, model: str) -> int | None: ... def get_max_output_tokens(self, model: str) -> int | None: ... diff --git a/src/skillspector/providers/chat_models.py b/src/skillspector/providers/chat_models.py index 7573e82..1caf59c 100644 --- a/src/skillspector/providers/chat_models.py +++ b/src/skillspector/providers/chat_models.py @@ -17,10 +17,39 @@ from __future__ import annotations +import logging +from urllib.parse import urlparse + from langchain_core.language_models.chat_models import BaseChatModel from langchain_openai import ChatOpenAI from pydantic import SecretStr +logger = logging.getLogger(__name__) + + +def validate_base_url(url: str | None) -> None: + """Warn if *url* is not a well-formed http(s) URL. + + Raises nothing — misconfigured URLs will still fail at the HTTP + layer, but an early warning helps operators catch typos. + """ + if url is None: + return + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + logger.warning( + "Provider base_url %r has scheme %r — expected http or https. " + "Requests will likely fail.", + url, + parsed.scheme or "(empty)", + ) + if not parsed.netloc: + logger.warning( + "Provider base_url %r has no host component. " + "Requests will likely fail.", + url, + ) + def create_openai_compatible_chat_model( *, @@ -35,6 +64,7 @@ def create_openai_compatible_chat_model( return None api_key, base_url = credentials + validate_base_url(base_url) return ChatOpenAI( model=model, base_url=base_url, diff --git a/tests/unit/test_reviewer_nits.py b/tests/unit/test_reviewer_nits.py new file mode 100644 index 0000000..a58d8c2 --- /dev/null +++ b/tests/unit/test_reviewer_nits.py @@ -0,0 +1,88 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for non-blocking reviewer nits from PRs #178, #179, #157.""" + +from __future__ import annotations + +import logging + +import pytest + +from skillspector.providers.base import ModelMetadataProvider +from skillspector.providers.chat_models import validate_base_url + + +class TestModelMetadataProtocolAttributes: + """ModelMetadataProvider Protocol declares DEFAULT_MODEL and SLOT_DEFAULTS.""" + + def test_protocol_declares_default_model(self) -> None: + assert hasattr(ModelMetadataProvider, "__protocol_attrs__") or True + annotations = getattr(ModelMetadataProvider, "__annotations__", {}) + assert "DEFAULT_MODEL" in annotations + + def test_protocol_declares_slot_defaults(self) -> None: + annotations = getattr(ModelMetadataProvider, "__annotations__", {}) + assert "SLOT_DEFAULTS" in annotations + + def test_existing_providers_satisfy_protocol(self) -> None: + from skillspector.providers.anthropic import AnthropicProvider + from skillspector.providers.nv_build import NvBuildProvider + from skillspector.providers.openai import OpenAIProvider + + for provider_cls in (NvBuildProvider, OpenAIProvider, AnthropicProvider): + assert hasattr(provider_cls, "DEFAULT_MODEL") + assert isinstance(provider_cls.DEFAULT_MODEL, str) + assert hasattr(provider_cls, "SLOT_DEFAULTS") + assert isinstance(provider_cls.SLOT_DEFAULTS, dict) + + +class TestValidateBaseUrl: + """validate_base_url warns on malformed URLs without raising.""" + + def test_valid_https_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("https://api.openai.com/v1") + assert len(caplog.records) == 0 + + def test_valid_http_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("http://localhost:11434/v1") + assert len(caplog.records) == 0 + + def test_none_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url(None) + assert len(caplog.records) == 0 + + def test_ftp_scheme_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("ftp://files.example.com/models") + assert any("ftp" in r.message for r in caplog.records) + + def test_empty_scheme_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("just-a-hostname:8080/v1") + assert any("(empty)" in r.message or "no host" in r.message for r in caplog.records) + + def test_no_host_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("http://") + assert any("no host" in r.message for r in caplog.records) + + def test_does_not_raise(self) -> None: + validate_base_url("not-a-url-at-all") + validate_base_url("") + validate_base_url("ftp://bad") From 0817d2d1136746a69747428a72590e6f5f3d05e4 Mon Sep 17 00:00:00 2001 From: mimran-khan Date: Wed, 24 Jun 2026 20:30:10 +0530 Subject: [PATCH 2/2] fix: remove stale type: ignore and no-op test assertion Remove unused # type: ignore[attr-defined] from constants.py now that ModelMetadataProvider declares DEFAULT_MODEL. Remove no-op `assert hasattr(...) or True` from test. --- src/skillspector/constants.py | 2 +- tests/unit/test_reviewer_nits.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/skillspector/constants.py b/src/skillspector/constants.py index 5114ebb..74c85a3 100644 --- a/src/skillspector/constants.py +++ b/src/skillspector/constants.py @@ -33,7 +33,7 @@ # Exposed for analyzers that need a final fallback symbol (e.g., # ``model = state.model or MODEL_CONFIG[ANALYZER_ID] or _SKILLSPECTOR_DEFAULT_MODEL``). -_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL # type: ignore[attr-defined] +_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL _MODEL_SLOTS: tuple[str, ...] = ( "default", diff --git a/tests/unit/test_reviewer_nits.py b/tests/unit/test_reviewer_nits.py index a58d8c2..7fcc865 100644 --- a/tests/unit/test_reviewer_nits.py +++ b/tests/unit/test_reviewer_nits.py @@ -29,7 +29,6 @@ class TestModelMetadataProtocolAttributes: """ModelMetadataProvider Protocol declares DEFAULT_MODEL and SLOT_DEFAULTS.""" def test_protocol_declares_default_model(self) -> None: - assert hasattr(ModelMetadataProvider, "__protocol_attrs__") or True annotations = getattr(ModelMetadataProvider, "__annotations__", {}) assert "DEFAULT_MODEL" in annotations