Skip to content

fix(cli): hermes config set now parses YAML lists/dicts (#64323) - #64399

Open
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/64323-config-set-lists
Open

fix(cli): hermes config set now parses YAML lists/dicts (#64323)#64399
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/64323-config-set-lists

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Fixes #64323. Added YAML safe_load when value starts with [ or {, so docker_volumes and other list configs are stored correctly instead of as literal strings.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have labels Jul 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #40546 — both add list/dict-literal parsing at the same set_config_value() site in hermes_cli/config.py (this PR uses yaml.safe_load, #40546 uses json.loads, same mechanism/site). #40546 is the earliest still-open PR for this fix. Also related to sibling #60558.

@teknium1 teknium1 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.

Thanks for targeting a real config set failure. Current main leaves bracketed values as strings in hermes_cli/config.py:8264-8279; terminal.docker_volumes is list-typed (hermes_cli/config.py:1241) and Docker discards non-list values (tools/environments/docker.py:615-618).

Problems

  • The new coercion block deletes the string-default guard from e4ea0a0ed7fc24761b2b425146893561a73216e1. The current regression suite requires approvals.mode values such as off to remain strings (tests/hermes_cli/test_set_config_value.py:256-289); this branch would coerce off to false.
  • The PR patch also includes unrelated DEFAULT_CONFIG changes outside the stated parsing fix.

Suggested changes

  • Preserve the string-typed guard and its tests; apply structured list/dict parsing only where it cannot reinterpret a declared string setting.
  • Remove the unrelated default-config hunks and keep the salvage focused on structured config values.

Automated hermes-sweeper review.

Comment thread hermes_cli/config.py

# Convert value to appropriate type (only for non-list/dict values)
if not isinstance(value, (list, dict)):
if value.lower() in {'true', 'yes', 'on'}:

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.

Current main intentionally preserves string-typed settings: e4ea0a0ed7fc24761b2b425146893561a73216e1 and tests/hermes_cli/test_set_config_value.py:256-289 require approvals.mode off to remain a string. This unconditional scalar coercion would write boolean false; retain the declared-string guard and parse structures only where that invariant is preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes config set silently writes list values as strings — docker docker_volumes mounts then silently no-op (false sense of isolation/config success)

3 participants