From 00e57e9c86b7671a12e41fa1e12cb76761729da5 Mon Sep 17 00:00:00 2001 From: youssef-joe Date: Sun, 8 Nov 2026 05:29:32 +0200 Subject: [PATCH 1/2] [#331] Batch read-only tool calls in a turn --- AUTHORS | 1 + doc/manual/en/30-tools.md | 7 +- src/session.rs | 176 +++++++++++++++++++++++++++++++++++--- src/tools.rs | 29 +++++++ 4 files changed, 199 insertions(+), 14 deletions(-) diff --git a/AUTHORS b/AUTHORS index 84869658..3f9dc6ad 100644 --- a/AUTHORS +++ b/AUTHORS @@ -10,3 +10,4 @@ Bassam Adnan Saleh Enab Tejas Tyagi abishekcs +Youssef Ali diff --git a/doc/manual/en/30-tools.md b/doc/manual/en/30-tools.md index d8dff744..642990c8 100644 --- a/doc/manual/en/30-tools.md +++ b/doc/manual/en/30-tools.md @@ -387,9 +387,10 @@ answer, so the main conversation's context stays small. Each subagent: `use_ssh`, which only copies files - runs at the same time as the other subagents the model starts in the same turn, up to four at once; their answers come back in the order the model - asked for them. Other tools in the turn still run one after another, in - order. Subagents that write share the workspace, so give concurrent writers - separate files + asked for them. Read-only tools the model starts together in one turn also + run at the same time; a tool that writes, or a subagent beside a read, keeps + the order the model asked for. Subagents that write share the workspace, so + give concurrent writers separate files - has a fixed budget of model requests, whatever `max_tool_rounds` says - runs silently: no streaming; `/tasks` shows the tool it is on and the tokens it has used diff --git a/src/session.rs b/src/session.rs index 439f045f..0c239efa 100644 --- a/src/session.rs +++ b/src/session.rs @@ -32,10 +32,9 @@ fn render_tool_result(result: Result) -> String { } } -/// Runs subagent `calls` concurrently, at most [`MAX_PARALLEL_SUBAGENTS`] at -/// a time, and returns their results in call order. Each runs on its own -/// clone of `tools` (clones share their state), and is aborted if the turn -/// is. +/// Runs `calls` concurrently, at most [`MAX_PARALLEL_SUBAGENTS`] at a time, +/// and returns their results in call order. Each runs on its own clone of +/// `tools` (clones share their state), and is aborted if the turn is. async fn execute_concurrently(tools: &ToolExecutor, calls: &[crate::llm::ToolCall]) -> Vec { let calls = calls .iter() @@ -54,6 +53,61 @@ async fn execute_concurrently(tools: &ToolExecutor, calls: &[crate::llm::ToolCal .await } +/// What a run of calls that may overlap has in common. +/// +/// A run is homogeneous: subagent starts overlap with subagent starts, and +/// read-only calls with read-only calls, but never across the two. A read +/// beside a writer would otherwise see a workspace the model had not asked it +/// to see yet. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum BatchKind { + Subagents, + ReadsOnly, +} + +/// The end (exclusive) of the run starting at `start`, and what the run is, +/// or `None` when that call must run alone. +fn parallel_run( + tools: &ToolExecutor, + calls: &[crate::llm::ToolCall], + start: usize, +) -> Option<(usize, BatchKind)> { + let kind = match calls[start].function.name.as_str() { + name if tools.runs_subagent(name) => BatchKind::Subagents, + name if tools.parallel_safe(name) && !tools.has_agent_hooks() => BatchKind::ReadsOnly, + _ => return None, + }; + + let mut continued_tasks: Vec<&str> = Vec::new(); + let mut end = start; + while end < calls.len() { + let call = &calls[end]; + let same_kind = match kind { + BatchKind::Subagents => tools.runs_subagent(&call.function.name), + BatchKind::ReadsOnly => tools.parallel_safe(&call.function.name), + }; + if !same_kind { + break; + } + // Two continuations of one task are not independent: the first + // restart wins and the second finds it running. Keep them in order. + if call.function.name == "continue_agent" { + let task = call + .function + .arguments + .get("task") + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + if continued_tasks.contains(&task) { + break; + } + continued_tasks.push(task); + } + end += 1; + } + Some((end, kind)) +} + /// Runs `run` on every call, at most `limit` at a time, each as its own /// task; the rendered results come back in call order. A task that panics /// becomes an error result rather than taking the turn down. @@ -619,13 +673,13 @@ impl ChatSession { on_tool_running(true); let mut index = 0; while index < tool_calls.len() { - // Consecutive subagent calls are independent of - // each other: run them together. Everything else + // Consecutive calls of one kind are independent + // of each other: subagent starts, or read-only + // tools. Run those together; everything else // runs in order, as the model asked. - let run_end = (index..tool_calls.len()) - .find(|&i| !tools.runs_subagent(&tool_calls[i].function.name)) - .unwrap_or(tool_calls.len()); - if run_end - index >= 2 { + if let Some((run_end, _)) = parallel_run(tools, &tool_calls, index) + && run_end - index >= 2 + { let batch = &tool_calls[index..run_end]; for tool_call in batch { on_tool_call(tool_call); @@ -719,7 +773,7 @@ impl std::error::Error for RoundsExhausted {} #[cfg(test)] mod tests { - use super::{ChatSession, run_concurrently, strip_stale_fragment}; + use super::{BatchKind, ChatSession, parallel_run, run_concurrently, strip_stale_fragment}; use crate::config::LlmConfiguration; use crate::context::fragments::ContextualFragment; use crate::test_support::serve_chat_responses; @@ -1268,6 +1322,106 @@ data: [DONE] assert_eq!(server.join().expect("server").len(), 2); } + fn tool_call(id: &str, name: &str, arguments: serde_json::Value) -> crate::llm::ToolCall { + crate::llm::ToolCall { + id: id.to_string(), + tool_type: "function".to_string(), + function: crate::llm::ToolCallFunction { + name: name.to_string(), + arguments: arguments + .as_object() + .cloned() + .unwrap_or_default() + .into_iter() + .collect(), + }, + } + } + + /// Read-only calls are independent, so a turn that asks for several files + /// runs them together. Anything that writes keeps the order the model + /// asked for. + #[test] + fn read_only_calls_overlap_but_writes_do_not() { + let workspace = tempfile::tempdir().unwrap(); + let tools = crate::tools::ToolExecutor::new(workspace.path()); + + let reads = vec![ + tool_call("a", "show_file", serde_json::json!({"path": "a.rs"})), + tool_call("b", "list_directory", serde_json::json!({})), + ]; + assert_eq!( + parallel_run(&tools, &reads, 0), + Some((2, BatchKind::ReadsOnly)) + ); + + let mixed = vec![ + tool_call("a", "show_file", serde_json::json!({"path": "a.rs"})), + tool_call("b", "create_file", serde_json::json!({"path": "b.rs"})), + ]; + assert_eq!( + parallel_run(&tools, &mixed, 0), + Some((1, BatchKind::ReadsOnly)) + ); + } + + /// A read and a subagent in one turn are not overlapped: the subagent may + /// write, and the read has to see what the model asked it to see. + #[test] + fn a_read_and_a_subagent_do_not_overlap() { + let workspace = tempfile::tempdir().unwrap(); + let tools = crate::tools::ToolExecutor::new(workspace.path()); + let calls = vec![ + tool_call("a", "show_file", serde_json::json!({"path": "a.rs"})), + tool_call("b", "explore_repository", serde_json::json!({"query": "x"})), + ]; + assert_eq!( + parallel_run(&tools, &calls, 0), + Some((1, BatchKind::ReadsOnly)) + ); + } + + /// Two continuations of one task are one task's work, not two: the second + /// has to wait for the first. Different tasks may overlap. + #[test] + fn continuations_of_one_task_stay_in_order() { + let workspace = tempfile::tempdir().unwrap(); + let tools = crate::tools::ToolExecutor::new(workspace.path()); + let same = vec![ + tool_call( + "a", + "continue_agent", + serde_json::json!({"task": "t1", "message": "one"}), + ), + tool_call( + "b", + "continue_agent", + serde_json::json!({"task": "t1", "message": "two"}), + ), + ]; + assert_eq!( + parallel_run(&tools, &same, 0), + Some((1, BatchKind::Subagents)) + ); + + let different = vec![ + tool_call( + "a", + "continue_agent", + serde_json::json!({"task": "t1", "message": "one"}), + ), + tool_call( + "b", + "continue_agent", + serde_json::json!({"task": "t2", "message": "two"}), + ), + ]; + assert_eq!( + parallel_run(&tools, &different, 0), + Some((2, BatchKind::Subagents)) + ); + } + /// Running out of rounds is a typed error. By default the turn is rolled /// back; a session told to keep it still has the work in its transcript, /// which is what a subagent's partial answer is made from. diff --git a/src/tools.rs b/src/tools.rs index 94478a75..4de9a03a 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -961,6 +961,35 @@ impl ToolExecutor { || crate::subagents::find(name).is_some() } + /// Whether `name` is a read-only tool that may share one turn with + /// another such call. + /// + /// A name-level allowlist on purpose: a tool whose safety depends on its + /// arguments (e.g. `run_agent` listing agents vs starting one) stays out. + /// `browse` is stateful (link numbers refer to the page it last showed), + /// the graph tools may build or refresh a cache, and MCP tools have + /// unknown side effects, so none of those qualify either. Subagent + /// starters are [`Self::runs_subagent`]'s business, not this list's. + pub fn parallel_safe(&self, name: &str) -> bool { + matches!( + name, + "show_file" + | "list_directory" + | "search_text" + | "expand_context" + | "fetch_url" + | "web_search" + | "use_skill" + ) + } + + /// Whether a hook runs around every tool call in this executor. Hooks can + /// observe call order, so the chat loop does not overlap otherwise + /// parallel-safe calls when one is installed. + pub fn has_agent_hooks(&self) -> bool { + self.agent_hooks.is_some() + } + /// Lets this conversation's subagents run in the background: an /// interactive client, which hands their reports to the model later. pub fn with_background_tasks(mut self) -> Self { From 8f6f6e631da6380984fefcc46bb18239e6ae50dc Mon Sep 17 00:00:00 2001 From: youssef-joe Date: Sun, 8 Nov 2026 13:10:48 +0200 Subject: [PATCH 2/2] [#331] Treat listing agents as a read-only call --- doc/manual/en/30-tools.md | 7 +-- src/session.rs | 94 ++++++++++++++++++++++++++++++++++++--- src/tools.rs | 27 ++++++++--- 3 files changed, 113 insertions(+), 15 deletions(-) diff --git a/doc/manual/en/30-tools.md b/doc/manual/en/30-tools.md index 642990c8..0c82efe5 100644 --- a/doc/manual/en/30-tools.md +++ b/doc/manual/en/30-tools.md @@ -388,9 +388,10 @@ answer, so the main conversation's context stays small. Each subagent: - runs at the same time as the other subagents the model starts in the same turn, up to four at once; their answers come back in the order the model asked for them. Read-only tools the model starts together in one turn also - run at the same time; a tool that writes, or a subagent beside a read, keeps - the order the model asked for. Subagents that write share the workspace, so - give concurrent writers separate files + run at the same time, and `run_agent` asked to `list` counts as one of them; + a tool that writes, or a subagent beside a read, keeps the order the model + asked for. Subagents that write share the workspace, so give concurrent + writers separate files - has a fixed budget of model requests, whatever `max_tool_rounds` says - runs silently: no streaming; `/tasks` shows the tool it is on and the tokens it has used diff --git a/src/session.rs b/src/session.rs index 0c239efa..ea1ae3dc 100644 --- a/src/session.rs +++ b/src/session.rs @@ -65,6 +65,19 @@ enum BatchKind { ReadsOnly, } +/// Whether `call` is local and read-only, so it may overlap another such +/// call. `run_agent` counts only when it lists agents. +fn is_read_only(tools: &ToolExecutor, call: &crate::llm::ToolCall) -> bool { + tools.parallel_safe(&call.function.name) + || tools.lists_agents(&call.function.name, &call.function.arguments) +} + +/// Whether `call` starts a subagent, as opposed to only listing agents. +fn starts_subagent(tools: &ToolExecutor, call: &crate::llm::ToolCall) -> bool { + tools.runs_subagent(&call.function.name) + && !tools.lists_agents(&call.function.name, &call.function.arguments) +} + /// The end (exclusive) of the run starting at `start`, and what the run is, /// or `None` when that call must run alone. fn parallel_run( @@ -72,10 +85,13 @@ fn parallel_run( calls: &[crate::llm::ToolCall], start: usize, ) -> Option<(usize, BatchKind)> { - let kind = match calls[start].function.name.as_str() { - name if tools.runs_subagent(name) => BatchKind::Subagents, - name if tools.parallel_safe(name) && !tools.has_agent_hooks() => BatchKind::ReadsOnly, - _ => return None, + let first = &calls[start]; + let kind = if starts_subagent(tools, first) { + BatchKind::Subagents + } else if is_read_only(tools, first) && !tools.has_agent_hooks() { + BatchKind::ReadsOnly + } else { + return None; }; let mut continued_tasks: Vec<&str> = Vec::new(); @@ -83,8 +99,8 @@ fn parallel_run( while end < calls.len() { let call = &calls[end]; let same_kind = match kind { - BatchKind::Subagents => tools.runs_subagent(&call.function.name), - BatchKind::ReadsOnly => tools.parallel_safe(&call.function.name), + BatchKind::Subagents => starts_subagent(tools, call), + BatchKind::ReadsOnly => is_read_only(tools, call), }; if !same_kind { break; @@ -1422,6 +1438,72 @@ data: [DONE] ); } + /// `run_agent` with `agent: "list"` only searches the registry, so it + /// batches with other reads. Starting an agent still does not. + #[test] + fn listing_agents_is_read_only_but_starting_one_is_not() { + let workspace = tempfile::tempdir().unwrap(); + let tools = crate::tools::ToolExecutor::new(workspace.path()); + + let listings = vec![ + tool_call( + "a", + "run_agent", + serde_json::json!({"agent": "list", "task": "review"}), + ), + tool_call("b", "show_file", serde_json::json!({"path": "a.rs"})), + ]; + assert_eq!( + parallel_run(&tools, &listings, 0), + Some((2, BatchKind::ReadsOnly)) + ); + + // Case and surrounding spaces do not change what the call is. + let upper = vec![ + tool_call( + "a", + "run_agent", + serde_json::json!({"agent": " LIST ", "task": "review"}), + ), + tool_call("b", "list_directory", serde_json::json!({})), + ]; + assert_eq!( + parallel_run(&tools, &upper, 0), + Some((2, BatchKind::ReadsOnly)) + ); + + let starting = vec![ + tool_call( + "a", + "run_agent", + serde_json::json!({"agent": "reviewer", "task": "review"}), + ), + tool_call("b", "show_file", serde_json::json!({"path": "a.rs"})), + ]; + assert_eq!( + parallel_run(&tools, &starting, 0), + Some((1, BatchKind::Subagents)) + ); + + // A listing and a start are different kinds, so they stay ordered. + let mixed = vec![ + tool_call( + "a", + "run_agent", + serde_json::json!({"agent": "list", "task": "review"}), + ), + tool_call( + "b", + "run_agent", + serde_json::json!({"agent": "reviewer", "task": "review"}), + ), + ]; + assert_eq!( + parallel_run(&tools, &mixed, 0), + Some((1, BatchKind::ReadsOnly)) + ); + } + /// Running out of rounds is a typed error. By default the turn is rolled /// back; a session told to keep it still has the work in its transcript, /// which is what a subagent's partial answer is made from. diff --git a/src/tools.rs b/src/tools.rs index 4de9a03a..e1de16af 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -964,12 +964,12 @@ impl ToolExecutor { /// Whether `name` is a read-only tool that may share one turn with /// another such call. /// - /// A name-level allowlist on purpose: a tool whose safety depends on its - /// arguments (e.g. `run_agent` listing agents vs starting one) stays out. - /// `browse` is stateful (link numbers refer to the page it last showed), - /// the graph tools may build or refresh a cache, and MCP tools have - /// unknown side effects, so none of those qualify either. Subagent - /// starters are [`Self::runs_subagent`]'s business, not this list's. + /// A name-level allowlist: a tool whose safety depends on its arguments + /// has its own test (see [`Self::lists_agents`]). `browse` is stateful + /// (link numbers refer to the page it last showed), the graph tools may + /// build or refresh a cache, and MCP tools have unknown side effects, so + /// none of those qualify. Subagent starters are [`Self::runs_subagent`]'s + /// business, not this list's. pub fn parallel_safe(&self, name: &str) -> bool { matches!( name, @@ -983,6 +983,21 @@ impl ToolExecutor { ) } + /// Whether this call asks `run_agent` to list matching agents. That is a + /// local search, not a subagent start, so it may share a turn with other + /// read-only calls. + pub fn lists_agents( + &self, + name: &str, + arguments: &std::collections::HashMap, + ) -> bool { + name == "run_agent" + && arguments + .get("agent") + .and_then(serde_json::Value::as_str) + .is_some_and(|agent| agent.trim().eq_ignore_ascii_case("list")) + } + /// Whether a hook runs around every tool call in this executor. Hooks can /// observe call order, so the chat loop does not overlap otherwise /// parallel-safe calls when one is installed.