From 4c654a3f0665069933589df060afa4776e6da181 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 21:00:09 +0200 Subject: [PATCH 1/2] feat(config, llm, cli): Add tool call fan-out A tool can now declare `fan_out` in its configuration, after which a single call to it carries several independent operations in an `ops` array. JP runs each one as its own invocation and folds the results into the one response the provider is waiting for. An assistant that already knows it wants fourteen file reads issues one call instead of fourteen, each of which would otherwise re-send the whole accumulated context. `fan_out = true` runs every operation at once and reports each failure on its own. The table form takes `concurrency`, which set to `1` runs them in order, and `on_error = "stop"`, which starts nothing further once one has failed. The folded result frames each operation with its position and names the ones that never ran, so a model that asked for five and reads three does not assume the other two succeeded. Nothing changes until a configuration sets the key: a tool without `fan_out` keeps its schema, its arguments, and its response unchanged, and no tool ships with it enabled. Terminal output stays identical either way, because a call carrying three operations renders three headers and prompts three times, so what the user approves is what they were shown. The envelope wraps whatever schema a tool declared, so it reaches MCP servers JP does not control, which no per-tool schema change could do. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/query/tool.rs | 3 +- .../jp_cli/src/cmd/query/tool/coordinator.rs | 481 +++++++++++++----- .../src/cmd/query/tool/coordinator_tests.rs | 2 + crates/jp_cli/src/cmd/query/tool/executor.rs | 17 + crates/jp_cli/src/cmd/query/tool/pending.rs | 23 +- .../src/cmd/query/tool/pending_tests.rs | 17 +- .../src/cmd/query/tool/prompter_tests.rs | 1 + crates/jp_cli/src/cmd/query/tool/schedule.rs | 363 +++++++++++++ .../src/cmd/query/tool/schedule_tests.rs | 411 +++++++++++++++ crates/jp_cli/src/cmd/query/turn_loop.rs | 67 ++- .../jp_cli/src/cmd/query/turn_loop_tests.rs | 361 +++++++++++++ crates/jp_cli/src/render/turn.rs | 20 +- crates/jp_config/src/conversation/tool.rs | 310 +++++++++++ .../jp_config/src/conversation/tool_tests.rs | 198 +++++++ ...onfig__tests__app_config_schema_shape.snap | 8 + crates/jp_llm/src/provider/anthropic.rs | 25 +- crates/jp_llm/src/provider/anthropic_tests.rs | 6 + crates/jp_llm/src/provider/cerebras.rs | 4 +- crates/jp_llm/src/provider/google.rs | 4 +- crates/jp_llm/src/provider/llamacpp.rs | 4 +- crates/jp_llm/src/provider/ollama.rs | 2 +- crates/jp_llm/src/provider/ollama_tests.rs | 2 + crates/jp_llm/src/provider/openai.rs | 8 +- crates/jp_llm/src/provider/openai_tests.rs | 1 + crates/jp_llm/src/provider/openrouter.rs | 18 +- crates/jp_llm/src/test.rs | 2 + crates/jp_llm/src/tool.rs | 54 +- crates/jp_llm/src/tool/executor.rs | 104 +++- crates/jp_llm/src/tool/fan_out.rs | 210 ++++++++ crates/jp_llm/src/tool/fan_out_tests.rs | 207 ++++++++ crates/jp_llm/src/tool/json_schema.rs | 14 +- crates/jp_llm/src/tool_tests.rs | 126 +++++ crates/jp_llm/src/window_tests.rs | 1 + docs/.vitepress/rfd-summaries.json | 4 + docs/rfd/108-transparent-tool-call-fan-out.md | 406 +++++++++++++++ 35 files changed, 3281 insertions(+), 203 deletions(-) create mode 100644 crates/jp_cli/src/cmd/query/tool/schedule.rs create mode 100644 crates/jp_cli/src/cmd/query/tool/schedule_tests.rs create mode 100644 crates/jp_llm/src/tool/fan_out.rs create mode 100644 crates/jp_llm/src/tool/fan_out_tests.rs create mode 100644 docs/rfd/108-transparent-tool-call-fan-out.md diff --git a/crates/jp_cli/src/cmd/query/tool.rs b/crates/jp_cli/src/cmd/query/tool.rs index 83f63d1f9..fd788d070 100644 --- a/crates/jp_cli/src/cmd/query/tool.rs +++ b/crates/jp_cli/src/cmd/query/tool.rs @@ -9,8 +9,9 @@ pub(crate) mod executor; pub(crate) mod inquiry; pub(crate) mod pending; pub(crate) mod prompter; +pub(crate) mod schedule; -pub(crate) use coordinator::{ToolCallDecision, ToolCallState, ToolCoordinator}; +pub(crate) use coordinator::{ExecutorGroup, ToolCallState, ToolCoordinator}; pub(crate) use executor::TerminalExecutorSource; pub(crate) use pending::{PendingEntry, PendingTools, build_execution_plan}; pub(crate) use prompter::ToolPrompter; diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 2e51af647..5127a0eee 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -87,7 +87,7 @@ use indexmap::IndexMap; use inquire::error::InquireError; use jp_config::{ conversation::tool::{ - FormatMode, QuestionTarget, ResultMode, RunMode, ToolSource, ToolsConfig, + FanOut, FormatMode, QuestionTarget, ResultMode, RunMode, ToolSource, ToolsConfig, style::ParametersStyle, }, interrupt::ToolInterruptConfig, @@ -104,6 +104,7 @@ use jp_inquire::{ReplyEditMode, prompt::PromptBackend}; use jp_llm::tool::{ StderrSink, executor::{Executor, ExecutorResult, ExecutorSource, PermissionInfo}, + fan_out::{self, OperationOutcome}, }; use jp_mcp::Client; use jp_printer::Printer; @@ -118,6 +119,7 @@ use super::{ ToolRenderer, inquiry::{self, InquiryBackend, InquiryError}, prompter::{PermissionResult, ToolPrompter}, + schedule::Schedule, }; use crate::{ Error, @@ -265,6 +267,11 @@ struct ExecutingTool { executor: Arc, tool_id: String, tool_name: String, + + /// This operation's display-state key, which is its tool call id unless the + /// call fans out. + state_key: String, + accumulated_answers: IndexMap, /// Where this tool's stderr goes while it runs. @@ -304,6 +311,73 @@ pub enum PermissionDecision { }, } +/// One operation of a tool call, after the permission decision. +pub enum GroupOp { + /// Approved and ready to run. + Run(Box), + + /// Decided before it ran: skipped by the user, or its argument formatter + /// failed. + /// The response stands in for the operation when the call's result is + /// folded. + Resolved(ToolCallResponse), +} + +/// Every operation belonging to one tool call. +/// +/// A call to a tool without fan-out holds exactly one operation, which is what +/// makes the fan-out machinery a no-op for it: one operation in, one response +/// out, no framing added. +pub struct ExecutorGroup { + /// The tool call id every operation in the group answers to. + pub tool_id: String, + + /// The tool being called. + pub tool_name: String, + + /// The call's fan-out policy, or `None` when it carries one operation. + pub fan_out: Option, + + /// The operations, in the order the assistant wrote them. + pub ops: Vec, +} + +impl ExecutorGroup { + /// Whether any operation still needs to run. + /// + /// A group with nothing left to run is folded straight into its response + /// rather than entering the execution loop. + #[must_use] + pub fn has_work(&self) -> bool { + self.ops.iter().any(|op| matches!(op, GroupOp::Run(_))) + } + + /// Fold a group whose operations were all decided before running. + fn fold_resolved(self) -> ToolCallResponse { + let outcomes: Vec<_> = self + .ops + .into_iter() + .map(|op| match op { + GroupOp::Resolved(response) => match response.result { + Ok(content) => OperationOutcome::Ok(content), + Err(message) => OperationOutcome::Error(message), + }, + GroupOp::Run(_) => { + unreachable!("fold_resolved is only called on a group with no work") + } + }) + .collect(); + + // Every operation was skipped or failed the same way, so the call as a + // whole did not run. `Ok` matches how a single skipped tool answers + // today: the user declining is not an error the assistant should retry. + ToolCallResponse { + id: self.tool_id, + result: Ok(fan_out::fold(&outcomes)), + } + } +} + /// Final outcome of [`ToolCoordinator::resolve_tool_call_decision`] — the /// per-tool permission pipeline. /// @@ -377,7 +451,17 @@ fn tool_question_to_inquiry_question(q: &Question) -> InquiryQuestion { } pub struct ToolCoordinator { - executors: Vec<(usize, Box)>, + /// Prepared executors, grouped by the plan index of the call they belong + /// to. + /// A group holds one executor per operation, so a call without fan-out + /// holds exactly one. + executors: Vec<(usize, Vec>)>, + + /// Display state per *operation*, keyed by `Executor::state_key`. + /// + /// Not keyed by tool call id: a fanned-out call renders one line per + /// operation, and two operations of one call would otherwise overwrite each + /// other's state and make `is_prompting` report whichever wrote last. tool_states: HashMap, tools_config: ToolsConfig, interrupt_config: ToolInterruptConfig, @@ -424,8 +508,8 @@ impl ToolCoordinator { self.tool_states.values().any(ToolCallState::is_prompting) } - pub(crate) fn set_tool_state(&mut self, tool_id: impl Into, state: ToolCallState) { - self.tool_states.insert(tool_id.into(), state); + pub(crate) fn set_tool_state(&mut self, state_key: impl Into, state: ToolCallState) { + self.tool_states.insert(state_key.into(), state); } fn clear_tool_states(&mut self) { @@ -722,7 +806,7 @@ impl ToolCoordinator { let mut unavailable = Vec::new(); for (index, request) in requests.into_iter().enumerate() { match self.prepare_one(request) { - Ok(executor) => self.executors.push((index, executor)), + Ok(executors) => self.executors.push((index, executors)), Err(response) => unavailable.push((index, response)), } } @@ -730,36 +814,82 @@ impl ToolCoordinator { unavailable } - /// Prepares a single executor for a tool call request. + /// Prepares the executors for a tool call request. /// - /// Returns the executor on success, or an error response if the tool cannot - /// be resolved (e.g. missing from config or definitions). + /// A call to a tool without fan-out yields exactly one executor. + /// A call to a fan-out tool yields one per operation in its `ops` array, + /// all sharing the request's tool call id and each carrying that + /// operation's arguments. + /// + /// # Errors + /// + /// Returns an error response when the tool cannot be resolved (missing from + /// config or definitions), or when a fan-out call's envelope is malformed. + #[allow(clippy::needless_pass_by_value)] pub fn prepare_one( &mut self, request: ToolCallRequest, - ) -> Result, ToolCallResponse> { - self.tool_states - .insert(request.id.clone(), ToolCallState::Queued); + ) -> Result>, ToolCallResponse> { + let Some(config) = self.tools_config.get(&request.name) else { + return Err(self.unavailable(&request)); + }; - if let Some(executor) = self - .tools_config - .get(&request.name) - .and_then(|config| self.executor_source.create(request.clone(), config)) - { - return Ok(executor); + // The envelope is taken apart here rather than inside the executor + // source, so a malformed one answers with a message naming what went + // wrong instead of looking like a tool that does not exist. + let operations = match config.fan_out() { + None => vec![(None, request.arguments.clone())], + Some(_) => match fan_out::expand(&request.arguments) { + Ok(ops) => ops + .into_iter() + .enumerate() + .map(|(op, arguments)| (Some(op), arguments)) + .collect(), + Err(error) => { + warn!( + tool = %request.name, + id = %request.id, + "Malformed fan-out envelope, returning error to LLM", + ); + self.set_tool_state(&request.id, ToolCallState::Completed); + return Err(ToolCallResponse { + id: request.id.clone(), + result: Err(error.message(&request.name)), + }); + } + }, + }; + + let mut executors = Vec::with_capacity(operations.len()); + for (op, arguments) in operations { + let mut op_request = request.clone(); + op_request.arguments = arguments; + + let Some(executor) = self.executor_source.create(op_request, config.clone(), op) else { + return Err(self.unavailable(&request)); + }; + + self.tool_states + .insert(executor.state_key(), ToolCallState::Queued); + executors.push(executor); } + Ok(executors) + } + + /// The response for a tool the LLM named but JP cannot run. + fn unavailable(&mut self, request: &ToolCallRequest) -> ToolCallResponse { warn!(tool = %request.name, "Tool not available, returning error to LLM"); self.set_tool_state(&request.id, ToolCallState::Completed); - Err(ToolCallResponse { - id: request.id, + ToolCallResponse { + id: request.id.clone(), result: Err(format!( "Tool '{}' is not available. It may have been available earlier in this \ conversation but is no longer enabled. Do not retry this tool until it it is \ available again in the list of enabled tools.", request.name, )), - }) + } } /// Renders the tool call header and arguments after permission approval. @@ -815,7 +945,7 @@ impl ToolCoordinator { }; if !interactive && matches!(info.run_mode, RunMode::Ask | RunMode::Edit) { - self.set_tool_state(&info.tool_id, ToolCallState::Running); + self.set_tool_state(&info.state_key, ToolCallState::Running); return PermissionDecision::Approved(executor); } @@ -828,11 +958,11 @@ impl ToolCoordinator { match persisted { Some(true) => { - self.set_tool_state(&info.tool_id, ToolCallState::Running); + self.set_tool_state(&info.state_key, ToolCallState::Running); PermissionDecision::Approved(executor) } Some(false) => { - self.set_tool_state(&info.tool_id, ToolCallState::Completed); + self.set_tool_state(&info.state_key, ToolCallState::Completed); PermissionDecision::Skipped(ToolCallResponse { id: info.tool_id.clone(), result: Ok("Tool skipped by user (remembered).".to_string()), @@ -863,7 +993,7 @@ impl ToolCoordinator { .insert(permission_key, true); } executor.set_arguments(arguments); - self.set_tool_state(&info.tool_id, ToolCallState::Running); + self.set_tool_state(&info.state_key, ToolCallState::Running); Ok(executor) } Ok(PermissionResult::Skip { reason, persist }) => { @@ -872,7 +1002,7 @@ impl ToolCoordinator { .remembered_permission_decisions .insert(permission_key, false); } - self.set_tool_state(&info.tool_id, ToolCallState::Completed); + self.set_tool_state(&info.state_key, ToolCallState::Completed); let msg = if let Some(r) = reason { format!("Tool skipped by user: {r}") } else { @@ -884,7 +1014,7 @@ impl ToolCoordinator { }) } Err(e) => { - self.set_tool_state(&info.tool_id, ToolCallState::Completed); + self.set_tool_state(&info.state_key, ToolCallState::Completed); Err(ToolCallResponse { id: info.tool_id.clone(), result: Err(format!("Permission prompt failed: {e}")), @@ -893,20 +1023,72 @@ impl ToolCoordinator { } } + /// Decide permission for every prepared operation, grouped by tool call. + /// + /// Each operation of a fanned-out call is prompted for separately, because + /// each one is rendered as its own call: approving three lines with one + /// prompt would ask the user to approve something other than what they were + /// shown. + /// + /// Returns the groups with work left to do, and the folded responses of the + /// calls whose every operation was decided here. pub async fn run_permission_phase( &mut self, prompter: &ToolPrompter, interactive: bool, turn_state: &mut TurnState, tool_renderer: &ToolRenderer, - ) -> ( - Vec<(usize, Box)>, - Vec<(usize, ToolCallResponse)>, - ) { - let mut approved_executors = Vec::new(); - let mut skipped_responses = Vec::new(); + ) -> (Vec<(usize, ExecutorGroup)>, Vec<(usize, ToolCallResponse)>) { + let mut groups = Vec::new(); + let mut resolved = Vec::new(); + + for (index, executors) in std::mem::take(&mut self.executors) { + let group = self + .decide_group(executors, prompter, interactive, turn_state, tool_renderer) + .await; + + if group.has_work() { + groups.push((index, group)); + } else { + resolved.push((index, group.fold_resolved())); + } + } - for (index, executor) in std::mem::take(&mut self.executors) { + (groups, resolved) + } + + /// Fold a group whose every operation was decided before it could run. + /// + /// The caller has already checked [`ExecutorGroup::has_work`]; this turns + /// what is left into the one response the call answers with. + #[must_use] + pub fn fold_decided_group(group: ExecutorGroup) -> ToolCallResponse { + group.fold_resolved() + } + + /// Run every operation of one call through the permission pipeline. + pub async fn decide_group( + &mut self, + executors: Vec>, + prompter: &ToolPrompter, + interactive: bool, + turn_state: &mut TurnState, + tool_renderer: &ToolRenderer, + ) -> ExecutorGroup { + let tool_id = executors + .first() + .map(|e| e.tool_id().to_owned()) + .unwrap_or_default(); + let tool_name = executors + .first() + .map(|e| e.tool_name().to_owned()) + .unwrap_or_default(); + let fan_out = self.tools_config.get(&tool_name).and_then(|c| c.fan_out()); + + let mut ops = Vec::with_capacity(executors.len()); + let mut rendered = Vec::new(); + + for executor in executors { // Funnel through the unified per-tool permission pipeline. The // streaming path in `turn_loop.rs` uses the same call so the // decide → pre-render → prompt → render policy stays in one @@ -927,18 +1109,32 @@ impl ToolCoordinator { rendered_arguments, } => { if let Some(content) = rendered_arguments { - self.rendered_arguments - .insert(executor.tool_id().to_owned(), content); + rendered.push(content); } - approved_executors.push((index, executor)); + ops.push(GroupOp::Run(executor)); } ToolCallDecision::Skipped(response) | ToolCallDecision::Failed(response) => { - skipped_responses.push((index, response)); + ops.push(GroupOp::Resolved(response)); } } } - (approved_executors, skipped_responses) + // One event carries the whole call, so its operations' custom-formatted + // output is stored as one record. Joining reproduces on replay exactly + // what was printed live, which was these chunks one after another, and + // keeps the metadata value a string for conversations recorded before + // fan-out existed. + if !rendered.is_empty() { + self.rendered_arguments + .insert(tool_id.clone(), rendered.join("\n")); + } + + ExecutorGroup { + tool_id, + tool_name, + fan_out, + ops, + } } /// Run the approved tools, answering their questions and result prompts as @@ -951,7 +1147,7 @@ impl ToolCoordinator { #[allow(clippy::too_many_lines)] pub async fn execute_with_prompting( &mut self, - executors: Vec<(usize, Box)>, + groups: Vec<(usize, ExecutorGroup)>, prompter: Arc, signals: &SignalRouter, turn_coordinator: &mut TurnCoordinator, @@ -967,14 +1163,14 @@ impl ToolCoordinator { tool_renderer: &mut ToolRenderer, interactive: bool, ) -> ExecutionResult { - if executors.is_empty() { + if groups.is_empty() { return ExecutionResult { responses: Vec::new(), outcome: ExecutionOutcome::Completed, }; } - debug!(tools = executors.len(), "Starting tool execution."); + debug!(tools = groups.len(), "Starting tool execution."); // Register the tool interrupt handler for this execution phase. While // registered, the first Ctrl-C press is delivered to this event loop; @@ -986,21 +1182,19 @@ impl ToolCoordinator { // waiting for the longest tool to finish. let (interrupt_guard, mut interrupt_rx) = signals.push_handler_for(conv.id()); - // The caller's `index` values come from the execution plan and may - // be sparse (e.g. when some tools in the same plan are - // pre-resolved and don't reach this function). We can't use them - // as offsets into a `Vec` sized to `executors.len()`, so we - // re-base to contiguous local indices for internal bookkeeping - // and pair each response back with its plan index on output. - let plan_indices: Vec = executors.iter().map(|(idx, _)| *idx).collect(); - let executors: Vec> = - executors.into_iter().map(|(_, exec)| exec).collect(); - - let total_tools = executors.len(); + // The caller's `index` values come from the execution plan and may be + // sparse (e.g. when some tools in the same plan are pre-resolved and + // don't reach this function), and one call may hold several operations. + // Both are flattened here: every operation gets a contiguous local index + // for internal bookkeeping, and `Schedule` remembers which call each one + // belongs to so the responses can be folded back per call on output. + let mut schedule = Schedule::new(groups); + + let total_ops = schedule.total_ops(); let cancellation_token = self.cancellation_token.clone(); let (event_tx, mut event_rx) = mpsc::channel::(32); let mut executing_tools: HashMap = HashMap::new(); - let mut results: Vec> = vec![None; total_tools]; + let mut results: Vec> = vec![None; total_ops]; let mut pending_prompts: VecDeque = VecDeque::new(); let mut prompt_active = false; @@ -1018,37 +1212,16 @@ impl ToolCoordinator { // shows how long a tool has been going. tool_renderer.start_progress(); - for (index, executor) in executors.into_iter().enumerate() { - let tool_id = executor.tool_id().to_string(); - let tool_name = executor.tool_name().to_string(); - // No pre-seeding: static answers flow through the late - // `static_answer` path so every question round-trip is recorded as - // an inquiry pair (RFD 082). - let accumulated_answers = IndexMap::new(); - - let executor: Arc = Arc::from(executor); - - let stderr = stderr_sink(tool_renderer, &self.tools_config, &tool_name); - - executing_tools.insert(index, ExecutingTool { - executor: Arc::clone(&executor), - tool_id: tool_id.clone(), - tool_name: tool_name.clone(), - accumulated_answers: accumulated_answers.clone(), - stderr: stderr.clone(), - }); - - self.set_tool_state(&tool_id, ToolCallState::Running); - - Self::spawn_tool_execution( + for (index, executor) in schedule.release(&results) { + self.start_operation( index, executor, - accumulated_answers, - mcp_client.clone(), - root.to_path_buf(), - cancellation_token.child_token(), - event_tx.clone(), - stderr, + &mut executing_tools, + tool_renderer, + mcp_client, + root, + &cancellation_token, + &event_tx, ); } @@ -1141,7 +1314,7 @@ impl ToolCoordinator { Self::record_inquiry_answer(conv, &inquiry_id, &answer); if let Some(tool) = executing_tools.get_mut(&index) { tool.accumulated_answers.insert(question_id, answer); - self.set_tool_state(&tool.tool_id, ToolCallState::Running); + self.set_tool_state(&tool.state_key, ToolCallState::Running); Self::spawn_tool_execution( index, tool.executor.clone(), @@ -1167,7 +1340,7 @@ impl ToolCoordinator { warn!(index, %error, "Received InquiryResult for unknown tool."); } Some(tool) => { - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); results[index] = Some(ToolCallResponse { id: tool.tool_id.clone(), @@ -1289,12 +1462,14 @@ impl ToolCoordinator { outcome.upgrade(ExecutionOutcome::Restart); } ToolInterruptResult::Cancelled { response, exit } => { - cancelled_indices = results - .iter() - .enumerate() - .filter(|(_, r)| r.is_none()) - .map(|(i, _)| i) - .collect(); + cancelled_indices = schedule.unfinished(&results); + + // A call holding operations back behind a + // concurrency limit would otherwise leave the + // loop waiting on results that can never + // arrive, because those operations were never + // spawned. + schedule.abandon_unstarted(); tools_cancelled = true; cancellation_message = response; if exit { @@ -1313,7 +1488,35 @@ impl ToolCoordinator { } } - if results.iter().all(Option::is_some) { + // Tell the schedule which operations failed, so a call configured + // to stop on error starts none of the ones still queued. + // Re-scanning every finished operation each time is cheap at these + // sizes, and recording the same failure twice is a no-op. + for (index, response) in results.iter().enumerate() { + if let Some(response) = response { + schedule.record_outcome(index, response); + } + } + + // Release whatever the finished operations made room for: the next + // operation of a call with a concurrency limit, or nothing at all + // for a call already running everything it has. + if !tools_cancelled { + for (index, executor) in schedule.release(&results) { + self.start_operation( + index, + executor, + &mut executing_tools, + tool_renderer, + mcp_client, + root, + &cancellation_token, + &event_tx, + ); + } + } + + if schedule.all_accounted_for(&results) { break; } } @@ -1324,36 +1527,27 @@ impl ToolCoordinator { tool_renderer.clear_progress(); - let mut responses: Vec<(usize, ToolCallResponse)> = plan_indices - .into_iter() - .zip(results.into_iter().map(|r| { - r.unwrap_or_else(|| ToolCallResponse { - id: "unknown".to_string(), - result: Err("Tool did not complete".to_string()), - }) - })) - .collect(); - if tools_cancelled { for &i in &cancelled_indices { - let Some((_, response)) = responses.get_mut(i) else { - continue; - }; - - response.result = Ok(if let Some(msg) = &cancellation_message { + let content = if let Some(msg) = &cancellation_message { format!("Tool run cancelled by user with a custom message:\n\n{msg}") } else { // No custom message: each cancelled tool answers with its - // configured cancellation response. - let tool_name = executing_tools - .get(&i) - .map(|tool| tool.tool_name.as_str()) - .unwrap_or_default(); - self.cancellation_response(tool_name) + // configured cancellation response. Read off the schedule + // rather than `executing_tools`, which only knows the + // operations that were actually spawned. + self.cancellation_response(schedule.tool_name(i)) + }; + + results[i] = Some(ToolCallResponse { + id: schedule.tool_id(i).to_owned(), + result: Ok(content), }); } } + let responses = schedule.fold(results); + ExecutionResult { responses, outcome } } @@ -1374,6 +1568,57 @@ impl ToolCoordinator { } } + /// Register an operation and spawn it. + /// + /// Called once per operation when the schedule releases it, which for a + /// call without a concurrency limit is all of them up front. + #[allow(clippy::too_many_arguments)] + fn start_operation( + &mut self, + index: usize, + executor: Box, + executing_tools: &mut HashMap, + tool_renderer: &ToolRenderer, + mcp_client: &Client, + root: &Utf8Path, + cancellation_token: &CancellationToken, + event_tx: &mpsc::Sender, + ) { + let tool_id = executor.tool_id().to_string(); + let tool_name = executor.tool_name().to_string(); + let state_key = executor.state_key(); + + // No pre-seeding: static answers flow through the late `static_answer` + // path so every question round-trip is recorded as an inquiry pair + // (RFD 082). + let accumulated_answers = IndexMap::new(); + + let executor: Arc = Arc::from(executor); + let stderr = stderr_sink(tool_renderer, &self.tools_config, &tool_name); + + executing_tools.insert(index, ExecutingTool { + executor: Arc::clone(&executor), + tool_id, + tool_name, + state_key: state_key.clone(), + accumulated_answers: accumulated_answers.clone(), + stderr: stderr.clone(), + }); + + self.set_tool_state(state_key, ToolCallState::Running); + + Self::spawn_tool_execution( + index, + executor, + accumulated_answers, + mcp_client.clone(), + root.to_path_buf(), + cancellation_token.child_token(), + event_tx.clone(), + stderr, + ); + } + fn spawn_tool_execution( index: usize, executor: Arc, @@ -1564,11 +1809,11 @@ impl ToolCoordinator { &results_file_link, ); } - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); *tracked_response = Some(response); } ResultMode::Skip => { - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); *tracked_response = Some(ToolCallResponse { id: response.id, result: Ok("Result delivery skipped by configuration.".to_string()), @@ -1616,7 +1861,7 @@ impl ToolCoordinator { &results_file_link, ); } - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); *tracked_response = Some(response); } } @@ -1821,7 +2066,7 @@ impl ToolCoordinator { .insert(answer_key, answer.clone()); } tool.accumulated_answers.insert(question_id, answer); - self.set_tool_state(&tool.tool_id, ToolCallState::Running); + self.set_tool_state(&tool.state_key, ToolCallState::Running); Self::spawn_tool_execution( index, tool.executor.clone(), @@ -1867,7 +2112,7 @@ impl ToolCoordinator { Self::record_inquiry_cancelled(conv, inquiry_id, reason); if let Some(tool) = executing_tools.get(&index) { - self.set_tool_state(&tool.tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); results[index] = Some(ToolCallResponse { id: tool.tool_id.clone(), result, @@ -1990,7 +2235,7 @@ impl ToolCoordinator { inquiry_id, } => { if let Some(tool) = executing_tools.get(&index) { - self.set_tool_state(&tool.tool_id, ToolCallState::AwaitingInput); + self.set_tool_state(&tool.state_key, ToolCallState::AwaitingInput); } Self::spawn_user_prompt(index, question, inquiry_id, prompter, event_tx); } diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index 6791930a6..9897632ed 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -489,6 +489,7 @@ async fn test_resolve_tool_call_decision_invalidates_prerender_on_edit() { arguments: pre_edit_args.clone(), permission_info: PermissionInfo { tool_id: "call_1".into(), + state_key: "call_1".into(), tool_name: "fs_delete_file".into(), tool_source: ToolSource::Builtin { tool: None }, run_mode: RunMode::Ask, @@ -546,6 +547,7 @@ fn test_permission_decision_cache_is_isolated_from_answers() { let info = PermissionInfo { tool_id: "call_1".into(), + state_key: "call_1".into(), tool_name: "my_tool".into(), tool_source: ToolSource::Builtin { tool: None }, run_mode: RunMode::Ask, diff --git a/crates/jp_cli/src/cmd/query/tool/executor.rs b/crates/jp_cli/src/cmd/query/tool/executor.rs index f8f0a64b1..e8941cf7e 100644 --- a/crates/jp_cli/src/cmd/query/tool/executor.rs +++ b/crates/jp_cli/src/cmd/query/tool/executor.rs @@ -100,8 +100,13 @@ impl ExecutorSource for TerminalExecutorSource { &self, mut request: ToolCallRequest, config: ToolConfigWithDefaults, + op: Option, ) -> Option> { let definition = self.definitions.get(&request.name)?.clone(); + + // Coercion reads the per-operation schema, and `request.arguments` is + // one operation's arguments whether or not the call fanned out, so a + // JSON-encoded number is repaired the same way either way. definition.coerce_arguments(&mut request.arguments); Some(Box::new(ToolExecutor::new( @@ -111,6 +116,7 @@ impl ExecutorSource for TerminalExecutorSource { Arc::new(self.builtin_executors.clone()), self.approvals.clone(), self.invocation.clone(), + op, ))) } } @@ -132,9 +138,13 @@ pub struct ToolExecutor { builtin_executors: Arc, approvals: Arc, invocation: InvocationContext, + + /// Which operation of the call this executor runs, when the call fans out. + op: Option, } impl ToolExecutor { + #[allow(clippy::too_many_arguments)] fn new( request: ToolCallRequest, config: ToolConfigWithDefaults, @@ -142,6 +152,7 @@ impl ToolExecutor { builtin_executors: Arc, approvals: Arc, invocation: InvocationContext, + op: Option, ) -> Self { Self { request, @@ -150,6 +161,7 @@ impl ToolExecutor { builtin_executors, approvals, invocation, + op, } } @@ -184,6 +196,10 @@ impl Executor for ToolExecutor { &self.request.name } + fn op_index(&self) -> Option { + self.op + } + fn arguments(&self) -> &serde_json::Map { &self.request.arguments } @@ -198,6 +214,7 @@ impl Executor for ToolExecutor { Some(PermissionInfo { tool_id: self.request.id.clone(), + state_key: self.state_key(), tool_name: self.request.name.clone(), tool_source: self.config.source().clone(), run_mode, diff --git a/crates/jp_cli/src/cmd/query/tool/pending.rs b/crates/jp_cli/src/cmd/query/tool/pending.rs index 28278ac72..025ae93d9 100644 --- a/crates/jp_cli/src/cmd/query/tool/pending.rs +++ b/crates/jp_cli/src/cmd/query/tool/pending.rs @@ -24,16 +24,21 @@ use jp_conversation::{ ConversationStream, event::{ToolCallRequest, ToolCallResponse}, }; -use jp_llm::tool::executor::Executor; + +use super::ExecutorGroup; /// The work product for a single tool call, as decided during the streaming /// phase. pub(crate) enum PendingEntry { - /// Permission was approved and the executor is ready to run. - Approved(Box), - /// Permission was denied (`Skip`) or the tool couldn't be resolved - /// (`Unavailable`); the response is already determined and just needs to be - /// committed in the right order. + /// At least one of the call's operations was approved and is ready to run. + /// + /// The group carries every operation of the call, including any the user + /// skipped, so the folded response accounts for all of them. + Approved(ExecutorGroup), + /// Nothing is left to run: the tool couldn't be resolved, or the user + /// declined every operation. + /// The response is already determined and just needs to be committed in the + /// right order. Resolved(ToolCallResponse), } @@ -53,9 +58,9 @@ impl PendingTools { Self::default() } - /// Record an approved executor for `id`. - pub(crate) fn insert_approved(&mut self, id: String, executor: Box) { - self.entries.insert(id, PendingEntry::Approved(executor)); + /// Record a call's approved operations for `id`. + pub(crate) fn insert_approved(&mut self, id: String, group: ExecutorGroup) { + self.entries.insert(id, PendingEntry::Approved(group)); } /// Record a pre-resolved response (skipped or unavailable) for `id`. diff --git a/crates/jp_cli/src/cmd/query/tool/pending_tests.rs b/crates/jp_cli/src/cmd/query/tool/pending_tests.rs index 2ae352082..d49478a5a 100644 --- a/crates/jp_cli/src/cmd/query/tool/pending_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/pending_tests.rs @@ -2,10 +2,10 @@ use jp_conversation::{ ConversationStream, event::{ChatRequest, ChatResponse, ToolCallRequest, ToolCallResponse}, }; -use jp_llm::tool::executor::MockExecutor; +use jp_llm::tool::executor::{Executor, MockExecutor}; use serde_json::Map; -use super::*; +use super::{super::coordinator::GroupOp, *}; fn req(id: &str, name: &str) -> ToolCallRequest { ToolCallRequest { @@ -22,8 +22,17 @@ fn resp(id: &str, content: &str) -> ToolCallResponse { } } -fn approved_executor(id: &str, name: &str) -> Box { - Box::new(MockExecutor::completed(id, name, "done")) +/// A call carrying one approved operation, which is what a tool without fan-out +/// produces. +fn approved_executor(id: &str, name: &str) -> ExecutorGroup { + ExecutorGroup { + tool_id: id.to_owned(), + tool_name: name.to_owned(), + fan_out: None, + ops: vec![GroupOp::Run( + Box::new(MockExecutor::completed(id, name, "done")) as Box, + )], + } } #[test] diff --git a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs index 53261d6bd..40c2de20b 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs @@ -177,6 +177,7 @@ fn prompter_with_editor(prompt: MockPromptBackend, editor: MockEditorBackend) -> fn make_permission_info(run_mode: RunMode, arguments: Value) -> PermissionInfo { PermissionInfo { tool_id: "call_123".to_string(), + state_key: "call_123".to_string(), tool_name: "test_tool".to_string(), tool_source: ToolSource::Builtin { tool: None }, run_mode, diff --git a/crates/jp_cli/src/cmd/query/tool/schedule.rs b/crates/jp_cli/src/cmd/query/tool/schedule.rs new file mode 100644 index 000000000..bf3470e61 --- /dev/null +++ b/crates/jp_cli/src/cmd/query/tool/schedule.rs @@ -0,0 +1,363 @@ +//! Which operations run, in what order, and how their results fold back. +//! +//! One provider tool call becomes one [`ExecutorGroup`], which holds one +//! operation without fan-out and several with it. +//! The execution loop works in flat local indices, one per operation, so this +//! type owns the translation: it hands out the operations that may start, +//! records which never will, and folds each call's operations back into the one +//! response the provider is waiting for. +//! +//! A call without fan-out passes through unchanged: one operation, started +//! immediately, folded to its own response with no framing added. + +use jp_conversation::event::ToolCallResponse; +use jp_llm::tool::{ + executor::Executor, + fan_out::{self, OperationOutcome}, +}; + +use super::coordinator::{ExecutorGroup, GroupOp}; + +/// One call's operations, as flat local indices into the execution loop's +/// bookkeeping. +struct Group { + /// The plan index the folded response is paired with on output. + plan_index: usize, + + /// The tool call id the folded response answers to. + tool_id: String, + + /// The tool being called, for messages that name it. + tool_name: String, + + /// Maximum operations of this call in flight at once, or `None` for + /// unbounded. + concurrency: Option, + + /// Whether a failure stops the operations that have not started. + stop_on_error: bool, + + /// Whether this call's response needs per-operation framing. + /// + /// False for a call without fan-out, whose single result is its whole + /// response. + fans_out: bool, + + /// Local indices of this call's operations, in the order the assistant + /// wrote them. + ops: Vec, + + /// How many of `ops` have been handed to the execution loop. + started: usize, + + /// One-based position of the first operation that failed, once one has. + first_failure: Option, +} + +/// Why an operation was never started. +enum NotRun { + /// An earlier operation of the same call failed under `on_error = "stop"`. + /// + /// Carries that operation's one-based position, which the folded result + /// names so the assistant can see which failure stopped the rest. + AfterFailure { after: usize }, + + /// The call was cancelled before this operation started. + /// + /// The caller writes the cancellation response into `results`, which the + /// fold prefers, so this only exists to let the execution loop finish: an + /// operation that never started will never send an event to wait for. + Cancelled, +} + +/// The execution loop's view of what to run and what to report. +pub(crate) struct Schedule { + groups: Vec, + + /// Which group each local index belongs to. + owner: Vec, + + /// Executors awaiting their turn, indexed by local index. + /// + /// Taken out when the operation starts; a `None` here means the operation + /// either started already or was decided before it could. + queued: Vec>>, + + /// Operations decided before the loop began, by local index. + /// + /// These hold the response the permission phase produced, which is folded + /// in place rather than executed. + predecided: Vec>, + + /// Operations that will never start, by local index. + not_run: Vec>, +} + +impl Schedule { + /// Flatten the groups into local indices. + pub(crate) fn new(groups: Vec<(usize, ExecutorGroup)>) -> Self { + let mut schedule = Self { + groups: Vec::with_capacity(groups.len()), + owner: Vec::new(), + queued: Vec::new(), + predecided: Vec::new(), + not_run: Vec::new(), + }; + + for (plan_index, group) in groups { + let group_id = schedule.groups.len(); + let mut ops = Vec::with_capacity(group.ops.len()); + + // A decision made before the loop began can already be a failure: + // an argument formatter that errored resolves its operation to one. + // Recorded here so a `stop` policy sees it on the very first + // release, rather than only noticing failures that happen later. + let mut first_failure = None; + + for (position, op) in group.ops.into_iter().enumerate() { + let local = schedule.owner.len(); + schedule.owner.push(group_id); + ops.push(local); + + match op { + GroupOp::Run(executor) => { + schedule.queued.push(Some(executor)); + schedule.predecided.push(None); + } + GroupOp::Resolved(response) => { + if response.result.is_err() && first_failure.is_none() { + first_failure = Some(position + 1); + } + schedule.queued.push(None); + schedule.predecided.push(Some(response)); + } + } + schedule.not_run.push(None); + } + + schedule.groups.push(Group { + plan_index, + tool_id: group.tool_id, + tool_name: group.tool_name, + concurrency: group.fan_out.and_then(|f| f.concurrency), + stop_on_error: group.fan_out.is_some_and(|f| f.stops_on_error()), + fans_out: group.fan_out.is_some(), + ops, + started: 0, + first_failure, + }); + } + + schedule + } + + /// Total operations across every call. + pub(crate) fn total_ops(&self) -> usize { + self.owner.len() + } + + /// Take the operations that may start now. + /// + /// Called once before the loop begins and again each time an operation + /// finishes, so a call with a concurrency limit releases its next operation + /// as an earlier one completes. + /// Operations a `stop` policy has ruled out are recorded here rather than + /// returned, so the caller's "is everything accounted for?" check sees + /// them. + pub(crate) fn release( + &mut self, + results: &[Option], + ) -> Vec<(usize, Box)> { + let mut released = Vec::new(); + + for group_id in 0..self.groups.len() { + loop { + let group = &self.groups[group_id]; + if group.started >= group.ops.len() { + break; + } + + // A call that stops on error starts nothing further once one of + // its operations has failed. Operations already running are left + // alone: they are past the point where not starting them is an + // option. + if group.stop_on_error + && let Some(after) = group.first_failure + { + for &local in &group.ops[group.started..] { + self.not_run[local] = Some(NotRun::AfterFailure { after }); + } + let group = &mut self.groups[group_id]; + group.started = group.ops.len(); + break; + } + + if let Some(limit) = group.concurrency { + let in_flight = group.ops[..group.started] + .iter() + .filter(|&&local| { + results[local].is_none() + && self.predecided[local].is_none() + && self.not_run[local].is_none() + }) + .count(); + if in_flight >= limit { + break; + } + } + + let local = group.ops[group.started]; + self.groups[group_id].started += 1; + + // A predecided operation occupies its slot without running, so + // it never counts against the concurrency limit and never + // blocks the next one. + if let Some(executor) = self.queued[local].take() { + released.push((local, executor)); + } + } + } + + released + } + + /// Record that a local index finished, so a `stop` policy can act on it. + pub(crate) fn record_outcome(&mut self, local: usize, response: &ToolCallResponse) { + if response.result.is_ok() { + return; + } + + let group_id = self.owner[local]; + let group = &mut self.groups[group_id]; + let position = group + .ops + .iter() + .position(|&op| op == local) + .map_or(1, |index| index + 1); + + if group.first_failure.is_none_or(|first| position < first) { + group.first_failure = Some(position); + } + } + + /// Whether every operation is accounted for: finished, decided before it + /// ran, or ruled out. + pub(crate) fn all_accounted_for(&self, results: &[Option]) -> bool { + (0..self.owner.len()).all(|local| self.is_accounted_for(local, results)) + } + + /// Whether one operation is accounted for. + pub(crate) fn is_accounted_for( + &self, + local: usize, + results: &[Option], + ) -> bool { + results[local].is_some() + || self.predecided[local].is_some() + || self.not_run[local].is_some() + } + + /// Local indices of every operation that has not reported yet, whether it + /// is running or still queued. + /// + /// Used when the user cancels, to decide which operations answer with the + /// cancellation response. + pub(crate) fn unfinished(&self, results: &[Option]) -> Vec { + (0..self.owner.len()) + .filter(|&local| !self.is_accounted_for(local, results)) + .collect() + } + + /// Give up on every operation that has not started yet. + /// + /// Called when the user cancels. + /// Without this the execution loop would wait forever for operations that + /// were never spawned and so will never send a result: an unbounded call + /// has everything in flight, but a call with a concurrency limit is holding + /// some back by design. + pub(crate) fn abandon_unstarted(&mut self) { + for group in &mut self.groups { + for &local in &group.ops[group.started..] { + self.not_run[local] = Some(NotRun::Cancelled); + } + group.started = group.ops.len(); + } + } + + /// The tool a local index belongs to, for a message that needs to name it. + pub(crate) fn tool_name(&self, local: usize) -> &str { + &self.groups[self.owner[local]].tool_name + } + + /// The tool call id a local index answers to. + pub(crate) fn tool_id(&self, local: usize) -> &str { + &self.groups[self.owner[local]].tool_id + } + + /// Fold each call's operations into the one response it answers with. + /// + /// Operation order follows the assistant's, not completion order, so the + /// sections line up with the `ops` array it wrote. + pub(crate) fn fold( + mut self, + results: Vec>, + ) -> Vec<(usize, ToolCallResponse)> { + let mut results: Vec> = results; + + self.groups + .drain(..) + .map(|group| { + let outcomes: Vec = group + .ops + .iter() + .map(|&local| { + // `results` is consulted first so a cancellation written + // over an abandoned operation wins over its "not run" + // marker, which only existed to end the wait. + let response = results[local] + .take() + .or_else(|| self.predecided[local].take()); + + match (response, self.not_run[local].take()) { + (Some(response), _) => match response.result { + Ok(content) => OperationOutcome::Ok(content), + Err(message) => OperationOutcome::Error(message), + }, + (None, Some(NotRun::AfterFailure { after })) => { + OperationOutcome::NotRun { after } + } + (None, Some(NotRun::Cancelled)) => { + OperationOutcome::Error("Operation cancelled.".to_owned()) + } + (None, None) => { + OperationOutcome::Error("Tool did not complete".to_owned()) + } + } + }) + .collect(); + + // A call that does not fan out answers with its one operation's + // result verbatim, including whether it failed. Folding would + // flatten that into a success carrying error text. + let result = if group.fans_out { + Ok(fan_out::fold(&outcomes)) + } else { + match outcomes.into_iter().next() { + Some(OperationOutcome::Ok(content)) => Ok(content), + Some(OperationOutcome::Error(message)) => Err(message), + _ => Err("Tool did not complete".to_owned()), + } + }; + + (group.plan_index, ToolCallResponse { + id: group.tool_id, + result, + }) + }) + .collect() + } +} + +#[cfg(test)] +#[path = "schedule_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs b/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs new file mode 100644 index 000000000..784cfed05 --- /dev/null +++ b/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs @@ -0,0 +1,411 @@ +use jp_config::conversation::tool::FanOutOnError; +use jp_llm::tool::executor::MockExecutor; + +use super::*; + +/// A group of `count` runnable operations under one tool call id. +fn group( + tool_id: &str, + count: usize, + concurrency: Option, + on_error: FanOutOnError, +) -> ExecutorGroup { + ExecutorGroup { + tool_id: tool_id.to_owned(), + tool_name: "my_tool".to_owned(), + fan_out: Some(jp_config::conversation::tool::FanOut { + concurrency, + on_error, + }), + ops: (0..count) + .map(|_| { + GroupOp::Run(Box::new(MockExecutor::completed(tool_id, "my_tool", "ok")) + as Box) + }) + .collect(), + } +} + +/// A call to a tool without fan-out: exactly one operation, no envelope. +fn single(tool_id: &str) -> ExecutorGroup { + ExecutorGroup { + tool_id: tool_id.to_owned(), + tool_name: "my_tool".to_owned(), + fan_out: None, + ops: vec![GroupOp::Run( + Box::new(MockExecutor::completed(tool_id, "my_tool", "ok")) as Box, + )], + } +} + +fn ok(tool_id: &str, content: &str) -> ToolCallResponse { + ToolCallResponse { + id: tool_id.to_owned(), + result: Ok(content.to_owned()), + } +} + +fn err(tool_id: &str, message: &str) -> ToolCallResponse { + ToolCallResponse { + id: tool_id.to_owned(), + result: Err(message.to_owned()), + } +} + +#[test] +fn an_unbounded_call_releases_every_operation_at_once() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 3, None, FanOutOnError::Continue))]); + let results = vec![None; schedule.total_ops()]; + + let released = schedule.release(&results); + + assert_eq!(released.iter().map(|(i, _)| *i).collect::>(), vec![ + 0, 1, 2 + ]); +} + +#[test] +fn a_sequential_call_releases_one_operation_at_a_time() { + let mut schedule = Schedule::new(vec![( + 0, + group("call_1", 3, Some(1), FanOutOnError::Continue), + )]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![0], + "only the first operation starts" + ); + + assert!( + schedule.release(&results).is_empty(), + "nothing else starts while the first is still running" + ); + + results[0] = Some(ok("call_1", "first")); + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![1], + "finishing the first releases the second" + ); +} + +#[test] +fn a_bounded_call_keeps_the_configured_number_in_flight() { + let mut schedule = Schedule::new(vec![( + 0, + group("call_1", 5, Some(2), FanOutOnError::Continue), + )]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![0, 1] + ); + + results[1] = Some(ok("call_1", "second")); + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![2], + "one slot freed releases exactly one operation" + ); +} + +/// Operations of different calls do not compete for each other's slots. +#[test] +fn concurrency_is_counted_per_call() { + let mut schedule = Schedule::new(vec![ + (0, group("call_1", 2, Some(1), FanOutOnError::Continue)), + (1, group("call_2", 2, Some(1), FanOutOnError::Continue)), + ]); + let results = vec![None; schedule.total_ops()]; + + let released = schedule.release(&results); + + assert_eq!( + released.iter().map(|(i, _)| *i).collect::>(), + vec![0, 2], + "each call starts its own first operation" + ); +} + +#[test] +fn stop_on_error_starts_nothing_after_a_failure() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 4, Some(1), FanOutOnError::Stop))]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + results[0] = Some(err("call_1", "boom")); + schedule.record_outcome(0, results[0].as_ref().expect("recorded")); + + assert!( + schedule.release(&results).is_empty(), + "the remaining operations never start" + ); + assert!( + schedule.all_accounted_for(&results), + "operations that will never start are accounted for, so the loop can exit" + ); + + let folded = schedule.fold(results); + assert_eq!(folded.len(), 1); + assert_eq!( + folded[0].1.result, + Ok( + "[1/4] error\nboom\n\n[2/4] not run (stopped after operation 1 failed)\n\n[3/4] not \ + run (stopped after operation 1 failed)\n\n[4/4] not run (stopped after operation 1 \ + failed)\n" + .to_owned() + ) + ); +} + +#[test] +fn continue_on_error_runs_every_operation() { + let mut schedule = Schedule::new(vec![( + 0, + group("call_1", 3, Some(1), FanOutOnError::Continue), + )]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + results[0] = Some(err("call_1", "boom")); + schedule.record_outcome(0, results[0].as_ref().expect("recorded")); + + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![1], + "a failure does not stop the rest" + ); +} + +/// The envelope is invisible at N=1: a one-operation fan-out call answers with +/// its operation's output and nothing else. +#[test] +fn a_single_operation_folds_without_framing() { + let schedule = Schedule::new(vec![(0, group("call_1", 1, None, FanOutOnError::Continue))]); + let results = vec![Some(ok("call_1", "file contents"))]; + + let folded = schedule.fold(results); + + assert_eq!(folded[0].1.result, Ok("file contents".to_owned())); +} + +/// A call to a tool without fan-out keeps its failure a failure. +/// Folding it would turn an error into a success carrying error text, which the +/// assistant reads as the tool having worked. +#[test] +fn a_call_without_fan_out_passes_its_error_through() { + let schedule = Schedule::new(vec![(0, single("call_1"))]); + let results = vec![Some(err("call_1", "not found"))]; + + let folded = schedule.fold(results); + + assert_eq!(folded[0].1.result, Err("not found".to_owned())); +} + +#[test] +fn folding_preserves_plan_indices_and_tool_ids() { + let schedule = Schedule::new(vec![ + (3, group("call_a", 2, None, FanOutOnError::Continue)), + (7, single("call_b")), + ]); + let results = vec![ + Some(ok("call_a", "one")), + Some(ok("call_a", "two")), + Some(ok("call_b", "three")), + ]; + + let folded = schedule.fold(results); + + assert_eq!(folded[0].0, 3); + assert_eq!(folded[0].1.id, "call_a"); + assert_eq!( + folded[0].1.result, + Ok("[1/2] ok\none\n\n[2/2] ok\ntwo\n".to_owned()) + ); + assert_eq!(folded[1].0, 7); + assert_eq!(folded[1].1.id, "call_b"); + assert_eq!(folded[1].1.result, Ok("three".to_owned())); +} + +/// Operations are folded in the order the assistant wrote them, not the order +/// they happened to finish in. +#[test] +fn folding_follows_the_assistants_order_not_completion_order() { + let schedule = Schedule::new(vec![(0, group("call_1", 3, None, FanOutOnError::Continue))]); + let results = vec![ + Some(ok("call_1", "first")), + Some(ok("call_1", "second")), + Some(ok("call_1", "third")), + ]; + + let folded = schedule.fold(results); + + assert_eq!( + folded[0].1.result, + Ok("[1/3] ok\nfirst\n\n[2/3] ok\nsecond\n\n[3/3] ok\nthird\n".to_owned()) + ); +} + +#[test] +fn a_predecided_operation_never_runs_but_still_reports() { + let mut group = group("call_1", 2, None, FanOutOnError::Continue); + group.ops[0] = GroupOp::Resolved(ok("call_1", "Tool skipped by user.")); + + let mut schedule = Schedule::new(vec![(0, group)]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + let released = schedule.release(&results); + assert_eq!( + released.iter().map(|(i, _)| *i).collect::>(), + vec![1], + "the skipped operation is not handed to the execution loop" + ); + + results[1] = Some(ok("call_1", "ran")); + assert!(schedule.all_accounted_for(&results)); + + let folded = schedule.fold(results); + assert_eq!( + folded[0].1.result, + Ok("[1/2] ok\nTool skipped by user.\n\n[2/2] ok\nran\n".to_owned()) + ); +} + +/// A skipped operation holds a slot without occupying one, so a sequential call +/// does not stall waiting for something that will never report. +#[test] +fn a_predecided_operation_does_not_hold_a_concurrency_slot() { + let mut group = group("call_1", 3, Some(1), FanOutOnError::Continue); + group.ops[0] = GroupOp::Resolved(ok("call_1", "skipped")); + + let mut schedule = Schedule::new(vec![(0, group)]); + let results = vec![None; schedule.total_ops()]; + + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![1], + "the skipped operation passes through and the next one starts" + ); +} + +#[test] +fn unfinished_lists_the_operations_that_have_not_reported() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 3, None, FanOutOnError::Continue))]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + schedule.release(&results); + + results[1] = Some(ok("call_1", "done")); + + assert_eq!(schedule.unfinished(&results), vec![0, 2]); +} + +/// A cancelled call holding operations behind a concurrency limit must still +/// let the execution loop finish. +/// Those operations were never spawned, so no result will ever arrive for them +/// and the loop would wait forever. +#[test] +fn abandoning_unstarted_operations_lets_a_cancelled_call_finish() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 4, Some(1), FanOutOnError::Stop))]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + assert!(!schedule.all_accounted_for(&results)); + + // The user cancels: every operation that has not reported answers with the + // cancellation response, including the three that never started. + let cancelled = schedule.unfinished(&results); + assert_eq!(cancelled, vec![0, 1, 2, 3]); + + schedule.abandon_unstarted(); + + // The one running operation reports, and the loop can now see that nothing + // else is outstanding. + results[0] = Some(ok("call_1", "cancelled")); + assert!( + schedule.all_accounted_for(&results), + "operations that were never spawned must not keep the loop waiting" + ); + + for &index in &cancelled { + results[index] = Some(ok("call_1", "cancelled")); + } + + let folded = schedule.fold(results); + assert_eq!( + folded[0].1.result, + Ok( + "[1/4] ok\ncancelled\n\n[2/4] ok\ncancelled\n\n[3/4] ok\ncancelled\n\n[4/4] \ + ok\ncancelled\n" + .to_owned() + ), + "a written cancellation wins over the marker that ended the wait" + ); +} + +/// An operation that failed before the loop began (an argument formatter that +/// errored) stops the rest under `on_error = "stop"`, the same as one that +/// failed while running. +#[test] +fn a_predecided_failure_stops_the_operations_behind_it() { + let mut group = group("call_1", 3, Some(1), FanOutOnError::Stop); + group.ops[0] = GroupOp::Resolved(err("call_1", "formatter failed")); + + let mut schedule = Schedule::new(vec![(0, group)]); + let results: Vec> = vec![None; schedule.total_ops()]; + + assert!( + schedule.release(&results).is_empty(), + "nothing starts behind a failure that was already decided" + ); + assert!(schedule.all_accounted_for(&results)); + + let folded = schedule.fold(results); + assert_eq!( + folded[0].1.result, + Ok( + "[1/3] error\nformatter failed\n\n[2/3] not run (stopped after operation 1 \ + failed)\n\n[3/3] not run (stopped after operation 1 failed)\n" + .to_owned() + ) + ); +} + +#[test] +fn schedule_reports_the_tool_behind_a_local_index() { + let schedule = Schedule::new(vec![ + (0, group("call_a", 2, None, FanOutOnError::Continue)), + (1, single("call_b")), + ]); + + assert_eq!(schedule.tool_id(0), "call_a"); + assert_eq!(schedule.tool_id(1), "call_a"); + assert_eq!(schedule.tool_id(2), "call_b"); + assert_eq!(schedule.tool_name(2), "my_tool"); +} diff --git a/crates/jp_cli/src/cmd/query/turn_loop.rs b/crates/jp_cli/src/cmd/query/turn_loop.rs index 5a2884920..556a467fd 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop.rs @@ -36,7 +36,7 @@ use jp_llm::{ model::ModelDetails, provider::get_provider, query::ChatQuery, - tool::{InvocationContext, ToolDefinition, executor::Executor}, + tool::{InvocationContext, ToolDefinition}, with_idle_timeout, with_output_limit, }; use jp_printer::{ErrChannel, Printer, RegionStyle, StatusRegion}; @@ -55,7 +55,7 @@ use super::{ handle_stream_error, }, tool::{ - PendingEntry, PendingTools, ToolCallDecision, ToolCallState, ToolCoordinator, ToolPrompter, + ExecutorGroup, PendingEntry, PendingTools, ToolCallState, ToolCoordinator, ToolPrompter, ToolRenderer, build_execution_plan, inquiry::{InquiryBackend, InquiryConfig, LlmInquiryBackend}, }, @@ -681,15 +681,16 @@ pub(super) async fn run_turn_loop( tool_renderer.complete(&req.id); match tool_coordinator.prepare_one(req.clone()) { - Ok(executor) => { - // Run the unified per-tool permission - // pipeline. The await blocks the - // streaming event loop while the user - // decides; LLM events buffer in the - // channel and are processed after. - let decision = tool_coordinator - .resolve_tool_call_decision( - executor, + Ok(executors) => { + // Run the unified permission pipeline + // over every operation of the call. The + // await blocks the streaming event loop + // while the user decides; LLM events + // buffer in the channel and are + // processed after. + let group = tool_coordinator + .decide_group( + executors, &prompter, interactive, &mut turn_state, @@ -697,25 +698,21 @@ pub(super) async fn run_turn_loop( ) .await; - match decision { - ToolCallDecision::Approved { - executor, - rendered_arguments, - } => { - if let Some(content) = rendered_arguments { - conv.update_events(|stream| { - store_rendered_arguments( - stream, &req.id, &content, - ); - }); - } - pending_tools - .insert_approved(req.id.clone(), executor); - } - ToolCallDecision::Skipped(resp) - | ToolCallDecision::Failed(resp) => { - pending_tools.insert_resolved(req.id.clone(), resp); - } + for (id, content) in + tool_coordinator.drain_rendered_arguments() + { + conv.update_events(|stream| { + store_rendered_arguments(stream, &id, &content); + }); + } + + if group.has_work() { + pending_tools.insert_approved(req.id.clone(), group); + } else { + pending_tools.insert_resolved( + req.id.clone(), + ToolCoordinator::fold_decided_group(group), + ); } } Err(resp) => { @@ -789,9 +786,9 @@ pub(super) async fn run_turn_loop( ) .await; - for (_idx, exec) in executors { - let id = exec.tool_id().to_owned(); - pending_tools.insert_approved(id, exec); + for (_idx, group) in executors { + let id = group.tool_id.clone(); + pending_tools.insert_approved(id, group); } for (_idx, resp) in skipped { pending_tools.insert_resolved(resp.id.clone(), resp); @@ -813,11 +810,11 @@ pub(super) async fn run_turn_loop( let (items, orphaned) = plan.into_parts(); - let mut approved: Vec<(usize, Box)> = Vec::new(); + let mut approved: Vec<(usize, ExecutorGroup)> = Vec::new(); let mut pre_resolved: Vec<(usize, ToolCallResponse)> = Vec::new(); for item in items { match item.work { - PendingEntry::Approved(exec) => approved.push((item.index, exec)), + PendingEntry::Approved(group) => approved.push((item.index, group)), PendingEntry::Resolved(resp) => pre_resolved.push((item.index, resp)), } } diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index 21ddaba91..695e59116 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1397,6 +1397,7 @@ async fn test_tool_interrupt_menu_cancel_escalates() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -1546,6 +1547,7 @@ async fn test_tool_stop_on_interrupt_commits_responses_without_follow_up() { options: IndexMap::default(), access: None, cancellation_response: Some(CUSTOM_CANCELLATION_RESPONSE.to_string()), + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -1689,6 +1691,7 @@ async fn test_interrupt_during_tool_prompt_completes_turn_early() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2028,6 +2031,7 @@ async fn test_tool_restart_on_interrupt() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2184,6 +2188,7 @@ async fn test_merged_stream_exits_after_tool_response() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2297,6 +2302,7 @@ async fn test_tool_call_with_run_mode_ask_approves() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2332,6 +2338,7 @@ async fn test_tool_call_with_run_mode_ask_approves() { MockExecutor::completed(&req.id, &req.name, "mock output").with_permission_info( PermissionInfo { tool_id: req.id.clone(), + state_key: req.id.clone(), tool_name: req.name.clone(), tool_source: ToolSource::Local { tool: None }, run_mode: RunMode::Ask, @@ -2441,6 +2448,7 @@ async fn test_tool_call_with_run_mode_ask_skips() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2476,6 +2484,7 @@ async fn test_tool_call_with_run_mode_ask_skips() { MockExecutor::completed(&req.id, &req.name, "should not see this") .with_permission_info(PermissionInfo { tool_id: req.id.clone(), + state_key: req.id.clone(), tool_name: req.name.clone(), tool_source: ToolSource::Local { tool: None }, run_mode: RunMode::Ask, @@ -2596,6 +2605,7 @@ async fn test_permission_prompt_follows_interactive_not_is_tty() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2629,6 +2639,7 @@ async fn test_permission_prompt_follows_interactive_not_is_tty() { MockExecutor::completed(&req.id, &req.name, "mock output").with_permission_info( PermissionInfo { tool_id: req.id.clone(), + state_key: req.id.clone(), tool_name: req.name.clone(), tool_source: ToolSource::Local { tool: None }, run_mode: RunMode::Ask, @@ -2723,6 +2734,7 @@ async fn test_tool_call_with_run_mode_unattended() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2862,6 +2874,7 @@ async fn test_tool_call_with_run_mode_skip() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -2902,6 +2915,7 @@ async fn test_tool_call_with_run_mode_skip() { ) .with_permission_info(PermissionInfo { tool_id: req.id.clone(), + state_key: req.id.clone(), tool_name: req.name.clone(), tool_source: ToolSource::Local { tool: None }, run_mode: RunMode::Skip, @@ -3017,6 +3031,7 @@ async fn test_multiple_tools_with_different_run_modes() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); // tool_unattended runs automatically config @@ -3038,6 +3053,7 @@ async fn test_multiple_tools_with_different_run_modes() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -3098,6 +3114,7 @@ async fn test_multiple_tools_with_different_run_modes() { MockExecutor::completed(&req.id, &req.name, "ask tool output") .with_permission_info(PermissionInfo { tool_id: req.id.clone(), + state_key: req.id.clone(), tool_name: req.name.clone(), tool_source: ToolSource::Local { tool: None }, run_mode: RunMode::Ask, @@ -3226,6 +3243,7 @@ async fn test_tool_call_returns_error() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -4533,6 +4551,7 @@ async fn test_parallel_tool_calls_rendered_atomically() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); config .conversation @@ -4553,6 +4572,7 @@ async fn test_parallel_tool_calls_rendered_atomically() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -4726,6 +4746,7 @@ async fn test_single_tool_call_rendered_with_args() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -4833,6 +4854,340 @@ async fn test_single_tool_call_rendered_with_args() { assert!(test_result.is_ok(), "Test timed out"); } +/// One provider tool call carrying three operations runs the tool three times +/// and answers with one response holding all three results. +/// +/// This is the whole of fan-out end to end: the envelope goes in, three +/// separate executions come out, the terminal shows three headers, and the +/// provider gets back the single response its one call is waiting for. +#[tokio::test] +#[expect(clippy::too_many_lines)] +async fn a_fanned_out_call_runs_every_operation_and_answers_once() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.style.tool_call.show = true; + config.conversation.tools.defaults.run = RunMode::Unattended; + config + .conversation + .tools + .insert("fs_read_file".to_string(), ToolConfig { + source: ToolSource::Local { tool: None }, + command: None, + run: Some(RunMode::Unattended), + format: None, + enable: None, + summary: None, + description: None, + examples: None, + parameters: IndexMap::new(), + result: None, + style: None, + questions: IndexMap::new(), + options: IndexMap::default(), + access: None, + cancellation_response: None, + fan_out: Some(jp_config::conversation::tool::FanOutConfig { + enabled: Some(true), + concurrency: None, + on_error: None, + }), + }); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::in_memory(root).with_backend(fs.clone()); + + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let chat_request = ChatRequest::from("Read three files"); + + // The envelope the provider is shown, carrying three operations. + let args = json!({ + "ops": [ + { "path": "a.rs" }, + { "path": "b.rs" }, + { "path": "c.rs" }, + ] + }); + + let provider: Arc = Arc::new({ + let events = vec![ + Event::tool_call_start(0, "call_1".to_string(), "fs_read_file".to_string()), + Event::tool_call_args(0, serde_json::to_string(&args).unwrap()), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ]; + + let followup = vec![ + Event::message(0, "Done.\n\n"), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ]; + + SequentialMockProvider { + responses: vec![events, followup], + call_index: AtomicUsize::new(0), + model: ModelDetails::empty(id::ModelIdConfig { + provider: ProviderId::Test, + name: "fan-out-mock".parse().expect("valid name"), + }), + } + }); + + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let router = detached_router(); + + // Counts executions rather than trusting the response text: an + // implementation that ran one operation and printed three sections + // would satisfy the output assertions below but not this one. + let runs = Arc::new(AtomicUsize::new(0)); + let executor_source = TestExecutorSource::new().with_executor("fs_read_file", { + let runs = Arc::clone(&runs); + move |req| { + runs.fetch_add(1, Ordering::SeqCst); + let path = req + .arguments + .get("path") + .and_then(Value::as_str) + .unwrap_or("") + .to_owned(); + + Box::new( + MockExecutor::completed(&req.id, &req.name, &format!("contents of {path}")) + .with_arguments(req.arguments.clone()), + ) + } + }); + let tool_defs = executor_source.tool_definitions(); + + run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &router, + &mcp_client, + root, + false, // interactive + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + chat_request.clone(), + InvocationContext::default(), + PendingStreamTrim::default(), + router.turn_interrupt(lock.id()), + ) + .await + .unwrap(); + + printer.flush(); + + assert_eq!( + runs.load(Ordering::SeqCst), + 3, + "the tool runs once per operation" + ); + + // One request in, one response out: the provider asked for one call and + // a second response would leave the stream unpaired. + let conv = lock.as_mut(); + let events = conv.events(); + let responses: Vec<_> = events + .iter() + .filter_map(|e| e.event.as_tool_call_response()) + .filter(|r| r.id == "call_1") + .collect(); + + assert_eq!(responses.len(), 1, "one call is answered once"); + assert_eq!( + responses[0].content(), + "[1/3] ok\ncontents of a.rs\n\n[2/3] ok\ncontents of b.rs\n\n[3/3] ok\ncontents of \ + c.rs\n", + "every operation's result is framed with its position" + ); + + // Three operations render as three calls, matching what a model that + // issued them separately would have shown. + let chrome = err.lock().clone(); + assert_eq!( + chrome.matches("Calling tool").count(), + 3, + "one header per operation.\nChrome:\n{chrome}" + ); + for path in ["a.rs", "b.rs", "c.rs"] { + assert!( + chrome.contains(path), + "header for {path} is missing.\nChrome:\n{chrome}" + ); + } + assert!( + !chrome.contains("ops"), + "the envelope must not reach the terminal.\nChrome:\n{chrome}" + ); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + +/// A malformed envelope answers with a message naming what went wrong, and the +/// tool never runs. +#[tokio::test] +#[expect(clippy::too_many_lines)] +async fn a_fanned_out_call_with_an_empty_envelope_is_refused() { + let test_result = Box::pin(timeout(Duration::from_secs(5), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + config + .conversation + .tools + .insert("fs_read_file".to_string(), ToolConfig { + source: ToolSource::Local { tool: None }, + command: None, + run: Some(RunMode::Unattended), + format: None, + enable: None, + summary: None, + description: None, + examples: None, + parameters: IndexMap::new(), + result: None, + style: None, + questions: IndexMap::new(), + options: IndexMap::default(), + access: None, + cancellation_response: None, + fan_out: Some(jp_config::conversation::tool::FanOutConfig { + enabled: Some(true), + concurrency: None, + on_error: None, + }), + }); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::in_memory(root).with_backend(fs.clone()); + + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let provider: Arc = Arc::new({ + let events = vec![ + Event::tool_call_start(0, "call_1".to_string(), "fs_read_file".to_string()), + Event::tool_call_args(0, r#"{"ops":[]}"#.to_owned()), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ]; + let followup = vec![ + Event::message(0, "Sorry.\n\n"), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ]; + + SequentialMockProvider { + responses: vec![events, followup], + call_index: AtomicUsize::new(0), + model: ModelDetails::empty(id::ModelIdConfig { + provider: ProviderId::Test, + name: "fan-out-empty-mock".parse().expect("valid name"), + }), + } + }); + + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let router = detached_router(); + + let runs = Arc::new(AtomicUsize::new(0)); + let executor_source = TestExecutorSource::new().with_executor("fs_read_file", { + let runs = Arc::clone(&runs); + move |req| { + runs.fetch_add(1, Ordering::SeqCst); + Box::new(MockExecutor::completed( + &req.id, + &req.name, + "should not run", + )) + } + }); + let tool_defs = executor_source.tool_definitions(); + + run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &router, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + ChatRequest::from("Read nothing"), + InvocationContext::default(), + PendingStreamTrim::default(), + router.turn_interrupt(lock.id()), + ) + .await + .unwrap(); + + assert_eq!( + runs.load(Ordering::SeqCst), + 0, + "a call with no operations runs nothing" + ); + + let conv = lock.as_mut(); + let events = conv.events(); + let response = events + .iter() + .filter_map(|e| e.event.as_tool_call_response()) + .find(|r| r.id == "call_1") + .expect("the call is answered"); + + assert_eq!( + response.result, + Err( + "Tool 'fs_read_file' was called with an empty `ops` array, so there was nothing \ + to do. Include at least one operation." + .to_owned() + ) + ); + })) + .await; + + assert!(test_result.is_ok(), "Test timed out"); +} + /// An executor that writes to the stderr sink the coordinator hands it, then /// completes. /// @@ -4945,6 +5300,7 @@ fn talking_tool_config(names: &[&str]) -> AppConfig { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); } @@ -5452,6 +5808,7 @@ async fn a_tool_can_opt_out_of_the_progress_window() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -6005,6 +6362,7 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, } } @@ -7115,6 +7473,7 @@ async fn test_parallel_tools_one_with_inquiry() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -7550,6 +7909,7 @@ async fn test_unavailable_tool_before_approved_does_not_panic() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); @@ -7982,6 +8342,7 @@ async fn reasoning_before_a_tool_call_shades_the_tool_chrome() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); diff --git a/crates/jp_cli/src/render/turn.rs b/crates/jp_cli/src/render/turn.rs index 768e6a631..06aa092f2 100644 --- a/crates/jp_cli/src/render/turn.rs +++ b/crates/jp_cli/src/render/turn.rs @@ -23,7 +23,7 @@ use jp_conversation::{ EventKind, stream::{TurnOrigin, turn_iter::Turn}, }; -use jp_llm::tool::InvocationContext; +use jp_llm::tool::{InvocationContext, fan_out}; use jp_printer::{ErrChannel, Printer}; use tracing::warn; @@ -218,8 +218,22 @@ impl TurnRenderer { self.tool.set_region(&req.id, region); if chrome_visible { - self.tool - .render_tool_call(&req.name, &req.arguments, &style.parameters); + // A fanned-out call was shown live as one header per + // operation, so replay reproduces that rather than + // printing the envelope the provider was sent. + // A call that does not fan out has one set of + // arguments and renders as the single header it always + // did. + let operations = tool_cfg + .as_ref() + .and_then(ToolConfigWithDefaults::fan_out) + .and(fan_out::expand(&req.arguments).ok()) + .unwrap_or_else(|| vec![req.arguments.clone()]); + + for arguments in &operations { + self.tool + .render_tool_call(&req.name, arguments, &style.parameters); + } // Show stored custom-formatter output when replaying // a tool call that was originally rendered with a diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index d09eeb8c0..be657a799 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -538,6 +538,18 @@ pub struct ToolConfig { /// tool to default-deny. #[setting(nested)] pub access: Option, + + /// Whether one call to this tool may carry several independent operations. + /// + /// When set, the tool's arguments are wrapped in an `ops` array whose + /// elements each hold one complete set of the tool's own arguments. + /// JP runs them as separate calls and folds the results into one response; + /// the tool itself still receives one operation at a time. + /// + /// Accepts a bool or a `{ enabled, concurrency, on_error }` table. + /// When unset, the tool takes one operation per call. + #[setting(nested)] + pub fan_out: Option, } impl AssignKeyValue for PartialToolConfig { @@ -560,6 +572,8 @@ impl AssignKeyValue for PartialToolConfig { "questions" => self.questions = kv.try_object()?, _ if kv.p("options") => kv.assign_to_entry(&mut self.options)?, _ if kv.p("access") => self.access.assign(kv)?, + "fan_out" => self.fan_out = kv.try_some_object_bool_or_from_str()?, + _ if kv.p("fan_out") => self.fan_out.assign(kv)?, _ => return missing_key(&kv), } @@ -597,6 +611,7 @@ impl PartialConfigDelta for PartialToolConfig { }) .collect(), access: delta_opt_partial(self.access.as_ref(), next.access), + fan_out: delta_opt_partial(self.fan_out.as_ref(), next.fan_out), } } } @@ -636,6 +651,7 @@ impl ToPartial for ToolConfig { .map(|(k, v)| (k.clone(), v.clone())) .collect(), access: partial_opt_config(self.access.as_ref(), defaults.access), + fan_out: partial_opt_config(self.fan_out.as_ref(), defaults.fan_out), } } } @@ -1251,6 +1267,19 @@ impl ToolConfigWithDefaults { None } + /// Return the fan-out policy for the tool, or `None` when one call carries + /// exactly one operation. + /// + /// Read from the tool's own config only. + /// There is no `'*'` default: fan-out changes the shape of a tool's + /// arguments, and a blanket key would rewrite the schema of every tool at + /// once, including the ones already shaped to take many targets in a single + /// call. + #[must_use] + pub fn fan_out(&self) -> Option { + self.tool.fan_out.as_ref()?.to_partial().effective() + } + /// Return the question target for the given question ID. #[must_use] pub fn question_target(&self, question_id: &str) -> Option<&QuestionTarget> { @@ -1675,6 +1704,287 @@ impl ToPartial for EnableConfig { } } +/// What happens to the operations after one of them fails. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize, ConfigEnum)] +#[serde(rename_all = "snake_case")] +pub enum FanOutOnError { + /// Run every operation, whatever the others do. + /// + /// Each failure is reported in its own section of the folded result. + #[default] + Continue, + + /// Start no further operations once one has failed. + /// + /// Operations already in flight run to completion; nothing is aborted. + /// The folded result names the operations that never started. + Stop, +} + +/// Resolved fan-out policy for a tool: how many operations may run at once and +/// what happens after one fails. +/// +/// Produced on demand by [`ToolConfigWithDefaults::fan_out`]; never stored +/// directly. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +pub struct FanOut { + /// Maximum operations in flight at once. + /// + /// `None` is unbounded. + pub concurrency: Option, + + /// What happens to the remaining operations after one fails. + pub on_error: FanOutOnError, +} + +impl FanOut { + /// Returns whether operations run strictly one at a time, in the order the + /// assistant wrote them. + #[must_use] + pub const fn is_sequential(&self) -> bool { + matches!(self.concurrency, Some(1)) + } + + /// Returns whether a failure stops the operations that have not started. + #[must_use] + pub const fn stops_on_error(&self) -> bool { + matches!(self.on_error, FanOutOnError::Stop) + } +} + +/// Whether a single tool call may carry several independent operations, and how +/// they run. +/// +/// A tool with fan-out enabled takes an `ops` array whose elements each hold +/// one complete set of the tool's own arguments. +/// JP runs them as if the assistant had issued separate calls, and folds the +/// results back into one response. +/// The tool itself is unchanged: it still receives one operation's arguments +/// per invocation. +/// +/// ```toml +/// # Bool shorthand: enabled, unbounded, every failure reported. +/// fan_out = true +/// +/// # Table form. Omitted fields fall back to unbounded and `continue`. +/// fan_out = { concurrency = 1, on_error = "stop" } # ordered writes +/// fan_out = { concurrency = 4 } # rate-limited endpoint +/// ``` +/// +/// Omit the key entirely to leave the tool's schema untouched. +/// Tools that already accept several targets in one call (`fs_modify_file`, +/// `bash`) are a different shape and should not set this. +#[derive(Debug, Clone, PartialEq, Config)] +#[config( + rename_all = "snake_case", + no_deserialize_derive, + schema_union_with = fan_out_input_shapes +)] +pub struct FanOutConfig { + /// Whether the tool accepts several operations in one call. + /// + /// Defaults to `true` when the table form is used, so naming any other + /// field turns fan-out on. + pub enabled: Option, + + /// Maximum operations in flight at once. + /// + /// Defaults to unbounded. + /// Set to `1` to run them one at a time, in the order the assistant wrote + /// them, which is what an ordered sequence of writes needs. + /// Set to a small number for an endpoint that rate-limits. + pub concurrency: Option, + + /// What happens to the remaining operations after one fails. + /// + /// - `continue` (the default): every operation runs, and each failure is + /// reported in its own section of the result. + /// - `stop`: no further operation is started. + /// Operations already running finish; nothing is aborted. + /// The result names the ones that never ran. + pub on_error: Option, +} + +impl PartialFanOutConfig { + /// Fan-out on, unbounded, reporting every failure. + pub const ON: Self = Self { + enabled: Some(true), + concurrency: None, + on_error: None, + }; + + /// Fan-out off. + pub const OFF: Self = Self { + enabled: Some(false), + concurrency: None, + on_error: None, + }; + + /// Resolve into the effective [`FanOut`], or `None` when fan-out is off. + /// + /// A `concurrency` of `0` is read as unbounded rather than as "never run + /// anything": a tool that accepts operations and then runs none of them is + /// not a state any configuration should be able to express by accident. + #[must_use] + pub fn effective(&self) -> Option { + if !self.enabled.unwrap_or(true) { + return None; + } + + Some(FanOut { + concurrency: self.concurrency.filter(|v| *v > 0), + on_error: self.on_error.unwrap_or_default(), + }) + } +} + +/// The non-table shapes `fan_out` accepts, for the schema. +/// +/// The table form is described by the derived struct schema; the bool shorthand +/// is what its hand-written `Deserialize` also accepts, which the derive cannot +/// see. +fn fan_out_input_shapes(schema: &schematic::SchemaBuilder) -> Vec { + use schematic::schema::BooleanType; + + vec![schema.nest().boolean(BooleanType::default())] +} + +impl From for PartialFanOutConfig { + fn from(enabled: bool) -> Self { + Self { + enabled: Some(enabled), + concurrency: None, + on_error: None, + } + } +} + +impl FromStr for PartialFanOutConfig { + type Err = String; + + fn from_str(s: &str) -> Result { + Ok(match s { + "true" | "on" => Self::ON, + "false" | "off" => Self::OFF, + _ => { + return Err(format!( + "invalid fan_out value: '{s}', expected a boolean or a {{ enabled, \ + concurrency, on_error }} table" + )); + } + }) + } +} + +impl AssignKeyValue for PartialFanOutConfig { + fn assign(&mut self, kv: KvAssignment) -> AssignResult { + match kv.key_string().as_str() { + "" => *self = kv.try_object_bool_or_from_str()?, + "enabled" => self.enabled = kv.try_some_bool()?, + "concurrency" => self.concurrency = kv.try_some_from_str()?, + "on_error" => self.on_error = kv.try_some_from_str()?, + _ => return missing_key(&kv), + } + + Ok(()) + } +} + +impl PartialConfigDelta for PartialFanOutConfig { + fn delta(&self, next: Self) -> Self { + Self { + enabled: delta_opt(self.enabled.as_ref(), next.enabled), + concurrency: delta_opt(self.concurrency.as_ref(), next.concurrency), + on_error: delta_opt(self.on_error.as_ref(), next.on_error), + } + } +} + +impl ToPartial for FanOutConfig { + fn to_partial(&self) -> Self::Partial { + PartialFanOutConfig { + enabled: self.enabled, + concurrency: self.concurrency, + on_error: self.on_error, + } + } +} + +/// Accept a bool or an `{ enabled, concurrency, on_error }` table on input. +/// Output is always the table form (auto-derived), matching how +/// [`PartialEnableConfig`] handles its own shorthand. +impl<'de> Deserialize<'de> for PartialFanOutConfig { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct FanOutVisitor; + + impl<'de> serde::de::Visitor<'de> for FanOutVisitor { + type Value = PartialFanOutConfig; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a boolean or an { enabled, concurrency, on_error } table") + } + + fn visit_bool(self, v: bool) -> Result { + Ok(PartialFanOutConfig::from(v)) + } + + fn visit_str(self, v: &str) -> Result { + v.parse().map_err(serde::de::Error::custom) + } + + fn visit_map(self, mut map: A) -> Result + where + A: serde::de::MapAccess<'de>, + { + let mut enabled: Option = None; + let mut concurrency: Option = None; + let mut on_error: Option = None; + + while let Some(key) = map.next_key::()? { + match key.as_str() { + "enabled" => { + if enabled.is_some() { + return Err(serde::de::Error::duplicate_field("enabled")); + } + enabled = Some(map.next_value()?); + } + "concurrency" => { + if concurrency.is_some() { + return Err(serde::de::Error::duplicate_field("concurrency")); + } + concurrency = Some(map.next_value()?); + } + "on_error" => { + if on_error.is_some() { + return Err(serde::de::Error::duplicate_field("on_error")); + } + on_error = Some(map.next_value()?); + } + other => { + return Err(serde::de::Error::unknown_field(other, &[ + "enabled", + "concurrency", + "on_error", + ])); + } + } + } + + Ok(PartialFanOutConfig { + enabled, + concurrency, + on_error, + }) + } + } + + deserializer.deserialize_any(FanOutVisitor) + } +} + /// Accept a bool, a legacy string, or a `{ state, allow_toggle }` table on /// input. /// Output is always the table form (auto-derived), mirroring how diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index dd4c2fd57..b1a51897c 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -1494,3 +1494,201 @@ fn test_delta_enable_records_only_changed_subfield() { }) ); } + +#[test] +fn fan_out_bool_shorthand_means_enabled_and_unbounded() { + let partial: PartialFanOutConfig = serde_json::from_value(json!(true)).expect("parses"); + + assert_eq!(partial, PartialFanOutConfig::ON); + assert_eq!( + partial.effective(), + Some(FanOut { + concurrency: None, + on_error: FanOutOnError::Continue, + }) + ); +} + +#[test] +fn fan_out_false_resolves_to_no_fan_out() { + let partial: PartialFanOutConfig = serde_json::from_value(json!(false)).expect("parses"); + + assert_eq!(partial.effective(), None); +} + +#[test] +fn fan_out_table_form_is_enabled_without_naming_enabled() { + let partial: PartialFanOutConfig = + serde_json::from_value(json!({ "concurrency": 1, "on_error": "stop" })).expect("parses"); + + let resolved = partial.effective().expect("the table form enables fan-out"); + assert_eq!(resolved, FanOut { + concurrency: Some(1), + on_error: FanOutOnError::Stop, + }); + assert!(resolved.is_sequential()); + assert!(resolved.stops_on_error()); +} + +/// A tool that accepts operations and then runs none of them is not a state any +/// configuration should reach by accident, so `0` reads as unbounded. +#[test] +fn fan_out_concurrency_of_zero_reads_as_unbounded() { + let partial: PartialFanOutConfig = + serde_json::from_value(json!({ "concurrency": 0 })).expect("parses"); + + assert_eq!( + partial.effective(), + Some(FanOut { + concurrency: None, + on_error: FanOutOnError::Continue, + }) + ); +} + +#[test] +fn fan_out_rejects_an_unknown_field() { + let err = serde_json::from_value::(json!({ "mode": "sequential" })) + .expect_err("an unknown field is rejected"); + + assert!( + err.to_string().contains("unknown field `mode`"), + "unexpected error: {err}" + ); +} + +#[test] +fn fan_out_resolves_from_toml_through_the_loader() { + let loaded: PartialAppConfig = toml::from_str( + r#" +[conversation.tools.'*'] +run = "unattended" + +[conversation.tools.reader] +source = "local" +fan_out = true + +[conversation.tools.writer] +source = "local" +fan_out = { concurrency = 1, on_error = "stop" } + +[conversation.tools.plain] +source = "local" +"#, + ) + .expect("the fixture parses"); + + let mut partial = PartialAppConfig::new_test(); + partial.conversation.tools = loaded.conversation.tools; + + let config = build(partial).expect("the fixture resolves"); + let tools = &config.conversation.tools; + + assert_eq!( + tools.get("reader").expect("reader is configured").fan_out(), + Some(FanOut { + concurrency: None, + on_error: FanOutOnError::Continue, + }) + ); + assert_eq!( + tools.get("writer").expect("writer is configured").fan_out(), + Some(FanOut { + concurrency: Some(1), + on_error: FanOutOnError::Stop, + }) + ); + assert_eq!( + tools.get("plain").expect("plain is configured").fan_out(), + None, + "a tool that says nothing takes one operation per call" + ); +} + +/// Fan-out rewrites a tool's argument shape, so a `'*'` block must not reach a +/// tool that never asked for it. +#[test] +fn fan_out_is_not_inherited_from_the_defaults_block() { + let loaded: PartialAppConfig = toml::from_str( + r#" +[conversation.tools.'*'] +run = "unattended" + +[conversation.tools.plain] +source = "local" +"#, + ) + .expect("the fixture parses"); + + let mut partial = PartialAppConfig::new_test(); + partial.conversation.tools = loaded.conversation.tools; + + let config = build(partial).expect("the fixture resolves"); + + assert_eq!( + config + .conversation + .tools + .get("plain") + .expect("plain is configured") + .fan_out(), + None + ); +} + +#[test] +fn fan_out_assign_kv_accepts_the_shorthand_and_the_subfields() { + use crate::assignment::KvAssignment; + + let mut tool = PartialToolConfig::default(); + + tool.assign(KvAssignment::try_from_cli("fan_out", "true").expect("parses")) + .expect("assigns"); + assert_eq!(tool.fan_out, Some(PartialFanOutConfig::ON)); + + tool.assign(KvAssignment::try_from_cli("fan_out.concurrency", "4").expect("parses")) + .expect("assigns"); + tool.assign(KvAssignment::try_from_cli("fan_out.on_error", "stop").expect("parses")) + .expect("assigns"); + + assert_eq!( + tool.fan_out + .as_ref() + .and_then(PartialFanOutConfig::effective), + Some(FanOut { + concurrency: Some(4), + on_error: FanOutOnError::Stop, + }) + ); +} + +#[test] +fn fan_out_delta_records_only_the_changed_subfield() { + use crate::delta::PartialConfigDelta as _; + + let prev = PartialToolConfig { + fan_out: Some(PartialFanOutConfig { + enabled: Some(true), + concurrency: Some(4), + on_error: Some(FanOutOnError::Continue), + }), + ..Default::default() + }; + let next = PartialToolConfig { + fan_out: Some(PartialFanOutConfig { + enabled: Some(true), + concurrency: Some(1), + on_error: Some(FanOutOnError::Continue), + }), + ..Default::default() + }; + + assert_eq!( + prev.delta(next).fan_out, + Some(PartialFanOutConfig { + enabled: None, + concurrency: Some(1), + on_error: None, + }) + ); +} diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap index 8bd969950..fb789e9b5 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap @@ -543,6 +543,14 @@ conversation: ConversationConfig state: bool | null |: null examples: string | null + fan_out: + |: + |: bool + | (expanded): FanOutConfig + concurrency: int | null + enabled: bool | null + on_error: "continue" | "stop" | null + |: null format: "ask" | "unattended" | null options: *: unknown diff --git a/crates/jp_llm/src/provider/anthropic.rs b/crates/jp_llm/src/provider/anthropic.rs index c55abbe5d..3c814d363 100644 --- a/crates/jp_llm/src/provider/anthropic.rs +++ b/crates/jp_llm/src/provider/anthropic.rs @@ -2158,21 +2158,24 @@ fn convert_tools( let mut tools: Vec<_> = tools .into_iter() .map(|tool| { + // The document arrives as its source declared it. `$ref` and + // `$defs` are supported, so they pass through; strict mode + // additionally requires the closed-object subset, which + // `transform_schema` produces. + let document = tool + .provider_schema() + .as_object() + .cloned() + .unwrap_or_default(); + types::Tool::Custom(types::CustomTool { name: tool.name, description: tool.docs.schema_description().map(str::to_owned), strict: strict.then_some(true), - input_schema: { - // The document arrives as its source declared it. `$ref` - // and `$defs` are supported, so they pass through; strict - // mode additionally requires the closed-object subset, - // which `transform_schema` produces. - let document = tool.parameters.as_object().cloned().unwrap_or_default(); - if strict { - transform_schema(document).into() - } else { - document.into() - } + input_schema: if strict { + transform_schema(document).into() + } else { + document.into() }, cache_control: None, }) diff --git a/crates/jp_llm/src/provider/anthropic_tests.rs b/crates/jp_llm/src/provider/anthropic_tests.rs index a89f705f6..893a46db4 100644 --- a/crates/jp_llm/src/provider/anthropic_tests.rs +++ b/crates/jp_llm/src/provider/anthropic_tests.rs @@ -1330,6 +1330,7 @@ fn test_forced_tool_with_reasoning_returns_fallback() { tools: vec![ToolDefinition { name: "my_tool".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }], tool_choice: ToolChoice::Function("my_tool".into()), @@ -1402,6 +1403,7 @@ fn test_forced_tool_thinking_always_on_uses_escalating_nudge() { tools: vec![ToolDefinition { name: "my_tool".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }], tool_choice: ToolChoice::Function("my_tool".into()), @@ -1486,6 +1488,7 @@ fn test_forced_tool_thinking_always_on_reasoning_off_still_soft_forces() { tools: vec![ToolDefinition { name: "my_tool".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }], tool_choice: ToolChoice::Function("my_tool".into()), @@ -1545,11 +1548,13 @@ fn test_forced_tool_function_multi_tool_preserves_name() { ToolDefinition { name: "read_file".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }, ToolDefinition { name: "commit".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }, ], @@ -1618,6 +1623,7 @@ fn test_forced_tool_without_reasoning_no_fallback() { tools: vec![ToolDefinition { name: "my_tool".into(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": {} }), }], tool_choice: ToolChoice::Required, diff --git a/crates/jp_llm/src/provider/cerebras.rs b/crates/jp_llm/src/provider/cerebras.rs index 207c4f3d7..fff6d04b1 100644 --- a/crates/jp_llm/src/provider/cerebras.rs +++ b/crates/jp_llm/src/provider/cerebras.rs @@ -764,12 +764,14 @@ fn convert_tools(tools: Vec) -> Vec { tools .into_iter() .map(|tool| { + let parameters = parameters_with_strict_mode(&tool.provider_schema(), false); + json!({ "type": "function", "function": { "name": tool.name, "description": tool.docs.schema_description().unwrap_or_default(), - "parameters": parameters_with_strict_mode(&tool.parameters, false), + "parameters": parameters, }, }) }) diff --git a/crates/jp_llm/src/provider/google.rs b/crates/jp_llm/src/provider/google.rs index 4b31a84b7..dd685cb46 100644 --- a/crates/jp_llm/src/provider/google.rs +++ b/crates/jp_llm/src/provider/google.rs @@ -1047,10 +1047,12 @@ fn convert_tools(tools: Vec) -> Vec { tools .into_iter() .map(|tool| { + let schema = tool.provider_schema().into_owned(); + types::Tool::FunctionDeclaration(types::ToolConfigFunctionDeclaration { function_declarations: vec![types::FunctionDeclaration { parameters: None, - parameters_json_schema: Some(closed_object_schema(tool.parameters)), + parameters_json_schema: Some(closed_object_schema(schema)), name: tool.name, description: tool .docs diff --git a/crates/jp_llm/src/provider/llamacpp.rs b/crates/jp_llm/src/provider/llamacpp.rs index db10c0545..d2a1dd65a 100644 --- a/crates/jp_llm/src/provider/llamacpp.rs +++ b/crates/jp_llm/src/provider/llamacpp.rs @@ -627,12 +627,14 @@ fn convert_tools(tools: Vec, tool_choice: &ToolChoice) -> Vec) -> Result> { tools .into_iter() .map(|tool| { - let parameters = json_schema::inline(&tool.parameters) + let parameters = json_schema::inline(&tool.provider_schema()) .as_object() .cloned() .unwrap_or_default(); diff --git a/crates/jp_llm/src/provider/ollama_tests.rs b/crates/jp_llm/src/provider/ollama_tests.rs index 55f0cc375..09e94f60c 100644 --- a/crates/jp_llm/src/provider/ollama_tests.rs +++ b/crates/jp_llm/src/provider/ollama_tests.rs @@ -10,6 +10,7 @@ fn tool_references_are_expanded() { let tools = vec![ToolDefinition { name: "crate_search_items".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": { @@ -45,6 +46,7 @@ fn tool_parameters_keep_the_schema_document() { let tools = vec![ToolDefinition { name: "read_file".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": { "path": { "type": "string" } }, diff --git a/crates/jp_llm/src/provider/openai.rs b/crates/jp_llm/src/provider/openai.rs index e10f22469..dddb5fc54 100644 --- a/crates/jp_llm/src/provider/openai.rs +++ b/crates/jp_llm/src/provider/openai.rs @@ -2018,18 +2018,22 @@ fn convert_tools(tools: Vec) -> Vec { tools .into_iter() .map(|tool| { + let schema = tool.provider_schema(); + // The strict subset requires a type on every property, which a // parameter the server left free-form does not have. Dropping // strict mode for that one tool costs its adherence guarantee; // sending it strict costs the whole request, and every other tool // in it. - let strict = !json_schema::has_unconstrained_node(&tool.parameters); + let strict = !json_schema::has_unconstrained_node(&schema); + let parameters = parameters_with_strict_mode(&schema, strict).into(); + drop(schema); types::Tool::Function { name: tool.name, strict, description: tool.docs.schema_description().map(str::to_owned), - parameters: parameters_with_strict_mode(&tool.parameters, strict).into(), + parameters, } }) .collect() diff --git a/crates/jp_llm/src/provider/openai_tests.rs b/crates/jp_llm/src/provider/openai_tests.rs index 2e666f9e3..925abd4e8 100644 --- a/crates/jp_llm/src/provider/openai_tests.rs +++ b/crates/jp_llm/src/provider/openai_tests.rs @@ -331,6 +331,7 @@ mod convert_tools { let tools = convert_tools(vec![ToolDefinition { name: "store".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters, }]); diff --git a/crates/jp_llm/src/provider/openrouter.rs b/crates/jp_llm/src/provider/openrouter.rs index be47d0ca3..a97d85521 100644 --- a/crates/jp_llm/src/provider/openrouter.rs +++ b/crates/jp_llm/src/provider/openrouter.rs @@ -768,13 +768,17 @@ fn create_request( let mut messages: RequestMessages = (&model.id, thread).try_into()?; let tools = tools .into_iter() - .map(|tool| Tool::Function { - function: ToolFunction { - parameters: parameters_with_strict_mode(&tool.parameters, true), - name: tool.name, - description: tool.docs.schema_description().map(str::to_owned), - strict: true, - }, + .map(|tool| { + let parameters = parameters_with_strict_mode(&tool.provider_schema(), true); + + Tool::Function { + function: ToolFunction { + parameters, + name: tool.name, + description: tool.docs.schema_description().map(str::to_owned), + strict: true, + }, + } }) .collect::>(); let thinking_active = reasoning.is_some() diff --git a/crates/jp_llm/src/test.rs b/crates/jp_llm/src/test.rs index dfb3524f8..51eb6f49f 100644 --- a/crates/jp_llm/src/test.rs +++ b/crates/jp_llm/src/test.rs @@ -103,6 +103,7 @@ mod harness_tests { vec![ToolDefinition { name: "run_me".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: json!({ "type": "object", "properties": { "foo": { "type": "string" } }, @@ -340,6 +341,7 @@ impl TestRequest { query.tools.push(ToolDefinition { name: name.into(), docs: ToolDocs::default(), + fan_out: None, parameters, }); } diff --git a/crates/jp_llm/src/tool.rs b/crates/jp_llm/src/tool.rs index da9db388f..e80088380 100644 --- a/crates/jp_llm/src/tool.rs +++ b/crates/jp_llm/src/tool.rs @@ -2,15 +2,17 @@ pub mod builtin; pub mod executor; +pub mod fan_out; pub mod json_schema; -use std::{ffi::OsStr, fmt, process::Stdio, sync::Arc}; +use std::{borrow::Cow, ffi::OsStr, fmt, process::Stdio, sync::Arc}; pub use builtin::BuiltinTool; use camino::Utf8Path; +use fan_out::{FAN_OUT_DESCRIPTION, envelope as fan_out_envelope}; use indexmap::IndexMap; use jp_config::{ - conversation::tool::{CommandConfig, ToolConfigWithDefaults, ToolSource}, + conversation::tool::{CommandConfig, FanOut, ToolConfigWithDefaults, ToolSource}, types::command::shell_command_line, }; use jp_conversation::event::ToolCallResponse; @@ -717,14 +719,41 @@ pub struct ToolDefinition { pub name: String, pub docs: ToolDocs, - /// JSON Schema for the tool's arguments, as its source declared it, with - /// configuration overrides applied. + /// JSON Schema for **one operation's** arguments, as the tool's source + /// declared it, with configuration overrides applied. + /// + /// Argument coercion, defaults, and validation all run against this, so a + /// tool receives and validates the same shape whether or not it fans out. + /// Use [`provider_schema`] for the document sent to the LLM. /// /// Adapting this to what a given API accepts belongs to that provider. + /// + /// [`provider_schema`]: Self::provider_schema pub parameters: Value, + + /// Whether one call may carry several operations, and how they run. + /// + /// `Some` means the provider is shown the fan-out envelope rather than + /// `parameters` directly. + pub fan_out: Option, } impl ToolDefinition { + /// The JSON Schema shown to the LLM provider. + /// + /// Without fan-out this is [`parameters`] unchanged. + /// With fan-out it is the envelope: an object holding a single required + /// `ops` array whose items are [`parameters`]. + /// + /// [`parameters`]: Self::parameters + #[must_use] + pub fn provider_schema(&self) -> Cow<'_, Value> { + match self.fan_out { + None => Cow::Borrowed(&self.parameters), + Some(_) => Cow::Owned(fan_out_envelope(&self.parameters)), + } + } + /// Coerce JSON-encoded argument strings to non-string schema types. /// /// Strings stay unchanged when the schema accepts strings or their contents @@ -1302,19 +1331,31 @@ async fn resolve_tool( mcp_client: &jp_mcp::Client, ) -> Result { let path = format!("conversation.tools.{name}.parameters"); - let definition = match config.source() { + let mut definition = match config.source() { ToolSource::Local { .. } | ToolSource::Builtin { .. } => ToolDefinition { name: name.to_owned(), docs: ToolDocs::from_config(config), parameters: json_schema::from_config(&path, config.parameters())?, + fan_out: None, }, ToolSource::Mcp { server, tool } => { resolve_mcp_tool(server, name, tool.as_deref(), config, mcp_client).await? } }; + // Validated before the envelope is attached: what a tool must declare is a + // property of the operation it performs, and the envelope is JP's own + // construction rather than anything the tool's source said. json_schema::validate(&path, &definition.parameters)?; + if let Some(fan_out) = config.fan_out() { + definition.fan_out = Some(fan_out); + definition.docs.summary = Some(match definition.docs.summary.take() { + Some(summary) => format!("{summary} {FAN_OUT_DESCRIPTION}"), + None => FAN_OUT_DESCRIPTION.to_owned(), + }); + } + Ok(definition) } @@ -1423,6 +1464,9 @@ async fn resolve_mcp_tool( name: name.to_owned(), docs, parameters, + // Attached by `resolve_tool` once the schema has been validated; an MCP + // server has no say in whether JP batches calls to it. + fan_out: None, }) } diff --git a/crates/jp_llm/src/tool/executor.rs b/crates/jp_llm/src/tool/executor.rs index adcf78946..b61f3cc4a 100644 --- a/crates/jp_llm/src/tool/executor.rs +++ b/crates/jp_llm/src/tool/executor.rs @@ -27,11 +27,34 @@ use super::{StderrSink, ToolDefinition}; #[async_trait] pub trait Executor: Send + Sync { /// Returns the tool call ID. + /// + /// Every operation of a fanned-out call shares one id, because the provider + /// asked for one call and expects one response. fn tool_id(&self) -> &str; /// Returns the tool name. fn tool_name(&self) -> &str; + /// Position of this executor's operation within its tool call. + /// + /// `None` when the call carries exactly one operation, which is every call + /// to a tool without fan-out configured. + fn op_index(&self) -> Option { + None + } + + /// Key identifying this operation's display state. + /// + /// A fanned-out call renders one line per operation and prompts once per + /// operation, so each needs a slot of its own rather than sharing the one + /// its tool call id would name. + fn state_key(&self) -> String { + match self.op_index() { + None => self.tool_id().to_owned(), + Some(op) => format!("{}#{op}", self.tool_id()), + } + } + /// Returns the tool call arguments. /// /// This is separate from [`permission_info()`] because arguments are always @@ -92,7 +115,13 @@ pub trait Executor: Send + Sync { /// This trait enables dependency injection of executor creation, allowing tests /// to use mock executors without executing real shell commands. pub trait ExecutorSource: Send + Sync { - /// Creates an executor for the given tool call request. + /// Creates an executor for one operation of the given tool call request. + /// + /// `request.arguments` holds that operation's arguments, already taken out + /// of the fan-out envelope by the caller, so an implementation never sees + /// the envelope itself. + /// `op` is the operation's position within the call, or `None` when the + /// call carries exactly one operation. /// /// Returns `None` if the tool cannot be resolved (e.g. missing from the /// definitions). @@ -100,6 +129,7 @@ pub trait ExecutorSource: Send + Sync { &self, request: ToolCallRequest, config: ToolConfigWithDefaults, + op: Option, ) -> Option>; } @@ -320,6 +350,7 @@ impl TestExecutorSource { .map(|name| ToolDefinition { name: name.clone(), docs: super::ToolDocs::default(), + fan_out: None, parameters: serde_json::json!({ "type": "object", "properties": {} }), }) .collect() @@ -337,9 +368,68 @@ impl ExecutorSource for TestExecutorSource { &self, request: ToolCallRequest, _config: ToolConfigWithDefaults, + op: Option, ) -> Option> { let factory = self.factories.get(&request.name)?; - Some(factory(request)) + let executor = factory(request); + + Some(match op { + None => executor, + Some(op) => Box::new(OpExecutor { + inner: executor, + op, + }), + }) + } +} + +/// Wraps a test executor so it reports the operation it stands for. +/// +/// Test factories build one executor from one request and know nothing about +/// fan-out; this carries the operation index the source was asked for without +/// every factory having to thread it through. +struct OpExecutor { + inner: Box, + op: usize, +} + +#[async_trait] +impl Executor for OpExecutor { + fn tool_id(&self) -> &str { + self.inner.tool_id() + } + + fn tool_name(&self) -> &str { + self.inner.tool_name() + } + + fn op_index(&self) -> Option { + Some(self.op) + } + + fn arguments(&self) -> &Map { + self.inner.arguments() + } + + fn permission_info(&self) -> Option { + self.inner.permission_info() + } + + fn set_arguments(&mut self, args: Value) { + self.inner.set_arguments(args); + } + + async fn execute( + &self, + answers: &IndexMap, + mcp_client: &Client, + root: &Utf8Path, + cancellation_token: CancellationToken, + stderr: Option, + ) -> ExecutorResult { + self.inner + .execute(answers, mcp_client, root, cancellation_token, stderr) + .await } } @@ -350,8 +440,18 @@ impl ExecutorSource for TestExecutorSource { #[derive(Debug, Clone)] pub struct PermissionInfo { /// The tool call ID. + /// + /// Shared by every operation of a fanned-out call; use [`state_key`] to + /// address one operation's display state. + /// + /// [`state_key`]: Self::state_key pub tool_id: String, + /// Key identifying this operation's display state. + /// + /// Matches [`Executor::state_key`] for the executor this info came from. + pub state_key: String, + /// The tool name. pub tool_name: String, diff --git a/crates/jp_llm/src/tool/fan_out.rs b/crates/jp_llm/src/tool/fan_out.rs new file mode 100644 index 000000000..c359176a8 --- /dev/null +++ b/crates/jp_llm/src/tool/fan_out.rs @@ -0,0 +1,210 @@ +//! Fan-out: one tool call carrying several independent operations. +//! +//! A tool with fan-out enabled is shown an envelope schema instead of its own: +//! an object holding a single [`FAN_OUT_KEY`] array whose elements each hold +//! one complete set of the tool's arguments. +//! The tool implementation is untouched, and still receives one operation's +//! arguments per invocation. +//! +//! This module owns both halves of that translation: [`envelope`] builds the +//! schema the provider sees, and [`expand`] takes a call's arguments back apart +//! into the operations to run. +//! +//! Result folding lives with the caller that collects the responses, not here. + +use jp_config::conversation::tool::FanOut; +use serde_json::{Map, Value, json}; + +/// The envelope's only property: the array of operations to run. +pub const FAN_OUT_KEY: &str = "ops"; + +/// Sentence appended to a fan-out tool's description, telling the model how the +/// envelope relates to the per-operation documentation it already has. +/// +/// The tool's own `examples` need no rewrite: each one already shows exactly +/// one operation, which is the shape of one element. +pub const FAN_OUT_DESCRIPTION: &str = "This tool accepts several operations in a single call. Put \ + each one in the `ops` array as its own complete object; \ + the documented parameters and examples describe one \ + element. Batch every operation you already know you need \ + into one call rather than issuing them one at a time."; + +/// Build the envelope schema wrapping a tool's per-operation schema. +/// +/// The result is always an object with one required array property, whatever +/// shape `operation` has. +#[must_use] +pub fn envelope(operation: &Value) -> Value { + json!({ + "type": "object", + "properties": { + FAN_OUT_KEY: { + "type": "array", + "minItems": 1, + "description": "The operations to perform. Each element is one complete set of \ + this tool's arguments.", + "items": operation.clone(), + } + }, + "required": [FAN_OUT_KEY], + "additionalProperties": false, + }) +} + +/// Why a call's arguments could not be taken apart into operations. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ExpandError { + /// The `ops` key is absent. + Missing, + + /// `ops` is present but is not an array. + NotAnArray, + + /// `ops` is an array with nothing in it. + Empty, + + /// An element of `ops` is not an object. + ElementNotAnObject { + /// Zero-based position of the offending element. + index: usize, + }, +} + +impl ExpandError { + /// The message handed back to the assistant. + /// + /// Each one names the envelope explicitly, because the model reaching this + /// point has the envelope schema in front of it and got the shape wrong. + #[must_use] + pub fn message(&self, tool_name: &str) -> String { + match self { + Self::Missing => format!( + "Tool '{tool_name}' takes its arguments in an `{FAN_OUT_KEY}` array, but the call \ + had no `{FAN_OUT_KEY}` key. Wrap the arguments in one: {{\"{FAN_OUT_KEY}\": \ + [{{...}}]}}." + ), + Self::NotAnArray => format!( + "Tool '{tool_name}' expects `{FAN_OUT_KEY}` to be an array of operations, and the \ + call gave it something else." + ), + Self::Empty => format!( + "Tool '{tool_name}' was called with an empty `{FAN_OUT_KEY}` array, so there was \ + nothing to do. Include at least one operation." + ), + Self::ElementNotAnObject { index } => format!( + "Tool '{tool_name}' expects every element of `{FAN_OUT_KEY}` to be an object \ + holding one operation's arguments; element {index} was not." + ), + } + } +} + +/// Take a fan-out call's arguments apart into one argument map per operation. +/// +/// The returned maps are what the tool is actually invoked with, so each is the +/// shape the tool's own schema describes. +/// +/// A call that omits the envelope entirely but looks like a single operation is +/// **not** accepted: a tool whose schema says `ops` and receives `path` has +/// been called wrongly, and silently running it would hide the mistake from the +/// model that made it. +/// +/// # Errors +/// +/// Returns [`ExpandError`] when the envelope is absent, is not an array, is +/// empty, or holds a non-object element. +pub fn expand(arguments: &Map) -> Result>, ExpandError> { + let Some(value) = arguments.get(FAN_OUT_KEY) else { + return Err(ExpandError::Missing); + }; + + let Some(items) = value.as_array() else { + return Err(ExpandError::NotAnArray); + }; + + if items.is_empty() { + return Err(ExpandError::Empty); + } + + items + .iter() + .enumerate() + .map(|(index, item)| match item { + Value::Object(map) => Ok(map.clone()), + _ => Err(ExpandError::ElementNotAnObject { index }), + }) + .collect() +} + +/// How one operation ended, for the folded result. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum OperationOutcome { + /// The operation ran and succeeded. + Ok(String), + + /// The operation ran and reported an error. + Error(String), + + /// The operation never started, because an earlier one failed under + /// `on_error = "stop"`. + NotRun { + /// One-based position of the operation whose failure stopped the rest. + after: usize, + }, +} + +/// Fold per-operation outcomes into the single body the assistant receives. +/// +/// Each operation gets a header naming its position, so a model reading the +/// result can line each section up with the operation it wrote. +/// Operations that never started say so explicitly: without that, a model that +/// asked for five and reads three assumes the other two succeeded silently. +/// +/// A single successful operation is returned bare, with no framing at all, so a +/// one-operation fan-out call reads exactly like a call to the same tool +/// without fan-out. +#[must_use] +pub fn fold(outcomes: &[OperationOutcome]) -> String { + if let [OperationOutcome::Ok(content)] = outcomes { + return content.clone(); + } + + let count = outcomes.len(); + let mut body = String::new(); + + for (index, outcome) in outcomes.iter().enumerate() { + if index > 0 { + body.push('\n'); + } + + let position = index + 1; + match outcome { + OperationOutcome::Ok(content) => { + body.push_str(&format!("[{position}/{count}] ok\n{content}\n")); + } + OperationOutcome::Error(message) => { + body.push_str(&format!("[{position}/{count}] error\n{message}\n")); + } + OperationOutcome::NotRun { after } => { + body.push_str(&format!( + "[{position}/{count}] not run (stopped after operation {after} failed)\n" + )); + } + } + } + + body +} + +/// Whether the outcomes so far mean no further operation should start. +#[must_use] +pub fn should_stop(fan_out: FanOut, outcomes: &[OperationOutcome]) -> bool { + fan_out.stops_on_error() + && outcomes + .iter() + .any(|outcome| matches!(outcome, OperationOutcome::Error(_))) +} + +#[cfg(test)] +#[path = "fan_out_tests.rs"] +mod tests; diff --git a/crates/jp_llm/src/tool/fan_out_tests.rs b/crates/jp_llm/src/tool/fan_out_tests.rs new file mode 100644 index 000000000..198756902 --- /dev/null +++ b/crates/jp_llm/src/tool/fan_out_tests.rs @@ -0,0 +1,207 @@ +use jp_config::conversation::tool::FanOutOnError; +use serde_json::json; + +use super::*; + +fn args(value: &Value) -> Map { + value.as_object().expect("the fixture is an object").clone() +} + +#[test] +fn envelope_wraps_the_operation_schema_in_a_required_array() { + let operation = json!({ + "type": "object", + "properties": { "path": { "type": "string" } }, + "required": ["path"], + }); + + let wrapped = envelope(&operation); + + assert_eq!( + wrapped, + json!({ + "type": "object", + "properties": { + "ops": { + "type": "array", + "minItems": 1, + "description": "The operations to perform. Each element is one complete set \ + of this tool's arguments.", + "items": { + "type": "object", + "properties": { "path": { "type": "string" } }, + "required": ["path"], + }, + } + }, + "required": ["ops"], + "additionalProperties": false, + }) + ); +} + +#[test] +fn expand_returns_one_argument_map_per_operation() { + let arguments = args(&json!({ + "ops": [ + { "path": "a.rs", "start_line": 1 }, + { "path": "b.rs" }, + ] + })); + + let ops = expand(&arguments).expect("the envelope is well formed"); + + assert_eq!(ops, vec![ + args(&json!({ "path": "a.rs", "start_line": 1 })), + args(&json!({ "path": "b.rs" })), + ]); +} + +#[test] +fn expand_accepts_a_single_operation() { + let arguments = args(&json!({ "ops": [{ "path": "a.rs" }] })); + + let ops = expand(&arguments).expect("one operation is a valid call"); + + assert_eq!(ops, vec![args(&json!({ "path": "a.rs" }))]); +} + +/// A tool whose schema says `ops` and which receives `path` was called wrongly. +/// Running it anyway would hide the mistake from the model that made it, and +/// teach it that the envelope is optional. +#[test] +fn expand_rejects_a_call_that_skipped_the_envelope() { + let arguments = args(&json!({ "path": "a.rs" })); + + assert_eq!(expand(&arguments), Err(ExpandError::Missing)); +} + +#[test] +fn expand_rejects_an_empty_array() { + let arguments = args(&json!({ "ops": [] })); + + assert_eq!(expand(&arguments), Err(ExpandError::Empty)); +} + +#[test] +fn expand_rejects_a_non_array_envelope() { + let arguments = args(&json!({ "ops": { "path": "a.rs" } })); + + assert_eq!(expand(&arguments), Err(ExpandError::NotAnArray)); +} + +#[test] +fn expand_names_the_position_of_a_non_object_element() { + let arguments = args(&json!({ "ops": [{ "path": "a.rs" }, "b.rs"] })); + + assert_eq!( + expand(&arguments), + Err(ExpandError::ElementNotAnObject { index: 1 }) + ); +} + +#[test] +fn expand_error_messages_name_the_tool_and_the_envelope() { + assert_eq!( + ExpandError::Missing.message("fs_read_file"), + "Tool 'fs_read_file' takes its arguments in an `ops` array, but the call had no `ops` \ + key. Wrap the arguments in one: {\"ops\": [{...}]}." + ); + assert_eq!( + ExpandError::Empty.message("fs_read_file"), + "Tool 'fs_read_file' was called with an empty `ops` array, so there was nothing to do. \ + Include at least one operation." + ); + assert_eq!( + ExpandError::ElementNotAnObject { index: 2 }.message("fs_read_file"), + "Tool 'fs_read_file' expects every element of `ops` to be an object holding one \ + operation's arguments; element 2 was not." + ); +} + +/// One successful operation reads exactly like a call to the same tool without +/// fan-out, which is what keeps the envelope invisible at N=1. +#[test] +fn fold_returns_a_lone_success_without_any_framing() { + let folded = fold(&[OperationOutcome::Ok("file contents".to_owned())]); + + assert_eq!(folded, "file contents"); +} + +/// A lone *failure* still gets framing: the assistant needs to see that the one +/// operation it asked for is the one that failed. +#[test] +fn fold_frames_a_lone_failure() { + let folded = fold(&[OperationOutcome::Error("not found".to_owned())]); + + assert_eq!(folded, "[1/1] error\nnot found\n"); +} + +#[test] +fn fold_frames_each_operation_with_its_position() { + let folded = fold(&[ + OperationOutcome::Ok("first".to_owned()), + OperationOutcome::Error("second failed".to_owned()), + OperationOutcome::Ok("third".to_owned()), + ]); + + assert_eq!( + folded, + "[1/3] ok\nfirst\n\n[2/3] error\nsecond failed\n\n[3/3] ok\nthird\n" + ); +} + +/// Without these lines a model that asked for five operations and reads three +/// assumes the other two succeeded silently. +#[test] +fn fold_names_the_operations_that_never_started() { + let folded = fold(&[ + OperationOutcome::Ok("File deleted.".to_owned()), + OperationOutcome::Error("File has uncommitted changes.".to_owned()), + OperationOutcome::NotRun { after: 2 }, + OperationOutcome::NotRun { after: 2 }, + ]); + + assert_eq!( + folded, + "[1/4] ok\nFile deleted.\n\n[2/4] error\nFile has uncommitted changes.\n\n[3/4] not run \ + (stopped after operation 2 failed)\n\n[4/4] not run (stopped after operation 2 failed)\n" + ); +} + +#[test] +fn should_stop_is_false_while_nothing_has_failed() { + let fan_out = FanOut { + concurrency: Some(1), + on_error: FanOutOnError::Stop, + }; + + assert!(!should_stop(fan_out, &[OperationOutcome::Ok( + "ok".to_owned() + )])); +} + +#[test] +fn should_stop_is_true_after_a_failure_under_stop() { + let fan_out = FanOut { + concurrency: Some(1), + on_error: FanOutOnError::Stop, + }; + + assert!(should_stop(fan_out, &[ + OperationOutcome::Ok("ok".to_owned()), + OperationOutcome::Error("boom".to_owned()), + ])); +} + +#[test] +fn should_stop_stays_false_under_continue() { + let fan_out = FanOut { + concurrency: Some(1), + on_error: FanOutOnError::Continue, + }; + + assert!(!should_stop(fan_out, &[OperationOutcome::Error( + "boom".to_owned() + )])); +} diff --git a/crates/jp_llm/src/tool/json_schema.rs b/crates/jp_llm/src/tool/json_schema.rs index fda533054..17fedc1ac 100644 --- a/crates/jp_llm/src/tool/json_schema.rs +++ b/crates/jp_llm/src/tool/json_schema.rs @@ -5,12 +5,22 @@ //! For an MCP tool that is the server's `inputSchema` with the user's //! configured overrides applied; for a local or built-in tool it is generated //! from configuration. -//! Nothing else rewrites it: adapting a schema to what a given API accepts is -//! the responsibility of that provider. +//! Adapting a schema to what a given API accepts is the responsibility of that +//! provider. +//! +//! One thing does rewrite it. +//! A tool configured for fan-out is shown an envelope holding an array of its +//! own schema, so one call can carry several operations; see [`fan_out`]. +//! That envelope is JP's construction, not anything the tool's source declared, +//! and it exists only on the way out: `ToolDefinition::parameters` still holds +//! the per-operation document, and argument validation, defaults, and coercion +//! all run against that. //! //! [`Node`] is the read-only view used by argument handling and validation. //! It follows same-document `$ref` pointers while reading, so a referenced enum //! or nested object answers questions the same way an inline one does. +//! +//! [`fan_out`]: super::fan_out use std::borrow::Cow; diff --git a/crates/jp_llm/src/tool_tests.rs b/crates/jp_llm/src/tool_tests.rs index 5173063a8..926a603be 100644 --- a/crates/jp_llm/src/tool_tests.rs +++ b/crates/jp_llm/src/tool_tests.rs @@ -230,6 +230,125 @@ fn param(kind: &str) -> Value { json!({ "type": kind }) } +#[test] +fn provider_schema_is_the_operation_schema_without_fan_out() { + let parameters = schema([("path", param("string"), true)]); + let definition = ToolDefinition { + name: "fs_read_file".to_owned(), + docs: ToolDocs::default(), + parameters: parameters.clone(), + fan_out: None, + }; + + assert_eq!(*definition.provider_schema(), parameters); +} + +#[test] +fn provider_schema_wraps_the_operation_schema_when_fanning_out() { + let parameters = schema([("path", param("string"), true)]); + let definition = ToolDefinition { + name: "fs_read_file".to_owned(), + docs: ToolDocs::default(), + parameters: parameters.clone(), + fan_out: Some(jp_config::conversation::tool::FanOut { + concurrency: None, + on_error: jp_config::conversation::tool::FanOutOnError::Continue, + }), + }; + + let wrapped = definition.provider_schema(); + assert_eq!(wrapped["properties"]["ops"]["items"], parameters); + assert_eq!(wrapped["required"], json!(["ops"])); + + // The per-operation schema is what validation and defaults still see, so a + // tool receives the same shape either way. + assert_eq!(definition.parameters, parameters); +} + +/// A tool configured for fan-out is shown the envelope, told about it in the +/// text the provider receives, and keeps its own schema for validation. +#[tokio::test] +async fn resolve_tool_wraps_a_fan_out_tool_for_the_provider() { + let partial: PartialToolConfig = serde_json::from_value(json!({ + "source": "local", + "summary": "Read a file.", + "fan_out": true, + "parameters": { + "path": { "type": "string", "required": true } + } + })) + .unwrap(); + let tool = ToolConfig::from_partial(partial, vec![]).unwrap(); + let mut app = AppConfig::new_test(); + app.conversation + .tools + .insert("fs_read_file".to_owned(), tool); + let config = app.conversation.tools.get("fs_read_file").unwrap(); + + let definition = resolve_tool("fs_read_file", &config, &Client::new(IndexMap::new())) + .await + .expect("the tool resolves"); + + assert_eq!( + definition.parameters["properties"]["path"], + json!({ "type": "string" }), + "validation still sees one operation's schema" + ); + + let provider_schema = definition.provider_schema(); + assert_eq!(provider_schema["required"], json!(["ops"])); + assert_eq!( + provider_schema["properties"]["ops"]["items"]["properties"]["path"], + json!({ "type": "string" }), + "the envelope's items are the tool's own schema" + ); + + let description = definition + .docs + .schema_description() + .expect("the tool has a summary"); + assert!( + description.starts_with("Read a file."), + "the tool's own summary comes first: {description}" + ); + assert!( + description.contains("`ops` array"), + "the model is told how the envelope relates to the documented parameters: {description}" + ); +} + +/// A tool that says nothing about fan-out is untouched, which is what keeps +/// this feature invisible to every tool that has not opted in. +#[tokio::test] +async fn resolve_tool_leaves_a_plain_tool_alone() { + let partial: PartialToolConfig = serde_json::from_value(json!({ + "source": "local", + "summary": "Read a file.", + "parameters": { + "path": { "type": "string", "required": true } + } + })) + .unwrap(); + let tool = ToolConfig::from_partial(partial, vec![]).unwrap(); + let mut app = AppConfig::new_test(); + app.conversation + .tools + .insert("fs_read_file".to_owned(), tool); + let config = app.conversation.tools.get("fs_read_file").unwrap(); + + let definition = resolve_tool("fs_read_file", &config, &Client::new(IndexMap::new())) + .await + .expect("the tool resolves"); + + assert!(definition.fan_out.is_none()); + assert_eq!(*definition.provider_schema(), definition.parameters); + assert_eq!( + definition.docs.schema_description(), + Some("Read a file."), + "no envelope instruction is added" + ); +} + #[tokio::test] async fn local_tool_rejects_scalar_enum_on_array_parameter() { let partial: PartialToolConfig = serde_json::from_value(json!({ @@ -301,6 +420,7 @@ fn coerces_json_strings_to_declared_parameter_types() { ToolDefinition { name: "test".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters, } .coerce_arguments(&mut arguments); @@ -328,6 +448,7 @@ fn leaves_strings_alone_for_a_parameter_with_no_declared_type() { ToolDefinition { name: "test".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters, } .coerce_arguments(&mut arguments); @@ -345,6 +466,7 @@ fn coerces_a_string_the_enum_excludes_into_the_member_it_parses_to() { ToolDefinition { name: "test".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters, } .coerce_arguments(&mut arguments); @@ -362,6 +484,7 @@ fn leaves_a_string_alone_when_the_enum_lists_it() { ToolDefinition { name: "test".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters, } .coerce_arguments(&mut arguments); @@ -384,6 +507,7 @@ async fn execute_coerces_json_strings_before_calling_tool() { let definition = ToolDefinition { name: "echo_arguments".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: schema([("start_line", param("integer"), false)]), }; let builtins = builtin::BuiltinExecutors::new().register("echo_arguments", EchoArguments); @@ -1061,6 +1185,7 @@ async fn test_execute_local_exposes_invocation_ids_in_context() { let definition = ToolDefinition { name: "echo_ids".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: schema([]), }; let invocation = InvocationContext { @@ -1132,6 +1257,7 @@ async fn test_execute_builtin_dispatches_on_source_name() { let definition = ToolDefinition { name: "docs".to_owned(), docs: ToolDocs::default(), + fan_out: None, parameters: schema([]), }; let mcp_client = Client::new(IndexMap::new()); diff --git a/crates/jp_llm/src/window_tests.rs b/crates/jp_llm/src/window_tests.rs index 42fc3dfa1..ba8e119e7 100644 --- a/crates/jp_llm/src/window_tests.rs +++ b/crates/jp_llm/src/window_tests.rs @@ -12,6 +12,7 @@ fn tool(name: &str, summary: Option<&str>) -> ToolDefinition { ..Default::default() }, parameters: serde_json::json!({ "type": "object", "properties": {} }), + fan_out: None, } } diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 115fbe1c1..75102f859 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -426,5 +426,9 @@ "107-background-task-infrastructure.md": { "hash": "a959c85b2a886843531a94ecf66fcb13a77c60fd0317a7dd6767aab2a3a16979", "summary": "`jp_task` provides bounded background task primitives for concurrent work that commits to the workspace before exit." + }, + "108-transparent-tool-call-fan-out.md": { + "hash": "1ddb213dc7c5342b0c46babfde6f9ae85a7e22da73bb36001403eaccecec7a3d", + "summary": "" } } diff --git a/docs/rfd/108-transparent-tool-call-fan-out.md b/docs/rfd/108-transparent-tool-call-fan-out.md new file mode 100644 index 000000000..e99ff1582 --- /dev/null +++ b/docs/rfd/108-transparent-tool-call-fan-out.md @@ -0,0 +1,406 @@ +# RFD 108: Transparent Tool Call Fan-Out + +- **Status**: Implemented +- **Category**: Design +- **Authors**: Jean Mertz +- **Date**: 2026-09-18 + +## Summary + +This RFD adds **fan-out**: a per-tool configuration flag that lets a single tool +call carry several independent operations, which JP executes as if the assistant +had issued them separately. +The tool's own implementation is unchanged. +JP wraps the tool's parameter schema in an envelope before showing it to the +provider, expands the envelope into N executors before running anything, and +folds the N results back into one response. + +## Motivation + +An assistant that already knows which files it wants pays for one +request-response cycle per call: + +```text +fs_read_file(path: "docs/pitch.md", start_line: 58, end_line: 68) +fs_read_file(path: "scripts/migrate.sh", start_line: 1928, end_line: 1940) +fs_read_file(path: "scripts/migrate.sh", start_line: 2072, end_line: 2082) +... 11 more +``` + +Each cycle re-sends the accumulated context. +Cached input is roughly a tenth the price of uncached input, so ten cached +cycles cost about one uncached request, but fourteen sequential cycles also cost +fourteen units of latency and fourteen turns of the agent loop. + +The obvious fix is to give each tool a multi-argument schema. +That fix has two problems. + +**It does not reach MCP tools.** `resolve_tool` (`tool.rs:1299`) builds a +`ToolDefinition` uniformly for local, built-in, and MCP sources; for MCP it +takes the server's own `inputSchema` and applies configured overrides. +JP does not own those schemas and cannot edit them. +A per-tool fix helps only the tools JP wrote. + +**It entangles two axes.** "What this tool does" and "how many operations fit in +one call" want to vary independently. +Hand-writing the second into every tool's schema means every new tool re-decides +it, and every existing tool that wants it needs a schema change, a validation +change, and a result-format change. + +Fan-out makes the second axis a property of the tool *configuration* rather than +the tool *implementation*, so it applies uniformly to every source, including +MCP servers JP does not control. + +## Design + +### What the user sees + +Nothing changes. +Fan-out exists on the wire and is invisible in the terminal. + +A call carrying three operations renders as three tool calls, prompts for +permission three times if the tool is not unattended, and shows three results: + +```text +Calling tool fs_read_file(path: "docs/pitch.md", start_line: 58, end_line: 68) +Calling tool fs_read_file(path: "scripts/migrate.sh", start_line: 1928, end_line: 1940) +Calling tool fs_read_file(path: "scripts/migrate.sh", start_line: 2072, end_line: 2082) +``` + +A call carrying one operation renders as one tool call, with the envelope +stripped, indistinguishable from a call made without fan-out. + +This invariant settles the permission question: whatever is rendered as a +distinct call is approved as a distinct call. +Collapsing three rendered calls into one approval prompt would ask the user to +approve something other than what they were shown. +The savings this RFD delivers are entirely in round-trips to the provider, not +in user interaction, and the tools that dominate the cost (`run = "unattended"` +readers) prompt zero times either way. + +### The envelope + +When a tool opts in, the schema shown to the provider is wrapped: + +```json +{ + "type": "object", + "properties": { + "ops": { "type": "array", "items": { "": "..." } } + }, + "required": ["ops"] +} +``` + +`ops` rather than `arguments` or `args`: each element is one **operation**, and +the tool's existing parameter object is already "the arguments". + +A tool's `examples` block needs no rewrite. +Each existing example shows exactly one operation, which is exactly the shape of +one `ops` element. +The schema wrap appends a generated sentence to the tool's description saying +so. + +### Where expansion happens + +Expansion happens at the **executor plan**, before any tool runs. +One `ToolCallRequest` becomes N executors that share a tool call id and differ +only in their arguments. + +This placement is the whole design. +The coordinator already keys its per-tool state by execution index: +`executing_tools` (`coordinator.rs:989`), `accumulated_answers` +(`coordinator.rs:268`), and `results` (`coordinator.rs:990`). +When a tool asks a question, the coordinator prompts and re-spawns **only that +index** (`coordinator.rs:1811-1815`); completed executions sit in `results` and +are never recomputed. + +So per-operation resumption is free. +An operation that returns `NeedsInput` suspends alone. +Operations that already completed are not re-run. +Operations that have not started are unaffected. + +Expanding inside a single executor instead would lose all of this: the +coordinator re-spawns an executor from scratch, so a batch loop inside one +executor would re-run its own committed side effects after every answered +question. + +Two existing mechanisms carry over without modification: + +- **Inquiry ids do not collide.** `next_inquiry_attempt` (`coordinator.rs:1625`) + increments per `(tool_id, question_id)`, so two operations asking the same + question under one tool call id get distinct attempts. + The counter was built for retries and covers fan-out unchanged. +- **"Answer once for the whole call" already works.** Before prompting, the + coordinator consults `remembered_tool_answers`, keyed by `(tool_name, + question_id)` (`coordinator.rs:1648-1666`). + An answer given at `PersistLevel::Turn` (`coordinator.rs:1804-1809`) resolves + every later operation silently, each still recorded as its own inquiry pair. + +### Signature changes + +`ExecutorSource::create` (`executor.rs:99`) returns one executor or `None`, +where `None` means the tool could not be resolved and becomes a "tool is not +available" response (`coordinator.rs:739-749`). + +A bare `Vec` would make that indistinguishable from an empty `ops` array, so the +option stays: + +```rust +// jp_llm::tool::executor +fn create(&self, request: ToolCallRequest, config: ToolConfigWithDefaults) + -> Option>>; + +// jp_cli::cmd::query::tool::coordinator +pub fn prepare_one(&mut self, request: ToolCallRequest) + -> Result>, ToolCallResponse>; +``` + +`None` means the tool does not exist. +`Some(vec![])` means the tool exists and the assistant sent zero operations, +which is an error response rather than an empty success: a zero-operation call +is a model mistake, and returning nothing teaches it nothing. + +`ToolDefinition.parameters` keeps holding the **per-operation** schema, so +`coerce_arguments_to_schema` (`tool.rs:733`), `apply_parameter_defaults` +(`tool.rs:870`), and `validate_tool_arguments` (`tool.rs:872`) run against it +unchanged. +A new `provider_schema()` accessor applies the wrap, and the seven provider +modules that read `tool.parameters` when building their request bodies switch to +it. + +### Configuration + +`fan_out` accepts a bool or a table, following the precedent `enable` already +sets in the same config tree: + +```toml +# reads: unbounded concurrency, report every failure +[conversation.tools.fs_read_file] +fan_out = true + +# writes: one at a time, in order, stop at the first failure +[conversation.tools.fs_delete_file.fan_out] +concurrency = 1 +on_error = "stop" + +# rate-limited remote calls +[conversation.tools.github_issues.fan_out] +concurrency = 4 +``` + +| Key | Default | Meaning | +| ------------- | ---------- | ------------------------------------------------------------ | +| `concurrency` | unbounded | Maximum operations in flight. `1` runs them in order. | +| `on_error` | `continue` | `stop` starts no further operations after the first failure. | + +`concurrency` is one integer rather than a `mode = "parallel" | "sequential"` +enum plus a later `max_concurrency`. +Two knobs that can contradict each other (`mode = "sequential", max_concurrency += 4`) are one axis wearing two names. +A future `delay` key for rate-limited endpoints slots in beside these without +disturbing either. + +`on_error` is genuinely independent of `concurrency`: independent reads want +unbounded concurrency and `continue`, ordered writes want `concurrency = 1` and +`stop`, and both other combinations are reachable. + +With `concurrency` above 1, `stop` means no further operations are *started*. +In-flight operations finish; nothing is aborted. + +### Where the safety boundary sits + +An earlier version of this design gated fan-out on tools that never return +`NeedsInput`, on the grounds that a question arriving mid-batch leaves earlier +side effects committed. + +That gate is unnecessary, and the property it was reaching for is better +expressed by the config above. +A tool configured with `concurrency = 1` and `on_error = "stop"` behaves exactly +as separate calls would: when operation 2 asks a question, operation 1 has +committed and operations 3 through 5 have not started. +That is the same state the assistant would have reached by issuing two calls. + +The question is not "can this tool ask?" but "is this tool's fan-out ordered?", +and the tool's configuration answers it. + +### Folding results + +N `ToolCallResponse` values sharing one id become one. +`ExecutionResult.responses` already carries `(index, response)` pairs and +documents that merging back into stream order is the caller's job +(`coordinator.rs:182-186`), so the shape fits. + +The folded body frames each operation and states what did not run: + +```text +[1/5] ok +File deleted. + +[2/5] error +File has uncommitted changes. Please stage or discard first. + +[3/5] not run (stopped after operation 2 failed) +[4/5] not run (stopped after operation 2 failed) +[5/5] not run (stopped after operation 2 failed) +``` + +Without the "not run" lines the assistant assumes all five were attempted and +reasons from a false premise. + +## Drawbacks + +**A second way to do the same thing.** `fs_modify_file` already accepts many +targets in one call, and `bash` accepts many commands. +Fan-out does not replace those and does not subsume them (see Non-Goals), so the +project carries two mechanisms that both mean "more than one thing per call". + +**The schema is rewritten.** `json_schema` states that a tool's parameters are +held exactly as the source declared them, and that adapting a schema is the +provider's job. +Fan-out is the first exception. +The module doc has to name it, or the next contributor will read the invariant +and be wrong. + +**Rendered-argument replay needs a new shape.** `RENDERED_ARGUMENTS_KEY` +(`event.rs:35`) stores one base64 blob per event so replay reproduces +custom-formatter output without re-running the formatter. +One event now carries N rendered chunks, so the value becomes a list. + +**More state keyed by something other than tool id.** `tool_states` +(`coordinator.rs:381`) is a `HashMap` keyed by tool call +id. +N operations under one id need a composite key. + +## Alternatives + +**Per-tool multi-argument schemas.** Give `fs_read_file` a `reads[]` parameter, +`fs_create_file` a `files[]` parameter, and so on. +Rejected: it does not reach MCP tools, and it re-decides the same question in +every tool. +It also requires each tool to grow its own result-framing and partial-failure +handling, which fan-out provides once. + +**Expansion inside a single executor.** Loop over operations in +`ToolDefinition::execute` (`tool.rs:793`), which is already the single funnel +for all three sources. +Rejected: the coordinator re-spawns an executor from scratch after an answered +question, so a loop inside one executor re-runs committed side effects. +The executor plan is one layer up and already has the per-operation state this +needs. +The plan is also the stabler place to sit: expanding before execution means +fan-out is indifferent to how any single operation is dispatched, so reworking +that dispatch neither blocks this design nor is blocked by it. + +**Gate fan-out on tools that cannot ask questions.** Rejected in favour of the +`concurrency` and `on_error` configuration, which expresses the same safety +property without excluding every write tool from the feature. + +## Non-Goals + +**Replacing batch operations.** `fs_modify_file` is one operation over a set of +targets, not N independent operations: its patterns apply in order across files +and each sees the previous one's output, it shows a single diff for one +approval, and it validates the whole set before writing anything. +Tools shaped that way opt out by never setting `fan_out`. + +**Shared parameters across operations.** Every operation carries its own +complete argument object. +Hoisting a common value to the call level (the way `fs_modify_file` has a +call-level `path` default) is a possible later extension and is not designed +here. + +**A delay knob.** Rate-limited endpoints will want one. +The config shape above leaves room for it; this RFD does not build it. + +**Changing what any tool does.** No tool implementation changes. +A tool that gains fan-out behaves identically per operation. + +## Risks and Open Questions + +**Is `ops` the right name?** The original proposal spelled it `args`. +`ops` is used here because "operation" is the concept and "arguments" is already +what the inner object is, but the name lands in every fanned-out call the +assistant writes and is worth one round of review. + +**Does the assistant use it?** A wrapped schema is a more complex schema. +Some models may keep issuing one operation per call, which costs an extra +envelope of tokens per call and delivers nothing. +Enabling it on `fs_read_file` first and measuring the operation-count +distribution answers this before the feature spreads. + +**Permission fatigue on write tools.** Fan-out plus a non-unattended write tool +means N prompts. +That is the honest behaviour, but it may make fan-out unattractive on exactly +the tools where ordering matters most. +The existing `PersistLevel::Turn` answer cache is the mitigation; whether it is +enough is an implementation-time observation. + +**Interrupt semantics.** Ctrl-C during a fanned-out call currently reaches an +execution phase, not an individual tool. +Whether "Stop & respond" should cancel the whole call or only the in-flight +operations needs a decision during implementation. + +## Implementation Plan + +### Phase 1: Configuration + +Add `FanOutConfig` to `jp_config::conversation::tool` with bool-or-table +parsing, `concurrency`, and `on_error`. +No behaviour yet. + +**Depends on:** nothing. +**Mergeable:** yes. + +### Phase 2: Schema envelope + +Add the fan-out field and `provider_schema()` to `ToolDefinition`. +Apply the wrap in `resolve_tool`, append the generated description sentence, and +switch the seven provider modules to the new accessor. +Behaviour-neutral while no tool opts in. + +**Depends on:** Phase 1. +**Mergeable:** yes. + +### Phase 3: Plan expansion + +Change `ExecutorSource::create` and `prepare_one` to the plural signatures. +Give `tool_states` a composite key. +Expand one request into N executors. +Run them under the existing unbounded model. + +**Depends on:** Phase 2. +**Mergeable:** yes. + +### Phase 4: Concurrency and error policy + +Honour `concurrency` in the spawn loop and `on_error` in the event loop. + +**Depends on:** Phase 3. +**Mergeable:** yes. + +### Phase 5: Rendering and folding + +Render one call line per operation, prompt per operation, fold N responses into +one with per-operation framing and "not run" lines. +Change `RENDERED_ARGUMENTS_KEY` to hold a list. + +**Depends on:** Phase 3. +**Mergeable:** yes, in parallel with Phase 4. + +### Phase 6: Enable and measure + +Turn on `fan_out` for `fs_read_file`, `fs_grep_files`, and `fs_list_files`. +Record the distribution of operations per call over a week of use before +enabling it anywhere else. + +**Depends on:** Phases 4 and 5. +**Mergeable:** yes. + +## References + +- [RFD 082] records every tool question round-trip as an inquiry pair, which is + what keeps N per-operation questions individually auditable under one tool + call id. + +[RFD 082]: 082-unified-inquiry-event-recording.md From ce4b8ca4785cb81cf27e4129f48303d9d9406c97 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:45:46 +0200 Subject: [PATCH 2/2] fix(llm, cli): Correct six fan-out defects found in review A tool configured for fan-out no longer breaks the schema it is shown. A `$defs` or `definitions` block now moves to the envelope's root, so every `#/$defs/Name` reference still resolves: nesting the block under `properties.ops.items` left each reference pointing at a root that no longer held it, which reached Ollama as a property with no type at all and other providers as a dangling pointer. MCP tools carry the server's document verbatim, so this struck the population fan-out exists for. Two failures stop being hidden from the assistant. A tool without `fan_out` whose argument formatter fails once again answers with an error rather than a success carrying error text, which Anthropic was receiving as `is_error: false` and replay was rendering in the success style. And `on_error = "stop"` now reads the tool's own outcome instead of the response left after result-mode policy, so a failure suppressed by `result = "skip"` or a declined `result = "ask"` prompt stops the operations behind it as configured. A turn can no longer stall. When a `stop` policy ruled out every remaining operation before any had started, nothing was spawned and no event would ever arrive, so the execution loop waited on a channel whose senders it held itself; completion is now checked before the receive. Ctrl-C during a fanned-out call also reaches the cancellation menu again: every prompt state transition keys on the operation rather than the shared call id, so an approved operation no longer strands an `AwaitingPermission` entry that made `is_prompting` true for the rest of the turn. Restarting and escalating now stop releasing queued operations, matching what cancelling already did. Signed-off-by: Jean Mertz --- .../src/cmd/conversation/print_tests.rs | 1 + .../jp_cli/src/cmd/query/tool/coordinator.rs | 107 +++++++---- .../src/cmd/query/tool/coordinator_tests.rs | 150 ++++++++++++++++ crates/jp_cli/src/cmd/query/tool/schedule.rs | 33 ++-- .../src/cmd/query/tool/schedule_tests.rs | 69 ++++++++ .../jp_cli/src/cmd/query/turn_loop_tests.rs | 167 +++++++++++++++++- crates/jp_llm/src/provider/ollama_tests.rs | 56 ++++++ crates/jp_llm/src/tool/fan_out.rs | 55 +++++- crates/jp_llm/src/tool/fan_out_tests.rs | 54 ++++++ 9 files changed, 636 insertions(+), 56 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index 9c91d8fe0..9217c4d95 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -79,6 +79,7 @@ fn tool_with_style(style: DisplayStyleConfig) -> ToolConfig { questions: IndexMap::new(), options: IndexMap::new(), access: None, + fan_out: None, } } diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 5127a0eee..14460abf9 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -354,6 +354,7 @@ impl ExecutorGroup { /// Fold a group whose operations were all decided before running. fn fold_resolved(self) -> ToolCallResponse { + let fans_out = self.fan_out.is_some(); let outcomes: Vec<_> = self .ops .into_iter() @@ -368,12 +369,9 @@ impl ExecutorGroup { }) .collect(); - // Every operation was skipped or failed the same way, so the call as a - // whole did not run. `Ok` matches how a single skipped tool answers - // today: the user declining is not an error the assistant should retry. ToolCallResponse { id: self.tool_id, - result: Ok(fan_out::fold(&outcomes)), + result: fan_out::fold_call(fans_out, outcomes), } } } @@ -657,7 +655,7 @@ impl ToolCoordinator { return ToolCallDecision::Skipped(response); } PermissionDecision::NeedsPrompt { executor, info } => { - self.set_tool_state(&info.tool_id, ToolCallState::AwaitingPermission); + self.set_tool_state(&info.state_key, ToolCallState::AwaitingPermission); // Pre-render before the prompt so the user sees the // rendered call (not raw arguments) when deciding. @@ -1246,7 +1244,25 @@ impl ToolCoordinator { let mut cancellation_message: Option = None; let mut cancelled_indices: Vec = Vec::new(); - while let Some(event) = event_rx.recv().await { + // Whether the schedule may still hand out queued operations. Every + // interrupt outcome that cancels the token clears this: restarting + // re-runs the batch from the top, and an escalation is a shutdown, so + // neither wants a fresh subprocess spawned on the way out. + let mut releasing = true; + + loop { + // Checked before the receive, not after handling one: a call whose + // `stop` policy ruled out every remaining operation spawned nothing + // at all, so no event will ever arrive to wake this loop. The + // senders are still alive, so `recv` would wait forever. + if schedule.all_accounted_for(&results) { + break; + } + + let Some(event) = event_rx.recv().await else { + break; + }; + match event { ExecutionEvent::ToolResult { index, result } => { let Some(tool) = executing_tools.get_mut(&index) else { @@ -1254,7 +1270,7 @@ impl ToolCoordinator { continue; }; let response = &mut results[index]; - self.handle_tool_result( + let failed = self.handle_tool_result( result, tool, index, @@ -1272,6 +1288,13 @@ impl ToolCoordinator { interactive, tool_renderer, ); + + // Recorded from the tool's own outcome rather than from + // `results[index]`, which result-mode policy may have + // already turned into a success. + if failed { + schedule.record_failure(index); + } } ExecutionEvent::PromptAnswer { index, @@ -1381,10 +1404,10 @@ impl ToolCoordinator { response, } => { prompt_active = false; - let tool_name = executing_tools - .get(&index) - .map(|t| t.tool_name.clone()) - .unwrap_or_default(); + let (tool_name, state_key) = executing_tools.get(&index).map_or_else( + || (String::new(), tool_id.clone()), + |t| (t.tool_name.clone(), t.state_key.clone()), + ); let is_error = response.result.is_err(); let (inline_results, results_file_link) = self .tools_config @@ -1405,7 +1428,7 @@ impl ToolCoordinator { tool_renderer.render_result(&response, &inline_results, &results_file_link); } - self.set_tool_state(&tool_id, ToolCallState::Completed); + self.set_tool_state(&state_key, ToolCallState::Completed); results[index] = Some(response); self.process_next_prompt( &mut pending_prompts, @@ -1459,17 +1482,14 @@ impl ToolCoordinator { | ToolInterruptResult::PromptFailed | ToolInterruptResult::Declined => {} ToolInterruptResult::Restart => { + schedule.abandon_unstarted(); + releasing = false; outcome.upgrade(ExecutionOutcome::Restart); } ToolInterruptResult::Cancelled { response, exit } => { cancelled_indices = schedule.unfinished(&results); - - // A call holding operations back behind a - // concurrency limit would otherwise leave the - // loop waiting on results that can never - // arrive, because those operations were never - // spawned. schedule.abandon_unstarted(); + releasing = false; tools_cancelled = true; cancellation_message = response; if exit { @@ -1481,6 +1501,8 @@ impl ToolCoordinator { // escalation so the turn loop begins a graceful // shutdown. ToolInterruptResult::Escalate => { + schedule.abandon_unstarted(); + releasing = false; outcome.upgrade(ExecutionOutcome::Escalated); } } @@ -1488,10 +1510,11 @@ impl ToolCoordinator { } } - // Tell the schedule which operations failed, so a call configured - // to stop on error starts none of the ones still queued. - // Re-scanning every finished operation each time is cheap at these - // sizes, and recording the same failure twice is a no-op. + // Backstop for the failure paths that write `results` directly (a + // cancelled prompt, an inquiry that could not be answered). The + // execution outcome itself is recorded in `handle_tool_result`, + // before result-mode policy can rewrite it. Recording the same + // failure twice is a no-op: the earliest position wins. for (index, response) in results.iter().enumerate() { if let Some(response) = response { schedule.record_outcome(index, response); @@ -1501,7 +1524,7 @@ impl ToolCoordinator { // Release whatever the finished operations made room for: the next // operation of a call with a concurrency limit, or nothing at all // for a call already running everything it has. - if !tools_cancelled { + if releasing { for (index, executor) in schedule.release(&results) { self.start_operation( index, @@ -1515,10 +1538,6 @@ impl ToolCoordinator { ); } } - - if schedule.all_accounted_for(&results) { - break; - } } // Deregister the tool interrupt handler; its forwarding task exits @@ -1761,6 +1780,14 @@ impl ToolCoordinator { }); } + /// Handle one operation's execution result. + /// + /// Returns whether the *tool* reported a failure, which is not the same as + /// whether `tracked_response` ends up holding one: `result = "skip"` and a + /// declined `result = "ask"` prompt both answer the assistant with a + /// success. + /// A `stop` fan-out policy keys off this return value, so it acts on what + /// the tool did rather than on what the assistant was told. #[allow(clippy::too_many_arguments)] #[allow(clippy::too_many_lines)] fn handle_tool_result( @@ -1781,7 +1808,7 @@ impl ToolCoordinator { turn_state: &mut TurnState, interactive: bool, tool_renderer: &ToolRenderer, - ) { + ) -> bool { match result { ExecutorResult::Completed(response) => { let is_error = response.result.is_err(); @@ -1836,7 +1863,7 @@ impl ToolCoordinator { } else { *prompt_active = true; self.set_tool_state( - &tool.tool_id, + &tool.state_key, ToolCallState::AwaitingResultEdit, ); Self::spawn_result_mode_prompt( @@ -1866,6 +1893,10 @@ impl ToolCoordinator { } } } + + // Read off the executor's own result, before any of the + // branches above had a chance to replace it. + is_error } ExecutorResult::NeedsInput { tool_id, @@ -1920,7 +1951,7 @@ impl ToolCoordinator { event_tx, tool.stderr.clone(), ); - return; + return false; } } @@ -1944,7 +1975,7 @@ impl ToolCoordinator { event_tx, tool.stderr.clone(), ); - return; + return false; } let target = self @@ -1971,7 +2002,7 @@ impl ToolCoordinator { }); } else { *prompt_active = true; - self.set_tool_state(&tool_id, ToolCallState::AwaitingInput); + self.set_tool_state(&tool.state_key, ToolCallState::AwaitingInput); Self::spawn_user_prompt( index, question, @@ -2002,11 +2033,12 @@ impl ToolCoordinator { ) }; Self::record_inquiry_cancelled(conv, &inquiry_id, reason); - self.set_tool_state(&tool_id, ToolCallState::Completed); + self.set_tool_state(&tool.state_key, ToolCallState::Completed); *tracked_response = Some(ToolCallResponse { id: tool_id.clone(), result: Err(message), }); + return true; } else { // The `InquiryRequest` is already recorded above; spawn the // async inquiry on a cloned snapshot. @@ -2021,8 +2053,11 @@ impl ToolCoordinator { cancellation_token.child_token(), event_tx.clone(), ); - self.set_tool_state(&tool_id, ToolCallState::AwaitingInput); + self.set_tool_state(&tool.state_key, ToolCallState::AwaitingInput); } + + // The operation has not finished: it is waiting on an answer. + false } } } @@ -2246,7 +2281,9 @@ impl ToolCoordinator { response, result_mode, } => { - self.set_tool_state(&tool_id, ToolCallState::AwaitingResultEdit); + if let Some(tool) = executing_tools.get(&index) { + self.set_tool_state(&tool.state_key, ToolCallState::AwaitingResultEdit); + } Self::spawn_result_mode_prompt( index, tool_id, diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index 9897632ed..676271364 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -18,6 +18,88 @@ fn empty_executor_source() -> Box { )) } +/// A tool that never opted into fan-out keeps its failure a failure. +/// +/// The argument formatter failing resolves the call's only operation to an +/// `Err`, and folding it into `Ok` would reach Anthropic as `is_error: false` +/// and render in the success style on replay. +#[test] +fn a_failure_on_a_tool_without_fan_out_stays_a_failure() { + let group = ExecutorGroup { + tool_id: "call_1".to_owned(), + tool_name: "fs_modify_file".to_owned(), + fan_out: None, + ops: vec![GroupOp::Resolved(ToolCallResponse { + id: "call_1".to_owned(), + result: Err( + "Tool 'fs_modify_file' was not executed because the argument formatter failed: \ + boom" + .to_owned(), + ), + })], + }; + + let response = ToolCoordinator::fold_decided_group(group); + + assert_eq!( + response.result, + Err( + "Tool 'fs_modify_file' was not executed because the argument formatter failed: boom" + .to_owned() + ), + "no framing is added and the error variant survives" + ); +} + +/// A skip on a tool without fan-out answers with the skip message verbatim. +#[test] +fn a_skip_on_a_tool_without_fan_out_is_unframed() { + let group = ExecutorGroup { + tool_id: "call_1".to_owned(), + tool_name: "fs_delete_file".to_owned(), + fan_out: None, + ops: vec![GroupOp::Resolved(ToolCallResponse { + id: "call_1".to_owned(), + result: Ok("Tool skipped by user.".to_owned()), + })], + }; + + let response = ToolCoordinator::fold_decided_group(group); + + assert_eq!(response.result, Ok("Tool skipped by user.".to_owned())); +} + +/// A fanned-out call whose operations were all decided still frames them, so +/// the assistant can tell which of the three it asked for was refused. +#[test] +fn a_fully_skipped_fan_out_call_frames_each_operation() { + let group = ExecutorGroup { + tool_id: "call_1".to_owned(), + tool_name: "fs_delete_file".to_owned(), + fan_out: Some(jp_config::conversation::tool::FanOut { + concurrency: None, + on_error: jp_config::conversation::tool::FanOutOnError::Continue, + }), + ops: vec![ + GroupOp::Resolved(ToolCallResponse { + id: "call_1".to_owned(), + result: Ok("Tool skipped by user.".to_owned()), + }), + GroupOp::Resolved(ToolCallResponse { + id: "call_1".to_owned(), + result: Err("formatter failed".to_owned()), + }), + ], + }; + + let response = ToolCoordinator::fold_decided_group(group); + + assert_eq!( + response.result, + Ok("[1/2] ok\nTool skipped by user.\n\n[2/2] error\nformatter failed\n".to_owned()) + ); +} + #[test] fn test_is_prompting_default_false() { let coordinator = ToolCoordinator::new( @@ -537,6 +619,74 @@ async fn test_resolve_tool_call_decision_invalidates_prerender_on_edit() { ); } +/// Approving a fanned-out operation must leave no prompt state behind. +/// +/// The permission prompt and its outcome have to name the same key. +/// Writing `AwaitingPermission` under the shared tool call id and `Running` +/// under the operation key strands the first entry: `is_prompting` then reports +/// true for the rest of the turn, and `handle_tool_interrupt` declines every +/// Ctrl-C as though a prompt were still open, so the tool cancellation menu +/// never appears. +#[tokio::test] +async fn approving_a_fanned_out_operation_clears_its_prompt_state() { + let tool_config = ToolConfig::from_partial( + jp_config::conversation::tool::PartialToolConfig { + source: Some(ToolSource::Builtin { tool: None }), + run: Some(RunMode::Ask), + ..Default::default() + }, + vec![], + ) + .expect("valid tool config"); + + let mut tools_config = jp_config::AppConfig::new_test().conversation.tools; + tools_config.insert("my_tool".to_string(), tool_config); + + let mut coordinator = ToolCoordinator::new(tools_config, empty_executor_source()); + + let (printer, _stdout, _stderr) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let root = Utf8TempDir::new().expect("temp dir"); + let tool_renderer = ToolRenderer::new( + ErrChannel::new(printer.clone()), + jp_config::AppConfig::new_test().style, + root.path().to_owned(), + jp_llm::tool::InvocationContext::default(), + ); + + // Operation 1 of a fanned-out call: one id, its own state key. + let executor: Box = Box::new( + MockExecutor::completed("call_1", "my_tool", "done").with_permission_info(PermissionInfo { + tool_id: "call_1".into(), + state_key: "call_1#1".into(), + tool_name: "my_tool".into(), + tool_source: ToolSource::Builtin { tool: None }, + run_mode: RunMode::Ask, + arguments: Value::Object(Map::new()), + }), + ); + + let prompter = ToolPrompter::with_backends( + printer.clone(), + None, + Arc::new(MockPromptBackend::new().with_inline_responses(['y'])), + ); + let mut turn_state = TurnState::default(); + + let decision = coordinator + .resolve_tool_call_decision(executor, &prompter, true, &mut turn_state, &tool_renderer) + .await; + + assert!( + matches!(decision, ToolCallDecision::Approved { .. }), + "the user approved the operation" + ); + assert!( + !coordinator.is_prompting(), + "no prompt is open once the operation is approved" + ); +} + #[test] fn test_permission_decision_cache_is_isolated_from_answers() { let mut coordinator = ToolCoordinator::new( diff --git a/crates/jp_cli/src/cmd/query/tool/schedule.rs b/crates/jp_cli/src/cmd/query/tool/schedule.rs index bf3470e61..cbbee9a6d 100644 --- a/crates/jp_cli/src/cmd/query/tool/schedule.rs +++ b/crates/jp_cli/src/cmd/query/tool/schedule.rs @@ -222,11 +222,29 @@ impl Schedule { } /// Record that a local index finished, so a `stop` policy can act on it. + /// + /// Reads the failure off the response, which is what the assistant will + /// receive. + /// Use [`record_failure`] where the response has already been through + /// result-mode policy, which can replace a failure with a success. + /// + /// [`record_failure`]: Self::record_failure pub(crate) fn record_outcome(&mut self, local: usize, response: &ToolCallResponse) { if response.result.is_ok() { return; } + self.record_failure(local); + } + + /// Record that a local index failed, whatever response the assistant ends + /// up seeing for it. + /// + /// `result = "skip"` and a declined `result = "ask"` prompt both replace a + /// failed response with a success before it reaches `results`, so a `stop` + /// policy reading the response alone would release the next operation after + /// a failure it was configured to stop on. + pub(crate) fn record_failure(&mut self, local: usize) { let group_id = self.owner[local]; let group = &mut self.groups[group_id]; let position = group @@ -336,22 +354,9 @@ impl Schedule { }) .collect(); - // A call that does not fan out answers with its one operation's - // result verbatim, including whether it failed. Folding would - // flatten that into a success carrying error text. - let result = if group.fans_out { - Ok(fan_out::fold(&outcomes)) - } else { - match outcomes.into_iter().next() { - Some(OperationOutcome::Ok(content)) => Ok(content), - Some(OperationOutcome::Error(message)) => Err(message), - _ => Err("Tool did not complete".to_owned()), - } - }; - (group.plan_index, ToolCallResponse { id: group.tool_id, - result, + result: fan_out::fold_call(group.fans_out, outcomes), }) }) .collect() diff --git a/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs b/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs index 784cfed05..0f8626074 100644 --- a/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs @@ -397,6 +397,75 @@ fn a_predecided_failure_stops_the_operations_behind_it() { ); } +/// `result = "skip"` answers the assistant with a success even when the tool +/// failed, so a `stop` policy reading the response alone would release the next +/// operation after a failure it was configured to stop on. +#[test] +fn a_failure_hidden_by_result_policy_still_stops_the_rest() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 3, Some(1), FanOutOnError::Stop))]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + + // What `result = "skip"` leaves behind: the tool failed, the assistant is + // told otherwise. + results[0] = Some(ok("call_1", "Result delivery skipped by configuration.")); + schedule.record_failure(0); + + assert!( + schedule.release(&results).is_empty(), + "the stop policy acts on what the tool did, not on what the assistant was told" + ); + assert!(schedule.all_accounted_for(&results)); +} + +/// The response-reading path is the backstop, and must not invent a failure +/// where the tool reported none. +#[test] +fn a_successful_operation_does_not_stop_the_rest() { + let mut schedule = Schedule::new(vec![(0, group("call_1", 3, Some(1), FanOutOnError::Stop))]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + results[0] = Some(ok("call_1", "wrote")); + schedule.record_outcome(0, results[0].as_ref().expect("recorded")); + + assert_eq!( + schedule + .release(&results) + .iter() + .map(|(i, _)| *i) + .collect::>(), + vec![1], + "a success releases the next operation" + ); +} + +/// Abandoning is what every interrupt outcome that cancels the token reaches +/// for: a restart re-runs the batch from the top and an escalation is a +/// shutdown, so neither wants the schedule handing out more work on the way +/// out. +#[test] +fn an_abandoned_schedule_releases_nothing_further() { + let mut schedule = Schedule::new(vec![( + 0, + group("call_1", 4, Some(1), FanOutOnError::Continue), + )]); + let mut results: Vec> = vec![None; schedule.total_ops()]; + + schedule.release(&results); + schedule.abandon_unstarted(); + + // The one running operation finishes, which would ordinarily free its slot. + results[0] = Some(ok("call_1", "wrote")); + + assert!( + schedule.release(&results).is_empty(), + "a finished operation must not free a slot for one that was abandoned" + ); + assert!(schedule.all_accounted_for(&results)); +} + #[test] fn schedule_reports_the_tool_behind_a_local_index() { let schedule = Schedule::new(vec![ diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index 695e59116..952a9a2b1 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -4949,9 +4949,9 @@ async fn a_fanned_out_call_runs_every_operation_and_answers_once() { let mcp_client = jp_mcp::Client::default(); let router = detached_router(); - // Counts executions rather than trusting the response text: an - // implementation that ran one operation and printed three sections - // would satisfy the output assertions below but not this one. + // Counts how many executors the plan expanded the envelope into, which + // is decided in `prepare_one` before anything runs. The per-operation + // output asserted below is what proves each one then executed. let runs = Arc::new(AtomicUsize::new(0)); let executor_source = TestExecutorSource::new().with_executor("fs_read_file", { let runs = Arc::clone(&runs); @@ -5000,7 +5000,7 @@ async fn a_fanned_out_call_runs_every_operation_and_answers_once() { assert_eq!( runs.load(Ordering::SeqCst), 3, - "the tool runs once per operation" + "the envelope expands into one executor per operation" ); // One request in, one response out: the provider asked for one call and @@ -5045,6 +5045,164 @@ async fn a_fanned_out_call_runs_every_operation_and_answers_once() { assert!(test_result.is_ok(), "Test timed out"); } +/// A `stop` policy that rules out every remaining operation before any of them +/// started must still end the turn. +/// +/// The first operation's argument formatter fails, which resolves it to an +/// error before the execution loop begins. +/// Under `on_error = "stop"` the second operation is then never released, so no +/// tool task is spawned and no event will ever arrive. +/// The loop has to notice it is already done rather than wait on a channel +/// whose senders it holds itself. +#[tokio::test] +#[expect(clippy::too_many_lines)] +async fn a_stop_policy_that_rules_out_every_operation_ends_the_turn() { + let test_result = Box::pin(timeout(Duration::from_secs(10), async { + let tmp = tempdir().unwrap(); + let root = tmp.path(); + let storage = root.join(".jp"); + + let mut config = AppConfig::new_test(); + config.conversation.tools.defaults.run = RunMode::Unattended; + + // Exits non-zero for the first operation's path and succeeds for the + // second, so one operation is resolved to a failure before the loop + // starts while the other is still runnable. + let style = DisplayStyleConfig { + parameters: ParametersStyle::Custom(CommandConfigOrString::String( + "sh -c 'test \"{{tool.arguments.path}}\" != bad.rs || exit 1; echo ok'".to_owned(), + )), + ..non_joining_style() + }; + + config + .conversation + .tools + .insert("writer".to_string(), ToolConfig { + source: ToolSource::Local { tool: None }, + command: None, + run: Some(RunMode::Unattended), + format: Some(jp_config::conversation::tool::FormatMode::Unattended), + enable: None, + summary: None, + description: None, + examples: None, + parameters: IndexMap::new(), + result: None, + style: Some(style), + questions: IndexMap::new(), + options: IndexMap::default(), + access: None, + cancellation_response: None, + fan_out: Some(jp_config::conversation::tool::FanOutConfig { + enabled: Some(true), + concurrency: Some(1), + on_error: Some(jp_config::conversation::tool::FanOutOnError::Stop), + }), + }); + + let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); + let mut workspace = Workspace::in_memory(root).with_backend(fs.clone()); + let lock = workspace + .create_and_lock_conversation(Conversation::default(), Arc::new(config.clone()), None) + .unwrap(); + + let args = json!({ "ops": [{ "path": "bad.rs" }, { "path": "good.rs" }] }); + + let provider: Arc = Arc::new(SequentialMockProvider { + responses: vec![ + vec![ + Event::tool_call_start(0, "call_1".to_string(), "writer".to_string()), + Event::tool_call_args(0, serde_json::to_string(&args).unwrap()), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ], + vec![ + Event::message(0, "Done.\n\n"), + Event::flush(0), + Event::Finished(FinishReason::Completed), + ], + ], + call_index: AtomicUsize::new(0), + model: ModelDetails::empty(id::ModelIdConfig { + provider: ProviderId::Test, + name: "fan-out-stop-mock".parse().expect("valid name"), + }), + }); + + let model = provider + .model_details(&"test-model".parse().unwrap()) + .await + .unwrap(); + + let (printer, _out, _err) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let mcp_client = jp_mcp::Client::default(); + let router = detached_router(); + + // The formatter reads the operation's arguments out of the executor, so + // the mock has to carry them. + let executor_source = TestExecutorSource::new().with_executor("writer", |req| { + Box::new( + MockExecutor::completed(&req.id, &req.name, "wrote") + .with_arguments(req.arguments.clone()), + ) + }); + let tool_defs = executor_source.tool_definitions(); + + run_turn_loop( + Arc::clone(&provider), + &model, + &config, + &router, + &mcp_client, + root, + false, + &[], + &lock, + ToolChoice::Auto, + &tool_defs, + printer.clone(), + Arc::new(MockPromptBackend::new()), + ToolCoordinator::new(config.conversation.tools.clone(), Box::new(executor_source)), + ChatRequest::from("Write two files"), + InvocationContext::default(), + PendingStreamTrim::default(), + router.turn_interrupt(lock.id()), + ) + .await + .unwrap(); + + let conv = lock.as_mut(); + let events = conv.events(); + let response = events + .iter() + .filter_map(|e| e.event.as_tool_call_response()) + .find(|r| r.id == "call_1") + .expect("the call is answered"); + + let content = response.content(); + assert!( + content.starts_with("[1/2] error\n"), + "the formatter failure is reported as operation 1.\nGot:\n{content}" + ); + assert!( + content.contains("[2/2] not run (stopped after operation 1 failed)"), + "the second operation never started, and says so.\nGot:\n{content}" + ); + assert!( + !content.contains("wrote"), + "neither operation reached the tool.\nGot:\n{content}" + ); + })) + .await; + + assert!( + test_result.is_ok(), + "the turn hung: nothing was spawned, so no event ever arrived to wake the loop" + ); +} + /// A malformed envelope answers with a message naming what went wrong, and the /// tool never runs. #[tokio::test] @@ -8469,6 +8627,7 @@ async fn a_tool_that_does_not_join_reasoning_renders_unshaded_live() { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); let fs = Arc::new(FsStorageBackend::new(&storage).expect("failed to create backend")); diff --git a/crates/jp_llm/src/provider/ollama_tests.rs b/crates/jp_llm/src/provider/ollama_tests.rs index 09e94f60c..d2e866db9 100644 --- a/crates/jp_llm/src/provider/ollama_tests.rs +++ b/crates/jp_llm/src/provider/ollama_tests.rs @@ -39,6 +39,62 @@ fn tool_references_are_expanded() { ); } +/// Wrapping a referenced schema in the fan-out envelope keeps its references +/// resolvable. +/// Nesting `$defs` under `properties.ops.items` would leave every `#/$defs/...` +/// pointing at a root that no longer holds it, and the inliner leaves an +/// unresolvable reference in place: Ollama drops `$ref` while decoding, so the +/// property would reach the model with no type at all. +#[test] +fn fan_out_tool_references_are_expanded() { + let tools = vec![ToolDefinition { + name: "crate_search_items".to_owned(), + docs: ToolDocs::default(), + fan_out: Some(jp_config::conversation::tool::FanOut { + concurrency: None, + on_error: jp_config::conversation::tool::FanOutOnError::Continue, + }), + parameters: json!({ + "type": "object", + "properties": { + "kinds": { "type": "array", "items": { "$ref": "#/$defs/EntryType" } } + }, + "required": ["kinds"], + "$defs": { "EntryType": { "type": "string", "enum": ["Enum", "Method"] } } + }), + }]; + + let converted = convert_tools(tools).expect("tools convert"); + let parameters = serde_json::to_value(&converted[0].function.parameters).expect("serializes"); + + assert_eq!( + parameters, + json!({ + "type": "object", + "properties": { + "ops": { + "type": "array", + "minItems": 1, + "description": "The operations to perform. Each element is one complete set \ + of this tool's arguments.", + "items": { + "type": "object", + "properties": { + "kinds": { + "type": "array", + "items": { "type": "string", "enum": ["Enum", "Method"] } + } + }, + "required": ["kinds"] + } + } + }, + "required": ["ops"], + "additionalProperties": false + }) + ); +} + /// The whole document is sent, not just its properties: Ollama reads `type` and /// `required` from the same object. #[test] diff --git a/crates/jp_llm/src/tool/fan_out.rs b/crates/jp_llm/src/tool/fan_out.rs index c359176a8..1ae74a4b8 100644 --- a/crates/jp_llm/src/tool/fan_out.rs +++ b/crates/jp_llm/src/tool/fan_out.rs @@ -33,9 +33,29 @@ pub const FAN_OUT_DESCRIPTION: &str = "This tool accepts several operations in a /// /// The result is always an object with one required array property, whatever /// shape `operation` has. +/// +/// A `$defs` or `definitions` block moves from the operation schema to the +/// envelope's root. +/// Same-document references are anchored at the document root (`#/$defs/Name`), +/// so leaving the block nested under `properties.ops.items` would point every +/// reference at a root that no longer holds it: Ollama's inliner would give up +/// and the property would reach the model with no type, and providers that +/// validate references would reject the request. +/// +/// A schema referring to its own root (`$ref: "#"`) is not rewritten, and would +/// resolve to the envelope rather than the operation. +/// No tool in the tree declares one. #[must_use] pub fn envelope(operation: &Value) -> Value { - json!({ + let mut operation = operation.clone(); + let definitions = operation.as_object_mut().map(|object| { + ["$defs", "definitions"] + .into_iter() + .filter_map(|key| object.remove(key).map(|block| (key.to_owned(), block))) + .collect::>() + }); + + let mut envelope = json!({ "type": "object", "properties": { FAN_OUT_KEY: { @@ -43,12 +63,18 @@ pub fn envelope(operation: &Value) -> Value { "minItems": 1, "description": "The operations to perform. Each element is one complete set of \ this tool's arguments.", - "items": operation.clone(), + "items": operation, } }, "required": [FAN_OUT_KEY], "additionalProperties": false, - }) + }); + + if let (Some(object), Some(definitions)) = (envelope.as_object_mut(), definitions) { + object.extend(definitions); + } + + envelope } /// Why a call's arguments could not be taken apart into operations. @@ -196,6 +222,29 @@ pub fn fold(outcomes: &[OperationOutcome]) -> String { body } +/// Fold one call's operation outcomes into the response it answers with. +/// +/// This is the single rule for turning per-operation outcomes into a +/// [`ToolCallResponse`] result, wherever the outcomes were collected. +/// +/// A call that does not fan out answers with its one operation's result +/// verbatim, error included: folding would flatten an error into a success +/// carrying error text, which reaches Anthropic as `is_error: false` and +/// renders in the success style on replay. +/// +/// [`ToolCallResponse`]: jp_conversation::event::ToolCallResponse +pub fn fold_call(fans_out: bool, outcomes: Vec) -> Result { + if fans_out { + return Ok(fold(&outcomes)); + } + + match outcomes.into_iter().next() { + Some(OperationOutcome::Ok(content)) => Ok(content), + Some(OperationOutcome::Error(message)) => Err(message), + Some(OperationOutcome::NotRun { .. }) | None => Err("Tool did not complete".to_owned()), + } +} + /// Whether the outcomes so far mean no further operation should start. #[must_use] pub fn should_stop(fan_out: FanOut, outcomes: &[OperationOutcome]) -> bool { diff --git a/crates/jp_llm/src/tool/fan_out_tests.rs b/crates/jp_llm/src/tool/fan_out_tests.rs index 198756902..0a38b35ae 100644 --- a/crates/jp_llm/src/tool/fan_out_tests.rs +++ b/crates/jp_llm/src/tool/fan_out_tests.rs @@ -40,6 +40,60 @@ fn envelope_wraps_the_operation_schema_in_a_required_array() { ); } +/// A same-document reference is anchored at the document root, so the `$defs` +/// block it points at has to move to the envelope's root when the operation +/// schema is nested under it. +#[test] +fn envelope_hoists_definitions_so_references_still_resolve() { + let operation = json!({ + "type": "object", + "properties": { + "kinds": { "type": "array", "items": { "$ref": "#/$defs/EntryType" } } + }, + "required": ["kinds"], + "$defs": { "EntryType": { "type": "string", "enum": ["Enum", "Method"] } }, + }); + + let wrapped = envelope(&operation); + + assert_eq!( + wrapped["$defs"], + json!({ "EntryType": { "type": "string", "enum": ["Enum", "Method"] } }), + "the definitions block sits at the root the references name" + ); + assert_eq!( + wrapped["properties"]["ops"]["items"]["$defs"], + Value::Null, + "and is gone from the nested copy, so it is defined exactly once" + ); + assert_eq!( + wrapped["properties"]["ops"]["items"]["properties"]["kinds"]["items"], + json!({ "$ref": "#/$defs/EntryType" }), + "the reference itself is untouched" + ); +} + +/// The older `definitions` spelling moves too. +#[test] +fn envelope_hoists_the_legacy_definitions_spelling() { + let operation = json!({ + "type": "object", + "properties": { "kind": { "$ref": "#/definitions/Kind" } }, + "definitions": { "Kind": { "type": "string" } }, + }); + + let wrapped = envelope(&operation); + + assert_eq!( + wrapped["definitions"], + json!({ "Kind": { "type": "string" } }) + ); + assert_eq!( + wrapped["properties"]["ops"]["items"]["definitions"], + Value::Null + ); +} + #[test] fn expand_returns_one_argument_map_per_operation() { let arguments = args(&json!({