Skip to content

fix: guard against list-valued keys in hermes config set (#76138) - #76234

Open
RelaxJonh wants to merge 1 commit into
NousResearch:mainfrom
RelaxJonh:fix/config-set-list-keys
Open

RelaxJonh wants to merge 1 commit into
NousResearch:mainfrom
RelaxJonh:fix/config-set-list-keys

Conversation

@RelaxJonh

Copy link
Copy Markdown
Contributor

Problem

hermes config set always writes a scalar value. When the target key is list-typed (e.g. toolsets, plugins.enabled, terminal.env_passthrough), the command silently writes a quoted YAML string instead of a YAML sequence, corrupting the config.

# What gets written (broken):
plugins:
  enabled: "[local_model_warning]"   # string, NOT a list

# What should exist:
plugins:
  enabled:
    - local_model_warning

The corrupted config silently breaks downstream consumers that iterate the value as a list.

Fix

Add a pre-write guard in set_config_value that checks:

  1. The existing value in user config
  2. The declared default in DEFAULT_CONFIG via _default_value_for_key()

When either is a list, refuse the write with a clear error message:

✗ 'toolsets' is a list-valued setting.  `hermes config set` writes a
  scalar and would corrupt it (quoted string instead of YAML list).
  Edit config.yaml directly, or use `hermes config edit`.

The guard covers both bare keys and dotted keys. --force bypasses it for scripted use.

Testing

  • All 39 existing config_set tests pass
  • The guard correctly rejects hermes config set plugins.enabled foo when the key is list-typed
  • --force allows the write through

Closes #76138

…h#76138)

hermes config set always writes a scalar, which corrupts list-typed
settings (writes a quoted YAML string instead of a YAML sequence).

Add a pre-write guard that checks _default_value_for_key() and the
existing user config value. When either is a list, refuse the write
with a clear error message pointing to hermes config edit.

The guard covers both bare keys (e.g. 'toolsets') and dotted keys
(e.g. 'plugins.enabled'). --force bypasses it for scripted use.

Closes NousResearch#76138
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 1, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #76139: both address scalar corruption of list-valued config settings, but this PR refuses the write while #76139 parses list input. Maintainer policy choice needed; not a duplicate.

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

Thanks for targeting a real config-corruption path. Current main passes a string through set_config_value to _set_nested (hermes_cli/config.py:4869-4880, :4948), so list-shaped CLI input can indeed persist as a scalar.

Problems

  • The proposed refusal also rejects the documented hermes config set toolsets '["hermes-cli", "browser"]' workflow at website/docs/user-guide/features/browser.md:422. The PR needs a maintainer decision between updating that contract or parsing supported list input; the related-PR discussion already identifies this distinction.
  • The diff changes only hermes_cli/config.py; it adds no regression tests. Existing tests cover list-index navigation (tests/hermes_cli/test_set_config_value.py:198) and mapping replacement (:574), not this list-leaf guard.
  • --force gains a list-overwrite bypass, while its parser help still says it only skips the unknown-key notice (hermes_cli/subcommands/config.py:43-47).

Suggested changes

  • Resolve the documented list-input behavior, add rejection/force regression tests, and update --force help if the bypass remains.

Automated hermes-sweeper review.

Comment thread hermes_cli/config.py
# YAML string instead of a YAML sequence). Check both the existing
# user-config value and the declared default — either being a list is
# enough to refuse (unless --force).
if not force and "." not in key:

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.

This guard rejects the documented hermes config set toolsets '["hermes-cli", "browser"]' workflow (website/docs/user-guide/features/browser.md:422). Please either parse that supported list syntax here or update the documentation after confirming refusal is the intended UX.

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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists 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 corrupts list-valued keys (writes scalar string instead of YAML list)

3 participants