From 721cd705d831a190ccc2f5a278b7f02a1b611749 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 20 Aug 2026 23:14:45 +0200 Subject: [PATCH] chore(ticket): Add ticket labels Users can label tickets by system area when creating or updating work items, then filter listings by every requested label. Labels appear in CLI, tool, and documentation board views. Boards define active and retired labels in `docs/ticket/.labels.json`. Writes reject unknown labels, while retired labels remain available on tickets that already carry them. Signed-off-by: Jean Mertz --- .config/jp/tools/src/ticket.rs | 126 ++++++- .config/jp/tools/src/ticket_tests.rs | 249 ++++++++++++- .jp/config/skill/ticket.toml | 11 +- .jp/mcp/tools/ticket/create.toml | 20 +- .jp/mcp/tools/ticket/label.toml | 51 +++ .jp/mcp/tools/ticket/labels.toml | 160 +++++++++ .jp/mcp/tools/ticket/list.toml | 19 +- crates/internal/ticket/Cargo.toml | 2 +- crates/internal/ticket/src/labels.rs | 337 ++++++++++++++++++ crates/internal/ticket/src/labels_tests.rs | 213 +++++++++++ crates/internal/ticket/src/lib.rs | 27 +- crates/internal/ticket/src/parse.rs | 26 +- crates/internal/ticket/src/parse_tests.rs | 44 +++ crates/internal/ticket/src/render.rs | 59 ++- crates/internal/ticket/src/render_tests.rs | 135 +++++-- crates/internal/ticket/src/store.rs | 110 ++++-- crates/internal/ticket/src/store_tests.rs | 226 ++++++++++-- crates/plugins/command/ticket/src/main.rs | 205 +++++++++-- .../plugins/command/ticket/src/main_tests.rs | 239 ++++++++++++- docs/.vitepress/config.mts | 2 + docs/.vitepress/loaders/ticket-shared.mjs | 79 +++- docs/.vitepress/rfd-summaries.json | 2 +- docs/.vitepress/theme/DocIndex.vue | 3 +- docs/.vitepress/theme/TicketBoard.vue | 5 + docs/.vitepress/theme/custom.css | 9 + docs/rfd/100-in-repo-ticket-tracking.md | 87 ++++- docs/ticket/.labels.json | 50 +++ justfile | 76 ++++ 28 files changed, 2420 insertions(+), 152 deletions(-) create mode 100644 .jp/mcp/tools/ticket/label.toml create mode 100644 .jp/mcp/tools/ticket/labels.toml create mode 100644 crates/internal/ticket/src/labels.rs create mode 100644 crates/internal/ticket/src/labels_tests.rs create mode 100644 docs/ticket/.labels.json 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