mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-12 13:52:15 +00:00
fix(config): route every migration write through one default-stripping chokepoint
A single 'hermes update' / 'hermes -p' could rewrite a hand-curated config.yaml
into a near-full DEFAULT_CONFIG dump (the 'you blow up my profile config on one
tweak' reports). Root cause: migrate_config() had ~16 independent save_config()
call sites, each author deciding ad hoc whether to materialise a value, and many
persisted pure schema defaults with strip_defaults=False. Defaults already merge
transparently at read time via load_config(), so writing them is pure bloat that
also shadows future default changes (see save_config's docstring).
Architectural fix (not a per-site patch): introduce a single _persist_migration()
chokepoint that enforces one invariant — a migration may persist only values that
DIFFER from the current schema default, plus explicit removals/renames of user
data; pure defaults are never written. Every migration write (all 17 sites incl.
the version-bump finalizer) now routes through it. The invariant is mechanically
correct for all cases and verified empirically:
- pure-default seeds (timezone='', curator/auxiliary.curator blocks, interim
flag, curator.consolidate=False, empty plugins.enabled) are stripped → merged
in at read time;
- non-default values (write_approval=True, model_catalog.ttl_hours=1) preserved
via explicit-raw-path preservation;
- behaviour flips (agent.verify_on_stop=False, schema default still 'auto')
preserved because False != 'auto';
- data transforms (custom_providers->providers, stt.model relocation,
write_mode->write_approval, compression.summary_* removal, MCP-disable)
persist their removals/renames.
An explicitly user-set non-default value (e.g. matrix.require_mention: false) is
preserved across the bump.
Guard tests lock the architecture: an AST check asserts migrate_config() makes no
direct save_config() call (all writes go through _persist_migration), and a
full-range v1->latest test asserts a lean config is never dumped. Two existing
change-detector tests that froze the on-disk representation of default-valued
keys are rewritten to assert the effective value via load_config() (behaviour
contract, not snapshot).
Validation: lean v1->latest migration drops from ~567 bytes to ~196 bytes;
148 config+setup and 196 profile/curator/migrate tests pass on scripts/run_tests.sh.
This commit is contained in:
kshitijk4poor
parent
a5e8cd4d40
commit
c717be8ded
2 changed files with 158 additions and 67 deletions
|
|
@ -4975,6 +4975,29 @@ def warn_deprecated_cwd_env_vars(config: Optional[Dict[str, Any]] = None) -> Non
|
|||
sys.stderr.write("\n".join(lines) + "\n\n")
|
||||
|
||||
|
||||
def _persist_migration(config: Dict[str, Any]) -> None:
|
||||
"""Persist a migrated config under the migration write invariant.
|
||||
|
||||
THE INVARIANT (single source of truth for the whole migration pipeline):
|
||||
a migration may only persist values that DIFFER from the current schema
|
||||
default, plus explicit removals/renames of user data. Pure schema defaults
|
||||
are never materialised to disk — ``load_config()``'s deep-merge supplies
|
||||
them at read time, so writing them adds nothing and actively shadows future
|
||||
default changes (see ``save_config``'s docstring). Materialising defaults on
|
||||
every version bump is what rewrote hand-curated configs into full
|
||||
DEFAULT_CONFIG dumps (the "hermes update / hermes -p blows up my config"
|
||||
reports).
|
||||
|
||||
Every migration step MUST route its write through this helper instead of
|
||||
calling ``save_config`` directly. It is a thin wrapper over
|
||||
``save_config(config)`` (default-stripping ON); centralising the call makes
|
||||
the invariant impossible to regress one migration at a time. Correctness
|
||||
across seeds, non-default values, behaviour flips, and data transforms is
|
||||
verified by the migration parity tests.
|
||||
"""
|
||||
save_config(config)
|
||||
|
||||
|
||||
def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, Any]:
|
||||
"""
|
||||
Migrate config to latest version, prompting for new required fields.
|
||||
|
|
@ -5018,7 +5041,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
display["tool_progress"] = "all"
|
||||
results["config_added"].append("display.tool_progress=all (default)")
|
||||
config["display"] = display
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
print(f" ✓ Migrated tool progress to config.yaml: {display['tool_progress']}")
|
||||
|
||||
|
|
@ -5033,7 +5056,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
else:
|
||||
config["timezone"] = ""
|
||||
results["config_added"].append("timezone= (empty, uses server-local)")
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
tz_display = config["timezone"] or "(server-local)"
|
||||
print(f" ✓ Added timezone to config.yaml: {tz_display}")
|
||||
|
|
@ -5107,7 +5130,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
config["providers"] = providers_dict
|
||||
# Remove the old list — runtime reads via get_compatible_custom_providers()
|
||||
config.pop("custom_providers", None)
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
print(f" ✓ Migrated {migrated_count} custom provider(s) to providers: section")
|
||||
for key in list(providers_dict.keys())[-migrated_count:]:
|
||||
|
|
@ -5175,7 +5198,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
provider_cfg = stt.setdefault(provider, {})
|
||||
provider_cfg["model"] = legacy_model
|
||||
config["stt"] = stt
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
print(f" ✓ Migrated legacy stt.model to provider-specific config")
|
||||
|
||||
|
|
@ -5189,7 +5212,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
display["interim_assistant_messages"] = True
|
||||
config["display"] = display
|
||||
results["config_added"].append("display.interim_assistant_messages=true (default)")
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
print(" ✓ Added display.interim_assistant_messages=true")
|
||||
|
||||
|
|
@ -5211,7 +5234,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
platforms[plat]["tool_progress"] = mode
|
||||
display["platforms"] = platforms
|
||||
config["display"] = display
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
migrated = ", ".join(f"{p}={m}" for p, m in old_overrides.items())
|
||||
print(f" ✓ Migrated tool_progress_overrides → display.platforms: {migrated}")
|
||||
|
|
@ -5247,7 +5270,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
migrated_keys.append(f"base_url={s_base_url}")
|
||||
if migrated_keys or s_model is not None or s_provider is not None or s_base_url is not None:
|
||||
config["compression"] = comp
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
if migrated_keys:
|
||||
print(f" ✓ Migrated compression.summary_* → auxiliary.compression: {', '.join(migrated_keys)}")
|
||||
|
|
@ -5303,7 +5326,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
|
||||
plugins_cfg["enabled"] = grandfathered
|
||||
config["plugins"] = plugins_cfg
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
results["config_added"].append(
|
||||
f"plugins.enabled (opt-in allow-list, {len(grandfathered)} grandfathered)"
|
||||
)
|
||||
|
|
@ -5383,15 +5406,15 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
touched = True
|
||||
|
||||
if touched:
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if added_curator:
|
||||
results["config_added"].append(
|
||||
f"curator ({len(added_curator)} default key(s))"
|
||||
)
|
||||
if not quiet:
|
||||
print(
|
||||
" ✓ Seeded curator defaults in config.yaml: "
|
||||
f"{', '.join(added_curator)}"
|
||||
" ✓ Curator settings now available "
|
||||
f"({', '.join(added_curator)}) — edit via `hermes config set`"
|
||||
)
|
||||
if added_aux:
|
||||
results["config_added"].append(
|
||||
|
|
@ -5399,8 +5422,8 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
)
|
||||
if not quiet:
|
||||
print(
|
||||
" ✓ Seeded auxiliary.curator defaults in config.yaml: "
|
||||
f"{', '.join(added_aux)}"
|
||||
" ✓ auxiliary.curator settings now available "
|
||||
f"({', '.join(added_aux)}) — edit via `hermes config set`"
|
||||
)
|
||||
|
||||
# ── Version 24 → 25: lower model_catalog TTL 24h → 1h ──
|
||||
|
|
@ -5414,7 +5437,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
if isinstance(raw_mc, dict) and raw_mc.get("ttl_hours") == 24:
|
||||
raw_mc["ttl_hours"] = 1
|
||||
config["model_catalog"] = raw_mc
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
results["config_added"].append("model_catalog.ttl_hours 24→1")
|
||||
if not quiet:
|
||||
print(" ✓ Lowered model_catalog.ttl_hours to 1 (hourly picker refresh)")
|
||||
|
|
@ -5443,32 +5466,18 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
f"{subsystem}.write_mode → write_approval={sub['write_approval']}"
|
||||
)
|
||||
if touched:
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
if not quiet:
|
||||
print(" ✓ Renamed write_mode → write_approval (boolean gate)")
|
||||
|
||||
# ── Version 29 → 30: seed curator.consolidate (default false) ──
|
||||
# Consolidation (the LLM umbrella-building fork) is now an opt-in toggle,
|
||||
# OFF by default. The deterministic inactivity prune still runs whenever
|
||||
# the curator is enabled; only the opinionated, aux-model-cost LLM pass is
|
||||
# gated. The runtime deep-merge already supplies the default, but we seed
|
||||
# the key so it's visible/editable in config.yaml. Existing installs that
|
||||
# WANT the old always-consolidate behavior must set it to true explicitly.
|
||||
# Only add the key when a curator section exists and lacks it — never
|
||||
# clobber a value the user already set.
|
||||
if current_ver < 30:
|
||||
config = read_raw_config()
|
||||
raw_curator = config.get("curator")
|
||||
if isinstance(raw_curator, dict) and "consolidate" not in raw_curator:
|
||||
raw_curator["consolidate"] = False
|
||||
config["curator"] = raw_curator
|
||||
save_config(config, strip_defaults=False)
|
||||
results["config_added"].append("curator.consolidate=false")
|
||||
if not quiet:
|
||||
print(
|
||||
" ✓ Seeded curator.consolidate: false "
|
||||
"(LLM consolidation is now opt-in; pruning stays on)"
|
||||
)
|
||||
# ── Version 29 → 30: curator.consolidate defaults to false ──
|
||||
# Consolidation (the LLM umbrella-building fork) is opt-in, OFF by default;
|
||||
# the deterministic inactivity prune still runs whenever the curator is
|
||||
# enabled. No write is needed: the schema default (curator.consolidate=false)
|
||||
# is supplied by load_config()'s deep-merge at read time, and persisting a
|
||||
# default-valued key would only bloat a lean config (it gets stripped on
|
||||
# save anyway). Existing installs that WANT the old always-consolidate
|
||||
# behavior set it to true explicitly via `hermes config set`.
|
||||
|
||||
# ── Version 30 → 31: switch verify_on_stop OFF (one-time) ──
|
||||
# verify_on_stop defaulted to the "auto" sentinel (surface-aware: on for
|
||||
|
|
@ -5491,7 +5500,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
if cur is None or is_auto_sentinel:
|
||||
raw_agent["verify_on_stop"] = False
|
||||
config["agent"] = raw_agent
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
results["config_added"].append("agent.verify_on_stop=false")
|
||||
if not quiet:
|
||||
print(
|
||||
|
|
@ -5518,7 +5527,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
if isinstance(raw_agent, dict) and raw_agent.get("verify_on_stop") is True:
|
||||
raw_agent["verify_on_stop"] = False
|
||||
config["agent"] = raw_agent
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
results["config_added"].append("agent.verify_on_stop=false")
|
||||
if not quiet:
|
||||
print(
|
||||
|
|
@ -5558,7 +5567,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
print(f" ⚠ Disabled MCP server '{server_name}' pending review")
|
||||
if mcp_touched:
|
||||
config["mcp_servers"] = raw_mcp_servers
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
|
||||
# ── Always: validate platform_toolsets after migration ──
|
||||
# A migration (or hand-edit) that leaves an invalid toolset name in
|
||||
|
|
@ -5662,29 +5671,21 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
else:
|
||||
print(" Set later with: hermes config set <key> <value>")
|
||||
|
||||
# Check for missing config fields
|
||||
# Check for missing config fields.
|
||||
#
|
||||
# New default keys are NOT materialised to disk: load_config() deep-merges
|
||||
# DEFAULT_CONFIG at read time, so a missing key already takes effect with
|
||||
# its default (see _persist_migration's invariant). We surface the list for
|
||||
# the informational "N new config option(s) available" display in
|
||||
# `hermes update`, but only the version bump is persisted.
|
||||
missing_config = get_missing_config_fields()
|
||||
|
||||
if missing_config:
|
||||
config = read_raw_config()
|
||||
|
||||
for field in missing_config:
|
||||
key = field["key"]
|
||||
default = field["default"]
|
||||
|
||||
_set_nested(config, key, default)
|
||||
results["config_added"].append(key)
|
||||
if not quiet:
|
||||
print(f" ✓ Added {key} = {default}")
|
||||
|
||||
# Update version and save
|
||||
config["_config_version"] = latest_ver
|
||||
save_config(config, strip_defaults=False)
|
||||
elif current_ver < latest_ver:
|
||||
# Just update version
|
||||
results["config_added"].extend(field["key"] for field in missing_config)
|
||||
|
||||
if current_ver < latest_ver:
|
||||
config = read_raw_config()
|
||||
config["_config_version"] = latest_ver
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
|
||||
# ── Skill-declared config vars ──────────────────────────────────────
|
||||
# Skills can declare config.yaml settings they need via
|
||||
|
|
@ -5725,7 +5726,7 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
|||
f"Skipped {var['key']} — skill '{var.get('skill', '?')}' may ask for it later"
|
||||
)
|
||||
print()
|
||||
save_config(config, strip_defaults=False)
|
||||
_persist_migration(config)
|
||||
else:
|
||||
print(" Set later with: hermes config set <key> <value>")
|
||||
|
||||
|
|
|
|||
|
|
@ -1086,11 +1086,17 @@ class TestInterimAssistantMessageConfig:
|
|||
with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}):
|
||||
migrate_config(interactive=False, quiet=True)
|
||||
raw = yaml.safe_load(config_path.read_text(encoding="utf-8"))
|
||||
loaded = load_config()
|
||||
|
||||
from hermes_cli.config import DEFAULT_CONFIG
|
||||
assert raw["_config_version"] == DEFAULT_CONFIG["_config_version"]
|
||||
# The user's explicit non-default value is preserved on disk.
|
||||
assert raw["display"]["tool_progress"] == "off"
|
||||
assert raw["display"]["interim_assistant_messages"] is True
|
||||
# interim_assistant_messages defaults to True and merges in transparently
|
||||
# at read time, so the migration must NOT materialise it to disk (that
|
||||
# was the config-bloat bug). It is still effective via load_config().
|
||||
assert "interim_assistant_messages" not in raw.get("display", {})
|
||||
assert loaded["display"]["interim_assistant_messages"] is True
|
||||
|
||||
|
||||
class TestCliRefreshIntervalConfig:
|
||||
|
|
@ -1322,21 +1328,105 @@ class TestWriteApprovalMigration:
|
|||
"skills:\n write_mode: 'off'\n")
|
||||
migrate_config(interactive=False, quiet=True)
|
||||
raw = yaml.safe_load((tmp_path / "config.yaml").read_text())
|
||||
assert raw["memory"]["write_approval"] is False
|
||||
assert raw["skills"]["write_approval"] is False
|
||||
loaded = load_config()
|
||||
# write_approval=False equals the schema default, so it is NOT
|
||||
# materialised to disk (lean-config invariant) — the legacy
|
||||
# write_mode key is gone and the effective value resolves to False
|
||||
# via load_config()'s deep-merge.
|
||||
assert "write_mode" not in raw.get("memory", {})
|
||||
assert "write_mode" not in raw.get("skills", {})
|
||||
assert loaded["memory"]["write_approval"] is False
|
||||
assert loaded["skills"]["write_approval"] is False
|
||||
|
||||
def test_unset_key_defaults_to_false(self, tmp_path):
|
||||
with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}):
|
||||
self._write(tmp_path, "_config_version: 28\nmemory:\n memory_enabled: true\n")
|
||||
migrate_config(interactive=False, quiet=True)
|
||||
raw = yaml.safe_load((tmp_path / "config.yaml").read_text())
|
||||
# No write_mode was persisted, so the rename is a no-op; the missing-
|
||||
# field pass then seeds the default (False = gate off). Either way the
|
||||
# gate ends up off and there's no leftover write_mode key.
|
||||
assert raw["memory"].get("write_approval", False) is False
|
||||
loaded = load_config()
|
||||
# No write_mode was persisted, so the rename is a no-op; the gate
|
||||
# ends up off (default) via deep-merge and there's no leftover
|
||||
# write_mode key on disk.
|
||||
assert loaded["memory"]["write_approval"] is False
|
||||
assert "write_mode" not in raw.get("memory", {})
|
||||
|
||||
|
||||
class TestMigrationWriteInvariant:
|
||||
"""Architectural guard: every migration write routes through the single
|
||||
_persist_migration() chokepoint, which strips schema defaults so a lean
|
||||
config is never bloated into a DEFAULT_CONFIG dump on a version bump.
|
||||
|
||||
These lock the centralised invariant so a future migration that calls
|
||||
save_config(...) directly (re-introducing the config-bloat bug class) is
|
||||
caught immediately.
|
||||
"""
|
||||
|
||||
def test_migrate_config_never_calls_save_config_directly(self):
|
||||
"""No `save_config(` call may live inside migrate_config()'s body — all
|
||||
writes must go through _persist_migration()."""
|
||||
import ast
|
||||
import inspect
|
||||
from hermes_cli import config as cfg_mod
|
||||
|
||||
src = inspect.getsource(cfg_mod.migrate_config)
|
||||
tree = ast.parse(src.lstrip())
|
||||
direct = [
|
||||
node for node in ast.walk(tree)
|
||||
if isinstance(node, ast.Call)
|
||||
and isinstance(node.func, ast.Name)
|
||||
and node.func.id == "save_config"
|
||||
]
|
||||
assert not direct, (
|
||||
"migrate_config must route every write through _persist_migration(); "
|
||||
f"found {len(direct)} direct save_config() call(s) — these re-introduce "
|
||||
"the config-bloat regression (lean config → DEFAULT_CONFIG dump)."
|
||||
)
|
||||
|
||||
@pytest.mark.parametrize("start_version", [1, "latest_minus_one"])
|
||||
def test_version_bump_keeps_config_lean(self, tmp_path, start_version):
|
||||
"""A lean config migrated to the latest version must never be rewritten
|
||||
into a defaults dump — neither across the whole range (start=1, where
|
||||
per-version seeds also fire) nor on a bare one-version bump (where only
|
||||
the catch-all finalizer runs). In both cases no default-only top-level
|
||||
section the user never wrote may land on disk, the merged view still
|
||||
exposes every default, and the user's explicit non-default value
|
||||
survives.
|
||||
"""
|
||||
latest = DEFAULT_CONFIG["_config_version"]
|
||||
start = latest - 1 if start_version == "latest_minus_one" else start_version
|
||||
config_path = tmp_path / "config.yaml"
|
||||
config_path.write_text(
|
||||
yaml.safe_dump({
|
||||
"_config_version": start,
|
||||
"model": {"default": "test-model", "provider": "openrouter"},
|
||||
"matrix": {"require_mention": False},
|
||||
}, sort_keys=False),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}):
|
||||
migrate_config(interactive=False, quiet=True)
|
||||
raw = yaml.safe_load(config_path.read_text(encoding="utf-8"))
|
||||
loaded = load_config()
|
||||
|
||||
assert raw["_config_version"] == latest
|
||||
# User's explicit non-default value preserved (not reset to True default).
|
||||
assert raw["matrix"]["require_mention"] is False
|
||||
assert loaded["matrix"]["require_mention"] is False
|
||||
# No default-only top-level section the user never wrote lands on disk —
|
||||
# neither from per-version seeds nor the catch-all finalizer.
|
||||
for default_key in (
|
||||
"timezone", "curator", "auxiliary", "tts", "compression",
|
||||
"whatsapp", "bedrock",
|
||||
):
|
||||
assert default_key not in raw, (
|
||||
f"{default_key} was materialised into a lean config by the "
|
||||
f"version bump — the default-dump regression returned"
|
||||
)
|
||||
# Defaults still take effect transparently via the read-time merge.
|
||||
assert loaded["curator"]["enabled"] == DEFAULT_CONFIG["curator"]["enabled"]
|
||||
assert loaded["display"]["compact"] == DEFAULT_CONFIG["display"]["compact"]
|
||||
|
||||
|
||||
class TestVerifyOnStopMigration:
|
||||
"""v30 → v31: switch verify_on_stop OFF once, preserving explicit choices."""
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue