fix(cron): delivery failures record last_status=delivery_failed and surface in list/doctor/manual run (#83993, salvage #100163 #86622) - #100916
Merged
Conversation
Contributor
૮ >ﻌ< ა ci reviewran on dcd9d29 — fix(cron): every last_status consumer renders delivery_faile
|
alt-glitch
added
type/bug
Contributor
|
@/tmp/comment-100916.md |
A successful agent run whose delivery failed used to persist last_status=ok and bury the failure in last_delivery_error. CLI list painted that as green and the run looked identical to a quiet success. Record last_status=delivery_failed instead, keep last_delivery_error, do not increment failure_streak, and teach cron list/doctor not to treat it as ok. Fixes #83993
…happened The _execute_job_now completion notice unconditionally claimed "(output was delivered there by the job itself)" for non-local delivery targets, even when the job record's last_delivery_error showed the delivery failed (#83993). Derive the note from the refreshed job record so a failed delivery is reported honestly to the calling agent.
Review follow-up on the #83993 fix: a stored falsy deliver ("", JSON null) fell through the local check and produced 'output was delivered there by the job itself' for a target that does not exist — the exact false-delivery-claim class the PR removes. Fire time already normalizes falsy deliver to local (no delivery, output persisted in last_output, no delivery error), so the summary now canonicalizes with the scheduler's own _normalize_deliver_value and reads saved-locally. Whitespace-only deliver is deliberately not folded in: fire time records 'no delivery target resolved' for it, and the error-driven FAILED wording must stay visible.
Main grew claim_job_for_fire(job_id, return_job=True) — a claimed snapshot dict instead of a bool — while this branch sat on an older base. The merge-ref CI ran the hybrid: the wiring tests still mocked return_value=True, which fails isinstance(claimed_job, dict) and fell into the 'already being fired' branch, so every dispatch assert failed. Mock the claim to return the job snapshot (the API's success shape), read the summary's deliver from the claimed snapshot the run actually executes, and keep the dispatch-result failure renderer. Rebased onto current main; cron suite 710 passed.
…or the distinct status A manual cronjob(action='run') derived success from last_status == 'ok' and read the error from last_error — so a run that now records delivery_failed came back as success=False with error=None, an unexplained failure. Surface last_delivery_error as the error in that case (the #84006 direction, re-applied on the delivery_failed status), and pin the manual-run completion summary to say 'Result: FAILED' over an undelivered run. Document the status in the cron user guide. Co-authored-by: webtecnica <webtecnica@gmail.com>
…tly (dashboard badge, Desktop inspector, /cron list, docs)
Audit of every last_status reader outside the scheduler (rg last_status across
web/, apps/desktop/, hermes_cli/, tui_gateway/, tools/, scripts/, website/):
- web dashboard CronPage: last_status was never rendered at all — a
delivery_failed job showed a green 'scheduled' badge and only a small red
'delivery: ...' line. New pure cronLastResult() helper maps the closed
literal set to tones (ok=success, delivery_failed/blocked_config=warning,
error/unknown=destructive) and the card now shows an amber
'delivery_failed' badge (title = last_delivery_error).
- Desktop hermes-bots routine inspector: 'Last result' printed the raw
literal; routineLastResult() spells out each one ('Ran, but delivery
failed', 'Blocked by configuration (not run)', ...), unknown passes through.
- /cron list (cli_commands_mixin): 'Last run: <ts> (delivery_failed)' now
appends the delivery reason, since last_error is None for those runs.
- hermes cron list/doctor and the cronjob tool already handled the literal
on this branch; no consumer compared == 'ok' for success apart from the
cronjob manual-run path, which the branch already fixed.
- developer-guide/cron-internals.md: table of last_status literals + which
detail field carries the reason.
Live repro (real 'hermes dashboard' on a temp HERMES_HOME with a
delivery_failed job, CronPage rendered against the live /api/cron/jobs):
before — badges [scheduled, default, telegram:123]; after — badges
[scheduled, delivery_failed (warning tone, title 'telegram: 502 Bad
Gateway'), default, telegram:123].
teknium1
force-pushed
the
p1/cron-delivery-2
branch
from
September 2, 2026 07:31
fd270cb to
dcd9d29
Compare
This was referenced Sep 2, 2026
Contributor
|
Thanks @teknium1 for the salvage and the clean landing — authorship on the three cherry-picked commits confirmed on my side, and the Happy to help if direction-3 work (cached-output redelivery, #16645) ever picks up. Nice working together on this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A cron run whose agent succeeded but whose output never reached the user is no longer recorded or shown as
ok: it gets a distinctlast_status: delivery_failed,hermes cron listrenders it yellow with the reason,cron doctorreports it as a delivery issue, and a manualcronjob runreportssuccess: falsewith the delivery error instead of a green result.Changes
cron/jobs.py::mark_job_run(salvage fix(cron): surface delivery_failed instead of last_status ok #100163 @jwilson411) — deriveslast_statusaserror>delivery_failed>ok(an explicitstatus=override such asblocked_configstill wins).failure_streakis untouched by delivery failures (the agent did its job);last_errorstaysNone. This is the storage-level fix the issue asked for: everything that keys off"ok"now sees the failure.hermes_cli/cron.py(fix(cron): surface delivery_failed instead of last_status ok #100163) —cron listrendersdelivery_failed: <last_delivery_error>in yellow (the "⚠ Delivery failed" detail line stays);cron doctorno longer double-reports a delivery failure as "last run failed: unknown error".tools/cronjob_tools.py::_run_claimed_job(re-applies the fix(cron): surface delivery failures instead of hiding under last_status ok #84006 @webtecnica direction) — a manual run returnssuccess: falseand surfaceslast_delivery_erroras the error when the record isdelivery_failed; previously it derived success from== "ok"and read onlylast_error, so a delivery-failed run came back as an unexplained failure.tools/cronjob_tools.py::_manual_run_delivery_note(salvage fix(cron): stop manual-run notice from asserting delivery that never happened #86622 @strzhao) — the manual-run completion summary no longer appends "output was delivered there by the job itself" over a failed delivery; it follows the refreshed record (⚠ delivery FAILED: <reason>), treats a falsydeliveras local, and reads the target from the claimed snapshot. Pinned so the headline readsResult: FAILEDrather thanResult: okfor such runs.website/docs/user-guide/features/cron.mdgains a "Delivery failures are a distinct status" section.Not taken: #84006's
"ok (delivery failed)"string status (a compound value every== "ok"reader would have to parse;delivery_failedis the cleaner enum) and #89132'sdelivery_state == "pending" && delivery_attempts == 0heuristic (those fields are not written anywhere on main; the storage-level status makes the CLI heuristic unnecessary).Validation
tests/cron/test_jobs.py,tests/hermes_cli/test_cron.py(new: delivery_failed status, override precedence, streak, doctor)tests/tools/test_cronjob_run_delivery_notice.py(new, 15 tests incl. real background-dispatch summary)tests/tools/test_cronjob_run_immediate.py(+2: manual run reports delivery_failed)test_cronjob_run_background.py,test_cronjob_tools.py,test_run_one_job.py,test_preflight_config.py,test_scheduler.py,test_cron_incidents.py,test_execution_ledger.py_run_claimed_jobchange →test_delivery_failed_status_is_not_success_and_surfaces_reasonruff checkon touched filesLive repro: real
cron.jobsstore under an isolatedHERMES_HOME, realmark_job_run(job, True, None, delivery_error="live adapter send failed: 502 ...")(the exact callrun_jobmakes), then the realhermes cron listrenderer — before (origin/main): recordlast_status='ok', list showsLast run: 2026-09-01T21:22:33 okwith the failure only on a secondary⚠ Delivery failed:line;_format_job→last_status='ok'. After: recordlast_status='delivery_failed', list showsLast run: ... delivery_failed: live adapter send failed: 502 Bad Gateway (target telegram:5160665427);_format_job→last_status='delivery_failed'.Closes #83993
Salvages #100163 (@jwilson411 — storage + CLI status; cherry-picked with authorship) and #86622 (@strzhao, issue author — manual-run notice honesty; 3 commits cherry-picked with authorship). Supersedes #84006 (@webtecnica — earliest submission for this issue; its tool-result direction is re-applied here with co-author credit) and #89132 (@LeonardoLGDS — CLI-only rendering, superseded by the status-level fix).
Infographic
De-risking (commit
dcd9d294415)Audit of every
last_statusconsumer outside the scheduler (rg -n last_statusacrossweb/,apps/desktop/,hermes_cli/,tui_gateway/,tools/,scripts/,website/; theauth.py/web_server.py:14288/video-gen hits are unrelated credential-pool / polling fields):CronPage.tsxlast_statusnever rendered — adelivery_failedjob showed a greenscheduledbadge + a small reddelivery:linecronLastResult()(web/src/lib/cron-job.ts) maps the closed literal set to tones (ok→success,delivery_failed/blocked_config→warning,error/unknown→destructive); card shows an amberdelivery_failedbadge (title=last_delivery_error)cron.tsx)Last resultprinted the raw literalroutineLastResult()spells out each literal (Ran, but delivery failed,Blocked by configuration (not run), …); unknown passes through/cron list(cli_commands_mixin.py)Last run: <ts> (delivery_failed)with no reason (last_errorisNone)(delivery_failed: telegram: 502 Bad Gateway)hermes cron list/cron doctor/cronjobtool== "ok"for successtools/cronjob_tools.pymanual-run path — already fixed by this branch== "ok"developer-guide/cron-internals.md: table oflast_statusliterals + which field carries the detailhermes dashboardon a tempHERMES_HOMEseeded with adelivery_failedjob, real/api/cron/jobs,CronPagerendered against it): before — card badges[scheduled, default, telegram:123]; after —[scheduled, delivery_failed (border-warning/30 bg-warning/15 text-warning, title "telegram: 502 Bad Gateway", data-testid cron-last-result), default, telegram:123].web/src/lib/cron-job.test.ts+5 (cronLastResult),apps/desktop/src/plugins/hermes-bots/cron-detail.test.tsx+1 (routineLastResultfor every literal),tests/hermes_cli/test_cron.py+2 (TestSlashCronListLastStatus). Runs: web vitest 15 passed, desktop vitest 10 passed,tsc --noEmit+ eslint clean in both, Python 160 passed (test_cron.py,test_cronjob_run_delivery_notice.py,test_cronjob_run_immediate.py,test_jobs.py).