From 815bcd2e8beb70488ed1aabd904f38bc4ac75fe4 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Wed, 16 Sep 2026 19:05:10 +0200 Subject: [PATCH] feat(tool): Delay rendering until questions are answered Custom tool formatters receive the full execution context and defer display until their questions are answered, so approval previews match the call that runs. Ticket creation asks the assistant to shorten titles over 60 characters before filing them, preserving the complete title in both the heading and filename. Signed-off-by: Jean Mertz --- .config/jp/tools/src/ticket.rs | 76 ++++- .config/jp/tools/src/ticket_tests.rs | 260 +++++++++++++++++- .jp/mcp/tools/ticket/create.toml | 14 +- crates/internal/ticket/src/store.rs | 7 +- .../jp_cli/src/cmd/query/tool/coordinator.rs | 192 +++++++++++-- .../src/cmd/query/tool/coordinator_tests.rs | 14 +- crates/jp_cli/src/cmd/query/tool/executor.rs | 26 +- .../jp_cli/src/cmd/query/turn_loop_tests.rs | 16 ++ crates/jp_cli/src/render/tool.rs | 137 +++++---- crates/jp_cli/src/render/tool_tests.rs | 134 ++++++++- ...__tool_context_renders_the_full_shape.snap | 41 +++ crates/jp_llm/src/tool.rs | 90 ++++-- crates/jp_llm/src/tool/executor.rs | 17 +- crates/jp_llm/src/tool_tests.rs | 32 +++ 14 files changed, 932 insertions(+), 124 deletions(-) create mode 100644 crates/jp_llm/src/snapshots/jp_llm__tool__tests__tool_context_renders_the_full_shape.snap diff --git a/.config/jp/tools/src/ticket.rs b/.config/jp/tools/src/ticket.rs index 3b6f1339b..347da1201 100644 --- a/.config/jp/tools/src/ticket.rs +++ b/.config/jp/tools/src/ticket.rs @@ -23,7 +23,8 @@ use comfort::{ DEFAULT_MAX_WIDTH, format::{FormatOptions, format_markdown_with}, }; -use serde_json::Value; +use jp_tool::Question; +use serde_json::{Map, Value}; use crate::{ Context, Tool, @@ -33,6 +34,20 @@ use crate::{ /// The handle tickets and comments written by the assistant carry. const HANDLE: &str = "jp"; +/// The longest title `ticket_create` accepts, in characters, unless +/// `options.max_title_length` overrides it (both routes carry the option). +/// +/// Tied to the slug budget so the filename carries the whole title: a slug is +/// never longer than the title it came from, since each character contributes +/// at most one ASCII byte. +const DEFAULT_MAX_TITLE_LENGTH: usize = store::MAX_SLUG_SIZE; + +/// The question `ticket_create` puts back when a title doesn't fit. +/// +/// `create.toml` routes it to the assistant, and the answer must be read under +/// this same id. +const SHORTER_TITLE: &str = "shorter_title"; + #[expect( clippy::needless_pass_by_value, reason = "consistent with other module run fns" @@ -61,6 +76,12 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { Err(refusal) => return error(refusal), }; + let limit = t.option_or("max_title_length", DEFAULT_MAX_TITLE_LENGTH); + let title = match fitting_title(&title, &t.answers, limit) { + Ok(title) => title, + Err(outcome) => return outcome, + }; + if ctx.action.is_format_arguments() { let date = Local::now().format("%Y-%m-%d").to_string(); return Ok(preview_create( @@ -181,6 +202,59 @@ fn create( Ok(format!("Created {id} at {}", relative(root, &path)).into()) } +/// Settle on a title of at most `limit` characters. +/// +/// A title over the limit is put back as a [`SHORTER_TITLE`] question, so the +/// ticket can be retitled without the body being sent a second time. +/// The answer replaces the title outright; one that is itself unusable ends the +/// call, because asking again under the same id would put the tool straight +/// back where it started. +/// +/// The error variant is the outcome to return in place of the caller's work. +/// Both actions go through here: a preview drawn before the title is settled +/// would show a heading and a filename the ticket won't carry, so the question +/// is put back there too and the preview is drawn once the answer is in. +fn fitting_title( + title: &str, + answers: &Map, + limit: usize, +) -> Result { + let Some(answer) = answers.get(SHORTER_TITLE) else { + let title = title.trim(); + let count = title.chars().count(); + if count <= limit { + return Ok(title.to_owned()); + } + + let question = match Question::text( + SHORTER_TITLE, + format!( + "The title is {count} characters, and a ticket takes at most {limit}. Give a \ + shorter title for: {title}" + ), + ) { + Ok(question) => question, + Err(err) => return Err(Err(err.into())), + }; + + return Err(Ok(question.into())); + }; + + let retitled = answer.as_str().unwrap_or_default().trim(); + if retitled.is_empty() { + return Err(error("The shorter title was empty.")); + } + + let count = retitled.chars().count(); + if count > limit { + return Err(error(format!( + "The shorter title is {count} characters, and a ticket takes at most {limit}." + ))); + } + + Ok(retitled.to_owned()) +} + /// Render the ticket file `create` is about to write. /// /// The id is left out because the file doesn't carry one: it is drawn when the diff --git a/.config/jp/tools/src/ticket_tests.rs b/.config/jp/tools/src/ticket_tests.rs index dd8dd54e0..90400ceb0 100644 --- a/.config/jp/tools/src/ticket_tests.rs +++ b/.config/jp/tools/src/ticket_tests.rs @@ -44,16 +44,41 @@ fn enum_values(schema: &toml::Value, what: &str) -> Vec { /// Drive a tool through the public `run` entry point, exercising argument /// parsing and dispatch. fn run_tool(dir: &Utf8TempDir, name: &str, args: Value) -> ToolResult { - dispatch(dir, Action::Run, name, args) + dispatch(dir, Action::Run, name, args, json!({}), json!({})) +} + +/// Drive a tool the way JP re-executes it once a question has been answered. +fn run_tool_with_answers(dir: &Utf8TempDir, name: &str, args: Value, answers: Value) -> ToolResult { + dispatch(dir, Action::Run, name, args, answers, json!({})) +} + +/// Drive a tool with the declaration-level options JP passes alongside the +/// arguments. +fn run_tool_with_options(dir: &Utf8TempDir, name: &str, args: Value, options: Value) -> ToolResult { + dispatch(dir, Action::Run, name, args, json!({}), options) } /// Drive a tool through the argument-formatting path JP takes before asking for /// approval. fn preview_tool(dir: &Utf8TempDir, name: &str, args: Value) -> ToolResult { - dispatch(dir, Action::FormatArguments, name, args) + dispatch( + dir, + Action::FormatArguments, + name, + args, + json!({}), + json!({}), + ) } -fn dispatch(dir: &Utf8TempDir, action: Action, name: &str, args: Value) -> ToolResult { +fn dispatch( + dir: &Utf8TempDir, + action: Action, + name: &str, + args: Value, + answers: Value, + options: Value, +) -> ToolResult { let ctx = Context { root: dir.path().to_path_buf(), action, @@ -65,12 +90,20 @@ fn dispatch(dir: &Utf8TempDir, action: Action, name: &str, args: Value) -> ToolR Value::Object(map) => map, _ => serde_json::Map::new(), }; + let answers = match answers { + Value::Object(map) => map, + _ => serde_json::Map::new(), + }; + let options = match options { + Value::Object(map) => map, + _ => serde_json::Map::new(), + }; run(ctx, Tool { name: name.to_owned(), arguments, - answers: serde_json::Map::new(), - options: serde_json::Map::new(), + answers, + options, }) } @@ -94,6 +127,13 @@ fn error_message(result: ToolResult) -> String { } } +fn question(result: ToolResult) -> Question { + match result.expect("tool result") { + Outcome::NeedsInput { question } => question, + other => panic!("expected a question, got: {other:?}"), + } +} + fn create_ticket(dir: &Utf8TempDir, title: &str) -> String { content(run_tool( dir, @@ -206,6 +246,51 @@ fn create_preview_renders_the_file_that_will_be_written() { ); } +/// The preview shows the document that will be written, so a title still +/// waiting to be shortened has nothing to show yet: it asks the same question +/// the run would, and JP comes back once the answer is in. +#[test] +fn create_preview_asks_before_drawing_a_title_that_will_change() { + let dir = Utf8TempDir::new().unwrap(); + + let question = question(preview_tool( + &dir, + "ticket_create", + json!({ + "kind": "chore", + "title": "Make provider fixtures deterministic and sanitize recorded model output", + "body": "Something is wrong." + }), + )); + + assert_eq!(question.id, "shorter_title"); +} + +/// The answered preview draws the ticket under the title that will be filed, +/// not the one the assistant first asked for. +#[test] +fn create_preview_draws_the_answered_title() { + let dir = Utf8TempDir::new().unwrap(); + + let out = strip_ansi(content(dispatch( + &dir, + Action::FormatArguments, + "ticket_create", + json!({ + "kind": "chore", + "title": "Make provider fixtures deterministic and sanitize recorded model output", + "body": "Something is wrong." + }), + json!({ "shorter_title": "Sanitize recorded model output in fixtures" }), + json!({}), + ))); + + assert!( + out.starts_with("> # Sanitize recorded model output in fixtures\n"), + "{out}" + ); +} + /// A preview leaves the board exactly as it found it: no file, and no id drawn /// that the ticket it previews won't carry. #[test] @@ -452,6 +537,171 @@ fn create_rejects_an_empty_title() { assert_eq!(out, "`title` must not be empty."); } +/// A title past the slug budget loses its tail in the filename, so it is put +/// back as a question instead of being filed under a truncated name. +#[test] +fn create_asks_for_a_shorter_title() { + let dir = Utf8TempDir::new().unwrap(); + + let question = question(run_tool( + &dir, + "ticket_create", + json!({ + "kind": "chore", + "title": "Make provider fixtures deterministic and sanitize recorded model output", + "body": "Something is wrong." + }), + )); + + assert_eq!(question.id, "shorter_title"); + assert_eq!( + question.text, + "The title is 71 characters, and a ticket takes at most 60. Give a shorter title for: \ + Make provider fixtures deterministic and sanitize recorded model output" + ); + assert!( + ids(&dir).is_empty(), + "the ticket was filed before the answer" + ); +} + +/// The answer replaces the title, and the rest of the call is the one that was +/// already made: the body does not travel a second time. +#[test] +fn a_shorter_title_answer_files_the_ticket() { + let dir = Utf8TempDir::new().unwrap(); + + let out = content(run_tool_with_answers( + &dir, + "ticket_create", + json!({ + "kind": "chore", + "title": "Make provider fixtures deterministic and sanitize recorded model output", + "body": "Something is wrong." + }), + json!({ "shorter_title": "Sanitize recorded model output in fixtures" }), + )); + + assert!( + out.ends_with("-sanitize-recorded-model-output-in-fixtures.md"), + "{out}" + ); + + let id = ids(&dir).pop().expect("one ticket"); + let ticket = content(run_tool( + &dir, + "ticket_show", + json!({ "id": id.to_string() }), + )); + assert!( + ticket.starts_with(&format!( + "```markdown\n# {id}: Sanitize recorded model output in fixtures\n" + )), + "{ticket}" + ); + assert!(ticket.contains("Something is wrong."), "{ticket}"); +} + +/// The question is asked once. +/// A replacement that doesn't fit either ends the call, rather than being asked +/// again under an id that already has an answer. +#[test] +fn a_shorter_title_answer_that_still_does_not_fit_ends_the_call() { + let dir = Utf8TempDir::new().unwrap(); + + let out = error_message(run_tool_with_answers( + &dir, + "ticket_create", + json!({ "kind": "chore", "title": "A title that does not fit" }), + json!({ + "shorter_title": + "Make provider fixtures deterministic and sanitize recorded model output" + }), + )); + + assert_eq!( + out, + "The shorter title is 71 characters, and a ticket takes at most 60." + ); + assert!(ids(&dir).is_empty(), "a refused title filed a ticket"); +} + +#[test] +fn an_empty_shorter_title_answer_ends_the_call() { + let dir = Utf8TempDir::new().unwrap(); + + let out = error_message(run_tool_with_answers( + &dir, + "ticket_create", + json!({ "kind": "chore", "title": "A title that does not fit" }), + json!({ "shorter_title": " " }), + )); + + assert_eq!(out, "The shorter title was empty."); +} + +/// The boundary is inclusive: a title that fits exactly is filed under a slug +/// that carries all of it. +#[test] +fn create_accepts_a_title_that_fills_the_slug() { + let dir = Utf8TempDir::new().unwrap(); + + let out = create_ticket( + &dir, + "Sanitize recorded model output in the provider test fixtures", + ); + + assert!( + out.ends_with("-sanitize-recorded-model-output-in-the-provider-test-fixtures.md"), + "{out}" + ); +} + +#[test] +fn the_title_limit_is_configurable() { + let dir = Utf8TempDir::new().unwrap(); + + let question = question(run_tool_with_options( + &dir, + "ticket_create", + json!({ "kind": "chore", "title": "Bump the deny list" }), + json!({ "max_title_length": 10 }), + )); + + assert_eq!( + question.text, + "The title is 18 characters, and a ticket takes at most 10. Give a shorter title for: \ + Bump the deny list" + ); +} + +/// The declaration routes the question and advertises the limit, both by hand, +/// so both have to be checked against what the tool does. +#[test] +fn create_declares_the_question_and_the_limit_it_enforces() { + let declaration = declaration("create.toml"); + let tool = &declaration["conversation"]["tools"]["ticket_create"]; + + assert_eq!( + tool["questions"][SHORTER_TITLE]["target"].as_str(), + Some("assistant"), + "the retitle question is not routed to the assistant" + ); + + let limit = format!("{DEFAULT_MAX_TITLE_LENGTH} characters"); + for field in [ + tool["description"].as_str().expect("description"), + tool["parameters"]["title"]["summary"] + .as_str() + .expect("title summary"), + ] { + assert!( + field.contains(&limit), + "ticket_create does not advertise the limit `{limit}`: {field}" + ); + } +} + #[test] fn comments_are_attributed_to_the_assistant_and_numbered() { let dir = Utf8TempDir::new().unwrap(); diff --git a/.jp/mcp/tools/ticket/create.toml b/.jp/mcp/tools/ticket/create.toml index ff9693381..87b50b9ed 100644 --- a/.jp/mcp/tools/ticket/create.toml +++ b/.jp/mcp/tools/ticket/create.toml @@ -14,9 +14,10 @@ tool call header misaligns below 80 columns" is a ticket even if the fix is subtle; "how should tool output be bounded?" is an RFD even if the implementation is trivial. -The ticket opens at status `Todo`, is numbered from a counter that never reuses -an id, and is attributed to you (`jp`). The title becomes the filename slug, so -keep it short and specific. +The ticket opens at status `Todo`, draws an id no other checkout will hand out, +and is attributed to you (`jp`). The title becomes the filename slug, so +keep it short and specific: at most 60 characters, or the tool asks you for a +shorter one before it files anything. """ examples = """ @@ -33,6 +34,11 @@ File a chore with no description: ``` """ +# A title past the limit is put back as a question rather than an error, so the +# ticket is retitled without the body being sent a second time. The assistant +# wrote the title, so the assistant shortens it. +questions.shorter_title.target = "assistant" + [conversation.tools.ticket_create.style] parameters = "just serve-tools {{context}} {{tool}}" inline_results = "full" @@ -47,7 +53,7 @@ summary = "What kind of work the ticket describes." [conversation.tools.ticket_create.parameters.title] type = "string" required = true -summary = "One-line summary of the work. Becomes the heading and the filename slug." +summary = "One-line summary of the work, at most 60 characters. Becomes the heading and the filename slug." [conversation.tools.ticket_create.parameters.body] type = "string" diff --git a/crates/internal/ticket/src/store.rs b/crates/internal/ticket/src/store.rs index e469318f9..c375b29a5 100644 --- a/crates/internal/ticket/src/store.rs +++ b/crates/internal/ticket/src/store.rs @@ -28,6 +28,11 @@ use crate::{ /// Directory holding the ticket files, relative to the workspace root. pub const DEFAULT_DIR: &str = "docs/ticket"; +/// Longest slug a ticket filename carries; a title beyond this is cut. +/// +/// Slugs are ASCII, so this is both a byte count and a character count. +pub const MAX_SLUG_SIZE: usize = 60; + /// Start of the id time range: `2026-08-10T00:00:00Z`, the day the ticket /// system went live. const EPOCH_SECS: u64 = 1_786_320_000; @@ -733,7 +738,7 @@ fn slug(title: &str) -> String { // Every pushed character is ASCII, so a byte index is a character index. let slug = slug.trim_matches('-'); - let cut = slug.len().min(60); + let cut = slug.len().min(MAX_SLUG_SIZE); match slug[..cut].trim_end_matches('-') { "" => "untitled".to_owned(), diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 93f7ea56b..2efb2f883 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -78,7 +78,7 @@ //! The coordinator uses the [`Executor`] trait for tool execution. use std::{ - collections::{HashMap, VecDeque}, + collections::{HashMap, HashSet, VecDeque}, sync::Arc, }; @@ -107,7 +107,7 @@ use jp_llm::tool::{ }; use jp_mcp::Client; use jp_printer::Printer; -use jp_tool::{AnswerType, Question}; +use jp_tool::{AccessPolicy, AnswerType, Question}; use jp_workspace::ConversationMut; use serde_json::{Map, Value}; use tokio::sync::mpsc; @@ -332,6 +332,20 @@ pub enum ToolCallDecision { Failed(ToolCallResponse), } +/// Result of [`ToolCoordinator::pre_render_for_prompt`]. +#[derive(Debug)] +pub(crate) enum PreRender { + /// The call was rendered. + /// `content` is custom formatter output to persist. + Done { content: Option }, + /// No pre-render was attempted: a custom formatter with `format = "ask"` + /// only runs once the user has approved the call. + Skipped, + /// The formatter asked a question. + /// Nothing was printed, and the call is rendered after it runs. + Deferred, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum ToolCallState { ReceivingArguments { name: String }, @@ -387,6 +401,12 @@ pub struct ToolCoordinator { /// Keyed by tool call ID. /// Drained by the turn loop to write into event metadata. rendered_arguments: HashMap, + /// Tool call IDs whose formatter asked a question instead of describing the + /// call. + /// + /// Nothing was printed for these; they are rendered once the call has run + /// and its answers are known. + deferred_renders: HashSet, } impl ToolCoordinator { @@ -399,6 +419,7 @@ impl ToolCoordinator { executor_source, cancellation_token: CancellationToken::new(), rendered_arguments: HashMap::new(), + deferred_renders: HashSet::new(), } } @@ -478,21 +499,19 @@ impl ToolCoordinator { /// via `format = "unattended"`; otherwise rendering is deferred until after /// approval. /// - /// Returns: + /// Returns [`PreRender`], or `Err(error_message)` if a custom formatter + /// command failed — the caller should treat that as a tool failure and + /// skip prompting. /// - /// - `Ok(Some(content))` if pre-render fired successfully — caller should - /// skip the post-approval render and use this content. - /// - `Ok(None)` if pre-render was suppressed (Custom style with `format = - /// "ask"`) — caller should follow the existing post-approval render - /// path. - /// - `Err(error_message)` if a custom formatter command failed — caller - /// should treat this as a tool failure and skip prompting. + /// The formatter is called with no answers: none exist before the call + /// runs. pub(crate) async fn pre_render_for_prompt( &self, tool_name: &str, arguments: &Map, + access: Option<&AccessPolicy>, tool_renderer: &ToolRenderer, - ) -> Result>, String> { + ) -> Result { // `FormatMode::Ask` exists to defer side-effecting *custom* // formatters until after approval — running a user-configured // shell command before the user okays the tool would be @@ -506,14 +525,21 @@ impl ToolCoordinator { }; if !should_pre_render { - return Ok(None); + return Ok(PreRender::Skipped); } match self - .render_approved_tool(tool_name, arguments, tool_renderer) + .render_approved_tool( + tool_name, + arguments, + &IndexMap::new(), + access, + tool_renderer, + ) .await { - RenderOutcome::Rendered { content } => Ok(Some(content)), + RenderOutcome::Rendered { content } => Ok(PreRender::Done { content }), + RenderOutcome::Deferred => Ok(PreRender::Deferred), RenderOutcome::Suppressed { error } => Err(error), } } @@ -546,6 +572,7 @@ impl ToolCoordinator { /// execute. /// 3. For approved tools, render the call (skipping if pre-rendered). /// 4. Return [`ToolCallDecision::Approved`], `Skipped`, or `Failed`. + #[expect(clippy::too_many_lines)] pub(crate) async fn resolve_tool_call_decision( &mut self, executor: Box, @@ -575,16 +602,43 @@ impl ToolCoordinator { PermissionDecision::NeedsPrompt { executor, info } => { self.set_tool_state(&info.tool_id, ToolCallState::AwaitingPermission); + // Grants that don't compile fail the call here rather than one + // round later at execution: the formatter is about to run under + // them too. + let access = match Self::render_access(executor.as_ref(), tool_renderer) { + Ok(access) => access, + Err(error) => { + return ToolCallDecision::Failed(Self::render_failed_response( + info.tool_id.clone(), + &info.tool_name, + &error, + )); + } + }; + // Pre-render before the prompt so the user sees the // rendered call (not raw arguments) when deciding. // Built-in parameter styles always pre-render; Custom // formatters are gated on `format = "unattended"` // because they shell out to a user-controlled command. let pre = match self - .pre_render_for_prompt(&info.tool_name, executor.arguments(), tool_renderer) + .pre_render_for_prompt( + &info.tool_name, + executor.arguments(), + access.as_ref(), + tool_renderer, + ) .await { - Ok(maybe_content) => maybe_content, + Ok(PreRender::Done { content }) => Some(content), + Ok(PreRender::Skipped) => None, + // Nothing was printed and nothing will be until the call + // has run. Counted as pre-rendered so step 3 doesn't run + // the formatter a second time to the same answer. + Ok(PreRender::Deferred) => { + self.deferred_renders.insert(info.tool_id.clone()); + Some(None) + } Err(error) => { return ToolCallDecision::Failed(Self::render_failed_response( info.tool_id.clone(), @@ -622,11 +676,31 @@ impl ToolCoordinator { } else { let tool_name = executor.tool_name().to_owned(); let args = executor.arguments().clone(); + let access = match Self::render_access(executor.as_ref(), tool_renderer) { + Ok(access) => access, + Err(error) => { + let id = executor.tool_id().to_owned(); + return ToolCallDecision::Failed(Self::render_failed_response( + id, &tool_name, &error, + )); + } + }; + match self - .render_approved_tool(&tool_name, &args, tool_renderer) + .render_approved_tool( + &tool_name, + &args, + &IndexMap::new(), + access.as_ref(), + tool_renderer, + ) .await { RenderOutcome::Rendered { content } => content, + RenderOutcome::Deferred => { + self.deferred_renders.insert(executor.tool_id().to_owned()); + None + } RenderOutcome::Suppressed { error } => { let id = executor.tool_id().to_owned(); return ToolCallDecision::Failed(Self::render_failed_response( @@ -765,6 +839,8 @@ impl ToolCoordinator { &self, tool_name: &str, arguments: &serde_json::Map, + answers: &IndexMap, + access: Option<&AccessPolicy>, tool_renderer: &ToolRenderer, ) -> RenderOutcome { if self.is_hidden(tool_name) { @@ -772,11 +848,39 @@ impl ToolCoordinator { } let style = self.parameter_style(tool_name); + let options = self + .tools_config + .get(tool_name) + .map(|config| config.options().clone()) + .unwrap_or_default(); + tool_renderer - .render_approved(tool_name, &self.invoked_name(tool_name), arguments, &style) + .render_approved( + tool_name, + &self.invoked_name(tool_name), + arguments, + answers, + &options, + access, + &style, + ) .await } + /// The access policy to hand a formatter describing `executor`'s call. + /// + /// Compiled against the directory the formatter runs in, which is the + /// renderer's root. + /// Grants that fail to compile are reported so the caller can fail the call + /// rather than describe it under a policy that isn't the one it would run + /// under. + fn render_access( + executor: &dyn Executor, + tool_renderer: &ToolRenderer, + ) -> Result, String> { + executor.access(tool_renderer.root()) + } + /// Determines permission for a single tool without blocking on user input. /// /// Does NOT render any output. @@ -1085,7 +1189,8 @@ impl ToolCoordinator { turn_state, interactive, tool_renderer, - ); + ) + .await; } ExecutionEvent::PromptAnswer { index, @@ -1505,7 +1610,7 @@ impl ToolCoordinator { #[allow(clippy::too_many_arguments)] #[allow(clippy::too_many_lines)] - fn handle_tool_result( + async fn handle_tool_result( &mut self, result: ExecutorResult, tool: &mut ExecutingTool, @@ -1527,6 +1632,9 @@ impl ToolCoordinator { match result { ExecutorResult::Completed(response) => { let is_error = response.result.is_err(); + self.render_deferred_call(tool, is_error, tool_renderer) + .await; + let (inline_results, results_file_link) = self .tools_config .get(&tool.tool_name) @@ -1769,6 +1877,50 @@ impl ToolCoordinator { } } + /// Render a call whose formatter deferred at permission time. + /// + /// The answers are in hand now, so the formatter describes the call as it + /// actually ran. + /// A call that ended in an error is left off the display: the formatter + /// already said it could not describe it, and the error response is + /// rendered on its own. + /// + /// Does nothing for a call that was already rendered. + async fn render_deferred_call( + &mut self, + tool: &ExecutingTool, + is_error: bool, + tool_renderer: &ToolRenderer, + ) { + if !self.deferred_renders.remove(&tool.tool_id) || is_error { + return; + } + + // The call ran, so its grants compiled. An error here can't be acted + // on anyway: the work is done and the result is about to be shown. + let Ok(access) = Self::render_access(tool.executor.as_ref(), tool_renderer) else { + return; + }; + + let outcome = self + .render_approved_tool( + &tool.tool_name, + tool.executor.arguments(), + &tool.accumulated_answers, + access.as_ref(), + tool_renderer, + ) + .await; + + if let RenderOutcome::Rendered { + content: Some(content), + } = outcome + { + self.rendered_arguments + .insert(tool.tool_id.clone(), content); + } + } + #[allow(clippy::too_many_arguments)] fn handle_prompt_answer( &mut self, 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 b0565e273..cf65478ba 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -305,13 +305,13 @@ async fn test_pre_render_for_prompt_function_call_fires_before_approval() { args.insert("path".into(), Value::String("src/foo.rs".into())); let result = coordinator - .pre_render_for_prompt("fs_delete_file", &args, &tool_renderer) + .pre_render_for_prompt("fs_delete_file", &args, None, &tool_renderer) .await; // Non-Custom styles should always pre-render. `content` is `None` // because only Custom formatters produce persistable rendered content. assert!( - matches!(result, Ok(Some(None))), + matches!(result, Ok(PreRender::Done { content: None })), "pre-render should fire for FunctionCall style, got: {result:?}" ); @@ -367,11 +367,11 @@ async fn test_pre_render_for_prompt_custom_ask_defers_rendering() { ); let result = coordinator - .pre_render_for_prompt("custom_tool", &Map::new(), &tool_renderer) + .pre_render_for_prompt("custom_tool", &Map::new(), None, &tool_renderer) .await; assert!( - matches!(result, Ok(None)), + matches!(result, Ok(PreRender::Skipped)), "Custom + format=ask should defer rendering, got: {result:?}" ); @@ -414,6 +414,9 @@ impl Executor for EditableExecutor { self.arguments = map; } } + fn access(&self, _root: &camino::Utf8Path) -> Result, String> { + Ok(None) + } async fn execute( &self, _answers: &IndexMap, @@ -819,7 +822,7 @@ async fn custom_formatter_receives_the_invoked_tool_name() { ); let outcome = coordinator - .render_approved_tool("ls", &Map::new(), &tool_renderer) + .render_approved_tool("ls", &Map::new(), &IndexMap::new(), None, &tool_renderer) .await; match outcome { @@ -827,6 +830,7 @@ async fn custom_formatter_receives_the_invoked_tool_name() { assert_eq!(content.as_deref(), Some("fs_list_files")); } RenderOutcome::Suppressed { error } => panic!("custom formatter failed: {error}"), + RenderOutcome::Deferred => panic!("custom formatter asked a question"), } // The header the user reads stays the name the assistant called. diff --git a/crates/jp_cli/src/cmd/query/tool/executor.rs b/crates/jp_cli/src/cmd/query/tool/executor.rs index f8f0a64b1..6b2e2aeb5 100644 --- a/crates/jp_cli/src/cmd/query/tool/executor.rs +++ b/crates/jp_cli/src/cmd/query/tool/executor.rs @@ -58,6 +58,7 @@ use jp_llm::{ }, }; use jp_mcp::Client; +use jp_tool::AccessPolicy; use serde_json::Value; use tokio_util::sync::CancellationToken; @@ -212,6 +213,15 @@ impl Executor for ToolExecutor { // If not an object, ignore (preserve original arguments) } + fn access(&self, root: &Utf8Path) -> Result, String> { + compile_tool_policy(self.config.access(), root, &self.approvals).map_err(|error| { + format!( + "invalid access policy for tool '{}': {error}", + self.request.name + ) + }) + } + async fn execute( &self, answers: &IndexMap, @@ -220,19 +230,15 @@ impl Executor for ToolExecutor { cancellation_token: CancellationToken, stderr: Option, ) -> ExecutorResult { - // Compile this tool's access grants into a runtime policy, baking - // approved external targets in. The policy travels to the tool in its - // context so the tool can self-enforce. A policy that fails to compile - // (invalid config) fails the tool rather than running it unenforced. - let access = match compile_tool_policy(self.config.access(), root, &self.approvals) { + // The policy travels to the tool in its context so the tool can + // self-enforce. A policy that fails to compile (invalid config) fails + // the tool rather than running it unenforced. + let access = match self.access(root) { Ok(access) => access, - Err(error) => { + Err(message) => { return ExecutorResult::Completed(ToolCallResponse { id: self.request.id.clone(), - result: Err(format!( - "invalid access policy for tool '{}': {error}", - self.request.name - )), + result: Err(message), }); } }; 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 f3dd7dae3..1f653644f 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1258,6 +1258,10 @@ impl Executor for SleepingExecutor { fn set_arguments(&mut self, _args: Value) {} + fn access(&self, _root: &Utf8Path) -> Result, String> { + Ok(None) + } + async fn execute( &self, _answers: &IndexMap, @@ -4888,6 +4892,10 @@ impl Executor for TalkingExecutor { fn set_arguments(&mut self, _args: Value) {} + fn access(&self, _root: &Utf8Path) -> Result, String> { + Ok(None) + } + async fn execute( &self, _answers: &IndexMap, @@ -5829,6 +5837,10 @@ impl Executor for AskingTalkingExecutor { fn set_arguments(&mut self, _args: Value) {} + fn access(&self, _root: &Utf8Path) -> Result, String> { + Ok(None) + } + async fn execute( &self, answers: &IndexMap, @@ -5903,6 +5915,10 @@ impl Executor for InquiryMockExecutor { } fn set_arguments(&mut self, _args: Value) {} + fn access(&self, _root: &camino::Utf8Path) -> Result, String> { + Ok(None) + } + async fn execute( &self, answers: &IndexMap, diff --git a/crates/jp_cli/src/render/tool.rs b/crates/jp_cli/src/render/tool.rs index e499ec5a2..42806a384 100644 --- a/crates/jp_cli/src/render/tool.rs +++ b/crates/jp_cli/src/render/tool.rs @@ -13,21 +13,27 @@ use std::{ use camino::{Utf8Path, Utf8PathBuf}; use crossterm::style::Stylize as _; +use indexmap::IndexMap; use jp_config::{ conversation::tool::{ CommandConfig, style::{InlineResults, LinkStyle, ParametersStyle, TruncateLines}, }, style::{StyleConfig, stderr_rows::StderrRows}, + types::json_value::JsonValue, }; use jp_conversation::event::ToolCallResponse; -use jp_llm::{CommandResult, run_tool_command, tool::InvocationContext}; +use jp_llm::{ + CommandResult, run_tool_command, + tool::{InvocationContext, ToolContext}, +}; use jp_md::{ format::{DefaultBackground, Formatter}, shade::ShadedWriter, }; use jp_printer::{ErrChannel, LineSink, OutputLines, RegionStyle, StatusRegion}; use jp_term::osc::hyperlink; +use jp_tool::AccessPolicy; use serde_json::{Map, Value}; use tokio_util::sync::CancellationToken; use tracing::warn; @@ -59,6 +65,12 @@ pub enum RenderOutcome { /// Header and arguments (if any) were printed. /// If a custom formatter produced output, it's returned for persistence. Rendered { content: Option }, + /// The custom formatter answered with a question rather than output: it + /// cannot describe the call until that question is answered. + /// + /// Nothing was printed and the call is still runnable. + /// The caller renders again once the answers are known. + Deferred, /// Custom formatter failed — nothing was printed. Suppressed { /// Error message from the custom formatter. @@ -282,6 +294,12 @@ impl ToolRenderer { /// the header followed by the formatted output. /// If the custom formatter fails, nothing is printed and /// [`RenderOutcome::Suppressed`] is returned. + /// If it answers with a question, nothing is printed and + /// [`RenderOutcome::Deferred`] is returned. + /// + /// The formatter is handed the same [`ToolContext`] the execution route + /// builds, so it reads the call it is describing rather than a subset of + /// it. /// /// On success, returns `Rendered { content }` where `content` is the /// custom-formatted output (if any) so the caller can persist it for @@ -296,12 +314,23 @@ impl ToolRenderer { name: &str, invoked_name: &str, arguments: &Map, + answers: &IndexMap, + options: &IndexMap, + access: Option<&AccessPolicy>, style: &ParametersStyle, ) -> RenderOutcome { if let ParametersStyle::Custom(cmd_config) = style { let cmd = cmd_config.clone().command(); - self.render_custom_tool_call(name, invoked_name, arguments, cmd) - .await + self.render_custom_tool_call( + name, + invoked_name, + arguments, + answers, + options, + access, + cmd, + ) + .await } else { self.render_tool_call(name, arguments, style); RenderOutcome::Rendered { content: None } @@ -313,17 +342,35 @@ impl ToolRenderer { /// Runs the custom formatter command first. /// If it succeeds, prints the "Calling tool X" header followed by the /// formatted output. - /// If it fails, nothing is printed — the tool call is suppressed from the - /// display. + /// If it fails or defers, nothing is printed — the tool call is kept off + /// the display. async fn render_custom_tool_call( &self, name: &str, invoked_name: &str, arguments: &Map, + answers: &IndexMap, + options: &IndexMap, + access: Option<&AccessPolicy>, cmd: CommandConfig, ) -> RenderOutcome { - match format_args_custom(invoked_name, arguments, cmd, &self.root, &self.invocation).await { - Ok(content) if !content.is_empty() => { + let formatted = format_args_custom( + &ToolContext { + action: jp_tool::Action::FormatArguments, + name: invoked_name, + arguments: &Value::Object(arguments.clone()), + answers, + options, + root: &self.root, + access, + invocation: &self.invocation, + }, + cmd, + ) + .await; + + match formatted { + Ok(Some(content)) if !content.is_empty() => { let styled_name = name.yellow().bold(); self.write_chrome(self.current_region.as_ref(), |w| { self.emit_separator_to(w)?; @@ -334,7 +381,7 @@ impl ToolRenderer { content: Some(content), } } - Ok(_) => { + Ok(Some(_)) => { // Custom formatter returned empty — just show the header. let styled_name = name.yellow().bold(); self.write_chrome(self.current_region.as_ref(), |w| { @@ -343,6 +390,7 @@ impl ToolRenderer { }); RenderOutcome::Rendered { content: None } } + Ok(None) => RenderOutcome::Deferred, Err(error) => { warn!(%error, tool = %name, "Custom formatter failed, suppressing tool call display"); RenderOutcome::Suppressed { error } @@ -410,6 +458,11 @@ impl ToolRenderer { .then(|| self.progress.source(tool)) } + /// The directory custom formatter commands are run in. + pub fn root(&self) -> &Utf8Path { + &self.root + } + /// Renders a tool call result with language detection, truncation, and file /// links. /// @@ -812,41 +865,35 @@ fn format_args_json(arguments: Map) -> String { /// Runs a custom arguments formatter command and returns the content. /// -/// `tool_name` is the name the tool is invoked under, which is the name its own +/// `ctx.name` is the name the tool is invoked under, which is the name its own /// implementation answers to rather than the key the assistant called. +/// +/// `Ok(None)` means the formatter answered with a question: it cannot describe +/// the call until that question is answered, and the caller should render again +/// once the answers are known. async fn format_args_custom( - tool_name: &str, - arguments: &Map, + ctx: &ToolContext<'_>, cmd: CommandConfig, - root: &Utf8Path, - invocation: &InvocationContext, -) -> Result { - let ctx = serde_json::json!({ - "tool": { - "name": tool_name, - "arguments": arguments, - }, - "context": { - "action": jp_tool::Action::FormatArguments, - "root": root, - "workspace_id": &invocation.workspace_id, - "conversation_id": &invocation.conversation_id, - }, - }); - - let result = run_tool_command(cmd.clone(), ctx, root, CancellationToken::new(), None) - .await - .map_err(|e| { - warn!( - command = %cmd, - error = %e, - "Custom parameters formatter failed" - ); - format!("Custom parameters formatter '{cmd}' failed: {e}") - })?; +) -> Result, String> { + let result = run_tool_command( + cmd.clone(), + ctx.to_value(), + ctx.root, + CancellationToken::new(), + None, + ) + .await + .map_err(|e| { + warn!( + command = %cmd, + error = %e, + "Custom parameters formatter failed" + ); + format!("Custom parameters formatter '{cmd}' failed: {e}") + })?; match result { - CommandResult::Success(content) => Ok(content.trim().to_owned()), + CommandResult::Success(content) => Ok(Some(content.trim().to_owned())), CommandResult::TransientError { message, trace } => { let detail = CommandResult::format_error(&message, &trace); warn!( @@ -863,16 +910,8 @@ async fn format_args_custom( ); Err(raw) } - CommandResult::NeedsInput(_) => { - warn!( - command = %cmd, - "Custom parameters formatter returned NeedsInput" - ); - Err(format!( - "Custom parameters formatter '{cmd}' returned unexpected NeedsInput" - )) - } - CommandResult::Cancelled => Ok(String::new()), + CommandResult::NeedsInput(_) => Ok(None), + CommandResult::Cancelled => Ok(Some(String::new())), CommandResult::InvalidInquiry { question_id } => { warn!( command = %cmd, @@ -898,7 +937,7 @@ async fn format_args_custom( stdout, success: true, .. - } => Ok(stdout.trim().to_owned()), + } => Ok(Some(stdout.trim().to_owned())), CommandResult::RawOutput { stderr, .. } => { warn!( command = %cmd, diff --git a/crates/jp_cli/src/render/tool_tests.rs b/crates/jp_cli/src/render/tool_tests.rs index d3651d50a..bc6877c63 100644 --- a/crates/jp_cli/src/render/tool_tests.rs +++ b/crates/jp_cli/src/render/tool_tests.rs @@ -9,7 +9,7 @@ use jp_config::{ use jp_conversation::event::ToolCallResponse; use jp_md::format::{BackgroundFill, DefaultBackground}; use jp_printer::{ErrChannel, OutputFormat, Printer, SharedBuffer, TerminalCapability}; -use serde_json::{Map, Value}; +use serde_json::{Map, Value, json}; use super::*; @@ -21,6 +21,33 @@ fn terminal_region() -> DefaultBackground { } } +/// No answers and no options, the shape both render paths pass for a call that +/// has not been asked anything. +fn no_extras() -> (IndexMap, IndexMap) { + (IndexMap::new(), IndexMap::new()) +} + +/// A formatter's view of a call, with the pieces a test wants to vary. +fn format_ctx<'a>( + name: &'a str, + arguments: &'a Value, + answers: &'a IndexMap, + options: &'a IndexMap, + root: &'a Utf8Path, + invocation: &'a jp_llm::tool::InvocationContext, +) -> ToolContext<'a> { + ToolContext { + action: jp_tool::Action::FormatArguments, + name, + arguments, + answers, + options, + root, + access: None, + invocation, + } +} + /// Strip ANSI escape codes for readable snapshots. fn strip_ansi(s: &str) -> String { let bytes = strip_ansi_escapes::strip(s); @@ -199,8 +226,11 @@ async fn test_render_custom_arguments_after_approval() { args.insert("host".into(), Value::String("myhost".into())); let style = ParametersStyle::Custom(CommandConfigOrString::String("echo custom-output".into())); + let (answers, options) = no_extras(); let outcome = renderer - .render_approved("ssh_run", "ssh_run", &args, &style) + .render_approved( + "ssh_run", "ssh_run", &args, &answers, &options, None, &style, + ) .await; assert!(matches!(outcome, RenderOutcome::Rendered { @@ -673,16 +703,87 @@ async fn test_format_custom_content_returns_raw_content() { let mut args = Map::new(); args.insert("key".into(), Value::String("value".into())); let cmd = CommandConfigOrString::String("echo hello-world".into()).command(); - let result = format_args_custom( + let (answers, options) = no_extras(); + let invocation = jp_llm::tool::InvocationContext::default(); + let arguments = Value::Object(args); + let ctx = format_ctx( "my_tool", - &args, - cmd, + &arguments, + &answers, + &options, root.path(), - &jp_llm::tool::InvocationContext::default(), + &invocation, + ); + + let result = format_args_custom(&ctx, cmd).await.unwrap(); + assert_eq!(result.as_deref(), Some("hello-world")); +} + +/// The formatter reads the same call the execution route will run: the answers +/// accumulated so far and the tool's configured options travel with the +/// arguments. +#[tokio::test] +async fn test_format_args_custom_exposes_answers_and_options() { + let root = Utf8TempDir::new().unwrap(); + let cmd = CommandConfigOrString::String( + "echo {{tool.answers.shorter_title}}/{{tool.options.max_title_length}}".into(), ) - .await + .command(); + + let answers = IndexMap::from([("shorter_title".to_owned(), Value::String("Short".into()))]); + let options = IndexMap::from([("max_title_length".to_owned(), JsonValue::from(json!(60)))]); + let invocation = jp_llm::tool::InvocationContext::default(); + let arguments = Value::Object(Map::new()); + let ctx = format_ctx( + "ticket_create", + &arguments, + &answers, + &options, + root.path(), + &invocation, + ); + + let result = format_args_custom(&ctx, cmd).await.unwrap(); + assert_eq!(result.as_deref(), Some("Short/60")); +} + +/// A formatter that answers with a question cannot describe the call yet. +/// Nothing is rendered, and the call is not treated as failed. +#[tokio::test] +async fn test_format_args_custom_question_defers_the_render() { + let root = Utf8TempDir::new().unwrap(); + let (printer, _out, err) = Printer::memory(OutputFormat::TextPretty); + let renderer = ToolRenderer::new( + ErrChannel::new(Arc::new(printer)), + AppConfig::new_test().style, + root.path().to_owned(), + jp_llm::tool::InvocationContext::default(), + ); + + // Serialized rather than hand-written so the test speaks the wire format + // a real tool emits, not a copy of it that can drift. + let payload = serde_json::to_string(&jp_tool::Outcome::NeedsInput { + question: jp_tool::Question::text("q", "Which one?").unwrap(), + }) .unwrap(); - assert_eq!(result, "hello-world"); + let style = ParametersStyle::Custom(CommandConfigOrString::String(format!("echo '{payload}'"))); + + let (answers, options) = no_extras(); + let outcome = renderer + .render_approved( + "my_tool", + "my_tool", + &Map::new(), + &answers, + &options, + None, + &style, + ) + .await; + + assert!(matches!(outcome, RenderOutcome::Deferred), "{outcome:?}"); + renderer.channel.flush(); + assert_eq!(err.lock().as_str(), ""); } /// Regression: the `format_arguments` path must surface the invocation's @@ -703,10 +804,19 @@ async fn test_format_args_custom_exposes_invocation_ids() { workspace_id: "ws-abc".into(), conversation_id: "conv-xyz".into(), }; - let result = format_args_custom("my_tool", &args, cmd, root.path(), &invocation) - .await - .unwrap(); - assert_eq!(result, "ws-abc/conv-xyz"); + let (answers, options) = no_extras(); + let arguments = Value::Object(args); + let ctx = format_ctx( + "my_tool", + &arguments, + &answers, + &options, + root.path(), + &invocation, + ); + + let result = format_args_custom(&ctx, cmd).await.unwrap(); + assert_eq!(result.as_deref(), Some("ws-abc/conv-xyz")); } #[test] diff --git a/crates/jp_llm/src/snapshots/jp_llm__tool__tests__tool_context_renders_the_full_shape.snap b/crates/jp_llm/src/snapshots/jp_llm__tool__tests__tool_context_renders_the_full_shape.snap new file mode 100644 index 000000000..5a87a6b2d --- /dev/null +++ b/crates/jp_llm/src/snapshots/jp_llm__tool__tests__tool_context_renders_the_full_shape.snap @@ -0,0 +1,41 @@ +--- +source: crates/jp_llm/src/tool_tests.rs +expression: rendered +--- +{ + "tool": { + "name": "ticket_create", + "arguments": { + "title": "Bump the deny list" + }, + "answers": { + "shorter_title": "Bump deny list" + }, + "options": { + "max_title_length": 60 + } + }, + "context": { + "action": "format_arguments", + "root": "/tmp", + "access": { + "fs": [ + { + "lexical_path": "docs", + "external": false, + "approved_target": null, + "read": null, + "write": null, + "create": null, + "update": null, + "delete": null, + "execute": null + } + ], + "net": [], + "env": [] + }, + "workspace_id": "ws-abc", + "conversation_id": "conv-xyz" + } +} diff --git a/crates/jp_llm/src/tool.rs b/crates/jp_llm/src/tool.rs index da9db388f..499938d67 100644 --- a/crates/jp_llm/src/tool.rs +++ b/crates/jp_llm/src/tool.rs @@ -11,7 +11,7 @@ use camino::Utf8Path; use indexmap::IndexMap; use jp_config::{ conversation::tool::{CommandConfig, ToolConfigWithDefaults, ToolSource}, - types::command::shell_command_line, + types::{command::shell_command_line, json_value::JsonValue}, }; use jp_conversation::event::ToolCallResponse; use jp_mcp::{ @@ -705,6 +705,68 @@ pub struct InvocationContext { pub conversation_id: String, } +/// Everything a tool command is told about the call it was invoked for. +/// +/// Both routes to a command build one of these: [`Action::Run`] executes the +/// call, [`Action::FormatArguments`] describes it for display. +/// They render to the same JSON, so a formatter reads exactly what the +/// execution will run. +/// +/// The rendered shape is: +/// +/// ```json +/// { +/// "tool": { "name": ..., "arguments": ..., "answers": ..., "options": ... }, +/// "context": { +/// "action": ..., "root": ..., "access": ..., +/// "workspace_id": ..., "conversation_id": ... +/// } +/// } +/// ``` +pub struct ToolContext<'a> { + /// Whether the command is being asked to run the call or to describe it. + pub action: Action, + /// The name the tool is invoked under, which a `source` override can make + /// different from the key the assistant called. + pub name: &'a str, + /// The call arguments, after defaults and coercion. + pub arguments: &'a Value, + /// Answers to questions the tool has asked so far, keyed by question ID. + pub answers: &'a IndexMap, + /// The tool's configured `options` block. + pub options: &'a IndexMap, + /// Directory the command runs in. + pub root: &'a Utf8Path, + /// Filesystem grants the command must confine itself to. + /// + /// `None` grants unrestricted, workspace-confined access. + pub access: Option<&'a jp_tool::AccessPolicy>, + /// Identity of the conversation the call belongs to. + pub invocation: &'a InvocationContext, +} + +impl ToolContext<'_> { + /// Render the context handed to the command template. + #[must_use] + pub fn to_value(&self) -> Value { + json!({ + "tool": { + "name": self.name, + "arguments": self.arguments, + "answers": self.answers, + "options": self.options, + }, + "context": { + "action": self.action, + "root": self.root.as_str(), + "access": self.access, + "workspace_id": &self.invocation.workspace_id, + "conversation_id": &self.invocation.conversation_id, + }, + }) + } +} + /// The definition of a tool. /// /// The definition source is either a [`ToolConfig`] for `local` tools, or a @@ -880,21 +942,17 @@ impl ToolDefinition { } } - let ctx = json!({ - "tool": { - "name": name, - "arguments": &arguments, - "answers": answers, - "options": config.options(), - }, - "context": { - "action": Action::Run, - "root": root.as_str(), - "access": access, - "workspace_id": &invocation.workspace_id, - "conversation_id": &invocation.conversation_id, - }, - }); + let ctx = ToolContext { + action: Action::Run, + name, + arguments: &arguments, + answers, + options: config.options(), + root, + access, + invocation, + } + .to_value(); let Some(command) = config.command() else { return Err(ToolError::MissingCommand); diff --git a/crates/jp_llm/src/tool/executor.rs b/crates/jp_llm/src/tool/executor.rs index adcf78946..aacf0e0d9 100644 --- a/crates/jp_llm/src/tool/executor.rs +++ b/crates/jp_llm/src/tool/executor.rs @@ -6,7 +6,7 @@ use indexmap::IndexMap; use jp_config::conversation::tool::{RunMode, ToolConfigWithDefaults, ToolSource}; use jp_conversation::event::{InquirySource, ToolCallRequest, ToolCallResponse}; use jp_mcp::Client; -use jp_tool::Question; +use jp_tool::{AccessPolicy, Question}; use serde_json::{Map, Value}; use tokio_util::sync::CancellationToken; @@ -55,6 +55,17 @@ pub trait Executor: Send + Sync { /// request. fn set_arguments(&mut self, args: Value); + /// The filesystem grants this call is confined to, compiled against `root`. + /// + /// `Ok(None)` means unrestricted, workspace-confined access. + /// An error means the declared grants could not be compiled and the call + /// must not run: an empty policy reads as unrestricted at the tool, so + /// degrading to one would widen access rather than narrow it. + /// + /// Both the execution and the display of a call read the policy from here, + /// so a formatter is confined to the same paths the run is. + fn access(&self, root: &Utf8Path) -> Result, String>; + /// Executes the tool once with the given answers. /// /// This method performs a single execution pass. @@ -246,6 +257,10 @@ impl Executor for MockExecutor { // result } + fn access(&self, _root: &Utf8Path) -> Result, String> { + Ok(None) + } + async fn execute( &self, _answers: &IndexMap, diff --git a/crates/jp_llm/src/tool_tests.rs b/crates/jp_llm/src/tool_tests.rs index 5173063a8..ea1452864 100644 --- a/crates/jp_llm/src/tool_tests.rs +++ b/crates/jp_llm/src/tool_tests.rs @@ -837,6 +837,38 @@ fn test_split_trims_whitespace() { assert_eq!(d, None); } +/// Both routes to a tool command render this shape, so it is the contract a +/// tool reads: the fields are pinned exactly, and the action is the only thing +/// that distinguishes running a call from describing one. +#[test] +fn test_tool_context_renders_the_full_shape() { + let arguments = json!({ "title": "Bump the deny list" }); + let answers = IndexMap::from([("shorter_title".to_owned(), json!("Bump deny list"))]); + let options = IndexMap::from([("max_title_length".to_owned(), JsonValue::from(json!(60)))]); + let access = jp_tool::AccessPolicy { + fs: vec![jp_tool::FsRule::new("docs")], + ..jp_tool::AccessPolicy::default() + }; + let invocation = InvocationContext { + workspace_id: "ws-abc".to_owned(), + conversation_id: "conv-xyz".to_owned(), + }; + + let ctx = ToolContext { + action: Action::FormatArguments, + name: "ticket_create", + arguments: &arguments, + answers: &answers, + options: &options, + root: "/tmp".into(), + access: Some(&access), + invocation: &invocation, + }; + + let rendered = serde_json::to_string_pretty(&ctx.to_value()).unwrap(); + insta::assert_snapshot!(rendered); +} + /// Regression: `{{tool}}` must render as valid JSON, including `null` for null /// fields (not Jinja2's `none`). /// Originally fixed with `AutoEscape::Json`, now handled by the custom