diff --git a/docs/src/commands.md b/docs/src/commands.md index a164935..2289a6e 100644 --- a/docs/src/commands.md +++ b/docs/src/commands.md @@ -5,7 +5,14 @@ Each command does one thing. They compose. ## A note on `--format` -`json` and `text` are implemented everywhere. **`markdown` and `compact-ai` currently have a real renderer only in `essence`** — for every other command they produce the text output verbatim. +`json` and `text` are implemented everywhere. `markdown` and `compact-ai` need a real renderer, and coverage is partial: + +| format | commands with a real renderer | +|---|---| +| `markdown` | `essence`, `anomalies` | +| `compact-ai` | `essence` | + +For every other command those formats produce the text output verbatim. Commands are being migrated onto a shared renderer, and a command only joins the table above once it genuinely renders that format — so the notice below can never claim more than is true. Rather than accept the flag and quietly ignore it, those commands now say so on stderr: diff --git a/vajra-cli/src/main.rs b/vajra-cli/src/main.rs index b069deb..cff8439 100644 --- a/vajra-cli/src/main.rs +++ b/vajra-cli/src/main.rs @@ -1,6 +1,7 @@ mod batch; mod corpus; mod hints; +mod render; mod treediff; use std::collections::BTreeMap; @@ -352,33 +353,36 @@ enum Command { // Entry point // --------------------------------------------------------------------------- -/// Commands with a real renderer for every advertised format. +/// Which commands genuinely render which formats. /// -/// Every other command renders `markdown` and `compact-ai` identically to -/// `text`. Silently accepting a format and ignoring it is the same failure as -/// reporting `errors: []` over a partial batch: the caller cannot distinguish -/// "rendered as Markdown" from "fell back to text". Until the renderer gap is -/// closed, say so on stderr rather than pretend. -const FULLY_RENDERED: &[&str] = &["essence"]; +/// Every command implements `text` and `json`. `markdown` and `compact-ai` +/// need a real renderer, and most commands do not yet have one — for those, +/// output is the text format verbatim. +/// +/// Migrating a command onto [`render::Report`] is what moves it into this +/// table, so the notice below can never claim more than is true. Silently +/// accepting a format and ignoring it is the same failure as reporting +/// `errors: []` over a partial batch: the caller cannot distinguish "rendered +/// as Markdown" from "fell back to text". +const RENDERS_MARKDOWN: &[&str] = &["essence", "anomalies"]; + +/// Commands with a bespoke compact-AI view. +const RENDERS_COMPACT_AI: &[&str] = &["essence"]; /// Warn when the requested format is not actually implemented for this command. fn warn_unimplemented_format(cli: &Cli) { if cli.quiet { return; } - let needs_renderer = matches!(cli.format, Format::Markdown | Format::CompactAi); - if !needs_renderer { - return; - } let name = command_name(&cli.command); - if FULLY_RENDERED.contains(&name) { + let (requested, implemented) = match cli.format { + Format::Markdown => ("markdown", RENDERS_MARKDOWN), + Format::CompactAi => ("compact-ai", RENDERS_COMPACT_AI), + Format::Text | Format::Json => return, + }; + if implemented.contains(&name) { return; } - let requested = match cli.format { - Format::Markdown => "markdown", - Format::CompactAi => "compact-ai", - _ => return, - }; eprintln!( "vajra: `{name}` has no {requested} renderer; output is the text format. \ Use --format json for a machine-readable form." @@ -1738,55 +1742,90 @@ fn cmd_anomalies(input: &str, cli: &Cli) -> Result<()> { println!("{json}"); } Format::Text | Format::Markdown | Format::CompactAi => { - let mut text = String::new(); - use std::fmt::Write; - - let _ = writeln!(text, "=== Type Instabilities ==="); + let mut report = render::Report::new(); + + report.heading("Anomaly Summary"); + report.fields(vec![ + ( + "Type instabilities".to_owned(), + output.type_instabilities.len().to_string(), + ), + ( + "Numeric outliers".to_owned(), + output.numeric_outliers.len().to_string(), + ), + ( + "Rare values".to_owned(), + output.rare_values.len().to_string(), + ), + ]); + + report.heading("Type Instabilities"); if output.type_instabilities.is_empty() { - let _ = writeln!(text, " (none detected)"); + report.table(render::Table::new(&["PATH"], "none detected")); } else { - for ti in &output.type_instabilities { - let _ = writeln!( - text, - " {}: instability={:.4}, dominant={}", - ti.path, ti.instability, ti.dominant_type - ); - let dist: Vec = ti - .type_distribution + report.nested( + output + .type_instabilities .iter() - .map(|(t, c)| format!("{t}={c}")) - .collect(); - let _ = writeln!(text, " types: {}", dist.join(", ")); - } + .map(|ti| { + let dist: Vec = ti + .type_distribution + .iter() + .map(|(t, c)| format!("{t}={c}")) + .collect(); + ( + format!( + "{}: instability={:.4}, dominant={}", + ti.path, ti.instability, ti.dominant_type + ), + vec![format!("types: {}", dist.join(", "))], + ) + }) + .collect(), + ); } - text.push('\n'); - let _ = writeln!(text, "=== Numeric Outliers ==="); - if output.numeric_outliers.is_empty() { - let _ = writeln!(text, " (none detected)"); - } else { - for no in &output.numeric_outliers { - let _ = writeln!( - text, - " {}: value={}, z_mad={:.4}, median={:.4}, mad={:.4}", - no.path, no.value, no.z_mad, no.median, no.mad - ); - } + report.heading("Numeric Outliers"); + let mut outliers = render::Table::new( + &["PATH", "VALUE", "Z_MAD", "MEDIAN", "MAD"], + "none detected", + ); + for no in &output.numeric_outliers { + outliers.push(vec![ + no.path.clone(), + no.value.to_string(), + format!("{:.4}", no.z_mad), + format!("{:.4}", no.median), + format!("{:.4}", no.mad), + ]); + } + report.table(outliers); + if !output.numeric_outliers.is_empty() { + report.note( + "Z_MAD is deviation from the median in MAD units, so it is robust to the\noutliers it is detecting.", + ); } - text.push('\n'); - let _ = writeln!(text, "=== Rare Values ==="); - if output.rare_values.is_empty() { - let _ = writeln!(text, " (none detected)"); - } else { - for rv in &output.rare_values { - let _ = writeln!( - text, - " {} \"{}\": count={}, rarity={:.4} bits", - rv.path, rv.value, rv.count, rv.rarity_bits - ); - } + report.heading("Rare Values"); + let mut rare = render::Table::new( + &["PATH", "VALUE", "COUNT", "RARITY (bits)"], + "none detected", + ); + for rv in &output.rare_values { + rare.push(vec![ + rv.path.clone(), + rv.value.clone(), + rv.count.to_string(), + format!("{:.4}", rv.rarity_bits), + ]); } + report.table(rare); + + let text = match cli.format { + Format::Markdown => report.to_markdown(), + _ => report.to_text(), + }; let text = maybe_redact(&text, cli); print!("{text}"); } diff --git a/vajra-cli/src/render.rs b/vajra-cli/src/render.rs new file mode 100644 index 0000000..d29be94 --- /dev/null +++ b/vajra-cli/src/render.rs @@ -0,0 +1,378 @@ +//! A minimal document model for command output. +//! +//! Commands historically formatted their own output with `println!` per format, +//! which is why `--format markdown` and `--format compact-ai` silently fell +//! through to text for eleven of twelve commands: each command had to implement +//! every format, so most implemented one. +//! +//! A command instead builds a [`Report`] — headings, key/value pairs, tables, +//! notes — and the renderer emits it per format. Adding a format then costs one +//! implementation here rather than twelve at the call sites. +//! +//! Deliberately small. This is not a layout engine: it covers the shapes vajra +//! commands actually produce, which after surveying them is flat key/value +//! summaries, tables with a caveat attached, and nested detail lists. + +use std::fmt::Write as _; + +/// One element of a report. +#[derive(Debug, Clone)] +pub enum Block { + /// A section heading. + Heading(String), + /// Aligned label/value pairs — a summary. + Fields(Vec<(String, String)>), + /// A table with a header row. + Table(Table), + /// Prose qualifying what precedes it. Never omitted: these carry the + /// caveats that stop a number being over-read. + Note(String), + /// A nested list under a lead line, for per-item detail. + Nested(Vec<(String, Vec)>), +} + +/// A table with a header row and body rows. +#[derive(Debug, Clone)] +pub struct Table { + /// Column headers. + pub headers: Vec, + /// Body rows. Short rows are padded, long rows are not truncated. + pub rows: Vec>, + /// Shown when `rows` is empty, instead of an empty table. + pub empty: String, +} + +impl Table { + /// A table with the given headers and an empty-state message. + pub fn new(headers: &[&str], empty: &str) -> Self { + Self { + headers: headers.iter().map(|h| (*h).to_owned()).collect(), + rows: Vec::new(), + empty: empty.to_owned(), + } + } + + /// Append a row. + pub fn push(&mut self, row: Vec) { + self.rows.push(row); + } + + /// Column widths for aligned text output. + fn widths(&self) -> Vec { + let mut w: Vec = self.headers.iter().map(String::len).collect(); + for row in &self.rows { + for (i, cell) in row.iter().enumerate() { + if i >= w.len() { + w.push(cell.len()); + } else if cell.len() > w[i] { + w[i] = cell.len(); + } + } + } + w + } +} + +/// A command's output, independent of format. +#[derive(Debug, Clone, Default)] +pub struct Report { + blocks: Vec, +} + +impl Report { + /// An empty report. + pub fn new() -> Self { + Self::default() + } + + /// Add a heading. + pub fn heading(&mut self, text: impl Into) -> &mut Self { + self.blocks.push(Block::Heading(text.into())); + self + } + + /// Add label/value pairs. + pub fn fields(&mut self, pairs: Vec<(String, String)>) -> &mut Self { + self.blocks.push(Block::Fields(pairs)); + self + } + + /// Add a table. + pub fn table(&mut self, table: Table) -> &mut Self { + self.blocks.push(Block::Table(table)); + self + } + + /// Add a qualifying note. + pub fn note(&mut self, text: impl Into) -> &mut Self { + self.blocks.push(Block::Note(text.into())); + self + } + + /// Add a nested detail list. + pub fn nested(&mut self, items: Vec<(String, Vec)>) -> &mut Self { + self.blocks.push(Block::Nested(items)); + self + } + + /// Render as aligned plain text. + #[must_use] + pub fn to_text(&self) -> String { + let mut out = String::new(); + let mut prev_was_heading = false; + for (i, block) in self.blocks.iter().enumerate() { + // A blank line separates blocks, but not a heading from what it + // introduces. + if i > 0 && !prev_was_heading { + out.push('\n'); + } + prev_was_heading = matches!(block, Block::Heading(_)); + match block { + Block::Heading(h) => { + let _ = writeln!(out, "=== {h} ==="); + } + Block::Fields(pairs) => { + let w = pairs.iter().map(|(k, _)| k.len()).max().unwrap_or(0); + for (k, v) in pairs { + let _ = writeln!(out, " {k: { + if t.rows.is_empty() { + let _ = writeln!(out, " ({})", t.empty); + } else { + let w = t.widths(); + let header: Vec = t + .headers + .iter() + .enumerate() + .map(|(i, h)| format!("{h: = row + .iter() + .enumerate() + .map(|(i, c)| { + format!("{c: { + for line in n.lines() { + let _ = writeln!(out, " {line}"); + } + } + Block::Nested(items) => { + for (lead, children) in items { + let _ = writeln!(out, " {lead}"); + for child in children { + let _ = writeln!(out, " {child}"); + } + } + } + } + } + out + } + + /// Render as GitHub-flavoured Markdown. + #[must_use] + pub fn to_markdown(&self) -> String { + let mut out = String::new(); + let mut prev_was_heading = false; + for (i, block) in self.blocks.iter().enumerate() { + // A blank line separates blocks, but not a heading from what it + // introduces. + if i > 0 && !prev_was_heading { + out.push('\n'); + } + prev_was_heading = matches!(block, Block::Heading(_)); + match block { + Block::Heading(h) => { + let _ = writeln!(out, "## {h}"); + } + Block::Fields(pairs) => { + let _ = writeln!(out, "| Field | Value |"); + let _ = writeln!(out, "|---|---|"); + for (k, v) in pairs { + let _ = writeln!(out, "| {} | {} |", escape(k), escape(v)); + } + } + Block::Table(t) => { + if t.rows.is_empty() { + let _ = writeln!(out, "_{}_", t.empty); + } else { + let _ = writeln!( + out, + "| {} |", + t.headers + .iter() + .map(|h| escape(h)) + .collect::>() + .join(" | ") + ); + let _ = writeln!(out, "|{}", "---|".repeat(t.headers.len().max(1))); + for row in &t.rows { + let _ = writeln!( + out, + "| {} |", + row.iter() + .map(|c| escape(c)) + .collect::>() + .join(" | ") + ); + } + } + } + Block::Note(n) => { + for line in n.lines() { + let _ = writeln!(out, "> {line}"); + } + } + Block::Nested(items) => { + for (lead, children) in items { + let _ = writeln!(out, "- {}", escape(lead)); + for child in children { + let _ = writeln!(out, " - {}", escape(child)); + } + } + } + } + } + out + } +} + +/// Escape the characters that would break a Markdown table cell. +fn escape(s: &str) -> String { + s.replace('|', "\\|").replace('\n', " ") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sample() -> Report { + let mut r = Report::new(); + r.heading("Summary"); + r.fields(vec![ + ("Records".to_owned(), "288".to_owned()), + ("Entropy".to_owned(), "0.8113".to_owned()), + ]); + let mut t = Table::new(&["PATH", "ENTROPY"], "no paths"); + t.push(vec!["$.a".to_owned(), "1.0000".to_owned()]); + t.push(vec!["$.b".to_owned(), "0.5000".to_owned()]); + r.table(t); + r.note("Ranked by entropy."); + r + } + + #[test] + fn text_render_is_aligned_and_complete() { + let out = sample().to_text(); + assert!(out.contains("=== Summary ===")); + assert!(out.contains("Records")); + assert!(out.contains("$.a")); + assert!(out.contains("Ranked by entropy.")); + } + + #[test] + fn markdown_render_produces_real_tables() { + let out = sample().to_markdown(); + assert!(out.contains("## Summary"), "heading:\n{out}"); + assert!(out.contains("| Field | Value |"), "fields table:\n{out}"); + assert!(out.contains("| PATH | ENTROPY |"), "data table:\n{out}"); + assert!(out.contains("|---|---|"), "separator row:\n{out}"); + assert!(out.contains("> Ranked by entropy."), "blockquote:\n{out}"); + } + + /// The whole point: markdown must not be the text output. + #[test] + fn markdown_differs_from_text() { + let r = sample(); + assert_ne!(r.to_markdown(), r.to_text()); + } + + /// A pipe inside a cell would otherwise break the table. + #[test] + fn pipes_are_escaped_in_markdown() { + let mut r = Report::new(); + let mut t = Table::new(&["RULE"], "none"); + t.push(vec!["a | b".to_owned()]); + r.table(t); + let out = r.to_markdown(); + assert!(out.contains("a \\| b"), "pipe must be escaped:\n{out}"); + } + + /// Newlines inside a cell would break row structure. + #[test] + fn newlines_are_flattened_in_markdown_cells() { + let mut r = Report::new(); + let mut t = Table::new(&["X"], "none"); + t.push(vec!["one\ntwo".to_owned()]); + r.table(t); + let out = r.to_markdown(); + assert!(out.contains("one two"), "newline flattened:\n{out}"); + // The row must remain a single line. + let rows: Vec<&str> = out.lines().filter(|l| l.contains("one")).collect(); + assert_eq!(rows.len(), 1); + } + + #[test] + fn empty_table_shows_its_empty_state() { + let mut r = Report::new(); + r.table(Table::new(&["A", "B"], "nothing found")); + assert!(r.to_text().contains("(nothing found)")); + assert!(r.to_markdown().contains("_nothing found_")); + } + + /// Notes carry the caveats that stop a number being over-read, so they must + /// survive every format. + #[test] + fn notes_survive_both_formats() { + let mut r = Report::new(); + r.note("Not a verdict."); + assert!(r.to_text().contains("Not a verdict.")); + assert!(r.to_markdown().contains("Not a verdict.")); + } + + #[test] + fn nested_lists_render_in_both_formats() { + let mut r = Report::new(); + r.nested(vec![( + "cluster of 3".to_owned(), + vec!["pkg-a".to_owned(), "pkg-b".to_owned()], + )]); + let text = r.to_text(); + assert!(text.contains("cluster of 3") && text.contains("pkg-a")); + let md = r.to_markdown(); + assert!(md.contains("- cluster of 3") && md.contains(" - pkg-a")); + } + + #[test] + fn ragged_rows_do_not_panic() { + let mut r = Report::new(); + let mut t = Table::new(&["A", "B", "C"], "none"); + t.push(vec!["1".to_owned()]); + t.push(vec![ + "1".to_owned(), + "2".to_owned(), + "3".to_owned(), + "4".to_owned(), + ]); + r.table(t); + let _ = r.to_text(); + let _ = r.to_markdown(); + } + + #[test] + fn render_is_deterministic() { + let r = sample(); + assert_eq!(r.to_text(), r.to_text()); + assert_eq!(r.to_markdown(), r.to_markdown()); + } +} diff --git a/vajra-cli/tests/format_honesty.rs b/vajra-cli/tests/format_honesty.rs index 151060e..90047b0 100644 --- a/vajra-cli/tests/format_honesty.rs +++ b/vajra-cli/tests/format_honesty.rs @@ -76,6 +76,48 @@ fn implemented_format_is_silent() -> Result<()> { Ok(()) } +/// Tracking is per-format, not per-command: `anomalies` renders real Markdown +/// but has no compact-AI view, and the notice must reflect exactly that. +#[test] +fn per_format_tracking_is_exact() -> Result<()> { + let dir = tempfile::tempdir()?; + let f = dir.path().join("d.json"); + std::fs::write(&f, FIXTURE)?; + + let (_, md_err) = run(&f, &["anomalies", "--format", "markdown"])?; + assert!( + md_err.is_empty(), + "anomalies renders markdown, so must not warn: {md_err:?}" + ); + + let (_, ai_err) = run(&f, &["anomalies", "--format", "compact-ai"])?; + assert!( + ai_err.contains("compact-ai"), + "anomalies has no compact-ai view, so must warn: {ai_err:?}" + ); + Ok(()) +} + +/// A migrated command must emit genuine Markdown, not the text output. +#[test] +fn migrated_command_emits_real_markdown() -> Result<()> { + let dir = tempfile::tempdir()?; + let f = dir.path().join("d.json"); + std::fs::write(&f, FIXTURE)?; + + let (md, _) = run(&f, &["anomalies", "--format", "markdown", "--quiet"])?; + let (text, _) = run(&f, &["anomalies", "--format", "text", "--quiet"])?; + + assert_ne!(md, text, "markdown must not be the text output"); + assert!(md.contains("## "), "expected Markdown headings:\n{md}"); + assert!(md.contains("|---|"), "expected a Markdown table:\n{md}"); + assert!( + text.contains("=== "), + "text keeps its own heading style:\n{text}" + ); + Ok(()) +} + #[test] fn text_and_json_never_warn() -> Result<()> { let dir = tempfile::tempdir()?; @@ -104,9 +146,14 @@ fn quiet_suppresses_the_warning() -> Result<()> { Ok(()) } -/// The warning is diagnostics only — stdout must be byte-identical to before. +/// For an *unmigrated* command the notice is diagnostics only, so stdout must +/// stay byte-identical to the text output. +/// +/// This test will start failing when `stats` is migrated onto the renderer — +/// that is the intended signal, not a regression. Point it at another +/// unmigrated command, or delete it once every command renders every format. #[test] -fn stdout_is_unchanged() -> Result<()> { +fn unmigrated_command_stdout_is_unchanged() -> Result<()> { let dir = tempfile::tempdir()?; let f = dir.path().join("d.json"); std::fs::write(&f, FIXTURE)?;