fix(providers): honor model registry overrides for CLI providers - #463
fix(providers): honor model registry overrides for CLI providers#463rioyu123 wants to merge 2 commits into
Conversation
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
The shared CLI-provider base now delegates its empty bundled registry path to the existing override resolver while preserving the None fallback when the override is unset, blank, or lacks the requested model. The Claude, Codex, and Gemini regressions cover that contract; 91 focused tests passed (9 skipped), all required checks are green, and the branch merges cleanly with current main. Approved.
yashrajp22
left a comment
There was a problem hiding this comment.
I reviewed this by building the wheel (uv build, 2.11.0) and running it end to end, not just reading the diff.
The fix works as intended. On a small test skill with SKILLSPECTOR_PROVIDER=claude_cli and SKILLSPECTOR_MODEL=claude-haiku-4-5:
- without the override: 10x
No token-limit info for model 'claude-haiku-4-5' — using 128000-token default(the exact symptom from #459) - with
SKILLSPECTOR_MODEL_REGISTRYpointing at a YAML that declares the model: 0 warnings, and the debug log showsResolved 'claude-haiku-4-5' context length: 200000
I also ran an edge-case matrix against the installed wheel: unset/blank env var, missing file, directory, unreadable YAML, unknown model, context_length: 0, float/string values — all fall back safely to None as promised, with a single warning where appropriate. tests/unit/test_providers.py passes (91 passed, 9 skipped).
Two asks before merge, both as inline comments:
- A malformed registry YAML now crashes the CLI at startup for CLI-provider users (this couldn't happen before this change) — please harden the two
registry.lookup_*functions in the same PR. - One regression test for that malformed case.
One note for after this merges: this closes #459 via its suggested fix 1, so its suggestion 3 is still open — the LLM batch failed ... stderr='' log hides the real error because the claude CLI prints rejections to stdout, not stderr. That gap is what made #459 hard to diagnose in the first place, so it deserves its own follow-up issue.
| 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) |
There was a problem hiding this comment.
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).
| "provider_type", | ||
| [ClaudeCLIProvider, CodexCLIProvider, GeminiCLIProvider], | ||
| ) | ||
| def test_honors_model_registry_override( |
There was a problem hiding this comment.
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.
Summary
SKILLSPECTOR_MODEL_REGISTRYcan override itNonefallback when the override is unset, blank, or does not contain the requested modelTesting
uv run ruff check src/ tests/uv run ruff format --check src/ tests/uv run pytest tests/unit/test_providers.py -q(91 passed, 9 skipped)uv run python -m builduv run twine check dist/*The full non-integration suite also completed with 3,918 passing tests, 26 skipped, and 4 expected failures. Its 22 failures were confined to unrelated Windows newline, symlink-permission, path-separator, and executable-shim cases.
Closes #459