fix(config): decode JSON array literals into YAML lists on config set

[thatssoheil]

Summary

hermes config set <allowlist-key> '["-5488240624"]' wrote the JSON literal as a quoted scalar string into config.yaml. Readers that split scalars on commas (_coerce_allow_set, the telegram env bridge) then matched nothing — silently breaking authorization allowlists like gateway.platforms.telegram.group_allowed_chats.

Fixes #76457.

Before

group_allowed_chats: '["-5488240624"]'   # parsed as str, matches nothing

After

group_allowed_chats:
  - '-5488240624'                          # parsed as list, matches

Approach (write side, per the issue's preferred direction)

In set_config_value(), when the schema default is a list and the passed value parses as a JSON array, decode it into a real YAML sequence. All other values keep historical behavior:

• plain scalars → unchanged (string)
• comma strings → unchanged (string; _coerce_allow_set still splits them)
• JSON object literals → unchanged (string; object writes are section writes the command deliberately rejects)
• string-typed defaults → unchanged (never decoded)

Tests

Added TestJsonListValues in tests/hermes_cli/test_set_config_value.py (5 cases): array→list, scalar stays string, comma-string stays string, object stays string, string-typed default keeps literal.

scripts/run_tests.sh tests/hermes_cli/test_set_config_value.py → 87 passed. Also ran test_config.py, test_managed_scope_writeguard.py, test_telegram_auth_check.py, test_telegram_group_gating.py → all green.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pestoura]

The implementation does not currently enforce the scope described in the PR. The new array branch sits inside if not isinstance(_default_value_for_key(key), str), so it runs not only for list-typed settings, but also for bool/int/float defaults and for unknown keys (_default_value_for_key() returns None). Unknown keys are intentionally supported as arbitrary string configuration for skills and external apps, so hermes config set custom.payload '["a"]' now changes from a string to a YAML sequence despite the stated compatibility boundary.

Please gate JSON-array decoding on the schema default actually being a list, e.g. cache the default once and use isinstance(default_value, list). Add regressions for an unknown key and a non-list known key receiving a valid JSON array, confirming both remain strings, alongside the existing list-key case.

[thatssoheil]

Thanks for the sharp catch — you are right that the first version decoded JSON arrays for unknown keys (default None). Fixed in the current head (b9b2b7430):

Refined gate in set_config_value(): JSON-array decoding now runs only when the key is list-typed:

1. schema default is a list (e.g. fallback_providers, terminal.shell_init_files), or
2. the existing user config already holds a list at that path, or
3. the key lives under a platform container (gateway.platforms.<name>.<field> / platforms.<name>.<field>) — open-dict extras like group_allowed_chats have no DEFAULT_CONFIG entry, but are read by _coerce_allow_set / the telegram env bridge as lists.

Unknown keys (custom.payload → default None, never a list) and non-list known defaults (telegram.reactions → bool) keep their historical stringification.

New regressions (in TestJsonListValues):

• test_unknown_key_with_json_array_stays_a_string — custom.payload with ["a","b"] stays a string
• test_non_list_default_with_json_array_stays_a_string — telegram.reactions with ["a","b"] stays a string

Verified: 89/89 in test_set_config_value.py, 110/110 across adjacent config/authz suites, ruff clean, and a live CLI check confirms all three cases (list key → list; unknown key → string; bool-default key → string).

[teknium1]

Thanks for narrowing the JSON decoding gate after the earlier review feedback. The current-main premise is real: hermes_cli/config.py:4869-4878 leaves JSON arrays as scalar strings, while Telegram’s list parser only recognizes actual lists or comma-separated scalars (plugins/platforms/telegram/adapter.py:7820-7827).

Problems

• hermes_cli/config.py:4938 only recognizes list defaults, existing lists, and paths containing platforms. The documented top-level configuration form is telegram.group_allowed_chats (website/docs/user-guide/messaging/telegram.md:138-145); it has no list default in hermes_cli/config_defaults.py:1936-1944 and no platforms segment, so this PR still serializes its JSON array as a scalar.
• _under_platform_container() at hermes_cli/config.py:4655 also matches gateway.platforms.telegram, which is the platform mapping itself rather than a field under it.

Suggested changes

• Cover direct top-level platform fields alongside nested platform fields, and add a regression that loads the result through load_gateway_config().
• Require a field below the platform name before treating a platforms path as array-decodable.

Automated hermes-sweeper review.

[teknium1]

