fix(providers): honor registry overrides for CLI providers (#463)

Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
Co-authored-by: Narendran Raghavan <32655573+rng1995@users.noreply.github.com>

Author: rioyu123

src/skillspector/providers/_agent_cli_base.py

@@ -40,7 +40,7 @@ class AgentCLIProviderBase:
     #: ``_provider.DEFAULT_MODEL`` lookup has an attribute; never pins a version.
     DEFAULT_MODEL: str = ""
     #: Optional path to a bundled ``model_registry.yaml`` for token budgets. CLI
-    #: providers leave this empty and fall back to package-wide default budgets.
+    #: providers leave this empty, but users can supply a global registry override.
     REGISTRY_PATH: str = ""
 
     # -- Credentials ---------------------------------------------------------
@@ -82,13 +82,9 @@ def complete(
     # -- Metadata ------------------------------------------------------------
 
     def get_context_length(self, model: str) -> int | None:
-        if not self.REGISTRY_PATH:
-            return None  # no registry -> caller uses the package-wide default budget
         return registry.lookup_context_length(self.REGISTRY_PATH, model)
 
     def get_max_output_tokens(self, model: str) -> int | None:
-        if not self.REGISTRY_PATH:
-            return None
         return registry.lookup_max_output_tokens(self.REGISTRY_PATH, model)
 
     def resolve_model(self, slot: str = "default") -> str:

src/skillspector/providers/registry.py

@@ -53,7 +53,12 @@ def _load(yaml_path: str) -> dict[str, dict[str, Any]]:
     try:
         raw = Path(yaml_path).read_text(encoding="utf-8")
         data = yaml.safe_load(raw) or {}
-        return data.get("models") or {}
+        models = data.get("models")
+        if models is None:
+            return {}
+        if not isinstance(models, dict):
+            raise ValueError("model registry 'models' must be a mapping")
+        return models
     except Exception:
         logger.warning("Could not load model registry at %s", yaml_path, exc_info=True)
         return {}
@@ -67,18 +72,33 @@ def _resolve_path(default_yaml_path: str) -> str:
 
 def lookup_context_length(default_yaml_path: str, model: str) -> int | None:
     """Return ``context_length`` for *model* from the resolved YAML registry."""
-    entry = _load(_resolve_path(default_yaml_path)).get(model)
-    if entry and entry.get("context_length"):
-        return int(entry["context_length"])
-    return None
+    return _lookup_token_limit(default_yaml_path, model, "context_length")
 
 
 def lookup_max_output_tokens(default_yaml_path: str, model: str) -> int | None:
     """Return ``max_output_tokens`` for *model* from the resolved YAML registry."""
-    entry = _load(_resolve_path(default_yaml_path)).get(model)
-    if entry and entry.get("max_output_tokens"):
-        return int(entry["max_output_tokens"])
-    return None
+    return _lookup_token_limit(default_yaml_path, model, "max_output_tokens")
+
+
+def _lookup_token_limit(default_yaml_path: str, model: str, field: str) -> int | None:
+    """Read a positive budget, falling back for malformed entries or values."""
+    yaml_path = _resolve_path(default_yaml_path)
+    entry = _load(yaml_path).get(model)
+    if entry is None:
+        return None
+    try:
+        if not isinstance(entry, dict):
+            raise ValueError("model registry entry must be a mapping")
+        value = entry.get(field)
+        if value is None:
+            return None
+        limit = int(value)
+        if limit <= 0:
+            raise ValueError("model token budget must be positive")
+        return limit
+    except (TypeError, ValueError, OverflowError):
+        logger.warning("Invalid %s for model %s in registry %s", field, model, yaml_path)
+        return None
 
 
 # Back-compat alias for tests that previously called ``_load_registry``.

tests/unit/test_providers.py

@@ -23,6 +23,7 @@
 from __future__ import annotations
 
 import sys
+from pathlib import Path
 
 import pytest
 from langchain_anthropic import ChatAnthropic
@@ -830,6 +831,108 @@ def test_is_available_reports_not_ready(self) -> None:
         assert reason
 
 
+class TestAgentCLIProviderMetadata:
+    """Shared model-registry behavior for supported agent CLI providers."""
+
+    @pytest.mark.parametrize(
+        "provider_type", [ClaudeCLIProvider, CodexCLIProvider, GeminiCLIProvider]
+    )
+    @pytest.mark.parametrize(
+        "contents",
+        [
+            "models: [test-model]\n",
+            "models:\n  test-model: 42\n",
+            "models:\n  test-model:\n    context_length: lots\n    max_output_tokens: lots\n",
+            "models:\n  test-model:\n    context_length: -1\n    max_output_tokens: -1\n",
+            "models:\n  test-model:\n    context_length: 0\n    max_output_tokens: 0\n",
+            "models:\n  test-model:\n    context_length: .inf\n    max_output_tokens: .inf\n",
+        ],
+        ids=["models-list", "scalar-entry", "bad-string", "negative", "zero", "infinite"],
+    )
+    def test_malformed_registry_returns_none(
+        self,
+        provider_type: type[ClaudeCLIProvider | CodexCLIProvider | GeminiCLIProvider],
+        contents: str,
+        monkeypatch: pytest.MonkeyPatch,
+        tmp_path: Path,
+    ) -> None:
+        registry_path = tmp_path / "model_registry.yaml"
+        registry_path.write_text(contents, encoding="utf-8")
+        monkeypatch.setenv("SKILLSPECTOR_MODEL_REGISTRY", str(registry_path))
+
+        provider = provider_type()
+        assert provider.get_context_length("test-model") is None
+        assert provider.get_max_output_tokens("test-model") is None
+
+    @pytest.mark.parametrize("invalid_field", ["context_length", "max_output_tokens"])
+    def test_invalid_budget_preserves_other_field(
+        self, invalid_field: str, monkeypatch: pytest.MonkeyPatch, tmp_path: Path
+    ) -> None:
+        valid_field = "max_output_tokens" if invalid_field == "context_length" else "context_length"
+        registry_path = tmp_path / "model_registry.yaml"
+        registry_path.write_text(
+            f'models:\n  test-model:\n    {invalid_field}: lots\n    {valid_field}: "32000"\n',
+            encoding="utf-8",
+        )
+        monkeypatch.setenv("SKILLSPECTOR_MODEL_REGISTRY", str(registry_path))
+
+        provider = ClaudeCLIProvider()
+        assert getattr(provider, f"get_{invalid_field}")("test-model") is None
+        assert getattr(provider, f"get_{valid_field}")("test-model") == 32_000
+
+    @pytest.mark.parametrize(
+        "provider_type",
+        [ClaudeCLIProvider, CodexCLIProvider, GeminiCLIProvider],
+    )
+    def test_honors_model_registry_override(
+        self,
+        provider_type: type[ClaudeCLIProvider | CodexCLIProvider | GeminiCLIProvider],
+        monkeypatch: pytest.MonkeyPatch,
+        tmp_path: Path,
+    ) -> None:
+        registry_path = tmp_path / "model_registry.yaml"
+        registry_path.write_text(
+            "models:\n  test-model:\n    context_length: 200000\n    max_output_tokens: 32000\n",
+            encoding="utf-8",
+        )
+        monkeypatch.setenv("SKILLSPECTOR_MODEL_REGISTRY", str(registry_path))
+
+        provider = provider_type()
+        assert provider.get_context_length("test-model") == 200_000
+        assert provider.get_max_output_tokens("test-model") == 32_000
+
+    @pytest.mark.parametrize("registry_value", [None, "   "])
+    def test_returns_none_without_registry(
+        self,
+        registry_value: str | None,
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        if registry_value is None:
+            monkeypatch.delenv("SKILLSPECTOR_MODEL_REGISTRY", raising=False)
+        else:
+            monkeypatch.setenv("SKILLSPECTOR_MODEL_REGISTRY", registry_value)
+
+        provider = ClaudeCLIProvider()
+        assert provider.get_context_length("test-model") is None
+        assert provider.get_max_output_tokens("test-model") is None
+
+    def test_unknown_model_returns_none(
+        self,
+        monkeypatch: pytest.MonkeyPatch,
+        tmp_path: Path,
+    ) -> None:
+        registry_path = tmp_path / "model_registry.yaml"
+        registry_path.write_text(
+            "models:\n  known-model:\n    context_length: 200000\n    max_output_tokens: 32000\n",
+            encoding="utf-8",
+        )
+        monkeypatch.setenv("SKILLSPECTOR_MODEL_REGISTRY", str(registry_path))
+
+        provider = ClaudeCLIProvider()
+        assert provider.get_context_length("unknown-model") is None
+        assert provider.get_max_output_tokens("unknown-model") is None
+
+
 class TestClaudeCLIProvider:
     """Claude CLI provider — metadata, availability, and capability detection."""