diff --git a/.config/jp/tools/src/ticket.rs b/.config/jp/tools/src/ticket.rs index dfb6e759b..a9322fbbe 100644 --- a/.config/jp/tools/src/ticket.rs +++ b/.config/jp/tools/src/ticket.rs @@ -13,7 +13,9 @@ use std::{fs, io, path::MAIN_SEPARATOR}; -use ::ticket::{Comment, Kind, ParseError, Status, Ticket, TicketId, parse, render, store}; +use ::ticket::{ + Comment, Kind, Label, NewTicket, ParseError, Status, Ticket, TicketId, parse, render, store, +}; use camino::{Utf8Path, Utf8PathBuf}; use chrono::{Local, SecondsFormat, Utc}; use comfort::{ @@ -54,6 +56,10 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { if title.trim().is_empty() { return error("`title` must not be empty."); } + let labels = match resolve_labels(root, t.opt::>("labels")?.as_deref())? { + Ok(labels) => labels, + Err(refusal) => return error(refusal), + }; if ctx.action.is_format_arguments() { let date = Local::now().format("%Y-%m-%d").to_string(); @@ -61,13 +67,26 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { kind, &title, implements.as_deref(), + &labels, body.as_deref(), &date, ) .into()); } - create(root, kind, &title, implements.as_deref(), body) + create(root, kind, &title, implements.as_deref(), &labels, body) + } + + "label" => { + let id = match id_arg(&t.req("id")?) { + Ok(id) => id, + Err(message) => return error(message), + }; + label( + root, + id, + &t.opt::>("labels")?.unwrap_or_default(), + ) } "comment" => { @@ -113,7 +132,8 @@ pub fn run(ctx: Context, t: Tool) -> ToolResult { Some(Ok(kind)) => Some(kind), None => None, }; - list(root, status, kind) + let labels = t.opt::>("labels")?.unwrap_or_default(); + list(root, status, kind, &labels) } _ => unknown_tool(t), @@ -125,18 +145,19 @@ fn create( kind: Kind, title: &str, implements: Option<&str>, + labels: &[Label], body: Option, ) -> ToolResult { let date = Local::now().format("%Y-%m-%d").to_string(); - let (id, path) = store::create( - &dir(root), + let (id, path) = store::create(&dir(root), &NewTicket { kind, - title.trim(), - HANDLE, - &date, + title: title.trim(), + authors: HANDLE, + date: &date, implements, - &body.unwrap_or_default(), - )?; + labels, + description: &body.unwrap_or_default(), + })?; reflow(&path)?; Ok(format!("Created {id} at {}", relative(root, &path)).into()) @@ -150,17 +171,56 @@ fn preview_create( kind: Kind, title: &str, implements: Option<&str>, + labels: &[Label], body: Option<&str>, date: &str, ) -> String { - preview(&render::ticket( - title.trim(), + preview(&render::ticket(&NewTicket { kind, - HANDLE, + title: title.trim(), + authors: HANDLE, date, implements, - body.unwrap_or_default(), - )) + labels, + description: body.unwrap_or_default(), + })) +} + +/// Check labels against the board's vocabulary. +/// +/// The outer error is the vocabulary file being unreadable, which is the +/// board's problem; the inner one is a label the board doesn't define, which is +/// the caller's and comes back as a tool error naming the known set. +fn resolve_labels( + root: &Utf8Path, + requested: Option<&[String]>, +) -> crate::Result, String>> { + // A call that names no labels doesn't read the vocabulary at all, so a + // board with a broken `.labels.json` can still file unlabelled tickets. + let Some(requested) = requested else { + return Ok(Ok(vec![])); + }; + + Ok(store::vocabulary(&dir(root))? + .resolve(requested) + .map_err(|refusal| refusal.to_string())) +} + +/// Replace a ticket's labels. +/// +/// Checked against the ticket rather than against the vocabulary alone, so a +/// retired label the ticket already carries can be listed again and kept. +fn label(root: &Utf8Path, id: TicketId, requested: &[String]) -> ToolResult { + let tickets = dir(root); + let vocabulary = store::vocabulary(&tickets)?; + + match store::set_labels(&tickets, id, &vocabulary, requested) { + Ok((_, applied)) if applied.is_empty() => Ok(format!("Cleared the labels on {id}.").into()), + Ok((_, applied)) => Ok(format!("{id}: {}", ::ticket::labels::join(&applied)).into()), + Err(store::Error::NoSuchTicket(_)) => error(format!("No {id}.")), + Err(store::Error::Rejected(refusal)) => error(refusal.to_string()), + Err(other) => Err(other.into()), + } } fn comment(root: &Utf8Path, id: TicketId, re: Option, body: &str) -> ToolResult { @@ -251,7 +311,17 @@ fn show(root: &Utf8Path, id: TicketId) -> ToolResult { } } -fn list(root: &Utf8Path, status: Option, kind: Option) -> ToolResult { +/// List the board, filtered by whatever the caller named. +/// +/// Labels are matched as written on the ticket rather than through the +/// vocabulary, so a ticket carrying a label the board has since dropped can +/// still be found. +fn list( + root: &Utf8Path, + status: Option, + kind: Option, + labels: &[String], +) -> ToolResult { let entries = store::list(&dir(root))?; let mut tickets = vec![]; @@ -265,11 +335,26 @@ fn list(root: &Utf8Path, status: Option, kind: Option) -> ToolResu tickets.retain(|(_, ticket)| { status.is_none_or(|status| status == ticket.metadata.status) && kind.is_none_or(|kind| kind == ticket.metadata.kind) + && carries_every_label(ticket, labels) }); Ok(render_list(&tickets, &unreadable).into()) } +/// Whether a ticket carries every one of `wanted`. +/// +/// Requiring all of them rather than any composes with the other filters: each +/// argument narrows the listing. +fn carries_every_label(ticket: &Ticket, wanted: &[String]) -> bool { + wanted.iter().all(|wanted| { + ticket + .metadata + .labels + .iter() + .any(|label| label.eq_ignore_ascii_case(wanted.trim())) + }) +} + /// The ticket directory inside the workspace. fn dir(root: &Utf8Path) -> Utf8PathBuf { root.join(store::DEFAULT_DIR) @@ -351,9 +436,13 @@ fn render_list(tickets: &[(TicketId, &Ticket)], unreadable: &[String]) -> String .blocked_by .as_deref() .map_or_else(String::new, |by| format!(" [blocked by {by}]")); + let labels = match ticket.metadata.labels.as_slice() { + [] => String::new(), + labels => format!(" [{}]", labels.join(", ")), + }; out.push_str(&format!( - "{id:<9} {status:<12} {kind:<8} {}{blocked}{comments}\n", + "{id:<9} {status:<12} {kind:<8} {}{labels}{blocked}{comments}\n", ticket.title )); } @@ -378,6 +467,9 @@ fn render_ticket(id: TicketId, ticket: &Ticket, path: &str) -> String { out.push_str(&format!("- **Path**: {path}\n")); out.push_str(&format!("- **Status**: {}\n", metadata.status)); out.push_str(&format!("- **Kind**: {}\n", metadata.kind)); + if !metadata.labels.is_empty() { + out.push_str(&format!("- **Labels**: {}\n", metadata.labels.join(", "))); + } out.push_str(&format!("- **Authors**: {}\n", metadata.authors)); out.push_str(&format!("- **Date**: {}\n", metadata.date)); for (label, value) in [ diff --git a/.config/jp/tools/src/ticket_tests.rs b/.config/jp/tools/src/ticket_tests.rs index 701e92915..9ca4818ae 100644 --- a/.config/jp/tools/src/ticket_tests.rs +++ b/.config/jp/tools/src/ticket_tests.rs @@ -26,9 +26,16 @@ fn declaration(file: &str) -> toml::Value { } fn advertised(file: &str, tool: &str, parameter: &str) -> Vec { - declaration(file)["conversation"]["tools"][tool]["parameters"][parameter]["enum"] + enum_values( + &declaration(file)["conversation"]["tools"][tool]["parameters"][parameter], + &format!("{tool}.{parameter}"), + ) +} + +fn enum_values(schema: &toml::Value, what: &str) -> Vec { + schema["enum"] .as_array() - .unwrap_or_else(|| panic!("{tool}.{parameter} has no enum")) + .unwrap_or_else(|| panic!("{what} has no enum")) .iter() .map(|value| value.as_str().expect("string variant").to_owned()) .collect() @@ -99,6 +106,19 @@ fn create_ticket(dir: &Utf8TempDir, title: &str) -> String { )) } +/// A ticket with everything but the title fixed. +fn draft<'a>(title: &'a str, labels: &'a [Label]) -> NewTicket<'a> { + NewTicket { + kind: Kind::Bug, + title, + authors: HANDLE, + date: DATE, + implements: None, + labels, + description: "Something is wrong.", + } +} + /// 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 { @@ -107,13 +127,27 @@ fn write_ticket(dir: &Utf8TempDir, title: &str) -> TicketId { 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."), + render::ticket(&draft(title, &[])), ) .unwrap(); id } +/// Give a board a vocabulary, so label writes have something to check against. +fn write_vocabulary(dir: &Utf8TempDir) { + let tickets = dir.path().join(store::DEFAULT_DIR); + std::fs::create_dir_all(&tickets).unwrap(); + std::fs::write( + tickets.join(::ticket::labels::FILE), + r#"{ + "active": {"cli": "The command line.", "config": "Configuration."}, + "retired": {"legacy-ui": "The old UI."} + }"#, + ) + .unwrap(); +} + /// 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)) @@ -144,6 +178,7 @@ fn create_preview_renders_the_file_that_will_be_written() { Kind::Bug, "Tool call header misaligned", Some("045"), + &[], Some("The header renders one column left of the body."), DATE, )); @@ -272,7 +307,7 @@ fn comment_preview_reports_a_duplicated_id() { dir.path() .join(store::DEFAULT_DIR) .join(format!("{}other.md", id.file_prefix())), - render::ticket("Other", Kind::Bug, HANDLE, DATE, None, "Something else."), + render::ticket(&draft("Other", &[])), ) .unwrap(); @@ -675,11 +710,15 @@ fn every_declared_tool_is_dispatched() { declared.extend(tools.keys().cloned()); } declared.sort(); + // A tool can be declared across more than one file: `labels.toml` carries + // the generated label enums for three of them. + declared.dedup(); assert_eq!(declared, [ "ticket_close", "ticket_comment", "ticket_create", + "ticket_label", "ticket_list", "ticket_show", ]); @@ -730,3 +769,205 @@ fn advertised_values_match_the_parser() { ); } } + +#[test] +fn create_writes_labels_and_list_shows_them() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + content(run_tool( + &dir, + "ticket_create", + json!({ + "kind": "bug", + "title": "Flag parsing drops the last value", + "labels": ["config", "cli"] + }), + )); + + let id = ids(&dir)[0]; + let listed = content(run_tool(&dir, "ticket_list", json!({}))); + + assert_eq!( + listed, + format!("{id} Todo Bug Flag parsing drops the last value [cli, config]\n") + ); +} + +#[test] +fn list_narrows_to_tickets_carrying_every_label() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + for (title, labels) in [ + ("Both", json!(["cli", "config"])), + ("One", json!(["cli"])), + ("None", json!([])), + ] { + content(run_tool( + &dir, + "ticket_create", + json!({ "kind": "bug", "title": title, "labels": labels }), + )); + } + + let listed = content(run_tool( + &dir, + "ticket_list", + json!({ "labels": ["config", "cli"] }), + )); + + assert_eq!(listed.lines().count(), 1, "{listed}"); + assert!(listed.contains("Both [cli, config]"), "{listed}"); +} + +/// The whole set is written, so a retried call lands the same way twice and a +/// label left off the call is a label removed. +#[test] +fn label_replaces_the_whole_set() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + let id = write_ticket(&dir, "Tool call header misaligned"); + + let out = content(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": ["config", "cli"] }), + )); + assert_eq!(out, format!("{id}: cli, config")); + + let out = content(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": ["cli"] }), + )); + assert_eq!(out, format!("{id}: cli")); + + let out = content(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": [] }), + )); + assert_eq!(out, format!("Cleared the labels on {id}.")); + + let shown = content(run_tool(&dir, "ticket_show", json!({ "id": FIXED_ID }))); + assert!(!shown.contains("Labels"), "{shown}"); +} + +/// A typo names the vocabulary, so the assistant can fix the call rather than +/// landing a label nothing groups by. +#[test] +fn an_unknown_label_is_refused_with_the_known_set() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + let message = error_message(run_tool( + &dir, + "ticket_create", + json!({ "kind": "bug", "title": "Typo", "labels": ["clii"] }), + )); + + assert_eq!( + message, + "`clii` is not a known label. Labels you can add: cli, config." + ); + assert!(ids(&dir).is_empty(), "a ticket was filed anyway"); +} + +/// The case the active/retired split exists for: an old ticket carries a label +/// the board has since retired, and adding a new one must not force the retired +/// one off first. +#[test] +fn a_retired_label_already_on_a_ticket_can_be_kept() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + // Written by hand: `legacy-ui` can no longer be applied through the tool, + // which is exactly the situation an old ticket is in. + let id = write_ticket(&dir, "Old ticket"); + let path = store::locate_ticket(&dir.path().join(store::DEFAULT_DIR), id).unwrap(); + let source = std::fs::read_to_string(&path).unwrap(); + std::fs::write( + &path, + render::set_metadata(&source, "Labels", "legacy-ui").unwrap(), + ) + .unwrap(); + + let out = content(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": ["legacy-ui", "cli"] }), + )); + + assert_eq!(out, format!("{id}: cli, legacy-ui")); +} + +#[test] +fn a_retired_label_cannot_be_added_fresh() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + write_ticket(&dir, "Fresh"); + + let message = error_message(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": ["legacy-ui"] }), + )); + + assert_eq!( + message, + "`legacy-ui` is retired and can only stay on a ticket that already carries it. Labels you \ + can add: cli, config." + ); +} + +/// The refusal has to land on the preview too: an unattended formatter that +/// errors fails the call before the user is asked to approve one that cannot +/// land. +#[test] +fn an_unknown_label_is_refused_before_the_preview() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + let message = error_message(preview_tool( + &dir, + "ticket_create", + json!({ "kind": "bug", "title": "Typo", "labels": ["clii"] }), + )); + + assert_eq!( + message, + "`clii` is not a known label. Labels you can add: cli, config." + ); +} + +#[test] +fn labelling_a_missing_ticket_says_so() { + let dir = Utf8TempDir::new().unwrap(); + write_vocabulary(&dir); + + let message = error_message(run_tool( + &dir, + "ticket_label", + json!({ "id": FIXED_ID, "labels": ["cli"] }), + )); + + assert_eq!(message, format!("No {FIXED_ID}.")); +} + +/// The board's vocabulary has to parse, whatever it currently holds. +/// +/// Deliberately not a comparison against the enums the tools advertise: those +/// are a mirror kept by `just ticket-labels-sync`, and adding a label should +/// never turn a second file into a failing test. +#[test] +fn the_board_vocabulary_parses() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../../docs/ticket") + .join(::ticket::labels::FILE); + let source = std::fs::read_to_string(&path) + .unwrap_or_else(|error| panic!("failed to read {}: {error}", path.display())); + + ::ticket::Vocabulary::parse(&source) + .unwrap_or_else(|error| panic!("{}: {error}", path.display())); +} diff --git a/.jp/config/skill/ticket.toml b/.jp/config/skill/ticket.toml index 898febade..3af724e2b 100644 --- a/.jp/config/skill/ticket.toml +++ b/.jp/config/skill/ticket.toml @@ -6,14 +6,20 @@ You have been given the ticket skill. Tickets are work items — bugs, features, chores — tracked as markdown files under `docs/ticket/`, alongside the RFDs. \ The following tools help you with this: -- ticket_list: List tickets by id, with status, kind, and comment count. Filter \ -by status or kind. +- ticket_list: List tickets by id, with status, kind, labels, and comment \ +count. Filter by status, kind, or label. - ticket_show: Read one ticket in full, with its comments numbered for replies. - ticket_create: File a new ticket, at status Todo. - ticket_comment: Append a comment to a ticket's discussion, optionally \ replying to an earlier one. +- ticket_label: Replace the labels on an existing ticket. - ticket_close: Mark a ticket Done. +Labels group tickets by area of the system, orthogonally to kind. The set is \ +closed and lives in `docs/ticket/.labels.json`; the tools advertise it, and a \ +label outside it is refused. `ticket_label` writes the whole set, so read the \ +ticket first when you mean to keep the labels it already carries. + Write a ticket when the work is clear enough to start, and an RFD when it needs \ a design first. The signal is whether there is a decision to argue about, not \ how hard the work is: a subtle rendering bug is a ticket, and a trivially \ @@ -34,3 +40,4 @@ ticket_show = { enable = true, run = "unattended", result = "unattended", style. ticket_create = { enable = true, result = "unattended", style.inline_results = "full", style.results_file_link = "off" } ticket_comment = { enable = true, result = "unattended", style.inline_results = "full", style.results_file_link = "off" } ticket_close = { enable = true, result = "unattended", style.inline_results = "full", style.results_file_link = "off" } +ticket_label = { enable = true, result = "unattended", style.inline_results = "full", style.results_file_link = "off" } diff --git a/.jp/mcp/tools/ticket/create.toml b/.jp/mcp/tools/ticket/create.toml index b71fcde6c..59ed18241 100644 --- a/.jp/mcp/tools/ticket/create.toml +++ b/.jp/mcp/tools/ticket/create.toml @@ -24,7 +24,7 @@ File a bug with a description: ```json {"kind": "bug", "title": "Tool call header misaligned", "body": "The header \ renders one column left of the body when the terminal is narrower than 80 \ -columns."} +columns.", "labels": ["cli"]} ``` File a chore with no description: @@ -63,3 +63,21 @@ Set this when the ticket is a phase of an accepted RFD's implementation plan. An RFD counts as in development while a ticket claiming it sits in the In Progress column, so this is what puts it there. """ + +[conversation.tools.ticket_create.parameters.labels] +type = "array" +required = false +summary = "The areas of the system this ticket touches." +description = """ +Labels group tickets by area, orthogonally to `kind`: `kind` says what type of +work it is, a label says what part of the system it lands in. Apply every label +that fits, or none when nothing does. + +The set is closed and defined in `docs/ticket/.labels.json`. A label outside it +is refused, and the refusal names the ones you can use. +""" + +# The accepted values live in `labels.toml`, generated from +# `docs/ticket/.labels.json` by `just ticket-labels-sync`. +[conversation.tools.ticket_create.parameters.labels.items] +type = "string" diff --git a/.jp/mcp/tools/ticket/label.toml b/.jp/mcp/tools/ticket/label.toml new file mode 100644 index 000000000..6eb30c6c3 --- /dev/null +++ b/.jp/mcp/tools/ticket/label.toml @@ -0,0 +1,51 @@ +[conversation.tools.ticket_label] +enable = false +source = "local" +command = "just serve-tools {{context}} {{tool}}" +summary = "Replace the labels on a ticket." +description = """ +Set the areas of the system a ticket touches. + +The whole set is written: pass every label the ticket should end up with, not +just the new ones. An empty list clears them. Read the ticket first with +`ticket_show` if you mean to keep the labels it already carries. + +The set is closed and defined in `docs/ticket/.labels.json`. A label outside it +is refused, and the refusal names the ones you can use. + +A label the board has retired stays valid on a ticket that already carries it, +so listing it again keeps it. It cannot be added to a ticket that doesn't have +it. +""" + +examples = """ +Label a ticket: +```json +{"id": "T-02wt0kx", "labels": ["config", "cli"]} +``` + +Clear every label: +```json +{"id": "T-02wt0kx", "labels": []} +``` +""" + +[conversation.tools.ticket_label.style] +parameters = "function_call" +inline_results = "full" +results_file_link = "off" + +[conversation.tools.ticket_label.parameters.id] +type = "string" +required = true +summary = "Ticket id, e.g. `T-02wt0kx`." + +[conversation.tools.ticket_label.parameters.labels] +type = "array" +required = true +summary = "Every label the ticket should carry. An empty list clears them." + +# The accepted values live in `labels.toml`, generated from +# `docs/ticket/.labels.json` by `just ticket-labels-sync`. +[conversation.tools.ticket_label.parameters.labels.items] +type = "string" diff --git a/.jp/mcp/tools/ticket/labels.toml b/.jp/mcp/tools/ticket/labels.toml new file mode 100644 index 000000000..5d7a358c1 --- /dev/null +++ b/.jp/mcp/tools/ticket/labels.toml @@ -0,0 +1,160 @@ +# Generated by `just ticket-labels-sync` from `docs/ticket/.labels.json`. +# +# The vocabulary lives with the tickets, because `jp ticket` and the docs build +# both read it and neither loads this config. This file mirrors the active +# labels into the tool schemas so the assistant is offered them rather than +# guessing. +# +# The mirror is a hint, not a contract. Nothing fails when it goes stale: the +# tools validate against `.labels.json`, so a label missing here still writes, +# and one left here after being retired is refused with a message naming the +# real set. Re-run the recipe to bring it back in step. +# +# RFD D38 replaces this file with a reference the loader resolves directly. + +[conversation.tools.ticket_create.parameters.labels.items] +enum = [ + "c:cli", + "c:macos", + "c:web", + "d:conversation", + "d:llm", + "d:mcp", + "d:plugins", + "d:storage", + "d:tooling", + "p:bookworm", + "p:comfort", + "p:grizzly", + "p:jp_attachment", + "p:jp_attachment_agentic_shepherd", + "p:jp_attachment_bear_note", + "p:jp_attachment_cmd_output", + "p:jp_attachment_file_content", + "p:jp_attachment_github", + "p:jp_attachment_http_content", + "p:jp_attachment_internal", + "p:jp_attachment_mcp_resources", + "p:jp_cli", + "p:jp_config", + "p:jp_conversation", + "p:jp_editor", + "p:jp_github", + "p:jp_id", + "p:jp_inquire", + "p:jp_llm", + "p:jp_macro", + "p:jp_mcp", + "p:jp_md", + "p:jp_openrouter", + "p:jp_plugin", + "p:jp_printer", + "p:jp_storage", + "p:jp_task", + "p:jp_term", + "p:jp_test", + "p:jp_tombmap", + "p:jp_tool", + "p:jp_workspace", + "p:json_edit", + "p:schematic", + "p:xct2cli", +] + +[conversation.tools.ticket_label.parameters.labels.items] +enum = [ + "c:cli", + "c:macos", + "c:web", + "d:conversation", + "d:llm", + "d:mcp", + "d:plugins", + "d:storage", + "d:tooling", + "p:bookworm", + "p:comfort", + "p:grizzly", + "p:jp_attachment", + "p:jp_attachment_agentic_shepherd", + "p:jp_attachment_bear_note", + "p:jp_attachment_cmd_output", + "p:jp_attachment_file_content", + "p:jp_attachment_github", + "p:jp_attachment_http_content", + "p:jp_attachment_internal", + "p:jp_attachment_mcp_resources", + "p:jp_cli", + "p:jp_config", + "p:jp_conversation", + "p:jp_editor", + "p:jp_github", + "p:jp_id", + "p:jp_inquire", + "p:jp_llm", + "p:jp_macro", + "p:jp_mcp", + "p:jp_md", + "p:jp_openrouter", + "p:jp_plugin", + "p:jp_printer", + "p:jp_storage", + "p:jp_task", + "p:jp_term", + "p:jp_test", + "p:jp_tombmap", + "p:jp_tool", + "p:jp_workspace", + "p:json_edit", + "p:schematic", + "p:xct2cli", +] + +[conversation.tools.ticket_list.parameters.labels.items] +enum = [ + "c:cli", + "c:macos", + "c:web", + "d:conversation", + "d:llm", + "d:mcp", + "d:plugins", + "d:storage", + "d:tooling", + "p:bookworm", + "p:comfort", + "p:grizzly", + "p:jp_attachment", + "p:jp_attachment_agentic_shepherd", + "p:jp_attachment_bear_note", + "p:jp_attachment_cmd_output", + "p:jp_attachment_file_content", + "p:jp_attachment_github", + "p:jp_attachment_http_content", + "p:jp_attachment_internal", + "p:jp_attachment_mcp_resources", + "p:jp_cli", + "p:jp_config", + "p:jp_conversation", + "p:jp_editor", + "p:jp_github", + "p:jp_id", + "p:jp_inquire", + "p:jp_llm", + "p:jp_macro", + "p:jp_mcp", + "p:jp_md", + "p:jp_openrouter", + "p:jp_plugin", + "p:jp_printer", + "p:jp_storage", + "p:jp_task", + "p:jp_term", + "p:jp_test", + "p:jp_tombmap", + "p:jp_tool", + "p:jp_workspace", + "p:json_edit", + "p:schematic", + "p:xct2cli", +] diff --git a/.jp/mcp/tools/ticket/list.toml b/.jp/mcp/tools/ticket/list.toml index 4df7e1c84..916256d8f 100644 --- a/.jp/mcp/tools/ticket/list.toml +++ b/.jp/mcp/tools/ticket/list.toml @@ -4,11 +4,11 @@ source = "local" command = "just serve-tools {{context}} {{tool}}" summary = "List the tickets in the repository, ordered by id." description = """ -One line per ticket: id, status, kind, title, and comment count. +One line per ticket: id, status, kind, title, labels, and comment count. Use it to find what is open before filing something new, and to resolve a title you half-remember into an id for `ticket_show`. Filter by status to read one -column of the board. +column of the board, or by label to read one area of the system. Files that don't parse as tickets are named at the end rather than skipped silently. @@ -29,6 +29,11 @@ Only open bugs: ```json {"status": "todo", "kind": "bug"} ``` + +Only tickets touching both the CLI and config: +```json +{"labels": ["cli", "config"]} +``` """ [conversation.tools.ticket_list.style] @@ -47,3 +52,13 @@ type = "string" required = false enum = ["bug", "feature", "chore"] summary = "Only tickets of this kind." + +[conversation.tools.ticket_list.parameters.labels] +type = "array" +required = false +summary = "Only tickets carrying every one of these labels." + +# The accepted values live in `labels.toml`, generated from +# `docs/ticket/.labels.json` by `just ticket-labels-sync`. +[conversation.tools.ticket_list.parameters.labels.items] +type = "string" diff --git a/crates/internal/ticket/Cargo.toml b/crates/internal/ticket/Cargo.toml index 46050ce3a..b098cfe95 100644 --- a/crates/internal/ticket/Cargo.toml +++ b/crates/internal/ticket/Cargo.toml @@ -15,11 +15,11 @@ version.workspace = true [dependencies] camino = { workspace = true } serde = { workspace = true, features = ["std", "derive"] } +serde_json = { workspace = true, features = ["std"] } [dev-dependencies] camino-tempfile = { workspace = true } indoc = { workspace = true } -serde_json = { workspace = true, features = ["std"] } [lints] workspace = true diff --git a/crates/internal/ticket/src/labels.rs b/crates/internal/ticket/src/labels.rs new file mode 100644 index 000000000..48d5e9fc2 --- /dev/null +++ b/crates/internal/ticket/src/labels.rs @@ -0,0 +1,337 @@ +//! The label vocabulary: the set of labels a ticket may carry. +//! +//! A board defines its labels in `.labels.json`, next to the ticket files: +//! +//! ```json +//! { +//! "active": { +//! "cli": "The command-line surface.", +//! "config": "Configuration loading and merging." +//! }, +//! "retired": { +//! "legacy-ui": "The pre-rewrite terminal UI." +//! } +//! } +//! ``` +//! +//! The set is closed. +//! A label named by neither list is refused, and the refusal names what is on +//! offer — otherwise a board accumulates near-synonyms (`macos`, `mac-os`, +//! `app/macos`) and grouping stops working. +//! +//! Retiring a label is not deleting it. +//! A retired label stays readable and stays writable on a ticket that already +//! carries it, so relabelling an old ticket doesn't force its history to be +//! rewritten; it just can't be added somewhere new. +//! Deleting the entry outright is the other option, and it turns every ticket +//! carrying that label into a build failure. +//! +//! Reading is liberal and writing is strict: a ticket parsed off disk reports +//! whatever labels it carries, but only [`Vocabulary::resolve`] and +//! [`Vocabulary::resolve_against`] produce the [`Label`] a write needs. + +use std::{collections::BTreeMap, fmt}; + +use serde::Deserialize; + +/// The vocabulary file, inside the ticket directory. +pub const FILE: &str = ".labels.json"; + +/// A label the board's vocabulary defines. +/// +/// Only [`Vocabulary::resolve`] and [`Vocabulary::resolve_against`] hand one +/// out, and it always carries the vocabulary's own spelling. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Label(String); + +impl Label { + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl fmt::Display for Label { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +/// The on-disk shape of `.labels.json`. +/// +/// Unknown fields are rejected so a file written in some other shape fails +/// loudly instead of parsing as an empty vocabulary and refusing every label. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct Document { + #[serde(default)] + active: BTreeMap, + #[serde(default)] + retired: BTreeMap, +} + +/// The labels a board defines, each with what it covers. +/// +/// An empty vocabulary is a board that hasn't defined any: it reads fine and +/// refuses every label. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Vocabulary { + active: BTreeMap, + retired: BTreeMap, +} + +impl Vocabulary { + /// Read a vocabulary from the contents of a `.labels.json`. + /// + /// # Errors + /// + /// Returns an error when the text isn't the documented shape, or when a + /// label is listed as both active and retired. + pub fn parse(source: &str) -> Result { + // An empty file is an empty vocabulary rather than a syntax error: it + // is what `touch` leaves behind, and it means the same thing. + if source.trim().is_empty() { + return Ok(Self::default()); + } + + let document: Document = + serde_json::from_str(source).map_err(|error| Error::Malformed(error.to_string()))?; + + let both: Vec = document + .active + .keys() + .filter(|name| document.retired.contains_key(*name)) + .cloned() + .collect(); + if !both.is_empty() { + return Err(Error::BothActiveAndRetired(both)); + } + + Ok(Self { + active: document.active, + retired: document.retired, + }) + } + + /// Whether the board defines no labels at all. + #[must_use] + pub fn is_empty(&self) -> bool { + self.active.is_empty() && self.retired.is_empty() + } + + /// The labels a write may add, in alphabetical order. + pub fn names(&self) -> impl Iterator { + self.active.keys().map(String::as_str) + } + + /// The labels that may stay where they already are but not be added, in + /// alphabetical order. + pub fn retired_names(&self) -> impl Iterator { + self.retired.keys().map(String::as_str) + } + + /// What a label covers, active or retired. + #[must_use] + pub fn description(&self, name: &str) -> Option<&str> { + self.active + .get(name) + .or_else(|| self.retired.get(name)) + .map(String::as_str) + } + + /// Check labels for a ticket that carries none yet. + /// + /// # Errors + /// + /// Returns every label the vocabulary doesn't define and every retired one, + /// since a new ticket has nothing for a retired label to be kept on. + pub fn resolve(&self, requested: &[String]) -> Result, Rejected> { + self.resolve_against(requested, &[]) + } + + /// Check labels for a ticket that already carries `current`. + /// + /// Matching ignores case and surrounding whitespace, and the result carries + /// the vocabulary's spelling, so a board's labels read the same on every + /// ticket. + /// The result is sorted and deduplicated: a ticket's label line is a set, + /// and the order it was typed in says nothing. + /// + /// A retired label already in `current` resolves; one that isn't is + /// refused. + /// That is what lets a label be added to an old ticket without first + /// stripping the retired labels it happens to carry. + /// + /// # Errors + /// + /// Returns every rejected label at once, so a caller fixing them doesn't + /// discover them one at a time. + pub fn resolve_against( + &self, + requested: &[String], + current: &[String], + ) -> Result, Rejected> { + let mut resolved = vec![]; + let mut unknown = vec![]; + let mut retired = vec![]; + + for name in requested { + let name = name.trim(); + if name.is_empty() { + continue; + } + + if let Some(known) = matching(self.active.keys(), name) { + resolved.push(Label(known.clone())); + } else if let Some(known) = matching(self.retired.keys(), name) { + match matching(current.iter(), known) { + Some(_) => resolved.push(Label(known.clone())), + None => retired.push(name.to_owned()), + } + } else { + unknown.push(name.to_owned()); + } + } + + if !unknown.is_empty() || !retired.is_empty() { + return Err(Rejected { + unknown, + retired, + active: self.names().map(ToOwned::to_owned).collect(), + }); + } + + resolved.sort(); + resolved.dedup(); + + Ok(resolved) + } +} + +/// The entry in `candidates` matching `name`, ignoring case. +fn matching<'a>( + mut candidates: impl Iterator, + name: &str, +) -> Option<&'a String> { + candidates.find(|candidate| candidate.eq_ignore_ascii_case(name)) +} + +/// A vocabulary file that can't be read. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Error { + /// The file isn't the documented `{ "active": {}, "retired": {} }` shape. + Malformed(String), + /// A label appears in both lists, so nothing can say whether it may be + /// added. + BothActiveAndRetired(Vec), +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Malformed(reason) => write!( + f, + "The label vocabulary is not an object with `active` and `retired` maps of label \ + to description: {reason}" + ), + Self::BothActiveAndRetired(labels) => write!( + f, + "{} listed as both active and retired in the label vocabulary.", + quoted(labels) + ), + } + } +} + +impl std::error::Error for Error {} + +/// Labels a write can't apply. +/// +/// Carries the addable set as well as the refusals, so the message stands on +/// its own wherever it is printed. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Rejected { + /// Named by neither list. + pub unknown: Vec, + /// Retired, and not already on the ticket. + pub retired: Vec, + /// The labels a write may add. + pub active: Vec, +} + +impl fmt::Display for Rejected { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut first = true; + + if !self.unknown.is_empty() { + let verb = if self.unknown.len() == 1 { + "is not a known label" + } else { + "are not known labels" + }; + write!(f, "{} {verb}.", quoted(&self.unknown))?; + first = false; + } + + if !self.retired.is_empty() { + if !first { + f.write_str(" ")?; + } + let verb = if self.retired.len() == 1 { + "is retired and can only stay on a ticket that already carries it" + } else { + "are retired and can only stay on tickets that already carry them" + }; + write!(f, "{} {verb}.", quoted(&self.retired))?; + } + + if self.active.is_empty() { + return write!( + f, + " This board defines no labels; add them to `{FILE}` in the ticket directory." + ); + } + + write!(f, " Labels you can add: {}.", self.active.join(", ")) + } +} + +impl std::error::Error for Rejected {} + +/// Render a list of names as a comma-separated run of backticked values. +fn quoted(names: &[String]) -> String { + names + .iter() + .map(|name| format!("`{name}`")) + .collect::>() + .join(", ") +} + +/// Split a ticket's `Labels` metadata value into its labels. +/// +/// Plain strings rather than [`Label`]s: a ticket read off disk may carry a +/// label the vocabulary no longer defines, and hiding it would make the file +/// and the listing disagree. +#[must_use] +pub fn split(value: &str) -> Vec { + value + .split(',') + .map(str::trim) + .filter(|label| !label.is_empty()) + .map(ToOwned::to_owned) + .collect() +} + +/// Render labels as a ticket's `Labels` metadata value. +#[must_use] +pub fn join(labels: &[Label]) -> String { + labels + .iter() + .map(Label::as_str) + .collect::>() + .join(", ") +} + +#[cfg(test)] +#[path = "labels_tests.rs"] +mod tests; diff --git a/crates/internal/ticket/src/labels_tests.rs b/crates/internal/ticket/src/labels_tests.rs new file mode 100644 index 000000000..2ea9ebf55 --- /dev/null +++ b/crates/internal/ticket/src/labels_tests.rs @@ -0,0 +1,213 @@ +use indoc::indoc; + +use super::*; + +const VOCABULARY: &str = indoc! {r#" + { + "active": { + "app/macos": "The native macOS app.", + "cli": "The command-line surface.", + "config": "Configuration loading and merging." + }, + "retired": { + "legacy-ui": "The pre-rewrite terminal UI." + } + } +"#}; + +fn vocabulary() -> Vocabulary { + Vocabulary::parse(VOCABULARY).unwrap() +} + +fn owned(labels: &[&str]) -> Vec { + labels.iter().map(|label| (*label).to_owned()).collect() +} + +#[test] +fn reads_active_and_retired_labels() { + let vocabulary = vocabulary(); + + assert_eq!(vocabulary.names().collect::>(), [ + "app/macos", + "cli", + "config" + ]); + assert_eq!(vocabulary.retired_names().collect::>(), [ + "legacy-ui" + ]); + assert_eq!( + vocabulary.description("app/macos"), + Some("The native macOS app.") + ); + assert_eq!( + vocabulary.description("legacy-ui"), + Some("The pre-rewrite terminal UI.") + ); + assert_eq!(vocabulary.description("nope"), None); +} + +/// A board with no vocabulary file at all reads as an empty one, so listing a +/// ticket never depends on the file being there. +#[test] +fn an_empty_file_is_an_empty_vocabulary() { + assert_eq!(Vocabulary::parse("").unwrap(), Vocabulary::default()); + assert_eq!(Vocabulary::parse(" \n").unwrap(), Vocabulary::default()); +} + +/// `retired` is for boards that have retired something; most have not. +#[test] +fn retired_is_optional() { + let vocabulary = Vocabulary::parse(r#"{"active": {"cli": "The CLI."}}"#).unwrap(); + + assert_eq!(vocabulary.names().collect::>(), ["cli"]); + assert_eq!(vocabulary.retired_names().count(), 0); +} + +/// A bare map of label to description is the shape someone reaches for first. +/// It has to fail loudly: parsed leniently it would read as an empty vocabulary +/// and refuse every label, blaming the caller for the file's problem. +#[test] +fn a_flat_map_is_refused_rather_than_read_as_empty() { + assert!(matches!( + Vocabulary::parse(r#"{"cli": "The CLI."}"#), + Err(Error::Malformed(_)) + )); +} + +#[test] +fn a_malformed_file_is_an_error() { + assert!(matches!( + Vocabulary::parse(r#"["cli"]"#), + Err(Error::Malformed(_)) + )); +} + +/// Nothing can say whether such a label may be added, so the file is wrong +/// rather than ambiguous. +#[test] +fn a_label_in_both_lists_is_an_error() { + let source = r#"{"active": {"cli": "a"}, "retired": {"cli": "b"}}"#; + + assert_eq!( + Vocabulary::parse(source), + Err(Error::BothActiveAndRetired(vec!["cli".to_owned()])) + ); +} + +#[test] +fn resolves_to_the_vocabularys_spelling() { + let resolved = vocabulary().resolve(&owned(&[" CLI ", "config"])).unwrap(); + + assert_eq!(join(&resolved), "cli, config"); +} + +/// The label line is a set: the order it was typed in says nothing, and asking +/// for one label twice is asking for it once. +#[test] +fn resolving_sorts_and_deduplicates() { + let resolved = vocabulary() + .resolve(&owned(&["config", "app/macos", "CONFIG"])) + .unwrap(); + + assert_eq!(join(&resolved), "app/macos, config"); +} + +#[test] +fn empty_entries_are_dropped() { + let resolved = vocabulary().resolve(&owned(&["", " ", "cli"])).unwrap(); + + assert_eq!(join(&resolved), "cli"); +} + +/// The case this split exists for: an old ticket carries a retired label, and +/// adding a new one must not force the retired one off first. +#[test] +fn a_retired_label_already_on_the_ticket_can_be_kept() { + let resolved = vocabulary() + .resolve_against(&owned(&["legacy-ui", "cli"]), &owned(&["legacy-ui"])) + .unwrap(); + + assert_eq!(join(&resolved), "cli, legacy-ui"); +} + +#[test] +fn a_retired_label_not_on_the_ticket_is_refused() { + let error = vocabulary() + .resolve_against(&owned(&["legacy-ui", "cli"]), &owned(&["config"])) + .unwrap_err(); + + assert_eq!(error.retired, ["legacy-ui"]); + assert!(error.unknown.is_empty()); + assert_eq!( + error.to_string(), + "`legacy-ui` is retired and can only stay on a ticket that already carries it. Labels you \ + can add: app/macos, cli, config." + ); +} + +/// A new ticket carries nothing, so there is nothing for a retired label to +/// stay on. +#[test] +fn a_new_ticket_cannot_take_a_retired_label() { + let error = vocabulary().resolve(&owned(&["legacy-ui"])).unwrap_err(); + + assert_eq!(error.retired, ["legacy-ui"]); +} + +/// Keeping a retired label is matched the same way as everything else. +#[test] +fn keeping_a_retired_label_ignores_case() { + let resolved = vocabulary() + .resolve_against(&owned(&["LEGACY-UI"]), &owned(&["legacy-ui"])) + .unwrap(); + + assert_eq!(join(&resolved), "legacy-ui"); +} + +/// One call reports every problem, so a caller fixing them doesn't have to +/// discover them one at a time. +#[test] +fn every_rejection_is_reported_at_once() { + let error = vocabulary() + .resolve(&owned(&["clii", "cli", "legacy-ui", "storage"])) + .unwrap_err(); + + assert_eq!(error.unknown, ["clii", "storage"]); + assert_eq!(error.retired, ["legacy-ui"]); + assert_eq!( + error.to_string(), + "`clii`, `storage` are not known labels. `legacy-ui` is retired and can only stay on a \ + ticket that already carries it. Labels you can add: app/macos, cli, config." + ); +} + +#[test] +fn one_unknown_label_reads_as_one() { + let error = vocabulary().resolve(&owned(&["storage"])).unwrap_err(); + + assert_eq!( + error.to_string(), + "`storage` is not a known label. Labels you can add: app/macos, cli, config." + ); +} + +/// A board that hasn't defined any labels should say so, rather than listing an +/// empty set and leaving the caller to guess where labels come from. +#[test] +fn an_empty_vocabulary_says_where_labels_come_from() { + let error = Vocabulary::default().resolve(&owned(&["cli"])).unwrap_err(); + + assert_eq!( + error.to_string(), + "`cli` is not a known label. This board defines no labels; add them to `.labels.json` in \ + the ticket directory." + ); +} + +#[test] +fn splits_a_metadata_value() { + assert_eq!(split("app/macos, config"), ["app/macos", "config"]); + assert_eq!(split("app/macos,config"), ["app/macos", "config"]); + assert_eq!(split(" app/macos ,, "), ["app/macos"]); + assert!(split("").is_empty()); +} diff --git a/crates/internal/ticket/src/lib.rs b/crates/internal/ticket/src/lib.rs index f96ba2ca3..6bc0fafe6 100644 --- a/crates/internal/ticket/src/lib.rs +++ b/crates/internal/ticket/src/lib.rs @@ -31,8 +31,8 @@ //! //! [`id`] defines the identifier, [`parse`] reads a document, [`render`] writes //! one, [`store`] holds the file operations (id allocation, create, comment, -//! close, list, import), and [`import`] carries the rules for content that -//! comes from upstream. +//! close, list, import), [`labels`] holds the board's label vocabulary, and +//! [`import`] carries the rules for content that comes from upstream. //! //! The id is in the filename and nowhere else, so renaming a ticket is a rename //! and there is no second copy to keep in step. @@ -45,11 +45,13 @@ use serde::Serialize; pub mod id; pub mod import; +pub mod labels; pub mod parse; pub mod render; pub mod store; pub use id::TicketId; +pub use labels::{Label, Vocabulary}; /// Where a ticket sits on the board. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize)] @@ -133,6 +135,12 @@ impl FromStr for Kind { pub struct Metadata { pub status: Status, pub kind: Kind, + /// The labels the ticket carries. + /// + /// Plain strings, not [`Label`]s: a hand-edited ticket can name a label the + /// vocabulary doesn't define, and dropping it here would make the listing + /// disagree with the file. + pub labels: Vec, pub authors: String, pub date: String, pub blocked_by: Option, @@ -141,6 +149,21 @@ pub struct Metadata { pub github: Option, } +/// The parts of a ticket that are settled when it is filed. +/// +/// A struct rather than a run of positional arguments: five of these are +/// strings, and at a call site nothing would catch two of them being swapped. +pub struct NewTicket<'a> { + pub kind: Kind, + pub title: &'a str, + pub authors: &'a str, + pub date: &'a str, + /// The RFD this work comes from, if any. + pub implements: Option<&'a str>, + pub labels: &'a [Label], + pub description: &'a str, +} + /// One comment on a ticket. /// /// `from` is a short handle (`john`, `jp`); imported GitHub comments use diff --git a/crates/internal/ticket/src/parse.rs b/crates/internal/ticket/src/parse.rs index fe63f5fb3..84132c466 100644 --- a/crates/internal/ticket/src/parse.rs +++ b/crates/internal/ticket/src/parse.rs @@ -13,7 +13,7 @@ use std::ops::Range; -use crate::{Comment, Metadata, ParseError, Ticket}; +use crate::{Comment, Metadata, ParseError, Ticket, labels}; /// Read a ticket document. pub fn document(source: &str) -> Result { @@ -65,6 +65,27 @@ pub fn title(source: &str) -> Option { Doc::new(source).title().ok() } +/// The labels a document's metadata block carries. +/// +/// Tolerant of a malformed header, like [`comment_count`], so a hand-mangled +/// ticket can still be relabelled rather than having to be repaired first. +/// Returns them as written: checking against the vocabulary is a write-time +/// concern, and a listing that hid a label the file carries would disagree with +/// the file. +#[must_use] +pub fn labels(source: &str) -> Vec { + let doc = Doc::new(source); + let Some(header) = doc.metadata_range() else { + return vec![]; + }; + + header + .filter_map(|i| meta_line(doc.lines[i])) + .find(|(key, _)| key.eq_ignore_ascii_case("labels")) + .map(|(_, value)| labels::split(value)) + .unwrap_or_default() +} + /// 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 @@ -222,6 +243,7 @@ fn without_comments_heading(text: String) -> String { fn metadata<'a>(pairs: impl Iterator) -> Result { let mut status = None; let mut kind = None; + let mut label_names = vec![]; let mut authors = None; let mut date = None; let mut blocked_by = None; @@ -233,6 +255,7 @@ fn metadata<'a>(pairs: impl Iterator) -> Result status = Some(value.parse()?), "kind" => kind = Some(value.parse()?), + "labels" => label_names = labels::split(value), "authors" => authors = Some(value.to_owned()), "date" => date = Some(value.to_owned()), "blocked by" => blocked_by = Some(value.to_owned()), @@ -246,6 +269,7 @@ fn metadata<'a>(pairs: impl Iterator) -> Result, - description: &str, -) -> String { - let mut out = format!("# {title}\n\n"); +pub fn ticket(new: &NewTicket<'_>) -> String { + let mut out = format!("# {}\n\n", new.title); out.push_str(&format!("- **Status**: {}\n", Status::Todo)); - out.push_str(&format!("- **Kind**: {kind}\n")); - out.push_str(&format!("- **Authors**: {authors}\n")); - out.push_str(&format!("- **Date**: {date}\n")); - if let Some(rfd) = implements { + out.push_str(&format!("- **Kind**: {}\n", new.kind)); + out.push_str(&format!("- **Authors**: {}\n", new.authors)); + out.push_str(&format!("- **Date**: {}\n", new.date)); + if let Some(rfd) = new.implements { out.push_str(&format!("- **Implements**: {rfd}\n")); } + if !new.labels.is_empty() { + out.push_str(&format!("- **Labels**: {}\n", labels::join(new.labels))); + } - let description = description.trim(); + let description = new.description.trim(); if !description.is_empty() { out.push('\n'); out.push_str(description); @@ -199,6 +196,34 @@ pub fn set_metadata(document: &str, key: &str, value: &str) -> Option { Some(out) } +/// Drop a field from the ticket's metadata block, returning the new document. +/// +/// Returns `None` when the document has no metadata block; a document that +/// doesn't carry the field comes back unchanged. +/// +/// Only the header block is considered, so a line quoted in a comment is left +/// alone. +#[must_use] +pub fn remove_metadata(document: &str, key: &str) -> Option { + let header = parse::metadata_range(document)?; + let mut lines: Vec = document.lines().map(ToOwned::to_owned).collect(); + + let existing = header.clone().find(|&i| { + parse::meta_line(&lines[i]).is_some_and(|(found, _)| found.eq_ignore_ascii_case(key)) + }); + let Some(index) = existing else { + return Some(document.to_owned()); + }; + lines.remove(index); + + let mut out = lines.join("\n"); + if document.ends_with('\n') { + out.push('\n'); + } + + Some(out) +} + #[cfg(test)] #[path = "render_tests.rs"] mod tests; diff --git a/crates/internal/ticket/src/render_tests.rs b/crates/internal/ticket/src/render_tests.rs index 306880d1a..1aba5f431 100644 --- a/crates/internal/ticket/src/render_tests.rs +++ b/crates/internal/ticket/src/render_tests.rs @@ -1,7 +1,26 @@ use indoc::indoc; use super::*; -use crate::parse; +use crate::{Kind, Label, Vocabulary, parse}; + +/// A ticket with everything but the parts under test fixed. +fn draft<'a>( + title: &'a str, + kind: Kind, + authors: &'a str, + labels: &'a [Label], + description: &'a str, +) -> NewTicket<'a> { + NewTicket { + kind, + title, + authors, + date: "2026-08-05", + implements: None, + labels, + description, + } +} fn new_comment(from: &str, body: &str, re: Option<&str>) -> Comment { Comment { @@ -14,14 +33,13 @@ fn new_comment(from: &str, body: &str, re: Option<&str>) -> Comment { #[test] fn renders_a_new_ticket() { - let out = ticket( + let out = ticket(&draft( "Tool call header misaligned", Kind::Bug, "John Doe", - "2026-08-05", - None, + &[], "The header renders one column left of the body.", - ); + )); assert_eq!(out, indoc! {" # Tool call header misaligned @@ -37,14 +55,13 @@ fn renders_a_new_ticket() { #[test] fn renders_a_new_ticket_without_a_description() { - let out = ticket( + let out = ticket(&draft( "Bump the deny list", Kind::Chore, "john", - "2026-08-05", - None, + &[], " ", - ); + )); assert_eq!(out, indoc! {" # Bump the deny list @@ -58,14 +75,13 @@ fn renders_a_new_ticket_without_a_description() { #[test] fn first_comment_opens_the_comments_section() { - let document = ticket( + let document = ticket(&draft( "Tool call header misaligned", Kind::Bug, "John Doe", - "2026-08-05", - None, + &[], "The header renders one column left of the body.", - ); + )); let out = append_comment( &document, @@ -116,14 +132,13 @@ fn renders_a_comment_block() { #[test] fn later_comments_are_a_pure_append() { let document = append_comment( - &ticket( + &ticket(&draft( "Tool call header misaligned", Kind::Bug, "John Doe", - "2026-08-05", - None, + &[], "Description.", - ), + )), &new_comment("john", "Reproduced at 72 columns.", None), ); @@ -149,14 +164,13 @@ fn later_comments_are_a_pure_append() { fn appended_comments_parse_back() { let document = append_comment( &append_comment( - &ticket( + &ticket(&draft( "Round trip", Kind::Feature, "john", - "2026-08-05", - None, + &[], "Description.", - ), + )), &new_comment("john", "First.", None), ), &new_comment("jp", "Second.", Some("#1")), @@ -253,4 +267,83 @@ fn adds_a_field_the_ticket_lacks() { #[test] fn reports_a_document_with_no_metadata_block() { assert_eq!(set_metadata("# Bare\n\nProse.\n", "Status", "Done"), None); + assert_eq!(remove_metadata("# Bare\n\nProse.\n", "Labels"), None); +} + +#[test] +fn removes_a_metadata_field() { + let document = indoc! {" + # Tool call header misaligned + + - **Status**: Todo + - **Kind**: Bug + - **Authors**: john + - **Date**: 2026-08-05 + - **Labels**: config + + Description. + "}; + + let out = remove_metadata(document, "Labels").unwrap(); + + assert_eq!(out, indoc! {" + # Tool call header misaligned + + - **Status**: Todo + - **Kind**: Bug + - **Authors**: john + - **Date**: 2026-08-05 + + Description. + "}); +} + +/// Clearing labels on a ticket that has none is not an error, so a caller +/// doesn't have to read the ticket first to know which write to make. +#[test] +fn removing_a_field_the_ticket_lacks_changes_nothing() { + let document = indoc! {" + # Tool call header misaligned + + - **Status**: Todo + - **Kind**: Bug + - **Authors**: john + - **Date**: 2026-08-05 + + Description. + "}; + + assert_eq!(remove_metadata(document, "Labels").unwrap(), document); +} + +/// Labels are written after the fields every ticket carries, which is where +/// `set_metadata` puts them too — so a ticket filed with labels and one +/// labelled later look the same. +#[test] +fn renders_labels_after_the_required_fields() { + let vocabulary = + Vocabulary::parse(r#"{"active": {"app/macos": "The app.", "config": "Config."}}"#).unwrap(); + let labels = vocabulary + .resolve(&["config".to_owned(), "app/macos".to_owned()]) + .unwrap(); + + let out = ticket(&draft( + "Labelled", + Kind::Bug, + "john", + &labels, + "Description.", + )); + + assert_eq!(out, indoc! {" + # Labelled + + - **Status**: Todo + - **Kind**: Bug + - **Authors**: john + - **Date**: 2026-08-05 + - **Labels**: app/macos, config + + Description. + "}); } diff --git a/crates/internal/ticket/src/store.rs b/crates/internal/ticket/src/store.rs index 24470d834..17ac4b1dc 100644 --- a/crates/internal/ticket/src/store.rs +++ b/crates/internal/ticket/src/store.rs @@ -17,10 +17,10 @@ use std::{ use camino::{Utf8Path, Utf8PathBuf}; use crate::{ - Comment, Kind, ParseError, Status, Ticket, TicketId, + Comment, Label, NewTicket, ParseError, Status, Ticket, TicketId, Vocabulary, id::{MAX_BUCKET, TAIL_SPACE}, import::{Import, escaped}, - parse, render, + labels, parse, render, }; /// Directory holding the ticket files, relative to the workspace root. @@ -62,6 +62,10 @@ pub enum Error { /// Allocation refuses rather than wrapping, which would reuse old time /// prefixes, or widening, which would break the fixed-width form. Exhausted, + /// The board's label vocabulary can't be read. + Labels(labels::Error), + /// A write named labels the board won't accept. + Rejected(labels::Rejected), /// The filesystem said no. Io(io::Error), } @@ -94,6 +98,8 @@ impl fmt::Display for Error { Self::Exhausted => f.write_str( "The ticket id format has no time buckets left; it needs a wider time component.", ), + Self::Labels(error) => error.fmt(f), + Self::Rejected(error) => error.fmt(f), Self::Io(error) => error.fmt(f), } } @@ -103,6 +109,8 @@ impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { Self::Parse(error) => Some(error), + Self::Labels(error) => Some(error), + Self::Rejected(error) => Some(error), Self::Io(error) => Some(error), _ => None, } @@ -115,6 +123,18 @@ impl From for Error { } } +impl From for Error { + fn from(error: labels::Error) -> Self { + Self::Labels(error) + } +} + +impl From for Error { + fn from(error: labels::Rejected) -> Self { + Self::Rejected(error) + } +} + impl From for Error { fn from(error: io::Error) -> Self { Self::Io(error) @@ -141,21 +161,11 @@ const CLAIM_ATTEMPTS: usize = 16; /// Create a ticket at `Todo`, returning its id and path. /// -/// `implements` names the RFD this work comes from, if any. -/// /// Creating the file exclusively is what claims the id: two processes drawing /// in the same bucket can land on one tail, and the loser finds out here rather /// than overwriting the winner's ticket. -pub fn create( - dir: &Utf8Path, - kind: Kind, - title: &str, - authors: &str, - date: &str, - implements: Option<&str>, - description: &str, -) -> Result<(TicketId, Utf8PathBuf)> { - let slug = slug(title); +pub fn create(dir: &Utf8Path, new: &NewTicket<'_>) -> Result<(TicketId, Utf8PathBuf)> { + let slug = slug(new.title); for _ in 0..CLAIM_ATTEMPTS { let id = allocate_id(dir)?; @@ -167,8 +177,7 @@ pub fn create( .open(&path) { Ok(mut file) => { - let document = render::ticket(title, kind, authors, date, implements, description); - file.write_all(document.as_bytes())?; + file.write_all(render::ticket(new).as_bytes())?; return Ok((id, path)); } @@ -238,6 +247,55 @@ pub fn set_field(dir: &Utf8Path, id: TicketId, key: &str, value: &str) -> Result Ok(path) } +/// Read the board's label vocabulary. +/// +/// A board with no `.labels.json` defines no labels, which is a board that +/// hasn't started using them rather than an error — reading a ticket must not +/// depend on the file being there. +/// A file that is present but unreadable *is* an error: silently treating it as +/// empty would reject every label a caller asks for and blame the caller. +pub fn vocabulary(dir: &Utf8Path) -> Result { + match fs::read_to_string(dir.join(labels::FILE)) { + Ok(source) => Ok(Vocabulary::parse(&source)?), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(Vocabulary::default()), + Err(error) => Err(error.into()), + } +} + +/// Replace a ticket's labels, returning its path and what it now carries. +/// +/// The whole set is written, so this is the only write labels need: an empty +/// list drops the field. +/// Replacing rather than merging keeps a retried call from landing twice. +/// +/// `requested` is checked against the ticket's current labels, not just against +/// the vocabulary, so a retired label the ticket already carries can be listed +/// again and kept. +/// The check happens here rather than in the caller because the current labels +/// come from the same read this write is about to replace. +pub fn set_labels( + dir: &Utf8Path, + id: TicketId, + vocabulary: &Vocabulary, + requested: &[String], +) -> Result<(Utf8PathBuf, Vec