Skip to content

fix(broker): agent-reply auto-route as true fallback with dedup (#280) - #831

Open
olegbrok wants to merge 3 commits into
mainfrom
fix/agent-reply-dedup-280
Open

fix(broker): agent-reply auto-route as true fallback with dedup (#280)#831
olegbrok wants to merge 3 commits into
mainfrom
fix/agent-reply-dedup-280

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

What

Make the #279 agent-to-agent reply auto-route a true fallback instead of an unconditional second delivery path, eliminating duplicate inbox entries when an agent replies explicitly.

Why

#279 delivers a completed agent-injected turn's final text to the requester's inbox. But agents (Murzik especially) also reply explicitly via send_to_agentcomms.send, which writes straight to the inbox. A turn that did both delivered the reply twice.

The dup hadn't surfaced in the wild yet only because no agent-injected turn had completed since #279 deployed (verified against the live conversations_agent_comms.db: the only auto_routed row in 3 days is a pushok self-test; every Murzik verdict came via explicit send_to_agent and predates the deploy). The next review verdict would have doubled.

How

  • inject_agent_message stamps the turn boundary: (responder, requester) -> start_ts.
  • route_agent_reply pops the marker and asks comms.has_message_since(responder, requester, since, exclude_auto_routed=True) — i.e. did the responder already send an explicit reply to the requester during this turn? If yes → suppress (the explicit copy is already in the inbox). If no → deliver (genuine fallback, e.g. an interrupted turn that produced text but never sent it).
  • Backend-agnostic — no reliance on tool_uses (codex/tmux turns omit them), which is why detection is done against the comms DB, not the turn payload.
  • Never content-matches — so it can't drop a genuinely new reply.
  • Fails safe — a missing marker (lost across a restart) or a probe error falls through to delivery. A stray dup is acceptable; a dropped review verdict is not.
  • Orphan markers swept by TTL (1h) on insert → bounded memory.
  • Kill-switch PINKY_AGENT_REPLY_DEDUP (default on); off restores the Server-side presence tracking: stamp last_seen agent-agnostic #279 unconditional delivery. Counted via the existing broker deduped stat.

Design note for review

This uses a timestamp window, not content equality. Rationale: the auto-route is a safety net for agents that produce a reply but don't send it. If an agent demonstrably knows how to send (it sent explicitly to the requester this turn), the net is off for that turn — it should send any additional content explicitly too. Trade-off: if an agent sends an unrelated explicit message mid-turn (e.g. a clarifying question) and then relies on the auto-route for its actual answer, the answer is suppressed (recoverable — it's in the transcript and the requester notices the gap). Considered acceptable vs. content-matching, which would silently miss real dups whenever a codex turn's final text differs from the sent content.

Tests

tests/test_broker.py — suppress-on-explicit-send, deliver-on-no-send, deliver-on-missing-marker (safety), killswitch-off-always-delivers, inject-marks-turn-start.
tests/test_agent_comms.pyhas_message_since incl. exclude-auto-routed, window boundary, legacy/empty-metadata-counts.

pytest tests/test_broker.py tests/test_agent_comms.py → 126 passed. ruff clean.

Deploy

Held — not for immediate deploy. Touches the live agent-reply path; wants Murzik review + a supervised daemon bounce (recent fan-out instability on the Mini). Inert behind the existing #279 routing being on; the dedup defaults on but only changes behavior on the explicit-send-this-turn path.

🤖 Opened by Barsik

…280)

requester's inbox. But agents also reply explicitly via send_to_agent
(comms.send -> inbox directly), so a turn that did both delivered the
reply TWICE. The dup hadn't surfaced yet only because no agent-injected
turn had completed since #279 deployed; the next Murzik review verdict
would have doubled.

Make auto-route a fallback: suppress it when the responder already sent
an explicit (non-auto_routed) message to the requester since the turn
began. The turn boundary is captured at inject_agent_message time
((responder, requester) -> start ts), consumed by route_agent_reply.

- Backend-agnostic: no reliance on tool_uses (codex turns omit them).
- Never content-matches, so it can't drop a genuinely new reply.
- Missing marker (e.g. lost across a restart) or a probe error falls
  through to delivery — a stray dup is acceptable, a dropped verdict is
  not.
- Orphan markers swept by TTL on insert (bounded memory).
- Kill-switch PINKY_AGENT_REPLY_DEDUP (default on) restores #279.

New AgentComms.has_message_since() backs the probe. Counted via the
existing broker `deduped` stat. Tests cover suppress-on-explicit-send,
deliver-on-no-send, deliver-on-missing-marker (safety), killswitch, the
inject marker, and the comms probe (incl. legacy/empty metadata).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@olegbrok
olegbrok force-pushed the fix/agent-reply-dedup-280 branch from fd9e6aa to 2c1511a Compare July 10, 2026 18:19
olegbrok and others added 2 commits July 10, 2026 12:52
A failed per-call handoff produces no route-back turn, but the #280
dedup marker stamped before the send survived it. The orphan stayed
live for up to the marker TTL and any unrelated explicit
responder->requester message would then suppress a DIFFERENT
injection's distinct route-back — a dropped verdict, not a stray dup.

The unmark is identity-guarded (timestamp token) so an older failed
call can never pop a newer concurrent injection's marker, and dedup
still fails open to delivery when no marker is present. Regressions:
failed handoff leaves no marker and cannot suppress a distinct later
route-back; raising sends unmark and propagate; True-handoff behavior
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity (#831 P1)

time.time() repeats at clock resolution, so two overlapping injections
for the same pair can stamp EQUAL floats — an equality-guarded unmark
then lets the older failed inject pop the newer successful marker.
Markers are now _ReplyTurnMarker records: started_at carries the dedup
window boundary consumed by route_agent_reply, while ownership is the
record's object identity compared with `is`. Adds the overlapping
same-timestamp regression (frozen clock, older send blocks and fails
while newer succeeds — newer marker must survive).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant