Conversation
`is_provider_enabled_in_env` split the variable on `,` with no strip, so
`NBI_ENABLED_PROVIDERS="github-copilot, ollama"` re-enabled only
`github-copilot`. The comma-space form is the natural way to write a
list, and a single stray space around a lone entry disabled it outright.
It fails closed, so the admin sees the provider still missing with no
error anywhere and no way to tell why short of reading the source.
`split_csv` lives 30 lines above in the same module and its docstring
already names this variable as a caller of the
comma-separated-whitespace-tolerant format. It was simply never used
here, which is also why the sibling flag `is_builtin_tool_enabled_in_env`
behaves correctly for the same input shape.
Dropping empty tokens closes a second, older fail-open along the way:
`''.split(',')` is `['']`, so an empty provider id matched an unset
variable and read as enabled. Unreachable today because no registered
provider has an empty id, but it is the wrong default for a re-enable
check.
Only surrounding whitespace is treated as noise. Matching stays case
sensitive and an id with an inner space still only matches itself, since
provider ids are exact strings everywhere else and loosening either
would make the denylist and the re-enable list disagree about what a
provider is called.
Nothing covered this function or the variable before. Verified by
mutation that the new tests pin the regression: reverting to the
unstripped split fails 8 of the 20, and the 12 that keep passing are the
cases that never depended on stripping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NBI_ENABLED_PROVIDERS="github-copilot, ollama"re-enabled onlygithub-copilot. The value was split on,with no strip, so any entry with surrounding whitespace never matched. Measured against the old function:NBI_ENABLED_PROVIDERSgithub-copilotollamagithub-copilot,ollamagithub-copilot, ollamaollamaollama,It fails closed, which is the safer direction, but silently: the admin sees the provider still missing from the picker, with no error anywhere and no way to tell why short of reading the source.
Solution
split_csvalready lives 30 lines above in the same module, and its docstring names this very variable as a caller of the "comma-separated, whitespace-tolerant, empty-dropping" format. It simply was not used here, which is also why the sibling flagis_builtin_tool_enabled_in_envhandles the same input shape correctly while this one did not.Dropping empty tokens closes a second, older fail-open along the way:
''.split(',')is[''], sois_provider_enabled_in_env('')returnedTruefor an unset variable. Unreachable today because no registered provider has an empty id, but it is the wrong default for a re-enable check.Only surrounding whitespace is treated as noise. Matching stays case sensitive, and an id with an inner space still only matches itself: provider ids are exact strings everywhere else (
llm_providerskeys), so loosening either would make the denylist and the re-enable list disagree about what a provider is called. Both are pinned by tests so a future "be more forgiving" change has to be deliberate.Testing
pytest1773 passed, including 20 new cases intests/test_enabled_providers_env.py. Nothing covered this function or the variable before.Covered: comma-separated, comma-space (the reported regression), surrounding whitespace in five forms including tab and newline, repeated and trailing commas, a provider not in the list, unset, blank values, the empty-provider-id fail-open, case sensitivity, and internal whitespace.
Verified by mutation that these pin the regression rather than the implementation. Reverting to the unstripped split fails 8 of the 20; the 12 that keep passing are exactly the cases that never depended on stripping.
tsc --noEmitclean, jest 423 passed,eslint,stylelint,prettierclean. No TypeScript changed.Risks and follow-ups
is_provider_enabledclosure in the capabilities handler (extension.py:663), reached whenallow_enabling_providers_with_envis on. That handler has no test harness in this repo, so "the closure consults this function correctly" remains uncovered, as it was before.Closes #435