Repository navigation
fix(gateway): keep Bot Mode DMs when Desktop owns the chat - #100544
fangliquanflq wants to merge 3 commits into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewing exact head 1339841f14c64b7e25e69dee4b0fd017af2f8df0 (1 commit) against the PR's recorded main base 622883bad7f55f56a6393cd994e36c65fbdff253; live upstream main is now 043c258ac29579100bab66fe426c6d4f57ded14f. I read the full 11-file diff, #100523, the competing #100542 implementation and its issue-claim history, adjacent #100547, the new handoff/owner tests, and the exact-head hosted checks. CI, Docker, and Nix are green on this head. There was no prior submitted review on this exact head.
The architectural direction here is strong: keep the single-owner transcript fence, route the DM through the owner that already has authority, and separate queue acknowledgement from a durable terminal outcome. That is materially better than bypassing ownership. I do not think this head is merge-ready yet, though, because the new durability protocol has correctness holes at exactly the claim/settlement boundary it is meant to make authoritative.
1. Blocking: pending disappearance does not prove the owner claimed the delivery
deliver_to_live_owner() says the sender losing pending -> .cancelled means "the owner crossed the claim boundary and only a receipt can say whether the message entered the session." That would be a sound certainty boundary if claiming were the only other transition that could remove pending.
It is not. claim_pending_delivery() independently does this for a normal expiry:
if float(payload.get("owner_deadline") or 0) <= now:
pending.unlink(missing_ok=True)
continueThere is therefore a real deadline race:
- owner polls just after
owner_deadline, sees the request expired, and unlinkspendingwithout claiming it; - sender reaches its cancellation
os.replace(pending, cancelled)and getsFileNotFoundError; - sender interprets that missing path as a lost claim race, waits up to the receipt timeout, then returns
ambiguous/delivery_timeout; - the request was in fact definitively not delivered and no receipt can ever exist.
That breaks the PR's core settlement claim and can turn a definite refusal into a 600-second ambiguous wait. The state machine needs one unambiguous transition owner per edge: e.g. only the sender may cancel an unclaimed pending, while the owner may only atomically pending -> claimed; or expiry needs its own durable terminal state that the sender can distinguish. Please add a deterministic boundary-race regression proving (a) expired + unclaimed is not_delivered, and (b) only an actually claimed request may become transport-ambiguous when the receipt is lost.
2. Blocking: the function documented as claiming the "oldest" request does not preserve DM order
deliver_to_live_owner() names each pending file with uuid.uuid4().hex, but claim_pending_delivery() selects with:
for pending in sorted(pending_dir.glob("*.json")):Lexicographic UUID order is unrelated to created_at. With two DMs queued for the same Bot Chat, the later DM can be claimed first. These are not fungible work items: each claimed payload becomes the next user turn in the canonical transcript, so reordering changes what the recipient reasons over and what reply the sender receives.
Please make queue order explicit (created_at plus a deterministic tie-break, or an order-preserving enqueue key with an atomic claim protocol) and add a two-or-more-message test that proves ingress order matches enqueue order. The existing single-request round trip cannot catch this class.
3. Blocking: the peer/API fast path widens this change from "peer DM handoff" to "all live Bot Chat requests" and drops request semantics
The new branch in _handle_session_chat() triggers solely on title == "Bot Chat" plus a string user_message, before the existing runtime-selection block. It parses system_message / instructions, but the handoff persists only user_message; on success it also returns empty usage / runtime objects. So an ordinary /api/sessions/{id}/chat request that supplies per-request system context, a model lock, or runtime selection changes behavior merely because the Desktop currently owns that Bot Chat.
The new test_peer_dm_routes_through_existing_live_owner_e2e does not exercise that contract because it monkeypatches both find_canonical_live_owner and deliver_to_live_owner; it proves routing, not that the live-owner execution preserves API request semantics.
This needs either an explicit peer-DM/internal-delivery discriminator so only that path enters the handoff, or a handoff envelope that preserves every execution field the ordinary session-chat path promises. #100547 is directly adjacent here: it adds per-message system context through prompt.submit and also changes tui_gateway/server.py. These PRs need an explicit composition/merge-order test rather than landing independently and letting one side silently discard the other's context.
4. Blocking edge: owner-detach between discovery and claim still creates an avoidable delivery failure; #100542 owns the complementary half
All three producers first snapshot liveness with find_canonical_live_owner() and then call deliver_to_live_owner(). If the Desktop detaches in that interval, no owner remains to claim the new file. After the owner wait (default bot_mode.turn_wait_seconds, 120s), this head returns target_busy / failure instead of re-resolving ownership and falling through to the existing CLI/API path that is now safe to acquire the session.
That is the other side of #100523's shape: the report explicitly says delivery succeeds once Desktop detaches. #100542 implements the detach/retry half using the existing envelope_ttl_seconds budget; this PR implements the through-live-owner half. Those mechanisms are complementary at the ownership phase transition, not merely interchangeable duplicates. The safe composition is: retry/fall through only after a definitive unclaimed result; never retry after claimed/ambiguous because that could duplicate a turn. Add a regression where the owner disappears after discovery but before claim and prove exactly-once delivery through the fallback.
Please also preserve the #100542 contributor's provenance if this becomes the canonical carrier. 686f6c61 claimed #100523 at 17:15:33Z and opened #100542 at 17:29:03Z; this PR opened four seconds later and directly overlaps tools/bot_mode_dm.py. If this architecture supersedes the carrier, the earlier contribution should remain visible rather than being flattened into "duplicate work."
5. Structural / merge-order gates
The new focused tools/bot_live_delivery.py module is the right extraction direction, but this PR still adds integration behavior directly into two existing godfiles: gateway/platforms/api_server.py (the hunk is already at ~4.7k lines) and tui_gateway/server.py (the hunk is at ~12.2k). Keep those entry points skinny and move the live-owner integration mechanics behind focused sub-2k modules rather than growing them further.
There are also two live FILE-LIST collisions that must be composed before merge:
- #100542:
tools/bot_mode_dm.py— same #100523 defect class and earlier issue claim; - #100547:
tui_gateway/server.py— per-message system-context propagation that this handoff currently cannot carry.
Finally, this head is now two commits behind live main. The exact-head CI/Docker/Nix matrix is green, which is good evidence for this object, but it is not a current-main composition receipt. Rebase/merge current main, resolve the two live interlocks above, and require the surviving commit graph to go green again.
The hard part here is being attacked in the right place: ownership is preserved instead of weakened, and the attempt to make delivery certainty typed/durable is exactly the right instinct. Once the claim boundary is actually exclusive, ordering is deterministic, API semantics are scoped correctly, and the detach transition composes with the earlier work, this can be a very solid fix. Thanks for taking on a genuinely tricky session-ownership problem. 🚀
…on instead of being dropped (#100523) When the Desktop has a bot's "Bot Chat" open, that session holds the single-owner lease, so the `hermes -p <bot> chat -c "Bot Chat"` subprocess `bot_relay.deliver` spawns refuses with "already has a live owner" and the DM payload is dropped — the sender was already acked. bot_relay.deliver now looks up a live in-process session for the target profile whose title resolves to "Bot Chat" (same profile_home match as session.resume's _find_live_unpersisted, pending_title for lazy sessions, otherwise the db title) and, when found, submits the message through the existing prompt.submit handler — the composer's choke point — so it lands as a normal user turn (role alternation preserved, streams to the open window). No live owner → the subprocess path runs exactly as before. On the local message_agent subprocess path, the lease refusal is surfaced as a structured `target_busy` delivery failure telling the sender the message was NOT delivered, instead of a raw exit-1 with the text buried in stderr. Closes #100523 Supersedes #100544, #100542 Co-authored-by: fangliquanflq <fangliquan@qq.com> Co-authored-by: 686f6c61 <github@00b.tech>
…on instead of being dropped (#100523) When the Desktop has a bot's "Bot Chat" open, that session holds the single-owner lease, so the `hermes -p <bot> chat -c "Bot Chat"` subprocess `bot_relay.deliver` spawns refuses with "already has a live owner" and the DM payload is dropped — the sender was already acked. bot_relay.deliver now looks up a live in-process session for the target profile whose title resolves to "Bot Chat" (same profile_home match as session.resume's _find_live_unpersisted, pending_title for lazy sessions, otherwise the db title) and, when found, submits the message through the existing prompt.submit handler — the composer's choke point — so it lands as a normal user turn (role alternation preserved, streams to the open window). No live owner → the subprocess path runs exactly as before. On the local message_agent subprocess path, the lease refusal is surfaced as a structured `target_busy` delivery failure telling the sender the message was NOT delivered, instead of a raw exit-1 with the text buried in stderr. Closes #100523 Supersedes #100544, #100542 Co-authored-by: fangliquanflq <fangliquan@qq.com> Co-authored-by: 686f6c61 <github@00b.tech>
|
Thanks — the routing insight here (deliver into the live in-process Bot Chat instead of fighting the single-owner fence) landed on main via #101293 with you co-authored. Reworked to ~+108 LOC: bot_relay.deliver looks up the live session and submits through prompt.submit with queued=True (never interrupts a turn in flight); the subprocess path surfaces the refusal as target_busy. Closing in favor of the merged fix. |
…on instead of being dropped (NousResearch#100523) When the Desktop has a bot's "Bot Chat" open, that session holds the single-owner lease, so the `hermes -p <bot> chat -c "Bot Chat"` subprocess `bot_relay.deliver` spawns refuses with "already has a live owner" and the DM payload is dropped — the sender was already acked. bot_relay.deliver now looks up a live in-process session for the target profile whose title resolves to "Bot Chat" (same profile_home match as session.resume's _find_live_unpersisted, pending_title for lazy sessions, otherwise the db title) and, when found, submits the message through the existing prompt.submit handler — the composer's choke point — so it lands as a normal user turn (role alternation preserved, streams to the open window). No live owner → the subprocess path runs exactly as before. On the local message_agent subprocess path, the lease refusal is surfaced as a structured `target_busy` delivery failure telling the sender the message was NOT delivered, instead of a raw exit-1 with the text buried in stderr. Closes NousResearch#100523 Supersedes NousResearch#100544, NousResearch#100542 Co-authored-by: fangliquanflq <fangliquan@qq.com> Co-authored-by: 686f6c61 <github@00b.tech>
…on instead of being dropped (NousResearch#100523) When the Desktop has a bot's "Bot Chat" open, that session holds the single-owner lease, so the `hermes -p <bot> chat -c "Bot Chat"` subprocess `bot_relay.deliver` spawns refuses with "already has a live owner" and the DM payload is dropped — the sender was already acked. bot_relay.deliver now looks up a live in-process session for the target profile whose title resolves to "Bot Chat" (same profile_home match as session.resume's _find_live_unpersisted, pending_title for lazy sessions, otherwise the db title) and, when found, submits the message through the existing prompt.submit handler — the composer's choke point — so it lands as a normal user turn (role alternation preserved, streams to the open window). No live owner → the subprocess path runs exactly as before. On the local message_agent subprocess path, the lease refusal is surfaced as a structured `target_busy` delivery failure telling the sender the message was NOT delivered, instead of a raw exit-1 with the text buried in stderr. Closes NousResearch#100523 Supersedes NousResearch#100544, NousResearch#100542 Co-authored-by: fangliquanflq <fangliquan@qq.com> Co-authored-by: 686f6c61 <github@00b.tech> (cherry picked from commit d29a793)
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>
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>
What does this PR do?
Bot Mode teammate DMs no longer disappear when the recipient's canonical Bot Chat is already attached to the Desktop. Instead of starting a competing CLI owner and failing after the sender has received an asynchronous acknowledgement, local, relay, and peer deliveries hand the message to the existing live owner and receive a durable terminal receipt.
Symptom
A Bot Mode agent receives
status: queued, but the background delivery process exits withSession ... already has a live ownerwhile the recipient's Bot Chat remains open in the Desktop. The message is not delivered.Impact
Users actively viewing a recipient bot can lose teammate DMs even though the sending agent was told that asynchronous delivery had started. The failure affects Bot Mode message delivery through local, Desktop-relay, and peer API paths.
Bug Cause
Trigger:
tools/bot_mode_dm.py:_run_deliverystartshermes -p <profile> chat ...while the target canonical Bot Chat has an active Desktop lease.Causal chain:
message_agentstarts an asynchronous local or relayed Bot Chat delivery.hermes_cli.active_sessions.try_acquire_active_sessioncorrectly rejects the competing owner, so the background process exits and the queued message is lost.Why it is wrong: The single-owner guard is a required transcript-consistency boundary, but Bot Mode treated the only safe owner as an unrecoverable delivery conflict instead of routing the inbound turn through it.
Working sibling / contrast: Delivery succeeds after the Desktop detaches because the CLI can then acquire the canonical session lease. The fix preserves that idle-session path and changes only the live-owner path.
Ruled out: Removing or bypassing per-session ownership is not safe; it would allow two agents to reason from divergent transcript snapshots.
Fix
Add a durable per-session handoff with atomic pending-to-claimed transitions and terminal receipts. Live TUI/Desktop owners poll at turn boundaries, claim one inbound DM only while idle, run it through the normal prompt path, and publish whether it settled, failed before delivery, or became transport-ambiguous. Local
message_agent, Desktop relay, and peer API delivery paths all use the same handoff when a canonical live owner exists, while idle sessions retain the existing CLI transport.Related Issue
Closes #100523
Type of Change
Changes Made
tools/bot_live_delivery.py- add durable live-owner request, claim, cancellation, and receipt primitives.tools/bot_mode_dm.py- route local Bot Mode DMs through an existing canonical session owner and report queue acknowledgement accurately.tui_gateway/server.py- claim queued DMs at idle turn boundaries and publish terminal receipts.tui_gateway/methods_bot_relay.py- use the same live-owner handoff for Desktop-relayed DMs.gateway/platforms/api_server.py- route peer DM submissions through the existing live owner.tests/- cover local, relay, peer, ownership, timeout, and turn-boundary behavior.website/docs/user-guide/bot-mode.md- document queue acknowledgements and terminal delivery outcomes.How to Test
message_agent, the Desktop relay, orhermes peer dm.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
cli-config.yaml.exampleis N/A because no config keys changedCONTRIBUTING.mdandAGENTS.mdare N/A because no contribution workflow changedmessage_agenttool description to distinguish queue acknowledgement from delivery receiptScreenshots / Logs
N/A - the fix is covered by behavior and integration tests.