Skip to content

fix(channels): resolve channel_send mirror owner via bindings, not just default_agent#6023

Merged
houko merged 7 commits into
librefang:mainfrom
neo-wanderer:fix/channel-send-mirror-binding-aware
Jun 9, 2026
Merged

fix(channels): resolve channel_send mirror owner via bindings, not just default_agent#6023
houko merged 7 commits into
librefang:mainfrom
neo-wanderer:fix/channel-send-mirror-binding-aware

Conversation

@neo-wanderer

Copy link
Copy Markdown
Contributor

Closes #6022

Summary

channel_send's outbound mirror (PR #4932, closes #4824) resolves the channel-owning agent via resolve_channel_owner(channel, _chat_id), which ignored chat_id and resolved purely by the channel's default_agent (sidecar_default_agent).
When a [[bindings]] peer_id rule 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_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, the binding match logic is hoisted into a pure BindingMatchRule::matches method in librefang-types and is now the single source of truth shared by both the inbound channel MessageRouter and the outbound mirror's owner resolution.

Changes

  • crates/librefang-types/src/config/types.rs: add BindingMatchRule::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; roles matches when the rule's role list is empty or intersects the provided roles).
  • crates/librefang-channels/src/router.rs: refactor the private binding_matches to delegate to BindingMatchRule::matches; signature and call sites (resolve_binding) unchanged, zero behaviour change.
  • crates/librefang-kernel/src/kernel/handles/channel_sender.rs: rewrite resolve_channel_owner to be binding-first — match the kernel bindings via BindingMatchRule::matches(channel, None, chat_id, None, &[]) (the inbound-for-outbound semantics already used by bound_recipients_for_agent), resolve the bound agent name → id, and fall back to the existing sidecar_default_agent path when no binding matches; rename _chat_idchat_id; the binding lock is held only briefly and dropped before the fallback.
  • crates/librefang-kernel/src/kernel/handles/channel_sender.rs (logging): add tracing::debug! routing-decision logs at each branch (binding match, binding-matched-but-agent-unregistered, default_agent fallback, unresolved) with structured channel / 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.
  • Tests:
    • librefang-types: seven unit tests for BindingMatchRule::matches covering 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_agent boots a real kernel, registers two agents, adds a peer_id binding, and asserts the mirror owner resolves to the bound agent for a matching (channel, peer_id), falls back to the channel default_agent for an unbound chat, and resolves to None when neither a binding nor a default_agent exists.

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; new binding_matches_* tests green (7/7).
  • cargo test -p librefang-channels — passes; all router::tests::test_binding_* behaviour-preservation tests green (484 total).
  • cargo test -p librefang-kernel — passes; resolve_channel_owner_prefers_binding_then_default_agent green.

Out-of-scope follow-up

The kernel-side fix replicates only the bindings + default_agent tiers of owner resolution.
The inbound MessageRouter also has direct_route and user_default tiers that the kernel's ChannelSender impl cannot currently reach, so full 4-tier parity for the mirror owner would require wiring a MessageRouter reference into the mirror path — a larger refactor deferred to a separate tracking issue, to be filed and linked after this PR lands.

…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.
@github-actions github-actions Bot added size/L 250-999 lines changed area/channels Messaging channel adapters area/kernel Core kernel (scheduling, RBAC, workflows) labels Jun 6, 2026
@houko

houko commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

The direction here is right and the extraction of binding_matches into a single BindingMatchRule::matches (eliminating the dual matcher) is clean and behavior-equivalent — I verified that field-by-field and the inbound path is unchanged. But resolve_channel_owner relies on a premise that doesn't hold on boot-from-config, and it can re-open the cross-agent leak this PR is meant to close (#6022 / #4824).

