fix(channels): account_id in ThreadKey + lazy sweep + push bypass note#3420
Closed
houko wants to merge 1 commit into
Closed
fix(channels): account_id in ThreadKey + lazy sweep + push bypass note#3420houko wants to merge 1 commit into
houko wants to merge 1 commit into
Conversation
Address review feedback on #3414: 1. Multi-tenant key collision (security FIX). `ThreadKey` was `(channel, thread)` only; two Slack workspaces (account_id A vs B) producing the same `thread_ts` collided in the registry, letting account A's claim shadow account B's distinct conversation. New shape `ThreadKey { channel, account_id: Option<String>, thread }`, constructor takes `(channel, account_id, thread)`. Empty account_id normalizes to `None` so single-tenant traffic keeps a stable key. 2. Unbounded registry growth (quality BUG / security FIX). `sweep_expired` had no caller — `decide` evicted only on access, so a daemon in a Slack workspace with churning `thread_ts` accumulated stale entries until restart. New `BridgeManager::ensure_thread_ownership_sweep_started`, armed lazily on the first `start_adapter` call (so the sync constructor stays runtime-free for unit tests), runs a 60-second `tokio::time::interval` tied to `shutdown_rx`. 3. REST push bypass (Explore BYPASS-RISK → BYPASS-OK with explicit doc). `push_message` (cron / workflow / `agent_send` / REST `/push` outbound) intentionally does **not** consult the registry: the caller has already chosen who is sending, so there is no routing decision to gate. Added a doc-comment explaining the asymmetry and pointing to the follow-up path (opt-in `respect_thread_ownership` parameter) if user-visible pain shows up. 4. Test coverage. Added `distinct_accounts_do_not_collide_on_same_thread_id` and `empty_account_id_normalized_to_none` in `thread_ownership::tests`, plus `multi_tenant_account_id_flows_into_thread_key` in `bridge::tests` so the helper actually exercises the new metadata read. The bridge-side helper still mirrors the gate's logic — the acknowledged debt of a real-dispatch integration test (drives `dispatch_message` end-to-end with a stub `ChannelBridgeHandle`) is captured in the helper's doc-comment for follow-up. Pre-flight: `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test -p librefang-channels --lib --no-fail-fast` 843 passed (3 new — 2 in `thread_ownership::tests`, 1 in `bridge::tests`). Out of scope, captured in PR description: mention-spoofing as design intent ("@-mention re-claims" matches the issue spec), restart drops all claims (single-process MVP), real-dispatch integration test for the gate's order-of-operations vs RBAC / journal / auto-reply.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
houko
added a commit
that referenced
this pull request
Jun 16, 2026
… groups (#5323) (#6127) * feat(channels,kernel): per-conversation agent routing for multi-agent groups (#5323) Route a group message that names a specific non-default agent to that agent instead of the channel default, and keep the conversation sticky to it. Two addressing paths feed a new agent resolution step ahead of the binding/default chain: an explicit @-mention the adapter surfaces in metadata["mention_names"] (resolved against agent names/handles), and a non-default agent's channel_overrides.group_trigger_patterns alias matching the text. Alias disambiguation uses a deterministic per-agent attention scorer (librefang_channels::bridge::best_alias_match) reusing the compiled regex cache, which the dispatch path also consults before the previously non-deterministic "first available" fallback (closes layer (c) of #5294). ThreadKey grows three optional slices — account_id (multi-tenant, the unlanded #3419/#3420 fix), chat_id, and peer_id (per-sender stickiness) — all defaulting to None so the historical (channel, thread) key is reproduced byte-for-byte. A topic-less group now claims by chat id instead of bypassing the registry, and a live claim makes a follow-up sticky to the same agent without a fresh mention; an explicit address re-claims for the new agent, preserving #3334 TTL semantics. New per-channel knobs conversation_ownership_ttl_seconds (default 600) and conversation_ownership_include_dms (default false). The Telegram / Discord / Slack / Matrix sidecar adapters surface mention_names and a per-group sender_user_id so the bridge can route and scope per peer. Built on #5671 PR-A's agent/available_agents schema; additive and backward compatible. Verified: cargo check --workspace --lib, clippy -D warnings, cargo fmt --check, cargo test -p librefang-channels (493) and -p librefang-types config, and the Python sidecar suites (288). * docs(channels): reflow new doc comments to one sentence per line Doc comments added in the multi-agent routing change were mid-sentence wrapped at ~80 columns. The prose convention (CLAUDE.md) requires no column limit — break only at sentence boundaries so a single-word edit doesn't reflow an entire paragraph. Affected: ThreadKey struct/builder docs, best_alias_match, RouteResolution, mention_names, agent_allows_channel, resolve_addressed_agent, build_thread_key, conversation_ownership_allows, module-level doc. No logic change; rustfmt passes. --------- Co-authored-by: Evan <[email protected]> Co-authored-by: Claude <[email protected]>
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
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.
Follow-up to merged #3414. Picks up the review-feedback fixes that were pushed to
feat/3334-thread-ownershipafter that branch was merged ate3b1b847, so they never made it ontomain. Cherry-picks one commit (bb81cd1ehere) onto a fresh branch frommain.The fixes target real bugs in the merged
thread_ownershipmodule:ThreadKeygainsaccount_id: Option<String>and the bridge dispatch sites readmessage.metadata["account_id"]thread_tscollide in the registry — account A's claim shadows account B's distinct conversation, so an agent in workspace B can be silently suppressedBridgeManager::ensure_thread_ownership_sweep_startedruns a 60stokio::time::intervalthat callssweep_expired, armed lazily on the firststart_adapter(so the sync constructor stays runtime-free for unit tests), tied toshutdown_rxthread_tsaccumulates stale claims until restart.decideonly evicts on access, never proactivelypush_messagedoc-comment explains the intentional registry bypass/push, cron, workflow output,agent_send) is agent-initiated; the caller has already chosen who is sending. Gating it would break legitimate cross-agent flows. Documents the asymmetry and points torespect_thread_ownershipas the future opt-in if real users hit the cross-cutting caseTest plan
cargo clippy -p librefang-channels --all-targets -- -D warningsclean on rebased branchcargo test -p librefang-channels --libthread-ownership suite — 18 passed (3 new —distinct_accounts_do_not_collide_on_same_thread_id,empty_account_id_normalized_to_none,multi_tenant_account_id_flows_into_thread_key)main(7518f79e)Why a separate PR
The original
feat/3334-thread-ownershipbranch was already merged when the review feedback was addressed; pushing more commits to that branch wouldn't reachmain. A fresh PR from a clean branch is the cleanest path. Same review reply still stands — see the comment thread on #3414 for the design context.Rollout
Behavior change for existing users: thread-ownership now correctly distinguishes workspaces, so multi-tenant deployments may see different (correct) suppression patterns than they did between the original merge and this fix. Single-tenant deployments are byte-identical (empty
account_idnormalizes toNone, same key shape as before).