feat(discord): smart auto-thread mode (true/false/smart)#1054
feat(discord): smart auto-thread mode (true/false/smart)#1054Hypn0sis wants to merge 2 commits intoRightNow-AI:mainfrom
Conversation
jaberjaber23
left a comment
There was a problem hiding this comment.
Thanks for the Discord work @Hypn0sis. The auto-thread feature (true / false / smart) is useful and the core logic is reasonable, but this PR has a few issues that block merge.
Blockers
threaded_message_ids.write().await.retain(|_| true)does nothing. In theTHREAD_DELETE/CHANNEL_DELETEhandlers you're callingretain(|_| true), which keeps every element — the comment says "prune message IDs for the deleted thread" but the closure always returnstrue. The dedupHashSet<MessageId>is therefore never pruned, and on a busy server it grows without bound. Either track aHashMap<ThreadId, HashSet<MessageId>>and drop the entry on thread delete, or put a size/time cap on the set.- Scope creep — ~500+ lines of unrelated deploy tooling.
deploy-remote.sh,sync-upstream-and-deploy.sh,.gitignore/CLAUDE.mdchanges are not about Discord smart-thread behavior. Please split them into a separate PR so this one can be reviewed on its own merits. - CI — Security Audit: FAILURE. Your PR body points to #1041 for the
rustls-webpkifix; now that #1041 is merged, please rebase and let CI re-run. Security Audit must be green before merge.
Concerns
- In
"true"mode the bot will create a thread per allowed message in group channels. Confirm this stays within Discord's rate limits on a hot channel (observed behavior at ~5–10 msg/s from multiple users). #[allow(dead_code)]on the test helper — either use it or remove it.regex_liteusage — confirm it's already a dependency and doesn't pull in a new transitive set.
Recommendation
Split the PR. Fix the retain(|_| true) bug and bound the dedup set. Rebase post-#1041. Re-request review and we'll land the Discord half quickly.
06fbc15 to
0ab8076
Compare
|
CI update after force-push Clippy is now fixed — 5 warnings in Regarding Security Audit: the two remaining advisories are pre-existing in
Both were present on |
- RwLock: replace Arc<Mutex<String>> with Arc<RwLock<String>> for
A2A_TASK_PROGRESS — eliminates exclusive lock in read-heavy path
- SSE parsing: extract pure parse_sse_data_line fn + SseLineOutcome enum;
refactor both send_task_streaming and send_task_streaming_with_progress
to use it; add timeout: Option<Duration> to both; 8 unit tests covering
all outcomes (final, update, empty, malformed, error, unknown)
- Bounded maps: add MAX_CONCURRENT_ASYNC_TASKS=256 cap with clear error;
AsyncTaskEntry{handle,inserted_at} tracks task age; OnceLock-based
background sweep (10 min interval, 2 h TTL) aborts stale handles;
TaskCleanupGuard RAII ensures cleanup on panic
- Prompt injection: rewrite inject_async_callback to deliver async results
via structural ToolUse+ToolResult pair instead of text framing; remote
agent content lands in a ToolResult block where LLM API semantics enforce
the data boundary; add prepend_turns: Option<Vec<Message>> to both
run_agent_loop and run_agent_loop_streaming so the synthetic ToolUse is
inserted AFTER validate_and_repair (prevents orphan removal of the
ToolResult user turn)
- Strip @default suffix from agent IDs in tool_agent_send (pre-existing fix)
- Update test to correctly verify prune_failed_tool_turns behavior
SSRF (RightNow-AI#1060): already using canonical crate::web_fetch::check_ssrf
Thread context (RightNow-AI#1054): context.thread_id passed through; will work
correctly once smart-thread sets it
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t mod - Replace retain(|_| true) no-op with a size-capped clear: when threaded_message_ids exceeds MAX_DEDUP_MSG_IDS (2000) it is cleared. MESSAGE_UPDATE embed events arrive within seconds so old entries are always safe to discard; prevents unbounded growth on busy servers. - Add #[cfg(test)] to mod tests so empty_threads() helper is only compiled in test mode — removes the need for #[allow(dead_code)].
8c4c78e to
0227ff1
Compare
|
CI update after rebase on v0.6.0 Rebased the branch onto current `main` (v0.6.0, commit `e6bab99`) as requested. The PR now contains only the two discord-relevant commits:
All unrelated commits (deploy tooling, Clippy fixes for other crates) have been dropped — the scope creep concern is resolved. On the remaining CI failures: All three failing jobs are pre-existing regressions on `main` itself — not introduced by this PR. You can verify by checking the latest CI run on `main` directly: run 24637821654 shows the same Clippy, Format, and Security Audit failures on `main` with no PR involved.
Happy to include a fix-commit for the Clippy and Format regressions if that would help unblock the merge, but wanted to flag them as upstream issues first rather than silently expand scope again. |
Summary
auto_threadconfig field toDiscordConfigwith three modes:"true"— always create a thread from each message"false"— never create a thread (default, opt-in)"smart"— create a thread only when the bot is @mentionedMESSAGE_UPDATEevents (Discord embed resolution) are deduplicated against already-forwardedMESSAGE_CREATEevents to prevent double responsesapi_send_thread_messageto POST directly tochannels/{thread_id}/messages(the correct Discord API endpoint for threads)RevoltAdapter::with_urls_and_channelsconstructor to support custom URLs + channel restrictions togetherConfig example
CI note
The Security Audit check may flag
rustls-webpki 0.102— that CVE is addressed in #1041 (already open). If that PR merges first, a rebase of this branch will clear the audit cleanly. Thefix: clippycommit in this PR may also overlap with #1041's fixes tocopilot.rs; a rebase will resolve any conflict trivially.Test plan
cargo check --workspace --libpassescargo clippy --workspace --all-targets -- -D warningspassescargo test --workspacepassesauto_thread = "smart", @mention bot in server channel → thread createdauto_thread = "smart", message without @mention → no thread createdauto_thread = "false"→ no threads ever created