fix: address non-blocking reviewer nits from #178, #179, and #157

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",

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: ...

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,

tests/unit/test_reviewer_nits.py

@@ -0,0 +1,87 @@
+# 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:
+        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")