feat(channels): thread ownership prevents multi-agent duplicate replies#3414
Merged
Conversation
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Design-intent items called out (no code change)
Test plan re-run on 34c4415
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. |
2 tasks
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
This was referenced Apr 27, 2026
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.
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
registry.decidecases:candidate, Allow.claimed_at), Allow.was_mentioned = true→ re-claim forcandidate, Allow.was_mentioned = false→ Suppress (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 = falsemakes the bridge skip the registry — useful for "broadcast" channels where multiple agents should chime in together.Files
librefang-channels/src/thread_ownership.rs(new)ThreadKey,Claim,ThreadOwnershipRegistry,DispatchDecision+ 9 unit testslibrefang-types/src/config/types.rsChannelOverrides::thread_ownership_enabled: bool = truelibrefang-channels/src/bridge.rsBridgeManagerownsArc<ThreadOwnershipRegistry>; threaded throughstart_adapter→flush_debounced→dispatch_message/dispatch_with_blocks. Both consult the registry afterresolve_or_fallback. 6 new bridge-side unit tests cover DM bypass / untreaded bypass / first claim / second-agent suppress / @-mention override / override-disabled bypassTest plan
cargo check --workspace --all-targetscleancargo clippy --workspace --all-targets -- -D warningscleancargo test -p librefang-channels --lib835 passed (9 new inthread_ownership::tests, 6 new inbridge::tests)cargo test -p librefang-types --lib702 passedOut of scope (per the issue)
sweep_expired()call, no persistence layer needed.debug!log line. Add Prometheus counters in a follow-up if operator visibility becomes a real ask.