Skip to content

fix(cli): parse JSON object values in config set - #40546

Open
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix-config-set-object-values
Open

fix(cli): parse JSON object values in config set#40546
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix-config-set-object-values

Conversation

@izumi0uu

@izumi0uu izumi0uu commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes hermes config set so JSON object/array CLI values are persisted as structured YAML mappings/lists instead of quoted strings.

This addresses a real configuration workflow where provider blocks such as providers.deepseek were being written with the wrong type, even though the CLI printed a success message.

Related Issue

Fixes #40545

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added json-based structured value coercion in hermes_cli/config.py
  • Preserved existing scalar coercion behavior for booleans / ints / floats
  • Only parse values that look like JSON objects ({...}) or arrays ([...])
  • Fall back to the raw string if JSON parsing fails
  • Added regression tests for:
    • generic object values (foo.bar '{"x":1}')
    • real provider blocks (providers.deepseek '{...}')
    • invalid object-like strings staying as strings

How to Test

  1. Reproduce the original bug on main:

    export HERMES_HOME="$(mktemp -d)"
    hermes config set providers.deepseek '{"base_url":"https://api.deepseek.com","key_env":"DEEPSEEK_API_KEY","api_mode":"chat_completions"}'
    cat "$HERMES_HOME/config.yaml"

    Before this fix, providers.deepseek is written as a quoted JSON string.

  2. Run the regression test file:

    scripts/run_tests.sh tests/hermes_cli/test_set_config_value.py
  3. Verify the fixed behavior:

    export HERMES_HOME="$(mktemp -d)"
    hermes config set providers.deepseek '{"base_url":"https://api.deepseek.com","key_env":"DEEPSEEK_API_KEY","api_mode":"chat_completions"}'
    cat "$HERMES_HOME/config.yaml"

    After this fix, providers.deepseek is a YAML mapping with base_url, key_env, and api_mode fields.

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/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.7.7

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

Screenshots / Logs

Relevant local verification:

scripts/run_tests.sh tests/hermes_cli/test_set_config_value.py
=== Summary: 1 files, 35 tests passed, 0 failed (100% complete) ===

@Morad37

Morad37 commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

In _coerce_cli_value, the json.loads() call is wrapped in a bare except Exception which means a RecursionError, MemoryError, or even a KeyboardInterrupt during parsing would all silently fall back to the raw string. A practical concern: a deeply nested JSON payload could hit Python's recursion limit and get silently stored as a string instead of raising clearly.

One other thing I noticed -- the prefix/suffix check uses startswith('{') and endswith('}') on the stripped value. If someone passes a top-level JSON primitive (e.g. just a number as a string, though the scalar rules would catch that), or a JSON object with leading/trailing whitespace only on the outer braces, it works fine. But something like {\n} (newline between braces) would also pass through json.loads and return an empty dict, which seems reasonable.

Not a blocker, just something to be aware of if edge cases around extremely nested config values come up.

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

Review: PR #40546 -- fix(cli): parse JSON object values in config set

Summary: Adds structured value parsing to "hermes config set" so JSON objects and arrays passed as CLI values are properly deserialized into YAML mappings/lists rather than stored as raw strings. Includes tests.

What works well:

  • The _coerce_cli_value helper is well-designed: it preserves the existing bool/int/float coercion first, then attempts JSON parsing only for {...} and [...] strings.
  • Uses json.loads (safe) -- not eval.
  • Graceful fallback: if JSON parsing fails, the raw string is returned unchanged. No crashes on invalid input.
  • Tests are included covering three scenarios: valid nested object, provider block object, and invalid JSON fallback. All tests properly verify type (isinstance) as well as value.
  • The docstring clearly explains the coercion order and fallback behavior.

Considerations:

  • Nested function definition: _coerce_cli_value is defined inside set_config_value, meaning it is recreated on every call. A module-level function would be more efficient, but this is a negligible concern for a CLI command that runs once per invocation.
  • Float detection fragility (pre-existing): The raw.replace(".", "", 1).isdigit() check can incorrectly classify strings like "1.2.3" or "..." as floats. This is existing code, not introduced here, but worth noting that json.loads would handle numeric parsing correctly if the coercion order were reconsidered in the future.

Verdict: Solid fix with good test coverage. Approve.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The underlying bug is still present in the inspected checkout: hermes_cli/config.py:8157-8167 performs only scalar coercion before writing.

Problems

  • The later string-type preservation change, e4ea0a0ed7fc24761b2b425146893561a73216e1, modifies the same coercion block. This PR's unconditional JSON parsing needs to preserve that newer invariant for settings whose declared default is str.
  • The PR supports arrays, but the added tests only cover mappings and malformed object-like input. The linked [Bug]: Hermes config set writes JSON object values as strings in config.yaml #40545 discussion also documents the array/MCP-args case.

Suggested changes

  • Integrate JSON coercion with the string-typed leaf guard from e4ea0a0ed7fc24761b2b425146893561a73216e1.
  • Add a JSON-array round-trip test for mcp_servers.<name>.args and assert the reloaded YAML value is a list.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@izumi0uu
izumi0uu force-pushed the fix-config-set-object-values branch from 1c0fddf to 129e616 Compare July 25, 2026 14:55
@izumi0uu

Copy link
Copy Markdown
Contributor Author

Addressed in 129e616. Integrated structured JSON coercion with the newer string-typed leaf guard: settings declared as str in DEFAULT_CONFIG now preserve their raw value, including JSON-shaped strings, while non-string and dynamic settings retain scalar coercion and can parse JSON objects or arrays. Malformed object-like input still falls back to the original string.
Added the requested MCP array round-trip regression for mcp_servers.filesystem.args, confirming that a JSON array is persisted and reloaded from YAML as a Python list. The existing mapping, provider-block, malformed JSON, scalar-coercion, and string-preservation cases remain covered.

@daerias

daerias commented Jul 29, 2026

Copy link
Copy Markdown

Thanks for working on this, @izumi0uu — this bug has been silently corrupting our config for weeks and we only traced it back to hermes config set today.

Affected version: Hermes Agent v0.19.0 (2026.7.20, bcb352ee)

Config keys we've seen corrupted by this:

Key What hermes config set wrote What YAML should look like
ollama.models '["qwen3.5:latest", ...]' (quoted JSON string) YAML sequence
terminal.shell_init_files '["~/.hermes/fal-env.sh", ...]' YAML sequence
skills.disabled '["medication-guide", ...]' YAML sequence (17 skill names)
hooks.pre_llm_call '[{"command":"...","timeout":5}]' YAML list of mappings

The hooks.pre_llm_call case was the most painful — the hook silently stopped firing because Hermes parsed the value as a literal string instead of a structured list. Took us a while to figure out why our temporal-grounding hook wasn't injecting.

Workaround we built (for anyone else hitting this): A small Python script that detects quoted JSON strings in config.yaml and rewrites them as proper YAML sequences/mappings, triggered via a macOS launchd WatchPaths agent so it auto-fixes after every hermes config set call. Happy to share if anyone wants it, but obviously this PR is the real fix.

Really hoping this lands soon — it's a silent data-corruption bug that's hard to diagnose unless you know what to look for. Thanks again for the fix.

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.

[Bug]: Hermes config set writes JSON object values as strings in config.yaml

6 participants