fix(cron): null-safe deliver in cron list + re-resolve BSM secrets per run

Two live cron bugs, both surfaced by @banditburai in #35616 (whose larger
watchdog/supervisor work is already superseded by the CronScheduler provider
refactor on main):

- #32896: `cron list` crashed on a present-but-null `deliver` field —
  `job.get("deliver", ["local"])` returns None for an explicit null, which
  then hit `", ".join(None)`. Coalesce with `or ["local"]` (same pitfall
  the sibling `repeat` line already guards against).

- #33465: cron jobs 401'd on Bitwarden/BSM-backed secrets. The per-run env
  reload used a bare `load_dotenv(override=True)`, which re-applied only the
  .env placeholder — startup had already recorded this HERMES_HOME in
  env_loader._APPLIED_HOMES, so the external-secret re-pull no-oped. Route the
  reload through load_hermes_dotenv() and call reset_secret_source_cache()
  first to force the re-pull (Bitwarden's 300s value-cache keeps it off the
  network; override honours secrets.bitwarden.override_existing, mirroring
  startup).

Tests: null-deliver regression guard in test_cron.py; reset-before-reload
ordering guard in test_scheduler.py. Migrated 31 scheduler-reload test seams
from patching dotenv.load_dotenv to the new load_hermes_dotenv /
reset_secret_source_cache seam.
This commit is contained in:
teknium1 2026-07-01 00:39:00 -07:00 committed by Teknium
parent cf427ccf08
commit 836732f54f
5 changed files with 148 additions and 38 deletions

View file

@ -2225,12 +2225,23 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]:
try:
# Re-read .env and config.yaml fresh every run so provider/key
# changes take effect without a gateway restart.
from dotenv import load_dotenv
try:
load_dotenv(str(_get_hermes_home() / ".env"), override=True, encoding="utf-8")
except UnicodeDecodeError:
load_dotenv(str(_get_hermes_home() / ".env"), override=True, encoding="latin-1")
# changes take effect without a gateway restart. Route through
# load_hermes_dotenv (not a bare load_dotenv) and reset the secret-
# source cache first: startup already applied external secrets and
# recorded this HERMES_HOME in _APPLIED_HOMES, so a naive reload would
# re-apply only the .env placeholder and never re-resolve a Bitwarden/
# BSM-backed secret — leaving cron jobs 401'ing on the placeholder
# (#33465). Clearing the cache forces the re-pull; the resolved secret
# overrides the placeholder only when secrets.bitwarden.override_existing
# is set (mirrors startup), and the Bitwarden value-cache keeps the
# forced re-pull off the network. load_hermes_dotenv also handles the
# utf-8/latin-1 encoding fallback internally.
from hermes_cli.env_loader import (
load_hermes_dotenv,
reset_secret_source_cache,
)
reset_secret_source_cache()
load_hermes_dotenv(hermes_home=_get_hermes_home())
delivery_target = _resolve_delivery_target(job)
if delivery_target:

View file

@ -137,7 +137,11 @@ def cron_list(show_all: bool = False):
repeat_completed = repeat_info.get("completed", 0)
repeat_str = f"{repeat_completed}/{repeat_times}" if repeat_times else ""
deliver = job.get("deliver", ["local"])
# `deliver` may be present-but-null in the job record (same pitfall as
# `repeat` above), so coalesce to the default rather than relying on the
# dict-default, which only applies to a missing key. A null value would
# otherwise reach `", ".join(None)` and crash the whole listing (#32896).
deliver = job.get("deliver") or ["local"]
if isinstance(deliver, str):
deliver = [deliver]
deliver_str = ", ".join(deliver)

View file