The kernel mesh binding store is not specificity-sorted on boot.
resolve_channel_owner takes the first match (bindings.iter().find(...)), commenting that the Vec is kept specificity-sorted. But:

  • self.mesh.bindings is a Mutex<Vec<AgentBinding>> stored as-is by MeshSubsystem::new (subsystems/mesh.rs:73), populated from config.bindings in config file order (boot.rs:1263, 1523).
  • Only the runtime add_binding path sorts (bindings_and_handle.rs:196). A plain config-boot store is unsorted.
  • The inbound router uses a separate store that is sorted (librefang-channels/src/router.rs:152 load_bindings), so inbound is most-specific-first but the outbound mirror reads the unsorted kernel store.

So when two bindings both match the same (channel, chat_id) — e.g. a broad channel-only binding (agent D) plus a specific peer_id binding (agent B) — and the broad one is written first in config, the mirror resolves to D while the inbound reply still routes to B. Outbound context lands on the wrong agent: the same failure mode as #6022, with the trigger changed from "default_agent" to "binding declaration order".

The tests don't catch this because they inject via kernel.add_binding(...) (which sorts) and use a single binding — they never exercise the unsorted config-boot path or a multi-match contest.

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 MeshSubsystem::new so the kernel store shares the inbound router's invariant — broader blast radius, so please add a store-ordering assertion test if you go that way.)

Add two regression tests for the currently-uncovered paths:

  1. Boot-from-config: start from KernelConfig { bindings: vec![...], .. } directly (not add_binding) and assert the mirror owner is correct — this is the unsorted path.
  2. Multi-match contest: put two matching bindings on the same (channel, chat_id) — a broad channel-only one and a specific peer_id one — with the broad one first in config, and assert the owner resolves to the specific peer_id agent. With the current find-based code this test fails; that's the regression it should pin.

The fallback-to-default_agent path, the BindingMatchRule::matches extraction, and the out-of-scope follow-up you flagged (direct_route / user_default layers) all look fine — just the binding-layer ordering needs to not rely on the unsorted store.

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
@neo-wanderer

Copy link
Copy Markdown
Contributor Author

Good catch — the premise is real and verified: MeshSubsystem::new (subsystems/mesh.rs:73) stores config.bindings as-is in file order, only add_binding (bindings_and_handle.rs:197) sorts, and the inbound router keeps a separately-sorted copy (router.rs::load_bindings). So resolve_channel_owner's find-first read of the unsorted kernel store could mirror outbound context to a broad binding declared earlier in config while the inbound reply routed to a more-specific one — the same split as #6022, retriggered by declaration order. The comment claiming the Vec is "kept specificity-sorted on load" was wrong for the config-boot path.

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 max_by_key snippet: max_by_key returns the last element on a specificity tie, whereas the inbound router does a stable descending sort and takes the first match. To keep the two paths from drifting on equal-specificity bindings, the fold replaces only on strictly-greater specificity, so the first equal-specificity match wins — matching inbound exactly.

Added both regression tests you asked for (crates/librefang-kernel/src/kernel/tests.rs):

  1. resolve_channel_owner_uses_unsorted_config_boot_bindings — boots from KernelConfig { bindings: vec![..] } directly (not add_binding), exercising the unsorted store.
  2. resolve_channel_owner_prefers_specific_binding_over_broad_in_config_order — broad channel-only binding declared first, specific peer_id binding second; asserts the specific agent wins. Fails against the old find-based code.

cargo check -p librefang-kernel --lib, clippy -p librefang-kernel --lib -- -D warnings, and the three resolve_channel_owner_* tests all pass locally.

@houko
houko enabled auto-merge (squash) June 9, 2026 15:24
@houko
houko merged commit 4ec5116 into librefang:main Jun 9, 2026
32 checks passed
@neo-wanderer
neo-wanderer deleted the fix/channel-send-mirror-binding-aware branch June 10, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/channels Messaging channel adapters area/kernel Core kernel (scheduling, RBAC, workflows) size/L 250-999 lines changed

Projects

None yet

2 participants