diff --git a/.config/jp/tools/src/ticket.rs b/.config/jp/tools/src/ticket.rs index 2b7926b26..3bd34a12b 100644 --- a/.config/jp/tools/src/ticket.rs +++ b/.config/jp/tools/src/ticket.rs @@ -6,13 +6,18 @@ //! Tickets the assistant writes are attributed to `jp`, and comments are //! rendered with their 1-based positions so a reply can name the comment it //! answers. +//! +//! `ticket_create` and `ticket_comment` also answer the format-arguments +//! action, previewing the document they are about to write in the shape it +//! takes on disk. -use std::path::MAIN_SEPARATOR; +use std::{fs, path::MAIN_SEPARATOR}; // The leading `::` picks the crate over this module, which shares its name. -use ::ticket::{Kind, ParseError, Status, Ticket, TicketId, store}; +use ::ticket::{Comment, Kind, ParseError, Status, Ticket, TicketId, parse, render, store}; use camino::{Utf8Path, Utf8PathBuf}; use chrono::{Local, SecondsFormat, Utc}; +use jp_md::format::Formatter; use serde_json::Value; use crate::{ @@ -35,13 +40,31 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { let Ok(kind) = t.req::("kind")?.parse::() else { return error("`kind` must be one of: bug, feature, chore."); }; - create( - root, - kind, - &t.req::("title")?, - t.opt::("implements")?.as_deref(), - t.opt("body")?, - ) + let title = t.req::("title")?; + let implements = t.opt::("implements")?; + let body = t.opt::("body")?; + + // Checked before the action split so a preview fails too: an + // unattended formatter that errors fails the call ahead of the + // approval prompt, which tells the assistant to fix the arguments + // rather than asking the user about a call that cannot land. + if title.trim().is_empty() { + return error("`title` must not be empty."); + } + + if ctx.action.is_format_arguments() { + let date = Local::now().format("%Y-%m-%d").to_string(); + return Ok(preview_create( + kind, + &title, + implements.as_deref(), + body.as_deref(), + &date, + ) + .into()); + } + + create(root, kind, &title, implements.as_deref(), body) } "comment" => { @@ -49,7 +72,19 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { Ok(id) => id, Err(message) => return error(message), }; - comment(root, id, t.opt("re")?, &t.req::("body")?) + let re = t.opt("re")?; + let body = t.req::("body")?; + + if body.trim().is_empty() { + return error("`body` must not be empty."); + } + + if ctx.action.is_format_arguments() { + let date = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true); + return preview_comment(root, id, re, &body, &date); + } + + comment(root, id, re, &body) } "close" => match id_arg(&t.req("id")?) { @@ -89,10 +124,6 @@ fn create( implements: Option<&str>, body: Option, ) -> ToolResult { - if title.trim().is_empty() { - return error("`title` must not be empty."); - } - let date = Local::now().format("%Y-%m-%d").to_string(); let (id, path) = store::create( &dir(root), @@ -107,11 +138,28 @@ fn create( Ok(format!("Created {id} at {}", relative(root, &path)).into()) } -fn comment(root: &Utf8Path, id: TicketId, re: Option, body: &str) -> ToolResult { - if body.trim().is_empty() { - return error("`body` must not be empty."); - } +/// 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 +/// ticket is claimed, and the result names it. +fn preview_create( + kind: Kind, + title: &str, + implements: Option<&str>, + body: Option<&str>, + date: &str, +) -> String { + preview(&render::ticket( + title.trim(), + kind, + HANDLE, + date, + implements, + body.unwrap_or_default(), + )) +} +fn comment(root: &Utf8Path, id: TicketId, re: Option, body: &str) -> ToolResult { let date = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true); match store::append_comment(&dir(root), id, HANDLE, &date, re, body.trim()) { Ok(position) => Ok(format!("Added {id}#{position}").into()), @@ -123,6 +171,52 @@ fn comment(root: &Utf8Path, id: TicketId, re: Option, body: &str) -> Tool } } +/// Render the comment block `comment` is about to append, under the heading of +/// the ticket it lands on. +/// +/// Fails when the ticket or the reply target isn't there, the same two +/// conditions the append itself rejects, so a call that cannot land is answered +/// before it is put to the user. +fn preview_comment( + root: &Utf8Path, + id: TicketId, + re: Option, + body: &str, + date: &str, +) -> ToolResult { + let path = match store::locate_ticket(&dir(root), id) { + Ok(path) => path, + Err(store::Error::NoSuchTicket(_)) => return error(format!("No {id}.")), + Err(other) => return Err(other.into()), + }; + let document = fs::read_to_string(path)?; + + // The count comes from the same tolerant reader the append uses, so the + // two agree on a file with a hand-mangled header. + let count = parse::comment_count(&document); + if let Some(position) = re + && (position == 0 || position > count) + { + return error(format!("No comment #{position} on {id}.")); + } + + // The heading names the ticket, which its own file doesn't: the id is what + // makes the preview readable next to the call that produced it. + let heading = match parse::title(&document) { + Some(title) => format!("# {id}: {title}"), + None => format!("# {id}"), + }; + + let comment = Comment { + from: HANDLE.to_owned(), + date: date.to_owned(), + re: re.map(|position| format!("#{position}")), + body: body.to_owned(), + }; + + Ok(preview(&format!("{heading}\n\n{}", render::comment(&comment))).into()) +} + fn close(root: &Utf8Path, id: TicketId) -> ToolResult { match store::close(&dir(root), id) { Ok((_, Status::Done)) => Ok(format!("{id} was already Done.").into()), @@ -185,6 +279,28 @@ fn relative(root: &Utf8Path, path: &Utf8Path) -> String { .replace(MAIN_SEPARATOR, "/") } +/// Style a document for the terminal as a tool-call preview. +/// +/// The document is quoted first, so the transcript carries a marker down the +/// whole preview and the reader can see where the ticket ends and the +/// conversation resumes. +/// Falls back to the unstyled source if the markdown can't be formatted. +fn preview(document: &str) -> String { + let mut quoted = String::with_capacity(document.len() * 2); + for line in document.lines() { + quoted.push('>'); + if !line.is_empty() { + quoted.push(' '); + quoted.push_str(line); + } + quoted.push('\n'); + } + + Formatter::new() + .format_terminal("ed) + .unwrap_or_else(|_| quoted.clone()) +} + /// Render the board as one line per ticket. fn render_list(tickets: &[(TicketId, &Ticket)], unreadable: &[String]) -> String { let mut out = String::new(); diff --git a/.config/jp/tools/src/ticket_tests.rs b/.config/jp/tools/src/ticket_tests.rs index fd2595341..97f331fb5 100644 --- a/.config/jp/tools/src/ticket_tests.rs +++ b/.config/jp/tools/src/ticket_tests.rs @@ -6,6 +6,12 @@ use serde_json::json; use super::*; +const DATE: &str = "2026-08-05"; +const STAMP: &str = "2026-08-05T14:03:11Z"; + +/// A ticket id the tests can name, since allocation draws unpredictable ones. +const FIXED_ID: &str = "T-02wt0kx"; + /// The directory holding this module's tool declarations. fn declarations() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../.jp/mcp/tools/ticket") @@ -31,9 +37,19 @@ fn advertised(file: &str, tool: &str, parameter: &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) +} + +/// 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) +} + +fn dispatch(dir: &Utf8TempDir, action: Action, name: &str, args: Value) -> ToolResult { let ctx = Context { root: dir.path().to_path_buf(), - action: Action::Run, + action, access: None, workspace_id: "test".into(), conversation_id: "test".into(), @@ -58,6 +74,12 @@ fn content(result: ToolResult) -> String { } } +/// A preview as a terminal shows it, with the ANSI styling removed. +fn strip_ansi(rendered: String) -> String { + let bytes = strip_ansi_escapes::strip(rendered); + String::from_utf8(bytes).expect("valid utf-8 after stripping ANSI") +} + fn error_message(result: ToolResult) -> String { match result.expect("tool result") { Outcome::Error { message, .. } => message, @@ -77,6 +99,21 @@ fn create_ticket(dir: &Utf8TempDir, title: &str) -> String { )) } +/// File a ticket under [`FIXED_ID`], bypassing allocation so the test can +/// assert against the id it will read back. +fn write_ticket(dir: &Utf8TempDir, title: &str) -> TicketId { + let id: TicketId = FIXED_ID.parse().unwrap(); + let tickets = dir.path().join(store::DEFAULT_DIR); + std::fs::create_dir_all(&tickets).unwrap(); + std::fs::write( + tickets.join(format!("{}slug.md", id.file_prefix())), + render::ticket(title, Kind::Bug, HANDLE, DATE, None, "Something is wrong."), + ) + .unwrap(); + + id +} + /// Ids are generated, so tests follow the ones that were handed out. fn ids(dir: &Utf8TempDir) -> Vec { store::list(&dir.path().join(store::DEFAULT_DIR)) @@ -101,6 +138,218 @@ fn create_reports_the_id_and_a_workspace_relative_path() { assert!(dir.path().join(&path).exists()); } +#[test] +fn create_preview_renders_the_file_that_will_be_written() { + let out = strip_ansi(preview_create( + Kind::Bug, + "Tool call header misaligned", + Some("045"), + Some("The header renders one column left of the body."), + DATE, + )); + + // The preview is quoted, so the rail shows where the ticket ends and the + // conversation resumes. Written line by line because the blank lines carry + // a trailing space that an editor would trim out of a block literal. + assert_eq!( + out, + concat!( + "> # Tool call header misaligned\n", + "> \n", + "> - **Status**: Todo\n", + "> - **Kind**: Bug\n", + "> - **Authors**: jp\n", + "> - **Date**: 2026-08-05\n", + "> - **Implements**: 045\n", + "> \n", + "> The header renders one column left of the body.\n", + ) + ); +} + +/// 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] +fn create_preview_writes_nothing() { + let dir = Utf8TempDir::new().unwrap(); + create_ticket(&dir, "Tool call header misaligned"); + + let out = strip_ansi(content(preview_tool( + &dir, + "ticket_create", + json!({ + "kind": "chore", + "title": "Bump the deny list" + }), + ))); + + assert!(out.starts_with("> # Bump the deny list\n"), "{out}"); + assert_eq!(ids(&dir).len(), 1, "the preview filed a ticket"); + + create_ticket(&dir, "Bump the deny list"); + assert_eq!(ids(&dir).len(), 2); +} + +#[test] +fn comment_preview_renders_the_block_under_the_ticket_it_lands_on() { + let dir = Utf8TempDir::new().unwrap(); + let id = write_ticket(&dir, "Tool call header misaligned"); + // The reply target has to exist, or the preview rejects the call. + run_tool( + &dir, + "ticket_comment", + json!({ "id": FIXED_ID, "body": "Reproduced at 72 columns." }), + ) + .unwrap(); + + let out = strip_ansi(content(preview_comment( + dir.path(), + id, + Some(1), + "The wrap calculation is off.", + STAMP, + ))); + + assert_eq!( + out, + concat!( + "> # T-02wt0kx: Tool call header misaligned\n", + "> \n", + "> ────────────────────────────────────────────────────────────────────────────────\n", + "> \n", + "> - **From**: jp\n", + "> - **Date**: 2026-08-05T14:03:11Z\n", + "> - **Re**: #1\n", + "> \n", + "> The wrap calculation is off.\n", + ) + ); +} + +/// A call that cannot land is answered before the user is asked about it: the +/// preview fails, and JP turns that into a tool failure ahead of the approval +/// prompt so the assistant can correct the id. +#[test] +fn comment_preview_rejects_a_missing_ticket() { + let dir = Utf8TempDir::new().unwrap(); + + let out = error_message(preview_comment( + dir.path(), + FIXED_ID.parse().unwrap(), + None, + "Reproduced at 72 columns.", + STAMP, + )); + + assert_eq!(out, "No T-02wt0kx."); +} + +/// The preview knows the comment count, which the assistant cannot see, so a +/// reply to a comment that isn't there fails here rather than at the write. +#[test] +fn comment_preview_rejects_a_missing_reply_target() { + let dir = Utf8TempDir::new().unwrap(); + let id = write_ticket(&dir, "Tool call header misaligned"); + + let out = error_message(preview_comment( + dir.path(), + id, + Some(3), + "The wrap calculation is off.", + STAMP, + )); + + assert_eq!(out, "No comment #3 on T-02wt0kx."); +} + +/// Two files claiming one id is a different problem from a missing ticket, and +/// points at a different fix. +#[test] +fn comment_preview_reports_a_duplicated_id() { + let dir = Utf8TempDir::new().unwrap(); + let id = write_ticket(&dir, "Tool call header misaligned"); + std::fs::write( + dir.path() + .join(store::DEFAULT_DIR) + .join(format!("{}other.md", id.file_prefix())), + render::ticket("Other", Kind::Bug, HANDLE, DATE, None, "Something else."), + ) + .unwrap(); + + let error = + preview_comment(dir.path(), id, None, "Reproduced at 72 columns.", STAMP).unwrap_err(); + + assert!( + error + .to_string() + .contains("is claimed by more than one file: "), + "{error}" + ); +} + +/// A hand-edited ticket that lost a metadata field still takes a comment: the +/// append never reads the header, so the preview must not demand one either. +#[test] +fn comment_preview_tolerates_a_malformed_header() { + let dir = Utf8TempDir::new().unwrap(); + let id: TicketId = FIXED_ID.parse().unwrap(); + let tickets = dir.path().join(store::DEFAULT_DIR); + std::fs::create_dir_all(&tickets).unwrap(); + std::fs::write( + tickets.join(format!("{}slug.md", id.file_prefix())), + "# Tool call header misaligned\n\n- **Status**: Todo\n- **Kind**: Bug\n- **Date**: \ + 2026-08-05\n\nSomething is wrong.\n", + ) + .unwrap(); + + let out = strip_ansi(content(preview_comment( + dir.path(), + id, + None, + "Reproduced at 72 columns.", + STAMP, + ))); + + assert_eq!( + out.lines().next(), + Some("> # T-02wt0kx: Tool call header misaligned") + ); + + // The write the preview promised really does land. + assert_eq!( + content(run_tool( + &dir, + "ticket_comment", + json!({ "id": FIXED_ID, "body": "Reproduced at 72 columns." }) + )), + "Added T-02wt0kx#1" + ); +} + +/// Arguments execution rejects are rejected by the preview too, so the +/// assistant is corrected before the call reaches the user. +#[test] +fn preview_rejects_the_arguments_execution_would_reject() { + let dir = Utf8TempDir::new().unwrap(); + + assert_eq!( + error_message(preview_tool( + &dir, + "ticket_create", + json!({ "kind": "chore", "title": " " }) + )), + "`title` must not be empty." + ); + assert_eq!( + error_message(preview_tool( + &dir, + "ticket_comment", + json!({ "id": FIXED_ID, "body": " \n " }) + )), + "`body` must not be empty." + ); +} + #[test] fn create_rejects_an_unknown_kind() { let dir = Utf8TempDir::new().unwrap(); diff --git a/.jp/mcp/tools/ticket/comment.toml b/.jp/mcp/tools/ticket/comment.toml index a31cdce89..fca81f905 100644 --- a/.jp/mcp/tools/ticket/comment.toml +++ b/.jp/mcp/tools/ticket/comment.toml @@ -1,6 +1,7 @@ [conversation.tools.ticket_comment] enable = false source = "local" +format = "unattended" command = "just serve-tools {{context}} {{tool}}" summary = "Append a comment to a ticket." description = """ @@ -28,7 +29,7 @@ Reply to the first comment: """ [conversation.tools.ticket_comment.style] -parameters = "function_call" +parameters = "just serve-tools {{context}} {{tool}}" inline_results = "full" results_file_link = "off" diff --git a/.jp/mcp/tools/ticket/create.toml b/.jp/mcp/tools/ticket/create.toml index 72785cebd..b71fcde6c 100644 --- a/.jp/mcp/tools/ticket/create.toml +++ b/.jp/mcp/tools/ticket/create.toml @@ -1,6 +1,7 @@ [conversation.tools.ticket_create] enable = false source = "local" +format = "unattended" command = "just serve-tools {{context}} {{tool}}" summary = "File a work item as a ticket under docs/ticket/." description = """ @@ -33,7 +34,7 @@ File a chore with no description: """ [conversation.tools.ticket_create.style] -parameters = "function_call" +parameters = "just serve-tools {{context}} {{tool}}" inline_results = "full" results_file_link = "off" diff --git a/crates/internal/ticket/src/parse.rs b/crates/internal/ticket/src/parse.rs index 73ee16b81..fac66f645 100644 --- a/crates/internal/ticket/src/parse.rs +++ b/crates/internal/ticket/src/parse.rs @@ -55,6 +55,16 @@ pub fn comment_count(source: &str) -> usize { Doc::new(source).boundaries().len() } +/// The title from a document's `# Title` heading. +/// +/// Tolerant of a malformed header, like [`comment_count`], so a caller that +/// only needs to name a ticket doesn't have to be able to read the rest of it. +/// Returns `None` when the first non-empty line isn't a level-one heading. +#[must_use] +pub fn title(source: &str) -> Option { + Doc::new(source).title().ok() +} + /// The line range of the metadata block that follows the title heading. /// /// Returns `None` when the heading is missing, or when the first thing after it diff --git a/crates/internal/ticket/src/parse_tests.rs b/crates/internal/ticket/src/parse_tests.rs index d3e11a6ef..cf597a1ac 100644 --- a/crates/internal/ticket/src/parse_tests.rs +++ b/crates/internal/ticket/src/parse_tests.rs @@ -231,6 +231,32 @@ fn counts_comments_without_validating_the_header() { assert_eq!(comment_count("no ticket here"), 0); } +/// A ticket whose metadata block is missing a field still has a readable title, +/// and an append never needs the rest of the header. +#[test] +fn reads_the_title_without_validating_the_header() { + let headerless = indoc! {" + # Tool call header misaligned + + - **Status**: Todo + - **Kind**: Bug + - **Date**: 2026-08-05 + + The header renders one column left of the body. + "}; + + assert_eq!( + document(headerless), + Err(ParseError::MissingField("Authors")) + ); + assert_eq!( + title(headerless).as_deref(), + Some("Tool call header misaligned") + ); + assert_eq!(title(PLAIN).as_deref(), Some("Tool call header misaligned")); + assert_eq!(title("no ticket here"), None); +} + #[test] fn rejects_a_document_without_a_title() { let source = "- **Status**: Todo\n"; diff --git a/crates/internal/ticket/src/render.rs b/crates/internal/ticket/src/render.rs index 440c71661..cb21b4877 100644 --- a/crates/internal/ticket/src/render.rs +++ b/crates/internal/ticket/src/render.rs @@ -98,13 +98,13 @@ pub fn replace_content( Some(out) } -/// Write one comment: the separator that opens it, its metadata, and its body. +/// Render one comment: the separator that opens it, its metadata, and its body. /// /// A comment carries its own separator, so appending one never has to touch /// what is already there. -/// Expects `out` not to end in a newline. -fn push_comment(out: &mut String, comment: &Comment) { - out.push_str("\n\n-----\n\n"); +#[must_use] +pub fn comment(comment: &Comment) -> String { + let mut out = String::from("-----\n\n"); out.push_str(&format!("- **From**: {}\n", comment.from)); out.push_str(&format!("- **Date**: {}\n", comment.date)); if let Some(re) = &comment.re { @@ -113,6 +113,16 @@ fn push_comment(out: &mut String, comment: &Comment) { out.push('\n'); out.push_str(comment.body.trim()); out.push('\n'); + + out +} + +/// Append one comment to a document. +/// +/// Expects `out` not to end in a newline. +fn push_comment(out: &mut String, entry: &Comment) { + out.push_str("\n\n"); + out.push_str(&comment(entry)); } /// Un-embed `id` from a document that names itself. diff --git a/crates/internal/ticket/src/render_tests.rs b/crates/internal/ticket/src/render_tests.rs index 4a11a4d75..306880d1a 100644 --- a/crates/internal/ticket/src/render_tests.rs +++ b/crates/internal/ticket/src/render_tests.rs @@ -3,7 +3,7 @@ use indoc::indoc; use super::*; use crate::parse; -fn comment(from: &str, body: &str, re: Option<&str>) -> Comment { +fn new_comment(from: &str, body: &str, re: Option<&str>) -> Comment { Comment { from: from.to_owned(), date: "2026-08-05T14:03:11Z".to_owned(), @@ -69,7 +69,7 @@ fn first_comment_opens_the_comments_section() { let out = append_comment( &document, - &comment("john", "Reproduced at 72 columns.", None), + &new_comment("john", "Reproduced at 72 columns.", None), ); assert_eq!(out, indoc! {" @@ -93,6 +93,25 @@ fn first_comment_opens_the_comments_section() { "}); } +#[test] +fn renders_a_comment_block() { + let out = comment(&new_comment( + "jp", + "The wrap calculation is off.", + Some("#1"), + )); + + assert_eq!(out, indoc! {" + ----- + + - **From**: jp + - **Date**: 2026-08-05T14:03:11Z + - **Re**: #1 + + The wrap calculation is off. + "}); +} + /// A second comment is written at the end and nothing above it moves. #[test] fn later_comments_are_a_pure_append() { @@ -105,12 +124,12 @@ fn later_comments_are_a_pure_append() { None, "Description.", ), - &comment("john", "Reproduced at 72 columns.", None), + &new_comment("john", "Reproduced at 72 columns.", None), ); let out = append_comment( &document, - &comment("jp", "The wrap calculation is off.", Some("#1")), + &new_comment("jp", "The wrap calculation is off.", Some("#1")), ); assert!(out.starts_with(&document)); @@ -138,9 +157,9 @@ fn appended_comments_parse_back() { None, "Description.", ), - &comment("john", "First.", None), + &new_comment("john", "First.", None), ), - &comment("jp", "Second.", Some("#1")), + &new_comment("jp", "Second.", Some("#1")), ); let parsed = parse::document(&document).unwrap();