mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
fix(config): warn-after-write for unrecognized keys instead of refusing
Transform of salvaged PR #34250 per maintainer direction: arbitrary config keys are a supported pattern (top-level scalars bridge into os.environ for skills and external apps), so hard-refusing unknown keys would break legitimate writes. Keep the contributor's schema walker and did-you-mean suggestion engine, but write the value first and print a post-write notice — no more bare success for plausible-but-wrong paths like gateway.discord.gateway_restart_notification, and no blocked writes. --force suppresses the notice for scripted use.
This commit is contained in:
parent
3b2e445890
commit
ed3a0b3948
3 changed files with 73 additions and 72 deletions
|
|
@ -8724,9 +8724,9 @@ def set_config_value(key: str, value: str, force: bool = False):
|
|||
Args:
|
||||
key: Dotted config path (e.g. ``terminal.backend``).
|
||||
value: String value (auto-coerced to bool/int/float when matching).
|
||||
force: When True, skip schema validation — useful for setting keys
|
||||
that a newer Hermes version added but this one doesn't know about
|
||||
yet. The CLI exposes this via ``hermes config set --force``.
|
||||
force: When True, skip the unknown-key warning — useful for scripted
|
||||
writes of keys the running version doesn't recognize yet. The CLI
|
||||
exposes this via ``hermes config set --force``.
|
||||
"""
|
||||
if is_managed():
|
||||
managed_error("set configuration values")
|
||||
|
|
@ -8757,28 +8757,14 @@ def set_config_value(key: str, value: str, force: bool = False):
|
|||
print(f"✓ Set {key} in {get_env_path()}")
|
||||
return
|
||||
|
||||
# Schema validation (#34067): refuse to silently write unknown top-level
|
||||
# keys into config.yaml. The previous behavior accepted arbitrary key
|
||||
# paths (e.g. ``gateway.discord.gateway_restart_notification`` instead of
|
||||
# the correct ``discord.gateway_restart_notification``), reported success,
|
||||
# and left the user debugging behavior that hadn't actually changed.
|
||||
# Unknown-key notice (#34067): the key is still written (arbitrary keys
|
||||
# are supported — top-level scalars are bridged into os.environ for
|
||||
# skills and external apps), but a plausible-but-wrong dotted path like
|
||||
# ``gateway.discord.gateway_restart_notification`` previously reported
|
||||
# bare success and left the user debugging behavior that never changed.
|
||||
# Warn after the write so the user gets immediate feedback plus a
|
||||
# "did you mean" hint, without blocking legitimate unknown keys.
|
||||
is_known, suggestion = _validate_config_key(key)
|
||||
if not is_known and not force:
|
||||
print(color(f"✗ Unknown config key: {key}", Colors.RED, Colors.BOLD))
|
||||
if suggestion:
|
||||
print(color(f" Did you mean: {suggestion}", Colors.YELLOW))
|
||||
else:
|
||||
print(color(
|
||||
" No close match found in the known config schema.",
|
||||
Colors.YELLOW,
|
||||
))
|
||||
print()
|
||||
print(
|
||||
" If this is a key your version of Hermes doesn't know about yet "
|
||||
"but a newer version does, bypass this check with:\n"
|
||||
f" hermes config set --force {key} {value}"
|
||||
)
|
||||
sys.exit(2)
|
||||
|
||||
# Otherwise it goes to config.yaml
|
||||
# Read the raw user config (not merged with defaults) to avoid
|
||||
|
|
@ -8846,6 +8832,23 @@ def set_config_value(key: str, value: str, force: bool = False):
|
|||
_display_value = value
|
||||
print(f"✓ Set {key} = {_display_value} in {config_path}")
|
||||
|
||||
# Post-write unknown-key notice (#34067): value IS saved, but tell the
|
||||
# user the runtime may never read it and suggest the likely-intended path.
|
||||
if not is_known and not force:
|
||||
print(color(
|
||||
f"⚠ '{key}' is not a recognized config key — it was saved anyway, "
|
||||
"but Hermes may not read it.",
|
||||
Colors.YELLOW,
|
||||
))
|
||||
if suggestion:
|
||||
print(color(f" Did you mean: {suggestion}", Colors.YELLOW))
|
||||
print(color(
|
||||
" (Custom top-level keys are supported and bridged to the "
|
||||
"environment for skills/external tools. Use --force to skip "
|
||||
"this notice.)",
|
||||
Colors.DIM,
|
||||
))
|
||||
|
||||
|
||||
def get_config_value(key: str, *, as_json: bool = False):
|
||||
"""Print a resolved configuration value."""
|
||||
|
|
@ -8958,7 +8961,7 @@ def config_command(args):
|
|||
print(" hermes config set terminal.backend docker")
|
||||
print(" hermes config set OPENROUTER_API_KEY sk-or-...")
|
||||
print()
|
||||
print(" --force: bypass schema validation for unknown keys")
|
||||
print(" --force: skip the unknown-key notice for unrecognized keys")
|
||||
sys.exit(1)
|
||||
set_config_value(key, value, force=force)
|
||||
|
||||
|
|
|
|||
|
|
@ -43,9 +43,8 @@ def build_config_parser(subparsers, *, cmd_config: Callable) -> None:
|
|||
config_set.add_argument(
|
||||
"--force",
|
||||
action="store_true",
|
||||
help="Bypass schema validation (write unknown keys without warning). "
|
||||
"Use this when setting a key that a newer Hermes version supports "
|
||||
"but the running version doesn't recognize yet.",
|
||||
help="Skip the unknown-key notice printed after writing a key the "
|
||||
"running version doesn't recognize (the value is saved either way).",
|
||||
)
|
||||
|
||||
# config unset
|
||||
|
|
|
|||
|
|
@ -469,86 +469,85 @@ class TestSecretRedactionInDisplay:
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSchemaValidation:
|
||||
"""#34067: ``hermes config set`` must refuse to silently write unknown
|
||||
keys to config.yaml. The headline case in the issue was
|
||||
``gateway.discord.gateway_restart_notification`` being silently accepted
|
||||
even though the correct path is ``discord.gateway_restart_notification``
|
||||
(platform configs live at the top level, not nested under ``gateway``).
|
||||
"""#34067: ``hermes config set`` must not report bare success for
|
||||
unrecognized keys. The key IS written (arbitrary keys are supported —
|
||||
top-level scalars bridge into os.environ for skills/external apps), but
|
||||
a post-write notice warns that Hermes may never read it and suggests the
|
||||
likely-intended path. Headline case: the plausible-but-wrong
|
||||
``gateway.discord.gateway_restart_notification`` (correct path:
|
||||
``discord.gateway_restart_notification``).
|
||||
"""
|
||||
|
||||
def test_unknown_top_level_key_is_refused(self, _isolated_hermes_home, capsys):
|
||||
"""An entirely-unknown top-level key triggers SystemExit(2) with a
|
||||
red error message."""
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
set_config_value("totally_made_up_key", "value")
|
||||
assert exc_info.value.code == 2
|
||||
def test_unknown_top_level_key_written_with_notice(self, _isolated_hermes_home, capsys):
|
||||
"""An unknown top-level key is saved AND a notice is printed."""
|
||||
set_config_value("totally_made_up_key", "value")
|
||||
out = capsys.readouterr().out
|
||||
assert "Unknown config key" in out
|
||||
assert "not a recognized config key" in out
|
||||
assert "totally_made_up_key" in out
|
||||
# And nothing was written to config.yaml.
|
||||
assert "totally_made_up_key" not in _read_config(_isolated_hermes_home)
|
||||
assert "saved anyway" in out
|
||||
# The value WAS written.
|
||||
assert "totally_made_up_key" in _read_config(_isolated_hermes_home)
|
||||
|
||||
def test_unknown_subkey_is_refused(self, _isolated_hermes_home, capsys):
|
||||
"""The headline #34067 bug: ``gateway`` is a valid top-level key but
|
||||
``gateway.discord`` is not a valid sub-key (gateway only has
|
||||
strict/media_delivery_allow_dirs/trust_recent_files/...)."""
|
||||
with pytest.raises(SystemExit) as exc_info:
|
||||
set_config_value("gateway.discord.gateway_restart_notification", "false")
|
||||
assert exc_info.value.code == 2
|
||||
def test_unknown_subkey_written_with_notice(self, _isolated_hermes_home, capsys):
|
||||
"""The headline #34067 path: written, but warned about — no more
|
||||
bare success for gateway.discord.gateway_restart_notification."""
|
||||
set_config_value("gateway.discord.gateway_restart_notification", "false")
|
||||
out = capsys.readouterr().out
|
||||
assert "Unknown config key" in out
|
||||
assert "gateway.discord.gateway_restart_notification" in out
|
||||
# Nothing was written.
|
||||
assert "discord" not in _read_config(_isolated_hermes_home).split("gateway:")[-1] if "gateway:" in _read_config(_isolated_hermes_home) else True
|
||||
assert "✓ Set" in out
|
||||
assert "not a recognized config key" in out
|
||||
|
||||
def test_platforms_container_is_accepted(self, _isolated_hermes_home):
|
||||
def test_platforms_container_is_accepted(self, _isolated_hermes_home, capsys):
|
||||
"""``platforms.<name>.<field>`` is a valid current shape: gateway/
|
||||
config.py resolves a top-level ``platforms`` map in addition to the
|
||||
top-level platform blocks, so it must NOT be refused."""
|
||||
top-level platform blocks, so it must NOT trigger the notice."""
|
||||
set_config_value("platforms.discord.enabled", "true")
|
||||
content = _read_config(_isolated_hermes_home)
|
||||
assert "enabled: true" in content
|
||||
assert "not a recognized config key" not in capsys.readouterr().out
|
||||
|
||||
def test_gateway_platforms_nested_is_accepted(self, _isolated_hermes_home):
|
||||
def test_gateway_platforms_nested_is_accepted(self, _isolated_hermes_home, capsys):
|
||||
"""Docs configure platforms under ``gateway.platforms.<name>`` — the
|
||||
canonical layout must validate as known."""
|
||||
canonical layout must validate as known (no notice)."""
|
||||
set_config_value("gateway.platforms.my_platform.extra.token", "abc")
|
||||
content = _read_config(_isolated_hermes_home)
|
||||
assert "token: abc" in content
|
||||
assert "not a recognized config key" not in capsys.readouterr().out
|
||||
|
||||
def test_unknown_approvals_subkey_is_refused(self, _isolated_hermes_home, capsys):
|
||||
"""``approvals`` is a defined schema, so a typo'd sub-key must be
|
||||
rejected rather than silently written."""
|
||||
with pytest.raises(SystemExit):
|
||||
set_config_value("approvals.notarealkey", "true")
|
||||
def test_unknown_approvals_subkey_warns_but_writes(self, _isolated_hermes_home, capsys):
|
||||
"""``approvals`` is a defined schema, so a typo'd sub-key gets the
|
||||
notice — but is still written."""
|
||||
set_config_value("approvals.notarealkey", "true")
|
||||
out = capsys.readouterr().out
|
||||
assert "not a recognized config key" in out
|
||||
assert "notarealkey" in _read_config(_isolated_hermes_home)
|
||||
|
||||
def test_known_approvals_subkey_is_accepted(self, _isolated_hermes_home):
|
||||
"""Real ``approvals.*`` keys still validate."""
|
||||
def test_known_approvals_subkey_is_accepted(self, _isolated_hermes_home, capsys):
|
||||
"""Real ``approvals.*`` keys still validate silently."""
|
||||
set_config_value("approvals.mode", "off")
|
||||
import yaml
|
||||
saved = yaml.safe_load(_read_config(_isolated_hermes_home))
|
||||
assert saved["approvals"]["mode"] == "off"
|
||||
assert "not a recognized config key" not in capsys.readouterr().out
|
||||
|
||||
def test_close_typo_suggests_correct_key(self, _isolated_hermes_home, capsys):
|
||||
"""Typo'd top-level keys should get a fuzzy-match suggestion."""
|
||||
with pytest.raises(SystemExit):
|
||||
set_config_value("disco", "false")
|
||||
set_config_value("disco", "false")
|
||||
out = capsys.readouterr().out
|
||||
assert "Did you mean" in out
|
||||
assert "discord" in out
|
||||
|
||||
def test_typoed_subkey_suggests_sibling(self, _isolated_hermes_home, capsys):
|
||||
"""``agent.max_turn`` should suggest ``agent.max_turns``."""
|
||||
with pytest.raises(SystemExit):
|
||||
set_config_value("agent.max_turn", "100")
|
||||
set_config_value("agent.max_turn", "100")
|
||||
out = capsys.readouterr().out
|
||||
assert "agent.max_turns" in out
|
||||
|
||||
def test_force_bypasses_validation(self, _isolated_hermes_home):
|
||||
"""``--force`` allows writing unknown keys (forward-compat with
|
||||
config keys that a newer Hermes version adds)."""
|
||||
# Should not raise.
|
||||
def test_force_suppresses_notice(self, _isolated_hermes_home, capsys):
|
||||
"""``--force`` writes unknown keys without the notice (scripted
|
||||
forward-compat writes)."""
|
||||
set_config_value("brand_new_future_key", "value", force=True)
|
||||
out = capsys.readouterr().out
|
||||
assert "not a recognized config key" not in out
|
||||
# And the value WAS written.
|
||||
content = _read_config(_isolated_hermes_home)
|
||||
assert "brand_new_future_key" in content
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue