Skip to content

feat(channels): thread ownership prevents multi-agent duplicate replies#3414

Merged
houko merged 2 commits into
mainfrom
feat/3334-thread-ownership
Apr 27, 2026
Merged

feat(channels): thread ownership prevents multi-agent duplicate replies#3414
houko merged 2 commits into
mainfrom
feat/3334-thread-ownership

Conversation

@houko

@houko houko commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Closes #3334.

Summary

When two or more LibreFang agents are bound to the same channel (one Slack workspace with both a "support" agent and a "research" agent, etc.), the router can resolve different agents on consecutive messages in the same thread — last @-mention wins, sticky-TTL falls off, etc. — and both agents end up replying, contradicting each other and running up cost.

This PR adds a single-process claim registry so each (channel, thread) is owned by one agent for the configured TTL. An explicit @-mention re-claims for the new agent. DMs and untreaded messages bypass entirely.

Design at a glance

                        is_group?            no  ────► dispatch
                            │
                           yes
                            ▼
                    thread_id present?       no  ────► dispatch
                            │
                           yes
                            ▼
              ChannelOverrides.thread_       no  ────► dispatch
              ownership_enabled?
                            │
                           yes
                            ▼
            registry.decide(key, candidate, was_mentioned)
              │                          │
        Allow │                  Suppress│
              ▼                          ▼
          dispatch                  drop, log debug!

registry.decide cases:

  1. No claim or expired claim → fresh-claim for candidate, Allow.
  2. Existing claim, candidate is the holder → extend (refresh claimed_at), Allow.
  3. Existing claim, different agent, was_mentioned = true → re-claim for candidate, Allow.
  4. Existing claim, different agent, was_mentioned = falseSuppress (carries the holder for the log line).

TTL defaults to 5 min; configurable per-registry. Channel-level escape hatch: [channels.X.overrides] thread_ownership_enabled = false makes the bridge skip the registry — useful for "broadcast" channels where multiple agents should chime in together.

Files

Change Why
librefang-channels/src/thread_ownership.rs (new) ThreadKey, Claim, ThreadOwnershipRegistry, DispatchDecision + 9 unit tests
librefang-types/src/config/types.rs ChannelOverrides::thread_ownership_enabled: bool = true
librefang-channels/src/bridge.rs BridgeManager owns Arc<ThreadOwnershipRegistry>; threaded through start_adapterflush_debounceddispatch_message / dispatch_with_blocks. Both consult the registry after resolve_or_fallback. 6 new bridge-side unit tests cover DM bypass / untreaded bypass / first claim / second-agent suppress / @-mention override / override-disabled bypass

Test plan

  • cargo check --workspace --all-targets clean
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo test -p librefang-channels --lib 835 passed (9 new in thread_ownership::tests, 6 new in bridge::tests)
  • cargo test -p librefang-types --lib 702 passed
  • Hand-verify on a live two-agent Slack deployment — left for reviewer if needed; logic is fully unit-covered.

Out of scope (per the issue)

  • Multi-process / multi-daemon coordination (slack-forwarder shared store). Single-process v1 only — tracked as a follow-up if a user reports duplicate replies across daemons. Memory stays bounded by the per-thread TTL plus an opt-in sweep_expired() call, no persistence layer needed.
  • Metric emission beyond the existing debug! log line. Add Prometheus counters in a follow-up if operator visibility becomes a real ask.

houko added 2 commits April 27, 2026 22:24
Migrate every project-defined animation from CSS keyframes / RAF /
tailwindcss-animate to a single motion-driven system, and add the
animation gaps that used to be missing (route transitions, tab swap,
list add/remove).

What moved
- 6 custom @Keyframes (fade-in-up / fade-in-scale / slide-in-right /
  message-in / shimmer / progress-fill) and the .stagger-children
  CSS cascade are deleted; replaced by `src/lib/motion.ts` variants.
- Modal/弹窗 now use <AnimatePresence>, so closing plays the same
  scale/blur or slide-out animation as opening (drawer/panel/modal
  variants all unified).
- AnimatedNumber and Typewriter_v2 swap hand-rolled RAF loops for
  motion's animate() — same API, same look, but on motion's
  scheduler, with automatic prefers-reduced-motion handling.
- Toast loses tailwindcss-animate's `animate-in slide-in-from-right-5`
  in favour of motion + AnimatePresence + layout, so dismissed
  toasts actually exit and the stack reshuffles smoothly.

What was added (new behaviour, not a rename)
- Page transitions: <AnimatePresence mode="wait"> wraps the router
  Outlet keyed on pathname so navigating between pages fades and
  slides instead of snapping.
- Tab content: 7 surfaces (MediaPage / AgentsPage / HandsPage /
  PluginsPage / McpServersPage / ProvidersPage / TomlViewer) now
  cross-fade tab bodies on activeTab change.
- List add/remove: <StaggerList> internally wraps each child in
  <AnimatePresence layout> with an exit variant, so 21 use sites
  (Agents, Sessions, Plugins, Hands, Telemetry, …) animate item
  removal and reflow neighbours smoothly. Behaviour is opt-in
  via stable keys — existing call sites already pass `key={item.id}`.

What was deliberately not migrated
- Tailwind utilities (`animate-spin` 90×, `animate-pulse` 32×,
  `animate-bounce`/`ping` 3×) are industry-standard one-liners;
  motion-ising them would balloon ~125 sites with zero visual
  benefit.
- `transition-colors` / `transition-all` (418 sites) are CSS
  hover/focus transitions, not keyframe animations.
- TerminalPage's RAF calls are `fitAddon` next-frame scheduling,
  not animation.

New shared building blocks
- `src/lib/motion.ts` — APPLE_EASE / APPLE_SPRING / APPLE_BOUNCE
  curves and `fadeInScale` / `fadeInUp` / `messageIn` /
  `slideInRight` / `staggerContainer` / `staggerItem` /
  `pageTransition` / `tabContent` / `toastSlide` variants.
- `src/components/ui/StaggerList.tsx` — drop-in replacement for the
  legacy .stagger-children className with built-in exit + layout.

Verified: pnpm typecheck, pnpm build, pnpm test (337/340 — the 3
TerminalPage failures are pre-existing on main).
Closes #3334.

When two or more LibreFang agents are bound to the same channel (one Slack
workspace with both a "support" agent and a "research" agent, etc.) the
router can resolve different agents on consecutive messages in the same
thread — last @-mention wins, sticky-TTL falls off, etc. — and both agents
end up replying. Add a single-process claim registry so each `(channel,
thread)` is owned by one agent for the configured TTL; an explicit
@-mention re-claims for the new agent.

Changes:

- `librefang-channels/src/thread_ownership.rs` (new)
  - `ThreadKey { channel, thread }` — hashable identity. Empty channel or
    thread rejected at construction.
  - `Claim` — agent_id + monotonic `Instant` claimed_at + per-claim TTL.
  - `ThreadOwnershipRegistry` — `DashMap` of claims, `decide(key,
    candidate, was_mentioned)` returns `Allow` (claim or extend) or
    `Suppress { holder }`. `decide_at` / `sweep_expired_at` are test
    seams accepting an explicit `Instant`.
  - 9 unit tests cover empty-key reject, first-claim, suppression,
    @-mention override, in-place extend, expired-claim takeover, sweep
    selectivity, ttl-zero clamp, distinct-thread isolation.

- `librefang-types/src/config/types.rs`
  - `ChannelOverrides::thread_ownership_enabled: bool` defaults to
    `true`. Setting to `false` makes the bridge skip the registry — the
    "broadcast" channel escape hatch.

- `librefang-channels/src/bridge.rs`
  - `BridgeManager` owns an `Arc<ThreadOwnershipRegistry>` and threads it
    through `start_adapter` → `flush_debounced` → `dispatch_message` →
    `dispatch_with_blocks`. Both dispatch paths consult the registry
    after `resolve_or_fallback` and before any side effect, suppressing
    with a `debug!` log line on conflict. DMs (`!is_group`) and
    untreaded messages bypass entirely.
  - 6 new unit tests cover DM bypass, untreaded-group bypass, first
    claim, second-agent suppress, @-mention override, override-disabled
    bypass — all using a small bridge-side helper that mirrors the same
    metadata reads the dispatch path performs.

Pre-flight: `cargo check --workspace --all-targets` clean,
`cargo clippy --workspace --all-targets -- -D warnings` clean,
`cargo test -p librefang-channels --lib` 835 / `librefang-types --lib`
702 — both green.

