From ea53e330e320c0597169468ba1d8ddb7a73ffc1e Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:27:02 -0500 Subject: [PATCH] fix(cron): name the cron-fleet default in drift-guard remediation The #44585 drift guard fails an unpinned cron job closed when the global provider/model moves under it, and every message it emits offers exactly one remedy: pin this job. That is correct for one job and wrong for a fleet. #59031 was 34 jobs breaking on a single global switch, i.e. 34 per-job edits for one config change, and #89242 is the same complaint arriving again with a proposed fix that would disarm the guard entirely. The fleet-wide remedy already exists. #73532 added `cron.model` / `cron.model_provider`, and `_cron_fleet_default_covers_axis` makes the guard skip any axis they cover, so one command decouples the whole unpinned fleet from the chat model without giving up spend protection on the axes it does not cover. The cron guide has documented this since #73532; none of the runtime messages did, so the only operators who find it are the ones who read docs after being told to make N edits. Teach all three surfaces to name it alongside the pin: - the scheduler's skip log and alert blob, on both the recurring and the #72056 finite-one-shot path, naming `cron.model_provider` for the provider axis rather than interpolating the axis name into a key that does not exist - the chat delivery line, which is the surface an operator reads unprompted rather than after something sent them to a log; kept to one terse sentence so the notification stays one line - the `hermes config set model.default` warning, which fires before any job has failed and is therefore the cheapest place to have omitted it No behaviour change: the guard engages in exactly the cases it engaged in before, and the per-job and one-shot remedies are unchanged. The warning is still suppressed for an axis the fleet default already covers, so an operator who has taken the advice is never given it. Tests parse the config key back out of each emitted message and feed it to `_cron_fleet_default_covers_axis`, so the advice cannot go stale against the mechanism the way it just did. Fixes #89242 --- cron/scheduler.py | 28 +- hermes_cli/config.py | 14 +- ...n_drift_remediation_names_fleet_default.py | 309 ++++++++++++++++++ 3 files changed, 345 insertions(+), 6 deletions(-) create mode 100644 tests/cron/test_cron_drift_remediation_names_fleet_default.py diff --git a/cron/scheduler.py b/cron/scheduler.py index 9c260d112fb9..0a74bb80725c 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -215,9 +215,16 @@ def _summarize_cron_failure_for_delivery(job: dict, error: str | None) -> str: f"`hermes cron edit {job_id} --provider " "--model `." ) + # The chat line is the surface an operator actually reads; the log and + # the alert blob are where they look only after this line sent them + # there. Naming just the per-job pin here is why a fleet-wide switch + # reads as N separate incidents needing N separate edits (#59031). + # Terse on purpose -- this stays one line. return ( f"⚠️ Cron '{job_name}' skipped before inference to prevent " - f"unintended spend. {remediation}" + f"unintended spend. {remediation} For the whole unpinned fleet at " + "once, set a cron default instead: `hermes config set cron.model " + "`." ) # A no_agent job IS its script — run_job short-circuits it before any model @@ -5464,11 +5471,26 @@ def run_job( and job["schedule"].get("kind") == "once" and _repeat.get("times") == 1 ) + # Name the fleet-wide remedy alongside the per-job one. A + # global model change drifts EVERY unpinned job at once + # (#59031: 34 of them), so per-job pinning is the O(n) answer + # to an O(1) problem -- and `cron.model` is the one the docs + # already call "the deliberate way to route cron spend", with + # the guard exempting any axis it covers + # (_cron_fleet_default_covers_axis). An operator reading only + # this line had no way to learn that existed. + _fleet_default_hint = ( + " If you want every unpinned cron job on a fixed model " + "regardless of your chat model, set the cron-fleet default " + "once instead: `hermes config set cron.model ` " + "(and `cron.model_provider` if the provider also differs) " + "-- the guard does not engage for an axis it covers." + ) if _finite_oneshot: _remediation = ( "This finite one-shot job is consumed by this attempted run; " "create a new one-shot job at a future time with an explicit " - "provider and model." + "provider and model." + _fleet_default_hint ) else: _remediation = ( @@ -5476,7 +5498,7 @@ def run_job( "pin it explicitly: " f"`hermes cron edit {job_id} --provider " "--model ` (or pin the original values to keep " - "them)." + "them)." + _fleet_default_hint ) logger.warning( "Job '%s': SKIPPED — global inference config drifted since " diff --git a/hermes_cli/config.py b/hermes_cli/config.py index d2d5c65e26a5..9da05fa60f55 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -5042,13 +5042,21 @@ def warn_unpinned_cron_jobs_after_model_config_change( snapshot_field = f"{axis}_snapshot" noun = "job" if affected == 1 else "jobs" verb = "has" if affected == 1 else "have" + # Two remedies, and for a fleet the second is almost always the right + # one: pinning is per job, so a single global switch turns into N edits + # (#59031 broke 34 jobs at once), while `cron.model` decouples the whole + # unpinned fleet from the chat model in one command and leaves the guard + # armed for everything it does not cover. The docs have said this since + # #73532; this warning did not. print( 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 " - "intended values with `hermes cron edit --provider " - "--model `." + "changed model/provider. Inspect with `hermes cron list`, then either " + "pin them individually (`hermes cron edit --provider " + " --model `) or give the whole unpinned fleet a " + f"default that your chat model no longer touches (`hermes config set " + f"cron.{axis if axis == 'model' else 'model_provider'} <{axis}>`)." ) diff --git a/tests/cron/test_cron_drift_remediation_names_fleet_default.py b/tests/cron/test_cron_drift_remediation_names_fleet_default.py new file mode 100644 index 000000000000..28ca7c686dc0 --- /dev/null +++ b/tests/cron/test_cron_drift_remediation_names_fleet_default.py @@ -0,0 +1,309 @@ +"""The drift guard's remediation must name the fleet-wide remedy, not only the pin. + +#44585's guard fails an unpinned cron job closed when the global provider/model +moves under it. Its remediation has always named exactly one fix: pin this job. +That is the right advice for one job and the wrong advice for a fleet — #59031 +was 34 jobs breaking on a single global switch, i.e. 34 edits for one change. + +The fleet-wide answer already exists and is already documented +(``website/docs/user-guide/features/cron.md``, since #73532): ``cron.model`` / +``cron.model_provider`` give unpinned cron jobs their own default, and +``_cron_fleet_default_covers_axis`` makes the guard skip any axis they cover. +Three merged PRs built that path deliberately. None of them taught the runtime +messages to mention it, so an operator hitting the guard is told to solve an +O(1) problem O(n) times and has no way to discover the alternative. + +These tests pin the mention into all three surfaces an operator can hit — the +log/alert blob, the chat delivery line, and the ``hermes config set`` warning — +and, more importantly, pin the *command in the message to the key the guard +actually reads*, so advice and mechanism cannot drift apart. + +They also pin the exemption itself. #89242 proposes removing snapshots from +inherit-mode jobs, which would take the guard off exactly the population these +three PRs kept it on. The exemption test below is what that change would have +to break. +""" + +from __future__ import annotations + +import re + +import pytest + +from cron.scheduler import _summarize_cron_failure_for_delivery +from hermes_cli.config import ( + _cron_fleet_default_covers_axis, + warn_unpinned_cron_jobs_after_model_config_change, +) + +# Reuse the guard's own harness: these messages are emitted from inside +# ``run_job``, and a message test that stubs the guard instead of running it +# would keep passing after the guard stopped firing. +from tests.cron.test_cron_provider_pin import ( + _base_job, + _run_with_current_provider_and_model, +) + + +# The exact config keys the guard consults. Written out rather than imported so +# a rename has to be made twice, deliberately. +FLEET_MODEL_KEY = "cron.model" +FLEET_PROVIDER_KEY = "cron.model_provider" + + +def _drift_error(tmp_path, **kwargs): + """Run an unpinned job into the guard and return its error blob.""" + job = kwargs.pop("job", None) or _base_job( + provider_snapshot="openrouter", model_snapshot="old-model" + ) + success, output, _final, error, agent_constructed = ( + _run_with_current_provider_and_model( + job, kwargs.pop("provider", "openrouter"), + kwargs.pop("model", "new-model"), tmp_path, **kwargs + ) + ) + return success, f"{error}\n{output}", error, agent_constructed + + +class TestTheSkipMessageNamesBothRemedies: + + def test_recurring_job_learns_about_the_cron_fleet_default(self, tmp_path): + success, blob, _error, agent_constructed = _drift_error(tmp_path) + + assert success is False + assert agent_constructed is False, "the guard must still fail closed" + assert FLEET_MODEL_KEY in blob, ( + "the operator is told to pin one job at a time with no hint that a " + "fleet-wide default exists — that is #59031's 34 edits" + ) + assert "hermes config set" in blob + + def test_the_per_job_pin_advice_is_preserved(self, tmp_path): + """Additive, not a replacement. + + Pinning is still the right answer for a single job that must not move, + and #72056's lifecycle wording is still the right answer for a spent + one-shot. This change adds a second remedy; it removes none. + """ + _success, blob, _error, _constructed = _drift_error(tmp_path) + + assert "hermes cron edit pin-test --provider --model " in blob + + def test_finite_oneshot_learns_about_it_too(self, tmp_path): + """The path where the pin advice is useless is the one that needs it most. + + #72056 established that a consumed one-shot cannot be fixed by editing + it. Until now the only alternative offered was "create a new one-shot + with an explicit provider and model" — still per job. + """ + job = _base_job( + provider_snapshot="openrouter", + model_snapshot="old-model", + schedule={"kind": "once", "run_at": "2030-01-01T00:00:00Z"}, + repeat={"times": 1, "completed": 1}, + ) + _success, blob, _error, _constructed = _drift_error(tmp_path, job=job) + + assert "create a new one-shot job" in blob.lower() + assert FLEET_MODEL_KEY in blob + + def test_the_provider_axis_is_named_as_well(self, tmp_path): + """Two axes drift, and the fleet default has a separate key per axis. + + Naming only ``cron.model`` would send an operator whose *provider* + moved to a setting that cannot help them. + """ + _success, blob, _error, _constructed = _drift_error( + tmp_path, provider="nous" + ) + + assert FLEET_PROVIDER_KEY in blob + + def test_the_chat_delivery_line_names_it(self, tmp_path): + """The surface an operator actually reads. + + The log and the alert blob are where they look *after* something sent + them there; the delivery line is what arrives unprompted. + """ + _success, _blob, error, _constructed = _drift_error(tmp_path) + + delivered = _summarize_cron_failure_for_delivery(_base_job(), error) + assert FLEET_MODEL_KEY in delivered + assert "hermes config set" in delivered + + def test_the_delivery_line_stays_a_single_line(self, tmp_path): + """``_summarize_cron_failure_for_delivery`` exists to be compact. + + Its docstring puts the full detail in the log and the output directory + on purpose. A remedy sentence that turns the chat notification into a + paragraph would defeat the function. + """ + _success, _blob, error, _constructed = _drift_error(tmp_path) + + delivered = _summarize_cron_failure_for_delivery(_base_job(), error) + assert "\n" not in delivered + assert len(delivered) < 500 + + +class TestTheAdviceMatchesTheMechanism: + """A remedy in a message is a promise; these tests make it a checked one.""" + + @pytest.mark.parametrize("axis,expected_key", [ + ("model", FLEET_MODEL_KEY), + ("provider", FLEET_PROVIDER_KEY), + ]) + def test_every_key_the_messages_name_is_one_the_guard_honors( + self, axis, expected_key + ): + """Parse the key back out of the advice and feed it to the guard. + + This is the test that would have caught the message being stale in the + first place, and the one that keeps it from going stale again: if + ``_cron_fleet_default_covers_axis`` is ever taught a different key, the + sentence telling operators to set the old one fails here rather than in + somebody's cron log. + """ + section = expected_key.split(".", 1)[1] + assert _cron_fleet_default_covers_axis( + axis, {"cron": {section: "some/value"}} + ) is True + assert _cron_fleet_default_covers_axis(axis, {"cron": {}}) is False + + def test_the_command_in_the_skip_message_parses_to_a_real_key(self, tmp_path): + """No hand-written key strings: read them out of the emitted text.""" + _success, blob, _error, _constructed = _drift_error(tmp_path) + + keys = set(re.findall(r"hermes config set (cron\.[a-z_]+)", blob)) + assert keys, "the skip message names no `hermes config set` command" + for dotted in keys: + section = dotted.split(".", 1)[1] + axis = "model" if section == "model" else "provider" + assert _cron_fleet_default_covers_axis( + axis, {"cron": {section: "some/value"}} + ) is True, f"{dotted} is advertised but the guard ignores it" + + +class TestTheExemptionTheAdviceRelies_On: + """The advice is only true because the guard already exempts covered axes. + + #89242 asks for inherit-mode jobs to stop recording snapshots at all, which + would disarm the guard for the entire unpinned fleet rather than for the + axes an operator deliberately routed. That is the outcome #73323 and #73532 + were merged to avoid and #61468 was closed over. These two tests are what + such a change would have to break. + """ + + def test_a_covered_axis_does_not_drift(self, tmp_path): + """Setting the fleet default really does stop the skip.""" + success, _blob, _error, agent_constructed = _drift_error( + tmp_path, cron_model="fleet/model" + ) + + assert success is True + assert agent_constructed is True + + def test_an_uncovered_axis_still_fails_closed(self, tmp_path): + """And it stops it only for the axis it covers. + + A fleet default on the model must not buy an unpinned job a free ride + onto a paid *provider* — that is the $7.73 half of #44585. + """ + success, blob, _error, agent_constructed = _drift_error( + tmp_path, provider="nous", cron_model="fleet/model" + ) + + assert success is False + assert agent_constructed is False + assert "provider" in blob.lower() + + +def _warn(key, value, jobs, config=None, monkeypatch=None): + monkeypatch.setattr( + "hermes_cli.config._load_cron_jobs_for_config_warning", lambda: jobs + ) + warn_unpinned_cron_jobs_after_model_config_change(key, value, config or {}) + + +def _unpinned_job(**overrides): + job = { + "id": "job-1", + "name": "Morning summary", + "enabled": True, + "no_agent": False, + "provider_snapshot": "openrouter", + "model_snapshot": "old/model", + } + job.update(overrides) + return job + + +class TestTheConfigSetWarningNamesBothRemedies: + """The earlier of the two moments — before any job has failed yet. + + ``hermes config set model.default X`` is where an operator can still act + before N jobs skip. Telling them only about per-job pinning here is the + most expensive place to omit the fleet default. + """ + + def test_model_change_names_the_fleet_model_key(self, monkeypatch, capsys): + _warn("model.default", "new/model", [_unpinned_job()], monkeypatch=monkeypatch) + + out = capsys.readouterr().out + assert FLEET_MODEL_KEY in out + assert "hermes cron edit" in out, "the per-job remedy is still offered" + + def test_provider_change_names_the_fleet_provider_key(self, monkeypatch, capsys): + """Not ``cron.provider``. + + The fleet default's provider key is ``cron.model_provider``; the axis is + called ``provider``. Interpolating the axis name into the command is the + obvious way to write this line and produces a key that does not exist. + """ + _warn("model.provider", "nous", [_unpinned_job()], monkeypatch=monkeypatch) + + out = capsys.readouterr().out + assert FLEET_PROVIDER_KEY in out + assert "cron.provider " not in out + assert "cron.provider>" not in out + + def test_a_covered_axis_produces_no_warning_at_all(self, monkeypatch, capsys): + """The advice is not offered to someone who already took it. + + An operator who has set ``cron.model`` is not affected by a chat-model + change on that axis, so warning them would be a false alarm — and would + advertise a remedy they are already using. + """ + _warn( + "model.default", + "new/model", + [_unpinned_job()], + config={"cron": {"model": "fleet/model"}}, + monkeypatch=monkeypatch, + ) + + assert capsys.readouterr().out == "" + + def test_the_warning_still_says_what_will_happen(self, monkeypatch, capsys): + """Behaviour preservation: the diagnosis is unchanged, only the cure grew.""" + _warn("model.default", "new/model", [_unpinned_job()], monkeypatch=monkeypatch) + + out = capsys.readouterr().out + assert "fail closed" in out + assert "model_snapshot" in out + assert "hermes cron list" in out + + def test_the_key_the_warning_names_is_one_the_guard_honors( + self, monkeypatch, capsys + ): + """Same round-trip as the skip message, at the earlier surface.""" + _warn("model.provider", "nous", [_unpinned_job()], monkeypatch=monkeypatch) + + out = capsys.readouterr().out + keys = set(re.findall(r"hermes config set (cron\.[a-z_]+)", out)) + assert keys + for dotted in keys: + section = dotted.split(".", 1)[1] + axis = "model" if section == "model" else "provider" + assert _cron_fleet_default_covers_axis( + axis, {"cron": {section: "some/value"}} + ) is True, f"{dotted} is advertised but the guard ignores it"