diff --git a/docs/lint.md b/docs/lint.md index 39c860e..4be9756 100644 --- a/docs/lint.md +++ b/docs/lint.md @@ -50,6 +50,28 @@ already has the button, or the edge aggregates.yaml lacks. A schema error no longer hides the needs findings - both are printed, so one round can fix them together. +`--findings ` writes the same findings one JSON object per +line: `id`, `level` (`fail`/`warn`), `code`, `scope` (`need`, +`bucket`, or `repo` for a schema error), `need` (the need's id, or the +bucket's name), `subject` (the state, the move as `from>to`, or the +page), `message` and `fix`. The file is written empty when the content +is clean. `id` hashes scope, need, code and subject and nothing of the +wording, so the same gap keeps its id from one run to the next: a +trainer can tell what a round closed, what is still open and what it +broke. The codes: + +| code | | +|---|---| +| `schema` | content the site would refuse to load | +| `need-duplicate`, `persona-unknown-need` | needs.yaml itself | +| `entry-missing`, `entry-gated`, `entry-no-responsible` | the page a visitor arrives on | +| `path-missing`, `path-closed`, `path-over-budget`, `path-no-action` | getting the answer recorded | +| `machine-missing`, `desk-missing`, `desk-gated-twice` | who works the bucket | +| `done-unknown`, `done-unreachable`, `success-not-done` | finishing a case | +| `stage-unknown`, `stage-unreachable`, `stage-order` | the business's own stages | +| `state-unreached`, `strand`, `edge-unoffered` | every state and move of the bucket | +| `mail-unreachable`, `mail-unknown-field` | what a state mails | + A need marked `planned` warns instead of failing. Exit status is the verdict, so a content repo's CI is one line. diff --git a/src/lint/check.rs b/src/lint/check.rs index 0030ac6..1c70723 100644 --- a/src/lint/check.rs +++ b/src/lint/check.rs @@ -24,6 +24,7 @@ pub async fn run(argv: Vec) -> anyhow::Result<()> { let mut skim = false; let mut sim_out: Option = None; let mut show_access = false; + let mut findings_out: Option = None; while let Some(arg) = args.next() { match arg.as_str() { @@ -35,6 +36,7 @@ pub async fn run(argv: Vec) -> anyhow::Result<()> { "--skim" => skim = true, "--needs-sim" => sim_out = args.next(), "--access" => show_access = true, + "--findings" => findings_out = args.next(), "--needs-score" => answers_in = args.next(), "--min-accuracy" => { min_accuracy = args.next().and_then(|v| v.parse().ok()).unwrap_or(min_accuracy) @@ -66,13 +68,14 @@ pub async fn run(argv: Vec) -> anyhow::Result<()> { (questions, aggregates_map, site, needs_raw) } else { eprintln!( - "usage: iris check (--repo [--branch main] [--subdir questions] | --path ) [--needs-tasks [--skim]] [--needs-sim ] [--access] [--needs-score [--min-accuracy 0.8]]" + "usage: iris check (--repo [--branch main] [--subdir questions] | --path ) [--findings ] [--needs-tasks [--skim]] [--needs-sim ] [--access] [--needs-score [--min-accuracy 0.8]]" ); std::process::exit(2); }; if let Err(e) = site.validate() { eprintln!("FAIL: {e}"); + write_findings(findings_out.as_deref(), &[schema_finding(&e.to_string())]); std::process::exit(1); } let aggregates_map = content::with_builtin_aggregates(aggregates_map); @@ -114,6 +117,7 @@ pub async fn run(argv: Vec) -> anyhow::Result<()> { println!(); } check_needs( + findings_out.as_deref(), needs_raw.as_deref(), &questions, &aggregates_map, @@ -129,14 +133,43 @@ pub async fn run(argv: Vec) -> anyhow::Result<()> { // One schema error is no reason to hide the rest: the needs // pass reads the same content and says what else is wrong, // so a fix round can take them all at once. + let mut all = vec![schema_finding(&e.to_string())]; if let Some(file) = needs_raw.as_deref().and_then(|raw| needs::parse(raw).ok()) { - print_findings(&needs::check(&file, &questions, &aggregates_map)); + let findings = needs::check(&file, &questions, &aggregates_map); + print_findings(&findings); + all.extend(findings); } + write_findings(findings_out.as_deref(), &all); std::process::exit(1); } } } +/// A content error the schema pass stopped at, as a finding: the repo +/// is its scope and the message its subject, since it has no other. +fn schema_finding(message: &str) -> needs::Finding { + needs::Finding { + need: String::new(), + level: needs::Level::Fail, + message: message.to_string(), + scope: "repo", + code: "schema", + subject: message.to_string(), + } +} + +/// `--findings`: every finding as one JSON line (id, level, code, +/// scope, need, subject, message, fix), the same ones the text output +/// prints - an empty file when there are none, so a reader can tell +/// "clean" from "not run". +fn write_findings(out: Option<&str>, findings: &[needs::Finding]) { + let Some(out) = out else { return }; + let lines: String = findings.iter().map(|f| format!("{}\n", f.to_json())).collect(); + if let Err(e) = std::fs::write(out, lines) { + eprintln!("WARN: could not write the findings to {out}: {e}"); + } +} + /// Each finding on its own line, FAIL or WARN first; the count of FAILs. fn print_findings(findings: &[needs::Finding]) -> usize { let mut failed = 0; @@ -157,6 +190,7 @@ fn print_findings(findings: &[needs::Finding]) -> usize { /// then the wording tasks written out and/or an engine's answers /// scored, when asked. A repo without `needs.yaml` skips all of it. fn check_needs( + findings_out: Option<&str>, raw: Option<&str>, questions: &std::collections::HashMap, aggregates_map: &std::collections::HashMap, @@ -167,6 +201,7 @@ fn check_needs( min_accuracy: f64, ) -> anyhow::Result<()> { let Some(raw) = raw else { + write_findings(findings_out, &[]); if tasks_out.is_some() || answers_in.is_some() { eprintln!("FAIL: --needs-tasks/--needs-score given, but the repo has no needs.yaml"); std::process::exit(1); @@ -177,10 +212,13 @@ fn check_needs( Ok(file) => file, Err(e) => { eprintln!("FAIL: {e}"); + write_findings(findings_out, &[schema_finding(&e.to_string())]); std::process::exit(1); } }; - let failed = print_findings(&needs::check(&file, questions, aggregates_map)); + let findings = needs::check(&file, questions, aggregates_map); + let failed = print_findings(&findings); + write_findings(findings_out, &findings); for need in &file.needs { if let Some(path) = needs::path_for(questions, need) { let steps: Vec = path diff --git a/src/lint/needs.rs b/src/lint/needs.rs index 9d0f016..c538815 100644 --- a/src/lint/needs.rs +++ b/src/lint/needs.rs @@ -144,6 +144,48 @@ pub struct Finding { /// "need" for a need's own path; "bucket" for what holds for every /// record in a bucket whichever need brought it in. pub scope: &'static str, + /// What kind of gap it is, stable across releases: `strand`, + /// `edge-unoffered`, `done-unreachable`, ... (docs/lint.md). + pub code: &'static str, + /// What in the content it is about: a state, a move (`from>to`), a + /// page. With the scope, need and code it makes the finding's id. + pub subject: String, +} + +impl Finding { + /// The same gap keeps the same id however its message is worded, + /// so a fix round can tell what it closed, what is still open and + /// what it broke. FNV-1a over scope, need, code and subject. + pub fn id(&self) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for part in [self.scope, &self.need, self.code, &self.subject] { + for byte in part.bytes().chain(std::iter::once(0)) { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(0x0100_0000_01b3); + } + } + format!("{:08x}", hash >> 32) + } + + /// The change that closes it, when the lint knows one: what follows + /// "fix:" in the message. + pub fn fix(&self) -> Option<&str> { + self.message.split_once(" - fix: ").map(|(_, fix)| fix) + } + + /// One line of `--findings` JSONL. + pub fn to_json(&self) -> serde_json::Value { + serde_json::json!({ + "id": self.id(), + "level": match self.level { Level::Fail => "fail", Level::Warn => "warn" }, + "code": self.code, + "scope": self.scope, + "need": self.need, + "subject": self.subject, + "message": self.message, + "fix": self.fix(), + }) + } } #[derive(Clone, Debug, PartialEq, Eq)] @@ -354,10 +396,12 @@ pub fn check( let mut strands_reported: HashSet<(String, String)> = HashSet::new(); let mut unreached_reported: HashSet<(String, String)> = HashSet::new(); for need in &file.needs { - let mut report = |level: Level, message: String| { + let mut report = |level: Level, code: &'static str, subject: String, message: String| { findings.push(Finding { need: need.id.clone(), scope: "need", + code, + subject, level: if need.planned || need.paused { Level::Warn } else { level }, message: match (need.paused, need.planned, level == Level::Fail) { (true, _, true) => format!("paused, not open now: {message}"), @@ -367,15 +411,17 @@ pub fn check( }) }; if !seen.insert(need.id.as_str()) { - report(Level::Fail, "declared more than once".to_string()); + report(Level::Fail, "need-duplicate", String::new(), "declared more than once".to_string()); continue; } // The visitor's side: can they get their answer recorded? match questions.get(&need.enters) { - None => report(Level::Fail, format!("entry page {:?} does not exist", need.enters)), + None => report(Level::Fail, "entry-missing", need.enters.clone(), format!("entry page {:?} does not exist", need.enters)), Some(page) if !visible(page, need.enters_as.as_deref()) => report( Level::Fail, + "entry-gated", + need.enters.clone(), format!( "entry page {:?} is gated on {:?}, which this visitor does not hold", need.enters, page.qualifies @@ -385,6 +431,8 @@ pub fn check( if page.responsible.is_none() { report( Level::Warn, + "entry-no-responsible", + need.enters.clone(), format!("entry page {:?} names no one responsible to turn to", need.enters), ); } @@ -412,6 +460,8 @@ pub fn check( .unwrap_or_default(); report( Level::Fail, + "path-closed", + at.clone(), format!( "the path exists but is closed: {at} is `disabled`, so the answer is advertised and refused" ), @@ -419,6 +469,8 @@ pub fn check( } None => report( Level::Fail, + "path-missing", + String::new(), format!( "no path from {:?}{} records into {:?} within {} submission(s)", need.enters, @@ -435,6 +487,8 @@ pub fn check( if best.fields > budget { report( Level::Fail, + "path-over-budget", + String::new(), format!( "the path asks for {} required field(s), over the budget of {budget}", best.fields @@ -445,6 +499,8 @@ pub fn check( if best.records_with_action == Some(false) { report( Level::Warn, + "path-no-action", + String::new(), "the recording alternative has no `action` - the visitor is never told what happens next".to_string(), ); } @@ -457,6 +513,8 @@ pub fn check( let Some(schema) = aggregates.get(&need.lands_in) else { report( Level::Fail, + "machine-missing", + need.lands_in.clone(), format!("aggregates.yaml declares no state machine for {:?}", need.lands_in), ); continue; @@ -475,6 +533,8 @@ pub fn check( .collect(); report( Level::Fail, + "desk-missing", + need.handled_by.clone(), format!( "no desk gated on {:?} reads {:?}{}", need.handled_by, @@ -556,7 +616,7 @@ pub fn check( match ways_in(state).first() { Some((from, to)) => format!(" - fix: {}", fix_for(from, to)), None => format!( - " - and no state a record reaches has a declared edge into it: add one in aggregates.yaml under {:?}", + " - fix: no state a record reaches has a declared edge into it; add one in aggregates.yaml under {:?}", need.lands_in ), } @@ -565,12 +625,16 @@ pub fn check( if !schema.has_state(state) { report( Level::Fail, + "done-unknown", + state.clone(), format!("done_when names {state:?}, which {:?} has no such state", need.lands_in), ); } else if !reached.contains(state) { unreached_reported.insert((need.lands_in.clone(), state.clone())); report( Level::Fail, + "done-unreachable", + state.clone(), format!( "{:?} cannot move a record to {state:?} - no desk of theirs offers the moves{}", need.handled_by, @@ -583,6 +647,8 @@ pub fn check( if !schema.has_state(stage) { report( Level::Fail, + "stage-unknown", + stage.clone(), format!( "the business describes a stage {stage:?} that {:?} has no state for", need.lands_in @@ -592,6 +658,8 @@ pub fn check( unreached_reported.insert((need.lands_in.clone(), stage.clone())); report( Level::Fail, + "stage-unreachable", + stage.clone(), format!( "stage {stage:?} exists but {:?} is offered no moves that reach it{}", need.handled_by, @@ -613,11 +681,13 @@ pub fn check( if reached.contains(before) && reached.contains(after) && !reachable(before, &moves).contains(after) { report( Level::Fail, + "stage-order", + format!("{before}>{after}"), format!( "stage {after:?} comes after {before:?} for the business, but no move leads on from {before:?} to it{}", match schema.allowed(before).iter().find(|to| reachable(to, &moves).contains(after) || *to == after) { Some(to) => format!(" - fix: {}", fix_for(before, to)), - None => format!(" - add an edge from {before:?} toward {after:?} in aggregates.yaml under {:?}", need.lands_in), + None => format!(" - fix: add an edge from {before:?} toward {after:?} in aggregates.yaml under {:?}", need.lands_in), } ), ); @@ -627,6 +697,8 @@ pub fn check( if !need.done_when.contains(state) { report( Level::Fail, + "success-not-done", + state.clone(), format!("success names {state:?}, which is not one of done_when"), ); } @@ -639,12 +711,14 @@ pub fn check( let fix = match schema.allowed(&state).first() { Some(to) => format!(" - fix: {}", fix_for(&state, to)), None => format!( - " - and {:?} declares no way out of it: add {state:?} to done_when if it is finished, or an edge out in aggregates.yaml", + " - fix: {:?} declares no way out of it; add {state:?} to done_when if it is finished, or an edge out in aggregates.yaml", need.lands_in ), }; report( Level::Fail, + "strand", + state.clone(), format!("records strand in {state:?}: {movers} is offered no move out of it{fix}"), ); } @@ -658,6 +732,8 @@ pub fn check( findings.push(Finding { need: persona.need.clone(), scope: "need", + code: "persona-unknown-need", + subject: persona.id.clone(), level: Level::Fail, message: format!("persona {:?} points at a need that is not declared", persona.id), }); @@ -690,10 +766,12 @@ fn operations( // Only open needs make a gap fail; a bucket that only planned // or paused needs use reports it as a warning. let open = needs.iter().any(|n| !n.planned && !n.paused); - let mut report = |level: Level, message: String| { + let mut report = |level: Level, code: &'static str, subject: String, message: String| { findings.push(Finding { need: bucket.to_string(), scope: "bucket", + code, + subject, level: if open { level } else { Level::Warn }, message, }) @@ -728,6 +806,8 @@ fn operations( ways.sort(); report( Level::Fail, + "state-unreached", + (*state).clone(), format!( "state {state:?} is never reached: no desk, link or clock moves a record there - fix: {}", match ways.first() { @@ -747,6 +827,8 @@ fn operations( if !strands_reported.contains(&(bucket.to_string(), (*state).clone())) { report( Level::Fail, + "strand", + (*state).clone(), format!( "records strand in {state:?}: {bucket:?} declares moves on to {} but no desk, link or clock offers one - fix: {}", out.join(", "), @@ -759,8 +841,10 @@ fn operations( for to in out.iter().filter(|to| !offered(state, to)) { report( Level::Warn, + "edge-unoffered", + format!("{state}>{to}"), format!( - "the move {state:?} -> {to:?} is declared but no desk, link or clock offers it - {}, or drop the edge from aggregates.yaml", + "the move {state:?} -> {to:?} is declared but no desk, link or clock offers it - fix: {}, or drop the edge from aggregates.yaml", button(state, to) ), ); @@ -771,8 +855,10 @@ fn operations( if page_group != group { report( Level::Warn, + "desk-gated-twice", + desk.page.clone(), format!( - "the desk on {} is gated twice: the page qualifies {page_group:?}, its list requires {group:?} - only members of both see the buttons; make them the same group", + "the desk on {} is gated twice: the page qualifies {page_group:?}, its list requires {group:?}, so only members of both see the buttons - fix: make them the same group", desk.page ), ); @@ -793,7 +879,7 @@ fn operations( mail_states.sort_by(|a, b| a.0.cmp(b.0)); for (state, mail) in mail_states { if !reached.contains(state) { - report(Level::Warn, format!("the mail on entering {state:?} never goes out: no record reaches {state:?}")); + report(Level::Warn, "mail-unreachable", state.clone(), format!("the mail on entering {state:?} never goes out: no record reaches {state:?} - fix: make {state:?} reachable, or drop its mail")); } let mut unknown: Vec = Vec::new(); for key in placeholders(&mail.subject).into_iter().chain(placeholders(&mail.body)) { @@ -811,8 +897,10 @@ fn operations( names.dedup(); report( Level::Fail, + "mail-unknown-field", + state.clone(), format!( - "the mail on entering {state:?} uses {}, which no form recording into {bucket:?} collects - it would go out blank there; the forms collect: {}", + "the mail on entering {state:?} uses {}, which no form recording into {bucket:?} collects, so it would go out blank there - fix: use one of the fields the forms collect ({}), or ask for it on the form", unknown.iter().map(|k| format!("{{{k}}}")).collect::>().join(", "), if names.is_empty() { "nothing".to_string() } else { names.join(", ") } ), @@ -1407,7 +1495,7 @@ mod tests { assert!(fails(&findings).is_empty(), "{findings:?}"); assert_eq!( warns(&findings), - vec!["the move \"open\" -> \"lost\" is declared but no desk, link or clock offers it - add `{ from: open, to: lost, label: ... }` to the transitions on /desk, or drop the edge from aggregates.yaml"] + vec!["the move \"open\" -> \"lost\" is declared but no desk, link or clock offers it - fix: add `{ from: open, to: lost, label: ... }` to the transitions on /desk, or drop the edge from aggregates.yaml"] ); } @@ -1422,7 +1510,7 @@ mod tests { let findings = check(&needs(" stages: [open, surveyed, quoted, won, lost]\n"), &questions, &aggregates); assert_eq!( fails(&findings), - vec!["stage \"quoted\" comes after \"surveyed\" for the business, but no move leads on from \"surveyed\" to it - add an edge from \"surveyed\" toward \"quoted\" in aggregates.yaml under \"projects\""] + vec!["stage \"quoted\" comes after \"surveyed\" for the business, but no move leads on from \"surveyed\" to it - fix: add an edge from \"surveyed\" toward \"quoted\" in aggregates.yaml under \"projects\""] ); } @@ -1435,7 +1523,7 @@ mod tests { let findings = check(&needs(""), &questions, &aggregates); assert_eq!( fails(&findings), - vec!["the mail on entering \"open\" uses {refnr}, which no form recording into \"projects\" collects - it would go out blank there; the forms collect: brief, budget, email"] + vec!["the mail on entering \"open\" uses {refnr}, which no form recording into \"projects\" collects, so it would go out blank there - fix: use one of the fields the forms collect (brief, budget, email), or ask for it on the form"] ); } @@ -1445,8 +1533,29 @@ mod tests { questions.get_mut("/desk").unwrap().qualifies = Some("staff".into()); let findings = check(&needs(""), &questions, &aggregates); assert!( - warns(&findings).contains(&"the desk on /desk is gated twice: the page qualifies \"staff\", its list requires \"owners\" - only members of both see the buttons; make them the same group"), + warns(&findings).contains(&"the desk on /desk is gated twice: the page qualifies \"staff\", its list requires \"owners\", so only members of both see the buttons - fix: make them the same group"), "{findings:?}" ); } + + #[test] + fn a_finding_keeps_its_id_when_reworded_and_says_its_fix() { + let (questions, aggregates) = site(" - { to: quoted, label: Quote }\n - { to: lost, label: Lose }\n"); + let findings = check(&needs(""), &questions, &aggregates); + let strand = findings.iter().find(|f| f.code == "strand").unwrap(); + assert_eq!(strand.subject, "quoted"); + assert_eq!(strand.fix(), Some("add `{ from: quoted, to: won, label: ... }` to the transitions on /desk")); + let reworded = Finding { message: "something else entirely".into(), ..clone_of(strand) }; + assert_eq!(reworded.id(), strand.id()); + let elsewhere = Finding { subject: "open".into(), ..clone_of(strand) }; + assert_ne!(elsewhere.id(), strand.id()); + let json = strand.to_json(); + assert_eq!(json["code"], "strand"); + assert_eq!(json["level"], "fail"); + assert_eq!(json["id"], strand.id()); + } + + fn clone_of(f: &Finding) -> Finding { + Finding { need: f.need.clone(), level: f.level, message: f.message.clone(), scope: f.scope, code: f.code, subject: f.subject.clone() } + } }