Skip to content

fix(bot-mode): deliver local DMs to Desktop-owned Bot Chats - #102746

Closed
helix4u wants to merge 3 commits into
NousResearch:mainfrom
helix4u:fix/bot-mode-live-local-dm
Closed

helix4u wants to merge 3 commits into
NousResearch:mainfrom
helix4u:fix/bot-mode-live-local-dm

Conversation

@helix4u

@helix4u helix4u commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Local message_agent deliveries normally open the target profile's canonical Bot Chat through a one-shot CLI process. When Desktop already owns that session, the single-owner lease correctly rejects the subprocess with already has a live owner, but the message is not delivered.

This keeps the existing CLI delivery as the first choice. Only after that exact refusal does the runner move the same message through Hermes' existing Desktop Bot relay and prompt.submit path.

This is a narrower alternative to #101564. It reuses the relay already shipped for cross-connection Bot Mode delivery instead of adding another filesystem mailbox and Desktop/TUI polling subsystem.

Related Issue

Fixes #101060

Related: #101293, #101564

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Preserve the direct local CLI delivery path as the first resolver rung.
  • Advertise Desktop's fresh local connection ID through the existing Bot relay roster.
  • After an exact live-owner refusal, enqueue the original message through that live Desktop connection and wait for the standard relay result.
  • Allow a single Desktop connection to drain and deliver its own local relay envelopes while retaining the existing cross-connection retention behavior.
  • Keep relay draining sequential so message order and RPC worker capacity remain bounded.
  • Add focused Python and Desktop tests for direct-first routing, local fallback metadata, single-connection roster/drain behavior, same-connection delivery, and reply handling.

How to Test

  1. Open the receiving bot's canonical Bot Chat in Desktop so Desktop owns its session lease.
  2. From another local bot profile, call message_agent for the receiving bot.
  3. Confirm the normal CLI attempt receives the live-owner refusal, the message appears in the already-open Desktop Bot Chat through the existing relay, and no duplicate local turn is created.

Focused automated verification performed:

  • npm test -- src/plugins/hermes-bots/relay.test.ts (22 passed)
  • npm run typecheck (passed)
  • scripts/run_tests.sh tests/tools/test_bot_mode_dm.py tests/tools/test_bot_relay.py tests/tui_gateway/test_bot_relay_methods.py -q -j 4
  • Directly reran the newly affected bot_mode_dm and gateway roster tests with the installed Hermes environment; all changed-path assertions passed.
  • python -m py_compile for all changed Python source and test files (passed)
  • git diff --check origin/main (passed)
  • python scripts/check-windows-footguns.py --diff origin/main reported only four pre-existing test-file findings already present on origin/main.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A. The regression is covered by focused relay and delivery-path tests.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 4, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: #101564 is an open competing fix for the same #101060 symptom using a filesystem mailbox + polling; this PR reuses the existing Desktop Bot relay instead. Different mechanisms — maintainer should pick one.

@helix4u
helix4u force-pushed the fix/bot-mode-live-local-dm branch from 2ef523c to 3c1277e Compare September 4, 2026 07:07
@helix4u

helix4u commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

CI note: the previous run failed only in tests/tui_gateway/test_subprocess_encoding.py::test_slash_worker_popen_uses_utf8_replace.

This was unrelated to the Bot Mode changes. The exact PR merge tree already passed encoding="utf-8" and errors="replace" to the slash-worker subprocess.Popen call. The test globally mocks subprocess.Popen and checks only mock_popen.call_args, so an unrelated subprocess call can become the last recorded call and make the assertion fail. The same Python suite passed on the previous PR head.

GitHub would not allow me to rerun the failed job without upstream repository admin rights, so I pushed the empty commit 546aade221 to trigger a fresh run without changing the source tree.

The replacement run passed, including contributor attribution, the full Python suite, Windows-only tests, and the final required-check aggregate.

@teknium1

teknium1 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks @helix4u for the same-install Desktop relay alternative, especially the fresh-route guard and keeping ordinary local CLI delivery available. #105442 is now merged at 5280fe99872a58f1b9d25b00a79d240ecd035f7d, building on @FalconOrtiz’s #101564 owner-mailbox proposal. It handles the Desktop-owned local-DM case before spawning a competing CLI, using durable cross-process ingress and the existing owner’s normal queued turn admission. This avoids needing a local Desktop connection ID, self-relay renderer loop, or a second relay waiter for this case.

Closing this alternative as superseded; your freshness and fallback analysis helped make the delivery boundary concrete. The exact relay implementation was not copied. Unowned targets retain their CLI lane, and CLI-only/non-advertising owners are not claimed as supported by this change. Restart older running backends after upgrading.

@teknium1 teknium1 closed this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) comp/tools Tool registry, model_tools, toolsets comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

3 participants