Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rioyu123 wants to merge 2 commits into
base: main
Choose a base branch
from
+58 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe1aefd
fix(providers): honor registry overrides for CLI providers
rioyu123 Aug 31, 2026
811f371
Merge branch 'main' into fix/cli-model-registry-override
rng1995 Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/skillspector/providers/_agent_cli_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,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 ---------------------------------------------------------
Expand Down Expand Up @@ -81,13 +81,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)

@yashrajp22 yashrajp22 Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard you removed was also (accidentally) the only thing keeping CLI providers away from parsing a user's hand-written registry file. Now that the file is parsed, a small mistake in it crashes the whole CLI instead of falling back to the default budget:

  • models: written as a list -> AttributeError: 'list' object has no attribute 'get'
  • a scalar entry like my-model: 42 -> AttributeError: 'int' object has no attribute 'get'
  • a non-numeric value like context_length: lots -> ValueError

And because constants._validate_model_config() runs at import time, the crash happens at startup: with SKILLSPECTOR_PROVIDER=claude_cli, SKILLSPECTOR_MODEL=my-model, and that YAML, skillspector dies with a raw traceback before doing anything. I ran this exact setup on current main and it starts fine there (warnings only), so this is a new failure mode from removing the guard.

The root cause is in registry.py: lookup_context_length() / lookup_max_output_tokens() call entry.get(...) and int(...) outside the try, so only file-level problems (missing/unreadable) are caught — shape and value problems are not. Since this PR is what turns hand-written registries into a real workflow for CLI users, could you harden those two functions in the same change? Treating a non-dict entry or a bad/non-positive value as "not found" (warn + return None, same as _load already does for unreadable files) covers all three cases in a few lines.

Everything else checks out — I built the wheel and confirmed the override works end to end (details in the review summary).


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:
Expand Down
57 changes: 57 additions & 0 deletions tests/unit/test_providers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from __future__ import annotations

import sys
from pathlib import Path

import pytest
from langchain_anthropic import ChatAnthropic
Expand Down Expand Up @@ -830,6 +831,62 @@ 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],
)
def test_honors_model_registry_override(

@yashrajp22 yashrajp22 Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice coverage for the happy paths. Once the registry lookups are hardened (see my comment in _agent_cli_base.py), could you add one test with a malformed registry — e.g. models:\n test-model: 42 — asserting the provider returns None instead of raising? That's the mistake a user is most likely to make when hand-writing this file for the first time.

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

Expand Down
Loading