diff --git a/cron/scheduler.py b/cron/scheduler.py index 7b00bfcb0f34f..49439098e1a0c 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -1142,14 +1142,20 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d return None if deliver_value == "origin": - if origin: + if origin and _is_known_delivery_platform(origin.get("platform", "")): return { "platform": origin["platform"], "chat_id": str(origin["chat_id"]), "thread_id": origin.get("thread_id"), } - # Origin missing (e.g. job created via API/script) — try each - # platform's home channel as a fallback instead of silently dropping. + # Origin missing OR points at a platform that cannot receive cron + # delivery. The latter is the api_server trap (#69304): jobs created + # in an api_server session capture ``origin.platform="api_server"``, + # but that adapter's ``send()`` is a hardwired no-op (HTTP is + # request/response only). Trusting such an origin here makes the job + # look healthy (last_status=ok) while every fire silently fails to + # deliver. Treat an undeliverable origin like a missing one and fall + # back to configured home channels instead of silently dropping. for platform_name in _iter_home_target_platforms(): chat_id = _get_home_target_chat_id(platform_name) if chat_id: @@ -1209,7 +1215,7 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d } platform_name = deliver_value - if origin and origin.get("platform") == platform_name: + if origin and origin.get("platform") == platform_name and _is_known_delivery_platform(platform_name): chat_id = _get_home_target_chat_id(platform_name) if chat_id: return { diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 014e88ec6dbc0..45c5cca00a15e 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -13,6 +13,23 @@ from tools.env_passthrough import clear_env_passthrough from tools.credential_files import clear_credential_files +# Every env var the cron home-channel fallback can read. Clearing them all +# makes "no delivery target" assertions hermetic against a developer machine +# that happens to have one configured (e.g. WEIXIN_HOME_CHANNEL set globally). +_ALL_HOME_CHANNEL_ENV_VARS = ( + "MATRIX_HOME_ROOM", "TELEGRAM_HOME_CHANNEL", "DISCORD_HOME_CHANNEL", + "SLACK_HOME_CHANNEL", "SIGNAL_HOME_CHANNEL", "MATTERMOST_HOME_CHANNEL", + "SMS_HOME_CHANNEL", "EMAIL_HOME_ADDRESS", "DINGTALK_HOME_CHANNEL", + "FEISHU_HOME_CHANNEL", "WECOM_HOME_CHANNEL", "WEIXIN_HOME_CHANNEL", + "BLUEBUBBLES_HOME_CHANNEL", "QQBOT_HOME_CHANNEL", "QQ_HOME_CHANNEL", + "WHATSAPP_HOME_CHANNEL", "WHATSAPP_CLOUD_HOME_CHANNEL", +) + + +def _clear_all_home_channels(monkeypatch): + for var in _ALL_HOME_CHANNEL_ENV_VARS: + monkeypatch.delenv(var, raising=False) + class TestPerJobToolsetMcpMerge: """A per-job enabled_toolsets allowlist must not silently drop MCP servers.""" @@ -492,6 +509,63 @@ def test_explicit_discord_channel_without_thread(self): "thread_id": None, } + def test_api_server_origin_does_not_pretend_to_deliver(self, monkeypatch): + """#69304 — an api_server-origin job must NOT resolve deliver=origin to + the api_server itself. That adapter's send() is a hardwired no-op, so + trusting the origin would make the job look healthy (last_status=ok) + while every report silently failed to deliver. With no home channel + configured the target is unresolvable (None), not a fake api_server + target.""" + _clear_all_home_channels(monkeypatch) + + job = { + "deliver": "origin", + "origin": {"platform": "api_server", "chat_id": "sess-123"}, + } + assert _resolve_delivery_target(job) is None + + def test_api_server_origin_falls_back_to_home_channel(self, monkeypatch): + """#69304 — an undeliverable api_server origin is treated like a missing + one: deliver=origin falls back to a configured home channel so the + report actually lands somewhere instead of being silently dropped.""" + _clear_all_home_channels(monkeypatch) + monkeypatch.setenv("TELEGRAM_HOME_CHANNEL", "-4242") + + job = { + "deliver": "origin", + "origin": {"platform": "api_server", "chat_id": "sess-123"}, + } + assert _resolve_delivery_target(job) == { + "platform": "telegram", + "chat_id": "-4242", + "thread_id": None, + } + + def test_bare_api_server_platform_with_matching_origin_resolves_to_nothing(self, monkeypatch): + """#69304 — deliver='api_server' (origin matches) is not a usable + delivery target: the bare-platform branch must not trust an + undeliverable origin either.""" + _clear_all_home_channels(monkeypatch) + + job = { + "deliver": "api_server", + "origin": {"platform": "api_server", "chat_id": "sess-123"}, + } + assert _resolve_delivery_target(job) is None + + def test_deliverable_origin_still_resolves(self): + """The undeliverable-origin guard must not break a normal deliverable + origin (telegram): deliver=origin still resolves to that chat.""" + job = { + "deliver": "origin", + "origin": {"platform": "telegram", "chat_id": "-999", "thread_id": "42"}, + } + assert _resolve_delivery_target(job) == { + "platform": "telegram", + "chat_id": "-999", + "thread_id": "42", + } + def test_list_form_deliver_is_normalized(self, monkeypatch): """deliver=['telegram'] (Python list) should resolve like 'telegram' string. diff --git a/tests/tools/test_cronjob_tools.py b/tests/tools/test_cronjob_tools.py index 41aea33c7dc0a..15e66efbf9612 100644 --- a/tests/tools/test_cronjob_tools.py +++ b/tests/tools/test_cronjob_tools.py @@ -597,6 +597,19 @@ def _setup_cron_dir(self, tmp_path, monkeypatch): "HERMES_SESSION_CHAT_NAME", ): monkeypatch.delenv(var, raising=False) + # Clear every gateway home channel so "no delivery target" assertions + # are hermetic against a developer machine that has one configured + # (e.g. WEIXIN_HOME_CHANNEL set globally would otherwise make the + # origin-fallback find a target and suppress the notice). + for var in ( + "MATRIX_HOME_ROOM", "TELEGRAM_HOME_CHANNEL", "DISCORD_HOME_CHANNEL", + "SLACK_HOME_CHANNEL", "SIGNAL_HOME_CHANNEL", "MATTERMOST_HOME_CHANNEL", + "SMS_HOME_CHANNEL", "EMAIL_HOME_ADDRESS", "DINGTALK_HOME_CHANNEL", + "FEISHU_HOME_CHANNEL", "WECOM_HOME_CHANNEL", "WEIXIN_HOME_CHANNEL", + "BLUEBUBBLES_HOME_CHANNEL", "QQBOT_HOME_CHANNEL", "QQ_HOME_CHANNEL", + "WHATSAPP_HOME_CHANNEL", "WHATSAPP_CLOUD_HOME_CHANNEL", + ): + monkeypatch.delenv(var, raising=False) from gateway.session_context import clear_session_vars, set_session_vars tokens = set_session_vars() # reset ContextVars to empty @@ -657,6 +670,45 @@ def test_gateway_origin_no_notice(self, monkeypatch): assert created["deliver"] == "origin" assert "local-only cron job" not in created["message"] + def test_api_server_origin_emits_targeted_notice(self, monkeypatch): + """#69304 — an api_server origin can't deliver; flag it at create time. + + The api_server adapter's send() is a no-op, so deliver=origin would run + the job but silently drop every report. The create-time notice must + surface this with a platform-specific message (not the generic CLI/TUI + one) so the agent relays it instead of promising delivery. + """ + from gateway.session_context import set_session_vars + + # No home channels configured (the autouse fixture clears them), so the + # origin-fallback also finds nothing. + set_session_vars(platform="api_server", chat_id="sess-123") + created = json.loads( + cronjob(action="create", prompt="x", schedule="every 2m") + ) + assert created["success"] is True + assert created["deliver"] == "origin" + # Targeted message naming the undeliverable origin platform. + assert "api_server" in created["message"] + assert "cannot receive cron deliveries" in created["message"] + assert "deliver='telegram'" in created["message"] + # Not the generic CLI/TUI wording. + assert "local-only cron job" not in created["message"] + + def test_api_server_origin_with_home_channel_no_notice(self, monkeypatch): + """An api_server origin that falls back to a configured home channel + does deliver somewhere, so no notice is emitted.""" + from gateway.session_context import set_session_vars + + monkeypatch.setenv("TELEGRAM_HOME_CHANNEL", "-111") + set_session_vars(platform="api_server", chat_id="sess-123") + created = json.loads( + cronjob(action="create", prompt="x", schedule="every 2m") + ) + assert created["success"] is True + assert "cannot receive cron deliveries" not in created["message"] + assert "local-only cron job" not in created["message"] + class TestValidateCronBaseUrl: """The cron base_url guard must not let a NAMED custom provider's stored diff --git a/tools/cronjob_tools.py b/tools/cronjob_tools.py index a623d38c6f563..73229ffe19004 100644 --- a/tools/cronjob_tools.py +++ b/tools/cronjob_tools.py @@ -308,6 +308,23 @@ def _origin_from_env() -> Optional[Dict[str, str]]: return None +def _origin_platform_can_deliver(platform: str) -> bool: + """Whether a captured-origin platform can actually receive cron delivery. + + Thin wrapper over ``cron.scheduler._is_known_delivery_platform`` so the + create-time notice stays in sync with the fire-time target resolver. The + api_server platform is the motivating case (#69304): it is a real gateway + platform with a working session, but its adapter ``send()`` is a no-op, so + an ``origin.platform="api_server"`` job can never deliver. + """ + try: + from cron.scheduler import _is_known_delivery_platform + + return _is_known_delivery_platform(platform) + except Exception: + return True # don't second-guess on import failure; resolver will gate + + def _local_delivery_notice(job: Dict[str, Any], user_deliver: Optional[str]) -> Optional[str]: """Return an informational notice when a created job won't deliver anywhere. @@ -336,6 +353,21 @@ def _local_delivery_notice(job: Dict[str, Any], user_deliver: Optional[str]) -> # If resolution can't be evaluated, fall back to the origin signal. if job.get("origin"): return None + origin = job.get("origin") or {} + if origin.get("platform") and not _origin_platform_can_deliver(origin["platform"]): + # The job was created in a session whose platform cannot receive cron + # delivery — the api_server trap (#69304): its adapter's send() is a + # no-op, so deliver=origin would run the job but silently drop every + # report. Point the caller at a real delivery channel. + return ( + "This cron job's origin platform " + f"({origin['platform']}) cannot receive cron deliveries — its " + "output is saved (view it with cronjob(action='list')) but will " + "NOT be delivered back into that session. To be notified when it " + "runs, recreate or update the job with deliver set to a " + "gateway-connected platform, e.g. deliver='telegram' or " + "deliver='all'." + ) return ( "This is a local-only cron job: its output is saved (view it with " "cronjob(action='list')) but will NOT be delivered back into this "