diff --git a/bots/finishingbot/README.adoc b/bots/finishingbot/README.adoc index ee74fefd..8d4e7849 100644 --- a/bots/finishingbot/README.adoc +++ b/bots/finishingbot/README.adoc @@ -49,7 +49,7 @@ the gitbot-fleet shared context layer and makes the final release-readiness dete |No |**SCM Files** -|All 6 `.machine_readable/` files (STATE.scm, META.scm, ECOSYSTEM.scm, PLAYBOOK.scm, AGENTIC.scm, NEUROSYM.scm), SPDX headers, required sections, content completeness +|Legacy `.machine_readable/` SCM files (STATE.scm, META.scm, ECOSYSTEM.scm, PLAYBOOK.scm, AGENTIC.scm, NEUROSYM.scm) are validated only if present (SPDX headers, required sections, content completeness); their absence is canonical, since those descriptiles are now clauses of `_chora.deed` |No |**Testing** @@ -171,6 +171,19 @@ finishing-bot release finishing-bot security --panic-attacker-path /path/to/panic-attacker ---- +=== Estate canon and exemptions + +The analyzers accept the estate's canonical locations, not only one spelling: + +* `ROADMAP.adoc` at `docs/status/ROADMAP.adoc` (or the root); `SECURITY`, `CONTRIBUTING` and `CODE_OF_CONDUCT` as `.md` or `.adoc`, at the root or in `.github/` (V1-DOC, V1-SEC-001, V1-COM, REL-004). +* `mise.toml` in place of `.tool-versions` (TOOL-001). +* `tests/fuzz/` as well as `fuzz/` and `.clusterfuzzlite/` (TEST-RU-003); a crate with no code yet (no `.rs` line that is not blank or a comment) is exempt from TEST-RU-005 and TEST-RU-006. +* A workflow named by file name in prose resolves under `.github/workflows/` (CLM-006). +* SCM-001 is retired: STATE, META, ECOSYSTEM, PLAYBOOK, AGENTIC and NEUROSYM are clauses of `_chora.deed`, not files. +* REL-008 matches key material (PEM headers, AWS key ids, vendor token shapes, long key literals), not the words "private key". + +A repository can acknowledge a finding in its `.hypatia-baseline.json`, the estate's documented exemption mechanism (`docs/upstream-proposals/standards--hypatia-baseline-consumer/docs/EXEMPTION-MECHANISMS.adoc`, "Layer 2"). `.hypatia-ignore` is not read: that document rejects it. An entry uses this bot's `rule_module`, `finishing_bot`, and the finding id as `type`, with the estate severity word (error = `high`, warning = `medium`, info = `info`, suggestion = `low`) and exactly one of `file` or `file_pattern`: `{"severity": "medium", "rule_module": "finishing_bot", "type": "PH-001", "file_pattern": "build/just/**", "note": "scanner names the tokens"}`. Matching is exact on severity, module and type, so a rule family needs one entry per id. Entries past `expires_at`, or carrying `severity_override` (this bot does not downgrade), acknowledge nothing; other modules' entries (`cicd_rules`, ...) are Hypatia's and are not read. Currently only the placeholder analyzer (PH-nnn, hence PH-100) honours it. See `src/baseline.rs`. + === Security Analysis finishing-bot integrates with `panic-attacker` for security validation: diff --git a/bots/finishingbot/src/analyzers/claims.rs b/bots/finishingbot/src/analyzers/claims.rs index 81a034ff..82e3d72e 100644 --- a/bots/finishingbot/src/analyzers/claims.rs +++ b/bots/finishingbot/src/analyzers/claims.rs @@ -23,6 +23,13 @@ static PATH_RE: LazyLock = LazyLock::new(|| { .expect("Cargo workspace member-path regex is a valid constant pattern") }); +/// The line of `content` containing byte offset `at` (prose context for a reference). +fn line_of(content: &str, at: usize) -> &str { + let start = content[..at].rfind('\n').map_or(0, |i| i + 1); + let end = content[at..].find('\n').map_or(content.len(), |i| at + i); + &content[start..end] +} + /// Claim verification analyzer pub struct ClaimsAnalyzer; @@ -297,7 +304,14 @@ impl ClaimsAnalyzer { for pattern in &file_ref_patterns { for caps in pattern.captures_iter(content) { let file_ref = &caps[1]; - if resolve_local_reference(repo_root, doc_path, file_ref).is_none() { + if resolve_local_reference( + repo_root, + doc_path, + file_ref, + line_of(content, caps.get(0).map_or(0, |m| m.start())), + ) + .is_none() + { debug!(doc = %doc_path.display(), reference = file_ref, "Broken file reference"); result.add( Finding::new( diff --git a/bots/finishingbot/src/analyzers/license.rs b/bots/finishingbot/src/analyzers/license.rs index 4ffbf853..7a09c94b 100644 --- a/bots/finishingbot/src/analyzers/license.rs +++ b/bots/finishingbot/src/analyzers/license.rs @@ -7,13 +7,15 @@ use crate::config::Config; use crate::error::Result; use regex::Regex; use std::path::Path; +use std::sync::LazyLock; use tracing::debug; use walkdir::WalkDir; -use std::sync::LazyLock; // Compiled once. These were rebuilt on every call (hypatia expect_in_hot_path). -static SPDX_PATTERN: LazyLock = - LazyLock::new(|| Regex::new(r"(?i)SPDX-License-Identifier:\s*[A-Za-z0-9.-]+").expect("SPDX header regex is a valid constant pattern")); +static SPDX_PATTERN: LazyLock = LazyLock::new(|| { + Regex::new(r"(?i)SPDX-License-Identifier:\s*[A-Za-z0-9.-]+") + .expect("SPDX header regex is a valid constant pattern") +}); /// Repositories that deliberately use AGPL-3.0-or-later instead of PMPL. /// These are co-developed projects with the user's son and use AGPL by deliberate choice. @@ -100,9 +102,7 @@ impl LicenseAnalyzer { "COPYING.md", ]; - let found_license = license_files - .iter() - .find(|name| path.join(name).exists()); + let found_license = license_files.iter().find(|name| path.join(name).exists()); match found_license { None => { @@ -207,7 +207,9 @@ impl LicenseAnalyzer { return Some("GPL-3.0-or-later".to_string()); } - if content_lower.contains("mit license") || content_lower.contains("permission is hereby granted, free of charge") { + if content_lower.contains("mit license") + || content_lower.contains("permission is hereby granted, free of charge") + { return Some("MIT".to_string()); } @@ -215,7 +217,9 @@ impl LicenseAnalyzer { return Some("Apache-2.0".to_string()); } - if content_lower.contains("bsd 3-clause") || content_lower.contains("redistribution and use in source and binary forms") { + if content_lower.contains("bsd 3-clause") + || content_lower.contains("redistribution and use in source and binary forms") + { return Some("BSD-3-Clause".to_string()); } @@ -331,7 +335,12 @@ impl LicenseAnalyzer { let new_content = if content.starts_with("#!") { let mut lines = content.lines(); let shebang = lines.next().unwrap_or(""); - format!("{}\n{}{}", shebang, header, lines.collect::>().join("\n")) + format!( + "{}\n{}{}", + shebang, + header, + lines.collect::>().join("\n") + ) } else { format!("{}{}", header, content) }; diff --git a/bots/finishingbot/src/analyzers/mod.rs b/bots/finishingbot/src/analyzers/mod.rs index 2e108a0f..124db560 100644 --- a/bots/finishingbot/src/analyzers/mod.rs +++ b/bots/finishingbot/src/analyzers/mod.rs @@ -17,6 +17,22 @@ use crate::error::Result; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; +/// First of `candidates` (repo-relative, in preference order) that exists +/// under `root`. Estate canon allows a document in more than one place +/// (`.adoc` or `.md`, repo root or `.github/`), and a rule that names exactly +/// one spelling reports a false positive for the others. +pub(crate) fn first_existing(root: &Path, candidates: &[&str]) -> Option { + candidates.iter().map(|c| root.join(c)).find(|p| p.exists()) +} + +/// Accepted spellings and locations of a security policy. +pub(crate) const SECURITY_POLICY: &[&str] = &[ + "SECURITY.md", + "SECURITY.adoc", + ".github/SECURITY.md", + ".github/SECURITY.adoc", +]; + /// Severity levels for findings #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] @@ -282,14 +298,14 @@ mod scope_tests { let cfg = Config::default(); let mut out = Vec::new(); let analyzers: Vec> = vec![ - Box::new(license::LicenseAnalyzer::default()), - Box::new(placeholder::PlaceholderAnalyzer::default()), - Box::new(claims::ClaimsAnalyzer::default()), - Box::new(release::ReleaseAnalyzer::default()), - Box::new(scm_files::ScmFilesAnalyzer::default()), - Box::new(testing::TestingAnalyzer::default()), - Box::new(tooling::ToolingAnalyzer::default()), - Box::new(v1_readiness::V1ReadinessAnalyzer::default()), + Box::new(license::LicenseAnalyzer), + Box::new(placeholder::PlaceholderAnalyzer), + Box::new(claims::ClaimsAnalyzer), + Box::new(release::ReleaseAnalyzer), + Box::new(scm_files::ScmFilesAnalyzer), + Box::new(testing::TestingAnalyzer), + Box::new(tooling::ToolingAnalyzer), + Box::new(v1_readiness::V1ReadinessAnalyzer), ]; for a in &analyzers { out.extend(a.analyze(dir, &cfg).unwrap().findings); @@ -304,7 +320,11 @@ mod scope_tests { .filter(|f| f.file.is_none() && !NO_MEANINGFUL_FILE.contains(&f.id.as_str())) .map(|f| f.id.as_str()) .collect(); - assert!(unpathed.is_empty(), "findings without a path: {:?}", unpathed); + assert!( + unpathed.is_empty(), + "findings without a path: {:?}", + unpathed + ); findings.len() } @@ -340,6 +360,9 @@ mod scope_tests { .into_iter() .find(|f| f.id == "LIC-001") .expect("LIC-001 on an empty repo"); - assert_eq!(f.file.as_deref(), Some(dir.path().join("LICENSE").as_path())); + assert_eq!( + f.file.as_deref(), + Some(dir.path().join("LICENSE").as_path()) + ); } } diff --git a/bots/finishingbot/src/analyzers/placeholder.rs b/bots/finishingbot/src/analyzers/placeholder.rs index f5281488..8c191d13 100644 --- a/bots/finishingbot/src/analyzers/placeholder.rs +++ b/bots/finishingbot/src/analyzers/placeholder.rs @@ -3,6 +3,7 @@ //! Placeholder detection analyzer use super::{AnalysisResult, Analyzer, Finding, Severity}; +use crate::baseline::Baseline; use crate::config::{Config, PlaceholderAction}; use crate::error::Result; use regex::{Regex, RegexSet}; @@ -39,11 +40,16 @@ impl Analyzer for PlaceholderAnalyzer { // Add custom regex patterns patterns.extend(config.placeholders.custom_patterns.clone()); + // Findings the repository has acknowledged in its .hypatia-baseline.json + // (rule_module "finishing_bot", type "PH-nnn"). + let baseline = Baseline::load(path); + let pattern_set = RegexSet::new(&patterns)?; let patterns_with_context: Vec = patterns .iter() .map(|p| Regex::new(p)) - .collect::, regex::Error>>()?; + .collect::, regex::Error>>( + )?; // Scan files for entry in WalkDir::new(path) @@ -75,6 +81,8 @@ impl Analyzer for PlaceholderAnalyzer { &pattern_set, &patterns_with_context, config, + &baseline, + path, &mut result, ); } @@ -95,7 +103,10 @@ impl Analyzer for PlaceholderAnalyzer { ), ) .with_file(Path::new(".finishing-bot/config.yml").to_path_buf()) - .with_suggestion(&format!("Remove at least {} placeholders before release", excess)), + .with_suggestion(&format!( + "Remove at least {} placeholders before release", + excess + )), ); } @@ -126,12 +137,10 @@ impl PlaceholderAnalyzer { /// Check if a file is likely binary fn is_binary_file(&self, path: &Path) -> bool { let binary_extensions = [ - "exe", "dll", "so", "dylib", "bin", "obj", "o", "a", "lib", - "png", "jpg", "jpeg", "gif", "bmp", "ico", "webp", "svg", - "mp3", "mp4", "wav", "ogg", "flac", "avi", "mkv", "mov", - "zip", "tar", "gz", "bz2", "xz", "7z", "rar", - "pdf", "doc", "docx", "xls", "xlsx", "ppt", "pptx", - "wasm", "pyc", "class", "lock", + "exe", "dll", "so", "dylib", "bin", "obj", "o", "a", "lib", "png", "jpg", "jpeg", + "gif", "bmp", "ico", "webp", "svg", "mp3", "mp4", "wav", "ogg", "flac", "avi", "mkv", + "mov", "zip", "tar", "gz", "bz2", "xz", "7z", "rar", "pdf", "doc", "docx", "xls", + "xlsx", "ppt", "pptx", "wasm", "pyc", "class", "lock", ]; path.extension() @@ -141,6 +150,7 @@ impl PlaceholderAnalyzer { } /// Scan file content for placeholders + #[allow(clippy::too_many_arguments)] fn scan_file_content( &self, path: &Path, @@ -148,6 +158,8 @@ impl PlaceholderAnalyzer { pattern_set: &RegexSet, patterns: &[Regex], config: &Config, + baseline: &Baseline, + root: &Path, result: &mut AnalysisResult, ) { let pattern_names = &config.placeholders.patterns; @@ -157,7 +169,21 @@ impl PlaceholderAnalyzer { for match_idx in matches { if let Some(pattern) = patterns.get(match_idx) { if let Some(m) = pattern.find(line) { - let pattern_name = pattern_names.get(match_idx).map(|s| s.as_str()).unwrap_or("custom"); + let pattern_name = pattern_names + .get(match_idx) + .map(|s| s.as_str()) + .unwrap_or("custom"); + let rule_id = format!("PH-{:03}", match_idx + 1); + let rel = normalise_rel(path.strip_prefix(root).unwrap_or(path)); + let severity = match config.placeholders.action { + PlaceholderAction::Remove => Severity::Error, + PlaceholderAction::Comment => Severity::Warning, + PlaceholderAction::Flag => Severity::Warning, + }; + if baseline.acknowledges(&rule_id, severity, &rel) { + debug!(file = %rel, rule = %rule_id, "Acknowledged in .hypatia-baseline.json"); + continue; + } debug!( file = %path.display(), @@ -166,12 +192,6 @@ impl PlaceholderAnalyzer { "Found placeholder" ); - let severity = match config.placeholders.action { - PlaceholderAction::Remove => Severity::Error, - PlaceholderAction::Comment => Severity::Warning, - PlaceholderAction::Flag => Severity::Warning, - }; - result.add( Finding::new( &format!("PH-{:03}", match_idx + 1), @@ -233,10 +253,8 @@ impl PlaceholderAnalyzer { let lines: Vec<&str> = content.lines().collect(); // Get line numbers to remove (1-indexed in findings) - let lines_to_remove: std::collections::HashSet = file_findings - .iter() - .filter_map(|f| f.line) - .collect(); + let lines_to_remove: std::collections::HashSet = + file_findings.iter().filter_map(|f| f.line).collect(); // Filter out lines with placeholders (convert to 0-indexed) let new_lines: Vec<&str> = lines @@ -319,3 +337,129 @@ impl PlaceholderAnalyzer { Ok(fixed) } } + +/// Repo-relative path with `/` separators, so `.hypatia-baseline.json` paths +/// and globs (always written with `/`) match on Windows too. +fn normalise_rel(p: &Path) -> String { + p.to_string_lossy().replace('\\', "/") +} + +#[cfg(test)] +mod baseline_tests { + use super::*; + use tempfile::TempDir; + + fn write(dir: &Path, rel: &str, body: &str) { + let p = dir.join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, body).unwrap(); + } + + /// A template-shaped repo: its own scanners legitimately name the tokens. + fn repo(baseline: Option<&str>) -> TempDir { + let d = TempDir::new().unwrap(); + write(d.path(), "build/just/scan.just", "grep -E 'TODO|FIXME' .\n"); + write(d.path(), "src/main.rs", "// TODO: real placeholder\n"); + if let Some(text) = baseline { + write(d.path(), ".hypatia-baseline.json", text); + } + d + } + + /// Repo-relative, `/`-separated paths of every PH-0nn finding. + fn files(d: &TempDir) -> Vec { + PlaceholderAnalyzer + .analyze(d.path(), &Config::default()) + .unwrap() + .findings + .into_iter() + .filter(|f| f.id.starts_with("PH-0")) + .map(|f| normalise_rel(f.file.unwrap().strip_prefix(d.path()).unwrap())) + .collect() + } + + /// The default config flags placeholders as warnings (estate word `medium`); + /// PH-001 is TODO and PH-002 is FIXME. + const BASELINE_JUST: &str = r#"[ + {"severity":"medium","rule_module":"finishing_bot","type":"PH-001","file_pattern":"build/just/**","note":"scanner names the tokens"}, + {"severity":"medium","rule_module":"finishing_bot","type":"PH-002","file_pattern":"build/just/**","note":"scanner names the tokens"} + ]"#; + + #[test] + fn backslash_paths_are_normalised_before_matching() { + let rel = normalise_rel(Path::new(r"build\just\scan.just")); + assert_eq!(rel, "build/just/scan.just"); + let b = Baseline::parse( + BASELINE_JUST, + chrono::NaiveDate::from_ymd_opt(2026, 11, 13).unwrap(), + ); + assert!(b.acknowledges("PH-001", Severity::Warning, &rel)); + } + + #[test] + fn without_a_baseline_every_placeholder_is_reported() { + let d = repo(None); + let got = files(&d); + assert!(got.contains(&"build/just/scan.just".to_string()), "{got:?}"); + assert!(got.contains(&"src/main.rs".to_string()), "{got:?}"); + } + + #[test] + fn hypatia_baseline_acknowledges_the_named_path_only() { + let d = repo(Some(BASELINE_JUST)); + let got = files(&d); + assert!( + !got.contains(&"build/just/scan.just".to_string()), + "{got:?}" + ); + // A real placeholder elsewhere still counts. + assert!(got.contains(&"src/main.rs".to_string()), "{got:?}"); + } + + #[test] + fn hypatia_ignore_is_not_read() { + let d = repo(None); + write( + d.path(), + ".hypatia-ignore", + "finishing_bot/PH:build/just/\n", + ); + assert!(files(&d).contains(&"build/just/scan.just".to_string())); + } + + #[test] + fn another_modules_or_severitys_entry_acknowledges_nothing_here() { + let hypatia = r#"[{"severity":"high","rule_module":"cicd_rules","type":"PH-001","file_pattern":"build/just/**"}]"#; + assert!(files(&repo(Some(hypatia))).contains(&"build/just/scan.just".to_string())); + let wrong_severity = BASELINE_JUST.replace("medium", "high"); + assert!(files(&repo(Some(&wrong_severity))).contains(&"build/just/scan.just".to_string())); + } + + #[test] + fn an_expired_baseline_entry_lets_the_finding_reappear() { + let expired = BASELINE_JUST.replace("\"note\"", "\"expires_at\":\"2020-01-01\",\"note\""); + assert!(files(&repo(Some(&expired))).contains(&"build/just/scan.just".to_string())); + } + + #[test] + fn acknowledged_scanner_files_no_longer_inflate_ph_100() { + let mut cfg = Config::default(); + cfg.placeholders.max_allowed = 2; // 2 hits in the scanner file, none elsewhere + let d = TempDir::new().unwrap(); + write(d.path(), "build/just/scan.just", "grep -E 'TODO|FIXME' .\n"); + let over = PlaceholderAnalyzer.analyze(d.path(), &cfg).unwrap(); + assert!( + !over.findings.iter().any(|f| f.id == "PH-100"), + "at the limit" + ); + cfg.placeholders.max_allowed = 1; + let over = PlaceholderAnalyzer.analyze(d.path(), &cfg).unwrap(); + assert!( + over.findings.iter().any(|f| f.id == "PH-100"), + "over the limit" + ); + write(d.path(), ".hypatia-baseline.json", BASELINE_JUST); + let ok = PlaceholderAnalyzer.analyze(d.path(), &cfg).unwrap(); + assert!(ok.findings.is_empty(), "{:?}", ok.findings); + } +} diff --git a/bots/finishingbot/src/analyzers/references.rs b/bots/finishingbot/src/analyzers/references.rs index 501dd0d4..32088178 100644 --- a/bots/finishingbot/src/analyzers/references.rs +++ b/bots/finishingbot/src/analyzers/references.rs @@ -23,13 +23,30 @@ fn local_file(root: &Path, candidate: &Path) -> Option { (resolved.starts_with(root) && resolved.is_file()).then_some(resolved) } +/// `Some(file name)` when `path` is a `workflows/.yml` reference, or a +/// bare `*.yml`/`*.yaml` name written in workflow prose (`in_workflow_context`). +/// A bare YAML name with no workflow context gets no `.github/workflows/` +/// fallback: it could be any config file, and must resolve normally. +fn workflow_file_name(path: &Path, in_workflow_context: bool) -> Option<&std::ffi::OsStr> { + let name = path.file_name()?; + let is_yaml = path.extension().is_some_and(|e| e == "yml" || e == "yaml"); + let parts: Vec<_> = path.components().collect(); + let qualified_ok = (parts.len() == 1 && in_workflow_context) + || (parts.len() == 2 && parts[0].as_os_str() == "workflows"); + (is_yaml && qualified_ok).then_some(name) +} + /// Prose can name a document-relative path, a repository-relative path, or an /// unanchored module basename. The last form is accepted only when unique. /// This resolves a file's identity; it does not verify its contents or claims. +/// +/// `context` is the prose line the reference sits on; it only decides whether a +/// bare YAML name is a workflow reference. pub(super) fn resolve_local_reference( repo: &Path, document: &Path, reference: &str, + context: &str, ) -> Option { let root = repo.canonicalize().ok()?; let path = Path::new(reference); @@ -44,12 +61,26 @@ pub(super) fn resolve_local_reference( if let Some(found) = local_file(&root, &root.join(path)) { return Some(found); } + // A workflow is named by its file name in prose (`estate-rules.yml`, or + // `workflows/estate-rules.yml`) but lives under .github/workflows/. Look + // there first, so the reference resolves even when the same basename also + // exists elsewhere (which would make the basename search ambiguous). + let workflow_prose = context.to_ascii_lowercase().contains("workflow"); + if let Some(name) = workflow_file_name(path, workflow_prose) { + if let Some(found) = local_file(&root, &root.join(".github/workflows").join(name)) { + return Some(found); + } + } // A qualified missing path must not be "repaired" by a same-named file // elsewhere. Nor may generated output stand in for a source module. if path.components().count() != 1 { return None; } let name = path.file_name()?; + // A bare YAML name that is not workflow prose is not a workflow reference, + // so the basename search must not be satisfied by a workflow file either. + let skip_workflows = workflow_file_name(path, true).is_some() && !workflow_prose; + let workflows_dir = root.join(".github/workflows"); let mut pending = vec![root.clone()]; let mut found = None; while let Some(directory) = pending.pop() { @@ -60,6 +91,7 @@ pub(super) fn resolve_local_reference( if !GENERATED_DIRS .iter() .any(|excluded| entry.file_name() == *excluded) + && !(skip_workflows && entry.path() == workflows_dir) { pending.push(entry.path()); } @@ -99,7 +131,10 @@ mod tests { path } fn resolves(&self, document: &str, reference: &str) -> bool { - resolve_local_reference(&self.0, &self.0.join(document), reference).is_some() + self.resolves_in(document, reference, "") + } + fn resolves_in(&self, document: &str, reference: &str, context: &str) -> bool { + resolve_local_reference(&self.0, &self.0.join(document), reference, context).is_some() } } impl Drop for Scratch { @@ -127,6 +162,44 @@ mod tests { assert!(!s.resolves("docs/note.adoc", "Echo.agda")); } + #[test] + fn workflow_names_resolve_under_dot_github_workflows() { + let s = Scratch::new(); + s.file(".github/workflows/estate-rules.yml"); + let ctx = "the estate-rules.yml workflow runs the gate"; + assert!(s.resolves_in("docs/note.adoc", "estate-rules.yml", ctx)); + assert!(s.resolves("docs/note.adoc", "workflows/estate-rules.yml")); + assert!(s.resolves("docs/note.adoc", ".github/workflows/estate-rules.yml")); + // Ambiguous basenames elsewhere do not defeat the workflow directory. + s.file("tools/estate-rules.yml"); + assert!(s.resolves_in("docs/note.adoc", "estate-rules.yml", ctx)); + } + + #[test] + fn a_bare_yml_name_without_workflow_context_does_not_fall_back_to_workflows() { + let s = Scratch::new(); + s.file(".github/workflows/config.yml"); + // Not a workflow reference: a missing config file is not satisfied by an + // unrelated workflow of the same name. + assert!(!s.resolves("docs/note.adoc", "config.yml")); + assert!(!s.resolves_in( + "docs/note.adoc", + "config.yml", + "see config.yml for settings" + )); + // Normal resolution still works for a real, unique file. + s.file("conf/real.yml"); + assert!(s.resolves("docs/note.adoc", "real.yml")); + } + + #[test] + fn a_workflow_name_with_no_such_workflow_is_still_unresolved() { + let s = Scratch::new(); + s.file(".github/workflows/other.yml"); + assert!(!s.resolves("docs/note.adoc", "estate-rules.yml")); + assert!(!s.resolves("docs/note.adoc", "workflows/estate-rules.yml")); + } + #[test] fn a_missing_qualified_path_is_not_satisfied_by_another_basename() { let s = Scratch::new(); diff --git a/bots/finishingbot/src/analyzers/release.rs b/bots/finishingbot/src/analyzers/release.rs index 16637abc..88a84d19 100644 --- a/bots/finishingbot/src/analyzers/release.rs +++ b/bots/finishingbot/src/analyzers/release.rs @@ -2,13 +2,12 @@ // SPDX-FileCopyrightText: 2025 Jonathan D.A. Jewell //! Release readiness analyzer -use super::{AnalysisResult, Analyzer, Finding, Severity}; +use super::{first_existing, AnalysisResult, Analyzer, Finding, Severity, SECURITY_POLICY}; use crate::config::Config; use crate::error::Result; use regex::Regex; use sha2::{Digest, Sha256, Sha512}; use std::path::Path; -use tracing::debug; use walkdir::WalkDir; /// Release readiness analyzer @@ -121,7 +120,10 @@ impl ReleaseAnalyzer { "REL-001", "Version Inconsistency", Severity::Error, - &format!("Version numbers are inconsistent:\n {}", version_list.join("\n ")), + &format!( + "Version numbers are inconsistent:\n {}", + version_list.join("\n ") + ), ) .with_file(path.join(&inconsistent[0].0)) .with_suggestion("Ensure all version files have the same version number"), @@ -150,16 +152,11 @@ impl ReleaseAnalyzer { "import pdb", "pdb.set_trace", "dbg!", - "println!", // In Rust release builds + "println!", // In Rust release builds "#[cfg(debug_assertions)]", ]; - let debug_files = [ - ".env.local", - ".env.development", - "debug.log", - "*.log", - ]; + let debug_files = [".env.local", ".env.development", "debug.log", "*.log"]; // Check for debug files for pattern in &debug_files { @@ -232,7 +229,10 @@ impl ReleaseAnalyzer { "REL-003", "Debug Statement", Severity::Info, - &format!("Found {} occurrence(s) of '{}' in source", count, pattern), + &format!( + "Found {} occurrence(s) of '{}' in source", + count, pattern + ), ) .with_file(entry_path.to_path_buf()) .with_suggestion("Review and remove debug statements for release"), @@ -246,33 +246,19 @@ impl ReleaseAnalyzer { /// Check for required security files fn check_security_files(&self, path: &Path, result: &mut AnalysisResult) { - let security_files = [ - ("SECURITY.md", "Security policy", Severity::Warning), - ("SECURITY.adoc", "Security policy", Severity::Warning), - ]; - - let has_security = security_files - .iter() - .any(|(name, _, _)| path.join(name).exists()); - - if !has_security { + // SECURITY.md or .adoc, at the root or in .github/ (GitHub reads both). + if first_existing(path, SECURITY_POLICY).is_none() { result.add( Finding::new( "REL-004", "Missing Security Policy", Severity::Warning, - "No SECURITY.md file found", + "No SECURITY.md found (checked the root and .github/)", ) - .with_file(path.join("SECURITY.md")) + .with_file(path.join(SECURITY_POLICY[0])) .with_suggestion("Add a SECURITY.md describing how to report vulnerabilities"), ); } - - // Check for .github/SECURITY.md - let github_security = path.join(".github/SECURITY.md"); - if !github_security.exists() && !has_security { - debug!("No security policy found in .github either"); - } } /// Verify release artifact hashes @@ -362,7 +348,10 @@ impl ReleaseAnalyzer { Err(_) => return, }; - let actual_hash = if config.release.hash_algorithms.contains(&"SHA512".to_string()) + let actual_hash = if config + .release + .hash_algorithms + .contains(&"SHA512".to_string()) && hash_path.to_string_lossy().contains("512") { let mut hasher = Sha512::new(); @@ -380,7 +369,10 @@ impl ReleaseAnalyzer { "REL-007", "Hash Mismatch", Severity::Error, - &format!("Hash mismatch for {}: expected {}, got {}", filename, expected_hash, actual_hash), + &format!( + "Hash mismatch for {}: expected {}, got {}", + filename, expected_hash, actual_hash + ), ) .with_file(hash_path.to_path_buf()) .with_suggestion("Regenerate hash file or investigate file corruption"), @@ -390,23 +382,6 @@ impl ReleaseAnalyzer { /// Check for sensitive data that shouldn't be released fn check_sensitive_data(&self, path: &Path, config: &Config, result: &mut AnalysisResult) { - let sensitive_patterns = [ - (r#"(?i)api[_-]?key\s*[:=]\s*['"]?[a-zA-Z0-9]{20,}"#, "API Key"), - (r#"(?i)secret[_-]?key\s*[:=]\s*['"]?[a-zA-Z0-9]{20,}"#, "Secret Key"), - (r#"(?i)password\s*[:=]\s*['"]?[^\s'"]{8,}"#, "Password"), - (r"(?i)private[_-]?key", "Private Key Reference"), - (r"-----BEGIN (RSA |DSA |EC |OPENSSH )?PRIVATE KEY-----", "Private Key"), - (r"(?i)aws[_-]?access[_-]?key", "AWS Access Key"), - (r"(?i)aws[_-]?secret", "AWS Secret"), - (r"ghp_[a-zA-Z0-9]{36}", "GitHub Personal Access Token"), - (r"gho_[a-zA-Z0-9]{36}", "GitHub OAuth Token"), - ]; - - let compiled: Vec<(Regex, &str)> = sensitive_patterns - .iter() - .filter_map(|(p, name)| Regex::new(p).ok().map(|r| (r, *name))) - .collect(); - for entry in WalkDir::new(path) .follow_links(false) .max_depth(5) @@ -430,9 +405,29 @@ impl ReleaseAnalyzer { .map(|ext| { matches!( ext, - "rs" | "js" | "ts" | "py" | "rb" | "go" | "java" | "kt" | "swift" - | "c" | "cpp" | "h" | "hpp" | "yml" | "yaml" | "json" | "toml" - | "xml" | "env" | "conf" | "cfg" | "ini" | "sh" | "bash" + "rs" | "js" + | "ts" + | "py" + | "rb" + | "go" + | "java" + | "kt" + | "swift" + | "c" + | "cpp" + | "h" + | "hpp" + | "yml" + | "yaml" + | "json" + | "toml" + | "xml" + | "env" + | "conf" + | "cfg" + | "ini" + | "sh" + | "bash" ) }) .unwrap_or(false); @@ -442,20 +437,19 @@ impl ReleaseAnalyzer { } if let Ok(content) = std::fs::read_to_string(entry_path) { - for (pattern, name) in &compiled { - if pattern.is_match(&content) { - result.add( - Finding::new( - "REL-008", - "Sensitive Data", - Severity::Error, - &format!("Potential {} found in source file", name), - ) - .with_file(entry_path.to_path_buf()) - .with_suggestion("Remove sensitive data and rotate any exposed credentials"), - ); - break; // One warning per file - } + if let Some(name) = sensitive_kind(&content) { + result.add( + Finding::new( + "REL-008", + "Sensitive Data", + Severity::Error, + &format!("Potential {} found in source file", name), + ) + .with_file(entry_path.to_path_buf()) + .with_suggestion( + "Remove sensitive data and rotate any exposed credentials", + ), + ); } } } @@ -476,7 +470,9 @@ impl ReleaseAnalyzer { "No [profile.release] configuration in Cargo.toml", ) .with_file(cargo_toml) - .with_suggestion("Consider adding release optimizations (lto, codegen-units)"), + .with_suggestion( + "Consider adding release optimizations (lto, codegen-units)", + ), ); } } @@ -494,8 +490,254 @@ impl ReleaseAnalyzer { "No GitHub release workflow found", ) .with_file(release_workflow.clone()) - .with_suggestion("Consider adding .github/workflows/release.yml for automated releases"), + .with_suggestion( + "Consider adding .github/workflows/release.yml for automated releases", + ), + ); + } + } +} + +/// Key material patterns for REL-008. These match credentials themselves +/// (PEM headers, assignments of a long literal, vendor token shapes), never +/// the WORDS "private key" or "aws secret": a config label such as +/// `trust.no-private-keys = "probe-deferred"` or a document describing key +/// hygiene is not a leaked key. +static SENSITIVE_PATTERNS: std::sync::LazyLock> = + std::sync::LazyLock::new(|| { + [ + ( + r#"(?i)api[_-]?key["']?\s*[:=]\s*['"]?[a-zA-Z0-9]{20,}"#, + "API Key", + ), + ( + r#"(?i)secret[_-]?key["']?\s*[:=]\s*['"]?[a-zA-Z0-9]{20,}"#, + "Secret Key", + ), + ( + r#"(?i)aws[_-]?secret[_-]?(access[_-]?)?key["']?\s*[:=]\s*['"]?[A-Za-z0-9/+=]{40}(?:$|[^A-Za-z0-9/+=])"#, + "AWS Secret Access Key", + ), + (r#"(?i)password["']?\s*[:=]\s*['"]?[^\s'"]{8,}"#, "Password"), + ( + r"-----BEGIN ([A-Z0-9]+ )*PRIVATE KEY( BLOCK)?-----", + "Private Key", + ), + // Unarmored key material: the key NAME, then a long hex or base64 value. + // `private_key_path = "/etc/..."` and prose never match: the name must be + // followed directly by the separator, and the value must start alphanumeric. + ( + r#"(?im)private[_-]?key["']?\s*[:=]\s*['"]?(?:[0-9a-f]{64,}|[a-z0-9][a-z0-9+/]{42,}={0,2})(?:["'\s,;}\]]|$)"#, + "Private Key", + ), + (r"\b(AKIA|ASIA)[0-9A-Z]{16}\b", "AWS Access Key ID"), + (r"ghp_[a-zA-Z0-9]{36}", "GitHub Personal Access Token"), + (r"gho_[a-zA-Z0-9]{36}", "GitHub OAuth Token"), + ] + .iter() + .map(|(p, name)| { + ( + Regex::new(p).expect("REL-008 sensitive pattern is a valid constant regex"), + *name, + ) + }) + .collect() + }); + +/// The kind of key material `content` contains, if any (first match wins). +fn sensitive_kind(content: &str) -> Option<&'static str> { + SENSITIVE_PATTERNS + .iter() + .find(|(re, _)| re.is_match(content)) + .map(|(_, name)| *name) +} + +#[cfg(test)] +mod sensitive_tests { + use super::*; + + #[test] + fn words_about_keys_are_not_key_material() { + for text in [ + r#"trust.no-private-keys = "probe-deferred""#, + "Never commit a private key or an AWS secret.", + "aws_access_key and aws-secret are documented here", + "private_key_path = \"/etc/ssl/key\"", + ] { + assert_eq!(sensitive_kind(text), None, "{text}"); + } + } + + #[test] + fn aws_secret_access_key_value_is_key_material() { + let key = concat!("wJalrXUt", "nFEMI/K7", "MDENG/bP", "xRfiCYEX", "AMPLEKEY"); + for text in [ + format!("aws_secret_access_key = \"{key}\""), + format!("AWS_SECRET_ACCESS_KEY={key}"), + // JSON: a closing quote sits between the name and the separator. + format!("{{\"aws_secret_access_key\": \"{key}\"}}"), + format!("\"AWS_SECRET_ACCESS_KEY\":\"{key}\""), + ] { + assert_eq!( + sensitive_kind(&text), + Some("AWS Secret Access Key"), + "{text}" + ); + } + assert_eq!(sensitive_kind("aws_secret_access_key = \"$SECRET\""), None); + assert_eq!( + sensitive_kind("The aws_secret_access_key is rotated monthly"), + None + ); + } + + #[test] + fn aws_secret_ending_in_base64_punctuation_is_key_material() { + // The 40th character may be `/`, `+` or `=`: `\b` cannot terminate those. + for last in ['/', '+', '=', 'A', '9'] { + let key = format!( + "{}{}", + concat!("wJalrXUt", "nFEMI/K7", "MDENG/bP", "xRfiCYEX", "AMPLEKE"), + last ); + assert_eq!(key.len(), 40); + for text in [ + format!("aws_secret_access_key = \"{key}\""), + format!("AWS_SECRET_ACCESS_KEY={key}"), + format!("AWS_SECRET_ACCESS_KEY={key}\n"), + format!("aws_secret_access_key: {key} # note"), + ] { + assert_eq!( + sensitive_kind(&text), + Some("AWS Secret Access Key"), + "{text}" + ); + } + } + // A run longer than 40 key characters is not a 40-character key. + let long = format!("AWS_SECRET_ACCESS_KEY={}", "A".repeat(41)); + assert_eq!(sensitive_kind(&long), None); + let long_slash = format!("AWS_SECRET_ACCESS_KEY={}/", "A".repeat(40)); + assert_eq!(sensitive_kind(&long_slash), None); + } + + #[test] + fn json_quoted_names_are_key_material() { + assert_eq!( + sensitive_kind(concat!("{\"api_key\": \"abcdefghij", "0123456789xyz\"}")), + Some("API Key") + ); + assert_eq!( + sensitive_kind(concat!("{\"secret_key\": \"abcdefghij", "0123456789xyz\"}")), + Some("Secret Key") + ); + assert_eq!( + sensitive_kind(concat!("{\"password\": \"hunter", "2hunter2\"}")), + Some("Password") + ); + } + + #[test] + fn unarmored_private_key_values_are_key_material() { + let hex = "0123456789abcdef".repeat(4); + let b64 = concat!("QUJDREVG", "R0hJSktM", "TU5PUFFS", "U1RVVldY", "WVowMTIz", "NDU2Nzg5"); + for text in [ + format!("private_key = \"{hex}\""), + format!("PRIVATE-KEY: {hex}"), + format!("{{\"private_key\": \"{hex}\"}}"), + format!("private_key = \"{b64}=\""), + format!("privateKey: {b64}"), + ] { + assert_eq!(sensitive_kind(&text), Some("Private Key"), "{text}"); + } + } + + #[test] + fn private_key_paths_prose_and_short_values_are_not_key_material() { + let hex = "0123456789abcdef".repeat(4); + for text in [ + "private_key_path = \"/etc/ssl/private/server.key\"".to_string(), + format!("private_key_file = \"{hex}\""), + "the private key is stored in the vault, never in the repo".to_string(), + "private_key = \"/etc/ssl/private/a/very/long/path/to/the/server/key\"".to_string(), + "private_key = \"deadbeef\"".to_string(), + "private_key = \"$PRIVATE_KEY\"".to_string(), + ] { + assert_eq!(sensitive_kind(&text), None, "{text}"); + } + } + + /// REL-008 must not flag its own source: fixtures are built from fragments. + #[test] + fn this_file_is_not_itself_key_material() { + assert_eq!(sensitive_kind(include_str!("release.rs")), None); + } + + #[test] + fn pem_headers_are_key_material() { + for kind in [ + "RSA PRIVATE KEY", + "PRIVATE KEY", + "EC PRIVATE KEY", + "OPENSSH PRIVATE KEY", + "ENCRYPTED PRIVATE KEY", + ] { + let text = format!("{}{}{}\nMIIB\n", "-----BEGIN ", kind, "-----"); + assert_eq!(sensitive_kind(&text), Some("Private Key"), "{kind}"); } + let pgp = format!("{}{}{}", "-----BEGIN ", "PGP PRIVATE KEY BLOCK", "-----"); + assert_eq!(sensitive_kind(&pgp), Some("Private Key")); + } + + #[test] + fn key_literals_are_key_material() { + let aws = format!("id = {}{}", "AKIA", "IOSFODNN7EXAMPLE"); + assert_eq!(sensitive_kind(&aws), Some("AWS Access Key ID")); + let ghp = format!("t = {}{}", "ghp_", "a".repeat(36)); + assert_eq!(sensitive_kind(&ghp), Some("GitHub Personal Access Token")); + assert_eq!( + sensitive_kind(concat!("api_key = \"abcdefghij", "0123456789xyz\"")), + Some("API Key") + ); + } + + #[test] + fn rel_008_ignores_a_label_but_reports_a_key_file() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::write( + dir.path().join("trust.toml"), + "trust.no-private-keys = \"probe-deferred\"\n", + ) + .unwrap(); + let mut result = AnalysisResult::new(); + ReleaseAnalyzer.check_sensitive_data(dir.path(), &Config::default(), &mut result); + assert!(result.findings.is_empty(), "{:?}", result.findings); + + std::fs::write( + dir.path().join("leak.toml"), + format!( + "k = \"\"\"\n-----BEGIN {} KEY-----\nAAAA\n\"\"\"\n", + "PRIVATE" + ), + ) + .unwrap(); + ReleaseAnalyzer.check_sensitive_data(dir.path(), &Config::default(), &mut result); + assert_eq!(result.findings.len(), 1); + assert_eq!(result.findings[0].id, "REL-008"); + } + + #[test] + fn security_policy_is_found_in_dot_github() { + let dir = tempfile::TempDir::new().unwrap(); + let mut result = AnalysisResult::new(); + ReleaseAnalyzer.check_security_files(dir.path(), &mut result); + assert_eq!(result.findings.len(), 1); + assert_eq!(result.findings[0].id, "REL-004"); + + std::fs::create_dir_all(dir.path().join(".github")).unwrap(); + std::fs::write(dir.path().join(".github/SECURITY.md"), "x").unwrap(); + let mut again = AnalysisResult::new(); + ReleaseAnalyzer.check_security_files(dir.path(), &mut again); + assert!(again.findings.is_empty()); } } diff --git a/bots/finishingbot/src/analyzers/scm_files.rs b/bots/finishingbot/src/analyzers/scm_files.rs index 08879e58..692c4b2d 100644 --- a/bots/finishingbot/src/analyzers/scm_files.rs +++ b/bots/finishingbot/src/analyzers/scm_files.rs @@ -29,7 +29,10 @@ impl Analyzer for ScmFilesAnalyzer { // Required SCM files for v1.0 let required_scm_files = [ ("STATE.scm", "Project state and progress tracking"), - ("META.scm", "Meta-level information and architectural decisions"), + ( + "META.scm", + "Meta-level information and architectural decisions", + ), ("ECOSYSTEM.scm", "Project's place in the ecosystem"), ("PLAYBOOK.scm", "Operational playbooks and procedures"), ("AGENTIC.scm", "Agentic AI integration documentation"), @@ -38,7 +41,7 @@ impl Analyzer for ScmFilesAnalyzer { let machine_readable_dir = path.join(".machine_readable"); - for (filename, description) in &required_scm_files { + for (filename, _description) in &required_scm_files { let correct_path = machine_readable_dir.join(filename); let root_path = path.join(filename); @@ -65,22 +68,19 @@ impl Analyzer for ScmFilesAnalyzer { ) .with_file(root_path) .with_suggestion(&format!( - "Move {} to .machine_readable/{}", filename, filename + "Move {} to .machine_readable/{}", + filename, filename )), ); } else { - result.add( - Finding::new( - &format!("SCM-001-{}", filename), - &format!("Missing {}", filename), - Severity::Error, - &format!("Required SCM file {} not found ({}).", filename, description), - ) - .with_file(correct_path.clone()) - .with_suggestion(&format!( - "Create .machine_readable/{} with proper metadata and content. See hyperpolymath SCM file specs.", - filename - )), + // SCM-001 (a required .scm file is missing) is retired. The six + // descriptive documents (STATE, META, ECOSYSTEM, PLAYBOOK, + // AGENTIC, NEUROSYM) are clauses of the repository's + // `_chora.deed`, not files, so absence is the canonical + // state. Files that DO exist are still validated above. + debug!( + "{} not present (retired: now a clause of the chora deed)", + filename ); } } @@ -128,7 +128,11 @@ impl ScmFilesAnalyzer { &format!("SCM-003-{}", filename), &format!("{} appears incomplete", filename), Severity::Warning, - &format!("{} is very short ({} bytes). May be a placeholder.", filename, content.len()), + &format!( + "{} is very short ({} bytes). May be a placeholder.", + filename, + content.len() + ), ) .with_file(file_path.to_path_buf()) .with_suggestion("Ensure file has complete documentation content"), @@ -148,7 +152,9 @@ impl ScmFilesAnalyzer { "STATE.scm should contain 'metadata' and 'current-position' sections.", ) .with_file(file_path.to_path_buf()) - .with_suggestion("Add required sections following hyperpolymath STATE.scm format"), + .with_suggestion( + "Add required sections following hyperpolymath STATE.scm format", + ), ); } "META.scm" @@ -185,3 +191,41 @@ impl ScmFilesAnalyzer { } } } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn ids(dir: &Path) -> Vec { + ScmFilesAnalyzer + .analyze(dir, &Config::default()) + .unwrap() + .findings + .into_iter() + .map(|f| f.id) + .collect() + } + + #[test] + fn absent_scm_files_are_not_a_finding() { + // The six descriptive documents are clauses of _chora.deed now. + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join("x_chora.deed"), "(deed)").unwrap(); + let got = ids(d.path()); + assert!( + got.iter().all(|i| !i.starts_with("SCM-001")), + "SCM-001 is retired, got {:?}", + got + ); + assert!(got.is_empty(), "got {:?}", got); + } + + #[test] + fn a_misplaced_scm_file_is_still_reported() { + // Negative control: the analyzer still runs for files that exist. + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join("STATE.scm"), ";; x\n").unwrap(); + assert!(ids(d.path()).contains(&"SCM-005-STATE.scm".to_string())); + } +} diff --git a/bots/finishingbot/src/analyzers/testing.rs b/bots/finishingbot/src/analyzers/testing.rs index 43886197..fdea70fc 100644 --- a/bots/finishingbot/src/analyzers/testing.rs +++ b/bots/finishingbot/src/analyzers/testing.rs @@ -187,15 +187,19 @@ impl TestingAnalyzer { // Check for fuzzing (cargo-fuzz or ClusterFuzzLite) let fuzz_dir = path.join("fuzz"); + let tests_fuzz_dir = path.join("tests/fuzz"); let clusterfuzz_dir = path.join(".clusterfuzzlite"); - if !fuzz_dir.exists() && !clusterfuzz_dir.exists() { + if !has_fuzz_harness(&fuzz_dir) + && !has_fuzz_harness(&tests_fuzz_dir) + && !clusterfuzz_dir.exists() + { result.add( Finding::new( "TEST-RU-003", "Missing fuzzing infrastructure", Severity::Error, - "Rust project missing fuzz/ or .clusterfuzzlite/ for fuzzing.", + "Rust project has no fuzz harness: expected fuzz/ or tests/fuzz/ with a Cargo.toml declaring [[bin]] targets or fuzz_targets/*.rs using fuzz_target!, or .clusterfuzzlite/.", ) .with_file(fuzz_dir.clone()) .with_suggestion("Add ClusterFuzzLite configuration for automated fuzzing"), @@ -222,9 +226,13 @@ impl TestingAnalyzer { } } + // A crate with no code yet has nothing to stress or benchmark, so + // TEST-RU-005 and TEST-RU-006 wait until there is some (see `rust_has_code`). + let has_code = rust_has_code(path); + // Check for stress test workflow let stress_workflow = path.join(".github/workflows/stress-test.yml"); - if !stress_workflow.exists() { + if !stress_workflow.exists() && has_code { result.add( Finding::new( "TEST-RU-005", @@ -241,7 +249,7 @@ impl TestingAnalyzer { // Check Cargo.toml for criterion dependency if let Ok(content) = std::fs::read_to_string(path.join("Cargo.toml")) { - if !content.contains("criterion") { + if has_code && !content.contains("criterion") { result.add( Finding::new( "TEST-RU-006", @@ -429,3 +437,414 @@ mod agda_layout_tests { .any(|finding| finding.id == "TEST-001")); } } + +/// Remove Rust block comments, which nest (`/* a /* b */ c */`). +/// +/// `/*` only opens a comment in code: not inside a `//` line comment (to the +/// end of the line), a string literal (`"..."` with escapes, `r#"..."#`) or a +/// char literal. Line comments and strings are kept verbatim; inside a block +/// comment only newlines are kept, so line numbers survive. +fn strip_block_comments(src: &str) -> String { + let b = src.as_bytes(); + let mut out = Vec::with_capacity(b.len()); + let (mut i, mut depth) = (0, 0usize); + while i < b.len() { + if depth > 0 { + if b[i] == b'/' && b.get(i + 1) == Some(&b'*') { + depth += 1; + i += 2; + } else if b[i] == b'*' && b.get(i + 1) == Some(&b'/') { + depth -= 1; + i += 2; + } else { + if b[i] == b'\n' { + out.push(b'\n'); + } + i += 1; + } + continue; + } + match b[i] { + b'/' if b.get(i + 1) == Some(&b'*') => { + depth = 1; + i += 2; + } + b'/' if b.get(i + 1) == Some(&b'/') => { + // Line comment: copy to (not including) the newline. + while i < b.len() && b[i] != b'\n' { + out.push(b[i]); + i += 1; + } + } + b'r' if raw_string_hashes(b, i).is_some() => { + let hashes = raw_string_hashes(b, i).unwrap_or(0); + // r, hashes, opening quote. + let open = 1 + hashes + 1; + out.extend_from_slice(&b[i..i + open]); + i += open; + while i < b.len() { + let closes = b[i] == b'"' && (1..=hashes).all(|k| b.get(i + k) == Some(&b'#')); + out.push(b[i]); + i += 1; + if closes { + out.extend_from_slice(&b[i..i + hashes]); + i += hashes; + break; + } + } + } + b'"' => { + out.push(b'"'); + i += 1; + while i < b.len() { + let c = b[i]; + out.push(c); + i += 1; + if c == b'\\' && i < b.len() { + out.push(b[i]); + i += 1; + } else if c == b'"' { + break; + } + } + } + b'\'' => { + // A char literal ('x', '\n', '"'); a lifetime ('a) has no + // closing quote two or three bytes on and is left alone. + let end = match (b.get(i + 1), b.get(i + 2), b.get(i + 3)) { + (Some(b'\\'), _, Some(b'\'')) => Some(i + 4), + (Some(c), Some(b'\''), _) if *c != b'\\' => Some(i + 3), + _ => None, + }; + let end = end.unwrap_or(i + 1); + out.extend_from_slice(&b[i..end]); + i = end; + } + c => { + out.push(c); + i += 1; + } + } + } + String::from_utf8_lossy(&out).into_owned() +} + +/// If a raw string literal (`r"`, `r#"`, ...) starts at `b[i]` (an `r` that is +/// not the tail of an identifier), the number of `#`s in its delimiter. +fn raw_string_hashes(b: &[u8], i: usize) -> Option { + if i > 0 && (b[i - 1].is_ascii_alphanumeric() || b[i - 1] == b'_') { + return None; + } + let hashes = b[i + 1..].iter().take_while(|&&c| c == b'#').count(); + (b.get(i + 1 + hashes) == Some(&b'"')).then_some(hashes) +} + +/// Whether the Rust project at `root` has any code yet. +/// +/// "No code" is exact: no `.rs` file outside build output (`target/`), +/// vendored trees, `node_modules/` and hidden directories contains a line that +/// is neither blank nor a comment. An empty `src/lib.rs`, one holding only +/// `//!` docs or an SPDX header, and a crate with no `.rs` files at all, are +/// all "no code"; `fn main() {}` is code. Used to exempt benchmark and stress +/// requirements from a freshly scaffolded crate. +pub(crate) fn rust_has_code(root: &Path) -> bool { + walkdir::WalkDir::new(root) + .follow_links(false) + .into_iter() + .filter_entry(|e| { + if e.depth() == 0 || !e.file_type().is_dir() { + return true; + } + let name = e.file_name().to_string_lossy(); + !(name.starts_with('.') + || matches!(name.as_ref(), "target" | "vendor" | "node_modules")) + }) + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().is_file() && e.path().extension().is_some_and(|x| x == "rs")) + .any(|e| { + std::fs::read_to_string(e.path()).is_ok_and(|src| { + strip_block_comments(&src) + .lines() + .map(str::trim) + .any(|l| !l.is_empty() && !l.starts_with("//")) + }) + }) +} + +/// True when `dir` (a `fuzz/` or `tests/fuzz/` directory) holds a recognisable +/// harness: a `Cargo.toml` with a real `[[bin]]` table whose target file exists +/// (cargo-fuzz layout), or a `fuzz_targets/*.rs` file that invokes +/// `fuzz_target!` outside comments. An empty directory, a placeholder file or a +/// commented-out declaration is not fuzzing infrastructure. +fn has_fuzz_harness(dir: &Path) -> bool { + if let Ok(manifest) = std::fs::read_to_string(dir.join("Cargo.toml")) { + if manifest_has_existing_bin(dir, &manifest) { + return true; + } + } + let Ok(entries) = std::fs::read_dir(dir.join("fuzz_targets")) else { + return false; + }; + entries.filter_map(|e| e.ok()).any(|e| { + let p = e.path(); + p.extension().is_some_and(|x| x == "rs") + && std::fs::read_to_string(&p).is_ok_and(|s| invokes_fuzz_target(&s)) + }) +} + +/// True when `src` invokes the `fuzz_target!` macro in code: not in a `//` or +/// `/* */` comment, and followed (after optional whitespace) by `(`, `{` or `|`. +fn invokes_fuzz_target(src: &str) -> bool { + const NAME: &str = "fuzz_target!"; + strip_block_comments(src).lines().any(|line| { + let code = line.split("//").next().unwrap_or(""); + code.match_indices(NAME).any(|(at, _)| { + code[at + NAME.len()..] + .trim_start() + .starts_with(['(', '{', '|']) + }) + }) +} + +/// True when the manifest has a `[[bin]]` table header on a non-comment line +/// whose target file exists under `dir`: its `path` if given, else the cargo +/// default for the named target (`src/bin/.rs`, `src/bin//main.rs`, +/// or `src/main.rs`). +fn manifest_has_existing_bin(dir: &Path, manifest: &str) -> bool { + let mut tables: Vec<(Option, Option)> = Vec::new(); + let mut in_bin = false; + for raw in manifest.lines() { + let line = raw.split('#').next().unwrap_or("").trim(); + if line.starts_with('[') { + in_bin = line == "[[bin]]"; + if in_bin { + tables.push((None, None)); + } + } else if in_bin { + if let Some((key, value)) = line.split_once('=') { + let value = value.trim().trim_matches(['"', '\'']).to_string(); + if let Some(t) = tables.last_mut() { + match key.trim() { + "path" => t.0 = Some(value), + "name" => t.1 = Some(value), + _ => {} + } + } + } + } + } + tables.iter().any(|(path, name)| match (path, name) { + (Some(p), _) => dir.join(p).is_file(), + (None, Some(n)) => { + dir.join(format!("src/bin/{n}.rs")).is_file() + || dir.join(format!("src/bin/{n}/main.rs")).is_file() + || dir.join("src/main.rs").is_file() + } + (None, None) => dir.join("src/main.rs").is_file(), + }) +} + +#[cfg(test)] +mod no_code_tests { + use super::*; + use tempfile::TempDir; + + fn write(dir: &Path, rel: &str, body: &str) { + let p = dir.join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, body).unwrap(); + } + + fn rust_repo(lib: Option<&str>) -> TempDir { + let d = TempDir::new().unwrap(); + write( + d.path(), + "Cargo.toml", + "[package]\nname = \"x\"\nversion = \"0.1.0\"\n", + ); + if let Some(body) = lib { + write(d.path(), "src/lib.rs", body); + } + d + } + + fn ids(dir: &Path) -> Vec { + TestingAnalyzer + .analyze(dir, &Config::default()) + .unwrap() + .findings + .into_iter() + .map(|f| f.id) + .collect() + } + + #[test] + fn no_code_means_no_non_comment_line_in_any_rs_file() { + for body in [ + None, + Some(""), + Some("// SPDX-License-Identifier: MPL-2.0\n\n//! docs only\n"), + Some("/* block\n comment */\n// c\n"), + ] { + let d = rust_repo(body); + assert!(!rust_has_code(d.path()), "{body:?}"); + } + for body in [ + "fn main() {}\n", + "// c\npub fn f() {}\n", + "/* c */ pub struct S;\n", + ] { + let d = rust_repo(Some(body)); + assert!(rust_has_code(d.path()), "{body:?}"); + } + } + + #[test] + fn a_block_comment_opener_inside_a_line_comment_or_string_is_not_a_comment() { + // `/*` after `//` is line-comment text, not the start of block mode. + for body in [ + "// block comments start with /*\npub fn f() {}\n", + "/// docs mention /* here\npub fn f() {}\n", + "pub fn f() {} // trailing /* note\n", + "const S: &str = \"/*\";\n", + "const R: &str = r#\"/* \"#;\n", + "const C: char = '\"';\nconst S: &str = \"/*\";\n", + ] { + let d = rust_repo(Some(body)); + assert!(rust_has_code(d.path()), "{body:?}"); + } + // Real block comments still hide their contents, including a `//` + // inside them, and `//` inside a string does not hide a real opener. + for body in [ + "/* // not a line comment */\n// c\n", + "/* a\n// b\nc */\n", + "// only /* a comment\n// and another\n", + ] { + let d = rust_repo(Some(body)); + assert!(!rust_has_code(d.path()), "{body:?}"); + } + let d = rust_repo(Some("let u = \"http://x\"; /* real\ncomment */\n")); + assert!(rust_has_code(d.path()), "code before the opener"); + let d = rust_repo(Some("/* c */ // x /* y\n")); + assert!(!rust_has_code(d.path())); + } + + #[test] + fn nested_block_comments_are_stripped() { + let d = rust_repo(Some("/* a /* b */ c */\n// x\n")); + assert!(!rust_has_code(d.path())); + let d = rust_repo(Some("/* a /* b */ c */ fn f() {}\n")); + assert!(rust_has_code(d.path())); + } + + #[test] + fn code_in_build_output_vendor_or_hidden_dirs_does_not_count() { + let d = rust_repo(Some("// nothing\n")); + write(d.path(), "target/debug/build/x.rs", "fn a() {}\n"); + write(d.path(), "vendor/dep/lib.rs", "fn b() {}\n"); + write(d.path(), ".claude/worktrees/w/src/lib.rs", "fn c() {}\n"); + assert!(!rust_has_code(d.path())); + } + + #[test] + fn stress_and_criterion_are_exempt_for_a_crate_with_no_code() { + let d = rust_repo(Some("// SPDX only\n")); + let got = ids(d.path()); + assert!(!got.contains(&"TEST-RU-005".to_string()), "{got:?}"); + assert!(!got.contains(&"TEST-RU-006".to_string()), "{got:?}"); + } + + #[test] + fn stress_and_criterion_still_required_once_there_is_code() { + let d = rust_repo(Some("pub fn f() {}\n")); + let got = ids(d.path()); + assert!(got.contains(&"TEST-RU-005".to_string()), "{got:?}"); + assert!(got.contains(&"TEST-RU-006".to_string()), "{got:?}"); + } + + #[test] + fn fuzz_under_tests_satisfies_ru_003() { + let d = rust_repo(Some("pub fn f() {}\n")); + assert!(ids(d.path()).contains(&"TEST-RU-003".to_string())); + // A placeholder is not a harness. + write(d.path(), "tests/fuzz/README", "todo\n"); + write(d.path(), "tests/fuzz/fuzz_targets/t.rs", "// target\n"); + assert!(ids(d.path()).contains(&"TEST-RU-003".to_string())); + write( + d.path(), + "tests/fuzz/fuzz_targets/t.rs", + "#![no_main]\nlibfuzzer_sys::fuzz_target!(|data: &[u8]| { let _ = data; });\n", + ); + assert!(!ids(d.path()).contains(&"TEST-RU-003".to_string())); + } + + #[test] + fn fuzz_cargo_manifest_with_bin_targets_satisfies_ru_003() { + let d = rust_repo(Some("pub fn f() {}\n")); + write(d.path(), "fuzz/.gitkeep", ""); + assert!(ids(d.path()).contains(&"TEST-RU-003".to_string())); + write( + d.path(), + "fuzz/Cargo.toml", + "[package]\nname = \"x-fuzz\"\n", + ); + assert!(ids(d.path()).contains(&"TEST-RU-003".to_string())); + write( + d.path(), + "fuzz/Cargo.toml", + "[package]\nname = \"x-fuzz\"\n\n[[bin]]\nname = \"t\"\npath = \"fuzz_targets/t.rs\"\n", + ); + write(d.path(), "fuzz/fuzz_targets/t.rs", "fn main() {}\n"); + assert!(!ids(d.path()).contains(&"TEST-RU-003".to_string())); + } + + #[test] + fn comment_only_fuzz_target_mention_is_not_a_harness() { + let d = rust_repo(Some("pub fn f() {}\n")); + for body in [ + "// TODO: add fuzz_target!\n", + "/* fuzz_target!(|d: &[u8]| {}); */\n", + "// fuzz_target!(|d: &[u8]| {});\nfn main() {}\n", + "fn main() {} // fuzz_target!(|d: &[u8]| {})\n", + "fn fuzz_target_name() {}\n", + ] { + write(d.path(), "fuzz/fuzz_targets/t.rs", body); + assert!( + ids(d.path()).contains(&"TEST-RU-003".to_string()), + "{body:?}" + ); + } + for body in [ + "fuzz_target!(|d: &[u8]| {});\n", + "libfuzzer_sys::fuzz_target! { |d: &[u8]| { let _ = d; } }\n", + "/* c */ fuzz_target! (|d: &[u8]| {});\n", + ] { + write(d.path(), "fuzz/fuzz_targets/t.rs", body); + assert!( + !ids(d.path()).contains(&"TEST-RU-003".to_string()), + "{body:?}" + ); + } + } + + #[test] + fn commented_or_dangling_bin_table_is_not_a_harness() { + let d = rust_repo(Some("pub fn f() {}\n")); + write(d.path(), "fuzz/fuzz_targets/t.rs", "// nothing\n"); + for manifest in [ + "[package]\nname = \"x\"\n# [[bin]]\n# name = \"t\"\n", + "[package]\nname = \"x\"\n\n[[bin]] # real header, missing file\nname = \"t\"\npath = \"fuzz_targets/missing.rs\"\n", + "[package]\nname = \"x\"\n\n[[bin]]\nname = \"t\"\n", + ] { + write(d.path(), "fuzz/Cargo.toml", manifest); + assert!(ids(d.path()).contains(&"TEST-RU-003".to_string()), "{manifest:?}"); + } + write(d.path(), "fuzz/src/bin/t.rs", "fn main() {}\n"); + write( + d.path(), + "fuzz/Cargo.toml", + "[package]\nname = \"x\"\n\n[[bin]]\nname = \"t\"\n", + ); + assert!(!ids(d.path()).contains(&"TEST-RU-003".to_string())); + } +} diff --git a/bots/finishingbot/src/analyzers/tooling.rs b/bots/finishingbot/src/analyzers/tooling.rs index c066e75c..e95b9103 100644 --- a/bots/finishingbot/src/analyzers/tooling.rs +++ b/bots/finishingbot/src/analyzers/tooling.rs @@ -8,6 +8,29 @@ use crate::error::Result; use std::path::Path; use tracing::debug; +/// mise manifests that pin tool versions (accepted in place of `.tool-versions`). +const MISE_MANIFESTS: &[&str] = &["mise.toml", ".mise.toml"]; + +/// The tool names a parseable mise manifest pins in a non-empty `[tools]` +/// table (`None` if it is unparseable or pins nothing). A backend prefix +/// (`asdf:just`, `core:rust`) is dropped, so the name is the tool itself. +fn mise_tool_names(p: &Path) -> Option> { + let table = std::fs::read_to_string(p) + .ok()? + .parse::() + .ok()?; + let tools = table.get("tools")?.as_table()?; + if tools.is_empty() { + return None; + } + Some( + tools + .keys() + .map(|k| k.rsplit(':').next().unwrap_or(k).to_string()) + .collect(), + ) +} + /// Tooling and version management analyzer pub struct ToolingAnalyzer; @@ -26,18 +49,35 @@ impl Analyzer for ToolingAnalyzer { let mut result = AnalysisResult::new(); let start = std::time::Instant::now(); - // Check for .tool-versions (asdf/mise) + // Check for a tool-version pin: .tool-versions (asdf/mise) or mise.toml + // (mise's own manifest, which the estate uses). Either satisfies TOOL-001. let tool_versions = path.join(".tool-versions"); - if !tool_versions.exists() { + let mise_manifest = MISE_MANIFESTS + .iter() + .map(|n| path.join(n)) + .find_map(|p| mise_tool_names(&p).map(|names| (p, names))); + if !tool_versions.exists() && mise_manifest.is_some() { + debug!("Found mise manifest instead of .tool-versions"); + // Same project-relevant tools as the .tool-versions branch: a + // manifest pinning only unrelated tools is not a clean pass. + if let Some((manifest, names)) = &mise_manifest { + self.check_project_tools( + |t| names.iter().any(|n| n == t), + manifest, + "the mise manifest", + &mut result, + ); + } + } else if !tool_versions.exists() { result.add( Finding::new( "TOOL-001", - "Missing .tool-versions", + "Missing tool-version pin", Severity::Error, - ".tool-versions file not found for asdf/mise version management.", + "Neither .tool-versions nor a mise.toml with a non-empty [tools] table found for asdf/mise version management.", ) - .with_file(tool_versions.clone()) - .with_suggestion("Create .tool-versions file with required tool versions (rust, just, etc.)"), + .with_file(path.join("mise.toml")) + .with_suggestion("Create mise.toml (or .tool-versions) with required tool versions (rust, just, etc.)"), ); } else { debug!("Found .tool-versions"); @@ -85,7 +125,11 @@ impl Analyzer for ToolingAnalyzer { if let Ok(entries) = std::fs::read_dir(&workflows_dir) { let workflow_count = entries .filter_map(|e| e.ok()) - .filter(|e| e.path().extension().is_some_and(|ext| ext == "yml" || ext == "yaml")) + .filter(|e| { + e.path() + .extension() + .is_some_and(|ext| ext == "yml" || ext == "yaml") + }) .count(); if workflow_count < 10 { @@ -119,10 +163,38 @@ impl Analyzer for ToolingAnalyzer { impl ToolingAnalyzer { /// Validate .tool-versions content - fn validate_tool_versions( + fn validate_tool_versions(&self, content: &str, file_path: &Path, result: &mut AnalysisResult) { + self.check_project_tools( + |t| content.contains(&format!("{t} ")), + file_path, + ".tool-versions", + result, + ); + + // Check for empty or very short file + if content.trim().is_empty() { + result.add( + Finding::new( + "TOOL-007", + "Empty .tool-versions", + Severity::Error, + ".tool-versions file is empty.", + ) + .with_file(file_path.to_path_buf()) + .with_suggestion("Add tool versions (e.g., 'rust 1.83.0')"), + ); + } + } + + /// The project-relevant tools a pin file (`.tool-versions` or a mise + /// manifest) must carry: rust (TOOL-005) and just (TOOL-006) for a Rust + /// project. `pins(t)` says whether the pin file pins tool `t`; `label` + /// names the pin file in the finding. + fn check_project_tools( &self, - content: &str, + pins: impl Fn(&str) -> bool, file_path: &Path, + label: &str, result: &mut AnalysisResult, ) { let is_rust = file_path @@ -131,45 +203,137 @@ impl ToolingAnalyzer { .unwrap_or(false); // Check for rust version if Rust project - if is_rust && !content.contains("rust ") { + if is_rust && !pins("rust") { result.add( Finding::new( "TOOL-005", - "Missing rust version in .tool-versions", + &format!("Missing rust version in {label}"), Severity::Warning, - "Rust project should specify rust version in .tool-versions.", + &format!("Rust project should specify rust version in {label}."), ) .with_file(file_path.to_path_buf()) - .with_suggestion("Add 'rust ' to .tool-versions"), + .with_suggestion(&format!("Add rust to {label}")), ); } // Check for just (task runner) - if is_rust && !content.contains("just ") { + if is_rust && !pins("just") { result.add( Finding::new( "TOOL-006", - "Missing just version in .tool-versions", + &format!("Missing just version in {label}"), Severity::Info, - "Consider adding 'just' task runner to .tool-versions.", + &format!("Consider adding the 'just' task runner to {label}."), ) .with_file(file_path.to_path_buf()) - .with_suggestion("Add 'just ' to .tool-versions if using justfile"), + .with_suggestion(&format!("Add just to {label} if using justfile")), ); } + } +} - // Check for empty or very short file - if content.trim().is_empty() { - result.add( - Finding::new( - "TOOL-007", - "Empty .tool-versions", - Severity::Error, - ".tool-versions file is empty.", - ) - .with_file(file_path.to_path_buf()) - .with_suggestion("Add tool versions (e.g., 'rust 1.83.0')"), - ); +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn ids(dir: &Path) -> Vec { + ToolingAnalyzer + .analyze(dir, &Config::default()) + .unwrap() + .findings + .into_iter() + .map(|f| f.id) + .collect() + } + + #[test] + fn tool_001_requires_mise_manifest_to_pin_tools() { + for (body, ok) in [ + ("", false), + ("not [valid toml", false), + ("[env]\nA = \"1\"\n", false), + ("[tools]\n", false), + ("[tools]\nrust = \"1.83\"\n", true), + ] { + for name in ["mise.toml", ".mise.toml"] { + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join(name), body).unwrap(); + assert_eq!( + !ids(d.path()).contains(&"TOOL-001".to_string()), + ok, + "{name}: {body:?}" + ); + } + } + } + + #[test] + fn a_mise_manifest_must_pin_the_tools_the_project_needs() { + let write = |d: &TempDir, mise: &str| { + std::fs::write(d.path().join("Cargo.toml"), "[package]\nname = \"x\"\n").unwrap(); + std::fs::write(d.path().join("mise.toml"), mise).unwrap(); + }; + // Unrelated tools only: a Rust repo gets the same findings as a + // `.tool-versions` that lacks rust and just. + let d = TempDir::new().unwrap(); + write(&d, "[tools]\nnode = \"22\"\n"); + let got = ids(d.path()); + assert!(got.contains(&"TOOL-005".to_string()), "{got:?}"); + assert!(got.contains(&"TOOL-006".to_string()), "{got:?}"); + // rust without just: only the just note. + let d = TempDir::new().unwrap(); + write(&d, "[tools]\nrust = \"1.90\"\n"); + let got = ids(d.path()); + assert!(!got.contains(&"TOOL-005".to_string()), "{got:?}"); + assert!(got.contains(&"TOOL-006".to_string()), "{got:?}"); + // Both pinned (also behind a backend prefix): clean. + let d = TempDir::new().unwrap(); + write(&d, "[tools]\nrust = \"1.90\"\n\"asdf:just\" = \"1.40\"\n"); + let got = ids(d.path()); + assert!( + !got.iter().any(|i| i == "TOOL-005" || i == "TOOL-006"), + "{got:?}" + ); + // A non-Rust repo is not asked for rust or just. + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join("mise.toml"), "[tools]\nnode = \"22\"\n").unwrap(); + let got = ids(d.path()); + assert!( + !got.iter().any(|i| i == "TOOL-005" || i == "TOOL-006"), + "{got:?}" + ); + } + + #[test] + fn tool_001_fires_when_neither_pin_exists_and_names_mise_toml() { + let d = TempDir::new().unwrap(); + let f = ToolingAnalyzer + .analyze(d.path(), &Config::default()) + .unwrap() + .findings + .into_iter() + .find(|f| f.id == "TOOL-001") + .expect("TOOL-001 on a repo with no pin"); + assert_eq!( + f.file.as_deref(), + Some(d.path().join("mise.toml").as_path()) + ); + } + + #[test] + fn mise_toml_satisfies_tool_001() { + for name in ["mise.toml", ".mise.toml"] { + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join(name), "[tools]\nrust = \"1.90\"\n").unwrap(); + assert!(!ids(d.path()).contains(&"TOOL-001".to_string()), "{name}"); } } + + #[test] + fn tool_versions_still_satisfies_tool_001() { + let d = TempDir::new().unwrap(); + std::fs::write(d.path().join(".tool-versions"), "rust 1.90.0\n").unwrap(); + assert!(!ids(d.path()).contains(&"TOOL-001".to_string())); + } } diff --git a/bots/finishingbot/src/analyzers/v1_readiness.rs b/bots/finishingbot/src/analyzers/v1_readiness.rs index 13f80cd2..35ca9a3b 100644 --- a/bots/finishingbot/src/analyzers/v1_readiness.rs +++ b/bots/finishingbot/src/analyzers/v1_readiness.rs @@ -2,7 +2,7 @@ // SPDX-FileCopyrightText: 2025 Jonathan D.A. Jewell //! V1 release readiness analyzer -use super::{AnalysisResult, Analyzer, Finding, Severity}; +use super::{first_existing, AnalysisResult, Analyzer, Finding, Severity, SECURITY_POLICY}; use crate::config::Config; use crate::error::Result; use std::path::Path; @@ -53,15 +53,26 @@ impl Analyzer for V1ReadinessAnalyzer { impl V1ReadinessAnalyzer { /// Check for core documentation files fn check_core_docs(&self, path: &Path, result: &mut AnalysisResult) { - let required_docs = [ - ("README.adoc", "Main project documentation"), - ("ROADMAP.adoc", "Project roadmap"), - ("LICENSE", "License file"), + // (name, description, accepted locations in preference order). The + // first location is the canonical one and is the path reported when + // the document is missing. Estate canon keeps the roadmap at + // docs/status/ROADMAP.adoc (the STATE journal). + let required_docs: [(&str, &str, &[&str]); 3] = [ + ( + "README.adoc", + "Main project documentation", + &["README.adoc"], + ), + ( + "ROADMAP.adoc", + "Project roadmap", + &["docs/status/ROADMAP.adoc", "ROADMAP.adoc"], + ), + ("LICENSE", "License file", &["LICENSE"]), ]; - for (filename, description) in &required_docs { - let file_path = path.join(filename); - if !file_path.exists() { + for (filename, description, locations) in &required_docs { + if first_existing(path, locations).is_none() { result.add( Finding::new( &format!("V1-DOC-{}", filename), @@ -69,8 +80,8 @@ impl V1ReadinessAnalyzer { Severity::Error, &format!("Required file {} not found ({}).", filename, description), ) - .with_file(file_path.clone()) - .with_suggestion(&format!("Create {} file", filename)), + .with_file(path.join(locations[0])) + .with_suggestion(&format!("Create {} file", locations[0])), ); } else { debug!("Found {}", filename); @@ -80,20 +91,19 @@ impl V1ReadinessAnalyzer { /// Check for security files fn check_security_files(&self, path: &Path, result: &mut AnalysisResult) { - let security_file = path.join("SECURITY.md"); - if !security_file.exists() { + if first_existing(path, SECURITY_POLICY).is_none() { result.add( Finding::new( "V1-SEC-001", "Missing SECURITY.md", Severity::Warning, - "SECURITY.md not found for responsible disclosure.", + "No security policy (SECURITY.md or .github/SECURITY.md) found for responsible disclosure.", ) - .with_file(security_file.clone()) + .with_file(path.join(SECURITY_POLICY[0])) .with_suggestion("Add SECURITY.md with security policy and contact information"), ); } else { - debug!("Found SECURITY.md"); + debug!("Found security policy"); } } @@ -104,7 +114,10 @@ impl V1ReadinessAnalyzer { // Check for banned language files let banned_patterns = [ ("*.ts", "TypeScript files found (use AffineScript instead)"), - ("*.py", "Python files found (use Julia/Rust/AffineScript instead)"), + ( + "*.py", + "Python files found (use Julia/Rust/AffineScript instead)", + ), ("*.go", "Go files found (use Rust instead)"), ]; @@ -137,7 +150,10 @@ impl V1ReadinessAnalyzer { &format!("{}. Found {} file(s).", message, found_files.len()), ) .with_file(found_files[0].clone()) - .with_suggestion(&format!("Remove or convert {} files to allowed languages", pattern)), + .with_suggestion(&format!( + "Remove or convert {} files to allowed languages", + pattern + )), ); } } @@ -160,14 +176,33 @@ impl V1ReadinessAnalyzer { /// Check for community files fn check_community_files(&self, path: &Path, result: &mut AnalysisResult) { - let community_files = [ - ("CODE_OF_CONDUCT.md", "Code of conduct"), - ("CONTRIBUTING.md", "Contribution guidelines"), + // The rule id keeps the historical .md spelling; the estate writes + // these as .adoc, and GitHub reads them from the root or .github/. + let community_files: [(&str, &str, &[&str]); 2] = [ + ( + "CODE_OF_CONDUCT.md", + "Code of conduct", + &[ + "CODE_OF_CONDUCT.md", + "CODE_OF_CONDUCT.adoc", + ".github/CODE_OF_CONDUCT.md", + ".github/CODE_OF_CONDUCT.adoc", + ], + ), + ( + "CONTRIBUTING.md", + "Contribution guidelines", + &[ + "CONTRIBUTING.md", + "CONTRIBUTING.adoc", + ".github/CONTRIBUTING.md", + ".github/CONTRIBUTING.adoc", + ], + ), ]; - for (filename, description) in &community_files { - let file_path = path.join(filename); - if !file_path.exists() { + for (filename, description, locations) in &community_files { + if first_existing(path, locations).is_none() { result.add( Finding::new( &format!("V1-COM-{}", filename), @@ -175,7 +210,7 @@ impl V1ReadinessAnalyzer { Severity::Warning, &format!("{} file not found ({}).", filename, description), ) - .with_file(file_path.clone()) + .with_file(path.join(locations[0])) .with_suggestion(&format!("Add {} for community health", filename)), ); } else { @@ -184,3 +219,93 @@ impl V1ReadinessAnalyzer { } } } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn run(dir: &Path) -> Vec { + V1ReadinessAnalyzer + .analyze(dir, &Config::default()) + .unwrap() + .findings + } + + fn ids(dir: &Path) -> Vec { + run(dir).into_iter().map(|f| f.id).collect() + } + + fn touch(dir: &Path, rel: &str) { + let p = dir.join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, "x").unwrap(); + } + + #[test] + fn roadmap_is_missing_when_absent_and_names_the_canonical_path() { + let d = TempDir::new().unwrap(); + let f = run(d.path()) + .into_iter() + .find(|f| f.id == "V1-DOC-ROADMAP.adoc") + .expect("missing roadmap is reported"); + assert_eq!( + f.file.as_deref(), + Some(d.path().join("docs/status/ROADMAP.adoc").as_path()) + ); + } + + #[test] + fn roadmap_accepts_docs_status_and_root() { + for rel in ["docs/status/ROADMAP.adoc", "ROADMAP.adoc"] { + let d = TempDir::new().unwrap(); + touch(d.path(), rel); + assert!( + !ids(d.path()).contains(&"V1-DOC-ROADMAP.adoc".to_string()), + "{rel}" + ); + } + } + + #[test] + fn community_files_accept_adoc_and_dot_github() { + let none = TempDir::new().unwrap(); + let got = ids(none.path()); + assert!(got.contains(&"V1-COM-CONTRIBUTING.md".to_string())); + assert!(got.contains(&"V1-COM-CODE_OF_CONDUCT.md".to_string())); + for rel in [ + "CONTRIBUTING.md", + "CONTRIBUTING.adoc", + ".github/CONTRIBUTING.md", + ".github/CONTRIBUTING.adoc", + ] { + let d = TempDir::new().unwrap(); + touch(d.path(), rel); + assert!( + !ids(d.path()).contains(&"V1-COM-CONTRIBUTING.md".to_string()), + "{rel}" + ); + } + for rel in ["CODE_OF_CONDUCT.adoc", ".github/CODE_OF_CONDUCT.md"] { + let d = TempDir::new().unwrap(); + touch(d.path(), rel); + assert!( + !ids(d.path()).contains(&"V1-COM-CODE_OF_CONDUCT.md".to_string()), + "{rel}" + ); + } + // Accepting one document does not excuse the other. + let d = TempDir::new().unwrap(); + touch(d.path(), "CONTRIBUTING.adoc"); + assert!(ids(d.path()).contains(&"V1-COM-CODE_OF_CONDUCT.md".to_string())); + } + + #[test] + fn security_policy_accepts_dot_github() { + let none = TempDir::new().unwrap(); + assert!(ids(none.path()).contains(&"V1-SEC-001".to_string())); + let d = TempDir::new().unwrap(); + touch(d.path(), ".github/SECURITY.md"); + assert!(!ids(d.path()).contains(&"V1-SEC-001".to_string())); + } +} diff --git a/bots/finishingbot/src/baseline.rs b/bots/finishingbot/src/baseline.rs new file mode 100644 index 00000000..931ed9a9 --- /dev/null +++ b/bots/finishingbot/src/baseline.rs @@ -0,0 +1,303 @@ +// SPDX-License-Identifier: MPL-2.0 +// SPDX-FileCopyrightText: 2026 Jonathan D.A. Jewell +//! Acknowledged findings from the repository's `.hypatia-baseline.json`. +//! +//! The estate's documented exemption mechanism is the per-repo +//! `.hypatia-baseline.json` (`docs/upstream-proposals/standards--hypatia-baseline-consumer/`, +//! `docs/EXEMPTION-MECHANISMS.adoc`, "Layer 2"). `.hypatia-ignore` is an +//! anti-pattern there ("never read by anything"), so finishing-bot does not read +//! it. Format (`docs/HYPATIA-BASELINE-FORMAT.adoc`): a JSON array of entries. +//! +//! ```json +//! { "severity": "high", "rule_module": "finishing_bot", "type": "PH-001", +//! "file_pattern": "build/just/**", "note": "scanner names the tokens", +//! "tracking_issue": "owner/repo#1", "expires_at": "2027-06-30" } +//! ``` +//! +//! An entry acknowledges a finding iff `severity`, `rule_module` and `type` match +//! EXACTLY and `file` equals (or `file_pattern` globs) the finding's repo-relative +//! path. This bot's `rule_module` is `finishing_bot`; its `type` is the finding id +//! (`PH-001`); entries of other modules (`cicd_rules`, ...) belong to Hypatia and +//! are not read here. `severity` uses the estate words, mapped from this bot's own +//! as the report-back driver does: error = `high`, warning = `medium`, +//! info = `info`, suggestion = `low`. +//! +//! Deliberate limits, so that nothing is silently half-honoured: +//! - matching is exact, so a rule family needs one entry per finding id (and per +//! severity the id can take); there is no `PH` prefix or `*` wildcard; +//! - an entry past its `expires_at` (or with an unparseable one) acknowledges +//! nothing, which makes the finding reappear; +//! - an entry with `severity_override` is not applied: this bot does not downgrade +//! findings, so the finding stays visible at its own severity; +//! - a baseline that is not a JSON array, or an entry that is malformed, exempts +//! nothing and is logged. + +use crate::analyzers::Severity; +use chrono::NaiveDate; +use serde_json::Value; +use std::path::Path; +use tracing::warn; + +/// The `rule_module` under which finishing-bot's rules are addressed. +pub const MODULE: &str = "finishing_bot"; + +/// The baseline file name, at the repository root. +pub const FILE_NAME: &str = ".hypatia-baseline.json"; + +#[derive(Debug, Default, Clone)] +pub struct Baseline { + entries: Vec, +} + +#[derive(Debug, Clone)] +struct Entry { + severity: String, + rule: String, + target: Target, +} + +#[derive(Debug, Clone)] +enum Target { + File(String), + Pattern(glob::Pattern), +} + +/// The estate severity word for one of this bot's severities. +fn estate_severity(s: Severity) -> &'static str { + match s { + Severity::Error => "high", + Severity::Warning => "medium", + Severity::Info => "info", + Severity::Suggestion => "low", + } +} + +fn text_of<'a>(entry: &'a Value, key: &str) -> Option<&'a str> { + entry.get(key).and_then(Value::as_str) +} + +/// Rewrite a gitignore-style `file_pattern` into an anchored glob over +/// repo-relative paths: a leading `/` anchors to the root (and is dropped); a +/// pattern with no `/` other than a trailing one matches at any depth +/// (`*.rs` -> `**/*.rs`); a trailing `/` names a directory, so everything +/// beneath it matches. Any other pattern containing `/` stays anchored. +fn gitignore_glob(pattern: &str) -> String { + let (rooted, rest) = match pattern.strip_prefix('/') { + Some(r) => (true, r), + None => (false, pattern), + }; + let (dir_only, body) = match rest.strip_suffix('/') { + Some(b) => (true, b), + None => (false, rest), + }; + let anchored = rooted || body.contains('/'); + let mut out = String::new(); + if !anchored && !body.starts_with("**/") { + out.push_str("**/"); + } + out.push_str(body); + if dir_only { + out.push_str("/**"); + } + out +} + +impl Baseline { + /// Load `/.hypatia-baseline.json`; an absent or unreadable file + /// acknowledges nothing. + pub fn load(root: &Path) -> Self { + match std::fs::read_to_string(root.join(FILE_NAME)) { + Ok(t) => Self::parse(&t, chrono::Utc::now().date_naive()), + Err(_) => Self::default(), + } + } + + /// Parse baseline text as of `today` (entries expire after their date). + pub fn parse(text: &str, today: NaiveDate) -> Self { + let value: Value = match serde_json::from_str(text) { + Ok(v) => v, + Err(e) => { + warn!("{FILE_NAME} is not valid JSON ({e}); acknowledging nothing"); + return Self::default(); + } + }; + let Some(array) = value.as_array() else { + warn!("{FILE_NAME} is not a JSON array; acknowledging nothing"); + return Self::default(); + }; + let mut entries = Vec::new(); + for entry in array { + if text_of(entry, "rule_module") != Some(MODULE) { + continue; // Hypatia's own entry (or another module's), not ours + } + if entry.get("severity_override").is_some() { + warn!("{FILE_NAME}: severity_override is not supported by finishing-bot; entry not applied"); + continue; + } + if let Some(expires) = entry.get("expires_at") { + match expires + .as_str() + .and_then(|d| NaiveDate::parse_from_str(d, "%Y-%m-%d").ok()) + { + Some(date) if date >= today => {} + Some(_) => continue, // expired: the finding reappears + None => { + warn!("{FILE_NAME}: unparseable expires_at; entry not applied"); + continue; + } + } + } + let (Some(severity), Some(rule)) = (text_of(entry, "severity"), text_of(entry, "type")) + else { + warn!("{FILE_NAME}: entry lacks severity or type; not applied"); + continue; + }; + let target = match (text_of(entry, "file"), text_of(entry, "file_pattern")) { + (Some(file), None) => Target::File(file.to_string()), + (None, Some(pattern)) => match glob::Pattern::new(&gitignore_glob(pattern)) { + Ok(p) => Target::Pattern(p), + Err(e) => { + warn!("{FILE_NAME}: bad file_pattern {pattern:?} ({e}); not applied"); + continue; + } + }, + _ => { + warn!( + "{FILE_NAME}: entry needs exactly one of file or file_pattern; not applied" + ); + continue; + } + }; + entries.push(Entry { + severity: severity.to_string(), + rule: rule.to_string(), + target, + }); + } + Self { entries } + } + + /// Does an entry acknowledge finding `rule_id` of `severity` at repo-relative + /// path `rel`? + pub fn acknowledges(&self, rule_id: &str, severity: Severity, rel: &str) -> bool { + let rel = rel.strip_prefix("./").unwrap_or(rel); + let severity = estate_severity(severity); + let options = glob::MatchOptions { + case_sensitive: true, + require_literal_separator: true, + require_literal_leading_dot: false, + }; + self.entries.iter().any(|e| { + e.severity == severity + && e.rule == rule_id + && match &e.target { + Target::File(f) => f == rel, + Target::Pattern(p) => p.matches_with(rel, options), + } + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn day(s: &str) -> NaiveDate { + NaiveDate::parse_from_str(s, "%Y-%m-%d").unwrap() + } + + const FILE: &str = r#"[ + {"severity":"critical","rule_module":"cicd_rules","type":"banned_language_file","file":"a.py"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-001","file_pattern":"build/just/**"}, + {"severity":"medium","rule_module":"finishing_bot","type":"PH-002","file":"Justfile","note":"why"}, + {"severity":"high","rule_module":"finishing_bot","type":"REL-003","file_pattern":"scripts/check-*.sh"} + ]"#; + + #[test] + fn exact_rule_severity_and_path_or_pattern_are_acknowledged() { + let b = Baseline::parse(FILE, day("2026-11-13")); + assert!(b.acknowledges("PH-001", Severity::Error, "build/just/a.just")); + assert!(b.acknowledges("PH-001", Severity::Error, "./build/just/deep/a.just")); + assert!(b.acknowledges("PH-002", Severity::Warning, "Justfile")); + assert!(b.acknowledges("REL-003", Severity::Error, "scripts/check-x.sh")); + } + + #[test] + fn nothing_but_an_exact_match_is_acknowledged() { + let b = Baseline::parse(FILE, day("2026-11-13")); + assert!(!b.acknowledges("PH-001", Severity::Error, "src/main.rs")); // other path + assert!(!b.acknowledges("PH-001", Severity::Warning, "build/just/a.just")); // severity + assert!(!b.acknowledges("PH-003", Severity::Error, "build/just/a.just")); // exact id + assert!(!b.acknowledges("PH", Severity::Error, "build/just/a.just")); // no prefix form + assert!(!b.acknowledges("PH-002", Severity::Warning, "sub/Justfile")); // exact file + assert!(!b.acknowledges("REL-003", Severity::Error, "scripts/sub/check-x.sh")); // * stops at / + assert!(!b.acknowledges("banned_language_file", Severity::Error, "a.py")); + // Hypatia's entry + } + + #[test] + fn file_pattern_follows_gitignore_anchoring() { + let text = r#"[ + {"severity":"high","rule_module":"finishing_bot","type":"PH-001","file_pattern":"*.rs"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-002","file_pattern":"/*.rs"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-003","file_pattern":"build/"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-004","file_pattern":"src/*.rs"} + ]"#; + let b = Baseline::parse(text, day("2026-11-13")); + let e = Severity::Error; + // no slash: any depth, including the root + assert!(b.acknowledges("PH-001", e, "main.rs")); + assert!(b.acknowledges("PH-001", e, "src/main.rs")); + assert!(b.acknowledges("PH-001", e, "a/b/c/x.rs")); + assert!(!b.acknowledges("PH-001", e, "src/main.py")); + // leading slash: root only + assert!(b.acknowledges("PH-002", e, "main.rs")); + assert!(!b.acknowledges("PH-002", e, "src/main.rs")); + // trailing slash: a directory at any depth, everything beneath it + assert!(b.acknowledges("PH-003", e, "build/x")); + assert!(b.acknowledges("PH-003", e, "a/build/x/y")); + assert!(!b.acknowledges("PH-003", e, "rebuild/x")); + // a slash inside the pattern keeps it anchored + assert!(b.acknowledges("PH-004", e, "src/main.rs")); + assert!(!b.acknowledges("PH-004", e, "x/src/main.rs")); + } + + #[test] + fn expired_and_unsupported_entries_acknowledge_nothing() { + let text = r#"[ + {"severity":"high","rule_module":"finishing_bot","type":"PH-001","file":"a","expires_at":"2026-11-12"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-002","file":"a","expires_at":"2026-11-13"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-003","file":"a","expires_at":"soon"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-004","file":"a","severity_override":"advisory"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-005"}, + {"severity":"high","rule_module":"finishing_bot","type":"PH-006","file":"a","file_pattern":"*"} + ]"#; + let b = Baseline::parse(text, day("2026-11-13")); + assert!(!b.acknowledges("PH-001", Severity::Error, "a"), "expired"); + assert!( + b.acknowledges("PH-002", Severity::Error, "a"), + "last valid day" + ); + for id in ["PH-003", "PH-004", "PH-005", "PH-006"] { + assert!(!b.acknowledges(id, Severity::Error, "a"), "{id}"); + } + } + + #[test] + fn malformed_or_absent_baseline_acknowledges_nothing() { + for text in ["", "{", "{}", "\"x\"", "[1, null]"] { + let b = Baseline::parse(text, day("2026-11-13")); + assert!(!b.acknowledges("PH-001", Severity::Error, "a"), "{text}"); + } + let d = tempfile::TempDir::new().unwrap(); + assert!(!Baseline::load(d.path()).acknowledges("PH-001", Severity::Error, "a")); + } + + #[test] + fn estate_severity_words_follow_the_report_back_mapping() { + assert_eq!(estate_severity(Severity::Error), "high"); + assert_eq!(estate_severity(Severity::Warning), "medium"); + assert_eq!(estate_severity(Severity::Info), "info"); + assert_eq!(estate_severity(Severity::Suggestion), "low"); + } +} diff --git a/bots/finishingbot/src/config.rs b/bots/finishingbot/src/config.rs index 9a1c8087..9aca0bf2 100644 --- a/bots/finishingbot/src/config.rs +++ b/bots/finishingbot/src/config.rs @@ -308,7 +308,10 @@ fn default_log_format() -> String { /// Load configuration from a path pub fn load_config(path: &Path) -> Result { if !path.exists() { - debug!("Config file not found at {}, using defaults", path.display()); + debug!( + "Config file not found at {}, using defaults", + path.display() + ); return Ok(Config::default()); } @@ -362,9 +365,8 @@ mod tests { #[test] fn test_placeholder_action_serde() { let yaml = "action: remove"; - let config: PlaceholderConfig = serde_yaml::from_str(&format!( - "patterns: [TODO]\n{}", yaml - )).unwrap(); + let config: PlaceholderConfig = + serde_yaml::from_str(&format!("patterns: [TODO]\n{}", yaml)).unwrap(); assert_eq!(config.action, PlaceholderAction::Remove); } } diff --git a/bots/finishingbot/src/fleet.rs b/bots/finishingbot/src/fleet.rs index 1c75b48a..6375a964 100644 --- a/bots/finishingbot/src/fleet.rs +++ b/bots/finishingbot/src/fleet.rs @@ -40,11 +40,20 @@ pub mod cicd_hyper_a { #[serde(tag = "type", rename_all = "kebab-case")] pub enum ReleaseCheck { /// License validation - License { spdx_id: Option, require_header: bool }, + License { + spdx_id: Option, + require_header: bool, + }, /// Placeholder detection - Placeholder { patterns: Vec, allow_in_tests: bool }, + Placeholder { + patterns: Vec, + allow_in_tests: bool, + }, /// Claim verification - Claim { claim_type: String, verify_method: String }, + Claim { + claim_type: String, + verify_method: String, + }, /// Release artifact check Artifact { required_files: Vec }, /// Version consistency @@ -129,17 +138,11 @@ pub mod cicd_hyper_a { } /// Check if release should be blocked based on cicd-hyper-a rules - pub fn should_block_release( - findings: &[super::Finding], - rules: &[ReleaseRule], - ) -> bool { + pub fn should_block_release(findings: &[super::Finding], rules: &[ReleaseRule]) -> bool { for finding in findings { for rule in rules { if finding.id == rule.id && rule.blocks_release { - if matches!( - finding.severity, - super::Severity::Error - ) { + if matches!(finding.severity, super::Severity::Error) { return true; } } @@ -366,8 +369,12 @@ pub fn run_in_context(ctx: &mut Context, config: &Config) -> AuditResult { } // Complete bot execution - if let Err(e) = ctx.complete_bot(BotId::Finishbot, findings_count, errors_count, files_checked) - { + if let Err(e) = ctx.complete_bot( + BotId::Finishbot, + findings_count, + errors_count, + files_checked, + ) { error!("Failed to complete finishing-bot in context: {}", e); } @@ -448,9 +455,14 @@ mod tests { #[test] fn test_finding_conversion() { - let finding = Finding::new("LIC-001", "Missing License", Severity::Error, "No LICENSE file") - .with_file(PathBuf::from("LICENSE")) - .with_suggestion("Add a LICENSE file"); + let finding = Finding::new( + "LIC-001", + "Missing License", + Severity::Error, + "No LICENSE file", + ) + .with_file(PathBuf::from("LICENSE")) + .with_suggestion("Add a LICENSE file"); let fleet_finding = to_fleet_finding(&finding, "license"); diff --git a/bots/finishingbot/src/lib.rs b/bots/finishingbot/src/lib.rs index 5900c002..b5e43f83 100644 --- a/bots/finishingbot/src/lib.rs +++ b/bots/finishingbot/src/lib.rs @@ -14,6 +14,7 @@ #![forbid(unsafe_code)] pub mod analyzers; +pub mod baseline; pub mod config; pub mod error; @@ -190,7 +191,10 @@ pub fn print_summary(result: &AuditResult) { } fn print_findings_section(category: &str, findings: &[Finding]) { - println!("┌─ {} ─────────────────────────────────────────────────────", category); + println!( + "┌─ {} ─────────────────────────────────────────────────────", + category + ); for finding in findings { let severity_icon = match finding.severity { @@ -201,10 +205,7 @@ fn print_findings_section(category: &str, findings: &[Finding]) { }; println!("│"); - println!( - "│ {} [{}] {}", - severity_icon, finding.id, finding.name - ); + println!("│ {} [{}] {}", severity_icon, finding.id, finding.name); println!("│ {}", finding.message); if let Some(ref file) = finding.file { diff --git a/bots/finishingbot/src/main.rs b/bots/finishingbot/src/main.rs index 9091b241..d3d40d76 100644 --- a/bots/finishingbot/src/main.rs +++ b/bots/finishingbot/src/main.rs @@ -3,6 +3,7 @@ //! finishing-bot CLI - Release Readiness Validator mod analyzers; +mod baseline; mod config; mod error; #[cfg(feature = "fleet")] @@ -12,8 +13,8 @@ use clap::{Parser, Subcommand}; use std::path::{Path, PathBuf}; use std::process::ExitCode; use tracing_subscriber::fmt::format::FmtSpan; -use tracing_subscriber::{fmt, EnvFilter}; use tracing_subscriber::prelude::*; +use tracing_subscriber::{fmt, EnvFilter}; /// finishing-bot: Release Readiness Validator /// @@ -145,39 +146,21 @@ fn main() -> ExitCode { // Handle commands match cli.command { - Some(Command::Init { format }) => { - handle_init(&config_path, &format) - } - Some(Command::Show) => { - handle_show(&config) - } - Some(Command::Audit { strict }) => { - handle_audit(&cli.path, &config, strict, &cli.format) - } - Some(Command::Fix { only }) => { - handle_fix(&cli.path, &config, only.as_deref()) - } - Some(Command::License { list_allowed }) => { - handle_license(&cli.path, &config, list_allowed) - } - Some(Command::Placeholders { action }) => { - handle_placeholders(&cli.path, &config, &action) - } - Some(Command::Claims) => { - handle_claims(&cli.path, &config) - } - Some(Command::Release) => { - handle_release(&cli.path, &config) - } + Some(Command::Init { format }) => handle_init(&config_path, &format), + Some(Command::Show) => handle_show(&config), + Some(Command::Audit { strict }) => handle_audit(&cli.path, &config, strict, &cli.format), + Some(Command::Fix { only }) => handle_fix(&cli.path, &config, only.as_deref()), + Some(Command::License { list_allowed }) => handle_license(&cli.path, &config, list_allowed), + Some(Command::Placeholders { action }) => handle_placeholders(&cli.path, &config, &action), + Some(Command::Claims) => handle_claims(&cli.path, &config), + Some(Command::Release) => handle_release(&cli.path, &config), #[cfg(feature = "fleet")] Some(Command::Fleet { repo, context, session, output, - }) => { - handle_fleet_mode(repo, context, session, output, config) - } + }) => handle_fleet_mode(repo, context, session, output, config), None => { // Default to audit handle_audit(&cli.path, &config, false, &cli.format) @@ -301,7 +284,10 @@ fn handle_show(config: &config::Config) -> ExitCode { println!("Licenses:"); println!(" Allowed: {}", config.licenses.allowed.join(", ")); println!(" Strict: {}", config.licenses.strict); - println!(" Require SPDX headers: {}", config.licenses.require_spdx_headers); + println!( + " Require SPDX headers: {}", + config.licenses.require_spdx_headers + ); println!(); println!("Placeholders:"); @@ -319,7 +305,10 @@ fn handle_show(config: &config::Config) -> ExitCode { println!("Release:"); println!(" Require hashes: {}", config.release.require_hashes); - println!(" Require signatures: {}", config.release.require_signatures); + println!( + " Require signatures: {}", + config.release.require_signatures + ); println!(); println!("General:"); @@ -434,9 +423,7 @@ fn handle_audit(path: &Path, config: &config::Config, strict: bool, format: &str } fn handle_fix(path: &Path, config: &config::Config, only: Option<&str>) -> ExitCode { - use crate::analyzers::{ - license::LicenseAnalyzer, placeholder::PlaceholderAnalyzer, Analyzer, - }; + use crate::analyzers::{license::LicenseAnalyzer, placeholder::PlaceholderAnalyzer, Analyzer}; if only.is_none() || only == Some("license") { let license = LicenseAnalyzer; diff --git a/bots/finishingbot/tests/analyzer_tests.rs b/bots/finishingbot/tests/analyzer_tests.rs index 995274f7..85a20070 100644 --- a/bots/finishingbot/tests/analyzer_tests.rs +++ b/bots/finishingbot/tests/analyzer_tests.rs @@ -137,10 +137,7 @@ mod license_analyzer { .iter() .filter(|f| f.id.starts_with("LIC-002")) .collect(); - assert!( - !spdx_findings.is_empty(), - "Should find missing SPDX header" - ); + assert!(!spdx_findings.is_empty(), "Should find missing SPDX header"); assert_eq!( spdx_findings[0].severity, Severity::Warning, @@ -228,11 +225,7 @@ mod placeholder_analyzer { let file_findings: Vec<_> = result .findings .iter() - .filter(|f| { - f.file - .as_ref() - .is_some_and(|p| p.ends_with("clean.rs")) - }) + .filter(|f| f.file.as_ref().is_some_and(|p| p.ends_with("clean.rs"))) .collect(); assert!( file_findings.is_empty(), @@ -255,21 +248,18 @@ mod placeholder_analyzer { let config = default_config(); let result = analyzer.analyze(&path, &config).unwrap(); - let rs_findings = result.findings.iter().filter(|f| { - f.file - .as_ref() - .is_some_and(|p| p.ends_with("code.rs")) - }); - let md_findings = result.findings.iter().filter(|f| { - f.file - .as_ref() - .is_some_and(|p| p.ends_with("notes.md")) - }); - let toml_findings = result.findings.iter().filter(|f| { - f.file - .as_ref() - .is_some_and(|p| p.ends_with("Cargo.toml")) - }); + let rs_findings = result + .findings + .iter() + .filter(|f| f.file.as_ref().is_some_and(|p| p.ends_with("code.rs"))); + let md_findings = result + .findings + .iter() + .filter(|f| f.file.as_ref().is_some_and(|p| p.ends_with("notes.md"))); + let toml_findings = result + .findings + .iter() + .filter(|f| f.file.as_ref().is_some_and(|p| p.ends_with("Cargo.toml"))); assert!(rs_findings.count() > 0, "Should detect in .rs files"); assert!(md_findings.count() > 0, "Should detect in .md files"); @@ -302,7 +292,11 @@ mod placeholder_analyzer { #[test] fn test_max_allowed_placeholders_no_error() { let (_temp, path) = setup_test_repo(); - fs::write(path.join("code.rs"), "// TODO: one\n// TODO: two\nfn main() {}\n").unwrap(); + fs::write( + path.join("code.rs"), + "// TODO: one\n// TODO: two\nfn main() {}\n", + ) + .unwrap(); let mut config = default_config(); config.placeholders.max_allowed = 5; @@ -456,7 +450,11 @@ mod claims_analyzer { ) .unwrap(); fs::create_dir_all(path.join("tests")).unwrap(); - fs::write(path.join("tests/test_main.rs"), "#[test]\nfn it_works() { assert!(true); }\n").unwrap(); + fs::write( + path.join("tests/test_main.rs"), + "#[test]\nfn it_works() { assert!(true); }\n", + ) + .unwrap(); let mut config = default_config(); config.claims.require_readme = true; @@ -629,24 +627,19 @@ mod scm_files_analyzer { use super::*; #[test] - fn test_missing_state_scm_error() { + fn test_missing_state_scm_is_retired() { let (_temp, path) = setup_test_repo(); - // No .machine_readable/ directory at all + // No .machine_readable/ directory at all: the descriptive documents are + // clauses of the chora deed now, so absence is not a finding. let analyzer = ScmFilesAnalyzer; let config = default_config(); let result = analyzer.analyze(&path, &config).unwrap(); assert!( - result.findings.iter().any(|f| f.id == "SCM-001-STATE.scm"), - "Should report missing STATE.scm" - ); - assert!( - result - .findings - .iter() - .any(|f| f.id == "SCM-001-STATE.scm" && f.severity == Severity::Error), - "Missing STATE.scm should be Error severity" + result.findings.iter().all(|f| !f.id.starts_with("SCM-001")), + "SCM-001 is retired; got {:?}", + result.findings.iter().map(|f| &f.id).collect::>() ); } @@ -669,10 +662,7 @@ mod scm_files_analyzer { let result = analyzer.analyze(&path, &config).unwrap(); assert!( - result - .findings - .iter() - .any(|f| f.id == "SCM-005-STATE.scm"), + result.findings.iter().any(|f| f.id == "SCM-005-STATE.scm"), "Should warn about STATE.scm in root instead of .machine_readable/" ); assert!( @@ -747,10 +737,7 @@ mod scm_files_analyzer { let result = analyzer.analyze(&path, &config).unwrap(); assert!( - result - .findings - .iter() - .any(|f| f.id == "SCM-002-STATE.scm"), + result.findings.iter().any(|f| f.id == "SCM-002-STATE.scm"), "Should warn about missing SPDX header in STATE.scm" ); } @@ -829,8 +816,18 @@ mod testing_analyzer { .unwrap(); fs::create_dir_all(path.join("benches")).unwrap(); fs::write(path.join("benches/bench.rs"), "fn main() {}\n").unwrap(); - fs::create_dir_all(path.join("fuzz")).unwrap(); - fs::write(path.join("fuzz/fuzz_target.rs"), "fn main() {}\n").unwrap(); + // A recognisable cargo-fuzz harness: a placeholder file no longer counts. + fs::create_dir_all(path.join("fuzz/fuzz_targets")).unwrap(); + fs::write( + path.join("fuzz/Cargo.toml"), + "[package]\nname = \"test-fuzz\"\n\n[[bin]]\nname = \"t\"\npath = \"fuzz_targets/t.rs\"\n", + ) + .unwrap(); + fs::write( + path.join("fuzz/fuzz_targets/t.rs"), + "libfuzzer_sys::fuzz_target!(|data: &[u8]| { let _ = data; });\n", + ) + .unwrap(); fs::create_dir_all(path.join(".github/workflows")).unwrap(); fs::write( path.join(".github/workflows/stress-test.yml"), @@ -848,18 +845,13 @@ mod testing_analyzer { .iter() .filter(|f| { f.severity == Severity::Error - && (f.id == "TEST-RU-001" - || f.id == "TEST-RU-003" - || f.id == "TEST-RU-005") + && (f.id == "TEST-RU-001" || f.id == "TEST-RU-003" || f.id == "TEST-RU-005") }) .collect(); assert!( testing_errors.is_empty(), "Well-tested repo should have no testing infrastructure errors, got: {:?}", - testing_errors - .iter() - .map(|f| &f.id) - .collect::>() + testing_errors.iter().map(|f| &f.id).collect::>() ); } } @@ -1097,11 +1089,7 @@ mod integration { let (_temp, path) = setup_test_repo(); // Create a fairly complete repo - fs::write( - path.join("LICENSE"), - "Palimpsest License\nMPL-2.0\n", - ) - .unwrap(); + fs::write(path.join("LICENSE"), "Palimpsest License\nMPL-2.0\n").unwrap(); let readme = "# My Project\n\n\ Description of the project with enough words.\n\n\ ## Installation\n\nRun the install command.\n\n\ @@ -1127,18 +1115,10 @@ mod integration { let config = default_config(); - let license = LicenseAnalyzer - .analyze(&path, &config) - .unwrap(); - let placeholder = PlaceholderAnalyzer - .analyze(&path, &config) - .unwrap(); - let claims = ClaimsAnalyzer - .analyze(&path, &config) - .unwrap(); - let release = ReleaseAnalyzer - .analyze(&path, &config) - .unwrap(); + let license = LicenseAnalyzer.analyze(&path, &config).unwrap(); + let placeholder = PlaceholderAnalyzer.analyze(&path, &config).unwrap(); + let claims = ClaimsAnalyzer.analyze(&path, &config).unwrap(); + let release = ReleaseAnalyzer.analyze(&path, &config).unwrap(); let result = AuditResult { license, @@ -1169,10 +1149,7 @@ mod integration { // Verify Finding serializes to JSON correctly let json = serde_json::to_string(&finding).expect("Finding should serialize to JSON"); assert!(json.contains("LIC-001"), "JSON should contain finding ID"); - assert!( - json.contains("error"), - "JSON should contain severity level" - ); + assert!(json.contains("error"), "JSON should contain severity level"); // Verify round-trip let deserialized: finishing_bot::Finding = @@ -1192,12 +1169,15 @@ mod integration { Severity::Error, "No LICENSE file found", )); - result.placeholder.findings.push(finishing_bot::Finding::new( - "PH-001", - "Placeholder: TODO", - Severity::Warning, - "Found TODO placeholder", - )); + result + .placeholder + .findings + .push(finishing_bot::Finding::new( + "PH-001", + "Placeholder: TODO", + Severity::Warning, + "Found TODO placeholder", + )); assert_eq!(result.total_findings(), 2); assert_eq!(result.total_errors(), 1); diff --git a/bots/glambot/README.adoc b/bots/glambot/README.adoc index 626b3a31..bf289555 100644 --- a/bots/glambot/README.adoc +++ b/bots/glambot/README.adoc @@ -90,22 +90,22 @@ Evaluates repository discoverability through README descriptions, HTML meta tags === Machine-Readability (`MachineAnalyzer`) -Validates JSON/YAML file syntax, structured data presence, robots.txt, and `.machine_readable/` directory with SCM files. +Validates JSON/YAML file syntax, structured data presence, robots.txt, and the `.machine_readable/` directory. Legacy SCM files there are validated only if present; their absence is canonical (the descriptiles are clauses of `_chora.deed`). [cols="1,2,1"] |=== | Finding ID | Description | Severity -| MACH-001 | Invalid JSON | Error +| MACH-001 | Invalid JSON | Error (`.devcontainer/*.json` and `.vscode/*.json` are tool-demanded JSONC and are parsed as JSONC) | MACH-002 | Invalid YAML | Error | MACH-003 | Missing structured data (HTML) | Warning | MACH-004 | Invalid JSON-LD | Error | MACH-005 | Missing robots.txt | Info | MACH-006 | Missing .machine_readable/ directory | Warning -| MACH-007 | Missing SCM file (STATE/META/ECOSYSTEM) | Info +| MACH-007 | _Retired_: the STATE/META/ECOSYSTEM `.scm` files are clauses of the repository's `_chora.deed`, so their absence is not reported | n/a |=== -**Auto-fixes:** Creates `.machine_readable/` directory with STATE.scm, META.scm, ECOSYSTEM.scm templates (MACH-006), adds schema.org JSON-LD template (MACH-003), creates robots.txt (MACH-005). +**Auto-fixes:** Adds schema.org JSON-LD template (MACH-003), creates robots.txt (MACH-005). === Git-SEO Integration (`GitSeoAnalyzer`) diff --git a/bots/glambot/src/analyzers/accessibility.rs b/bots/glambot/src/analyzers/accessibility.rs index 29c1e1f6..c39c9de5 100644 --- a/bots/glambot/src/analyzers/accessibility.rs +++ b/bots/glambot/src/analyzers/accessibility.rs @@ -5,18 +5,21 @@ use super::{AnalysisResult, Analyzer, Finding, Severity}; use crate::config::Config; use crate::error::Result; +use regex::Regex; use std::path::Path; -use walkdir::WalkDir; use std::sync::LazyLock; -use regex::Regex; +use walkdir::WalkDir; // Compiled once. These were rebuilt on every call (hypatia expect_in_hot_path). -static MD_IMAGE_RE: LazyLock = - LazyLock::new(|| Regex::new(r"!\[(.*?)\]\((.*?)\)").expect("static markdown image regex literal is valid")); -static ADOC_IMAGE_RE: LazyLock = - LazyLock::new(|| Regex::new(r"image::(.*?)\[(.*?)\]").expect("static asciidoc image regex literal is valid")); -static MD_LINK_RE: LazyLock = - LazyLock::new(|| Regex::new(r"\[(.*?)\]\((.*?)\)").expect("static markdown link regex literal is valid")); +static MD_IMAGE_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"!\[(.*?)\]\((.*?)\)").expect("static markdown image regex literal is valid") +}); +static ADOC_IMAGE_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"image::(.*?)\[(.*?)\]").expect("static asciidoc image regex literal is valid") +}); +static MD_LINK_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"\[(.*?)\]\((.*?)\)").expect("static markdown link regex literal is valid") +}); /// Accessibility analyzer pub struct AccessibilityAnalyzer; @@ -64,8 +67,7 @@ impl Analyzer for AccessibilityAnalyzer { // Add alt text placeholders to images in markdown files if let Some(ref file) = finding.file { if let Ok(content) = std::fs::read_to_string(file) { - let updated = content - .replace("![](", "![TODO: Add alt text]("); + let updated = content.replace("![](", "![TODO: Add alt text]("); if updated != content { std::fs::write(file, &updated)?; fixes.push(format!( @@ -131,7 +133,6 @@ impl Analyzer for AccessibilityAnalyzer { impl AccessibilityAnalyzer { fn check_image_alt_text(&self, path: &Path, config: &Config, result: &mut AnalysisResult) { - let md_image_re = &*MD_IMAGE_RE; let adoc_image_re = &*ADOC_IMAGE_RE; @@ -162,7 +163,9 @@ impl AccessibilityAnalyzer { ) .with_file(entry.path().to_path_buf()) .with_line(line_num + 1) - .with_suggestion("Add descriptive alt text: ![Description](image.png)"), + .with_suggestion( + "Add descriptive alt text: ![Description](image.png)", + ), ); } else if alt_text.len() < 3 { result.add( @@ -192,7 +195,9 @@ impl AccessibilityAnalyzer { ) .with_file(entry.path().to_path_buf()) .with_line(line_num + 1) - .with_suggestion("Add descriptive alt text: image::file[Description]"), + .with_suggestion( + "Add descriptive alt text: image::file[Description]", + ), ); } } @@ -246,7 +251,6 @@ impl AccessibilityAnalyzer { } fn check_link_text(&self, path: &Path, config: &Config, result: &mut AnalysisResult) { - let md_link_re = &*MD_LINK_RE; let non_descriptive = ["click here", "here", "read more", "link", "more"]; @@ -327,11 +331,7 @@ mod tests { let result = analyzer.analyze(dir.path(), &config).unwrap(); assert!(result.has_errors(), "Missing alt text should be an error"); - let error_ids: Vec<&str> = result - .errors() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let error_ids: Vec<&str> = result.errors().iter().map(|f| f.id.as_str()).collect(); assert!( error_ids.contains(&"ACC-001"), "Should report ACC-001 for missing alt text" @@ -349,11 +349,7 @@ mod tests { let config = default_config(); let result = analyzer.analyze(dir.path(), &config).unwrap(); - let warning_ids: Vec<&str> = result - .warnings() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let warning_ids: Vec<&str> = result.warnings().iter().map(|f| f.id.as_str()).collect(); assert!( warning_ids.contains(&"ACC-003"), "Should report ACC-003 for skipped heading level" @@ -370,11 +366,7 @@ mod tests { let config = default_config(); let result = analyzer.analyze(dir.path(), &config).unwrap(); - let warning_ids: Vec<&str> = result - .warnings() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let warning_ids: Vec<&str> = result.warnings().iter().map(|f| f.id.as_str()).collect(); assert!( warning_ids.contains(&"ACC-004"), "Should report ACC-004 for non-descriptive link text" @@ -398,7 +390,9 @@ mod tests { let fixes = analyzer.fix(dir.path(), &config, &findings).unwrap(); assert!( - fixes.iter().any(|f| f.contains("WCAG compliance checklist")), + fixes + .iter() + .any(|f| f.contains("WCAG compliance checklist")), "Should generate WCAG checklist" ); assert!( diff --git a/bots/glambot/src/analyzers/git_seo_integration.rs b/bots/glambot/src/analyzers/git_seo_integration.rs index ed66117c..c59f93b2 100644 --- a/bots/glambot/src/analyzers/git_seo_integration.rs +++ b/bots/glambot/src/analyzers/git_seo_integration.rs @@ -38,6 +38,7 @@ impl GitSeoAnalyzer { // logged loudly instead; install hint: https://github.com/hyperpolymath/git-seo if !git_seo_installed { warn!("git-seo is not installed; the Git-SEO analyzer did not run (no findings reported, not a clean result)"); + result.skipped = Some("git-seo is not installed".to_string()); result.duration_ms = start.elapsed().as_millis() as u64; return Ok(result); } @@ -142,10 +143,7 @@ impl Analyzer for GitSeoAnalyzer { if !funding_path.exists() { let content = "# SPDX-License-Identifier: MPL-2.0\n# Funding information\n# github: hyperpolymath\n"; std::fs::write(&funding_path, content)?; - fixes.push(format!( - "Created FUNDING.yml at {}", - funding_path.display() - )); + fixes.push(format!("Created FUNDING.yml at {}", funding_path.display())); } // If git-seo is installed, also run the tool @@ -245,12 +243,8 @@ fn run_git_seo_apply(repo_url: &str) -> Result { /// Process git-seo JSON report into glambot findings fn process_seo_report(report: &serde_json::Value, result: &mut AnalysisResult) { // Extract overall score - let score = report["scores"]["total"] - .as_f64() - .unwrap_or(0.0); - let percentage = report["scores"]["percentage"] - .as_f64() - .unwrap_or(0.0); + let score = report["scores"]["total"].as_f64().unwrap_or(0.0); + let percentage = report["scores"]["percentage"].as_f64().unwrap_or(0.0); // Create finding based on score let severity = if percentage >= 80.0 { @@ -346,7 +340,11 @@ mod tests { let result = GitSeoAnalyzer::default() .analyze_with(dir.path(), &default_config(), true) .unwrap(); - let gs003: Vec<_> = result.findings.iter().filter(|f| f.id == "GS-003").collect(); + let gs003: Vec<_> = result + .findings + .iter() + .filter(|f| f.id == "GS-003") + .collect(); assert_eq!(gs003.len(), 1, "a repo with no remote reports GS-003"); assert_eq!( gs003[0].file.as_deref(), @@ -364,8 +362,32 @@ mod tests { assert!( result.findings.is_empty(), "tool-availability advice is not a repo finding, got: {:?}", - result.findings.iter().map(|f| f.id.as_str()).collect::>() + result + .findings + .iter() + .map(|f| f.id.as_str()) + .collect::>() + ); + assert_eq!( + result.skipped.as_deref(), + Some("git-seo is not installed"), + "an unavailable git-seo is reported as skipped, not as a clean scan" ); + let json = serde_json::to_value(&result).unwrap(); + assert_eq!(json["skipped"], "git-seo is not installed"); + } + + #[test] + fn a_run_is_not_a_skip() { + let dir = git_repo_without_remote(); + let result = GitSeoAnalyzer + .analyze_with(dir.path(), &default_config(), true) + .unwrap(); + assert!(result.skipped.is_none()); + assert!(serde_json::to_value(&result) + .unwrap() + .get("skipped") + .is_none()); } #[test] @@ -383,10 +405,7 @@ mod tests { process_seo_report(&report, &mut result); // High score should produce Info severity, not Error - assert!( - !result.has_errors(), - "High score should not produce errors" - ); + assert!(!result.has_errors(), "High score should not produce errors"); let finding = &result.findings[0]; assert_eq!(finding.severity, Severity::Info); } diff --git a/bots/glambot/src/analyzers/machine.rs b/bots/glambot/src/analyzers/machine.rs index 487eb451..ed51a38d 100644 --- a/bots/glambot/src/analyzers/machine.rs +++ b/bots/glambot/src/analyzers/machine.rs @@ -57,87 +57,6 @@ impl Analyzer for MachineAnalyzer { for finding in findings { match finding.id.as_str() { - "MACH-006" => { - // Create missing .machine_readable/ directory structure - let mr_dir = path.join(".machine_readable"); - if !mr_dir.exists() { - std::fs::create_dir_all(&mr_dir)?; - fixes.push(format!( - "Created .machine_readable/ directory at {}", - mr_dir.display() - )); - } - - // Generate template SCM files - let repo_name = path - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or("project"); - - let state_path = mr_dir.join("STATE.scm"); - if !state_path.exists() { - let state_content = format!( - ";; SPDX-License-Identifier: MPL-2.0\n\ - ;; STATE.scm - Project state for {name}\n\n\ - (state\n\ - \x20 (metadata\n\ - \x20 (version \"0.1.0\")\n\ - \x20 (schema-version \"1.0\")\n\ - \x20 (created \"TODO\")\n\ - \x20 (updated \"TODO\")\n\ - \x20 (project \"{name}\")\n\ - \x20 (repo \"hyperpolymath/{name}\"))\n\n\ - \x20 (project-context\n\ - \x20 (name \"{name}\")\n\ - \x20 (tagline \"TODO: Add tagline\")\n\ - \x20 (tech-stack (\"TODO\"))))\n", - name = repo_name - ); - std::fs::write(&state_path, state_content)?; - fixes.push(format!( - "Generated STATE.scm template at {}", - state_path.display() - )); - } - - let meta_path = mr_dir.join("META.scm"); - if !meta_path.exists() { - let meta_content = format!( - ";; SPDX-License-Identifier: MPL-2.0\n\ - ;; META.scm - Meta information for {name}\n\n\ - (meta\n\ - \x20 (architecture-decisions ())\n\ - \x20 (development-practices ())\n\ - \x20 (design-rationale ()))\n", - name = repo_name - ); - std::fs::write(&meta_path, meta_content)?; - fixes.push(format!( - "Generated META.scm template at {}", - meta_path.display() - )); - } - - let ecosystem_path = mr_dir.join("ECOSYSTEM.scm"); - if !ecosystem_path.exists() { - let eco_content = format!( - ";; SPDX-License-Identifier: MPL-2.0\n\ - ;; ECOSYSTEM.scm - Ecosystem context for {name}\n\n\ - (ecosystem\n\ - \x20 (version \"1.0\")\n\ - \x20 (name \"{name}\")\n\ - \x20 (type \"application\")\n\ - \x20 (purpose \"TODO\")\n\ - \x20 (related-projects ()))\n", - name = repo_name - ); - std::fs::write(&ecosystem_path, eco_content)?; - fixes.push(format!( - "Generated ECOSYSTEM.scm template at {}", - ecosystem_path.display() - )); - } - } "MACH-003" => { // Add structured data (schema.org JSON-LD) template if let Some(ref file) = finding.file { @@ -170,10 +89,7 @@ impl Analyzer for MachineAnalyzer { let content = "# robots.txt for repository documentation\nUser-agent: *\nAllow: /\n\nSitemap: TODO\n"; std::fs::write(&robots_path, content)?; - fixes.push(format!( - "Created robots.txt at {}", - robots_path.display() - )); + fixes.push(format!("Created robots.txt at {}", robots_path.display())); } } _ => {} @@ -202,7 +118,15 @@ impl MachineAnalyzer { result.files_checked += 1; if let Ok(content) = std::fs::read_to_string(entry.path()) { - if let Err(e) = serde_json::from_str::(&content) { + // .devcontainer/*.json and .vscode/*.json are JSONC (comments and + // trailing commas) because the tools that read them demand it; the + // estate JSON policy carves them out. Parse them as JSONC. + let parsed = if is_tool_demanded_jsonc(path, entry.path()) { + serde_json::from_str::(&strip_jsonc(&content)) + } else { + serde_json::from_str::(&content) + }; + if let Err(e) = parsed { result.add( Finding::new( "MACH-001", @@ -268,7 +192,8 @@ impl MachineAnalyzer { if let Ok(content) = std::fs::read_to_string(entry.path()) { let has_json_ld = content.contains("application/ld+json"); - let has_microdata = content.contains("itemscope") || content.contains("itemprop"); + let has_microdata = + content.contains("itemscope") || content.contains("itemprop"); let has_schema_org = content.contains("schema.org"); if !has_json_ld && !has_microdata && config.machine.require_schema_org { @@ -280,15 +205,20 @@ impl MachineAnalyzer { "HTML file has no structured data (JSON-LD or microdata).", ) .with_file(entry.path().to_path_buf()) - .with_suggestion("Add schema.org structured data for better machine readability"), + .with_suggestion( + "Add schema.org structured data for better machine readability", + ), ); } if has_json_ld { // Try to validate JSON-LD - if let Some(json_ld_start) = content.find("") { - let json_ld = &content[json_ld_start + 36..json_ld_start + json_ld_end]; + let json_ld = + &content[json_ld_start + 36..json_ld_start + json_ld_end]; if serde_json::from_str::(json_ld).is_err() { result.add( Finding::new( @@ -342,37 +272,123 @@ impl MachineAnalyzer { "MACH-006", "Missing .machine_readable/ directory", Severity::Warning, - "No .machine_readable/ directory found. SCM files (STATE.scm, META.scm, ECOSYSTEM.scm) should be placed here.", + "No .machine_readable/ directory found (contractiles and other machine-readable estate metadata live here).", ) - .fixable() .with_file(mr_dir.clone()) - .with_suggestion("Create .machine_readable/ directory with STATE.scm, META.scm, ECOSYSTEM.scm"), + // Not auto-fixable: git cannot track an empty directory, and a + // placeholder file would be invented content. What belongs here + // (contractiles, the chora deed) is owner-authored. + .with_suggestion("Create the .machine_readable/ directory"), ); return; } result.files_checked += 1; - // Check for required SCM files - let required_scm = ["STATE.scm", "META.scm", "ECOSYSTEM.scm"]; - for scm_file in &required_scm { - // Check both direct path and 6scm subdirectory - let direct_path = mr_dir.join(scm_file); - let sub_path = mr_dir.join("6scm").join(scm_file); - if !direct_path.exists() && !sub_path.exists() { - result.add( - Finding::new( - "MACH-007", - &format!("Missing {}", scm_file), - Severity::Info, - &format!("{} not found in .machine_readable/ directory.", scm_file), - ) - .with_file(mr_dir.join(scm_file)) - .with_suggestion(&format!("Add {} to .machine_readable/", scm_file)), - ); + // MACH-007 (missing STATE/META/ECOSYSTEM .scm) is retired: those + // documents are clauses of the repository's `_chora.deed` now, + // not files under .machine_readable/, so their absence is canonical. + } +} + +/// True for JSON files whose consumer demands JSONC: anything under a +/// `.devcontainer/` or `.vscode/` directory of the repository. Only the +/// repo-relative components count: a checkout that itself lives under a +/// `.vscode/` or `.devcontainer/` ancestor does not make every JSON JSONC. +fn is_tool_demanded_jsonc(root: &Path, path: &Path) -> bool { + let Ok(rel) = path.strip_prefix(root) else { + return false; + }; + rel.parent().is_some_and(|dir| { + dir.components() + .any(|c| c.as_os_str() == ".devcontainer" || c.as_os_str() == ".vscode") + }) +} + +/// Reduce JSONC to JSON: drop a BOM, `//` and `/* */` comments and trailing +/// commas, leaving string contents untouched. +fn strip_jsonc(src: &str) -> String { + let src = src.strip_prefix('\u{feff}').unwrap_or(src); + let chars: Vec = src.chars().collect(); + let mut out = String::with_capacity(src.len()); + let mut i = 0; + while i < chars.len() { + let c = chars[i]; + if c == '"' { + out.push(c); + i += 1; + while i < chars.len() { + out.push(chars[i]); + if chars[i] == '\\' && i + 1 < chars.len() { + out.push(chars[i + 1]); + i += 2; + continue; + } + if chars[i] == '"' { + i += 1; + break; + } + i += 1; + } + } else if c == '/' && chars.get(i + 1) == Some(&'/') { + while i < chars.len() && chars[i] != '\n' { + i += 1; + } + } else if c == '/' && chars.get(i + 1) == Some(&'*') { + let start = i; + i += 2; + while i + 1 < chars.len() && !(chars[i] == '*' && chars[i + 1] == '/') { + i += 1; } + if i + 1 < chars.len() { + // A comment is whitespace, not nothing: `1/*c*/2` must not + // fuse into `12`. Newlines inside it are kept (line numbers). + out.push(' '); + out.extend(chars[start..i].iter().filter(|c| **c == '\n')); + i += 2; + } else { + // Unterminated block comment: keep the text so parsing fails. + out.extend(&chars[start..]); + i = chars.len(); + } + } else { + out.push(c); + i += 1; } } + // Trailing commas: a comma whose next non-space char closes a container. + let b: Vec = out.chars().collect(); + let mut res = String::with_capacity(b.len()); + let mut in_str = false; + let mut j = 0; + while j < b.len() { + let c = b[j]; + if in_str { + res.push(c); + if c == '\\' && j + 1 < b.len() { + res.push(b[j + 1]); + j += 1; + } else if c == '"' { + in_str = false; + } + } else if c == '"' { + in_str = true; + res.push(c); + } else if c == ',' { + let next = b[j + 1..].iter().find(|x| !x.is_whitespace()); + // Only a comma after a completed member or element is trailing; + // `{,}`, `[,]` and `,,` stay so the parse fails. + let prev = res.chars().rev().find(|x| !x.is_whitespace()); + let after_value = !matches!(prev, None | Some('{' | '[' | ',' | ':')); + if !(after_value && matches!(next, Some('}') | Some(']'))) { + res.push(c); + } + } else { + res.push(c); + } + j += 1; + } + res } #[cfg(test)] @@ -430,11 +446,7 @@ mod tests { let result = analyzer.analyze(dir.path(), &config).unwrap(); assert!(result.has_errors(), "Invalid JSON should produce errors"); - let error_ids: Vec<&str> = result - .errors() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let error_ids: Vec<&str> = result.errors().iter().map(|f| f.id.as_str()).collect(); assert!( error_ids.contains(&"MACH-001"), "Should report MACH-001 for invalid JSON" @@ -450,11 +462,7 @@ mod tests { let config = default_config(); let result = analyzer.analyze(dir.path(), &config).unwrap(); - let ids: Vec<&str> = result - .findings - .iter() - .map(|f| f.id.as_str()) - .collect(); + let ids: Vec<&str> = result.findings.iter().map(|f| f.id.as_str()).collect(); assert!( ids.contains(&"MACH-006"), "Should report MACH-006 for missing .machine_readable/" @@ -462,40 +470,163 @@ mod tests { } #[test] - fn test_fix_creates_machine_readable_structure() { + fn mach_006_is_not_auto_fixable_and_fix_creates_nothing() { let dir = TempDir::new().unwrap(); - let analyzer = MachineAnalyzer::default(); let config = default_config(); + let result = analyzer.analyze(dir.path(), &config).unwrap(); + let f = result.findings.iter().find(|f| f.id == "MACH-006").unwrap(); + assert!(!f.fixable, "an empty dir is untracked by git; not fixable"); + let findings = vec![f.clone()]; + let fixes = analyzer.fix(dir.path(), &config, &findings).unwrap(); + assert_eq!( + fixes, + vec!["No machine-readability fixes applicable".to_string()] + ); + assert!(!dir.path().join(".machine_readable").exists()); + } - let findings = vec![Finding::new( - "MACH-006", - "Missing .machine_readable/", - Severity::Warning, - "No .machine_readable/ directory found", - )]; + fn ids(dir: &Path) -> Vec { + MachineAnalyzer::default() + .analyze(dir, &default_config()) + .unwrap() + .findings + .into_iter() + .map(|f| f.id) + .collect() + } - let fixes = analyzer.fix(dir.path(), &config, &findings).unwrap(); + #[test] + fn mach_007_is_retired() { + let dir = TempDir::new().unwrap(); + std::fs::create_dir_all(dir.path().join(".machine_readable")).unwrap(); + let got = ids(dir.path()); + assert!(!got.contains(&"MACH-007".to_string()), "{got:?}"); + assert!(!got.contains(&"MACH-006".to_string()), "{got:?}"); + // The directory itself is still expected. + let empty = TempDir::new().unwrap(); + assert!(ids(empty.path()).contains(&"MACH-006".to_string())); + } + #[test] + fn mach_006_is_not_auto_fixable() { + // The README documents the auto-fix list; MACH-006 must stay off it. + let dir = TempDir::new().unwrap(); + let a = MachineAnalyzer::default(); + let findings = a.analyze(dir.path(), &default_config()).unwrap().findings; + assert!(findings.iter().any(|f| f.id == "MACH-006")); + a.fix(dir.path(), &default_config(), &findings).unwrap(); + assert!(!dir.path().join(".machine_readable").exists()); + } + + const JSONC: &str = + "{\n // a comment\n \"url\": \"http://x//y\", /* inline */\n \"list\": [1, 2,],\n}\n"; + + #[test] + fn devcontainer_and_vscode_json_are_parsed_as_jsonc() { + for rel in [ + ".devcontainer/devcontainer.json", + ".vscode/settings.json", + ".devcontainer/sub/devcontainer.json", + ] { + let dir = TempDir::new().unwrap(); + let p = dir.path().join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(&p, JSONC).unwrap(); + assert!(!ids(dir.path()).contains(&"MACH-001".to_string()), "{rel}"); + } + } + + #[test] + fn the_same_text_elsewhere_is_still_invalid_json() { + let dir = TempDir::new().unwrap(); + std::fs::write(dir.path().join("config.json"), JSONC).unwrap(); + assert!(ids(dir.path()).contains(&"MACH-001".to_string())); + } + + #[test] + fn genuinely_broken_jsonc_is_still_reported() { + let dir = TempDir::new().unwrap(); + let p = dir.path().join(".vscode/settings.json"); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(&p, "{ \"a\": }").unwrap(); + assert!(ids(dir.path()).contains(&"MACH-001".to_string())); + } + + #[test] + fn unterminated_block_comment_is_invalid() { + let dir = TempDir::new().unwrap(); + let p = dir.path().join(".vscode/settings.json"); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(&p, "{\"a\": 1} /*").unwrap(); + assert!(ids(dir.path()).contains(&"MACH-001".to_string())); + std::fs::write(&p, "{\"a\": 1} /* ok */").unwrap(); + assert!(!ids(dir.path()).contains(&"MACH-001".to_string())); + } + + #[test] + fn empty_containers_with_a_lone_comma_are_invalid() { + for body in ["{,}", "[,]", "{ , }", "[1,,]", "{\"a\":1,,}", "{\"a\":,}"] { + let dir = TempDir::new().unwrap(); + let p = dir.path().join(".vscode/settings.json"); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(&p, body).unwrap(); + assert!(ids(dir.path()).contains(&"MACH-001".to_string()), "{body}"); + } + } + + #[test] + fn valid_trailing_commas_still_work() { + for body in [ + "{\"a\":1,}", + "[1,2,]", + "{\"a\":[1,],}", + "{\"a\":1 , \n}", + "[\"x\", /* c */ ]", + ] { + let v: std::result::Result = + serde_json::from_str(&strip_jsonc(body)); + assert!(v.is_ok(), "{body}"); + } + } + + #[test] + fn jsonc_detection_ignores_ancestors_above_the_repo_root() { + let dir = TempDir::new().unwrap(); + let root = dir.path().join(".vscode/checkout"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::write(root.join("config.json"), JSONC).unwrap(); + assert!(ids(&root).contains(&"MACH-001".to_string())); + assert!(!is_tool_demanded_jsonc(&root, &root.join("config.json"))); + assert!(is_tool_demanded_jsonc( + &root, + &root.join(".vscode/settings.json") + )); + } + + #[test] + fn strip_jsonc_leaves_string_contents_alone() { + let v: serde_json::Value = serde_json::from_str(&strip_jsonc(JSONC)).unwrap(); + assert_eq!(v["url"], "http://x//y"); + assert_eq!(v["list"], serde_json::json!([1, 2])); + } + + #[test] + fn a_block_comment_is_whitespace_so_it_cannot_fuse_tokens() { + // `1/*c*/2` must not become the valid number 12. + let stripped = strip_jsonc(r#"{"value": 1/*c*/2}"#); assert!( - fixes.iter().any(|f| f.contains(".machine_readable")), - "Should create .machine_readable directory" - ); - assert!( - dir.path().join(".machine_readable").exists(), - ".machine_readable/ should exist after fix" - ); - assert!( - dir.path().join(".machine_readable").join("STATE.scm").exists(), - "STATE.scm should exist after fix" - ); - assert!( - dir.path().join(".machine_readable").join("META.scm").exists(), - "META.scm should exist after fix" - ); - assert!( - dir.path().join(".machine_readable").join("ECOSYSTEM.scm").exists(), - "ECOSYSTEM.scm should exist after fix" + serde_json::from_str::(&stripped).is_err(), + "fused into valid JSON: {stripped}" ); + // A comment between tokens that need no separator still parses. + let ok = strip_jsonc("{\"a\": 1, /* c */ \"b\": 2}"); + assert!(serde_json::from_str::(&ok).is_ok()); + } + + #[test] + fn a_multiline_block_comment_keeps_its_newlines() { + let stripped = strip_jsonc("{\"a\":\n/* one\ntwo\nthree */ 1}"); + assert_eq!(stripped.matches('\n').count(), 3, "{stripped:?}"); } } diff --git a/bots/glambot/src/analyzers/mod.rs b/bots/glambot/src/analyzers/mod.rs index 129bdeb7..0c346a40 100644 --- a/bots/glambot/src/analyzers/mod.rs +++ b/bots/glambot/src/analyzers/mod.rs @@ -2,11 +2,11 @@ // SPDX-FileCopyrightText: 2025 Jonathan D.A. Jewell //! Analyzers for presentation quality validation -pub mod visual; pub mod accessibility; -pub mod seo; -pub mod machine; pub mod git_seo_integration; +pub mod machine; +pub mod seo; +pub mod visual; use crate::config::Config; use crate::error::Result; @@ -107,6 +107,11 @@ pub struct AnalysisResult { pub files_checked: usize, /// Duration in milliseconds pub duration_ms: u64, + /// Why the analyzer did not run (tool missing or failed). `None` means it ran. + /// A skipped analyzer has no findings, which is NOT a clean scan: consumers + /// (scripts/fleet-report-back.sh) must read this before trusting an empty list. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub skipped: Option, } impl AnalysisResult { @@ -241,12 +246,20 @@ mod scope_tests { findings.extend(a.analyze(dir.path(), &cfg).unwrap().findings); } // Negative control: the empty repo really does produce repo-level findings. - assert!(findings.len() >= 3, "got {:?}", findings.iter().map(|f| &f.id).collect::>()); + assert!( + findings.len() >= 3, + "got {:?}", + findings.iter().map(|f| &f.id).collect::>() + ); let unpathed: Vec<&str> = findings .iter() .filter(|f| f.file.is_none()) .map(|f| f.id.as_str()) .collect(); - assert!(unpathed.is_empty(), "findings without a path: {:?}", unpathed); + assert!( + unpathed.is_empty(), + "findings without a path: {:?}", + unpathed + ); } } diff --git a/bots/glambot/src/analyzers/seo.rs b/bots/glambot/src/analyzers/seo.rs index cf96ed92..9faffa9b 100644 --- a/bots/glambot/src/analyzers/seo.rs +++ b/bots/glambot/src/analyzers/seo.rs @@ -149,9 +149,7 @@ impl Analyzer for SeoAnalyzer { impl SeoAnalyzer { fn check_repo_description(&self, path: &Path, result: &mut AnalysisResult) { let readme_files = ["README.md", "README.adoc"]; - let found = readme_files - .iter() - .find(|name| path.join(name).exists()); + let found = readme_files.iter().find(|name| path.join(name).exists()); if let Some(readme) = found { let readme_path = path.join(readme); @@ -202,7 +200,9 @@ impl SeoAnalyzer { "HTML file missing meta description tag.", ) .with_file(entry.path().to_path_buf()) - .with_suggestion("Add in "), + .with_suggestion( + "Add in ", + ), ); } @@ -215,7 +215,9 @@ impl SeoAnalyzer { "HTML file missing meta keywords tag.", ) .with_file(entry.path().to_path_buf()) - .with_suggestion("Add in "), + .with_suggestion( + "Add in ", + ), ); } } @@ -270,9 +272,7 @@ impl SeoAnalyzer { fn check_keywords(&self, path: &Path, result: &mut AnalysisResult) { let readme_files = ["README.md", "README.adoc"]; - let found = readme_files - .iter() - .find(|name| path.join(name).exists()); + let found = readme_files.iter().find(|name| path.join(name).exists()); if let Some(readme) = found { let readme_path = path.join(readme); @@ -358,11 +358,7 @@ mod tests { let result = analyzer.analyze(dir.path(), &config).unwrap(); // Should find missing description (SEO-001) - let ids: Vec<&str> = result - .findings - .iter() - .map(|f| f.id.as_str()) - .collect(); + let ids: Vec<&str> = result.findings.iter().map(|f| f.id.as_str()).collect(); assert!( ids.contains(&"SEO-001"), "Should report SEO-001 for short description" @@ -381,11 +377,7 @@ mod tests { let analyzer = SeoAnalyzer::default(); let result = analyzer.analyze(dir.path(), &config).unwrap(); - let ids: Vec<&str> = result - .findings - .iter() - .map(|f| f.id.as_str()) - .collect(); + let ids: Vec<&str> = result.findings.iter().map(|f| f.id.as_str()).collect(); assert!( ids.contains(&"SEO-002"), "Should report SEO-002 for missing meta description" diff --git a/bots/glambot/src/analyzers/visual.rs b/bots/glambot/src/analyzers/visual.rs index e8768087..50f2a5e9 100644 --- a/bots/glambot/src/analyzers/visual.rs +++ b/bots/glambot/src/analyzers/visual.rs @@ -67,7 +67,10 @@ impl Analyzer for VisualAnalyzer { repo_name ); std::fs::write(&readme_path, template)?; - fixes.push(format!("Created README.adoc template at {}", readme_path.display())); + fixes.push(format!( + "Created README.adoc template at {}", + readme_path.display() + )); } } "VIS-004" => { @@ -85,12 +88,20 @@ impl Analyzer for VisualAnalyzer { }; // Insert badges after the first heading let updated = if let Some(pos) = content.find('\n') { - format!("{}{}{}", &content[..pos + 1], badge_section, &content[pos + 1..]) + format!( + "{}{}{}", + &content[..pos + 1], + badge_section, + &content[pos + 1..] + ) } else { format!("{}{}", content, badge_section) }; std::fs::write(&readme_path, updated)?; - fixes.push(format!("Added badges section to {}", readme_path.display())); + fixes.push(format!( + "Added badges section to {}", + readme_path.display() + )); } } break; @@ -107,7 +118,10 @@ impl Analyzer for VisualAnalyzer { if !placeholder_path.exists() { let content = "# Screenshots\n\nTODO: Add screenshots or visual documentation here.\n\n## UI Preview\n\n\n"; std::fs::write(&placeholder_path, content)?; - fixes.push(format!("Created screenshot placeholder at {}", placeholder_path.display())); + fixes.push(format!( + "Created screenshot placeholder at {}", + placeholder_path.display() + )); } } _ => {} @@ -125,9 +139,7 @@ impl Analyzer for VisualAnalyzer { impl VisualAnalyzer { fn check_readme(&self, path: &Path, _config: &Config, result: &mut AnalysisResult) { let readme_files = ["README.md", "README.adoc", "README.rst", "README.txt"]; - let found = readme_files - .iter() - .find(|name| path.join(name).exists()); + let found = readme_files.iter().find(|name| path.join(name).exists()); match found { None => { @@ -155,17 +167,22 @@ impl VisualAnalyzer { "VIS-002", "README too short", Severity::Warning, - &format!("README is only {} characters. Should be more descriptive.", content.len()), + &format!( + "README is only {} characters. Should be more descriptive.", + content.len() + ), ) .with_file(readme_path.clone()) - .with_suggestion("Expand README with project description, usage, installation"), + .with_suggestion( + "Expand README with project description, usage, installation", + ), ); } // Check for title/heading - let has_title = content.lines().any(|line| { - line.starts_with("# ") || line.starts_with("= ") - }); + let has_title = content + .lines() + .any(|line| line.starts_with("# ") || line.starts_with("= ")); if !has_title { result.add( @@ -186,9 +203,7 @@ impl VisualAnalyzer { fn check_badges(&self, path: &Path, result: &mut AnalysisResult) { let readme_files = ["README.md", "README.adoc"]; - let found = readme_files - .iter() - .find(|name| path.join(name).exists()); + let found = readme_files.iter().find(|name| path.join(name).exists()); if let Some(readme) = found { let readme_path = path.join(readme); @@ -204,7 +219,9 @@ impl VisualAnalyzer { "README does not contain any badges (build status, coverage, etc.).", ) .with_file(readme_path) - .with_suggestion("Add badges for build status, test coverage, license, etc."), + .with_suggestion( + "Add badges for build status, test coverage, license, etc.", + ), ); } } @@ -325,11 +342,7 @@ mod tests { // Should find missing README (VIS-001) which is an error assert!(result.has_errors(), "Bare repo should have errors"); - let error_ids: Vec<&str> = result - .errors() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let error_ids: Vec<&str> = result.errors().iter().map(|f| f.id.as_str()).collect(); assert!( error_ids.contains(&"VIS-001"), "Should report missing README" @@ -346,11 +359,7 @@ mod tests { let config = default_config(); let result = analyzer.analyze(dir.path(), &config).unwrap(); - let warning_ids: Vec<&str> = result - .warnings() - .iter() - .map(|f| f.id.as_str()) - .collect(); + let warning_ids: Vec<&str> = result.warnings().iter().map(|f| f.id.as_str()).collect(); assert!( warning_ids.contains(&"VIS-002"), "Short README should trigger VIS-002 warning" diff --git a/bots/glambot/src/config/mod.rs b/bots/glambot/src/config/mod.rs index 4b90c21f..2dd55bfc 100644 --- a/bots/glambot/src/config/mod.rs +++ b/bots/glambot/src/config/mod.rs @@ -109,8 +109,8 @@ impl Default for SeoConfig { require_opengraph: false, check_titles: true, check_descriptions: true, - enable_git_seo: Some(true), // Enable by default - enable_auto_apply: Some(false), // Safety: manual by default + enable_git_seo: Some(true), // Enable by default + enable_auto_apply: Some(false), // Safety: manual by default } } } diff --git a/bots/glambot/src/lib.rs b/bots/glambot/src/lib.rs index 0100e162..e599600d 100644 --- a/bots/glambot/src/lib.rs +++ b/bots/glambot/src/lib.rs @@ -21,8 +21,8 @@ pub mod fleet; pub use analyzers::{ accessibility::AccessibilityAnalyzer, git_seo_integration::GitSeoAnalyzer, - machine::MachineAnalyzer, seo::SeoAnalyzer, visual::VisualAnalyzer, AnalysisResult, - Analyzer, AuditResult, Finding, Severity, + machine::MachineAnalyzer, seo::SeoAnalyzer, visual::VisualAnalyzer, AnalysisResult, Analyzer, + AuditResult, Finding, Severity, }; pub use config::Config; pub use error::{GlambotError, Result}; diff --git a/bots/glambot/src/main.rs b/bots/glambot/src/main.rs index 50b71e1b..c350fc4e 100644 --- a/bots/glambot/src/main.rs +++ b/bots/glambot/src/main.rs @@ -174,7 +174,10 @@ fn handle_show(config: &config::Config) -> ExitCode { println!("Accessibility:"); println!(" WCAG level: {:?}", config.accessibility.wcag_level); - println!(" Require alt text: {}", config.accessibility.require_alt_text); + println!( + " Require alt text: {}", + config.accessibility.require_alt_text + ); println!(); println!("SEO:"); @@ -184,7 +187,10 @@ fn handle_show(config: &config::Config) -> ExitCode { println!("Machine:"); println!(" Validate JSON: {}", config.machine.validate_json); - println!(" Require schema.org: {}", config.machine.require_schema_org); + println!( + " Require schema.org: {}", + config.machine.require_schema_org + ); println!(); println!("General:"); @@ -197,8 +203,7 @@ fn handle_show(config: &config::Config) -> ExitCode { fn handle_audit(path: &PathBuf, config: &config::Config, strict: bool, format: &str) -> ExitCode { use crate::analyzers::{ accessibility::AccessibilityAnalyzer, git_seo_integration::GitSeoAnalyzer, - machine::MachineAnalyzer, seo::SeoAnalyzer, visual::VisualAnalyzer, Analyzer, - AuditResult, + machine::MachineAnalyzer, seo::SeoAnalyzer, visual::VisualAnalyzer, Analyzer, AuditResult, }; let mut result = AuditResult::default(); @@ -247,8 +252,12 @@ fn handle_audit(path: &PathBuf, config: &config::Config, strict: bool, format: & Ok(r) => r, Err(e) => { eprintln!("Git-SEO analysis error: {}", e); - // Don't fail if git-seo is unavailable, just skip - Default::default() + // Don't fail the audit, but say it was skipped: an empty result + // here must not read as a clean scan. + analyzers::AnalysisResult { + skipped: Some(format!("git-seo analysis error: {e}")), + ..Default::default() + } } }; } diff --git a/bots/glambot/tests/integration.rs b/bots/glambot/tests/integration.rs index b5390e3b..258b8ea8 100644 --- a/bots/glambot/tests/integration.rs +++ b/bots/glambot/tests/integration.rs @@ -139,9 +139,8 @@ fn test_all_analyzers_deficient_repo() { ); // Verify correct total count - let total = visual_result.findings.len() - + acc_result.findings.len() - + machine_result.findings.len(); + let total = + visual_result.findings.len() + acc_result.findings.len() + machine_result.findings.len(); assert!( total >= 4, "Should have at least 4 total findings across analyzers, got {}", @@ -191,11 +190,7 @@ fn test_finding_severity_levels() { let result = machine.analyze(dir.path(), &config).unwrap(); // Verify we get different severity levels - let severities: Vec = result - .findings - .iter() - .map(|f| f.severity) - .collect(); + let severities: Vec = result.findings.iter().map(|f| f.severity).collect(); // Invalid JSON should be Error assert!( @@ -237,7 +232,7 @@ fn json_audit_stdout_is_only_the_report() { .output() .unwrap(); let text = String::from_utf8(out.stdout).unwrap(); - let parsed: serde_json::Value = - serde_json::from_str(&text).unwrap_or_else(|e| panic!("stdout is not pure JSON ({e}): {text}")); + let parsed: serde_json::Value = serde_json::from_str(&text) + .unwrap_or_else(|e| panic!("stdout is not pure JSON ({e}): {text}")); assert!(parsed.is_object()); } diff --git a/docs/REPORT-BACK.adoc b/docs/REPORT-BACK.adoc index 22cecc0f..b84b89a5 100644 --- a/docs/REPORT-BACK.adoc +++ b/docs/REPORT-BACK.adoc @@ -169,7 +169,11 @@ that emits a path-less finding; rhodibot and seambot already comply. | glambot | `glambot --path --format json audit` | Visual, accessibility, SEO, machine-readability and git-SEO analyzers, - flattened. + flattened. An analyzer that could not run (git-seo not installed) + reports a `skipped` reason in its own object; the driver keeps `status` `ran` + but copies the reason into the envelope `reason`, so an empty result is not + mistaken for a clean scan. A first-class `skipped` field in the envelope + would need a change to the verdict-grid spec in standards. | accessibilitybot | `accessibilitybot fleet ` diff --git a/scripts/fleet-report-back.sh b/scripts/fleet-report-back.sh index bc63424e..fc00b4cf 100755 --- a/scripts/fleet-report-back.sh +++ b/scripts/fleet-report-back.sh @@ -538,8 +538,8 @@ cleanup_adapter() { rm -f "$ADAPTER_OUT" "$ADAPTER_ERR"; } # status 124 always produces a timeout error; invalid output produces an error # with a stderr diagnostic. Bind $repo for relativize_filter, write the member's # envelope, and remove the capture files. Envelope I/O failures are not caught. -translate_or_error() { # member jq-program - local m="$1" prog="$2" findings +translate_or_error() { # member jq-program [ran-note-jq-program] + local m="$1" prog="$2" findings note="" if [ "$ADAPTER_RC" -eq 124 ]; then write_envelope "$m" error "adapter exceeded the ${timeout_s}s timeout" '[]' elif findings=$(jq -c --arg repo "$repo" "$(relativize_filter) $prog" "$ADAPTER_OUT" 2> "$ADAPTER_ERR.jq") \ @@ -550,7 +550,12 @@ translate_or_error() { # member jq-program # a well-formed findings array. write_envelope "$m" error "adapter exited with status $ADAPTER_RC: $(first_err_line)" '[]' else - write_envelope "$m" ran "" "$findings" + # An analyzer that did not run has no findings, which is not a clean scan: + # the optional program reads the adapter output for such skips and the note + # becomes the envelope reason (ran + reason = ran, with caveats). + if [ -n "${3:-}" ]; then note=$(jq -r "$3" "$ADAPTER_OUT" 2>/dev/null) || note=""; fi + [ -z "$note" ] || echo "fleet-report-back: $m: $note" >&2 + write_envelope "$m" ran "$note" "$findings" fi elif grep -q 'unexpected output shape' "$ADAPTER_ERR.jq" 2>/dev/null; then # The shape guard fired: valid JSON, but not what this adapter documents. @@ -680,7 +685,12 @@ run_member() { translate_or_error "$m" "$SHAPE_ANALYZERS [ .[] | .findings[] | $FINDING ]" ;; glambot) run_adapter "$bin" --path "$repo" --format json audit - translate_or_error "$m" "$SHAPE_ANALYZERS [ .[] | .findings[] | $FINDING ]" ;; + # An analyzer that could not run (git-seo missing) reports `skipped`; say so + # in the envelope reason rather than leaving an empty list that reads clean. + translate_or_error "$m" "$SHAPE_ANALYZERS [ .[] | .findings[] | $FINDING ]" \ + '[ (if type == "object" then to_entries[] else {key: "analyzer", value: .[]} end) + | select((.value.skipped // null) | type == "string") + | "\(.key) analyzer skipped: \(.value.skipped); its empty result is not a clean scan" ] | join("; ")' ;; accessibilitybot) run_adapter "$bin" fleet "$repo" translate_or_error "$m" "$SHAPE_OBJ_FINDINGS [ .findings[] | $FINDING ]" ;; @@ -694,7 +704,7 @@ if [ "${#members[@]}" -eq 0 ]; then fi for m in "${members[@]+"${members[@]}"}"; do run_member "$m" - echo "fleet-report-back: $m -> $(jq -r '.status + (if .status == "ran" then " (\(.findings | length) finding(s))" else ": " + (.reason // "") end)' "$out/$m.json")" + echo "fleet-report-back: $m -> $(jq -r '.status + (if .status == "ran" then " (\(.findings | length) finding(s))" + (if .reason then " [" + .reason + "]" else "" end) else ": " + (.reason // "") end)' "$out/$m.json")" # A path-less finding is graded as part of EVERY change (standards # fork-governance SPEC section 3.0: absence of a location is not evidence of # absence of a defect), so a repository-level rule that omits its path makes diff --git a/tests/fleet-report-back-test.sh b/tests/fleet-report-back-test.sh index a8f77b92..ffd96600 100755 --- a/tests/fleet-report-back-test.sh +++ b/tests/fleet-report-back-test.sh @@ -173,6 +173,27 @@ check "a missing binary is error" error "$(field "$TMP/o4/glambot.json" .status) check "...naming the binary and the crate" yes \ "$(field "$TMP/o4/glambot.json" .reason | grep -q "glambot.*bots/glambot" && echo yes || echo no)" + +# A skipped analyzer (git-seo not installed) has no findings, which is not a +# clean scan: the envelope still says `ran`, but its reason says what did not. +stub glambot '[ "$1 $3 $4 $5" = "--path --format json audit" ] || { echo "bad args: $*" >&2; exit 9; } +cat <&2; exit 9; } +cat <&2; exit 9; } +cat <