This also returns true for gateway.platforms.telegram (and platforms.telegram), where the path names the platform mapping rather than a field below it. Require a field segment after the platform name before enabling JSON-array decoding.

[teknium1]

This gate misses the documented direct form telegram.group_allowed_chats: it has no list default and no platforms segment, so a fresh JSON-array write remains a scalar. Please cover direct platform config fields and add a gateway-loading regression.

[thatssoheil]

Both points addressed in the current head (9a6abd61b):

1. Top-level platform fields now decode. telegram.group_allowed_chats (the documented form in website/docs/user-guide/messaging/telegram.md, bridged into platform extras by gateway/config.py) now writes a real YAML list. _under_platform_container() treats a top-level telegram.<field> as list-typed when the field is allowlist-shaped (*_allowed_*, *_allow_from, or the known allowlist names) — so telegram.reactions (bool default) and other non-allowlist fields still stay strings.

2. _under_platform_container() now requires a field below the platform name. gateway.platforms.telegram alone (the platform mapping) no longer matches — only ...platforms.<name>.<field> with at least one field below.

New regressions:

• test_top_level_platform_field_written_as_list — telegram.group_allowed_chats → list
• test_platform_mapping_alone_is_not_decoded — gateway.platforms.telegram → stays string
• test_loaded_through_gateway_config_is_a_list — end-to-end: the written YAML list survives load_gateway_config() (via cfg.platforms[Platform.TELEGRAM].to_dict())
• test_non_list_default_with_json_array_stays_a_string — telegram.reactions (bool default) stays string (also guards the top-level branch against over-matching)

Verified: 92/92 in test_set_config_value.py, 110/110 across adjacent config/authz suites, ruff clean, and live CLI checks: top-level list-key → list, nested list-key → list, bool-default → string, unknown key → string.

[thatssoheil]

Addressed the code-review findings in the current head (d463090b5):

Standards axis:

• _lookup_existing deleted — now reuses the existing _get_nested (config.py:1065) with its _MISSING sentinel.
• _under_platform_container renamed to _is_allowlist_platform_field — the name now matches what it does (allowlist-shaped field detection, both nested and top-level forms), and it reuses _PLATFORM_CONTAINER_KEYS instead of a hardcoded "platforms" literal. Allowlist-shape matching is extracted to _looks_like_allowlist_field (single source of truth for the field-name patterns).
• _decode_json_value replaces the boolean array checker — json.loads runs once, not twice.
• test_loaded_through_gateway_config_is_a_list no longer swallows exceptions — a regression in load_gateway_config surfaces instead of silently skipping (and reads the value from PlatformConfig.extra, where it actually lands).
• Test-class docstring now matches the tests (decodes when key is list-typed, not "only when schema default is a list").

Spec axis:

• The spec's { trigger is implemented: a JSON object literal on a mapping-typed key (schema mapping section like terminal, or existing dict at path) becomes a YAML mapping — verified live: hermes config set terminal '{"backend": "local", "cwd": "/repo"}' → dict. This is scoped so unknown keys / non-mapping defaults keep stringification (no regression of the earlier fix).
• The allowlist-field matcher no longer over-matches: gateway.platforms.telegram.reactions and other non-allowlist fields stay strings, consistent between top-level and nested forms.

Verified: 93/93 test_set_config_value.py (incl. new test_json_object_on_mapping_key_written_as_mapping), 110/110 adjacent suites, ruff clean, live CLI matrix all correct (top-level list → list, bool-default → str, unknown → str, mapping {...} → dict, nested list → list).

[GottZ]

This was generated by AI during triage.

Summary

One PR addresses #76457. #76470 targets the reported scalar-string serialization failure by decoding JSON arrays for schema-list, existing-list, and allowlist-shaped platform fields, with regressions for YAML persistence and gateway loading, but its added mapping-key branch exceeds that list-focused scope.

Related pull requests

• fix(config): decode JSON array literals into YAML lists on config set #76470 best fix — (+222/-1) — keep open with a salvage path: the diff fixes the reported nested and documented top-level allowlist paths and addresses the contributor keep_open review’s direct-path and platform-mapping concerns through _is_allowlist_platform_field() and regression tests. However, the mapping-key branch accepts any structured _decode_json_value() result, so a JSON array supplied to a mapping-only key such as terminal becomes a YAML list rather than retaining historical string semantics.

Suggested consolidation

