diff --git a/hermes_cli/config.py b/hermes_cli/config.py index f2f0a17bb2..02858f0ca2 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -16,6 +16,7 @@ import logging import os import platform import re +import shlex import shutil import stat import subprocess @@ -1217,6 +1218,48 @@ def _validate_web_backends(config: Dict[str, Any], issues: List[ConfigIssue]) -> "Run 'hermes tools' and pick a different Web Search & Extract provider") +def _container_slots() -> Dict[str, str]: + """Dotted key -> ``"list"``/``"mapping"`` for every slot the schema fixes to a container: + ``DEFAULT_CONFIG`` (sections included) plus the known-container table for roots it omits.""" + slots: Dict[str, str] = {} + + def walk(node: Dict[str, Any], prefix: str) -> None: + for key, value in node.items(): + path = f"{prefix}.{key}" if prefix else key + if isinstance(value, dict): + slots[path] = "mapping" + walk(value, path) + elif isinstance(value, list): + slots[path] = "list" + + walk(DEFAULT_CONFIG, "") + slots.update(_KNOWN_CONTAINER_TYPES) + return slots + + +def _validate_quoted_containers(config: Dict[str, Any], issues: List[ConfigIssue]) -> None: + """A container slot holding ONE quoted string (``enabled: '["a","b"]'``) is skipped by every + isinstance-gated reader while ``config get`` echoes it back, so plugins silently unmount and + exclusions silently lapse (#83308, #105706). Finding only — the file is never rewritten.""" + for key, kind in _container_slots().items(): + # ``parse_config_string_list`` readers accept the quoted form; nothing is ignored there. + if key in _SCALAR_AS_ONE_ITEM_LIST_KEYS: + continue + value = cfg_get(config, *key.split(".")) + if not isinstance(value, str) or not _looks_structured_value(value): + continue + try: + parsed = yaml.safe_load(value) + except yaml.YAMLError: + continue + if isinstance(parsed, (list, dict)): + _issue(issues, "warning", + f"{key} is the quoted string {value!r} — Hermes expects a YAML {kind} here " + "and every reader ignores the string", + f"Run: hermes config set {key} {shlex.quote(value)} (stores a real {kind}), " + "or remove the quotes in config.yaml") + + def validate_config_structure(config: Optional[Dict[str, Any]] = None) -> List["ConfigIssue"]: """Validate config.yaml structure and return detected issues (accepts a pre-loaded dict). Catches common YAML mistakes that otherwise surface as confusing runtime errors.""" @@ -1256,6 +1299,7 @@ def validate_config_structure(config: Optional[Dict[str, Any]] = None) -> List[" f"Move '{key}' under the appropriate section") _validate_web_backends(config, issues) + _validate_quoted_containers(config, issues) return issues @@ -3254,6 +3298,11 @@ _KNOWN_CONTAINER_TYPES = { "providers": "mapping", "model.aliases": "mapping", "model_aliases": "mapping", + # Omitted from DEFAULT_CONFIG on purpose (an empty default would clobber a user allow-list), + # so without these rows `config set plugins.enabled foo` stored a string every reader ignored. + "plugins.enabled": "list", + "plugins.disabled": "list", + "model_catalog.excluded_providers": "list", } # List slots whose readers go through ``parse_config_string_list``: a bare name is one entry. _SCALAR_AS_ONE_ITEM_LIST_KEYS = frozenset({"agent.disabled_toolsets", "skills.disabled"}) diff --git a/tests/hermes_cli/test_config_set_list_values.py b/tests/hermes_cli/test_config_set_list_values.py index 2ee932618c..3c81e23421 100644 --- a/tests/hermes_cli/test_config_set_list_values.py +++ b/tests/hermes_cli/test_config_set_list_values.py @@ -161,3 +161,16 @@ def test_round_trip_through_load_config(user_home): set_config_value("platform_toolsets.line", '["clarify", "file", "web"]') cfg = load_config() assert cfg["platform_toolsets"]["line"] == ["clarify", "file", "web"] + + +def test_bare_string_into_list_slot_absent_from_defaults_is_refused(user_home, capsys): + """`plugins.enabled` / `model_catalog.excluded_providers` are omitted from DEFAULT_CONFIG, so the + container guard did not know them and `config set plugins.enabled a,b` stored a string every + isinstance(list) reader ignored (#83308, #105706).""" + from hermes_cli.config import set_config_value, read_raw_config + + for key in ("plugins.enabled", "model_catalog.excluded_providers"): + with pytest.raises(SystemExit): + set_config_value(key, "a,b") + assert "must be a list" in capsys.readouterr().err + assert read_raw_config() in (None, {}) diff --git a/tests/hermes_cli/test_config_validation.py b/tests/hermes_cli/test_config_validation.py index 9e6081566f..cabf72cd8d 100644 --- a/tests/hermes_cli/test_config_validation.py +++ b/tests/hermes_cli/test_config_validation.py @@ -193,3 +193,27 @@ class TestUnknownTopLevelKeys: assert any("base_url" in i.message for i in misplaced) assert any("api_key" in i.message for i in misplaced) + + +class TestQuotedContainerValues: + """A list/mapping slot holding one quoted string is ignored by every reader (#83308, #105706).""" + + def test_quoted_list_in_container_slot_is_flagged_with_remedy(self): + issues = validate_config_structure({ + "plugins": {"enabled": '["a","b"]'}, + "model_catalog": {"excluded_providers": '["openai-api"]'}, + }) + flagged = {i.message.split(" ", 1)[0]: i for i in issues if "quoted string" in i.message} + assert set(flagged) == {"plugins.enabled", "model_catalog.excluded_providers"} + assert "hermes config set plugins.enabled '[\"a\",\"b\"]'" in flagged["plugins.enabled"].hint + + def test_string_typed_and_tolerant_slots_are_not_flagged(self): + """`approvals.mode` is a string in the schema; `model: name` is the documented shorthand; + `agent.disabled_toolsets` readers parse the quoted form themselves.""" + issues = validate_config_structure({ + "approvals": {"mode": "[off]"}, + "model": "gpt-4o", + "agent": {"disabled_toolsets": '["web"]'}, + "plugins": {"enabled": ["a"]}, + }) + assert not [i for i in issues if "quoted string" in i.message] diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index 48fe123aae..1f01c6c217 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -978,6 +978,8 @@ Custom-endpoint config checks (both warn-only; `--fix` does not rewrite them): - `custom_providers` that is not a YAML list (for example a string left by a bad `config set`) is reported as an error naming the key and the received type — the runtime ignores every custom endpoint until it is a list again. - A legacy `custom_providers` list entry with no matching `providers:` entry (same endpoint URL) is reported with the move to make: such an entry is still served from the retired list store (the model picker and the Custom Endpoints page dual-read it) rather than the `providers:` map every other surface edits, and the one-shot v12 migration that moved the list into `providers:` does not run again. +**Config Structure** also flags any list/mapping setting stored as one quoted string (`plugins.enabled: '["a","b"]'`, `model_catalog.excluded_providers: '["openai-api"]'` — the shape older `config set` versions wrote): every reader ignores such a string, so the plugins silently stay unmounted and the exclusion never applies. The finding names the key and the `hermes config set ''` command that stores a real list; the same warning appears in the startup banner. `--fix` does not rewrite the file. + ## `hermes dump` ```bash @@ -1324,7 +1326,7 @@ Subcommands: | `show` | Show current config values. | | `edit` | Open `config.yaml` in your editor. | | `get [--json] [--raw]` | Print a single config value by dotted key (e.g. `hermes config get model.default`). `--json` emits machine-readable output. Credential-shaped values (`api_key`, `*_TOKEN`, `*_SECRET`, `password`, …) are masked (`sk-o...7890`) because the agent runs this from sessions whose transcripts persist; `--raw` prints the real value (or set `security.redact_secrets: false`). A nested key under a known section that the schema does not define (`compression.compressor.enabled`) still prints its file value, plus a stderr notice that Hermes may not read it; stdout and the exit code (0) are unchanged. | -| `set [--force]` | Set a config value. Dotted paths go to `config.yaml`; every `UPPER_SNAKE` name (`OPENROUTER_API_KEY`, `DISCORD_HOME_CHANNEL`, `TELEGRAM_GROUP_ALLOWED_USERS`, `HERMES_TIMEZONE`, …) is an environment variable and goes to `.env` — the same file the platform setup flows and `/sethome` write, and the one every runtime reader resolves against. `config set` never writes an `UPPER_SNAKE` key into `config.yaml`, `--force` included; names on the env writer's denylist (`HERMES_YOLO_MODE`, `PATH`, …) are refused outright; any other `UPPER_SNAKE` name is saved to `.env` as-is (plugins, skills and external tools read it from the process environment). A known key written under the wrong prefix (`gateway.discord.foo`, where `discord.foo` is itself a known key) is refused with a did-you-mean and nothing is written; any other unknown path under a known section (`agent.max_turnz`, or a runtime-read key that has no seeded default) is written with a did-you-mean notice, and an unknown lowercase *top-level* key is written with a notice (top-level scalars are bridged into the environment for skills). `--force` writes the refused wrong-prefix path too. Values are type-checked against the schema: a key that must hold a list or a mapping (`custom_providers`, `model.aliases`, `display.platforms`, any key already holding one) refuses a plain string or a wrong-shaped literal, and a value that looks like a list/mapping but is not valid YAML/JSON is refused instead of being stored as a string — nothing is written and the error names the expected type. Pass a YAML/JSON literal (`hermes config set custom_providers '[{name: x, base_url: https://...}]'`); to store a string that merely starts with `[` or `{`, quote it in YAML (`"'[text'"`). `--force` still replaces a whole mapping section; a non-list in a list slot has no override, except that a bare name for a list of names read leniently (`agent.disabled_toolsets`, `skills.disabled`) is stored as a one-item list. | +| `set [--force]` | Set a config value. Dotted paths go to `config.yaml`; every `UPPER_SNAKE` name (`OPENROUTER_API_KEY`, `DISCORD_HOME_CHANNEL`, `TELEGRAM_GROUP_ALLOWED_USERS`, `HERMES_TIMEZONE`, …) is an environment variable and goes to `.env` — the same file the platform setup flows and `/sethome` write, and the one every runtime reader resolves against. `config set` never writes an `UPPER_SNAKE` key into `config.yaml`, `--force` included; names on the env writer's denylist (`HERMES_YOLO_MODE`, `PATH`, …) are refused outright; any other `UPPER_SNAKE` name is saved to `.env` as-is (plugins, skills and external tools read it from the process environment). A known key written under the wrong prefix (`gateway.discord.foo`, where `discord.foo` is itself a known key) is refused with a did-you-mean and nothing is written; any other unknown path under a known section (`agent.max_turnz`, or a runtime-read key that has no seeded default) is written with a did-you-mean notice, and an unknown lowercase *top-level* key is written with a notice (top-level scalars are bridged into the environment for skills). `--force` writes the refused wrong-prefix path too. Values are type-checked against the schema: a key that must hold a list or a mapping (`custom_providers`, `model.aliases`, `display.platforms`, `plugins.enabled`/`plugins.disabled`, `model_catalog.excluded_providers`, any key already holding one) refuses a plain string or a wrong-shaped literal, and a value that looks like a list/mapping but is not valid YAML/JSON is refused instead of being stored as a string — nothing is written and the error names the expected type. Pass a YAML/JSON literal (`hermes config set custom_providers '[{name: x, base_url: https://...}]'`); to store a string that merely starts with `[` or `{`, quote it in YAML (`"'[text'"`). `--force` still replaces a whole mapping section; a non-list in a list slot has no override, except that a bare name for a list of names read leniently (`agent.disabled_toolsets`, `skills.disabled`) is stored as a one-item list. | | `unset ` | Remove a config key, reverting it to the built-in default. For `UPPER_SNAKE` names this removes the `.env` entry and also drops a stale top-level `config.yaml` copy left by older `config set` runs (`get` reports such a copy as stale). | | `path` | Print the config file path. | | `env-path` | Print the `.env` file path. |