Deferred items from review of #1097 (typed Relaycast wire contract). Each is a known, non-blocking follow-up; grouped here so they can be picked up together. File references verified against the broker crate as of the PR merge.
1. Synthetic reaction event-id collision (P2)
crates/broker/src/relaycast/wire.rs, WireMessageReacted::into_inbound builds a synthetic reaction event id as:
event_id: format!("reaction-{message_id}-{from}-{emoji}"),
The action field (add vs. remove) is not part of the id. Consequence: an add and a later remove of the same emoji, by the same agent, on the same message produce identical event_ids and collide. Downstream dedup could then drop the remove.
This is currently an acknowledged design decision (see the "Synthetic event ids" note in the PR #1097 description) and it deliberately matches the tolerant oracle — normalize_reaction in the broker's relaycast module (crates/broker/src/relaycast/bridge.rs) also omits action. The broker asserts the typed lane and the tolerant lane produce the same output, so the two must stay in parity.
Fix: fold action into the synthetic id so add/remove no longer collide. Because of the typed == tolerant parity assertion, this cannot be a one-sided change to wire.rs — it has to be coordinated so both lanes agree. That means either updating normalize_reaction in lockstep, or accepting a deliberate divergence with the parity tests updated to reflect it.
Also: add the same-agent add/remove fixture suggested in review to packages/contracts/fixtures/relaycast-ws-event-fixtures.json (an add followed by a remove of the same emoji, same agent, same message) to lock in the fixed behavior.
2. Memoize TypedThreadMessage::parse in thread-history assembly
Flagged independently by three reviewers. In crates/broker/src/runtime/messages.rs, build_thread_infos and its per-message helpers each independently call TypedThreadMessage::parse(value):
message_sender
message_target
message_preview
message_timestamp_string
message_thread_id
derive_thread_name
- plus the
TypedThreadMessage::parse(message).is_some() check inside build_thread_infos
That is roughly 6-8 full serde deserializations of the same JSON value per message, per assembly pass.
Fix: parse each message once and thread the parsed TypedThreadMessage through the helpers instead of re-parsing inside each one.
Smaller related nits that could ride along with the same refactor:
- Add a discriminating-key precheck at the top of
TypedThreadMessage::parse so obviously non-matching values bail before a full deserialize attempt.
- Eliminate the
from.clone() String clone in WireMessageReacted::into_inbound (crates/broker/src/relaycast/wire.rs, ~line 255) — from is moved into event_id/text via format! and cloned only to populate the from field; it can be reordered so the owned value lands in from without a clone.
- Remove the per-call allocation in
target_is_conversation_id (crates/broker/src/runtime/messages.rs). It currently does MessageTarget::new(target).kind(), allocating a MessageTarget (owned String) just to classify a &str. If fixed, add a borrowing classifier — e.g. MessageTargetKind::classify(&str) — and route through it rather than reintroducing inline starts_with/== prefix checks at the call site. The classification convention is intentionally centralized in crates/broker/src/ids.rs (MessageTarget::kind / MessageTargetKind), and the doc comment there asks call sites to match MessageTargetKind rather than re-derive the prefix rules; a borrowing API keeps that centralization intact.
3. Fixture cosmetics (P3)
packages/contracts/fixtures/relaycast-ws-event-fixtures.json reuses the same agent_id for distinct agents (e.g. alice and bob share an id). Give each named agent its own agent_id so the fixtures read correctly and don't imply two agents are the same identity.
References: PR #1097.
Deferred items from review of #1097 (typed Relaycast wire contract). Each is a known, non-blocking follow-up; grouped here so they can be picked up together. File references verified against the broker crate as of the PR merge.
1. Synthetic reaction event-id collision (P2)
crates/broker/src/relaycast/wire.rs,WireMessageReacted::into_inboundbuilds a synthetic reaction event id as:The
actionfield (add vs. remove) is not part of the id. Consequence: an add and a later remove of the same emoji, by the same agent, on the same message produce identicalevent_ids and collide. Downstream dedup could then drop the remove.This is currently an acknowledged design decision (see the "Synthetic event ids" note in the PR #1097 description) and it deliberately matches the tolerant oracle —
normalize_reactionin the broker'srelaycastmodule (crates/broker/src/relaycast/bridge.rs) also omitsaction. The broker asserts the typed lane and the tolerant lane produce the same output, so the two must stay in parity.Fix: fold
actioninto the synthetic id so add/remove no longer collide. Because of the typed == tolerant parity assertion, this cannot be a one-sided change towire.rs— it has to be coordinated so both lanes agree. That means either updatingnormalize_reactionin lockstep, or accepting a deliberate divergence with the parity tests updated to reflect it.Also: add the same-agent add/remove fixture suggested in review to
packages/contracts/fixtures/relaycast-ws-event-fixtures.json(an add followed by a remove of the same emoji, same agent, same message) to lock in the fixed behavior.2. Memoize
TypedThreadMessage::parsein thread-history assemblyFlagged independently by three reviewers. In
crates/broker/src/runtime/messages.rs,build_thread_infosand its per-message helpers each independently callTypedThreadMessage::parse(value):message_sendermessage_targetmessage_previewmessage_timestamp_stringmessage_thread_idderive_thread_nameTypedThreadMessage::parse(message).is_some()check insidebuild_thread_infosThat is roughly 6-8 full serde deserializations of the same JSON value per message, per assembly pass.
Fix: parse each message once and thread the parsed
TypedThreadMessagethrough the helpers instead of re-parsing inside each one.Smaller related nits that could ride along with the same refactor:
TypedThreadMessage::parseso obviously non-matching values bail before a full deserialize attempt.from.clone()String clone inWireMessageReacted::into_inbound(crates/broker/src/relaycast/wire.rs, ~line 255) —fromis moved intoevent_id/textviaformat!and cloned only to populate thefromfield; it can be reordered so the owned value lands infromwithout a clone.target_is_conversation_id(crates/broker/src/runtime/messages.rs). It currently doesMessageTarget::new(target).kind(), allocating aMessageTarget(ownedString) just to classify a&str. If fixed, add a borrowing classifier — e.g.MessageTargetKind::classify(&str)— and route through it rather than reintroducing inlinestarts_with/==prefix checks at the call site. The classification convention is intentionally centralized incrates/broker/src/ids.rs(MessageTarget::kind/MessageTargetKind), and the doc comment there asks call sites to matchMessageTargetKindrather than re-derive the prefix rules; a borrowing API keeps that centralization intact.3. Fixture cosmetics (P3)
packages/contracts/fixtures/relaycast-ws-event-fixtures.jsonreuses the sameagent_idfor distinct agents (e.g. alice and bob share an id). Give each named agent its ownagent_idso the fixtures read correctly and don't imply two agents are the same identity.References: PR #1097.