fix(cron): name the cron-fleet default in drift-guard remediation - #89292
fix(cron): name the cron-fleet default in drift-guard remediation#89292jackulau wants to merge 1 commit into
Conversation
The NousResearch#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. NousResearch#59031 was 34 jobs breaking on a single global switch, i.e. 34 per-job edits for one config change, and NousResearch#89242 is the same complaint arriving again with a proposed fix that would disarm the guard entirely. The fleet-wide remedy already exists. NousResearch#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 NousResearch#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 NousResearch#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 NousResearch#89242
66196b0 to
ea53e33
Compare
|
|
Thanks — and point 2 is worth acting on rather than filing, so I have taken it Your read of the fork is right, and I would put it slightly more sharply: after That is what these tests document, so the link matters in both directions: On point 1 — parsing the Nothing changed on this branch; it is rebased on current |
What does this PR do?
The #44585 drift guard fails an unpinned cron job closed when the global provider/model moves under it. 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 — 34 per-job edits for one config change. #89242 is the same complaint arriving again, this time with a proposed fix (stop snapshotting inherit-mode jobs) that would disarm the guard for 100% of the population it exists to protect.
The fleet-wide remedy already exists. #73532 added
cron.model/cron.model_provider, and_cron_fleet_default_covers_axismakes 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 doesn't cover.website/docs/user-guide/features/cron.mdhas said so since #73532:None of the runtime messages did. So the only operators who find it are the ones who go read the docs after being told to make N edits — and #89242 is what happens when they don't. This teaches all three surfaces to name it.
No behaviour change. The guard engages in exactly the cases it engaged in before, and the existing per-job and finite-one-shot remedies are unchanged — this is additive to both.
Related Issue
Fixes #89242
Type of Change
Changes Made
cron/scheduler.py_fleet_default_hint, appended to both branches — the recurring one and fix(cron): preserve actionable drift recovery alerts #72056's finite-one-shot one. The one-shot path is where it matters most: fix(cron): preserve actionable drift recovery alerts #72056 correctly established that a consumed one-shot can't be fixed by editing it, so until now the only alternative offered there was "create a new one-shot with an explicit provider and model" — still per job._summarize_cron_failure_for_deliverygains a terse one-sentence version. This is the surface an operator reads unprompted; the log and the alert blob are where they look only after something sent them there. Deliberately shorter than the log wording — that function exists to stay compact, and there's a test asserting the delivery line stays a single line under 500 chars.hermes_cli/config.pywarn_unpinned_cron_jobs_after_model_config_changenow offers both remedies. This warning (fix(cron): warn before model config changes trip cron drift guard #73323) fires before any job has failed, which makes it the cheapest place to have omitted the fleet default and the most valuable place to add it.cron.model_provider, notcron.provider. The axis is calledproviderbut the fleet-default key ismodel_provider, so interpolating the axis name — the obvious way to write this line — produces a key that doesn't exist. There's a dedicated test for that.tests/cron/test_cron_drift_remediation_names_fleet_default.py— new, 16 tests.The interesting ones round-trip the advice through the mechanism: they
re.findallthehermes config set cron.Xcommand back out of each emitted message and feedXto_cron_fleet_default_covers_axis. A message that advertises a key the guard ignores fails here rather than in somebody's cron log. That's what would have caught this being stale in the first place.Two tests also pin the exemption itself (a covered axis doesn't drift; an uncovered axis still fails closed), because the advice is only true while that exemption holds. They reuse
test_cron_provider_pin.py's own_run_with_current_provider_and_modelharness on purpose — a message test that stubbed the guard instead of running it would keep passing after the guard stopped firing.How to Test
pytest tests/cron/test_cron_drift_remediation_names_fleet_default.py -q # 16 passedManually, with any unpinned agent job:
Mutation proof — each surface is independently load-bearing:
_fleet_default_hint→"")config setwarning wording_cron_fleet_default_covers_axis→ alwaysFalseThat last row is the one I care about: it's the mutation that #89242's proposed fix is a variant of, and it fails loudly.
Baseline —
pytest tests/cron/ tests/hermes_cli/test_cron_model_impact.py tests/cli/test_cli_save_config_value.py tests/hermes_cli/test_set_config_value.py -qrun serially, with and without the change:test_file_permissions.py(POSIX0700/0600mode bits),test_cron_workdir.py::test_tilde_expands, andtest_media_delivery_parity.py::TestMediaPolicyEnvBridge::test_bridge_helper_exists_and_applies_config. None is touched by this diff.test_cleanup_timeout.py::test_run_job_bounds_sessiondb_finalization. It's a wall-clock-bounded finalization test and passes in isolation; I'm calling it flaky rather than claiming this PR fixed something.tests/cron/test_cron_provider_pin.py,test_cron_drift_alert_once.pyandtests/hermes_cli/test_cron_model_impact.py— the files that pin #44585's and #73532's contracts for these same messages — all pass unchanged.Overlap with open PRs
Ran the duplicate gate over #89242 before starting: no open or merged PR references it, and no open PR touches the drift-guard remediation strings in
cron/scheduler.pyorhermes_cli/config.py.The relevant prior art is all merged, and this composes with it rather than competing:
config settime, for #59031cron.model/cron.model_provider+ the guard exemptionWhat this deliberately does not do
I did not implement the fix #89242 proposes, and I'd push back on it. Returning
Nonefor both snapshots when both axes are inherit-mode removes the guard from exactly the jobs it was written for — after that change, there is no job left anywhere that it protects. The scenario in the issue's own reproduction is #44585's scenario: an unattended job silently following the global default onto a paid model. The no-snapshot back-compat path the issue cites is narrower than it looks; the code comment calls it "Back-compat" because it's a migration affordance for jobs created before #44585 shipped and for axes whose creation-time resolution failed, not a policy statement.This was also settled once already. @teknium1 closed #61468 with:
So the reporter's actual need has two supported answers today (
cron.modelfor "route my fleet deliberately",cron.model_drift_guard: falsefor "let my fleet track the global"). What was missing is that the runtime never mentions either. That's the defect this PR fixes, and it's message-only precisely so it can't prejudge a policy call that isn't mine to make. If a maintainer does want inherit-mode snapshots reconsidered on the merits, that's a separate change and I've kept it out of here.I also left the docs alone — they're already correct and already say all three things. The gap was one-directional.
Checklist
Code
main)pytest tests/ -qwholesale: on Windowstests/hermes_cli/can't be collected (test_doctor_journal_modes.pycallsos.geteuid), so a full-suite number from here would be meaningless. CI runs it.Documentation & Housekeeping
website/docs/user-guide/features/cron.mdalready documents all three remedies correctly; this PR brings the runtime up to the docs, not the other way roundcron.model/cron.model_provider/cron.model_drift_guardall already exist)