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