diff --git a/src/skillspector/constants.py b/src/skillspector/constants.py index 874841c..375992c 100644 --- a/src/skillspector/constants.py +++ b/src/skillspector/constants.py @@ -36,7 +36,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/src/skillspector/nodes/analyzers/mcp_rug_pull.py b/src/skillspector/nodes/analyzers/mcp_rug_pull.py index 09dd9b5..8d2bd6d 100644 --- a/src/skillspector/nodes/analyzers/mcp_rug_pull.py +++ b/src/skillspector/nodes/analyzers/mcp_rug_pull.py @@ -61,7 +61,10 @@ # RP2: Manifest-permission pre-staging _PERMISSION_EXPANSION_PATTERNS = [ (r'"permissions?"\s*:\s*\[[^\]]*\]', 0.60), - (r"(?:add|grant|request|require)\s+(?:new|additional|extra|more)\s+(?:permissions?|tools?|access)", 0.70), + ( + r"(?:add|grant|request|require)\s+(?:new|additional|extra|more)\s+(?:permissions?|tools?|access)", + 0.70, + ), ] @@ -121,7 +124,7 @@ def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: line_end = content.find("\n", m.end()) if line_end == -1: line_end = len(content) - line_remainder = content[m.end():line_end] + line_remainder = content[m.end() : line_end] if _VERSION_PIN_RE.search(full_match + line_remainder): continue line_num = _find_line(content, m.start()) @@ -151,7 +154,7 @@ def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: line_end = content.find("\n", m.end()) if line_end == -1: line_end = len(content) - line_remainder = content[m.end():line_end] + line_remainder = content[m.end() : line_end] if _VERSION_PIN_RE.search(full_match + line_remainder): continue line_num = _find_line(content, m.start()) @@ -167,8 +170,7 @@ def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: tags=list(_TAGS), matched_text=full_match[:200], explanation=( - "uvx/uv tool run commands without ==version create " - "a rug-pull risk." + "uvx/uv tool run commands without ==version create a rug-pull risk." ), remediation="Pin the version: uvx package-name==1.2.3", ) @@ -180,7 +182,7 @@ def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: line_end = content.find("\n", m.end()) if line_end == -1: line_end = len(content) - line_remainder = content[m.end():line_end] + line_remainder = content[m.end() : line_end] if _VERSION_PIN_RE.search(full_match + line_remainder): continue pkg = m.group(1) @@ -239,8 +241,7 @@ def _check_rp1(manifest: dict, file_cache: dict[str, str]) -> list[Finding]: Finding( rule_id="RP1", message=( - f"Manifest references MCP server without version pin: " - f"'{m.group(0).strip()}'." + f"Manifest references MCP server without version pin: '{m.group(0).strip()}'." ), severity="MEDIUM", confidence=0.70, @@ -463,7 +464,9 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: prev_prop = prev_params[name] prop_diffs = [] if curr_prop["type"] != prev_prop["type"]: - prop_diffs.append(f"type changed from {prev_prop['type']} to {curr_prop['type']}") + prop_diffs.append( + f"type changed from {prev_prop['type']} to {curr_prop['type']}" + ) if curr_prop["default"] != prev_prop["default"]: prop_diffs.append( f"default changed from {prev_prop['default']} to {curr_prop['default']}" @@ -478,7 +481,9 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: if added_params: changes.append(f"added: {', '.join(curr_params[p]['name'] for p in added_params)}") if removed_params: - changes.append(f"removed: {', '.join(prev_params[p]['name'] for p in removed_params)}") + changes.append( + f"removed: {', '.join(prev_params[p]['name'] for p in removed_params)}" + ) if changed_params: changes.append(f"modified: {', '.join(changed_params)}") diff --git a/src/skillspector/nodes/report.py b/src/skillspector/nodes/report.py index 0610356..6295e12 100644 --- a/src/skillspector/nodes/report.py +++ b/src/skillspector/nodes/report.py @@ -681,4 +681,4 @@ def report(state: SkillspectorState) -> dict[str, object]: "filtered_findings": filtered_findings, "suppressed_findings": suppressed, } - return out \ No newline at end of file + return out 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..5ce78e0 100644 --- a/src/skillspector/providers/chat_models.py +++ b/src/skillspector/providers/chat_models.py @@ -17,10 +17,38 @@ 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 +63,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/nodes/test_meta_analyzer_fallback.py b/tests/nodes/test_meta_analyzer_fallback.py index 17cbd4c..d67bf7a 100644 --- a/tests/nodes/test_meta_analyzer_fallback.py +++ b/tests/nodes/test_meta_analyzer_fallback.py @@ -98,7 +98,6 @@ def test_low_severity_below_threshold_still_dropped(self) -> None: result = _fallback_filtered(findings) assert len(result) == 0 - def test_none_severity_treated_as_low(self) -> None: """Finding with None severity does not crash — treated as LOW.""" findings = [_finding(confidence=0.8, severity=None)] diff --git a/tests/nodes/test_report.py b/tests/nodes/test_report.py index 2f1b895..71445d6 100644 --- a/tests/nodes/test_report.py +++ b/tests/nodes/test_report.py @@ -643,6 +643,7 @@ def test_report_no_baseline_unchanged() -> None: assert result["risk_score"] == 50 assert result["suppressed_findings"] == [] + def test_report_executable_scripts_multiplier() -> None: """1.3x multiplier applied only to findings from executable files.""" # 2 HIGH findings in run.py = 2 × 25 × 1.3 = 65 (float-based accumulation) @@ -674,8 +675,14 @@ def test_report_doc_findings_no_multiplier() -> None: _finding("P2", "HIGH", file="SKILL.md"), ], "component_metadata": [ - {"path": "SKILL.md", "type": "markdown", "lines": 10, "executable": False, "size_bytes": 500}, - {"path": "run.py", "type": "python", "lines": 5, "executable": True, "size_bytes": 200} + { + "path": "SKILL.md", + "type": "markdown", + "lines": 10, + "executable": False, + "size_bytes": 500, + }, + {"path": "run.py", "type": "python", "lines": 5, "executable": True, "size_bytes": 200}, ], "has_executable_scripts": True, "manifest": {}, diff --git a/tests/test_mcp_rug_pull.py b/tests/test_mcp_rug_pull.py index a95e045..1d67ee1 100644 --- a/tests/test_mcp_rug_pull.py +++ b/tests/test_mcp_rug_pull.py @@ -21,7 +21,9 @@ from skillspector.state import SkillspectorState -def _state(manifest: dict | None = None, file_cache: dict[str, str] | None = None) -> SkillspectorState: +def _state( + manifest: dict | None = None, file_cache: dict[str, str] | None = None +) -> SkillspectorState: state: SkillspectorState = {} if manifest is not None: state["manifest"] = manifest @@ -32,10 +34,12 @@ def _state(manifest: dict | None = None, file_cache: dict[str, str] | None = Non def test_rp1_npx_unpinned(): """RP1 detects npx without @version suffix.""" - result = node(_state( - manifest={"name": "test-skill"}, - file_cache={"setup.sh": "npx @scope/mcp-server\n"}, - )) + result = node( + _state( + manifest={"name": "test-skill"}, + file_cache={"setup.sh": "npx @scope/mcp-server\n"}, + ) + ) rp1 = [f for f in result["findings"] if f.rule_id == "RP1"] assert len(rp1) == 1 assert "npx @scope/mcp-server" in rp1[0].matched_text @@ -43,20 +47,24 @@ def test_rp1_npx_unpinned(): def test_rp1_npx_pinned_no_finding(): """RP1 does not fire when npx has @version.""" - result = node(_state( - manifest={"name": "test-skill"}, - file_cache={"setup.sh": "npx @scope/mcp-server@1.2.3\n"}, - )) + result = node( + _state( + manifest={"name": "test-skill"}, + file_cache={"setup.sh": "npx @scope/mcp-server@1.2.3\n"}, + ) + ) rp1 = [f for f in result["findings"] if f.rule_id == "RP1"] assert len(rp1) == 0 def test_rp1_uvx_unpinned(): """RP1 detects uvx without ==version.""" - result = node(_state( - manifest={"name": "test-skill"}, - file_cache={"install.sh": "uvx my-mcp-server\n"}, - )) + result = node( + _state( + manifest={"name": "test-skill"}, + file_cache={"install.sh": "uvx my-mcp-server\n"}, + ) + ) rp1 = [f for f in result["findings"] if f.rule_id == "RP1"] assert len(rp1) >= 1 assert any("uvx" in f.matched_text for f in rp1) @@ -64,46 +72,56 @@ def test_rp1_uvx_unpinned(): def test_rp1_docker_unpinned(): """RP1 detects docker run without tag.""" - result = node(_state( - manifest={"name": "test-skill"}, - file_cache={"Dockerfile": "FROM org/mcp-server\n"}, - )) + node( + _state( + manifest={"name": "test-skill"}, + file_cache={"Dockerfile": "FROM org/mcp-server\n"}, + ) + ) # RP1 docker pattern matches "docker pull|run|create" # FROM in Dockerfile isn't matched by our regex, so update test - result2 = node(_state( - manifest={"name": "test-skill"}, - file_cache={"setup.sh": "docker run org/mcp-server\n"}, - )) + result2 = node( + _state( + manifest={"name": "test-skill"}, + file_cache={"setup.sh": "docker run org/mcp-server\n"}, + ) + ) rp1 = [f for f in result2["findings"] if f.rule_id == "RP1"] assert len(rp1) >= 1 def test_rp1_multiple_patterns(): """Multiple unpinned references produce multiple RP1 findings.""" - result = node(_state( - manifest={"name": "test-skill"}, - file_cache={ - "setup.sh": "npx @scope/server-a\nnpx @org/server-b\n", - }, - )) + result = node( + _state( + manifest={"name": "test-skill"}, + file_cache={ + "setup.sh": "npx @scope/server-a\nnpx @org/server-b\n", + }, + ) + ) rp1 = [f for f in result["findings"] if f.rule_id == "RP1"] assert len(rp1) == 2 def test_rp3_version_wildcard(): """RP3 detects wildcard version.""" - result = node(_state( - manifest={"version": "*", "name": "test"}, - )) + result = node( + _state( + manifest={"version": "*", "name": "test"}, + ) + ) rp3 = [f for f in result["findings"] if f.rule_id == "RP3"] assert len(rp3) >= 1 def test_rp3_version_ok_no_finding(): """RP3 does not fire on pinned version.""" - result = node(_state( - manifest={"version": "1.2.3", "name": "test"}, - )) + result = node( + _state( + manifest={"version": "1.2.3", "name": "test"}, + ) + ) rp3 = [f for f in result["findings"] if f.rule_id == "RP3"] assert len(rp3) == 0 diff --git a/tests/unit/test_constants.py b/tests/unit/test_constants.py index 9eee0a0..7f2789a 100644 --- a/tests/unit/test_constants.py +++ b/tests/unit/test_constants.py @@ -80,9 +80,7 @@ def test_unset_slot_env_falls_through_to_provider(self) -> None: # Without any slot override, the provider's resolve_model() runs. assert mod.MODEL_CONFIG["default"] == mod._SKILLSPECTOR_DEFAULT_MODEL - def test_multiple_slots_independently_overridden( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: + def test_multiple_slots_independently_overridden(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("SKILLSPECTOR_MODEL_META_ANALYZER", "model-a") monkeypatch.setenv("SKILLSPECTOR_MODEL_SEMANTIC_DEVELOPER_INTENT", "model-b") mod = _reload_constants() @@ -118,9 +116,7 @@ def test_known_model_no_warning( with caplog.at_level(logging.WARNING, logger="skillspector.constants"): mod = _reload_constants() default_model = mod._SKILLSPECTOR_DEFAULT_MODEL - warnings_for_default = [ - r for r in caplog.records if default_model in r.message - ] + warnings_for_default = [r for r in caplog.records if default_model in r.message] assert len(warnings_for_default) == 0 def test_strict_validation_raises_on_unknown_model( @@ -139,17 +135,13 @@ def test_strict_validation_passes_with_known_model( mod = _reload_constants() assert mod.MODEL_CONFIG["default"] == mod._SKILLSPECTOR_DEFAULT_MODEL - def test_strict_validation_disabled_by_default( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: + def test_strict_validation_disabled_by_default(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("SKILLSPECTOR_MODEL", "nonexistent-model") # No SKILLSPECTOR_STRICT_MODEL_VALIDATION set — should warn, not raise. mod = _reload_constants() assert mod.MODEL_CONFIG["default"] == "nonexistent-model" - def test_strict_validation_case_insensitive( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: + def test_strict_validation_case_insensitive(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("SKILLSPECTOR_MODEL", "nonexistent-model") monkeypatch.setenv("SKILLSPECTOR_STRICT_MODEL_VALIDATION", "True") with pytest.raises(ValueError, match="Strict model validation enabled"): diff --git a/tests/unit/test_reviewer_nits.py b/tests/unit/test_reviewer_nits.py new file mode 100644 index 0000000..7fcc865 --- /dev/null +++ b/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")