fix(config): treat an empty providers list as no secret providers - #73
Merged
Conversation
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 freeto 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.
The bug
The shipped
configs/secrets.yamlships with every provider example commented out:YAML parses that as
{'version': 1, 'providers': None}.SecretsFileConfig.providerswas
List[SecretProviderConfig]— required, no default — so validation failed andConfigManager.load_secretsraised:NL2SQLContext.__init__callsload_secrets, so the default non-demo path wasbroken for anyone using the file as shipped — including a fresh
pip install nl2sql-engine. It stayed invisible in demo mode only because.env.demopointsSECRETS_CONFIGatconfigs/secrets.demo.yaml, which does notexist, and a missing file short-circuits to
[].Surfaced by the repaired
nl2sql doctor, which reported the error instead ofcrashing on it.
Which shapes failed
secrets.yamlshapeproviders Input should be a valid list[]Input should be a valid dictionary ... input_value=[][]providers:key at allproviders Field required[]providers: [][]Only the explicit empty list worked. All four are shapes a user legitimately
produces, and all four now load as "no secret providers configured".
The fix
Two defects, both real:
configs/secrets.py—providersnow defaults to an empty list, plus afield_validator(..., mode="before")coercingNoneto[]. In pydantic v2 amissing key and an explicit
Noneare different cases: the default covers onlythe missing key, so the validator is what handles a
providers:key left emptyby commenting its entries out.
configs/manager.py—yaml.safe_load(content) or []produced a listfor an empty file, which then failed
model_validatewith a confusing "shouldbe a valid dictionary". The envelope is a mapping, so the fallback is now
{}.configs/secrets.yamlitself is deliberately left alone. Commenting out everyexample is a legitimate thing for a user to do, and the loader has to tolerate it;
adding
providers: []to the shipped file would paper over the defect for that onefile and leave every user-edited copy broken.
Sibling loaders in
manager.pywere checked for the same fallback/model mismatch:load_datasources,load_llmandload_sample_questionsall useor {}againstmapping-shaped envelopes, and
load_policiesusesjson.loadswith no fallback.None has the mismatch, and their remaining required fields (
datasources,default) are genuinely required — an empty datasources or LLM config is amisconfiguration worth an error. Left as-is.
Tests
packages/nl2sql/tests/unit/test_config_manager_secrets_empty.pycovers all fourshapes, that a real multi-provider config still parses, and — the regression that
actually shipped — that the repo's own
configs/secrets.yamlloads without raising.Written first; 4 of 6 failed against
mainwith the ValidationError above.Unit suite
247 passed, 1 skipped, 47 deselected(241 + 6 new), key-freeintegration
28 passed, 267 deselected; both run twice underpytest-randomly.