fix(comms): fail-closed inter-agent delivery + check-inbox nudge (#215 PR3) - #853
Merged
Conversation
…PR3) Inter-agent messages to codex-tmux agents were silently black-holed: send_to_agent returned "delivered" but the target's check_inbox stayed empty. Root cause in send_agent_message_direct (api.py): it marked the durable comms inbox row `read` the instant broker.inject_agent_message returned True. That bool only means "session CONNECTED and send() didn't raise" — for a TmuxSession/CodexTmuxSession, send() merely appends to a volatile in-memory _message_queue drained by a worker that pastes into an EXTERNAL pane. A dead / mid-turn / restarted / stale-CONNECTED pane drops the paste (and the queue is lost on teardown), yet the row was already read=1, so check_inbox (unread-only) never surfaced it. Classic fail-open-on-absence. Fix (NOT flag-gated): retire the durable copy only when the transport positively confirms consumption. New Transport capability `injection_confirms_consumption` — True for the in-process SDK StreamingSession (send == client.query into the live turn stream), False for tmux/codex (external pane). The handler marks read only when confirmed; otherwise the message stays unread+queued so check_inbox is the durable backstop. Fail-closed: unknown/missing sessions never confirm. Notify hook (#215 PR3, flag-gated PINKYBOT_AGENT_MSG_NOTIFY, default ON): when a message is left queued for a tmux target, broker.notify_unread_ agent_messages nudges it via the readiness-gated internal-prompt queue (_enqueue_internal_prompt) — "[pinky] N unread agent message(s) — call check_inbox". Best-effort; the durable unread row is the real guarantee. Coalesced (one per burst, PINKYBOT_AGENT_MSG_NUDGE_BACKOFF, default 15s), can't corrupt a mid-turn pane (single-consumer queue serializes turns), and never loops (an internal prompt is not a comms message). No-op for SDK/offline/unknown transports and when nothing is unread. Tests: tests/test_broker.py — confirms-consumption per transport (incl. fail-closed no-session); nudge fires for tmux, coalesces, respects the kill-switch, no-ops for SDK/no-session/nothing-unread. tests/test_api.py — end-to-end: a tmux target's live-inject stays unread and check_inbox still returns it (+ one nudge); an SDK-confirmed target is marked read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Murzik's review: SDK confirmation was transport-static, not outcome-specific. StreamingSession.send() swallows a failed client.query() (reconnect, no raise, message not replayed), yet inject_agent_message returned True and the API read the static injection_confirms_consumption=True capability — so a transient SDK query failure still marked the durable row read and black-holed that exact message. The API also looked the session up a SECOND time for the capability, which could race a session swap and describe a different transport than the one that injected. Fix: confirmation now comes from the exact injection attempt on the exact session object. - Transport send() contract: every send returns a per-call handoff bool. StreamingSession: True when client.query() accepted the message, False on drop or on the swallowed-exception path (behavior otherwise unchanged — test_failed_send_clears_pending_route passes unmodified). TmuxSession (inherited by CodexTmuxSession) and CodexSession: True on successful enqueue, False on drop — informational only, since their capability attr is False (CodexSession's now explicit). - broker.inject_agent_message() returns InjectResult(delivered, confirmed) — confirmed = per-call handoff AND the capability attr, computed on the SAME streaming object that performed the inject, in the same call. The racy broker.injection_confirms_consumption() second-lookup method is DELETED (no callers remain). - Callers destructure explicitly: api.py send path + its auto-wake re-inject branch gate mark_read on the returned confirmed; pollers.py Slack approval keys the card feedback off attempt-level delivered (no comms row on that path); routes/presentations.py documents its intentional result discard; test_slack_purchase_approval.py's broker stub returns a real InjectResult. Tests: test_broker.py — inject-result matrix: no-session (False, False); tmux connected (True, False); SDK success (True, True); SDK failed handoff (True, False) — capability must not overrule a failed handoff. test_api.py — real StreamingSession.send() handoff-bool unit test (success True / query-raise False, reconnect still awaited) + endpoint regression: SDK session whose query fails ⇒ row stays unread, check_inbox still returns it, confirmed=false, no nudge for SDK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…f bool (#853) Murzik's non-blocking round-2 note: implementations now return the handoff bool but the canonical Protocol still declared -> None. Align the Protocol annotation + docs (handoff vs consumption, drop => False) and the structural test stub. No runtime change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem — inter-agent messages to codex-tmux agents silently black-hole
send_to_agentreports success, the target'scheck_inboxnever shows the message. Chronic for codex-tmux targets (murzik, kuzya) — bad enough that murzik ran*/5mincheck-inbox crons as a workaround.Root cause (
api.py:8627-8629pre-fix):send_agent_message_directpersisted the message as an unread inbox row (correct), then calledbroker.inject_agent_message(...)and marked the row read on the baredeliveredbool. Butdelivered=Trueonly means "session was CONNECTED andsend()didn't raise":StreamingSession.send()→ in-processclient.queryinto the live turn stream — a real handoff.TmuxSession.send()(base ofCodexTmuxSession,tmux_session.py:3211) → appends to a volatile in-memoryasyncio.Queuedrained by a worker that pastes into an external tmux pane. Dead / busy / mid-turn / restarted / stale-CONNECTED pane ⇒ paste silently drops; queue lost on teardown.Marking read on that bool retires the durable copy from
check_inbox's unread-only view ⇒ black hole. Classic fail-open-on-absence.Forensics (live comms store, read-only, 2026-07-09): daemon log pairs
comms: barsik -> murzikwithbroker: injected agent messagefor each delivery; every injected message's row isread=1. The one message whose inject returnedFalse(msg 4491) is correctlyread=0and was the only one to surface. Aggregate: barsik→murzik all-time 1686 read-on-inject vs 1 unread; barsik→kuzya 188 vs 1. Essentially every message ever sent to the codex agents was retired on inject — whether or not the pane consumed it.Fix
api.pynow computesconfirmed = delivered and broker.injection_confirms_consumption(name)and onlymark_reads when confirmed. Otherwise the row stays unread+queued socheck_inboxremains the durable backstop. Response gains a"confirmed"field;"queued"semantics unchanged.injection_confirms_consumptionclass attr —StreamingSession = True(in-process handoff),TmuxSession = False(inherited byCodexTmuxSession). Broker helper is fail-closed: missing session / unknown transport ⇒False.broker.notify_unread_agent_messages()enqueues[pinky] N unread agent message(s) — call check_inbox…via the readiness-gated_enqueue_internal_prompt(can't corrupt a mid-turn pane; daemon-internal, so no nudge loops). Coalesced per-agent (reserve-before-await; default 15s window viaPINKYBOT_AGENT_MSG_NUDGE_BACKOFF). Best-effort by design — the unread row is the guarantee, the nudge is timeliness.PINKYBOT_AGENT_MSG_NOTIFY(default ON;0/false/no/offdisables the nudge only). The correctness fix is deliberately unflagged.Known tradeoff
A healthy tmux pane may now see a message twice: once live-injected, once via
check_inbox(row intentionally stays unread until retrieval). Never-drop > never-dup. Cleaner future model (nudge-only for tmux, single delivery throughcheck_inbox) interacts with the #279 reply-routing flow — tracked as a follow-up.Overlap with PR #831 (#280 dedup, open/held)
Shared files
broker.py/test_broker.py, no semantic collision — #831 dedupes the reply path (route_agent_reply), this fixes the send path. Both add a_statskey + adjacent blocks; whichever lands second needs a trivial rebase.Testing
ruff checkon all 6 touched files: clean.check_inboxreturns it, one nudge; SDK inject ⇒ marked read).test_broker.py+test_agent_comms.py126 passed;test_api.py363 passed;test_streaming_session.py+test_tmux_session.py360 passed. Independently re-verified post-build: affected subsets re-run green from the worktree (pytestpythonpath=["src"]confirmed to import branch code, not the editable prod install).Closes the delivery-drop incident from 2026-07-09 (two observed drops; murzik's polling workaround since removed). Implements #215 PR3.
🤖 Opened by Barsik
🤖 Generated with Claude Code