From 00295a916ac5680a4d81d49aedb8c6a0ea90cca5 Mon Sep 17 00:00:00 2001 From: Michael Lam Date: Sun, 17 May 2026 01:36:46 -0700 Subject: [PATCH] fix: deliver manual cron run results --- CHANGELOG.md | 4 + api/routes.py | 38 +++++++- tests/test_cron_manual_run_persistence.py | 114 ++++++++++++++++++---- 3 files changed, 133 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93fb8cdd..7b618775 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- **PR #2452** by @Michaelyklam (fixes #2451) — Manual WebUI cron triggers now deliver the same final response or failure notice as scheduled cron runs, while still saving output files and recording delivery errors separately from job execution failures. + ## [v0.51.82] — 2026-05-17 — Release BF (stage-375 — 2-PR batch — table renderer pipe protection + Catppuccin appearance skin) ### Added diff --git a/api/routes.py b/api/routes.py index 677a6677..c6056d02 100644 --- a/api/routes.py +++ b/api/routes.py @@ -754,8 +754,15 @@ def _run_cron_tracked(job, profile_home=None, execution_profile_home=None): agent config/.env while running. When no job profile is selected, both homes are the same and legacy server-default behavior is preserved. """ + import importlib + from cron.jobs import mark_job_run, save_job_output + _cron_scheduler = importlib.import_module("cron.scheduler") + + _silent_marker = getattr(_cron_scheduler, "SILENT_MARKER", "[SILENT]") + _deliver_result = getattr(_cron_scheduler, "_deliver_result", None) + job_id = job.get("id", "") execution_profile_home = execution_profile_home or profile_home @@ -772,11 +779,29 @@ def _run_cron_tracked(job, profile_home=None, execution_profile_home=None): job, execution_profile_home ) - # Persist output and run metadata back to the job's owning cron store, - # even when the selected execution profile is different. + # Persist output, deliver the same content the scheduled cron path would + # send, and write run metadata back to the job's owning cron store even + # when the selected execution profile is different. def _persist_success(): save_job_output(job_id, output) + deliver_content = ( + final_response + if success + else f"⚠️ Cron job '{job.get('name', job_id)}' failed:\n{error}" + ) + should_deliver = bool(deliver_content) + if should_deliver and success and _silent_marker in deliver_content.strip().upper(): + should_deliver = False + + delivery_error = None + if should_deliver and _deliver_result is not None: + try: + delivery_error = _deliver_result(job, deliver_content) + except Exception as de: + delivery_error = str(de) + logger.error("Delivery failed for manual cron job %s: %s", job_id, de) + # Match the scheduled cron path: an apparently successful run with no # final response should not leave the job looking healthy. _success, _error = success, error @@ -784,7 +809,14 @@ def _run_cron_tracked(job, profile_home=None, execution_profile_home=None): _success = False _error = "Agent completed but produced empty response (model error, timeout, or misconfiguration)" - mark_job_run(job_id, _success, _error) + try: + mark_job_run(job_id, _success, _error, delivery_error=delivery_error) + except TypeError: + # Older/fake cron.jobs modules used by focused WebUI tests may + # not expose the newer delivery_error parameter. Real Hermes + # scheduler builds do, so this is only a compatibility shim for + # legacy test doubles and deployments. + mark_job_run(job_id, _success, _error) _with_cron_home(profile_home, _persist_success) except Exception as e: diff --git a/tests/test_cron_manual_run_persistence.py b/tests/test_cron_manual_run_persistence.py index 49943b63..7d281592 100644 --- a/tests/test_cron_manual_run_persistence.py +++ b/tests/test_cron_manual_run_persistence.py @@ -1,21 +1,32 @@ """Regression tests for manual WebUI cron runs.""" - -def test_manual_cron_run_saves_output_and_marks_job(monkeypatch): - import api.routes as routes - - calls = [] - +def _install_cron_fakes(monkeypatch, calls, deliver_result=None, silent_marker="[SILENT]"): cron_jobs = type("CronJobs", (), {})() cron_jobs.save_job_output = lambda job_id, output: calls.append( ("save", job_id, output) ) - cron_jobs.mark_job_run = lambda job_id, success, error=None: calls.append( - ("mark", job_id, success, error) + cron_jobs.mark_job_run = lambda job_id, success, error=None, delivery_error=None: calls.append( + ("mark", job_id, success, error, delivery_error) ) + cron_scheduler = type("CronScheduler", (), {})() + cron_scheduler.SILENT_MARKER = silent_marker + if deliver_result is None: + deliver_result = lambda job, content: calls.append( + ("deliver", job["id"], content) + ) or None + cron_scheduler._deliver_result = deliver_result + monkeypatch.setitem(__import__("sys").modules, "cron.jobs", cron_jobs) + monkeypatch.setitem(__import__("sys").modules, "cron.scheduler", cron_scheduler) + + +def test_manual_cron_run_saves_output_delivers_and_marks_job(monkeypatch): + import api.routes as routes + + calls = [] + _install_cron_fakes(monkeypatch, calls) monkeypatch.setattr( routes, "_run_cron_job_in_profile_subprocess", @@ -27,25 +38,17 @@ def test_manual_cron_run_saves_output_and_marks_job(monkeypatch): assert calls == [ ("save", "job123", "manual output"), - ("mark", "job123", True, None), + ("deliver", "job123", "done"), + ("mark", "job123", True, None, None), ] assert routes._is_cron_running("job123") == (False, 0.0) -def test_manual_cron_run_marks_empty_response_as_failure(monkeypatch): +def test_manual_cron_run_marks_empty_response_as_failure_without_delivery(monkeypatch): import api.routes as routes calls = [] - - cron_jobs = type("CronJobs", (), {})() - cron_jobs.save_job_output = lambda job_id, output: calls.append( - ("save", job_id, output) - ) - cron_jobs.mark_job_run = lambda job_id, success, error=None: calls.append( - ("mark", job_id, success, error) - ) - - monkeypatch.setitem(__import__("sys").modules, "cron.jobs", cron_jobs) + _install_cron_fakes(monkeypatch, calls) monkeypatch.setattr( routes, "_run_cron_job_in_profile_subprocess", @@ -58,4 +61,75 @@ def test_manual_cron_run_marks_empty_response_as_failure(monkeypatch): assert calls[0] == ("save", "job-empty", "manual output") assert calls[1][0:3] == ("mark", "job-empty", False) assert "empty response" in calls[1][3] + assert calls[1][4] is None assert routes._is_cron_running("job-empty") == (False, 0.0) + + +def test_manual_cron_run_records_delivery_errors_separately(monkeypatch): + import api.routes as routes + + calls = [] + + def fail_delivery(job, content): + calls.append(("deliver", job["id"], content)) + return "discord not configured" + + _install_cron_fakes(monkeypatch, calls, deliver_result=fail_delivery) + monkeypatch.setattr( + routes, + "_run_cron_job_in_profile_subprocess", + lambda job, execution_profile_home: (True, "manual output", "done", None), + ) + + routes._mark_cron_running("job-delivery-error") + routes._run_cron_tracked({"id": "job-delivery-error"}) + + assert calls == [ + ("save", "job-delivery-error", "manual output"), + ("deliver", "job-delivery-error", "done"), + ("mark", "job-delivery-error", True, None, "discord not configured"), + ] + assert routes._is_cron_running("job-delivery-error") == (False, 0.0) + + +def test_manual_cron_run_skips_silent_success_delivery(monkeypatch): + import api.routes as routes + + calls = [] + _install_cron_fakes(monkeypatch, calls) + monkeypatch.setattr( + routes, + "_run_cron_job_in_profile_subprocess", + lambda job, execution_profile_home: (True, "manual output", "[SILENT]", None), + ) + + routes._mark_cron_running("job-silent") + routes._run_cron_tracked({"id": "job-silent"}) + + assert calls == [ + ("save", "job-silent", "manual output"), + ("mark", "job-silent", True, None, None), + ] + assert routes._is_cron_running("job-silent") == (False, 0.0) + + +def test_manual_cron_run_delivers_failure_notice(monkeypatch): + import api.routes as routes + + calls = [] + _install_cron_fakes(monkeypatch, calls) + monkeypatch.setattr( + routes, + "_run_cron_job_in_profile_subprocess", + lambda job, execution_profile_home: (False, "manual output", "", "boom"), + ) + + routes._mark_cron_running("job-failed") + routes._run_cron_tracked({"id": "job-failed", "name": "Nightly check"}) + + assert calls[0] == ("save", "job-failed", "manual output") + assert calls[1][0:2] == ("deliver", "job-failed") + assert "Nightly check" in calls[1][2] + assert "boom" in calls[1][2] + assert calls[2] == ("mark", "job-failed", False, "boom", None) + assert routes._is_cron_running("job-failed") == (False, 0.0)