diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 6f4adb56f3e..5c568fd24fd 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -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 ") - # 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 ") diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index b830975e318..bfa4fffc7ee 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -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."""