From 2e7d829a2ea8143dc6dc6dfc234ccb5fac110ab7 Mon Sep 17 00:00:00 2001 From: Victor Kyriazakos Date: Tue, 18 Aug 2026 16:58:45 +0000 Subject: [PATCH] fix(cron): mirror continuable-cron briefs for origin-fallback and opted-in explicit targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A managed cron (created by a provisioning script, not from a live gateway chat) never captures an origin. With cron.mirror_delivery: true and deliver: origin, its brief was delivered to the home channel — the user's own DM — but the transcript mirror and the in_channel session seed were silently skipped: _target_matches_origin returns False for an empty origin, and the whole continuable machinery keys off that check. A user replying to the brief landed in a session with no record of it. Field report 2026-08-17 (enterprise, Slack DM surface). The June origin-scoping refactor (c06ceb3232) was written to exclude broadcasts, and the exclusion is kept. What changes is the classification: a home-channel FALLBACK for deliver=origin is the user's primary conversation standing in for the origin, not a broadcast. Changes: - Delivery targets carry a resolution-provenance tag (_resolved_from: origin / origin_fallback / explicit; broadcast expansions untagged). - _target_mirror_eligible replaces the bare origin check at the mirror gate: origin unchanged; origin_fallback eligible under the same flags as origin (per-job attach_to_session wins, else global cron.mirror_delivery); explicit platform:chat targets eligible ONLY under per-job attach_to_session — the global flag never activates them, so it cannot start writing transcript entries into arbitrary explicitly-addressed chats. 'all'/bare-platform stay never-eligible. - Dedup OR-merges provenance so 'origin,all' resolving to the same chat keeps eligibility regardless of token order. - _inchannel_seed_allowed guards the flat-session seed: group-channel session keys are user-isolated, so a seed without a user_id (origin- less job into a shared channel) would create an orphan session no reply resolves to — those targets fall back to the plain mirror. DM targets (keys don't embed user_id) always seed. - cronjob tool schema text updated to describe the new attach scope. Behavioral note: origin-less deliver=origin jobs under global mirror_delivery now activate the full continuable path — on default 'thread' surface this opens a dedicated thread in the home channel where the brief previously posted flat. That is the documented continuable behavior; the silent flat post was the bug. 15 new tests (tests/cron/test_mirror_origin_fallback.py): eligibility matrix (origin/fallback/explicit/all/bare/other-chat), dedup order both ways, end-to-end mirror via _deliver_result for all four shapes, origin regression control, seed user_id guard. --- cron/scheduler.py | 119 ++++++++- tests/cron/test_mirror_origin_fallback.py | 240 ++++++++++++++++++ tests/cron/test_scheduler.py | 5 + .../test_send_message_plugin_extensibility.py | 1 + tools/cronjob_tools.py | 2 +- 5 files changed, 358 insertions(+), 9 deletions(-) create mode 100644 tests/cron/test_mirror_origin_fallback.py diff --git a/cron/scheduler.py b/cron/scheduler.py index 9c260d112fb9a..8958d4f8a3f2d 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -1582,6 +1582,75 @@ def _target_matches_origin(origin: dict, platform_name: str, chat_id: str, return True +# Resolution-provenance ranking for the dedup OR-merge in +# _resolve_delivery_targets: higher rank = stronger mirror claim. Broadcast +# expansions rank 0 so "origin,all"/"all,origin" hitting the same chat keeps +# the origin(-fallback) tag regardless of token order. +_MIRROR_PROVENANCE_RANK = { + "origin": 3, + "origin_fallback": 2, + "explicit": 1, +} + + +def _target_mirror_eligible(job: dict, target: dict, *, global_mirror: bool) -> bool: + """Whether a resolved delivery target may receive the transcript mirror. + + The June origin-scoping refactor gated mirroring on target == origin, + which correctly excluded broadcasts but also silenced two legitimate + conversation shapes — both hit by script-provisioned ("managed") crons, + which never capture an origin (``_origin_from_env`` only fires for jobs + created from a live gateway chat): + + - ``origin_fallback``: ``deliver=origin`` with no captured origin resolves + to the home channel — the user's primary conversation standing in for + the origin, not a broadcast. Eligible under the same flags as a true + origin target. (Field report 2026-08-17: brief delivered to the Slack + DM, mirror silently skipped, reply hit a context-less session.) + - ``explicit``: a ``platform:chat_id`` target is eligible ONLY when the + job itself opts in via ``attach_to_session: true`` — the job author + declaring this target a conversation (managed per-user DM briefings). + The global ``cron.mirror_delivery`` flag never activates explicit + targets: it must not start writing transcript entries into arbitrary + explicitly-addressed chats (shared channels, other users' DMs). + + Broadcast expansions (``all``, bare-platform home targets) carry no + provenance tag and are never eligible — unchanged invariant. + """ + origin = _resolve_origin(job) or {} + if _target_matches_origin( + origin, target.get("platform", ""), target.get("chat_id", ""), + target.get("thread_id"), + ): + return True + resolved_from = target.get("_resolved_from") + if resolved_from == "origin_fallback": + # Same activation rules as an origin target: per-job attach wins, + # else the global flag. + per_job = job.get("attach_to_session") + if isinstance(per_job, bool): + return per_job + return bool(global_mirror) + if resolved_from == "explicit": + return job.get("attach_to_session") is True + return False + + +def _inchannel_seed_allowed(*, is_dm: bool, user_id: Optional[str]) -> bool: + """Whether the flat in_channel session seed may run for a target. + + Group-channel session keys are user-isolated + (``…:group::`` — see _seed_cron_channel_session); a + seed without a real user_id would create an orphan session that no + inbound reply ever resolves to, which is worse than no seed (the plain + mirror can still land if a session exists). DM keys don't embed + user_id, so DM targets are always seedable. Origin-captured jobs carry + the scheduler's user_id; origin-less managed jobs typically don't, and + their group-channel targets must fall back to the plain mirror. + """ + return bool(is_dm or user_id) + + def _maybe_mirror_cron_delivery( job: dict, platform_name: str, @@ -2159,6 +2228,9 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d "platform": origin["platform"], "chat_id": str(origin["chat_id"]), "thread_id": _origin_delivery_thread(origin), + # Resolution provenance for mirror eligibility (see + # _target_mirror_eligible): this IS the origin conversation. + "_resolved_from": "origin", } # Origin missing (e.g. job created via API/script) — try each # platform's home channel as a fallback instead of silently dropping. @@ -2174,6 +2246,11 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d "platform": platform_name, "chat_id": chat_id, "thread_id": _get_home_target_thread_id(platform_name), + # The fallback stands in for the user's primary + # conversation (NOT a broadcast) — mirror-eligible so + # continuable crons work for script-provisioned jobs + # that never captured an origin. + "_resolved_from": "origin_fallback", } return None @@ -2218,6 +2295,9 @@ def _resolve_single_delivery_target(job: dict, deliver_value: str) -> Optional[d "platform": platform_name, "chat_id": chat_id, "thread_id": thread_id, + # Explicit platform:chat target — mirror-eligible only under the + # job's own attach_to_session opt-in (see _target_mirror_eligible). + "_resolved_from": "explicit", } platform_name = deliver_value @@ -2314,15 +2394,24 @@ def _resolve_delivery_targets(job: dict) -> List[dict]: for raw in raw_parts: parts.extend(_expand_routing_tokens(raw)) - seen = set() + seen = {} targets = [] for part in parts: target = _resolve_single_delivery_target(job, part) if target: key = (target["platform"].lower(), str(target["chat_id"]), target.get("thread_id")) if key not in seen: - seen.add(key) + seen[key] = target targets.append(target) + else: + # OR-merge resolution provenance on dedup: "origin,all" (either + # order) resolving to the same chat must keep the + # origin/origin_fallback tag — a mirror-eligible token must not + # lose eligibility to token order (see _target_mirror_eligible). + kept = seen[key] + if _MIRROR_PROVENANCE_RANK.get(str(target.get("_resolved_from") or ""), 0) > \ + _MIRROR_PROVENANCE_RANK.get(str(kept.get("_resolved_from") or ""), 0): + kept["_resolved_from"] = target.get("_resolved_from") return targets @@ -2640,11 +2729,13 @@ def _deliver_result(job: dict, content: str, adapters=None, loop=None) -> Option job["id"], platform_name, chat_id, thread_id, ) - # Mirror is scoped to the ORIGIN conversation only. A fan-out / broadcast - # / home-channel-fallback target is never mirrored (it is not the - # conversation the job was created in, and may have no session at all). - mirror_this_target = mirror_enabled and _target_matches_origin( - origin, platform_name, chat_id, thread_id + # Mirror scope: the origin conversation, the home-channel FALLBACK for + # an origin-less deliver=origin job (a script-provisioned managed cron + # standing in for the user's primary conversation — not a broadcast), + # or an explicit target the job opted into via attach_to_session. + # Broadcast/fan-out targets are never mirrored (_target_mirror_eligible). + mirror_this_target = mirror_enabled and _target_mirror_eligible( + job, target, global_mirror=mirror_enabled, ) # Pass the origin's user_id so a per-user-isolated group chat resolves to # the exact member who scheduled the job — parity with send_message. @@ -3075,7 +3166,19 @@ def _deliver_result(job: dict, content: str, adapters=None, loop=None) -> Option # session (the shipped mirror only appends to an existing # session — the flat row is otherwise absent for a # chat_postMessage delivery, so the brief would be lost). - if in_channel_surface and mirror_this_target and not thread_seeded: + # Guarded by _inchannel_seed_allowed: group-channel keys + # are user-isolated, so a seed without a user_id (origin- + # less managed cron into a shared channel) would create an + # orphan session no reply resolves to — fall back to the + # plain mirror instead. + if ( + in_channel_surface + and mirror_this_target + and not thread_seeded + and _inchannel_seed_allowed( + is_dm=is_dm_target, user_id=origin_user_id, + ) + ): inchannel_seeded = _seed_cron_channel_session( job, runtime_adapter, platform_name, chat_id, mirror_text, is_dm=is_dm_target, diff --git a/tests/cron/test_mirror_origin_fallback.py b/tests/cron/test_mirror_origin_fallback.py new file mode 100644 index 0000000000000..ac2b678e545af --- /dev/null +++ b/tests/cron/test_mirror_origin_fallback.py @@ -0,0 +1,240 @@ +"""Mirror eligibility for origin-fallback and explicit cron delivery targets. + +Field report (enterprise, 2026-08-17): `cron.mirror_delivery: true` with +`deliver: origin` delivered the brief to Slack but never appended it to the +reply-facing gateway session, so a user reply hit a session with no context. + +Root cause: the job was created by a provisioning script, so it carries no +captured origin. `deliver: origin` falls back to the home channel — the +user's actual conversation — but `_target_matches_origin` returns False for +an empty origin, so the mirror and the in_channel seed never fire. The June +origin-scoping refactor (c06ceb3232) correctly excluded broadcasts, but the +origin-FALLBACK target is not a broadcast: it is the best available stand-in +for the user's primary conversation. + +Design under test: +- Delivery targets carry a `mirror_eligibility` tag set at resolution time: + * origin match -> eligible (unchanged) + * origin-fallback (deliver=origin, no origin) -> eligible (NEW) + * explicit platform:chat -> eligible ONLY with per-job attach_to_session + (NEW, opt-in; the global flag never activates explicit targets) + * `all` / bare-platform expansion -> never eligible (unchanged invariant) +- Dedup across tokens (e.g. "origin,all" hitting the same chat) OR-merges + eligibility so token order cannot strip it. +- The in_channel flat-session seed requires a DM-shaped target or a known + user_id: group-channel session keys are user-isolated, and a seed without + user_id would create an orphan session no reply ever resolves to. +""" + +import pytest + +from cron.scheduler import ( + _deliver_result, + _resolve_delivery_targets, + _target_mirror_eligible, +) + + +@pytest.fixture(autouse=True) +def _home_channel(monkeypatch): + monkeypatch.setenv("SLACK_HOME_CHANNEL", "D0HOME") + monkeypatch.delenv("TELEGRAM_HOME_CHANNEL", raising=False) + monkeypatch.delenv("DISCORD_HOME_CHANNEL", raising=False) + + +class TestMirrorEligibilityResolution: + def test_origin_target_is_eligible(self): + job = { + "deliver": "origin", + "origin": {"platform": "slack", "chat_id": "D0AAA", "chat_type": "dm"}, + } + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert _target_mirror_eligible(job, targets[0], global_mirror=True) + + def test_origin_fallback_target_is_eligible(self): + """deliver=origin with no captured origin: the home-channel fallback + is the user's conversation, not a broadcast — mirror it.""" + job = {"deliver": "origin", "origin": None} + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert targets[0]["chat_id"] == "D0HOME" + assert _target_mirror_eligible(job, targets[0], global_mirror=True) + + def test_all_expansion_is_never_eligible(self): + """Broadcast targets stay unmirrored even with the global flag on.""" + job = {"deliver": "all", "origin": None} + targets = _resolve_delivery_targets(job) + assert targets, "home channel should expand from 'all'" + for t in targets: + assert not _target_mirror_eligible(job, t, global_mirror=True) + + def test_bare_platform_target_is_not_eligible(self): + job = {"deliver": "slack", "origin": None} + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert not _target_mirror_eligible(job, targets[0], global_mirror=True) + + def test_explicit_target_not_eligible_under_global_flag(self): + """Global mirror_delivery must not write sessions into arbitrary + explicitly-addressed chats.""" + job = {"deliver": "slack:D0EXPL", "origin": None} + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert not _target_mirror_eligible(job, targets[0], global_mirror=True) + + def test_explicit_target_eligible_with_per_job_attach(self): + """attach_to_session=true on the job is the author declaring the + explicit target a conversation — managed per-user DM crons.""" + job = { + "deliver": "slack:D0EXPL", + "origin": None, + "attach_to_session": True, + } + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert _target_mirror_eligible(job, targets[0], global_mirror=False) + + def test_origin_and_all_dedup_keeps_eligibility(self): + """'origin,all' resolving to the same home chat must not lose the + fallback's eligibility to dedup order.""" + job = {"deliver": "origin,all", "origin": None} + targets = _resolve_delivery_targets(job) + # Home channel deduped to one target. + slack_targets = [t for t in targets if t["platform"].lower() == "slack"] + assert len(slack_targets) == 1 + assert _target_mirror_eligible(job, slack_targets[0], global_mirror=True) + + def test_all_and_origin_reversed_order_keeps_eligibility(self): + job = {"deliver": "all,origin", "origin": None} + targets = _resolve_delivery_targets(job) + slack_targets = [t for t in targets if t["platform"].lower() == "slack"] + assert len(slack_targets) == 1 + assert _target_mirror_eligible(job, slack_targets[0], global_mirror=True) + + def test_explicit_other_chat_with_origin_not_eligible(self): + """An explicit target that is NOT the origin stays unmirrored under + the global flag even when the job has an origin elsewhere.""" + job = { + "deliver": "slack:D0OTHER", + "origin": {"platform": "slack", "chat_id": "D0AAA", "chat_type": "dm"}, + } + targets = _resolve_delivery_targets(job) + assert len(targets) == 1 + assert not _target_mirror_eligible(job, targets[0], global_mirror=True) + + +class TestFallbackMirrorEndToEnd: + """Drive _deliver_result with a stubbed sender + mirror recorder.""" + + @pytest.fixture() + def slack_env(self, monkeypatch, tmp_path): + home = tmp_path / "hermes-home" + home.mkdir() + (home / "config.yaml").write_text( + "cron:\n mirror_delivery: true\n" + "platforms:\n slack:\n enabled: true\n token: xoxb-test\n" + ) + monkeypatch.setenv("HERMES_HOME", str(home)) + + send_calls = [] + + async def fake_sender(pconfig, chat_id, message, *, thread_id=None, + media_files=None, force_document=False, caption=None): + send_calls.append({"chat_id": chat_id, "thread_id": thread_id}) + return {"success": True, "chat_id": chat_id, "message_id": "1.2"} + + import gateway.platform_registry as reg + import hermes_cli.plugins as hp + + entry = reg.platform_registry.get("slack") + if entry is None: + hp.discover_plugins() + entry = reg.platform_registry.get("slack") + if entry is None: + pytest.skip("slack platform entry not registered") + monkeypatch.setattr(entry, "standalone_sender_fn", fake_sender) + monkeypatch.setattr(hp, "discover_plugins", lambda *a, **k: None) + + mirror_calls = [] + + import cron.scheduler as sched + + def fake_mirror(platform, chat_id, text, source_label="cli", + thread_id=None, user_id=None, role="assistant"): + mirror_calls.append({ + "platform": platform, "chat_id": chat_id, + "thread_id": thread_id, "user_id": user_id, "role": role, + }) + return True + + import gateway.mirror as mirror_mod + + monkeypatch.setattr(mirror_mod, "mirror_to_session", fake_mirror) + return {"send": send_calls, "mirror": mirror_calls} + + def test_origin_fallback_job_mirrors_brief(self, slack_env): + """The field repro: managed cron, deliver=origin, no origin captured. + The brief must be mirrored into the home-channel session.""" + job = {"id": "j1", "name": "brief", "deliver": "origin", "origin": None} + err = _deliver_result(job, "Risk-off close brief", adapters=None, loop=None) + assert err is None + assert len(slack_env["send"]) == 1 + assert len(slack_env["mirror"]) == 1, ( + "origin-fallback delivery must mirror the brief into the " + "home-channel session (the reply-continuity bug)" + ) + assert slack_env["mirror"][0]["chat_id"] == "D0HOME" + assert slack_env["mirror"][0]["role"] == "user" + + def test_all_broadcast_does_not_mirror(self, slack_env): + job = {"id": "j2", "name": "cast", "deliver": "all", "origin": None} + err = _deliver_result(job, "broadcast text", adapters=None, loop=None) + assert err is None + assert len(slack_env["send"]) == 1 + assert len(slack_env["mirror"]) == 0 + + def test_explicit_target_with_attach_mirrors(self, slack_env): + job = { + "id": "j3", "name": "managed-dm", "deliver": "slack:D0USER7", + "origin": None, "attach_to_session": True, + } + err = _deliver_result(job, "managed brief", adapters=None, loop=None) + assert err is None + assert len(slack_env["mirror"]) == 1 + assert slack_env["mirror"][0]["chat_id"] == "D0USER7" + + def test_explicit_target_without_attach_does_not_mirror(self, slack_env): + job = { + "id": "j4", "name": "plain-explicit", "deliver": "slack:D0USER8", + "origin": None, + } + err = _deliver_result(job, "plain text", adapters=None, loop=None) + assert err is None + assert len(slack_env["mirror"]) == 0 + + def test_origin_job_still_mirrors_unchanged(self, slack_env): + """Regression control: the June origin-scoped behavior is untouched.""" + job = { + "id": "j5", "name": "origin-job", "deliver": "origin", + "origin": {"platform": "slack", "chat_id": "D0AAA", "chat_type": "dm"}, + } + err = _deliver_result(job, "origin brief", adapters=None, loop=None) + assert err is None + assert len(slack_env["mirror"]) == 1 + assert slack_env["mirror"][0]["chat_id"] == "D0AAA" + + +class TestInChannelSeedUserIdGuard: + """Group-channel seeds are user-keyed; a seed with no user_id would create + an orphan session. DM targets are safe (key has no user_id).""" + + def test_seed_requires_dm_or_user_id(self): + from cron.scheduler import _inchannel_seed_allowed + + # DM-shaped chat, no user_id: allowed (DM keys don't embed user). + assert _inchannel_seed_allowed(is_dm=True, user_id=None) + # Group chat with known user: allowed. + assert _inchannel_seed_allowed(is_dm=False, user_id="U123") + # Group chat, no user: refused — would orphan the session. + assert not _inchannel_seed_allowed(is_dm=False, user_id=None) diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index a1b8cd64e5a64..807337468bcc2 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -177,6 +177,7 @@ def test_origin_delivery_preserves_thread_id(self): "platform": "telegram", "chat_id": "-1001", "thread_id": "17585", + "_resolved_from": "origin", } @@ -221,6 +222,7 @@ def test_explicit_telegram_topic_target_overrides_cron_thread_id(self, monkeypat "platform": "telegram", "chat_id": "-1003724596514", "thread_id": "17", + "_resolved_from": "explicit", } @@ -237,6 +239,7 @@ def test_human_friendly_label_resolved_via_channel_directory(self): "platform": "whatsapp", "chat_id": "12345678901234@lid", "thread_id": None, + "_resolved_from": "explicit", } @@ -252,6 +255,7 @@ def test_raw_id_not_mangled_when_directory_returns_none(self): "platform": "whatsapp", "chat_id": "12345@lid", "thread_id": None, + "_resolved_from": "explicit", } def test_unresolved_target_still_delivered_as_written(self): @@ -270,6 +274,7 @@ def test_unresolved_target_still_delivered_as_written(self): "platform": "telegram", "chat_id": "ops-room", "thread_id": None, + "_resolved_from": "explicit", } diff --git a/tests/tools/test_send_message_plugin_extensibility.py b/tests/tools/test_send_message_plugin_extensibility.py index e1acf11636d4f..b7f318279db7a 100644 --- a/tests/tools/test_send_message_plugin_extensibility.py +++ b/tests/tools/test_send_message_plugin_extensibility.py @@ -184,6 +184,7 @@ def test_cli_and_cron_share_plugin_target_normalization(plugin_platform, monkeyp "platform": name, "chat_id": "@alice@example.com", "thread_id": None, + "_resolved_from": "explicit", } diff --git a/tools/cronjob_tools.py b/tools/cronjob_tools.py index 502dc4a74e9d6..abd974cf6f937 100644 --- a/tools/cronjob_tools.py +++ b/tools/cronjob_tools.py @@ -1735,7 +1735,7 @@ def cronjob( }, "attach_to_session": { "type": "boolean", - "description": "When True, this job becomes CONTINUABLE: the user can reply to its delivery and the agent has the brief in context instead of asking 'what is that?'. On thread-capable platforms (Telegram topics, Discord/Slack threads) a dedicated thread is opened for the job and its replies; on DM-only platforms (WhatsApp/Signal) the brief is mirrored into the origin DM session. Use this for conversational recurring jobs the user will reply to — daily briefings, reminders that kick off follow-up work. Leave unset for fire-and-forget alerts/watchdogs. Overrides the global cron.mirror_delivery config for this one job. Only the origin chat is touched (never fan-out targets); no effect when deliver='local'." + "description": "When True, this job becomes CONTINUABLE: the user can reply to its delivery and the agent has the brief in context instead of asking 'what is that?'. On thread-capable platforms (Telegram topics, Discord/Slack threads) a dedicated thread is opened for the job and its replies; on DM-only platforms (WhatsApp/Signal) the brief is mirrored into the origin DM session. Use this for conversational recurring jobs the user will reply to — daily briefings, reminders that kick off follow-up work. Leave unset for fire-and-forget alerts/watchdogs. Overrides the global cron.mirror_delivery config for this one job. Only the job's own conversation is touched: the origin chat, the home-channel fallback when deliver='origin' captured no origin (script-created jobs), or the job's single explicit platform:chat target. Broadcast targets (deliver='all') are never attached; no effect when deliver='local'." }, }, "required": ["action"]