Keep #76470 open with a salvage path, consistent with the contributor keep_open review: retain the list-typed and allowlist-field decoding plus the gateway-loading regressions, but restrict the mapping-key branch to decoded dictionaries and add a regression proving that JSON arrays on mapping-only keys remain strings. There are no duplicate PRs to close.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I76457(["issue #76457 (open)"])
    P76470["PR #76470 (open)"]
    P76470 -->|best fix| I76457
    class I76457 open
    class P76470 open
    class P76470 best
    class P76470 target
    click I76457 "https://github.com/NousResearch/hermes-agent/issues/76457"
    click P76470 "https://github.com/NousResearch/hermes-agent/pull/76470"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 13 kB of PR diffs, 7 kB of issue/PR text, 8 kB of discussion (9 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[thatssoheil]

Addressed the mapping-key over-decode concern from the GottZ triage in 47659a2:

Restricted mapping branch to dicts only: The mapping-key decode path now checks isinstance(decoded, dict) before applying — a JSON array on a mapping-typed key (e.g. terminal) stays a string instead of becoming a YAML list. JSON objects still decode to YAML mappings as intended.

Regression test: New test_json_array_on_mapping_key_stays_a_string proves that hermes config set terminal '["a", "b"]' writes a quoted scalar, not a sequence.

94/94 tests pass.

[thatssoheil]

@pestoura Good catch on the scope concern — I traced through the logic and both cases you flagged are already handled correctly in the current code (47659a2):

1. Unknown keys (default_value = None): The list branch requires isinstance(default_value, list) or isinstance(existing_value, list) or _is_allowlist_platform_field(key) — all three are False for a new unknown key, so it falls through to the mapping branch where isinstance(["a"], dict) is False. Stays a string.

2. Non-list known keys (bool/int/float defaults): Same gate — isinstance(False, list) etc. are all False, so JSON arrays stay strings.

Both cases have regression tests that pass:

• test_unknown_key_with_json_array_stays_a_string — custom.payload '["a", "b"]' → string
• test_non_list_default_with_json_array_stays_a_string — telegram.reactions '["a", "b"]' → string

The GottZ triage fix (restricting the mapping branch to dicts) was the missing piece — without it, a JSON array on a mapping-typed key like terminal would have leaked through. That's now guarded.

[thatssoheil]

@teknium1 Both concerns are already addressed in the current code (47659a2):

1. Top-level telegram.group_allowed_chats: _is_allowlist_platform_field() at line 4717 explicitly handles this — when segments[0] in _SCHEMA_DEFINED_DICT_KEYS and len(segments) > 1, it checks _looks_like_allowlist_field(segments[1]). The test test_top_level_platform_field_written_as_list confirms telegram.group_allowed_chats '["-5488240624"]' decodes to a YAML list.

2. gateway.platforms.telegram false positive: Line 4714 requires idx + 2 < len(segments) — there must be at least one field segment below the platform name. gateway.platforms.telegram alone has exactly 3 segments with platforms at idx=1, so 1 + 2 = 3 is NOT < 3, and it correctly doesn't match.

The GottZ triage fix (this PR's scope) was specifically about restricting the mapping-key branch to dicts only. The allowlist field detection was already correct before this PR.

[thatssoheil]

@teknium1 @pestoura All review findings are addressed in the current head (47659a2) and CI is green (45 checks, 0 failures):

1. JSON-array decode gated on list-typed schema defaults — unknown keys (custom.payload) and non-list known keys stay strings (test_unknown_key_with_json_array_stays_a_string, test_non_list_known_key_stays_a_string).
2. Top-level platform fields decode — telegram.group_allowed_chats (documented form) writes a real YAML list (test_top_level_platform_field_written_as_list); _is_allowlist_platform_field only matches allowlist-shaped fields, so telegram.reactions (bool default) stays a string.
3. Platform-mapping false positive fixed — gateway.platforms.telegram alone no longer matches; a field segment is required below the platform name.
4. Mapping-key branch restricted to dicts — terminal '["a"]' stays a quoted scalar (test_json_array_on_mapping_key_stays_a_string).
5. Gateway-loading regression — test_loaded_through_gateway_config_is_a_list loads the written config through load_gateway_config.

94/94 tests in test_set_config_value.py. Ready for re-review.

[teknium1]

Resolved via PR #88163 (merged) — hermes config set now parses structured list/dict values (yaml.safe_load, string-typed-key guard respected, conservative trigger). This was an 8-PR duplicate cluster: first submitter was @liuhao1024 (#37460), and the merged base was #59182 (@sam7894604) with authorship preserved; all cluster authors are credited in the PR body. Thanks for taking a run at it!