mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-27 17:58:07 +00:00
fix(cron): clear inherited thread_id for in_channel delivery
cron_continuable_surface=in_channel is meant to deliver a cron brief FLAT into a channel (thread_id=None) so a plain channel reply continues the job via the shared-channel session. The scheduler already seeds that flat session (_seed_cron_channel_session) but never cleared the ORIGINAL target/origin thread_id before routing the actual delivery, so a job scheduled from inside a Slack thread still delivered into that origin thread — the seeded continuable session then never matched where the brief actually landed. Clear thread_id once in_channel_surface is finalized, before it is re-read for route_thread_id / the standalone fallback. Scope the clear to EXACTLY the target that gets flat-seeded — the origin-continuable target (mirror_this_target) on a live, in_channel-capable adapter (runtime_adapter is not None): - fan-out / broadcast / explicit-thread targets keep their thread_id — they are not continuable and are never seeded, so flattening them would reroute an explicit thread and could collapse two targets into duplicate flat sends; - the standalone no-live-adapter path keeps the origin thread — it can never seed the flat session, and without an adapter the D6 capability fail-safe can't run, so flattening there would both drop the brief out of any continuable lane and bypass D6. This keeps the clear in lockstep with the seed condition (in_channel_surface and mirror_this_target) and the D6 fail-safe. mirror_this_target / origin_user_id are computed earlier using the original thread_id to match the origin conversation. Regression tests in TestCronContinuableSurfaceInChannel: - a job whose origin carries a thread_id must not forward that thread_id to the live adapter (DeliveryRouter folds target.thread_id into send metadata), and the seeded continuable session stays flat (thread_id=None); - with no live adapter, the standalone send falls back to the origin thread rather than flattening.
This commit is contained in:
parent
cbc1054e23
commit
206bbfdbdc
2 changed files with 97 additions and 0 deletions
|
|
@ -1612,6 +1612,31 @@ def _deliver_result(job: dict, content: str, adapters=None, loop=None) -> Option
|
|||
)
|
||||
in_channel_surface = False
|
||||
|
||||
if in_channel_surface and mirror_this_target and runtime_adapter is not None:
|
||||
# Force flat delivery (D2): the continuable-channel target must
|
||||
# ignore any inherited origin/target thread_id, or the flat
|
||||
# continuable session seeded below (thread_id=None, via
|
||||
# _seed_cron_channel_session) never matches where the brief is
|
||||
# actually delivered — route_thread_id further down in this loop
|
||||
# reads `thread_id` and would otherwise route into the origin
|
||||
# thread instead of flat into the channel.
|
||||
#
|
||||
# Scoped to EXACTLY the target that gets flat-seeded: the origin-
|
||||
# continuable target (`mirror_this_target`) on a live, in_channel-
|
||||
# capable adapter. The `runtime_adapter is not None` guard keeps
|
||||
# this in lockstep with the D6 capability fail-safe (which only runs
|
||||
# with a live adapter) and with the seed itself
|
||||
# (`in_channel_surface and mirror_this_target`, inside the
|
||||
# live-adapter block below): fan-out / broadcast / explicit-thread
|
||||
# targets keep their thread_id (they are not continuable and are
|
||||
# never seeded), and the standalone no-adapter path falls back to
|
||||
# thread delivery — it can never seed the flat session, so
|
||||
# flattening there would drop the brief out of any continuable lane
|
||||
# while also bypassing the D6 capability check. Placed AFTER
|
||||
# mirror_this_target / origin_user_id are computed above — those
|
||||
# need the ORIGINAL thread_id to match the origin conversation.
|
||||
thread_id = None
|
||||
|
||||
# For an in_channel delivery the flat continuation session is created
|
||||
# explicitly below (the shipped mirror only APPENDS to an existing
|
||||
# session, and the flat channel row is otherwise absent for a
|
||||
|
|
|
|||
|
|
@ -4754,6 +4754,78 @@ class TestCronContinuableSurfaceInChannel:
|
|||
mirror_mock.assert_called_once()
|
||||
assert mirror_mock.call_args.kwargs.get("thread_id") is None
|
||||
|
||||
def test_in_channel_from_origin_thread_delivers_flat_not_to_thread(self):
|
||||
"""Regression: a job scheduled from INSIDE a Slack thread (origin carries
|
||||
a thread_id) must still deliver FLAT when cron_continuable_surface is
|
||||
in_channel — not into the origin thread. Without clearing the inherited
|
||||
thread_id, the live-adapter route (DeliveryRouter._deliver_to_platform)
|
||||
folds target.thread_id into send_metadata['thread_id'], so the brief
|
||||
would land in the origin thread while the seeded continuable session
|
||||
(thread_id=None, asserted above) never matches where it actually went."""
|
||||
adapter = self._slack_adapter(supports_inchannel=True)
|
||||
_, mirror_mock = self._run_inchannel_delivery(
|
||||
{"cron_continuable_surface": "in_channel"}, adapter,
|
||||
origin={
|
||||
"platform": "slack", "chat_id": "C123", "user_id": "U_HUMAN",
|
||||
"thread_id": "999.888",
|
||||
},
|
||||
)
|
||||
# The thread-open branch must still be skipped (in_channel behavior).
|
||||
adapter.send.assert_awaited_once()
|
||||
_, send_kwargs = adapter.send.await_args
|
||||
send_metadata = send_kwargs.get("metadata") or {}
|
||||
assert "thread_id" not in send_metadata, (
|
||||
"in_channel delivery must be flat — the origin's thread_id must "
|
||||
"not be forwarded to the adapter, even though the job was "
|
||||
"scheduled from inside that thread"
|
||||
)
|
||||
# The seeded continuable session must match where the brief actually
|
||||
# landed: flat (thread_id=None), not the origin thread.
|
||||
adapter._session_store.get_or_create_session.assert_called_once()
|
||||
seeded = adapter._session_store.get_or_create_session.call_args[0][0]
|
||||
assert seeded.thread_id is None
|
||||
mirror_mock.assert_called_once()
|
||||
assert mirror_mock.call_args.kwargs.get("thread_id") is None
|
||||
|
||||
def test_in_channel_standalone_no_adapter_preserves_origin_thread(self):
|
||||
"""Fail-safe (D6 bypass guard): with NO live adapter, an
|
||||
in_channel-configured job must NOT be flattened. The flat continuable
|
||||
session can only be seeded on the live-adapter path, and the D6
|
||||
capability check can't run without an adapter — so the standalone send
|
||||
must fall back to the origin thread rather than silently flattening
|
||||
(which would bypass D6 and drop the brief out of any continuable lane).
|
||||
Scoping the thread_id clear to `runtime_adapter is not None` keeps the
|
||||
clear in lockstep with the seed and the D6 fail-safe."""
|
||||
from gateway.config import Platform
|
||||
|
||||
pconfig = MagicMock()
|
||||
pconfig.enabled = True
|
||||
pconfig.extra = {"cron_continuable_surface": "in_channel"}
|
||||
mock_cfg = MagicMock()
|
||||
mock_cfg.platforms = {Platform.SLACK: pconfig}
|
||||
|
||||
with patch("gateway.config.load_gateway_config", return_value=mock_cfg), \
|
||||
patch("cron.scheduler.load_config", return_value={"cron": {"wrap_response": False}}), \
|
||||
patch("tools.send_message_tool._send_to_platform",
|
||||
new=AsyncMock(return_value={"success": True})) as send_mock:
|
||||
job = {
|
||||
"id": "brief-job",
|
||||
"name": "Daily Brief",
|
||||
"deliver": "origin",
|
||||
"origin": {
|
||||
"platform": "slack", "chat_id": "C123",
|
||||
"user_id": "U_HUMAN", "thread_id": "999.888",
|
||||
},
|
||||
}
|
||||
# No adapters/loop → standalone (no-live-adapter) delivery path.
|
||||
_deliver_result(job, "Here is today's brief.")
|
||||
|
||||
send_mock.assert_called_once()
|
||||
assert send_mock.call_args.kwargs.get("thread_id") == "999.888", (
|
||||
"standalone in_channel delivery must fall back to the origin thread "
|
||||
"(no live adapter can seed a flat continuable session), not flatten"
|
||||
)
|
||||
|
||||
def test_thread_mode_default_still_opens_thread(self):
|
||||
"""G1 regression: the default (thread) mode is byte-identical — the
|
||||
thread-open branch still fires when no surface key is set."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue