fix(channels): resolve channel_send mirror owner via bindings, not just default_agent#6023
Conversation
…st default_agent channel_send's outbound mirror (PR librefang#4932, closes librefang#4824) resolved the channel-owning agent via resolve_channel_owner(channel, _chat_id), which ignored chat_id and resolved purely by the channel's default_agent. When a [[bindings]] peer_id rule routes a specific (channel, chat_id) conversation to a non-default agent, the inbound reply went to the bound agent while the mirror was written to the default agent's session, so the bound agent never saw its own outbound context — the exact gap librefang#4824 meant to close. Make owner resolution binding-first: the kernel now iterates its specificity-sorted bindings and resolves the first whose match_rule matches (channel, chat_id), falling back to the channel default_agent only when no binding matches. To kill the duplicate-matcher drift that caused this bug class, hoist the binding match logic into a pure BindingMatchRule::matches method in librefang-types, now shared by both the inbound channel MessageRouter and the outbound mirror's owner resolution.
|
The direction here is right and the extraction of The kernel mesh binding store is not specificity-sorted on boot.
So when two bindings both match the same The tests don't catch this because they inject via Suggested fix (option A, minimal): don't depend on store order — select the highest-specificity match explicitly: bindings
.iter()
.filter(|b| b.match_rule.matches(channel, None, chat_id, None, &[]))
.max_by_key(|b| b.match_rule.specificity())
.map(|b| b.agent.clone())(Alternatively, option B: sort the Vec inside Add two regression tests for the currently-uncovered paths:
The fallback-to- |
The kernel binding store read by `resolve_channel_owner` is only specificity-sorted on the runtime `add_binding` path; the config-boot store (`MeshSubsystem::new`, populated from `config.bindings` in file order) is left unsorted, and the inbound `MessageRouter` keeps a separate, independently-sorted copy. Taking the first match from the unsorted kernel store could therefore resolve the outbound `channel_send` mirror to a broad binding declared earlier in config while the inbound reply routed to a more-specific one — re-opening the cross-agent mirror leak this PR set out to close (librefang#6022), with the trigger changed from `default_agent` to binding declaration order. Select the highest-specificity match explicitly so the result is independent of store order, with a strict-`>` fold that keeps the first match among equal-specificity bindings — matching the inbound router's stable-sort-then-first-match tie-break so the two paths cannot drift. Add two regression tests exercising the previously-uncovered paths: - boot-from-config (unsorted store) single-binding resolution - multi-match contest: a broad `channel`-only binding declared before a specific `peer_id` binding must still resolve to the specific one
|
Good catch — the premise is real and verified: Fixed in b127308 (option A — select max specificity explicitly, no dependence on store order): let mut best: Option<&AgentBinding> = None;
for b in bindings.iter().filter(|b| b.match_rule.matches(channel, None, chat_id, None, &[])) {
if best.is_none_or(|cur| b.match_rule.specificity() > cur.match_rule.specificity()) {
best = Some(b);
}
}
best.map(|b| b.agent.clone())One refinement over the Added both regression tests you asked for (
|
Closes #6022
Summary
channel_send's outbound mirror (PR #4932, closes #4824) resolves the channel-owning agent viaresolve_channel_owner(channel, _chat_id), which ignoredchat_idand resolved purely by the channel'sdefault_agent(sidecar_default_agent).When a
[[bindings]]peer_idrule routes a specific(channel, chat_id)conversation to a non-default agent, the inbound reply is delivered to the bound agent B but the outbound mirror is written to the default agent D's session.The bound agent never sees its own outbound context — the exact gap #4824 set out to close.
The fix makes owner resolution binding-first: the kernel now iterates its (specificity-sorted) bindings and resolves the first whose
match_rulematches(channel, chat_id), falling back to the channeldefault_agentonly when no binding matches.To kill the duplicate-matcher drift that caused this bug class, the binding match logic is hoisted into a pure
BindingMatchRule::matchesmethod inlibrefang-typesand is now the single source of truth shared by both the inbound channelMessageRouterand the outbound mirror's owner resolution.Changes
crates/librefang-types/src/config/types.rs: addBindingMatchRule::matches(channel, account_id, peer_id, guild_id, roles), a pure matcher reproducing the exact semantics of the former private matcher in the channels router (all specified fields ANDed; unset fields are wildcards;rolesmatches when the rule's role list is empty or intersects the provided roles).crates/librefang-channels/src/router.rs: refactor the privatebinding_matchesto delegate toBindingMatchRule::matches; signature and call sites (resolve_binding) unchanged, zero behaviour change.crates/librefang-kernel/src/kernel/handles/channel_sender.rs: rewriteresolve_channel_ownerto be binding-first — match the kernel bindings viaBindingMatchRule::matches(channel, None, chat_id, None, &[])(the inbound-for-outbound semantics already used bybound_recipients_for_agent), resolve the bound agent name → id, and fall back to the existingsidecar_default_agentpath when no binding matches; rename_chat_id→chat_id; the binding lock is held only briefly and dropped before the fallback.crates/librefang-kernel/src/kernel/handles/channel_sender.rs(logging): addtracing::debug!routing-decision logs at each branch (binding match, binding-matched-but-agent-unregistered, default_agent fallback, unresolved) with structuredchannel/chat_id/ agent-identifier fields only — no message bodies or PII — so an operator can see why the mirror landed where it did on the per-send hot path.librefang-types: seven unit tests forBindingMatchRule::matchescovering channel-only wildcard, all-unset, peer_id match/mismatch, account_id constraint, guild_id constraint, roles intersection, and the AND-of-all-specified-fields case.librefang-kernel:resolve_channel_owner_prefers_binding_then_default_agentboots a real kernel, registers two agents, adds apeer_idbinding, and asserts the mirror owner resolves to the bound agent for a matching(channel, peer_id), falls back to the channeldefault_agentfor an unbound chat, and resolves toNonewhen neither a binding nor adefault_agentexists.Verification
cargo check --workspace --lib— passes.cargo clippy -p librefang-types -p librefang-channels -p librefang-kernel --all-targets -- -D warnings— clean, zero warnings.cargo test -p librefang-types— passes; newbinding_matches_*tests green (7/7).cargo test -p librefang-channels— passes; allrouter::tests::test_binding_*behaviour-preservation tests green (484 total).cargo test -p librefang-kernel— passes;resolve_channel_owner_prefers_binding_then_default_agentgreen.Out-of-scope follow-up
The kernel-side fix replicates only the bindings + default_agent tiers of owner resolution.
The inbound
MessageRouteralso hasdirect_routeanduser_defaulttiers that the kernel'sChannelSenderimpl cannot currently reach, so full 4-tier parity for the mirror owner would require wiring aMessageRouterreference into the mirror path — a larger refactor deferred to a separate tracking issue, to be filed and linked after this PR lands.