fix: handle non-table profiles in config.load - #13349
Conversation
- Add validation that 'profiles' key is a dict, not a string - Add validation that each profile value is a dict - Return load_error instead of crashing with AttributeError - Add tests for invalid profile containers
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Move test_config_fix.py into tests/test_config.py where pytest discovers it - Replace tempfile.NamedTemporaryFile(delete=False) with pytest tmp_path fixture - Prevents temp file leaks and Windows PermissionError from open handle conflicts Fixes cubic-dev-ai review comments on PR #13349
|
Thanks for the clean fix — this addresses #13340 well and the quality bar is high. sdks/python-cli/omi_cli/config.py — the two isinstance checks in load() guard exactly the crash path from the issue: 'profiles' being a scalar and a profile value being a scalar both now return a Config with load_error set instead of raising AttributeError in Profile construction. This slots nicely into the existing error model: like malformed TOML, the file is reported via load_error, which also means save() keeps refusing to overwrite a hand-repairable config. Error messages naming the offending key and type ('profiles', "profile 'default'") are helpful. (Two blank lines carry trailing whitespace — cosmetic only.) sdks/python-cli/tests/test_config.py — the four regression tests are placed where pytest actually collects (testpaths = ["tests"]), reuse the conftest config_path fixture for isolation, and cover both repro shapes from the issue plus the two controls (valid profile loads; missing profiles section with a non-default active_profile still resolves). Good response to the earlier review note about the uncollected root-level test file. One tiny doc nit: the PR description still says tests were added as test_config_fix.py, but they now live in tests/test_config.py — worth updating the Testing section so future readers aren't confused. No CI runs are visible on this head yet; the tests should cover it once it runs. Leaving for routine human maintainer review before merge — nice first contribution. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
kodjima33
left a comment
There was a problem hiding this comment.
Bug fix, confidence ~4-5/5: stated root cause (AttributeError when 'profiles' TOML key is a string/scalar), linked fixes #13340, scoped diff (57 lines/2 files), tests added for 4 scenarios, cubic-dev-ai review confirms all issues addressed. CI hasn't triggered yet (pending/no-checks, acceptable per hard floor) — first PR from this contributor.
Summary
Fixes
AttributeError: 'str' object has no attribute 'items'whenprofileskey in TOML config is a string instead of a table.Problem
config.load()crashes when encountering valid TOML files like:or:
Solution
Added validation in
load()to check that:profileskey is a dict (table), not a string or other scalarWhen validation fails, returns a
Configwithload_errorset instead of crashing.Testing
Added
test_config_fix.pywith tests for:Fixes #13340