fix(providers): honor model registry overrides for CLI providers - #463
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.
Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head 7d79956ce69b0b566eca3407c82b39e66b89dc64. The post-review registry hardening resolves both prior comments: malformed top-level/model entries and invalid, non-positive, or infinite budgets fall back to None without discarding a valid sibling field, while Claude, Codex, and Gemini CLI providers honor a valid override. The complete diff and focused regressions leave no required code or test change.
All required checks pass. Do not merge yet: GitHub reports BEHIND, and both addressed review threads remain unresolved.
Summary
SKILLSPECTOR_MODEL_REGISTRYeven though they have no bundled registry.Nonefallback for missing models or unset/blank overrides, and warn rather than crash on malformed registry entries or invalid/non-positive budgets.Closes #459.
Testing
On Linux / Python 3.12 with
uv sync --all-extras --locked:uv run --locked pytest -q tests/unit/test_providers.py tests/unit/test_model_info.pyuv run --locked make test-ciuv run --locked make lintuv run --locked make format-checkuv run --locked python -m builduv run --locked twine check dist/*The new malformed-registry regressions were run before and after the fix. Optional live-provider tests and Docker smoke were not run locally; hosted CI is reported separately.