Skip to content

fix(channels): account_id in ThreadKey + lazy sweep + push bypass note#3420

Closed
houko wants to merge 1 commit into
mainfrom
fix/3414-account-id-and-sweep
Closed

fix(channels): account_id in ThreadKey + lazy sweep + push bypass note#3420
houko wants to merge 1 commit into
mainfrom
fix/3414-account-id-and-sweep

Conversation

@houko

@houko houko commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up to merged #3414. Picks up the review-feedback fixes that were pushed to feat/3334-thread-ownership after that branch was merged at e3b1b847, so they never made it onto main. Cherry-picks one commit (bb81cd1e here) onto a fresh branch from main.

The fixes target real bugs in the merged thread_ownership module:

Fix Why it matters
ThreadKey gains account_id: Option<String> and the bridge dispatch sites read message.metadata["account_id"] Without this, two Slack workspaces (or two Discord guilds) producing the same thread_ts collide in the registry — account A's claim shadows account B's distinct conversation, so an agent in workspace B can be silently suppressed
BridgeManager::ensure_thread_ownership_sweep_started runs a 60s tokio::time::interval that calls sweep_expired, armed lazily on the first start_adapter (so the sync constructor stays runtime-free for unit tests), tied to shutdown_rx Without this, the registry grows monotonically — a daemon in a Slack workspace with churning thread_ts accumulates stale claims until restart. decide only evicts on access, never proactively
push_message doc-comment explains the intentional registry bypass Outbound push (REST /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 to respect_thread_ownership as the future opt-in if real users hit the cross-cutting case

Test plan

  • cargo clippy -p librefang-channels --all-targets -- -D warnings clean on rebased branch
  • cargo test -p librefang-channels --lib thread-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)
  • Cherry-pick from a clean cut of the post-merge branch tip; no conflicts against current main (7518f79e)

Why a separate PR

The original feat/3334-thread-ownership branch was already merged when the review feedback was addressed; pushing more commits to that branch wouldn't reach main. 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_id normalizes to None, same key shape as before).

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.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added the size/M 50-249 lines changed label Apr 27, 2026
@github-actions github-actions Bot added the area/channels Messaging channel adapters label Apr 27, 2026
@houko

houko commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #3419 — same fix area but #3419 has the polished version (CHANGELOG entry, sharper sweep loop comments, and set_missed_tick_behavior(Delay) so a stalled task doesn't fire several sweeps back-to-back on resume). Closing this one to avoid the duplicate.

@houko houko closed this Apr 27, 2026
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/channels Messaging channel adapters size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant