feat: parallel sub-agent execution + read-only classifier hardening

crates/aish-llm/Cargo.toml

@@ -16,6 +16,7 @@ tokio.workspace = true
 tracing.workspace = true
 uuid.workspace = true
 futures.workspace = true
+parking_lot.workspace = true
 bytes.workspace = true
 sha2.workspace = true
 base64.workspace = true

crates/aish-shell/src/app.rs

@@ -1,5 +1,5 @@
 use std::io::{self, Read, Write};
-use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicUsize, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicU32, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
 
@@ -674,7 +674,7 @@ pub struct AishShell {
     inline_ai: Option<Arc<crate::inline_completion::InlineCompleter>>,
     /// Session-scoped approval memory shared with the LLM session. Kept on the
     /// shell so slash commands (e.g. `/forget-approvals`) can reset it.
-    approval_memory: Arc<Mutex<aish_llm::ApprovalMemory>>,
+    approval_memory: Arc<parking_lot::Mutex<aish_llm::ApprovalMemory>>,
 }
 
 impl AishShell {
@@ -1258,8 +1258,8 @@ impl AishShell {
         let compaction_active_ref = compaction_active.clone();
         let compaction_notice_shown = Arc::new(AtomicBool::new(false));
         let compaction_notice_shown_ref = compaction_notice_shown.clone();
-        let sub_agent_ui_active = Arc::new(AtomicBool::new(false));
-        let sub_agent_ui_active_ref = sub_agent_ui_active.clone();
+        let sub_agent_active_count = Arc::new(AtomicU32::new(0));
+        let sub_agent_active_count_ref = sub_agent_active_count.clone();
         let sub_agent_animation = Arc::new(SubAgentThinkingAnimation::new());
         let sub_agent_animation_ref = sub_agent_animation.clone();
 
@@ -1358,9 +1358,16 @@ impl AishShell {
                         compaction_notice_shown_ref.store(false, Ordering::SeqCst);
                         animation_ref.start(&t("shell.status.thinking"));
                     }
+                    LlmEventType::OpEnd if crate::llm_event_ui::sub_agent_llm_event(&event) => {
+                        // A sub-agent OpEnd marks one sub-session finishing, not the
+                        // parent turn end. Ignore it so a fast-finishing sibling does
+                        // not reset the active count (or stop the spinner) while other
+                        // sub-agents are still running. The count is decremented via
+                        // ToolExecutionEnd's saturating subtraction instead.
+                    }
                     LlmEventType::OpEnd => {
                         // Operation ends — stop animation and show timing
-                        sub_agent_ui_active_ref.store(false, Ordering::SeqCst);
+                        sub_agent_active_count_ref.store(0, Ordering::SeqCst);
                         sub_agent_animation_ref.stop();
                         animation_ref.stop();
                         let ttft = *ttft_value_ref.lock().unwrap();
@@ -1417,7 +1424,7 @@ impl AishShell {
                         reasoning_buf_ref.lock().unwrap().clear();
                         reasoning_frame_ref.store(0, Ordering::SeqCst);
                         renderer_ref.lock().unwrap().reset();
-                        if !sub_agent_ui_active_ref.load(Ordering::SeqCst) {
+                        if sub_agent_active_count_ref.load(Ordering::SeqCst) == 0 {
                             animation_ref.start(&t("shell.status.thinking"));
                         }
                     }
@@ -1573,7 +1580,7 @@ impl AishShell {
                     }
                     LlmEventType::ToolExecutionStart => {
                         if crate::llm_event_ui::is_parent_agent_spawn_tool_event(&event) {
-                            sub_agent_ui_active_ref.store(true, Ordering::SeqCst);
+                            sub_agent_active_count_ref.fetch_add(1, Ordering::SeqCst);
                             animation_ref.stop();
                             sub_agent_animation_ref.stop();
                             clear_reasoning();
@@ -1669,7 +1676,13 @@ impl AishShell {
                     }
                     LlmEventType::ToolExecutionEnd => {
                         if crate::llm_event_ui::is_parent_agent_spawn_tool_event(&event) {
-                            sub_agent_ui_active_ref.store(false, Ordering::SeqCst);
+                            // Decrement; saturating so a stray End (no matching Start,
+                            // e.g. after an OpEnd reset) cannot underflow the count.
+                            sub_agent_active_count_ref
+                                .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |v| {
+                                    Some(v.saturating_sub(1))
+                                })
+                                .ok();
                         }
                         // Stop progress spinner started at ToolExecutionStart.
                         animation_ref.stop();
@@ -1883,8 +1896,8 @@ impl AishShell {
         // Session-scoped approval memory: when the user approves a command with
         // "remember", equivalent commands (same host + normalized text) skip
         // confirmation — and the sandbox preflight — for the rest of the session.
-        let approval_memory: Arc<Mutex<aish_llm::ApprovalMemory>> =
-            Arc::new(Mutex::new(aish_llm::ApprovalMemory::new()));
+        let approval_memory: Arc<parking_lot::Mutex<aish_llm::ApprovalMemory>> =
+            Arc::new(parking_lot::Mutex::new(aish_llm::ApprovalMemory::new()));
         llm_session.set_approval_memory(approval_memory.clone());
 
         if let Some(ref audit) = audit_store {
@@ -3423,7 +3436,7 @@ impl AishShell {
     /// so previously "remembered" commands prompt for confirmation again.
     fn handle_forget_approvals(&mut self) {
         let count = {
-            let mut memory = self.approval_memory.lock().unwrap();
+            let mut memory = self.approval_memory.lock();
             let n = memory.len();
             memory.clear();
             n

crates/aish-tools/src/agent_tool/agent_tool.rs

@@ -125,7 +125,15 @@ impl Tool for AgentTool {
         args: serde_json::Value,
         session: &'a LlmSession,
     ) -> Pin<Box<dyn Future<Output = ToolResult> + Send + 'a>> {
-        Box::pin(async move {
+        use futures::FutureExt;
+        // Wrap the sub-agent run in `catch_unwind` so a panicking sub-session
+        // degrades to a `ToolResult::error` instead of unwinding through the
+        // caller. This matches the framework's `Tool::execute_async` contract
+        // (types.rs) and is essential under parallel `join_all` execution,
+        // where an uncontained panic would abort every concurrent sibling and
+        // the whole turn (e.g. a poisoned `Arc<Mutex>` shared via the parent
+        // session cascades a `.lock().unwrap()` panic to all siblings).
+        let inner = std::panic::AssertUnwindSafe(async move {
             let (_description, prompt, subagent_type) = match Self::validate_args(&args) {
                 Ok(v) => v,
                 Err(err) => return err,
@@ -162,6 +170,21 @@ impl Tool for AgentTool {
             }
 
             Self::spawn_result_to_tool_result(result)
+        });
+        Box::pin(async move {
+            match inner.catch_unwind().await {
+                Ok(result) => result,
+                Err(payload) => {
+                    let message = if let Some(s) = payload.downcast_ref::<&str>() {
+                        s.to_string()
+                    } else if let Some(s) = payload.downcast_ref::<String>() {
+                        s.clone()
+                    } else {
+                        "Agent sub-agent execution panicked".to_string()
+                    };
+                    ToolResult::error(format!("Error: {}", message))
+                }
+            }
         })
     }
 }
@@ -214,4 +237,24 @@ mod tests {
         assert!(!names.contains(&"command-diagnose"));
         assert!(!names.contains(&"diagnose"));
     }
+    #[test]
+    fn description_encourages_parallel_independent_agents() {
+        let tool = AgentTool::new();
+        // The tool prompt must steer the model toward emitting multiple Agent
+        // calls in one response — that is what unlocks concurrent execution.
+        let desc = tool.description();
+        assert!(desc.contains("Parallelize"), "missing parallel guidance");
+        assert!(
+            desc.contains("this single response"),
+            "must tell the model to emit calls in one response"
+        );
+        assert!(
+            desc.contains("concurrently"),
+            "must state the calls run concurrently"
+        );
+        assert!(
+            desc.contains("I/O-bound"),
+            "must explain why read-only tasks parallelize well"
+        );
+    }
 }

crates/aish-tools/src/agent_tool/prompt.rs

@@ -39,7 +39,16 @@ pub const USAGE_SECTION: &str = "\
 thorough), and whether the sub-agent must stay read-only. Default to quick or medium unless the \
 user asked for exhaustive coverage; do not expand scope to \"everywhere\" on your own.
 - When the sub-agent finishes, only its final conclusion is returned here; summarize for the user if needed.
-- Launch multiple agents in one turn when their tasks are independent.
+- **Parallelize independent work — decide for the user.** Users rarely say \"parallel\"; when \
+their request naturally covers several independent areas, YOU decompose it and emit **multiple \
+`Agent` calls in this single response** so they run concurrently. Read-only investigation, \
+environment checks, system diagnosis, and file scans are I/O-bound and independent — parallel \
+sub-agents finish in a fraction of the time, so default to fanning out. Pick the right type per \
+area: \"explain the frontend, backend, and database\" → 3× explore; \"check the auth, payment, \
+and logging services are healthy\" → 3× troubleshoot; \"scan src/, tests/, and configs/ for TODO \
+markers\" → 3× explore; \"audit env vars, disk usage, and open ports\" → troubleshoot or explore. \
+Do not wait for one to return before issuing the next, and do not make the user ask for parallelism. \
+Keep dependent tasks (a later one needs an earlier result) sequential.
 - If you delegate research to a sub-agent, do not duplicate the same searches in this session.";
 
 pub fn parameters(subagent_types: &[String]) -> serde_json::Value {

crates/aish-tools/src/bash/read_only.rs

@@ -111,6 +111,12 @@ fn split_compound_segments(command: &str) -> Vec<String> {
 
         if !in_single_quote && !in_double_quote {
             if ch == '&' {
+                // `>&N` / `<&N` duplicate a file descriptor; the `&` is part of
+                // the redirect token, not a command separator.
+                if current.ends_with('>') || current.ends_with('<') {
+                    current.push(ch);
+                    continue;
+                }
                 if chars.peek() == Some(&'&') {
                     chars.next();
                     segments.push(current.clone());
@@ -153,27 +159,39 @@ fn has_background_operator(command: &str) -> bool {
     let mut in_single_quote = false;
     let mut in_double_quote = false;
     let mut chars = command.chars().peekable();
+    let mut prev: Option<char> = None;
 
     while let Some(ch) = chars.next() {
         if ch == '\'' && !in_double_quote {
             in_single_quote = !in_single_quote;
+            prev = Some(ch);
             continue;
         }
         if ch == '"' && !in_single_quote {
             in_double_quote = !in_double_quote;
+            prev = Some(ch);
             continue;
         }
         if ch == '\\' && !in_single_quote {
             chars.next();
+            prev = None;
             continue;
         }
         if !in_single_quote && !in_double_quote && ch == '&' {
+            // `>&N` / `<&N` duplicate a file descriptor (e.g. `2>&1`, `<&2`);
+            // the `&` is part of the redirect, not a background operator.
+            if prev == Some('>') || prev == Some('<') {
+                prev = Some(ch);
+                continue;
+            }
             if chars.peek() == Some(&'&') {
                 chars.next();
+                prev = Some('&');
                 continue;
             }
             return true;
         }
+        prev = Some(ch);
     }
 
     false
@@ -188,10 +206,18 @@ fn non_readonly_segment_reason(segment: &str) -> Option<String> {
     }) {
         return Some("command or process substitution".into());
     }
-    if scan_outside_quotes(segment, |window| window.starts_with('*')) {
-        return Some("unquoted glob".into());
-    }
     if scan_outside_quotes(segment, |window| {
+        // `>&N` (e.g. `2>&1`) duplicates a file descriptor and `>&-` closes
+        // it; both are read-only. But `>&word` where `word` is NOT a digit
+        // or `-` redirects stdout to a file (e.g. `echo x >& /tmp/f` writes
+        // /tmp/f in bash), which IS a write — let it fall through to the `>`
+        // check below instead of blanket-allowing every `>&`.
+        if window.starts_with(">&") {
+            let after = window[2..].chars().next();
+            if matches!(after, Some(c) if c.is_ascii_digit()) || after == Some('-') {
+                return false;
+            }
+        }
         if let Some(rest) = window.strip_prefix(">>") {
             return !rest.trim_start().starts_with("/dev/null");
         }
@@ -242,15 +268,10 @@ fn non_readonly_segment_reason(segment: &str) -> Option<String> {
         }
     }
 
-    if base == "find"
-        && tokens.iter().any(|token| {
-            matches!(
-                token.to_ascii_lowercase().as_str(),
-                "-delete" | "-exec" | "-execdir" | "-ok" | "-okdir"
-            )
-        })
-    {
-        return Some("find mutating or command execution".into());
+    if base == "find" {
+        if let Some(reason) = find_action_not_readonly(&tokens) {
+            return Some(reason);
+        }
     }
 
     if base == "curl" && curl_writes_or_mutates(segment) {
@@ -265,6 +286,44 @@ fn non_readonly_segment_reason(segment: &str) -> Option<String> {
     None
 }
 
+/// Judge `find` action flags. `-delete` is inherently destructive; `-exec`,
+/// `-execdir`, `-ok` and `-okdir` run an embedded command whose read-only
+/// status is determined by recursively classifying it. This lets
+/// `find ... -exec grep/wc/head {} \;` through while still blocking
+/// `find ... -exec rm {} \;`.
+fn find_action_not_readonly(tokens: &[String]) -> Option<String> {
+    const EXEC_FLAGS: [&str; 4] = ["-exec", "-execdir", "-ok", "-okdir"];
+    let mut index = 0;
+    while index < tokens.len() {
+        let flag = tokens[index].to_ascii_lowercase();
+        if flag == "-delete" {
+            return Some("find -delete".into());
+        }
+        if EXEC_FLAGS.contains(&flag.as_str()) {
+            // Collect the embedded command up to the terminator `\;` or `+`.
+            let mut end = index + 1;
+            while end < tokens.len() {
+                let term = normalize_shell_word(&tokens[end]);
+                if term == ";" || term == "+" {
+                    break;
+                }
+                end += 1;
+            }
+            let embedded = tokens[index + 1..end].join(" ");
+            if embedded.trim().is_empty() {
+                return Some("find -exec without command".into());
+            }
+            if !matches!(classify(&embedded), ReadOnlyVerdict::ReadOnly) {
+                return Some("find -exec non-read-only command".into());
+            }
+            index = end + 1;
+        } else {
+            index += 1;
+        }
+    }
+    None
+}
+
 fn curl_writes_or_mutates(segment: &str) -> bool {
     let compact = segment
         .chars()
@@ -698,8 +757,13 @@ mod tests {
     }
 
     #[test]
-    fn blocks_unquoted_glob() {
-        assert_not_read_only("ls *.txt");
+    fn allows_unquoted_glob() {
+        // Glob expansion does not mutate the filesystem; write commands
+        // (`rm *.txt`, `cp *.x /d`) are still caught by the BLOCKED list and
+        // redirects (`echo *.x > f`) by the write-redirect check.
+        assert_read_only("ls *.txt");
+        assert_read_only("grep -r foo *.md");
+        assert_read_only("cat *.log");
     }
 
     #[test]
@@ -727,11 +791,41 @@ mod tests {
     fn blocks_trailing_background_operator() {
         assert_not_read_only("sleep 9999 &");
     }
-
     #[test]
-    fn blocks_find_exec_and_ok() {
+    fn fd_redirects_are_read_only() {
+        // `2>&1`, `>&2`, `<&2` duplicate file descriptors; they are read-only
+        // and must not be misclassified as background jobs or write redirects.
+        assert_read_only("ls 2>&1");
+        assert_read_only("cat foo 2>&1");
+        assert_read_only("grep bar file 2>&1 | head");
+        assert_read_only("echo hi >&2");
+        assert_read_only("echo hi >&-");
+        assert_read_only("cmd <&2");
+        // Real background operators and write redirects are still caught.
+        assert_not_read_only("sleep 10 &");
+        assert_not_read_only("cmd > file &");
+        assert_not_read_only("echo hi > file");
+        // `>&word` where `word` is NOT a digit redirects stdout to a file
+        // (verified in bash: `echo x >& /tmp/f` writes the file). These must
+        // NOT slip through the `>&N` allowance.
+        assert_not_read_only("echo x >& /tmp/f");
+        assert_not_read_only("echo x >&/tmp/f");
+        assert_not_read_only("echo x >& f");
+    }
+
+    #[test]
+    fn blocks_find_exec_mutating_and_allows_read_only() {
+        // -exec/-ok/-execdir running a mutating command -> blocked
         assert_not_read_only("find . -exec rm {} \\;");
         assert_not_read_only("find . -ok rm {} \\;");
+        assert_not_read_only("find . -execdir mv {} /tmp \\;");
+        // -exec/-ok running a read-only command -> allowed
+        assert_read_only("find . -exec grep foo {} \\;");
+        assert_read_only("find . -exec wc -l {} \\;");
+        assert_read_only("find . -exec head -n 1 {} \\;");
+        assert_read_only("find . -ok grep foo {} \\;");
+        // -delete stays destructive
+        assert_not_read_only("find . -name '*.tmp' -delete");
     }
 
     #[test]