refactor(config): read the version stamp once and drop has_version_stamp
migrate_config() and the docker boot script each parsed config.yaml twice: check_config_version() coerced a missing `_config_version` to 0 and threw the "was it present" bit away, so has_version_stamp() re-read the file to recover it, guarded only by a call-order promise in its docstring. That promise did not hold for the docker script, which used the tolerant check: a list-rooted config.yaml read as "unversioned, not below the floor", ran the backup + migrate_config() dance and exited 1 (base: floor warning, exit 0). Factor the read into _read_config_version_stamp() -> (Optional[int], latest); None means the mapping has no stamp. check_config_version() is a thin wrapper (None -> 0) so its 10 callers see identical output. migrate_config() and the docker script decide `unversioned` from that single read; the docker script now does the strict read itself and leaves an unparseable or non-mapping file alone with a warning and exit 0, matching its invalid-YAML posture. has_version_stamp() is deleted. Docker tests that mocked the pre-check now mock the new helper.
This commit is contained in:
@@ -910,14 +910,12 @@ def _coerce_config_version(value: Any) -> int:
|
||||
return max(version, 0)
|
||||
|
||||
|
||||
def check_config_version(*, raise_on_parse_error: bool = False) -> Tuple[int, int]:
|
||||
"""Return ``(current_version, latest_version)`` from the raw on-disk config.
|
||||
Reads the raw file rather than ``load_config()``: the deep-merge would make a file lacking
|
||||
``_config_version`` inherit the latest version, hiding that the schema was never migrated.
|
||||
Invalid YAML gets a parse warning, not an automatic schema rewrite. Tolerant runtime status
|
||||
callers keep the historical latest/latest fallback for malformed YAML; mutation and explicit
|
||||
validation paths set ``raise_on_parse_error`` so a parse failure or a non-mapping root cannot
|
||||
be mistaken for an up-to-date config."""
|
||||
def _read_config_version_stamp(*, raise_on_parse_error: bool = False) -> Tuple[Optional[int], int]:
|
||||
"""Single raw read behind ``check_config_version()``: ``(stamp, latest_version)`` where
|
||||
*stamp* is ``None`` when config.yaml parsed but carries no ``_config_version`` key (a
|
||||
never-stamped current-schema file, not an ancient install — ``migrate_config()`` gives it only
|
||||
the legacy-key steps). A missing file, or malformed YAML under a tolerant caller, reads as
|
||||
``latest`` exactly as ``check_config_version()`` always reported it."""
|
||||
latest = _coerce_config_version(DEFAULT_CONFIG.get("_config_version", 1)) or 1
|
||||
config_path = get_config_path()
|
||||
if not config_path.exists():
|
||||
@@ -945,9 +943,23 @@ def check_config_version(*, raise_on_parse_error: bool = False) -> Tuple[int, in
|
||||
f"a mapping, got {type(config).__name__}"
|
||||
)
|
||||
config = {}
|
||||
if "_config_version" not in config:
|
||||
return None, latest
|
||||
return _coerce_config_version(config.get("_config_version")), latest
|
||||
|
||||
|
||||
def check_config_version(*, raise_on_parse_error: bool = False) -> Tuple[int, int]:
|
||||
"""Return ``(current_version, latest_version)`` from the raw on-disk config.
|
||||
Reads the raw file rather than ``load_config()``: the deep-merge would make a file lacking
|
||||
``_config_version`` inherit the latest version, hiding that the schema was never migrated.
|
||||
Invalid YAML gets a parse warning, not an automatic schema rewrite. Tolerant runtime status
|
||||
callers keep the historical latest/latest fallback for malformed YAML; mutation and explicit
|
||||
validation paths set ``raise_on_parse_error`` so a parse failure or a non-mapping root cannot
|
||||
be mistaken for an up-to-date config. A file with no version key reads as 0."""
|
||||
stamp, latest = _read_config_version_stamp(raise_on_parse_error=raise_on_parse_error)
|
||||
return (0 if stamp is None else stamp), latest
|
||||
|
||||
|
||||
# ---- Config structure validation ----
|
||||
|
||||
# DEFAULT_CONFIG is the single source of truth for documented roots; the set is derived so new
|
||||
@@ -1299,7 +1311,8 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
||||
|
||||
# Validate config.yaml before any migration side effect: sanitize_env_file() rewrites .env,
|
||||
# which must not happen when the migration will be refused for malformed YAML.
|
||||
current_ver, latest_ver = check_config_version(raise_on_parse_error=True)
|
||||
stamp, latest_ver = _read_config_version_stamp(raise_on_parse_error=True)
|
||||
current_ver = 0 if stamp is None else stamp
|
||||
|
||||
try:
|
||||
fixes = sanitize_env_file()
|
||||
@@ -1315,9 +1328,9 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A
|
||||
# Missing/unparseable files never trip the floor gate.
|
||||
# Imported lazily because the steps call back into this module.
|
||||
from hermes_cli.config_migrations import (
|
||||
SUPPORT_FLOOR_VERSION, has_version_stamp, run_migrations, support_floor_message)
|
||||
SUPPORT_FLOOR_VERSION, run_migrations, support_floor_message)
|
||||
|
||||
has_explicit_version = has_version_stamp()
|
||||
has_explicit_version = stamp is not None
|
||||
floor_refused = (
|
||||
has_explicit_version and current_ver < SUPPORT_FLOOR_VERSION and current_ver < latest_ver)
|
||||
if floor_refused:
|
||||
|
||||
@@ -36,14 +36,6 @@ def support_floor_message() -> str:
|
||||
"after reviewing the changelog.")
|
||||
|
||||
|
||||
def has_version_stamp() -> bool:
|
||||
"""Whether config.yaml carries a ``_config_version`` key. ``check_config_version()`` reads a
|
||||
missing one as 0, but such a file is never-stamped current-schema content, not an ancient
|
||||
install: the floor must not refuse it and only :data:`LEGACY_KEY_STEPS` may run on it.
|
||||
Callers have already gone through ``check_config_version()``, so the read cannot fail here."""
|
||||
return "_config_version" in _cfg().read_user_config_raw()
|
||||
|
||||
|
||||
def _cfg():
|
||||
"""Return the live ``hermes_cli.config`` module (lazy, cycle-free, monkeypatch-friendly)."""
|
||||
from hermes_cli import config
|
||||
|
||||
@@ -8,6 +8,8 @@ from pathlib import Path
|
||||
from typing import Iterable
|
||||
|
||||
from hermes_cli.config import (
|
||||
InvalidUserConfigError,
|
||||
_read_config_version_stamp,
|
||||
check_config_version,
|
||||
get_config_path,
|
||||
get_env_path,
|
||||
@@ -16,7 +18,6 @@ from hermes_cli.config import (
|
||||
from hermes_cli.config_backups import backup_config, list_config_backups
|
||||
from hermes_cli.config_migrations import (
|
||||
SUPPORT_FLOOR_VERSION,
|
||||
has_version_stamp,
|
||||
support_floor_message,
|
||||
)
|
||||
from utils import env_var_enabled
|
||||
@@ -48,7 +49,15 @@ def main() -> int:
|
||||
print("[config-migrate] HERMES_SKIP_CONFIG_MIGRATION is set; skipping config migration")
|
||||
return 0
|
||||
|
||||
current_ver, latest_ver = check_config_version()
|
||||
# Strict read: malformed YAML or a non-mapping root is left alone with a warning (the
|
||||
# tolerant check_config_version() already printed one) and the boot continues, instead of
|
||||
# running the backup/migrate dance that migrate_config() would refuse anyway.
|
||||
try:
|
||||
stamp, latest_ver = _read_config_version_stamp(raise_on_parse_error=True)
|
||||
except InvalidUserConfigError as exc:
|
||||
print(f"[config-migrate] WARNING: {exc}; leaving config.yaml untouched", file=sys.stderr)
|
||||
return 0
|
||||
current_ver = 0 if stamp is None else stamp
|
||||
if current_ver >= latest_ver:
|
||||
return 0
|
||||
|
||||
@@ -56,9 +65,9 @@ def main() -> int:
|
||||
# leaves the file untouched), so don't run the backup/verify dance that
|
||||
# would raise "did not advance config version" and block the boot.
|
||||
# Warn-and-continue matches the CLI's fail-safe posture. A config with no
|
||||
# _config_version (a volume seeded from the template) is not below the
|
||||
# floor: migrate_config() stamps it.
|
||||
if current_ver < SUPPORT_FLOOR_VERSION and has_version_stamp():
|
||||
# _config_version (stamp None: a volume seeded from the template) is not
|
||||
# below the floor: migrate_config() stamps it.
|
||||
if stamp is not None and current_ver < SUPPORT_FLOOR_VERSION:
|
||||
print(
|
||||
f"[config-migrate] WARNING: {support_floor_message()}",
|
||||
file=sys.stderr,
|
||||
|
||||
@@ -155,7 +155,9 @@ def test_docker_config_migrate_restores_backups_after_failed_migration(
|
||||
config_path.write_text(original_config, encoding="utf-8")
|
||||
env_path.write_text(original_env, encoding="utf-8")
|
||||
|
||||
monkeypatch.setattr(module, "check_config_version", lambda: (12, DEFAULT_CONFIG["_config_version"]))
|
||||
monkeypatch.setattr(
|
||||
module, "_read_config_version_stamp",
|
||||
lambda *, raise_on_parse_error=False: (12, DEFAULT_CONFIG["_config_version"]))
|
||||
monkeypatch.setattr(module, "get_config_path", lambda: config_path)
|
||||
monkeypatch.setattr(module, "get_env_path", lambda: env_path)
|
||||
|
||||
@@ -186,8 +188,10 @@ def test_docker_config_migrate_restores_backups_when_version_does_not_advance(
|
||||
config_path.write_text(original_config, encoding="utf-8")
|
||||
env_path.write_text(original_env, encoding="utf-8")
|
||||
|
||||
calls = iter([(12, DEFAULT_CONFIG["_config_version"]), (12, DEFAULT_CONFIG["_config_version"])])
|
||||
monkeypatch.setattr(module, "check_config_version", lambda: next(calls))
|
||||
monkeypatch.setattr(
|
||||
module, "_read_config_version_stamp",
|
||||
lambda *, raise_on_parse_error=False: (12, DEFAULT_CONFIG["_config_version"]))
|
||||
monkeypatch.setattr(module, "check_config_version", lambda: (12, DEFAULT_CONFIG["_config_version"]))
|
||||
monkeypatch.setattr(module, "get_config_path", lambda: config_path)
|
||||
monkeypatch.setattr(module, "get_env_path", lambda: env_path)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user