feat: /close slash command to manually terminate a thread session#42
feat: /close slash command to manually terminate a thread session#42qijie850 wants to merge 4 commits intoopenabdev:mainfrom
Conversation
Each Discord thread now gets its own working directory under <working_dir>/sessions/<thread_id>/, preventing file conflicts when multiple threads run concurrently. Directories are automatically cleaned up when sessions are reaped by cleanup_idle. Closes openabdev#38 Co-authored-by: wsxqaza12 <wsxqaza12@users.noreply.github.com>
thepagent
left a comment
There was a problem hiding this comment.
Good work using Discord slash commands — exactly the approach we discussed in #40. A few items:
Must fix:
-
Overlapping diff with #41 — This PR includes the same
pool.rsper-thread dir changes from #41. Please rebase onto #41's branch (or wait for #41 to merge, then rebase). Otherwise we'll get merge conflicts. -
Carry over #41 review fixes — The
pool.rschanges here have the same issues flagged in #41 review:- Path traversal hardening (
thread_idvalidation) shutdown()directory cleanup- sync
exists()→ usetokio::fsor just handleNotFound - Extract
thread_dir()helper (now duplicated 3 times)
- Path traversal hardening (
-
Make response ephemeral —
/closeresponse is visible to everyone. Add.ephemeral(true)so only the caller sees it:
CreateInteractionResponseMessage::new()
.content(response_content)
.ephemeral(true)- Global command registration on every restart —
create_global_commandinreadyruns on every bot restart. Global commands have rate limits and take up to 1 hour to propagate. Consider:- Use guild commands instead (instant propagation, registered per-server)
- Or gate with a one-time check
Looks good otherwise:
- Lock scope in
close_sessionis well done (release write lock before async fs cleanup) - Thread detection via
thread_metadata.is_some()is correct - Clean separation between slash command handling and text message flow
Suggested merge order: land #41 first → rebase this PR → address feedback → merge.
- Add thread_id path traversal validation (ASCII digits only) - Clean up session directories in shutdown() - Replace sync exists() check with async NotFound handling - Extract thread_dir() helper to deduplicate path construction Co-authored-by: wsxqaza12 <wsxqaza12@users.noreply.github.com>
…, add sessions/ to .gitignore Address review feedback from neilkuan (openabdev#41 comment): - cleanup_idle/shutdown: collect paths and release write lock before async fs operations so concurrent get_or_create() calls aren't blocked - get_or_create: clear old session directory when rebuilding a stale connection so the replacement agent starts clean - Add sessions/ to .gitignore to prevent accidental staging Co-authored-by: wsxqaza12 <wsxqaza12@users.noreply.github.com>
|
Thanks for the review! Agreed on all points. Here's the plan: Merge order: Will wait for #41 to land first, then rebase this PR on top — that automatically resolves #1 and #2 (the overlapping #3 — Ephemeral response: Will add #4 — Guild commands: Good call on the rate limit issue. Will switch to guild commands ( Will push the fixes once #41 is merged and the rebase is clean! |
Adds a Discord slash command /close that lets users manually close a coding session inside a thread: - Kills the ACP child process (frees RAM) - Removes the per-thread working directory - Responds with an ephemeral confirmation Implementation notes: - Uses guild commands (instant propagation) instead of global commands - Response is ephemeral (only visible to the caller) - close_session releases the write lock before async I/O - Text messages with /close pass through to the agent untouched Closes openabdev#40 Co-authored-by: wsxqaza12 <wsxqaza12@users.noreply.github.com>
ca2a736 to
735ad8c
Compare
|
Rebased onto #41 (including all review fixes) and addressed all feedback: Changes:
|
Summary
Adds a Discord slash command
/closethat lets users manually terminate a coding session inside a thread.Closes #40
Depends on #41 (per-thread working directories)
Changes
src/discord.rs:/closeas a global slash command onreadyInteractionCreateevents for the/closecommandsrc/acp/pool.rs:close_session(thread_id)method that kills the ACP process and removes the per-thread working directoryDesign Decision: Slash Command vs Text Matching
Per @thepagent's feedback on #40, this uses Discord slash commands (
InteractionCreateevent) instead of text matching to avoid shadowing agent-side/closecommands:Zero collision between broker commands and agent commands.
Behavior
/closein a thread → kills session + cleans workdir → "\u2705 Session closed."/closewith no active session → "\u2139\ufe0f No active session in this thread."/closeoutside a thread → "\u26a0\ufe0f Only works inside a thread."Testing
Compiled and verified against serenity 0.12 on EC2 (Ubuntu 24.04). Slash command registration and interaction handling tested locally.