Conversation
…debar display - Add SubtaskHandle<T> with spawn_subagent, spawn_task, spawn_background functions - Integrate with ThreadManager for automatic lifecycle tracking - Support CancellationToken-based graceful cancellation - Add SubtaskRegistry for tracking multiple concurrent subtasks - Enhance ThreadInfoDto/ThreadInfo with kind_icon and status_icon fields - Update send_threads_update() to populate icons for all thread sources - Update sidebar to display descriptions and use structured status icons - Add /thread bg and /thread fg commands for backgrounding/foregrounding - Add thread commands to /help output - Fix sessions.rs test that was outside mod tests block - Wire client-side mapping of new icon fields through gateway_client Co-Authored-By: Erica Stith <rexlunae@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…to gateway - Use chars().take(N) instead of byte slicing for label/description truncation to prevent panics on multi-byte UTF-8 characters - ThreadBackground now sends ThreadSwitch with thread_id=0 to gateway instead of being a no-op Co-Authored-By: Erica Stith <rexlunae@gmail.com>
- Add clear_foreground() method to ThreadManager - Gateway ThreadSwitch handler now treats thread_id=0 as 'background all' instead of trying to look up a nonexistent thread Co-Authored-By: Erica Stith <rexlunae@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Erica Stith <rexlunae@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ive cancellation only Co-Authored-By: Erica Stith <rexlunae@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ing threads Co-Authored-By: Erica Stith <rexlunae@gmail.com>
switch_to returns the old foreground ID (Option<ThreadId>), which is None after /thread bg clears the foreground. The gateway checked .is_some() on this return value, causing /thread fg to always report 'Thread not found' after backgrounding. switch_foreground returns bool, correctly indicating whether the switch succeeded. Co-Authored-By: Erica Stith <rexlunae@gmail.com>
| pub fn clear_foreground(&mut self) { | ||
| if let Some(old_id) = self.foreground_id { | ||
| if let Some(t) = self.threads.get_mut(&old_id) { | ||
| t.is_foreground = false; | ||
| } | ||
| } | ||
| self.foreground_id = None; | ||
| } |
There was a problem hiding this comment.
🟡 clear_foreground() does not emit a thread event, breaking event-driven sidebar updates
Every other state-changing method in ThreadManager emits an event — switch_foreground emits Foregrounded (manager.rs:256), remove emits Removed (manager.rs:374), set_status emits StatusChanged (manager.rs:226), etc. The new clear_foreground() modifies foreground_id and the thread's is_foreground flag but does not emit any event. The gateway's event-driven sidebar update loop at crates/rustyclaw-core/src/gateway/mod.rs:1555-1561 listens for these events to push updates. While the specific call site at mod.rs:1346 manually sends updates after calling clear_foreground(), this breaks the event system contract — any future caller of clear_foreground() will silently fail to notify subscribers.
| pub fn clear_foreground(&mut self) { | |
| if let Some(old_id) = self.foreground_id { | |
| if let Some(t) = self.threads.get_mut(&old_id) { | |
| t.is_foreground = false; | |
| } | |
| } | |
| self.foreground_id = None; | |
| } | |
| pub fn clear_foreground(&mut self) { | |
| let old_id = self.foreground_id; | |
| if let Some(old) = old_id { | |
| if let Some(t) = self.threads.get_mut(&old) { | |
| t.is_foreground = false; | |
| } | |
| } | |
| self.foreground_id = None; | |
| if old_id.is_some() { | |
| self.emit(ThreadEvent::Foregrounded { | |
| thread_id: ThreadId(0), | |
| previous_foreground: old_id, | |
| }); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this is a valid consistency concern. The current call site in the gateway (mod.rs:1346) manually sends send_threads_update after clear_foreground(), so the sidebar does update correctly today. Adding the event emission would be a defensive improvement for future callers. Leaving this as-is for now since the current behavior is correct; happy to add the event if the reviewer wants it.
The info message was sent unconditionally, even when the gateway was disconnected (ws_sink is None). Move it inside the if-let block so the user only sees 'Current thread backgrounded...' when the frame was actually sent. This matches the pattern used by other thread commands. Co-Authored-By: Erica Stith <rexlunae@gmail.com>
There was a problem hiding this comment.
Pull request overview
Implements a unified subtask/subagent abstraction in rustyclaw-core and enhances the gateway → TUI thread metadata flow so the sidebar can render richer status/description information, plus adds /thread bg and /thread fg commands for foreground management.
Changes:
- Added
SubtaskHandle<T>+ spawn helpers andSubtaskRegistryintegrated withThreadManager. - Extended thread protocol DTOs with
kind_icon/status_iconand updated gateway + TUI mapping/rendering. - Added background/foreground thread commands and a
ThreadManager::clear_foreground()API; fixed a misplaced unit test block.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rustyclaw-tui/src/gateway_client.rs | Maps new kind_icon / status_icon fields from gateway thread DTOs into TUI ThreadInfo. |
| crates/rustyclaw-tui/src/components/sidebar.rs | Renders structured status_icon and adds a truncated description line in the sidebar. |
| crates/rustyclaw-tui/src/app/app.rs | Sends ThreadSwitch frames for /thread bg (sentinel 0) and /thread fg <id>. |
| crates/rustyclaw-tui/src/action.rs | Extends TUI ThreadInfo model with icon fields. |
| crates/rustyclaw-core/src/threads/subtask.rs | New subtask abstraction: spawn/join/cancel integrated with ThreadManager, plus unit tests. |
| crates/rustyclaw-core/src/threads/mod.rs | Exposes the new subtask module/types from the threads module. |
| crates/rustyclaw-core/src/threads/manager.rs | Adds clear_foreground() to support backgrounding all threads. |
| crates/rustyclaw-core/src/sessions.rs | Fixes test placement by moving test_subagent_appears_in_active_list back inside mod tests. |
| crates/rustyclaw-core/src/gateway/protocol/frames.rs | Extends ThreadInfoDto with icon fields and updates frame-size tests. |
| crates/rustyclaw-core/src/gateway/mod.rs | Populates new icon fields and implements thread_id == 0 sentinel handling + switch_foreground() usage. |
| crates/rustyclaw-core/src/commands.rs | Adds /thread bg and /thread fg parsing, help text, and autocomplete names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /// Icon for the thread kind (e.g. chat, sub-agent, background, task) | ||
| pub kind_icon: Option<String>, | ||
| /// Icon for the thread status (e.g. running, completed, failed) | ||
| pub status_icon: Option<String>, | ||
| pub is_foreground: bool, | ||
| pub message_count: usize, | ||
| pub has_summary: bool, |
| /// The underlying tokio JoinHandle (for abort). | ||
| join_handle: Option<tokio::task::JoinHandle<()>>, | ||
| } |
| /// Wait for the subtask to complete and return its result. | ||
| /// | ||
| /// This consumes the handle. After joining, the thread status is updated | ||
| /// to Completed or Failed. | ||
| pub async fn join(mut self) -> Result<T, String> { | ||
| let rx = self | ||
| .result_rx | ||
| .take() | ||
| .ok_or_else(|| "SubtaskHandle already joined".to_string())?; | ||
|
|
||
| match rx.await { | ||
| Ok(Ok(value)) => Ok(value), | ||
| Ok(Err(e)) => Err(e), | ||
| Err(_) => { | ||
| // Sender dropped — task panicked or was aborted | ||
| Err("Subtask channel closed unexpectedly".to_string()) | ||
| } | ||
| } |
| // Give tokio a moment to process the status update | ||
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | ||
|
|
The oxidized-skills crate was removed/yanked from crates.io, causing all CI checks to fail with 'no matching package named oxidized-skills found' when the cargo cache expires. Switch to the git repository source at tag v0.3.1 which matches the previously-used version. Co-Authored-By: Erica Stith <rexlunae@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Erica Stith <rexlunae@gmail.com>
…ad bg switch_to() returns the old foreground ID, which is None after /thread bg. This caused the .is_some() check to fail, skipping the ThreadSwitched notification and permanently desyncing the client. switch_foreground() returns bool indicating success, which is the correct check here. Co-Authored-By: Erica Stith <rexlunae@gmail.com>
There was a problem hiding this comment.
🟡 TUI ThreadSwitched handler does not distinguish background (thread_id=0) from a real thread switch, clearing messages and showing confusing text
When the user issues /thread bg, the gateway sends back ThreadSwitched { thread_id: 0, context_summary: None }. The TUI handler at crates/rustyclaw-tui/src/app/app.rs:1663-1683 unconditionally clears all display messages and replaces them with "Switched to thread (id: 0)", which is a confusing sentinel value the user shouldn't see. This also overwrites the earlier helpful info message sent at crates/rustyclaw-tui/src/app/app.rs:611 ("Current thread backgrounded. Use /thread fg or sidebar to switch."). The handler should check for thread_id == 0 and either skip message clearing or display an appropriate "No foreground thread" message.
(Refers to lines 1663-1683)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a theoretical concern but doesn't actually happen in the /thread bg flow. The gateway's clear_foreground() handler (lines 1415-1432 in gateway/mod.rs) only sends a ThreadsUpdate frame back — it never sends a ThreadSwitched frame with thread_id=0. The ThreadSwitched event is only emitted by explicit ThreadSwitch requests (line 1425) and auto-switch (line 1206), both of which use real thread IDs via switch_foreground().
The only way ThreadSwitched { thread_id: 0 } could reach the TUI is if someone explicitly sent ThreadSwitch { thread_id: 0 } back as a server frame, which the gateway doesn't do. That said, adding a guard for thread_id == 0 in the TUI handler would be a reasonable defensive improvement for the future — will note it as a follow-up.
The test comment correctly noted 'Still blocked due to error_count > 0' but the assertion was negated (!should_block). With error_count=2, should_block() always returns true regardless of score threshold. Co-Authored-By: Erica Stith <rexlunae@gmail.com>
feat: unified subtask/subagent abstraction with enhanced sidebar
Summary
Adds a unified subtask abstraction (
SubtaskHandle<T>) for spawning concurrent work (subagents, background tasks, one-shot tasks) as async tokio tasks integrated withThreadManager. Enhances the protocol and sidebar to display richer thread metadata (status icons, descriptions).Core abstraction (
threads/subtask.rs— new file, ~626 lines):SubtaskHandle<T>— generic joinable handle with cancellation viaCancellationToken, status/description updatesspawn_subagent,spawn_task,spawn_background— factory functions that create a ThreadManager entry, spawn a tokio task, and wire up lifecycle updatesSubtaskRegistry— tracks multiple concurrent subtasks for bulk cancel/listcancel()signals viaCancellationToken; the spawned task is expected to check the token and clean up its own ThreadManager statusProtocol changes (
ThreadInfoDto):kind_icon: Option<String>andstatus_icon: Option<String>fieldsSidebar enhancements:
status_iconfrom gateway when available, falls back to string matchingchars().take(N))Commands:
/thread bg— background the current thread (sendsThreadSwitch { thread_id: 0 }sentinel to gateway)/thread fg <id>— foreground a thread by ID (sendsThreadSwitch); rejectsid=0since it collides with the background sentinel/helpoutput and command autocompleteThread management:
ThreadManager::clear_foreground()method to support backgrounding all threadsThreadSwitchhandler recognizesthread_id == 0as sentinel for "clear foreground"switch_foreground()(returnsbool) instead ofswitch_to()(returnsOption<ThreadId>) for all switch operations — both explicit thread switches and auto-switchBugfixes:
test_subagent_appears_in_active_listinsidemod testsblock insessions.rstest_should_blockassertion inskill_audit.rs(pre-existing on main — assertion was negated, contradicting its own comment abouterror_count > 0always blocking)Updates since last revision
sidebar.rsnow useschars().take(N).collect()instead of byte-index slicing, preventing panics on multi-byte characters./thread bgend-to-end:ThreadBackgroundhandler now sendsThreadSwitch { thread_id: 0 }to gateway, and gateway recognizes this sentinel via newclear_foreground()method onThreadManager, properly clearing the foreground state instead of returning "Thread 0 not found"./thread fg 0sentinel collision:/thread fg 0now returns an error message ("Thread ID 0 is reserved") instead of silently backgrounding the current thread.tokio::spawn(async {})placeholder fromRegistryEntry.cancel()andcancel_all()now rely solely on cooperativeCancellationTokensignaling.handle.abort()call fromSubtaskHandle::Drop. Now relies purely on cooperative cancellation so the spawned task has a chance to updateThreadManagerstatus before exiting./thread fgafter/thread bg: Gateway'sThreadSwitchhandler now usesswitch_foreground(target_id)instead ofswitch_to(target_id).is_some(). Theswitch_tomethod returns the old foreground ID (which isNoneafter backgrounding), causing the check to incorrectly fail even when the switch succeeds.switch_foregroundreturns aboolindicating success, fixing/thread fgfunctionality after backgrounding.switch_to().is_some()which failed after/thread bg(when foreground isNone), causing client/server desync. Now usesswitch_foreground()which correctly returnsbool, matching the fix already applied to the explicit ThreadSwitch handler.test_should_blockassertion inskill_audit.rs— the test was checking!should_block(50)but the comment noted "Still blocked due to error_count > 0", which is correct behavior. Flipped the assertion to match the intended behavior.oxidized-skills→oxidized-agentic-auditcrate rename.Review & Testing Checklist for Human
ThreadInfoDtonow has two new fields (kind_icon,status_icon). Since bincode is positional, this is a breaking wire format change — any mismatch between client/server versions will cause deserialization failures. Verify this is acceptable or if a version bump / migration strategy is needed.CancellationTokenwill run indefinitely if cancelled. Verify this design choice is acceptable. The spawned task is expected to cooperatively checktoken.is_cancelled()ortoken.cancelled().awaitin its main loop.thread_id == 0sentinel convention: The gateway treatsthread_id == 0as "clear foreground."ThreadId::new()starts from 1 (viaAtomicU64), so 0 should never be a real thread ID — but verify no code path createsThreadId(0)for a real thread.kind_iconis unused in rendering: Thekind_iconfield flows through the protocol but the sidebar only rendersstatus_icon. Verify this is intended as future-proofing or if it should be displayed (e.g. prepended to the label).test_should_blockwhich exists on main — the assertion was negated (testing!should_block) when the comment and logic require it to return true. Verify the fix is correct (witherror_count=2,should_block()always returns true).Test Plan
/thread new <label>/thread bg— verify it clears the foreground and messages area/thread fg <id>after backgrounding — verify it switches successfully (should say "Switched to thread (id: N)") instead of "Thread N not found"/thread fg 0— verify it returns an error instead of backgrounding/thread bg— send a message that triggers auto-switch (if enabled) and verify the client receives theThreadSwitchednotification (no desync)Notes
View original video (rec-7e9b6cc83ef8444eb965f43fb0854134-edited.mp4)
SubtaskHandleAPI is fully unit tested (6/6 tests pass) but lacks integration tests for spawn → sidebar → join flow