Skip to content

fix(capabilities): let disabled_providers suppress a provider's models (#431) - #432

Open
pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/431-disabled-providers-models
Open

pjdoland wants to merge 1 commit into
plmbr:mainfrom
pjdoland:fix/431-disabled-providers-models

Conversation

@pjdoland

Copy link
Copy Markdown
Collaborator

Summary

disabled_providers removed a provider from the picker but not its models from the capabilities response. With disabled_providers = ["ollama"], llm_providers correctly omitted Ollama while chat_models still carried every Ollama model in the same payload.

Two consequences. The response advertised models the admin had switched off, and anything reading capabilities.chat_models without cross-checking llm_providers (src/api.ts:331, :335) treated them as available. Worse, enumerating a provider's models is not free for all of them: the Ollama list is built by calling the Ollama host, so the response performed work on behalf of a provider that was disabled.

Solution

GetCapabilitiesHandler.get already filters the provider list through an is_provider_enabled closure (extension.py:659-663, applied at :673). The three model lists came straight from AIServiceManager, whose chat_model_ids and siblings walk every registered provider with no notion of enablement (ai_service_manager.py:492-508). They now pass through a small pure helper using that same predicate, so provider and model filtering cannot disagree.

Two deliberate choices worth review:

  • The helper takes the predicate, not the denylist. The caller's version already folds in the per-pod re-enable env var (allow_enabling_providers_with_env plus is_provider_enabled_in_env), and duplicating that resolution in util.py would be a second source of truth for it.
  • An entry whose provider cannot be read is kept. For a denylist, failing open is the less obvious instinct, so it is documented in the docstring and pinned by a test. Every entry is built in-process with a provider key, so the branch only matters if that shape changes, and silently hiding a model for a reason the admin did not ask for is the worse and quieter failure.

Gating construction inside AIServiceManager was considered and rejected: the traitlet is attached only to the handler (extension.py:4317) and is deliberately absent from the manager's options (:4219), so the manager has no denylist to consult and giving it one is a much larger change than this warrants.

This does not orphan a previously reachable selection. A denylisted provider is already missing from llm_providers, so it cannot be the selected provider, and the settings panel scopes the model list to the selected provider before rendering (settings-panel.tsx:447-449). I checked this specifically because filtering a model list is exactly how a persisted selection can silently change under a user.

Testing

pytest 1760 passed, including 7 new cases in tests/test_disabled_provider_models.py: a disabled provider's models dropped, an enabled provider untouched, a permissive predicate leaving the list byte-identical, a re-enabled provider keeping its models, the fail-open entries (missing key, null provider, non-dict), non-list input degrading to empty, and the input list left unmutated.

tsc --noEmit clean, jest 423 passed, eslint, stylelint, prettier clean. No TypeScript changed.

Risks and follow-ups

  • The three call sites have no harness-level test. The capabilities handler has no test harness in this repo, so the helper is covered but "the handler actually calls it for all three keys" is not. tests/test_config_integration.py documents the same reproduce-in-isolation limitation for the adjacent _setup_handlers expression. A handler fixture would retire both gaps and is worth doing separately.
  • Stacking order. This stacks behind the branch for perf(ollama): defer and bound the chat-model enumeration (#427) #428, which moves the Ollama enumeration off server startup and onto the capabilities path, making this gap matter more. The two are independent PRs against main and touch different files, so either can merge first.
  • Review coverage was curtailed: the persona review I would normally run was cut short by a session rate limit, so this had my own diff pass plus the automated gate rather than independent reviewer eyes.
  • Noticed in passing, not touched here: is_provider_enabled_in_env (util.py:379-381) splits NBI_ENABLED_PROVIDERS on , without stripping, so "github-copilot, ollama" silently fails to re-enable ollama. Separate issue, separate fix, and split_csv in the same module is the ready-made remedy.

Closes #431

plmbr#431)

The capabilities response filtered `llm_providers` through the enablement
predicate but returned the model lists straight from AIServiceManager,
whose `chat_model_ids` and siblings walk every registered provider with
no notion of enablement. With `disabled_providers = ["ollama"]` the
provider correctly vanished from the picker while every Ollama model
stayed in the same response.

Two consequences. The payload advertised models an admin had switched
off, and anything reading `capabilities.chat_models` without
cross-checking `llm_providers` treated them as available. Worse,
enumerating a provider's models is not free for all of them: the Ollama
list is built by calling the Ollama host, so the response performed work
on behalf of a provider that was disabled. That matters more once the
Ollama enumeration moves off server startup and onto the capabilities
path, so this stacks behind the branch for plmbr#428.

The filter is a small pure helper beside the existing provider-enablement
utilities, applied at the three payload sites. It takes the predicate
rather than the denylist, because the caller's version already folds in
the per-pod re-enable env var and that resolution should not be
duplicated. An entry whose provider cannot be read is kept
deliberately: every entry is built in-process with a `provider` key, so
that only matters if the shape changes, and hiding a model for a reason
the admin did not ask for would be the worse and quieter failure.

Filtering cannot orphan a selection that was previously reachable. A
denylisted provider is already absent from `llm_providers`, so it cannot
be the selected provider, and the settings panel scopes the model list to
the selected provider before rendering it.

Tests cover the helper: a disabled provider's models dropped, an enabled
provider untouched, a permissive predicate leaving the list identical, a
re-enabled provider keeping its models, the fail-open entries, non-list
input degrading to empty, and the input list left unmutated. The three
call sites are one-liners around it and are not exercised, because the
capabilities handler has no test harness here; the same
reproduce-in-isolation limitation is already documented in
tests/test_config_integration.py.
@pjdoland pjdoland added the bug Something isn't working label Sep 14, 2026
@pjdoland
pjdoland requested a review from mbektas September 14, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(capabilities): disabled_providers does not suppress a provider's models

1 participant