Repository navigation
fix(send_message): route bare in-turn target to current channel, not home - #71
Conversation
…home A send_message/react call with no explicit chat target silently fell back to the platform global home channel. An agent posting a mid-turn update from a non-home channel (e.g. a Discord project channel) leaked the message into the home channel — sometimes a different platform (Telegram DM) entirely. Root cause: _handle_send / _handle_react default an unspecified target to config.get_home_channel(platform). That is correct for cron/CLI/standalone, but inside a live messaging turn the destination should be the channel the turn is happening in. Fix: a single tri-state resolver (_resolve_send_target) gated on whether a real MESSAGING origin is bound for this turn (_has_messaging_origin: a non-empty session platform, excluding cron). Inside a messaging turn a bare target resolves to the current channel (HERMES_SESSION_* task-local contextvars) or returns an actionable error — it can never reach the home channel. Non-messaging contexts (cron/CLI/TUI/desktop/ACP, which set HERMES_GATEWAY_SESSION without binding a platform) keep the existing home default. Schema descriptions updated to match; thread id preserved. Repro (pre-fix): in a Discord turn on channel A with home=B, call send_message(target="discord", message="x") -> delivered to B. Post-fix: delivered to A. Tests: tests/tools/test_send_message_origin.py (9 cases: origin send + react, no-context home, TUI/gateway-session home, cron home, unresolvable error, explicit unchanged, contextvar read, thread fidelity). Scope: out of scope is delegate_task subagent bare sends (contextvars do not cross the executor boundary; they fall to home = todays behavior).
d80ec73 to
f9af6b3
Compare
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
unresolved-import |
1 |
First entries
tests/tools/test_send_message_origin.py:29: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/tools/test_send_message_origin.py:291: [unresolved-attribute] unresolved-attribute: Attribute `kwargs` is not defined on `None` in union `_Call | None`
tests/tools/test_send_message_origin.py:290: [unresolved-attribute] unresolved-attribute: Attribute `args` is not defined on `None` in union `_Call | None`
✅ Fixed issues: none
Unchanged: 5874 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
| if parsed and (parsed.get("platform") or "").strip().lower() == want: | ||
| chat_id = (parsed.get("chat_id") or "").strip() | ||
| if chat_id: | ||
| thread_id = (parsed.get("session_id") or "").strip() or None |
There was a problem hiding this comment.
Wrong key name in the session-key fallback path —
"session_id" does not exist in the dict returned by _parse_session_key. That function returns {"platform", "chat_type", "chat_id"} and optionally "thread_id" (only for dm/thread chat types). Using "session_id" always evaluates to None, so the fallback silently drops the thread ID every time it fires, causing messages to land at channel root instead of the correct thread.
| thread_id = (parsed.get("session_id") or "").strip() or None | |
| thread_id = (parsed.get("thread_id") or "").strip() or None |
| # --------------------------------------------------------------------------- # | ||
| # AC5 — explicit cross-channel target unchanged | ||
| # --------------------------------------------------------------------------- # | ||
| def test_explicit_target_unchanged(_clear_session): | ||
| config = _config_with_home() | ||
| # Even inside a discord turn, an explicit chat id is honored verbatim. | ||
| tokens = set_session_vars(platform="discord", chat_id=ORIGIN_CHAT) | ||
| try: | ||
| with patch("gateway.config.load_gateway_config", return_value=config), \ | ||
| patch("tools.interrupt.is_interrupted", return_value=False), \ | ||
| patch("model_tools._run_async", side_effect=_run_async_immediately), \ | ||
| patch("tools.send_message_tool._send_to_platform", | ||
| new=AsyncMock(return_value={"success": True})) as send_mock, \ | ||
| patch("gateway.mirror.mirror_to_session", return_value=True): | ||
| result = _send("discord:7777000011112222") | ||
| finally: | ||
| clear_session_vars(tokens) | ||
|
|
||
| assert result["success"] is True | ||
| assert send_mock.await_args.args[2] == "7777000011112222" | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- # | ||
| # AC7 — origin resolution reads task-local session-context (not a global) | ||
| # --------------------------------------------------------------------------- # | ||
| def test_origin_resolution_reads_session_context(_clear_session): | ||
| from tools.send_message_tool import _resolve_send_target, _SEND_TARGET_ORIGIN | ||
|
|
||
| tokens = set_session_vars(platform="discord", chat_id=ORIGIN_CHAT) | ||
| try: | ||
| state, chat_id, _thread = _resolve_send_target("discord") |
There was a problem hiding this comment.
AC7b covers the primary path, not the fallback
test_origin_preserves_thread_id sets both chat_id=ORIGIN_CHAT and thread_id=ORIGIN_THREAD via set_session_vars, so it always hits the primary branch of _interactive_origin (lines 196-200). The session_id / thread_id typo in the fallback branch (reached when the session vars have a platform but an empty chat_id, requiring resolution through _parse_session_key) is never exercised.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…me (v2) (#83) * fix(send_message): route subagent bare sends to parent origin, not home Follow-up to #71. After #71 fixed the main-turn bare-target home-channel leak, the leak recurred from delegate_task SUBAGENTS: a child runs in a bare ThreadPoolExecutor worker with empty contextvars, so a subagent calling send_message/react with a bare target saw no session origin and fell back to the global home channel (a #tdd progress update landed in #hermes). Fix: dedicated routing-only send-origin contextvars (_SEND_ORIGIN_*) in gateway/session_context.py, read ONLY by the send resolver. The delegate child-run wrapper (_run_with_thread_capture) binds them from the parent origin captured at spawn (_blackbox_parent_*) and clears them in a finally. This propagates via the worker-thread copy_context() into the child tool-call threads where send_message actually executes. Crucially does NOT reuse HERMES_SESSION_PLATFORM (which drives approval gating, skill-disable, TTS, prompt-cache) — the child keeps its subagent identity for all of those; only send routing sees the new vars. Also: grandchild chaining (orchestrator subagent spawning a grandchild) — the spawn-capture falls back to get_send_origin() so origin is transitive through nesting; and a NOT_IN_TURN-despite-session-key tripwire warning to catch any remaining/unknown leak path (instrument-first for the unproven main-turn case). Tests: tests/tools/test_subagent_send_origin.py (14 cases): parent-origin routing for send+react, resolution order (gateway turn wins), no-origin falls to home, concurrent-children isolation, nestable clear (grandchild), instrumentation warns/silent-on-cron/never-raises, and the security non-regression (send-origin does not flip approval context). Seam proven: with the resolver blind to send-origin, the same call returns NOT_IN_TURN (home leak). 165 passed in the changed-module suites (1 pre-existing heartbeat-timing flake unrelated). Spec: docs/specs/PRD-send-message-origin-leak-v2.md (2 Opus review passes; Pass-1 caught that the naive _SESSION_PLATFORM-reuse design would flip subagent approval gating; Pass-2 caught the grandchild chain gap). * docs(send_message): PRD v2 — subagent origin-leak fix (2 Opus passes) * fix(send_message): require platform AND chat in _has_messaging_origin Greptile P2: _has_messaging_origin checked only get_send_origin()[0] (platform) while _interactive_origin requires both platform and chat_id. A platform-only send-origin (e.g. set_send_origin("discord","")) would flip _has_messaging_origin True but _interactive_origin None -> IN_TURN_UNRESOLVABLE hard error instead of falling closed to home. Require both, matching the resolver contract. Regression test added. --------- Co-authored-by: Apollo <apollo@daemonarchy.local>
…y upstream parity merge The 2026-06-29 upstream parity merge (PR #119) took upstream's toolsets.py structure, which silently dropped two fork-only toolsets: * messaging -> send_message (agent-callable; the fork's origin-routing work #71/#83 is built on it). Upstream deliberately omits an agent-callable send_message; this fork re-enables it. * moa -> mixture_of_agents (tools/mixture_of_agents_tool.py, fully deleted). Post-merge resolve_toolset('messaging')/('moa') returned [] with no error, so on deploy Apollo would silently lose both tools across cli/telegram/discord. Caught by the config-migration dry-run's post-migration toolset validation. Restores: - toolsets.py: messaging + moa entries (marked FORK-CUSTOM so a future merge preserves them) - tools/mixture_of_agents_tool.py (self-registers moa via registry.register) - agent/display.py: mixture_of_agents formatter + arg-key - model_tools.py: moa_tools legacy alias - acp_adapter/tools.py: mixture_of_agents in polished ACP tools - hermes_cli/tools_config.py: messaging+moa UI rows, moa default-off + env req - hermes_cli/config.py: OPENROUTER_API_KEY tools list (+ mixture_of_agents) - hermes_cli/tips.py: mixture_of_agents tip Adds tests/agent/test_fork_custom_toolsets.py — a HARD regression gate that fails CI if either fork toolset is dropped again (proven: 6/6 pass restored, 6/6 fail on the broken pre-restore tree). Hardened against the editable-install live-tree leak so a deleted module can't false-pass via a sibling install.
Only psutil.NoSuchProcess means gone/no-op; AccessDenied et al. now refuse the signal in all three psutil probes. Red on base, 34/34 guard tests green.
…dren (C5 #71 CI follow-up) CI slice 9/16 red: test_process_probe_failure_never_authorizes_reclaim[psutil-missing] stubs sys.modules['psutil']=None, so psutil.Process raises AttributeError on the test's own spawned child and the guard refused its cleanup kill. For pids on the spawn/initial-children record only AccessDenied fails closed; broken probe machinery trusts the record. Parent-chain branch still fails closed on anything. Red locally before, 45/45 (stall + guard self-test) after.
Only psutil.NoSuchProcess means gone/no-op; AccessDenied et al. now refuse the signal in all three psutil probes. Red on base, 34/34 guard tests green.
…dren (C5 #71 CI follow-up) CI slice 9/16 red: test_process_probe_failure_never_authorizes_reclaim[psutil-missing] stubs sys.modules['psutil']=None, so psutil.Process raises AttributeError on the test's own spawned child and the guard refused its cleanup kill. For pids on the spawn/initial-children record only AccessDenied fails closed; broken probe machinery trusts the record. Parent-chain branch still fails closed on anything. Red locally before, 45/45 (stall + guard self-test) after.
Problem
A
send_message/reactcall with no explicit chat target silently fell back to the platform's global home channel. When an agent posted a mid-turn update from a non-home channel (e.g. a Discord project channel), the message leaked into the home channel — sometimes a different platform (Telegram DM) entirely.Real incident (2026-06-21): a long agent turn in Discord
#instacartcalledsend_messagewith a bare target; the progress checkpoint surfaced in the Discord home channel#hermesinstead.agent.logshows the exact two-step signature —send_message error "Both 'target' and 'message' are required"thensend_message completed (200 chars)— and the leaked text landed in the home-channel session at the same millisecond.Root cause
_handle_send(and the identical sibling_handle_react) default an unspecifiedchat_idtoconfig.get_home_channel(platform). That is correct for cron/CLI/standalone, but inside a live messaging turn the destination should be the channel the turn is happening in.targetis optional in the schema ("required": []) and the description literally said "'platform' (uses home channel)", so a bare target was a documented path straight to the leak.Fix
A single tri-state resolver,
_resolve_send_target(platform_name):HERMES_SESSION_*contextvars the gateway binds per turn).The home-channel lookup is reachable only on the
NOT_IN_TURNbranch, so a live messaging turn can never silently hit home (structural, not emergent)._has_messaging_origin()gates on an actually-bound messaging platform (approval._get_session_platform()truthiness, minus cron) — deliberately narrower than_is_gateway_approval_context(), because the TUI/desktop/ACP gateways setHERMES_GATEWAY_SESSIONwithout binding a messaging platform; using the broad helper would turn their legitimate bare→home sends into errors.Both
_handle_sendand_handle_reactroute through the shared resolver (whole bug class, not just the reported site). Schema descriptions (top-level +targetproperty) updated to match; origin thread id is preserved; alogger.inforecords origin-routed sends.Reproduction
Tests
tests/tools/test_send_message_origin.py— 9 cases, real_handle_send/_handle_reactpath, only the network send + config stubbed:HERMES_GATEWAY_SESSIONset, no platform bound (TUI/desktop/ACP) → home, not an errorHERMES_CRON_SESSION) → homeAll 9 pass; ruff clean; no regression in the existing
send_messagesuites.Scope / non-goals
delegate_tasksubagent bare sends are out of scope: contextvars don't cross the executor boundary, so a subagent has no bound messaging origin → falls to home (today's behavior, never worse). The reported leak was a main-session send.Spec:
docs/specs/PRD-send-message-origin-leak.md(2 Opus review passes; Pass-2 caught and fixed the TUI/desktop over-broad-gate blocker).