Skip to content

fix(cli): validate active_profile is a string in config.load (#13442) - #13446

Merged
kodjima33 merged 2 commits into
BasedHardware:mainfrom
gianluca-disanto:fix-python-cli-active-profile
Sep 11, 2026
Merged

fix(cli): validate active_profile is a string in config.load (#13442)#13446
kodjima33 merged 2 commits into
BasedHardware:mainfrom
gianluca-disanto:fix-python-cli-active-profile

Conversation

@gianluca-disanto

@gianluca-disanto gianluca-disanto commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #13442

Validates that active_profile in the config file is a string during config.load(). If a non-string value is present (such as a list, integer, boolean, or inline table from an otherwise valid TOML file), load_error is populated and active_profile falls back to the default profile rather than crashing with unhandled runtime exceptions.

Problem & Root Cause

At main, config.load() reads active = data.get("active_profile", DEFAULT_PROFILE_NAME) without validating its type.

When a TOML configuration contains a valid TOML non-string scalar or collection (for instance, active_profile = ["work"] or active_profile = 42):

  1. Config.active_profile receives the non-string object and Config.load_error remains None.
  2. When subsequent commands invoke config.get_profile(), the lookup target in self.profiles crashes with TypeError: unhashable type: 'list'.
  3. Commands like auth status or config set abort with an unhandled traceback instead of reporting a clean diagnostic error.
  4. Because load_error was not set, save() does not refuse to overwrite, which could clobber or corrupt the user's repairable config file.

Changes

  1. sdks/python-cli/omi_cli/config.py:

    • Added validation immediately after reading active_profile:
      active = data.get("active_profile", DEFAULT_PROFILE_NAME)
      if not isinstance(active, str):
          return Config(
              path=p,
              active_profile=DEFAULT_PROFILE_NAME,
              profiles={},
              load_error=f"'active_profile' must be a string, got {type(active).__name__}",
          )
    • Matches the error-handling idiom previously introduced for 'profiles' must be a table in fix: handle non-table profiles in config.load #13349.
    • Ensures Config.was_load_error is True, so save() refuses to overwrite and protects user configs.
  2. sdks/python-cli/tests/test_config.py:

    • Added parametrized regression tests covering multiple non-string types (list, int, bool, dict).
    • Added test verifying that save() raises PermissionError and refuses to overwrite when active_profile has an invalid type.
    • Added test verifying that read-only diagnostics commands (omi version, omi config path) continue to execute successfully when active_profile is invalid.

Verification & Test Results

Ran test suite with pytest tests/test_config.py:

  • 34 of 34 tests pass (including all 6 new regression cases).
  • Verified save() refusal against corrupt configuration.
  • Verified CLI diagnostics (version, config path) remain functional.

Product invariants affected

none

Failure class (fixes)

Failure-Class: none

Review in cubic

…rdware#13442)

- Reject non-string active_profile values (lists, ints, booleans, tables) during config.load()
- Set load_error to prevent commands from crashing with TypeError: unhashable type
- Ensure write operations refuse to overwrite config when active_profile is invalid
- Add comprehensive regression tests covering multiple non-string active_profile types and diagnostics

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@aryanorastar aryanorastar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python CLI change itself looks correct, and the platform test matrix is passing. I found two required repository-contract failures that should be fixed before approval:

  1. PR Metadata Preflight is failing because the fix: commit has no Failure-Class: declaration in the PR body. Please add exactly one accepted declaration, such as Failure-Class: none if no failure class applies.

  2. Hygiene is failing because sdks/python-cli/tests/test_config.py has a new blank line at EOF. Remove the extra blank line and rerun the required checks.

After these two mechanical fixes and a green rerun, I would be happy to re-review.

@gianluca-disanto

gianluca-disanto commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

The Python CLI change itself looks correct, and the platform test matrix is passing. I found two required repository-contract failures that should be fixed before approval:

  1. PR Metadata Preflight is failing because the fix: commit has no Failure-Class: declaration in the PR body. Please add exactly one accepted declaration, such as Failure-Class: none if no failure class applies.
  2. Hygiene is failing because sdks/python-cli/tests/test_config.py has a new blank line at EOF. Remove the extra blank line and rerun the required checks.

After these two mechanical fixes and a green rerun, I would be happy to re-review.

@aryanorastar Thanks for the quick review! I've removed the extra blank line at EOF in commit 9db163e and added Failure- Class: none to the PR description.

@aryanorastar aryanorastar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The active_profile type validation is correctly implemented, preserves invalid configuration files by refusing overwrite, and has focused regression coverage for invalid TOML types and diagnostic commands. The author also fixed the repository-contract issues: Failure-Class: none is present and the trailing blank line is removed. The latest Python CLI and metadata checks pass.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Verified against main and the current head — this is a clean, well-targeted fix. Detailed pass:

sdks/python-cli/omi_cli/config.py

  • The new guard sits exactly where it should: immediately after active = data.get("active_profile", DEFAULT_PROFILE_NAME) and before the profiles table validation, so a non-string selector can no longer leak into Config.active_profile (which is typed str and used as a dict key by get_profile()).
  • It mirrors the established load-error idiom in this function ('profiles' must be a table, profile '<name>' must be a table, and the TOML/UTF-8 parse failures): empty profiles, DEFAULT_PROFILE_NAME fallback, descriptive load_error.
  • Because load_error is set, save()'s refusing to overwrite PermissionError path now also protects files with a bad active_profile — at main such a file loads with load_error=None, meaning a write command could clobber a config the user could have repaired by hand. That's the real data-protection win here.
  • isinstance(active, str) is exhaustive for what tomllib can produce at this point (str, bool, int, float, datetime, list, dict), so no other type can slip through.

sdks/python-cli/tests/test_config.py

  • The parametrized cases cover all four realistic TOML shapes (list, int, bool, inline table → dict) and assert the offending type name inside load_error.
  • test_active_profile_non_string_refuses_save_overwrite checks both the PermissionError from save() and that the file on disk is preserved intact — good.
  • test_active_profile_non_string_diagnostics_succeed locks in the invariant documented on load(): read-only diagnostics (omi version, omi config path) keep working precisely when the config needs repair.
  • Fixtures (config_path, cli_runner from tests/conftest.py) are used correctly, and the relevant matrix jobs (Linux Python 3.10/3.12, Windows ACL) are green on this head.

Good follow-through on the earlier review round. From my side this is ready to merge as-is.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added positive-signal Automation verified a genuine fix/quality contribution python labels Sep 10, 2026

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix(cli) active_profile type validation -- 5/5: root cause clear, fixes #13442, scoped diff (56 lines/2 files), CI green (Linux 3.10/3.12, Windows), regression tests added.

@kodjima33
kodjima33 merged commit 059b00a into BasedHardware:main Sep 11, 2026
38 of 40 checks passed
@gianluca-disanto
gianluca-disanto deleted the fix-python-cli-active-profile branch September 11, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

positive-signal Automation verified a genuine fix/quality contribution python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python CLI accepts non-string active_profile and fails profile commands

4 participants