fix(config): flag quoted list/mapping values in doctor + startup, guard the list slots config set missed
A list/mapping slot holding ONE quoted string (`plugins:\n enabled: '["a","b"]'`, `model_catalog:\n excluded_providers: '["openai-api"]'`) is skipped by every isinstance-gated reader (`plugins_cmd._config_name_set` -> set(), `plugins._names`, `inventory.excluded_providers` -> []) while `config get` echoes it back, so user plugins silently unmount and provider exclusions silently lapse. Neither `hermes doctor` nor the startup `print_config_warnings` banner said a word. Reader/doctor half: one schema-aware pass in `validate_config_structure` (`_validate_quoted_containers`) walks `DEFAULT_CONFIG` (sections included) plus `_KNOWN_CONTAINER_TYPES` and warns when the user value is a string that parses to a list/mapping. The message names the key, the quoted value and the remedy (`hermes config set <key> '<literal>'`). Finding only: the file is never rewritten. String-typed keys (`approvals.mode: "[off]"`), the `model: <name>` shorthand and the `parse_config_string_list`-read slots (`agent.disabled_toolsets`, `skills.disabled`) are not flagged. Feeds both the doctor "Config Structure" section and the startup banner. Writer half (audit of every path that can put a string in a container slot): - `hermes config set` (`hermes_cli/config.py::set_config_value`): already parses bracket/brace literals (#88163) and refuses wrong-shaped values (`_refuse_container_type_mismatch`), BUT the guard only knew slots present in DEFAULT_CONFIG or `_KNOWN_CONTAINER_TYPES`. `plugins.enabled`, `plugins.disabled` and `model_catalog.excluded_providers` are deliberately absent from DEFAULT_CONFIG, so `config set plugins.enabled foo` / `plugins.enabled a,b` / `model_catalog.excluded_providers openai-api` stored a plain string (live repro on base). Added the three keys to `_KNOWN_CONTAINER_TYPES`: those writes are now refused with the literal hint. - `cli.py::save_config_value` + callers (cli_*_mixin, gateway/slash_commands, gateway/run_busy): every caller passes a bool/enum string for scalar keys; no list-slot caller. Not reachable. - `hermes_cli/plugins_cmd.py::_save_plugin_sets` / `_write_config_value` and `plugins_cmd_catalog` (via `_save_enabled_set`): write `sorted(set)` — real lists. Not reachable. - Dashboard `PUT /api/config` (`web_routers/config_env.py::update_config` -> `_denormalize_config_from_web` -> `save_config`): schema-driven form; `web/src/components/AutoField.tsx` splits list-typed fields into a real array before the PUT. Not reachable. - tui_gateway `config.set`: fixed `_CONFIG_SETTERS` table of scalar keys only (out of this lane's files anyway). Not reachable. Conclusion: the quoted shapes on real machines are leftovers of pre-#88163 `config set` runs plus the DEFAULT_CONFIG-absent slots fixed here. Live repro (fake HOME/HERMES_HOME, both quoted shapes in config.yaml): before: validate_config_structure() -> []; startup banner silent; doctor has no Config Structure section; _config_name_set('plugins','enabled') -> set() after: two warnings naming plugins.enabled / model_catalog.excluded_providers with `hermes config set ... '["a","b"]'`; doctor prints them under Config Structure; running the remedy yields real lists, validate -> [], _config_name_set -> {'a','b'} writer: `config set plugins.enabled a,b` wrote `enabled: a,b` on base; now refused ("must be a list ... nothing was written"). Fixes #83308 Fixes #105706 credit: @fangliquanflq #105725 (schema-aware detection in validate_config_structure; slim redo) credit: @Luna161 #83313 (first report + per-key warning in plugins_cmd)
This commit is contained in:
teknium1
@@ -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"})
|
||||
|
||||
@@ -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, {})
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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 <key> '<literal>'` 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 <key> [--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 <key> <value> [--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 <key> <value> [--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 <key>` | 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. |
|
||||
|
||||
Reference in New Issue
Block a user