Skip to content

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

Open
rioyu123 wants to merge 2 commits into
NVIDIA:mainfrom
rioyu123:fix/cli-model-registry-override
Open

fix(providers): honor model registry overrides for CLI providers#463
rioyu123 wants to merge 2 commits into
NVIDIA:mainfrom
rioyu123:fix/cli-model-registry-override

Conversation

@rioyu123

Copy link
Copy Markdown

Summary

  • let agent CLI providers pass their empty bundled registry path through the shared resolver so SKILLSPECTOR_MODEL_REGISTRY can override it
  • preserve the existing None fallback when the override is unset, blank, or does not contain the requested model
  • cover Claude, Codex, and Gemini CLI providers with regression tests

Testing

  • 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 build
  • uv 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

Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com>

@rng1995 rng1995 left a comment

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.

[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 yashrajp22 left a comment

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.

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_REGISTRY pointing at a YAML that declares the model: 0 warnings, and the debug log shows Resolved '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:

  1. 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.
  2. 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)

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

"provider_type",
[ClaudeCLIProvider, CodexCLIProvider, GeminiCLIProvider],
)
def test_honors_model_registry_override(

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI providers cannot use SKILLSPECTOR_MODEL_REGISTRY: an empty REGISTRY_PATH short-circuits the override, forcing the 128k default

3 participants