@ -52,7 +52,8 @@ def _run_with_current_provider(job, current_provider, tmp_path):
fake_db = MagicMock()
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -252,7 +253,8 @@ def _run_with_current_provider_and_model(job, current_provider, current_model, t
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._get_hermes_home", return_value=tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",

View file

@ -966,7 +966,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1012,7 +1013,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1055,7 +1057,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1095,7 +1098,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1132,7 +1136,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1169,7 +1174,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1197,7 +1203,8 @@ class TestRunJobSessionPersistence:
return fake_db, [
patch("cron.scheduler._hermes_home", tmp_path),
patch("cron.scheduler._resolve_origin", return_value=None),
patch("dotenv.load_dotenv"),
patch("hermes_cli.env_loader.load_hermes_dotenv"),
patch("hermes_cli.env_loader.reset_secret_source_cache"),
patch("hermes_state.SessionDB", return_value=fake_db),
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1336,7 +1343,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1412,7 +1420,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1451,7 +1460,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1493,7 +1503,8 @@ class TestRunJobSessionPersistence:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -1613,6 +1624,53 @@ class TestRunJobSessionPersistence:
assert os.getenv("HERMES_CRON_AUTO_DELIVER_THREAD_ID") is None
fake_db.close.assert_called_once()
def test_run_job_resets_secret_source_cache_before_reload(self, tmp_path, monkeypatch):
"""Each run must clear the secret-source cache before re-reading the
env, so a long-running gateway re-resolves Bitwarden/BSM-backed secrets
instead of leaving the startup .env placeholder in place (#33465).
A bare ``load_dotenv`` re-load can't do this: startup already recorded
this HERMES_HOME in ``_APPLIED_HOMES``, so the external-secret pull
no-ops and only the placeholder is re-applied. The scheduler must call
``reset_secret_source_cache()`` (forcing the re-pull) and route through
``load_hermes_dotenv`` (which then re-applies external secret sources).
"""
job = {"id": "bsm-job", "name": "bsm", "prompt": "hello"}
fake_db = MagicMock()
call_order = []
def _record_reset():
call_order.append("reset")
def _record_load(*args, **kwargs):
call_order.append("load")
return []
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("hermes_cli.env_loader.reset_secret_source_cache", _record_reset), \
patch("hermes_cli.env_loader.load_hermes_dotenv", _record_load), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
return_value={
"api_key": "***",
"base_url": "https://example.invalid/v1",
"provider": "openrouter",
"api_mode": "chat_completions",
},
), \
patch("run_agent.AIAgent") as mock_agent_cls:
mock_agent = MagicMock()
mock_agent.run_conversation.return_value = {"final_response": "ok"}
mock_agent_cls.return_value = mock_agent
success, _output, _final, error = run_job(job)
assert success is True
assert error is None
# reset MUST precede the reload, else _APPLIED_HOMES no-ops the re-pull.
assert call_order[:2] == ["reset", "load"], call_order
def test_run_job_clears_stale_auto_delivery_thread_id_between_jobs(self, tmp_path, monkeypatch):
jobs = [
{
@ -1709,7 +1767,8 @@ class TestRunJobConfigLogging:
# (>30s wall clock) under load. See PR #33661 follow-up.
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value={"provider": "openrouter", "api_key": "x",
"base_url": "https://example.invalid",
@ -1743,7 +1802,8 @@ class TestRunJobConfigLogging:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value={"provider": "openrouter", "api_key": "x",
"base_url": "https://example.invalid",
@ -1781,7 +1841,8 @@ class TestRunJobConfigEnvVarExpansion:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1814,7 +1875,8 @@ class TestRunJobConfigEnvVarExpansion:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1844,7 +1906,8 @@ class TestRunJobConfigEnvVarExpansion:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1873,7 +1936,8 @@ class TestRunJobConfigEnvVarExpansion:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1917,7 +1981,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1941,7 +2006,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1973,7 +2039,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -1996,7 +2063,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -2025,7 +2093,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -2051,7 +2120,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -2081,7 +2151,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -2105,7 +2176,8 @@ class TestRunJobModelResolution:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch("hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=self._RUNTIME), \
@ -2147,7 +2219,8 @@ class TestRunJobSkillBacked:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -2207,7 +2280,8 @@ class TestRunJobSkillBacked:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("tools.credential_files._resolve_hermes_home", return_value=tmp_path), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -2245,7 +2319,8 @@ class TestRunJobSkillBacked:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
@ -2291,7 +2366,8 @@ class TestRunJobSkillBacked:
with patch("cron.scheduler._hermes_home", tmp_path), \
patch("cron.scheduler._resolve_origin", return_value=None), \
patch("dotenv.load_dotenv"), \
patch("hermes_cli.env_loader.load_hermes_dotenv"), \
patch("hermes_cli.env_loader.reset_secret_source_cache"), \
patch("hermes_state.SessionDB", return_value=fake_db), \
patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",

View file

@ -122,6 +122,23 @@ class TestCronCommandLifecycle:
out = capsys.readouterr().out
assert "Repeat: ∞" in out
def test_list_does_not_crash_when_deliver_is_null(self, tmp_cron_dir, capsys):
"""A job can be persisted with ``"deliver": null`` (present-but-null).
`cron list` must fall back to the default channel rather than crashing
on ``", ".join(None)`` same dict-default pitfall as ``repeat`` (#32896).
"""
from cron.jobs import load_jobs, save_jobs
create_job(prompt="No deliver", schedule="every 1h")
jobs = load_jobs()
jobs[0]["deliver"] = None
save_jobs(jobs)
cron_command(Namespace(cron_command="list", all=True))
out = capsys.readouterr().out
assert "Deliver: local" in out
class TestGatewayNotRunningWarning:
"""`cron create` / `cron list` must warn when the gateway (and thus the