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.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..14460abf9 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,71 @@ 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 fans_out = self.fan_out.is_some(); + 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(); + + ToolCallResponse { + id: self.tool_id, + result: fan_out::fold_call(fans_out, outcomes), + } + } +} + /// Final outcome of [`ToolCoordinator::resolve_tool_call_decision`] — the /// per-tool permission pipeline. /// @@ -377,7 +449,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 +506,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) { @@ -573,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. @@ -722,7 +804,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 +812,82 @@ impl ToolCoordinator { unavailable } - /// Prepares a single executor for a tool call request. + /// Prepares the executors for a tool call request. + /// + /// 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. /// - /// Returns the executor on success, or an error response if the tool cannot - /// be resolved (e.g. missing from config or definitions). + /// # 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 +943,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 +956,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 +991,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 +1000,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 +1012,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 +1021,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; - for (index, executor) in std::mem::take(&mut self.executors) { + if group.has_work() { + groups.push((index, group)); + } else { + resolved.push((index, group.fold_resolved())); + } + } + + (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 +1107,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 +1145,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 +1161,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 +1180,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 +1210,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, ); } @@ -1073,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 { @@ -1081,7 +1270,7 @@ impl ToolCoordinator { continue; }; let response = &mut results[index]; - self.handle_tool_result( + let failed = self.handle_tool_result( result, tool, index, @@ -1099,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, @@ -1141,7 +1337,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 +1363,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(), @@ -1208,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 @@ -1232,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, @@ -1286,15 +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 = results - .iter() - .enumerate() - .filter(|(_, r)| r.is_none()) - .map(|(i, _)| i) - .collect(); + cancelled_indices = schedule.unfinished(&results); + schedule.abandon_unstarted(); + releasing = false; tools_cancelled = true; cancellation_message = response; if exit { @@ -1306,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); } } @@ -1313,8 +1510,33 @@ impl ToolCoordinator { } } - if results.iter().all(Option::is_some) { - break; + // 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); + } + } + + // 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 releasing { + for (index, executor) in schedule.release(&results) { + self.start_operation( + index, + executor, + &mut executing_tools, + tool_renderer, + mcp_client, + root, + &cancellation_token, + &event_tx, + ); + } } } @@ -1324,36 +1546,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 +1587,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, @@ -1516,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( @@ -1536,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(); @@ -1564,11 +1836,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()), @@ -1591,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( @@ -1616,11 +1888,15 @@ 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); } } } + + // 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, @@ -1675,7 +1951,7 @@ impl ToolCoordinator { event_tx, tool.stderr.clone(), ); - return; + return false; } } @@ -1699,7 +1975,7 @@ impl ToolCoordinator { event_tx, tool.stderr.clone(), ); - return; + return false; } let target = self @@ -1726,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, @@ -1757,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. @@ -1776,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 } } } @@ -1821,7 +2101,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 +2147,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 +2270,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); } @@ -2001,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 6791930a6..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( @@ -489,6 +571,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, @@ -536,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( @@ -546,6 +697,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..cbbee9a6d --- /dev/null +++ b/crates/jp_cli/src/cmd/query/tool/schedule.rs @@ -0,0 +1,368 @@ +//! 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. + /// + /// 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 + .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(); + + (group.plan_index, ToolCallResponse { + id: group.tool_id, + result: fan_out::fold_call(group.fans_out, outcomes), + }) + }) + .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..0f8626074 --- /dev/null +++ b/crates/jp_cli/src/cmd/query/tool/schedule_tests.rs @@ -0,0 +1,480 @@ +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() + ) + ); +} + +/// `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![ + (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..952a9a2b1 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,498 @@ 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 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); + 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 envelope expands into one executor 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 `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] +#[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 +5458,7 @@ fn talking_tool_config(names: &[&str]) -> AppConfig { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, }); } @@ -5452,6 +5966,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 +6520,7 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { options: IndexMap::default(), access: None, cancellation_response: None, + fan_out: None, } } @@ -7115,6 +7631,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 +8067,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 +8500,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")); @@ -8108,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_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..d2e866db9 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": { @@ -38,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] @@ -45,6 +102,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..1ae74a4b8 --- /dev/null +++ b/crates/jp_llm/src/tool/fan_out.rs @@ -0,0 +1,259 @@ +//! 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. +/// +/// 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 { + 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: { + "type": "array", + "minItems": 1, + "description": "The operations to perform. Each element is one complete set of \ + this tool's arguments.", + "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. +#[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 +} + +/// 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 { + 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..0a38b35ae --- /dev/null +++ b/crates/jp_llm/src/tool/fan_out_tests.rs @@ -0,0 +1,261 @@ +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, + }) + ); +} + +/// 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!({ + "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