Skip to content

fix(config): parse bracket-delimited list values in config set - #37460

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/config-set-list-value
Closed

fix(config): parse bracket-delimited list values in config set#37460
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/config-set-list-value

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes hermes config set so that bracket-delimited values like [icarus] are parsed as YAML lists instead of being stored as literal strings. Previously, hermes config set enabled '[icarus]' wrote enabled: '[icarus]' (a string), silently breaking plugin loading.

Related Issue

Fixes #37455

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/config.py: Added YAML list parsing for values matching [...] syntax in set_config_value(). When the value starts with [ and ends with ], it is parsed via yaml.safe_load(); if the result is a list, it replaces the raw string. Invalid YAML falls back to the original string behavior.
  • tests/hermes_cli/test_set_config_value.py: Added TestBracketListValues class with 6 regression tests: single-element list, multi-element list, quoted items, invalid YAML fallback, empty list, and nested key list value.

How to Test

  1. Run hermes config set enabled '[icarus]' and verify config.yaml contains enabled:\n- icarus (a YAML list, not a string)
  2. Run hermes config set enabled '[icarus, zeus]' and verify both items appear as a list
  3. Run python3 -m pytest tests/hermes_cli/test_set_config_value.py -v — all 38 tests should pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/hermes_cli/test_set_config_value.py -q and all 38 tests pass
  • I've added tests for my changes (6 new regression tests)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: hermes_cli/config.py:set_config_value (called from config_command CLI handler)
  • Blast radius: LOW — only affects config set value parsing; no impact on config reading, migration, or other commands
  • Related patterns: Similar to fix(gateway): accept bare-string home_channel from hermes config set #33145 (bare-string home_channel handling); both extend set_config_value type coercion

When the user runs `hermes config set enabled '[icarus]'`, the value was
stored as a literal YAML string instead of a list. This adds YAML parsing
for values matching `[...] ` syntax, converting them to proper lists.

Fixes NousResearch#37455
@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 labels Jun 2, 2026

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

Code Review Summary

Verdict: Approved

Nice fix: hermes config set now parses [bracket-delimited] values as YAML lists instead of storing them as literal strings. Uses YAML safe_load to parse the bracket content, falling back to string on parse failure. +6 tests covering single/multi/empty/quoted items and invalid input.

✅ Looks Good

  • +61/-0 lines with 6 comprehensive tests
  • Clean fallback behavior on invalid YAML
  • Enables config values like hermes config set enabled '[icarus, zeus]'
  • Well-documented with regression reference (#37455)

Reviewed by Hermes Agent

@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 identifying a real config set type-coercion gap. Current main still only coerces booleans and numeric scalars in hermes_cli/config.py:8157-8167, while plugin loading requires plugins.enabled to be a list in hermes_cli/plugins.py:263-266.

Problems

  • The invalid-YAML regression test added at tests/hermes_cli/test_set_config_value.py:280 uses [not: valid: yaml, which has no closing ]. It therefore never enters the new bracket-delimited parsing branch and does not test its fallback behavior.
  • GitHub currently reports this PR as merge-conflicting. The current target has moved to hermes_cli/config.py:8100-8186 and contains newer managed-scope, config-load, alias, and env-bridge behavior that must be retained during salvage.

Suggested changes

  • Transplant the coercion into the current scalar-coercion block at hermes_cli/config.py:8157.
  • Replace the fallback fixture with malformed YAML that still ends in ], then assert it persists as the original string.

Automated hermes-sweeper review.

reloaded = yaml.safe_load(_read_config(_isolated_hermes_home))
assert reloaded["enabled"] == ["icarus", "zeus"]

def test_non_list_bracket_string_kept_as_string(self, _isolated_hermes_home):

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.

This input lacks the closing ], so it never reaches the new startswith('[') and endswith(']') parser branch. Please use malformed YAML that still ends in ] so this test exercises the fallback.

@teknium1

Copy link
Copy Markdown
Contributor

Resolved via PR #88163 (merged) — hermes config set now parses structured list/dict values through yaml.safe_load, gated behind the string-typed-key guard with a conservative trigger. You were the FIRST to submit a fix for this bug (June 2), ahead of the seven later PRs — first-submitter credit is noted in the merged PR body. The merged base was #59182 (@sam7894604), whose variant composed with the string-typed-key guard main grew after your submission. Thanks for surfacing it first!

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

config set: string value overwrites list for 'enabled' key, silently breaking plugin loading

4 participants