Skip to content

fix(bot-mode): deliver local DMs into Desktop-owned Bot Chat (#101060) - #101564

Closed
FalconOrtiz wants to merge 2 commits into
NousResearch:mainfrom
FalconOrtiz:fix/fe-101060-message-agent-bot-chat-lock
Closed

FalconOrtiz wants to merge 2 commits into
NousResearch:mainfrom
FalconOrtiz:fix/fe-101060-message-agent-bot-chat-lock

Conversation

@FalconOrtiz

Copy link
Copy Markdown

Why

message_agent between local profiles still used a one-shot CLI turn into the canonical Bot Chat. When Desktop already held that session lease, the runner exited 1 after the tool had already returned status: sent, so the DM never appeared and the sender treated dispatch as success. #101293 fixed the Desktop-relay path via in-process prompt.submit; the local CLI path stayed broken (reconfirmed on #101060).

Scope

  • tools/bot_live_delivery.py: durable pending/claim/receipt handoff under the target profile runtime dir.
  • tools/bot_mode_dm.py _run_delivery: when a Desktop/TUI owner holds Bot Chat, deliver through that handoff instead of the fenced CLI; keep target_busy when no owner claims in time.
  • tui_gateway/server.py: idle notification poller claims one pending DM and runs it through _run_prompt_submit with a terminal receipt; new leases mark bot_live_delivery_consumer.
  • Focused tests for find/claim/receipt, poller intake, and the _run_delivery handoff path.

Out of scope: changing the async sent ack shape, peer/A2A transports, cron deliver=bot-chat (see #100319).

Tradeoffs

Reuses the #100544 live-owner handoff idea in a smaller form rather than inventing a second Desktop-only RPC. Accepts Desktop/TUI surface owners without requiring a prior consumer flag so already-open Bot Chats work after upgrade. CLI owners stay on the existing subprocess path.

Blast Radius

Local Bot Mode teammate DMs when the target Bot Chat is open in Desktop/TUI. Relay deliveries already covered by #101293 stay unchanged. Idle poller adds a cheap filesystem probe per session tick.

Verification

python -m pytest tests/tools/test_bot_live_owner_delivery.py tests/tui_gateway/test_bot_live_owner_delivery.py tests/tools/test_bot_mode_dm.py::test_delivery_runner_hands_off_to_live_owner tests/tools/test_bot_mode_dm.py::test_delivery_runner_surfaces_live_owner_refusal tests/tui_gateway/test_bot_relay_methods.py::test_deliver_lands_in_live_bot_chat_instead_of_subprocess -q

12 passed, exit 0.

Fixes #101060

…earch#101060)

When the target Bot Chat already holds a desktop/tui lease, the one-shot
CLI path was fenced out after message_agent acked sent. Hand the payload
to the live owner through a durable runtime queue so the open chat runs
the turn, with target_busy only when no owner claims in time.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: 686f6c61 <github@00b.tech>
Co-authored-by: Cursor <cursoragent@cursor.com>
@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 area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 2, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: #101293 repaired the Desktop relay path for this delivery family; this PR covers the remaining local CLI-to-live-owner path described by #101060.

@grym3s

grym3s commented Sep 3, 2026

Copy link
Copy Markdown

Thanks for picking this up — this is the right shape (owner-side handoff, complement to #101293's relay fast path) and it's the only open PR that touches the local runner path #101060 needs. I reviewed the diff against the failure modes we hit reproducing #101060 on Windows (three lost replies in one evening on main @ b1193b2, timestamps in my last comment on #101060). Three things I'd treat as blocking, the rest as follow-ups; every item comes with a test that fails on this branch today, and we're happy to contribute those tests or the fixes as commits on your branch.

Blocking

  1. Expiry unlink turns "never claimed" into "ambiguous". Owner side: claim_pending_delivery unlinks a pending file whose owner_deadline has passed (bot_live_delivery.py +219-221). Sender side at the same deadline: os.replace(pending, cancelled) → except FileNotFoundError: pass → falls through to the 600 s receipt wait → ambiguous/delivery_timeout (+138-161). Scenario: target busy until ≈turn_wait_seconds; its first idle poll after the deadline unlinks; the sender, sleeping 50 ms, sees FNF and waits ten more minutes for a receipt that cannot exist, for a message that provably never entered the session. This is the same race flagged on fix(gateway): keep Bot Mode DMs when Desktop owns the chat #100544. Fix: rename expired files to an expired/ dir instead of unlinking, and have the sender read FNF + expired present as not_delivered. Test: write a pending with owner_deadline in the past, call claim_pending_delivery, then run the sender's deadline branch → expect not_delivered, currently ambiguous.

  2. Crash auto-continue replays the DM. The DM runs through _run_prompt_submit with no marker flag, so run() writes the normal interrupted-turn marker with the DM text as its prompt; the next session.resume after a Desktop crash mid-DM replays it as a fresh turn (server.py _maybe_schedule_auto_continue; only bot_room is exempt today). And terminal_receipt calls complete_delivery unguarded: a failed receipt write (missing claimed file → FileNotFoundError, or any OSError from _atomic_json) surfaces as a turn error for a turn that completed, and because terminal_receipt_committed stays False the marker is not retired — so the replay happens on top of a "failed" receipt. Fix: record_turn_start(..., auto_continue=False) for DM-originated turns (mirror the bot_room exemption), and wrap the receipt commit so a write failure degrades to "ambiguous" without erroring the UI. Test: block run_conversation, assert read_turn_marker(...)["prompt"] == claimed["message"], then _maybe_schedule_auto_continue on a cold session must not schedule.

  3. Payload is still deleted on every non-delivered outcome, and the tool still says sent synchronously. _spawn_delivery and finally: _unlink_dm_file(dm_file) are untouched, so on target_busy, ambiguous, cancelled or failed the runner exits 1 and the payload is gone — message_agent reports status=sent on dispatch, then fails to deliver into desktop-owned Bot Chat sessions (single-surface lock) — DMs silently lost between local profiles #101060's silent loss becomes visible loss. And each attempt mints a fresh uuid4() (+117), so a retry after ambiguous is a second execution. I saw the ack shape is out of scope here, fair enough — but the payload retention and a stable id are inside this PR's own path: retain the payload on non-delivered outcomes with a typed receipt (reason, payload_file, retry_command) and derive the delivery id from the payload path so a re-run reads the existing receipt instead of re-executing. The sent→queued change can be a separate PR. Test: outcome ambiguous → payload still exists (currently deleted); same payload twice → second call returns the first receipt.

Follow-ups

  1. Human input during the DM turn: _handle_busy_submit is untouched, so with the default interrupt mode a human message typed 1 ms after the claim is injected into the DM turn or hard-interrupts it (→ receipt cancelled → DM lost). Fix: a session flag during DM turns that forces queue mode. Test: real prompt.submit while the DM turn blocks → queued, and the receipt carries the DM's own reply.
  2. No queued_prompt/queued_prompts gate: the poll checks only running/_closing, so between running=False and the tail drain a mailbox DM can be claimed ahead of an already-queued human or relay prompt (breaks the FIFO in docs/session-lifecycle.md §8, and interleaves with fix(bot-mode): DMs to a Desktop-owned Bot Chat land in the live session instead of being dropped (#100523) #101293's queued=True path). Test: _enqueue_prompt, then poll with running=False → must not claim.
  3. _owner_can_consume accepts any desktop/tui lease even without the marker (+26-30), and the marker is a boolean. On a mixed-version install (Desktop and profile gateways on different checkouts, which is common on Windows source builds) the sender writes pending, waits turn_wait_seconds, and loses the message. I understand the tradeoff (already-open chats work right after upgrade), but the failure mode is a silent loss after a wait. Middle ground: keep the surface heuristic, but when the wait expires with no claim and the lease carries no marker, fall back to the existing CLI path (which will get the typed refusal) instead of target_busy; and make the marker a version string. Test: lease with surface="desktop" and no marker → find_canonical_live_owner is None.
  4. claim_pending_delivery runs inside history_lock on every notification-poller tick (0.1–0.25 s) for every live session, not just Bot Chats. Fix: 1 s throttle and claim outside the lock with a recheck. Test: non-blocking acquire of history_lock during a claim succeeds.
  5. Windows: renaming a file another process has open raises PermissionError, not FileNotFoundError; the owner's _read_json(pending) can overlap the sender's cancel rename (+144-146) → uncaught → runner "failed" while the owner claims and executes. Catch OSError and re-poll for claimed.
  6. ambient_home = HERMES_HOME or ~/.hermes ignores the platform default (get_hermes_home() → %LOCALAPPDATA%\hermes on win32); with HERMES_HOME unset the owner is never found and delivery silently degrades to the refused CLI path.
  7. find_canonical_live_owner constructs a full SessionDB() on the target's state.db from the sender process, with exceptions uncaught (+39-49); a locked or never-created target DB now fails delivery before the subprocess even runs. Prefer a read-only probe with errors caught.
  8. The tui-side tests mock claim_pending_delivery, complete_delivery and _run_prompt_submit; we have real-lease / cross-process / real-pipeline tests for the contention repro, the claim-vs-cancel race, the human-queue case, the crash marker and receipt failure that we can add as a commit if you'd like them.

Reproduction harness that this PR should turn green: Forge (open tab) → message_agent → Mercury (no tab) delivers; Mercury's reply → Forge is refused already held by pid=… surface=desktop and lost. Same for Custodian ↔ Mercury. Happy to run the branch against that live setup and report.

Co-authored-by: Cursor <cursoragent@cursor.com>
@FalconOrtiz

Copy link
Copy Markdown
Author

Addressed grym3s blocking trio on 52518e9d6:

  1. Expired pending deliveries renamed into expired/ instead of unlink; sender maps that to not_delivered (no 10 min ambiguous wait).
  2. DM-originated turns use auto_continue=False; receipt write failures degrade without leaving a replayable crash marker.
  3. Non-delivered outcomes retain payload + stable delivery id derived from payload path; replay returns cached receipt.

Focused pytest: 19 passed. Local e2e smoke: e2e_smoke_ok — ~/.cursor/orchestrate/hermes-agent/reports/e2e-smoke-101564-52518e9d.md. Ready for re-review (follow-ups 4–11 still deferred).

@FalconOrtiz

Copy link
Copy Markdown
Author

Requesting squash-merge when you are satisfied with re-review.

grym3s blocking trio addressed on 52518e9d6 (expiry→not_delivered, no DM crash replay, payload retained on non-delivered). Local e2e_smoke_ok. Follow-ups 4–11 still deferred.

I do not have merge rights on NousResearch/hermes-agent — please squash-merge if this looks good.

teknium1 pushed a commit that referenced this pull request Sep 7, 2026
Adapt FalconOrtiz's owner-mailbox proposal from #101564 onto the current
notification poller and topical modules. The durable mailbox is cross-process
ingress only: the existing owner admits its normal prompt turn after the
current turn and human FIFO clear. Retain immutable receipts, stable admission
identities, capability and lease fencing, compression lineage, and disable
blind recovery replay of imported turns.

The original stale server hunks and expiring receipt protocol were rebuilt
rather than cherry-picked: the current facade decomposition and durable busy
admission contract differ. Credit the earlier owner-mailbox work in #100544
and durable producer work in #100319.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: 686f6c61 <github@00b.tech>
@teknium1

teknium1 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks @FalconOrtiz — your owner-mailbox proposal was adapted and landed in #105442, main 5280fe99872a58f1b9d25b00a79d240ecd035f7d. Contributor authorship is preserved in db96c8ced7da9a74f4e0258adb51a4c1361958d2, alongside credit for @fangliquanflq’s earlier owner-routing work and @686f6c61’s durable ingress work. The stale facade hunks and expiring protocol were rebuilt into the current topical modules: durable owner-fenced ingress, retained terminal receipts, human-FIFO admission, explicit consumer advertisement and no blind crash replay.

I am leaving this PR open rather than calling the entire diff superseded: your second commit also retains payloads and typed receipts for failed legacy CLI/peer deliveries. Main preserves that evidence for live-owner ingress, but the legacy _run_delivery fallback still unlinks its payload in finally, including on failure. A focused current-main probe confirmed that residual. The remaining work can be narrowed to legacy-transport failure retention; the Desktop/TUI live-owner handoff itself has shipped. Thank you for both the design and the follow-up reliability work.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Sep 8, 2026
hermes-release-bot Bot pushed a commit to mekenthompson/hermes-agent that referenced this pull request Sep 8, 2026
Adapt FalconOrtiz's owner-mailbox proposal from NousResearch#101564 onto the current
notification poller and topical modules. The durable mailbox is cross-process
ingress only: the existing owner admits its normal prompt turn after the
current turn and human FIFO clear. Retain immutable receipts, stable admission
identities, capability and lease fencing, compression lineage, and disable
blind recovery replay of imported turns.

The original stale server hunks and expiring receipt protocol were rebuilt
rather than cherry-picked: the current facade decomposition and durable busy
admission contract differ. Credit the earlier owner-mailbox work in NousResearch#100544
and durable producer work in NousResearch#100319.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: 686f6c61 <github@00b.tech>
@FalconOrtiz

Copy link
Copy Markdown
Author

Narrowed residual is in #108210. Live-owner admission shipped in #105442. This follow-up only retains failed CLI/peer _run_delivery payloads and typed receipts so replay does not re-execute. Leaving #101564 open.

@FalconOrtiz

Copy link
Copy Markdown
Author

Closing as superseded. Live-owner ingress shipped in #105442. The leftover CLI/peer payload retention is #108210. This branch is dirty against main and must not land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping 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/) duplicate This issue or pull request already exists 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

4 participants