ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

fix(cron): delivery bookkeeping reads the failure lane it actually routed through

Browse Source 
Review findings (Salt, NS-788):

B1: delivery_outcome classification, unresolved_origin, and incident
'alerted' marking all read the deliver lane while the notice itself was
routed through failure_deliver — a silenced failure recorded
delivery_outcome='delivered' and marked its incident alerted (corrupting
the 'failure seen' vs 'operator was pinged' distinction the incident
store documents), and a failure delivered via failure_deliver over an
unresolvable deliver=origin recorded 'not_configured'. New
_delivery_lane_value() helper feeds the SAME lane to routing and
bookkeeping at all five sites (both classifiers, both unresolved_origin
computations, both zero-target checks). Three regression tests assert
outcome + alerted-marking; verified to bite on the pre-fix classifier.

S1: failure_deliver now goes through _resolve_cron_context_deliver on
tool create/update, matching deliver — a job created from inside a cron
run can no longer store literal 'origin' in its failure lane.

S2/T1: corrected the false 'same helper' comment in create_job; the
str/list flatten mirrors the tool layer for direct callers.

Full cron suite + interrupt tests: 87 files, 1112 passed, 0 failed.
This commit is contained in:
Victor Kyriazakos
2026-09-01 14:45:23 +00:00
committed by kshitij
parent c9491e6a7d
commit fd35e1ec5a
4 changed files with 103 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cron/jobs.py
+4 -2
View File
@@ -2453,8 +2453,10 @@ def create_job(
normalized_no_agent = bool(no_agent)
normalized_attach = attach_to_session if isinstance(attach_to_session, bool) else None
normalized_reasoning_effort = _normalize_reasoning_effort(reasoning_effort)
# failure_deliver shares deliver's grammar and normalization exactly —
# same helper, no parallel validation path (NS-788).
# failure_deliver shares deliver's value grammar; the str/list
# flatten below mirrors the tool layer's _normalize_deliver_param for
# direct create_job callers (the tool pre-normalizes). Semantic
# validation happens at resolution time via the shared deliver path.
normalized_failure_deliver = (
str(failure_deliver).strip() if isinstance(failure_deliver, str) else None
)
cron/scheduler.py
+27 -14
View File
@@ -2838,6 +2838,19 @@ def _expand_routing_tokens(part: str) -> List[str]:
return expanded
def _delivery_lane_value(job: dict, *, for_failure: bool = False):
"""Raw deliver-lane value for a run outcome: the failure lane when
``for_failure`` and the job overrides it, else ``deliver``. Keeps
delivery bookkeeping (outcome classification, unresolved-origin,
incident 'alerted' marking) reading the SAME lane the notice was
actually routed through (NS-788 review finding B1)."""
if for_failure:
failure_deliver = job.get("failure_deliver")
if failure_deliver is not None and str(failure_deliver).strip():
return failure_deliver
return job.get("deliver", "local")
def _resolve_delivery_targets(job: dict, *, for_failure: bool = False) -> List[dict]:
"""Resolve all concrete auto-delivery targets for a cron job.
@@ -2856,11 +2869,7 @@ def _resolve_delivery_targets(job: dict, *, for_failure: bool = False) -> List[d
Absent ``failure_deliver``, failure delivery follows ``deliver``
exactly as before.
"""
deliver_raw = job.get("deliver", "local")
if for_failure:
failure_deliver = job.get("failure_deliver")
if failure_deliver is not None and str(failure_deliver).strip():
deliver_raw = failure_deliver
deliver_raw = _delivery_lane_value(job, for_failure=for_failure)
deliver = _normalize_deliver_value(deliver_raw)
if deliver == "local":
return []
@@ -3172,10 +3181,9 @@ def _deliver_result(
"""
targets = _resolve_delivery_targets(job, for_failure=for_failure)
if not targets:
deliver_raw = job.get("deliver", "local")
if for_failure and str(job.get("failure_deliver") or "").strip():
deliver_raw = job.get("failure_deliver")
deliver_value = _normalize_deliver_value(deliver_raw)
deliver_value = _normalize_deliver_value(
_delivery_lane_value(job, for_failure=for_failure)
)
if deliver_value == "local":
return None # local-only jobs don't deliver — not a failure
# deliver=origin with no resolvable origin and no configured home
@@ -7697,8 +7705,9 @@ def _run_one_job_body(
if should_deliver:
unresolved_origin = (
_normalize_deliver_value(job.get("deliver", "local")) == "origin"
and not _resolve_delivery_targets(job)
_normalize_deliver_value(_delivery_lane_value(job, for_failure=not success))
== "origin"
and not _resolve_delivery_targets(job, for_failure=not success)
)
try:
with _side_effect_fence() as owns_delivery:
@@ -7801,7 +7810,9 @@ def _run_one_job_body(
error="Fire claim ownership lost before terminal completion.",
)
return True
normalized_deliver = _normalize_deliver_value(job.get("deliver", "local"))
normalized_deliver = _normalize_deliver_value(
_delivery_lane_value(job, for_failure=not success)
)
if delivery_error:
delivery_outcome = "failed"
elif should_deliver and unresolved_origin:
@@ -7858,7 +7869,7 @@ def _run_one_job_body(
and not _fire_claim_ownership_lost()
):
normalized_deliver = _normalize_deliver_value(
job.get("deliver", "local")
_delivery_lane_value(job, for_failure=True)
)
unresolved_origin = False
# Durable failure incident: same ack gate as the normal failure
@@ -7892,7 +7903,9 @@ def _run_one_job_body(
"Delivery failed for job %s: %s", job["id"], delivery_exc
)
if not delivery_error and normalized_deliver == "origin":
unresolved_origin = not _resolve_delivery_targets(job)
unresolved_origin = not _resolve_delivery_targets(
job, for_failure=True
)
if delivery_error:
delivery_outcome = "failed"
elif unresolved_origin:
tests/cron/test_cron_failure_deliver.py
+64
View File
@@ -341,3 +341,67 @@ class TestToolSurface:
))
assert result["success"] is True
assert not get_job(job["id"]).get("failure_deliver")
class TestOutcomeBookkeeping:
"""Review finding B1 (NS-788): delivery bookkeeping — outcome
classification, unresolved-origin, incident 'alerted' marking — must
read the SAME lane the notice was actually routed through, or the
execution history and incident store record lies (silenced failures
logged 'delivered'; delivered failures logged 'not_configured')."""
@staticmethod
def _outcome(state):
assert state["finished"], "finish_execution never called"
_a, kw = state["finished"][-1]
return kw.get("delivery_outcome")
def test_fd_local_failure_records_suppressed_not_delivered(
self, run_env, monkeypatch
):
alerted = []
monkeypatch.setattr(s, "_mark_incident_alerted", alerted.append)
monkeypatch.setattr(s, "run_job", _failing_run_job())
s.run_one_job({
"id": "b1a", "name": "scout",
"deliver": "slack:D0MAIN", "failure_deliver": "local",
})
assert run_env["send"] == []
assert self._outcome(run_env) == "suppressed"
assert alerted == [], "silenced failure must NOT mark incident alerted"
def test_fd_explicit_target_failure_records_delivered(
self, run_env, monkeypatch
):
"""deliver=origin (unresolvable) + failure_deliver=explicit target:
the notice IS delivered — outcome must say so, not 'not_configured'."""
alerted = []
monkeypatch.setattr(s, "_mark_incident_alerted", alerted.append)
monkeypatch.setattr(
s, "_upsert_incident_for_failure", lambda *_a, **_kw: (False, "inc-b1")
)
monkeypatch.setattr(s, "run_job", _failing_run_job())
s.run_one_job({
"id": "b1b", "name": "scout",
"deliver": "origin", "failure_deliver": "slack:D0OPS",
})
assert [c["chat_id"] for c in run_env["send"]] == ["D0OPS"]
assert self._outcome(run_env) == "delivered"
assert alerted == ["inc-b1"], "delivered failure ping must mark incident alerted"
def test_success_outcome_still_reads_deliver_lane(self, run_env, monkeypatch):
"""Success bookkeeping is untouched: fd set, success delivers to
deliver and records 'delivered'."""
monkeypatch.setattr(s, "run_job", _succeeding_run_job())
s.run_one_job({
"id": "b1c", "name": "scout",
"deliver": "slack:D0MAIN", "failure_deliver": "local",
})
assert [c["chat_id"] for c in run_env["send"]] == ["D0MAIN"]
assert self._outcome(run_env) == "delivered"
tools/cronjob_tools.py
+8 -2
View File
@@ -1643,7 +1643,9 @@ def cronjob(
# dispatch below: models do not make model-config
# decisions (standing policy).
reasoning_effort=reasoning_effort,
failure_deliver=_normalize_deliver_param(failure_deliver),
failure_deliver=_resolve_cron_context_deliver(
_normalize_deliver_param(failure_deliver)
),
)
except CronSchedulerRegistrationError as exc:
_partial = exc.to_dict()
@@ -1848,12 +1850,16 @@ def cronjob(
)
if failure_deliver is not None:
# '' clears the override (job falls back to deliver on
# failures); non-empty values share deliver's validation.
# failures); non-empty values share deliver's validation
# AND its cron-context origin resolution (a job created
# from inside a cron run must never store literal
# 'origin' — same rule as deliver).
_norm_fd = _normalize_deliver_param(failure_deliver)
if _norm_fd:
bot_chat_error = _validate_bot_chat_deliver(_norm_fd)
if bot_chat_error:
return tool_error(bot_chat_error, success=False)
_norm_fd = _resolve_cron_context_deliver(_norm_fd)
updates["failure_deliver"] = _norm_fd
if skills is not None or skill is not None:
canonical_skills = _canonical_skills(skill, skills)