From 560800f3cc8115302f52c532b88314a0387510c5 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:12:21 +0500 Subject: [PATCH] refactor: salvage follow-ups for PR #59177 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace _load_cron_jobs_for_config_warning with lazy import of cron.jobs.load_jobs — picks up BOM handling, corruption repair, and context-local store resolution for free - Re-add model.name to axis mapping (was dropped during merge); model.name is a legacy alias for model.default - Fix grammar: '1 enabled unpinned cron job have' -> 'has' - Pass user_config to cron_model_drift_guard_enabled in set_config_value to avoid a redundant load_config() re-read of the file just written --- hermes_cli/config.py | 33 ++++++++++------------- tests/hermes_cli/test_set_config_value.py | 10 ++++--- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 94eb49d6de970..2a37a6d63fb9a 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -8856,7 +8856,7 @@ def edit_config(): def _cron_model_drift_axis_for_config_key(key: str) -> Optional[str]: """Return the cron drift guard axis affected by a config key, if any.""" normalized = str(key or "").strip().lower() - if normalized in {"model", "model.default", "model.model"}: + if normalized in {"model", "model.default", "model.model", "model.name"}: return "model" if normalized in {"model.provider", "provider"}: return "provider" @@ -8887,29 +8887,23 @@ def cron_model_drift_guard_enabled( def _load_cron_jobs_for_config_warning() -> List[Dict[str, Any]]: - """Best-effort direct read of the active profile's cron jobs database.""" - jobs_path = get_hermes_home() / "cron" / "jobs.json" + """Best-effort read of the active profile's cron jobs database. + + Delegates to ``cron.jobs.load_jobs`` to reuse its BOM handling, corruption + repair, and context-local store resolution (tests, embedders). Falls back + to an empty list on any failure so config writes never break. + """ try: - if not jobs_path.exists(): - return [] - data = json.loads(jobs_path.read_text(encoding="utf-8")) + from cron.jobs import load_jobs + return load_jobs() except Exception: return [] - if isinstance(data, dict): - raw_jobs = data.get("jobs", []) - elif isinstance(data, list): - raw_jobs = data - else: - return [] - if not isinstance(raw_jobs, list): - return [] - return [job for job in raw_jobs if isinstance(job, dict)] - def warn_unpinned_cron_jobs_after_model_config_change( key: str, value: Any, + config: Optional[Dict[str, Any]] = None, ) -> None: """Warn when a global model/provider change will trip cron's drift guard. @@ -8921,7 +8915,7 @@ def warn_unpinned_cron_jobs_after_model_config_change( axis = _cron_model_drift_axis_for_config_key(key) if axis is None: return - if not cron_model_drift_guard_enabled(): + if not cron_model_drift_guard_enabled(config): return new_value = str(value or "").strip().lower() @@ -8946,8 +8940,9 @@ def warn_unpinned_cron_jobs_after_model_config_change( return noun = "job" if affected == 1 else "jobs" + verb = "has" if affected == 1 else "have" print( - f"⚠️ {affected} enabled unpinned cron {noun} have stored " + f"⚠️ {affected} enabled unpinned cron {noun} {verb} stored " f"{snapshot_field} values that differ from the new global {axis}. " "They will fail closed on their next run instead of silently using the " "changed model/provider. Inspect with `hermes cron list`, then pin the " @@ -9270,7 +9265,7 @@ def set_config_value(key: str, value: str, force: bool = False): else: _display_value = value print(f"✓ Set {key} = {_display_value} in {config_path}") - warn_unpinned_cron_jobs_after_model_config_change(key, value) + warn_unpinned_cron_jobs_after_model_config_change(key, value, user_config) # 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. diff --git a/tests/hermes_cli/test_set_config_value.py b/tests/hermes_cli/test_set_config_value.py index ece13dc65a206..b5590665c53f5 100644 --- a/tests/hermes_cli/test_set_config_value.py +++ b/tests/hermes_cli/test_set_config_value.py @@ -521,27 +521,29 @@ class TestCronModelDriftConfigWarning: assert "Set model.default = new-model" in captured.out assert "fail closed" not in captured.out - def test_model_name_does_not_warn_for_unread_cron_axis( + def test_model_name_change_warns_like_model_default( self, _isolated_hermes_home, capsys, ): + """model.name is a legacy alias for model.default — the warning must fire.""" _write_cron_jobs( _isolated_hermes_home, [ { "id": "model-drift-job", "enabled": True, + "model": None, "model_snapshot": "old-model", } ], ) - set_config_value("model.name", "display-only-name") + set_config_value("model.name", "new-model") captured = capsys.readouterr() - assert "Set model.name = display-only-name" in captured.out - assert "fail closed" not in captured.out + assert "Set model.name = new-model" in captured.out + assert "fail closed" in captured.out def test_explicit_opt_out_suppresses_warning( self,