From 498725ae59653218e842917530732e8f320dbbe7 Mon Sep 17 00:00:00 2001 From: ethernet Date: Fri, 18 Sep 2026 02:57:38 -0400 Subject: [PATCH] test(install-e2e): a migration retiring a key is not user-state loss An install from v2026.3.12 ships `.env` with `LLM_MODEL` set, because that release's template wrote it. The upgrade runs config migration 12 -> 13, which clears that dead var -- and the user-state verifier reported the change as the upgrade modifying the user's own state, failing the leg. The verifier exists to catch an upgrade taking state away; a var the CURRENT tree retires is not that. The retired set is parsed out of `hermes_cli/config_migrations.py` (the `for dead in (...): save_env_value(dead, "")` shape) rather than restated here, so it cannot drift from the tree, and an unreadable source retires nothing -- every .env change stays fatal. Tolerance is deliberately narrow: only keys the tree retires, only when the upgrade EMPTIED them, and only when no key was added or removed. Clearing a live key, or deleting a retired one outright, still fails. --- tests/install/e2e-assets/verify-user-state.py | 89 ++++++++++++++++++- tests/scripts/test_verify_user_state.py | 75 ++++++++++++++++ 2 files changed, 162 insertions(+), 2 deletions(-) diff --git a/tests/install/e2e-assets/verify-user-state.py b/tests/install/e2e-assets/verify-user-state.py index 86f2ce8d32..d3087d721d 100644 --- a/tests/install/e2e-assets/verify-user-state.py +++ b/tests/install/e2e-assets/verify-user-state.py @@ -44,6 +44,7 @@ Usage: from __future__ import annotations import argparse +import ast import hashlib import json import os @@ -67,6 +68,15 @@ JUDGED_ROOTS = ("memories", "cron", "sessions", "profiles", "photon", "skills/" + SKILL_ARCHIVE) # Trees recorded for the report but never judged (see the module docstring). ADVISORY_ROOTS = (SKILLS_ROOT,) + +# The tree's own migrations clear dead provider vars out of .env (the old setup +# wizard wrote LLM_MODEL/OPENAI_MODEL; config.yaml is the source of truth now). +# A key the CURRENT tree retires is not user state, so the upgrade clearing it +# is reported, not failed. Derived from the migration source so a newly retired +# var cannot drift out of this set; unreadable source retires nothing, which +# keeps every .env change fatal. +MIGRATION_SOURCE = Path(__file__).resolve().parents[3] / "hermes_cli" / "config_migrations.py" +EMPTY_VALUE_DIGEST = hashlib.sha256(b"").hexdigest()[:12] # state.db tables whose row counts stand in for "the user's data is still here". # Counting rows rather than hashing bytes: a live SQLite file changes for # benign reasons (WAL checkpoint, migration, a later turn). @@ -186,6 +196,67 @@ def _env_key_diff(before: dict, after: dict) -> dict: } +def _clears_loop_var(node: ast.AST, loop_var: str) -> bool: + """``save_env_value(loop_var, "")`` -- the migration's retire-this-key write.""" + return ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "save_env_value" + and len(node.args) == 2 + and isinstance(node.args[0], ast.Name) + and node.args[0].id == loop_var + and isinstance(node.args[1], ast.Constant) + and node.args[1].value == "" + ) + + +def retired_env_vars(source: Path | None = None) -> frozenset[str]: + """Provider vars the tree's own migrations clear to empty. + + Matches the loop form the 12 -> 13 migration uses + (``for dead in ("X", "Y"): save_env_value(dead, "")``) -- the only shape + that both names the keys and acts on them. A migration written another way + is simply not derived, which fails a leg loudly instead of silently + tolerating a real loss. + """ + path = MIGRATION_SOURCE if source is None else source + try: + tree = ast.parse(path.read_text(encoding="utf-8-sig")) + except (OSError, SyntaxError, UnicodeError): + return frozenset() + retired: set[str] = set() + for node in ast.walk(tree): + if not isinstance(node, ast.For) or not isinstance(node.target, ast.Name): + continue + if not any(_clears_loop_var(call, node.target.id) for call in ast.walk(node)): + continue + retired.update( + element.value for element in ast.walk(node.iter) + if isinstance(element, ast.Constant) and isinstance(element.value, str)) + return frozenset(retired) + + +def retired_env_clear(rel: str, pair: dict, retired: frozenset[str] | None = None) -> list[str]: + """Names a .env change may carry: the tree's retired vars, and emptied. + + Everything else stays fatal -- an added or removed key, a live key's value, + or a retired key the migration did NOT clear. Values are compared as the + same digests the snapshot records, so "emptied" is exact. + """ + known = retired_env_vars() if retired is None else retired + if os.path.basename(rel) != ".env" or not known: + return [] + diff = _env_key_diff(pair["before"], pair["after"]) + if diff["keys_added"] or diff["keys_removed"] or not diff["keys_changed"]: + return [] + if any(key not in known for key in diff["keys_changed"]): + return [] + after_keys = pair["after"].get("env_keys") or {} + if any(after_keys.get(key) != EMPTY_VALUE_DIGEST for key in diff["keys_changed"]): + return [] + return diff["keys_changed"] + + def _env_line_summary(path: str) -> dict: """Line-level shape of a .env without any content: totals, blanks, key order. @@ -379,8 +450,17 @@ def verify_home(home: str, snap: dict) -> dict: shrank = sorted(k for k, v in judged["modified"].items() if _rows_shrank(k, v["before"], v["after"])) tolerated_modified = sorted(set(judged["modified"]) - set(failing_modified)) - # A rewritten .env is fatal by design; name the variables that moved, or the - # caller is left holding two hashes of a secrets file and no lead. + # A rewritten .env is fatal by design -- except when the tree's own migration + # is what rewrote it, clearing a key that tree no longer reads. + retired_env: dict[str, list[str]] = {} + for rel, pair in list(failing_modified.items()): + cleared = retired_env_clear(rel, pair) + if cleared: + retired_env[rel] = cleared + del failing_modified[rel] + tolerated_modified = sorted(set(tolerated_modified) | {rel}) + # The rest still names the variables that moved, or the caller is left + # holding two hashes of a secrets file and no lead. for rel, pair in failing_modified.items(): if os.path.basename(rel) == ".env": pair["key_diff"] = _env_key_diff(pair["before"], pair["after"]) @@ -404,6 +484,7 @@ def verify_home(home: str, snap: dict) -> dict: "modified": failing_modified, "added": judged["added"], "tolerated_modified": tolerated_modified, + "retired_env_cleared": retired_env, "rows_shrank": shrank, "advisory": advisory, "ok": not failing_deleted and not failing_modified and not shrank, @@ -438,7 +519,11 @@ def _render(report: dict) -> str: f"counts and names, never content)") for rel in report["rows_shrank"]: lines.append(f" ROWS SHRANK {rel}") + for rel, names in report.get("retired_env_cleared", {}).items(): + lines.append(f" tolerated (retired var cleared: {','.join(names)}) {rel}") for rel in report["tolerated_modified"]: + if rel in report.get("retired_env_cleared", {}): + continue lines.append(f" tolerated (config rewrite) {rel}") for rel in report.get("tolerated_deleted", []): lines.append(f" tolerated (sqlite sidecar) {rel}") diff --git a/tests/scripts/test_verify_user_state.py b/tests/scripts/test_verify_user_state.py index 004237bffd..5f090c6e74 100644 --- a/tests/scripts/test_verify_user_state.py +++ b/tests/scripts/test_verify_user_state.py @@ -416,3 +416,78 @@ def test_any_sqlite_database_is_judged_by_rows_not_bytes(tmp_path): report = vus.verify_home(str(home), snap) assert report["rows_shrank"] == ["cron/executions.db"] assert report["ok"] is False + + +# --- retired provider vars --------------------------------------------------- + +def test_retired_env_vars_come_from_the_migration_source(tmp_path): + """The set is read from hermes_cli/config_migrations.py, not invented here. + + The 12 -> 13 migration clears LLM_MODEL/OPENAI_MODEL: the old setup wizard + wrote them and nothing reads them now. A verifier carrying its own copy of + that list would silently stop following the tree the moment it changes. + """ + assert vus.retired_env_vars() == {"LLM_MODEL", "OPENAI_MODEL"} + # Unreadable or unparseable source retires nothing, so every .env change + # stays fatal rather than quietly becoming tolerated. + assert vus.retired_env_vars(tmp_path / "absent.py") == frozenset() + broken = tmp_path / "broken.py" + broken.write_text("def (:\n", encoding="utf-8") + assert vus.retired_env_vars(broken) == frozenset() + + +def test_a_retired_var_the_upgrade_empties_is_tolerated_and_named(tmp_path): + """An old release's .env holds LLM_MODEL; the migration empties it. + + This verifier exists to catch an upgrade taking the user's state away. A key + the tree itself retires is not that, so it is reported -- by name, never by + value -- instead of failing the leg. + """ + home = tmp_path / "home" + home.mkdir() + (home / ".env").write_text( + "LLM_MODEL=anthropic/claude-opus-4.6\nOPENROUTER_API_KEY=secret-value\n", + encoding="utf-8", + ) + snap = vus.snapshot_home(str(home)) + # Exactly the migration's write: same key, cleared value. + (home / ".env").write_text( + "LLM_MODEL=\nOPENROUTER_API_KEY=secret-value\n", encoding="utf-8" + ) + + report = vus.verify_home(str(home), snap) + + assert report["ok"] is True, vus._render(report) + assert report["retired_env_cleared"] == {".env": ["LLM_MODEL"]} + rendered = vus._render(report) + assert "tolerated (retired var cleared: LLM_MODEL) .env" in rendered + assert "secret-value" not in rendered + assert "claude-opus" not in rendered + + +def test_a_live_key_cleared_by_an_upgrade_still_fails(tmp_path): + """The exception is retired keys only -- emptying a real key is still loss.""" + home = tmp_path / "home" + home.mkdir() + (home / ".env").write_text("OPENROUTER_API_KEY=secret-value\n", encoding="utf-8") + snap = vus.snapshot_home(str(home)) + (home / ".env").write_text("OPENROUTER_API_KEY=\n", encoding="utf-8") + + report = vus.verify_home(str(home), snap) + + assert report["ok"] is False + assert report["modified"][".env"]["key_diff"]["keys_changed"] == ["OPENROUTER_API_KEY"] + + +def test_a_retired_var_removed_outright_still_fails(tmp_path): + """Clearing a retired key is the migration; deleting the line is not it.""" + home = tmp_path / "home" + home.mkdir() + (home / ".env").write_text("LLM_MODEL=anthropic/claude-opus-4.6\n", encoding="utf-8") + snap = vus.snapshot_home(str(home)) + (home / ".env").write_text("", encoding="utf-8") + + report = vus.verify_home(str(home), snap) + + assert report["ok"] is False + assert report["modified"][".env"]["key_diff"]["keys_removed"] == ["LLM_MODEL"]