diff --git a/cron/jobs.py b/cron/jobs.py index 0afd293c70..385d747619 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -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 ) diff --git a/cron/scheduler.py b/cron/scheduler.py index 3dd9b67d4b..856f2dc283 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -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: diff --git a/tests/cron/test_cron_failure_deliver.py b/tests/cron/test_cron_failure_deliver.py index dab0e061ea..136a4e8363 100644 --- a/tests/cron/test_cron_failure_deliver.py +++ b/tests/cron/test_cron_failure_deliver.py @@ -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" diff --git a/tools/cronjob_tools.py b/tools/cronjob_tools.py index 54015836d0..cdcdc6cea5 100644 --- a/tools/cronjob_tools.py +++ b/tools/cronjob_tools.py @@ -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)