Out of scope (per the issue): multi-process / multi-daemon coordination
(slack-forwarder shared store). Tracked as a follow-up if a user reports
duplicate replies across daemons. The in-memory registry is bounded by
the per-thread TTL plus an opt-in `sweep_expired` call, so memory stays
under control without a persistence layer.
@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.

@houko
houko merged commit 6761951 into main Apr 27, 2026
15 checks passed
@houko
houko deleted the feat/3334-thread-ownership branch April 27, 2026 13:52
@github-actions github-actions Bot added area/channels Messaging channel adapters size/XL 1000+ lines changed labels Apr 27, 2026
@houko

houko commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

Internal multi-angle review (quality + security + missed-channel-paths) found 2 must-fix items, 1 quality bug, 1 explore bypass to document, plus 2 design-intent items to call out. All addressed in 34c4415.

Finding Source Severity Resolution
ThreadKey was (channel, thread) only — two Slack workspaces with the same thread_ts collided security FIX Added account_id: Option<String> to ThreadKey. Constructor takes (channel, account_id, thread). Bridge dispatch sites now read message.metadata["account_id"] and pass it through. New tests distinct_accounts_do_not_collide_on_same_thread_id and multi_tenant_account_id_flows_into_thread_key
sweep_expired had no caller → registry grew monotonically until daemon restart quality + security BUG / FIX New BridgeManager::ensure_thread_ownership_sweep_started, armed lazily on first start_adapter call (so the sync constructor stays runtime-free for unit tests), runs a 60s tokio::time::interval tied to shutdown_rx. Hard cap left as follow-up — rare case, would benefit from real numbers
push_message (REST /push, cron, workflow output, agent_send) bypasses the registry Explore BYPASS-OK Documented inline: push is the agent-initiated outbound path, the caller has already chosen who is sending — no routing decision to gate. Gating it would break legitimate cross-agent flows. Follow-up path: opt-in respect_thread_ownership parameter if user-visible pain shows up
6 bridge tests use a hand-rolled helper rather than driving real dispatch_message — order-of-operations between gate, RBAC, journal, auto-reply unverified quality NIT (debt) Acknowledged in the helper's doc-comment; the helper still catches metadata-read + key-build regressions, which is where the real risk lives. Real-dispatch test (stub ChannelBridgeHandle + recording adapter) is captured as follow-up debt — would require expanding the existing MockHandle substantially

Design-intent items called out (no code change)

  • Mention spoofing → claim re-route. Anyone in a group thread can @-mention an agent and re-claim the thread for that agent. Adapter-side validation prevents text-only forgery (Slack requires app_mention event, Discord matches bot user ID, Telegram entity-validates), but a real platform user can still redirect traffic from agent A to agent B. This matches the issue spec — "@-mention re-claims" is design intent, not a vulnerability. Documented in PR description.
  • Restart drops claims. Single-process MVP, no persistence. After daemon restart, the first message in a previously-claimed thread is a fresh-claim opportunity for whichever agent the resolver picks. Single-process scope per the issue; persistence + multi-daemon coordination is the explicit follow-up if real users hit it.

Test plan re-run on 34c4415

  • 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)
  • Sweep task is Drop-clean (terminates on shutdown_rx.changed())
  • ThreadKey API change: ThreadKey::new now takes (channel, account_id, thread). No external consumers — only the bridge dispatch sites use it, and both are updated.

The fixup commit kept the in-memory single-process scope intact (no persistence layer added) — multi-daemon coordination remains explicit out-of-scope per the issue, with the added safety that account_id-keyed claims now correctly distinguish workspaces within the same daemon.

houko added a commit that referenced this pull request Apr 27, 2026
…ship field (#3417)

PR #3414 added `thread_ownership_enabled: bool` to ChannelConfig but
didn't regenerate the golden fixture, so config_schema_golden has
been failing on every PR since (Ubuntu / Windows / macOS). The
diff is purely additive — new field with default `true` — no
existing structure changed.

Regenerated via:
  cargo test -p librefang-api --test config_schema_golden \
    -- --ignored regenerate_golden --nocapture
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/XL 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread ownership: prevent multi-agent duplicate replies in shared channels

1 participant