feat(provider): allow scoped LLM provider injection (#243)#249
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes. Context-local provider dispatch is the right shape, but unusable bindings are reported as available, injected CLI providers take the wrong construction path, and model-limit caching can leak values across provider contexts. These make scoped injection unreliable and can produce dishonest LLM accounting.
Signed-off-by: Rod Boev <rod.boev@gmail.com>
295fdb8 to
f0f2f3e
Compare
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review of head f0f2f3e. This PR adds a ContextVar-backed scoped provider binding (use_provider/reset_provider/has_provider_binding) so embedding applications can inject a governed LLM provider without exporting keys or monkeypatching. All three previously raised blockers are resolved; no new blockers found.
Prior-issue resolution checklist
- Unusable bindings reported as available (
mcp_server.py) — Resolved.run_scannow derivesllm_availablefrom the sharedis_llm_available()path instead ofresolve_provider_credentials(). For bound HTTP providers,is_llm_available()probes native chat-model construction;providers.create_chat_modelraises the no-LLM-keyValueErrorwhen a bound provider returnsNoneinstead of silently falling back to OpenAI (fallback preserved for the unbound path). Covered bytest_run_scan_disables_llm_for_unavailable_bound_provider,test_run_scan_uses_bound_provider_without_credentials, andtest_injected_provider_without_native_model_does_not_fall_back_to_openai(which setsOPENAI_API_KEYto prove no fallback leak). - Injected CLI providers sent through
create_chat_model(llm_utils.py) — Resolved.is_llm_available()now checkshas_cli_capability(get_active_provider())before the binding probe, andget_chat_model()routes CLI-capable bound providers toAgentCLIChatModel(duck-typedhas_cli_capabilitycovers externally injected providers). Regressiontest_bound_cli_provider_uses_cli_availabilityassertsis_available()is used andcreate_chat_modelis never called. - Process-global context-length cache keyed only by model label (
model_info.py) — Resolved.@functools.cacheremoved from_resolve_context_length; resolution now consults the context-localget_metadata_provider()at call time. The remainingregistry._loadcache is keyed by YAML path, so no cross-provider leakage. Regressiontest_token_limits_follow_current_bound_provider_for_same_model_labelverifies the same label yields different limits under different bindings.
Verification
Applied the head diff onto current main locally: it applies cleanly, the full unit/node test suite passes (1267 passed, 12 skipped, 6 xfailed), and ruff check is clean.
Non-blocking notes
run_scannow callsis_llm_available()on every scan, includinguse_llm=False; for CLI providers this delegates tois_available(), which may spawn a subprocess (binary/auth probe) per scan. Consider whether static-only scans should short-circuit if MCP latency matters.- With the
functools.cacheremoved, the "No token-limit info for model ... using default" warning in_resolve_context_lengthnow logs on every call for unknown labels rather than once per process. Consider alogger.warning-once guard if log volume becomes noisy. SKILLSPECTOR_MODEL_{SLOT}env overrides still take precedence over an injected provider inbuild_model_config(). This matches the documented precedence, but embedders should be aware host env vars can override their bound provider's model choices; a docstring note onuse_providerwould help.
| raise ValueError(f"output_format must be one of {VALID_FORMATS}, got {output_format!r}") | ||
|
|
||
| llm_available = resolve_provider_credentials() is not None | ||
| llm_available, _ = is_llm_available() |
There was a problem hiding this comment.
Non-blocking: this now runs on every scan, including use_llm=False. For CLI providers is_llm_available() delegates to is_available(), which can spawn a subprocess (binary/auth probe) per request. If MCP scan latency matters, consider computing availability lazily or only probing when use_llm is requested (the payload's llm_available field would need a cheaper source in that case).
Summary
Embedding applications can now bind a governed in-process LLM provider for the current scan context. That lets hosts reuse their existing completion API without exporting raw keys, invoking an agent CLI, or monkeypatching SkillSpector internals.
Closes #243
Root cause
Provider selection lived entirely behind
SKILLSPECTOR_PROVIDERand the built-in fallback chain. Every public helper routed through_select_active_provider(), but that selector had no scoped override for a provider object that the embedding application already owns.The first implementation also left three provider-context edges open: bound HTTP providers could be treated as available even when their native chat-model construction failed, bound CLI providers could be sent through the HTTP provider path, and model context length was cached only by label even though the same label can resolve differently under different providers.
Diff Notes
ContextVar-backed provider binding API inskillspector.providers.Scope
This does not add a new provider family, credential store, CLI transport, or graph-level policy. The hook stays in the provider adapter layer, and MCP scan accounting only observes whether a provider is bound for the current context.
Attribution
The upstream issue proposed the
ContextVarbinding shape; this PR implements that design in the existing provider selector.Verification
pytest tests/unit/test_llm_utils.py tests/unit/test_mcp_server.py tests/unit/test_model_info.py tests/unit/test_providers.py- passruff check src/ tests/ruff format --check src/ tests/24 rows, state domains checkedLint & Test (Python 3.12),Lint & Test (Python 3.13), andDCO Check- pending maintainer approval and rerun after push