diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8fbf888..5c7455e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -45,7 +45,7 @@ repos: # a second copy of it to go stale. - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.16.2 + rev: v0.16.3 hooks: - id: ruff-check args: [--fix] diff --git a/Cargo.lock b/Cargo.lock index 3d2bb06..822d723 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -22,6 +22,16 @@ dependencies = [ "serde_core", ] +[[package]] +name = "cc" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d262e149917187838d5b42777c8253bcb64500067342904e7d429499a6f277e" +dependencies = [ + "find-msvc-tools", + "shlex", +] + [[package]] name = "cfg-if" version = "1.0.4" @@ -77,6 +87,12 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" +[[package]] +name = "find-msvc-tools" +version = "0.1.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26b73573e6edcd2af0cdf47bd6cb58f0b3839491263c314eaad1ccf24430e1de" + [[package]] name = "getrandom" version = "0.2.17" @@ -372,6 +388,20 @@ dependencies = [ "syn 3.0.3", ] +[[package]] +name = "serde_json" +version = "1.0.151" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c841b55ecdae098c80dcae9cf767f6f8a0c2cdb3416bbef72181df4d0fe73f14" +dependencies = [ + "indexmap", + "itoa", + "memchr", + "serde", + "serde_core", + "zmij", +] + [[package]] name = "serde_spanned" version = "1.1.1" @@ -394,12 +424,24 @@ dependencies = [ "unsafe-libyaml", ] +[[package]] +name = "shlex" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" + [[package]] name = "siphasher" version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ee5873ec9cce0195efcb7a4e9507a04cd49aec9c83d0389df45b1ef7ba2e649" +[[package]] +name = "streaming-iterator" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b2231b7c3057d5e4ad0156fb3dc807d900806020c5ffa3ee6ff2c8c76fb8520" + [[package]] name = "syn" version = "2.0.119" @@ -461,6 +503,46 @@ version = "1.1.2+spec-1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" +[[package]] +name = "tree-sitter" +version = "0.26.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "83c567a8e18ae93f20982c90370b16fd24023aeaf52f6052b96957ab253a0fec" +dependencies = [ + "cc", + "regex", + "regex-syntax", + "serde_json", + "streaming-iterator", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-language" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "009994f150cc0cd50ff54917d5bc8bffe8cad10ca10d81c34da2ec421ae61782" + +[[package]] +name = "tree-sitter-python" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6bf85fd39652e740bf60f46f4cda9492c3a9ad75880575bf14960f775cb74a1c" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-rust" +version = "0.24.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "439e577dbe07423ec2582ac62c7531120dbfccfa6e5f92406f93dd271a120e45" +dependencies = [ + "cc", + "tree-sitter-language", +] + [[package]] name = "unicode-ident" version = "1.0.24" @@ -513,6 +595,9 @@ dependencies = [ "serde", "serde_yaml_ng", "toml", + "tree-sitter", + "tree-sitter-python", + "tree-sitter-rust", "unicode-script", "unicode_names2", ] @@ -582,3 +667,9 @@ dependencies = [ "quote", "syn 2.0.119", ] + +[[package]] +name = "zmij" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29666d0abbfad1e3dc4dcf6144730dd3a3ab225bbbdac83319345b1b44ccfc1b" diff --git a/Cargo.toml b/Cargo.toml index 7777a43..3c386cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,6 +48,15 @@ unicode_names2 = "3.1" # `parameterize-do-not-enumerate` names. encoding_rs = "0.8" serde_yaml_ng = "0.10.0" +# A parser, for the one question a regex over file contents cannot answer: +# whether the text it matched is a comment. `grep-regex` reads bytes, so +# `let s = "// TODO";` is a hit and a rule about comments is a rule about +# anything that spells one. These give the comment its own node kind -- and +# `///` a different kind from `//`, which is the distinction a line-prefix test +# gets wrong by construction: `///` starts with `//`. +tree-sitter = "0.26" +tree-sitter-rust = "0.24" +tree-sitter-python = "0.25" [profile.release] strip = true diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index be87dcf..e8603aa 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -33,6 +33,8 @@ anything else (`[rule."my rule"]`). | field | fails when | |---|---| | `regexp` | the regex matches anywhere in the selected files | +| `comment_regexp` | the regex matches a **comment** in a selected Rust or Python file | +| `trivial_comments` | a comment contributes no word the code beneath it already names | | `path_regexp` | a tracked path matches the regex | | `require_regexp` | a selected file does **not** contain the regex | | `max_lines` | a selected file is longer than that, or grew past its baseline | @@ -213,6 +215,53 @@ built-in is added unless one of the declared rules is itself `forbidden_literals = "running-os-identity"`. Declaring a rule about something else is not a decision to stop checking this. +### `comment_regexp` and `trivial_comments` — the comment, not the line + +Both parse the file rather than searching it, in Rust and Python. That is the +whole reason they are separate checks: `regexp` reads bytes, so +`let marker = "// TODO";` is a hit for a rule about `// TODO` and there is no +way to write the difference down. These read comment nodes, so a marker inside a +string literal is a string literal. + +```toml +[rule.no-before-after-narrative-in-source] +message = "State what holds, not what it replaced." +comment_regexp = '(?i)\bused to be\b' +files.include = ["src"] +files.glob = ["*.rs", "*.py"] + +[rule.no-trivial-comment] +message = "This comment says only what the code beneath it already says." +trivial_comments = true +files.include = ["src"] +files.glob = ["*.rs", "*.py"] +``` + +**Documentation comments are excluded from both.** `///` and `//!` are published +output, not remarks to the next reader, and a check that cannot tell them apart +from `//` is one whose findings, acted on, delete a public item's documentation. +The grammar marks them; nothing here matches on the prefix, which is what a +prefix test gets wrong by construction — `///` starts with `//`. + +`trivial_comments` is a subset test and carries no list of boring verbs: a +comment fails when every word it contributes is a word the statements beneath it +already name, counting their string literals. `// Stop and disable dnsmasq` over +`systemd::stop("dnsmasq")` and `systemd::disable("dnsmasq")` fails; the same +comment with a reason attached does not, and no list had to be edited for that to +be true. The code it is judged against runs from the comment to the next blank +line or the next comment — where a reader stops attributing it. + +Five shapes are left alone, each because its words restate the code by design +while the comment is doing something else: a trailing comment on the same line as +code, one line of a multi-line comment run, a separator (`---`, `===`, box +drawing), a worked example (containing `=` or `→`), and a parenthesised aside. +A tree that wants its separators gone writes a `comment_regexp` saying so; this +check does not reach that verdict on its own. + +There is no fixer, and that is a decision rather than a gap. A comment worth +deleting is usually worth replacing with the reason the code is that way, and +that is not an edit a checker can make. + ### `forbidden_literals` — what must appear nowhere ```toml diff --git a/policy/principles.toml b/policy/principles.toml index b042409..71d5615 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -79,6 +79,59 @@ regexp = '(?i)\b(?:became|was renamed|renamed to|formerly|previously (?:called|n files.glob = ["*.md"] files.exclude = ["CHANGELOG*", "**/tests/**", "**/test/**"] +# The same sentence, in the files the *.md rule was never able to reach. +# +# A `regexp` over source finds the phrase in a string literal as readily as in a +# comment, and this repository has both -- the message a rule prints is a string +# containing the prose the rule is about. So the subject is the comment, read off +# the parse, and `///` is excluded because a doc comment is published output +# rather than a remark to the next reader. +# +# The pattern is not the *.md one. `used to be` is shared, and `no longer +# needed` and `previously, this` are added because those are the tenses a +# comment reaches for when it explains the diff to whoever arrives next. +# +# `was renamed`, `renamed to` and `formerly` are deliberately NOT here, though +# the *.md rule carries all three. Each was measured against this tree and each +# fired on domain prose rather than on history: `guard/names.rs` is about +# repositories that get renamed, and `selection.rs:309` about a root renamed +# away underneath a walk. A document does not talk about renaming; the code that +# handles it does nothing else. +[rule.no-before-after-narrative-in-source] +message = """ +State what holds, not what it replaced. A comment that explains what the code +used to do goes stale the day either version moves again, and the story of the +change belongs in the commit that made it. +""" +comment_regexp = '(?i)\b(?:used to be|used to (?:use|call|return|do|have|rely)|previously[,:]?\s+(?:this|we|it|the)|no longer (?:needed|used|required|necessary))\b' +files.include = ["src", "scripts", "uphold_check.py"] +files.glob = ["*.rs", "*.py"] +files.exclude = ["**/tests/**", "**/test/**"] + +# A comment carrying no word its own code does not already name. +# +# The test is a subset and not a list of boring verbs: every word the comment +# contributes has to be a word the statements under it already say, counting +# their string literals. `// Stop and disable dnsmasq` over `systemd::stop(...)` +# and `systemd::disable(...)` is trivial because all three words are in the code; +# the same comment with a reason attached is not, and no list had to be edited +# to know that. +# +# Tests are excluded because a test's comment names the case rather than the +# code -- `// Delete the applied local IPv6` above an assertion is what tells a +# reader which of nine assertions this one is. +[rule.no-trivial-comment] +message = """ +This comment says only what the code beneath it already says. Delete it, or +replace it with the reason the code is that way -- a comment that repeats an +identifier goes stale the moment the identifier changes and helps nobody until +it does. +""" +trivial_comments = true +files.include = ["src", "scripts", "uphold_check.py"] +files.glob = ["*.rs", "*.py"] +files.exclude = ["**/tests/**", "**/test/**"] + # --------------------------------------------------------------------------- # Guards -- what git is about to do. # diff --git a/src/comments.rs b/src/comments.rs new file mode 100644 index 0000000..bfb8266 --- /dev/null +++ b/src/comments.rs @@ -0,0 +1,581 @@ +//! Comments, as the parser sees them. +//! +//! Every other check in this crate reads bytes, which is the right answer when +//! the question is about bytes. It is the wrong answer for a rule about +//! comments: `let marker = "// TODO";` is a line containing `// TODO` and no +//! comment at all, and a rule written against the text cannot tell the +//! difference. So this module hands the checks a list of comments rather than a +//! list of lines, and the language decides what one is. +//! +//! The distinction that matters most here is the one a prefix test cannot make. +//! `///` starts with `//`, so any check that recognises a comment by its opening +//! characters treats a Rust doc comment as an ordinary one -- and a tool that +//! then deletes what it matched deletes the documentation of a public item. The +//! grammar gives the doc comment its own marker node, so [`Comment::doc`] is +//! read from the parse rather than guessed from the spelling. + +use std::collections::BTreeSet; + +use tree_sitter::{Node, Parser}; + +/// The languages a comment rule can be asked about. +/// +/// Two, because two is what the fleet is written in. A third is a grammar +/// dependency and three lines in [`Language::for_path`]; it is not a redesign, +/// which is the property worth keeping. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Language { + Rust, + Python, +} + +impl Language { + /// The language of a repository-relative path, or `None` for a file no + /// comment rule can read. + pub(crate) fn for_path(path: &str) -> Option { + match path.rsplit_once('.') { + Some((_, "rs")) => Some(Self::Rust), + Some((_, "py" | "pyi")) => Some(Self::Python), + _ => None, + } + } + + fn grammar(self) -> tree_sitter::Language { + match self { + Self::Rust => tree_sitter_rust::LANGUAGE.into(), + Self::Python => tree_sitter_python::LANGUAGE.into(), + } + } + + /// The node kinds that ARE comments in this grammar. + const fn comment_kinds(self) -> &'static [&'static str] { + match self { + Self::Rust => &["line_comment", "block_comment"], + Self::Python => &["comment"], + } + } +} + +/// One comment, with the code it sits above. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Comment { + /// 1-based, matching every other line number this crate reports. + pub line: u64, + /// The comment with its markers removed. What a `comment_regexp` matches + /// against, so a pattern never has to know how a language spells `//`. + pub text: String, + /// A documentation comment: `///` and `//!` in Rust. Kept apart from the + /// ordinary kind because it is an artefact -- rustdoc publishes it -- and a + /// rule that treats it as a comment about the code is a rule that deletes + /// the public API's documentation. + pub doc: bool, + /// Whether the comment stands on its own line rather than trailing code. + pub own_line: bool, + /// Whether another comment sits directly above or below it. A comment in a + /// run is prose spanning several lines, and judging one line of it alone + /// reads half a sentence. + pub in_run: bool, + /// The words of the code this comment introduces: identifiers, split on + /// case and underscore, plus the contents of any string literal. Empty when + /// the comment introduces nothing. + pub subject: BTreeSet, +} + +/// Words that carry no information about what code does, so their presence in a +/// comment should not stop it being trivial. Deliberately short: every word here +/// is one that appears in "the thing" as often as in "do the thing", and a list +/// that grew to include verbs would be the enumerated verb table this check +/// exists to avoid. +const FILLER: &[&str] = &[ + "a", "an", "the", "this", "that", "these", "those", "it", "its", "we", "our", "us", "to", + "for", "of", "and", "or", "in", "on", "at", "by", "with", "from", "into", "as", "is", "are", + "be", "all", "any", "each", "every", "new", "old", "existing", "current", "here", "then", + "now", "again", "back", "up", "down", "out", "off", "over", "per", +]; + +/// Words that make a comment an explanation rather than a restatement. +/// +/// A comment that says WHY -- a reason, a condition, a hazard -- is the comment +/// worth keeping, and no subset test can recognise one: "close the file" and +/// "close the file or the lock outlives the process" have the same words plus a +/// clause. The clause is what this list finds. +const EXPLANATORY: &[&str] = &[ + "because", + "since", + "so", + "otherwise", + "unless", + "until", + "while", + "when", + "if", + "but", + "though", + "although", + "however", + "note", + "todo", + "fixme", + "hack", + "warning", + "workaround", + "caveat", + "assumes", + "assume", + "must", + "should", + "cannot", + "never", + "always", + "only", + "ensure", + "avoid", + "prevent", + "requires", + "require", + "needs", + "need", + "safety", + "invariant", +]; + +/// Strip a comment's markers, whatever the language spells them as. +fn strip_markers(raw: &str) -> String { + let trimmed = raw.trim(); + let body = trimmed + .strip_prefix("///") + .or_else(|| trimmed.strip_prefix("//!")) + .or_else(|| trimmed.strip_prefix("//")) + .or_else(|| trimmed.strip_prefix("#")) + .unwrap_or(trimmed); + let body = body + .strip_prefix("/*") + .map_or(body, |rest| rest.trim_end_matches("*/")); + body.trim().to_owned() +} + +/// A Rust doc comment, read from the grammar rather than from the spelling. +/// +/// The grammar marks `///` and `//!` with their own marker nodes inside the +/// comment. Falling back to the prefix when a grammar version does not emit +/// them keeps the answer right rather than convenient: the fallback is the +/// same test, done worse, and it is only reached when the better one is absent. +fn is_doc_comment(node: Node<'_>, source: &str) -> bool { + let mut cursor = node.walk(); + let marked = node.children(&mut cursor).any(|child| { + matches!( + child.kind(), + "doc_comment" | "outer_doc_comment_marker" | "inner_doc_comment_marker" + ) + }); + if marked { + return true; + } + let text = node_text(node, source); + let trimmed = text.trim_start(); + trimmed.starts_with("///") || trimmed.starts_with("//!") || trimmed.starts_with("/**") +} + +fn node_text<'a>(node: Node<'_>, source: &'a str) -> &'a str { + source.get(node.byte_range()).unwrap_or_default() +} + +/// Split an identifier into its words: `set_zone_target` and `setZoneTarget` +/// both become `set`, `zone`, `target`. +fn identifier_words(identifier: &str, out: &mut BTreeSet) { + let mut word = String::new(); + let mut previous_lower = false; + for character in identifier.chars() { + if character.is_alphanumeric() { + if character.is_uppercase() && previous_lower && !word.is_empty() { + out.insert(std::mem::take(&mut word)); + } + word.push(character.to_ascii_lowercase()); + previous_lower = character.is_lowercase() || character.is_numeric(); + } else { + if !word.is_empty() { + out.insert(std::mem::take(&mut word)); + } + previous_lower = false; + } + } + if !word.is_empty() { + out.insert(word); + } +} + +/// Every word the code under a comment names. +/// +/// String literals are in here beside the identifiers on purpose. `// Stop and +/// disable dnsmasq` over `systemd::stop("dnsmasq")` restates the literal, not an +/// identifier, and a subject built from identifiers alone would call that +/// comment informative. +fn subject_words(node: Node<'_>, source: &str) -> BTreeSet { + let mut words = BTreeSet::new(); + let mut cursor = node.walk(); + let mut pending = vec![node]; + while let Some(current) = pending.pop() { + match current.kind() { + // Identifiers and literals in one arm, because they are one thing + // to this check: both are words the code puts on the page, and a + // comment repeating either is repeating the code. + "identifier" + | "type_identifier" + | "field_identifier" + | "primitive_type" + | "shorthand_field_identifier" + | "string_content" + | "string_literal" + | "string" => { + identifier_words(node_text(current, source), &mut words); + } + _ => {} + } + pending.extend(current.children(&mut cursor)); + } + words +} + +/// The statements a comment introduces. +/// +/// A run of them, not one: `// Stop and disable dnsmasq` sits above a stop and a +/// disable, and a subject built from the first line alone would find `disable` +/// missing and call the comment informative. The run ends where the reader would +/// end it -- at a blank line, or at the next comment -- so what counts as "the +/// code this comment is about" is the same thing on the page and in the check. +fn introduced_code<'tree>(comment: Node<'tree>, kinds: &[&str]) -> Vec> { + let mut sibling = comment.next_named_sibling(); + let mut previous_end = comment.end_position().row; + let mut run = Vec::new(); + while let Some(node) = sibling { + if kinds.contains(&node.kind()) { + break; + } + // A blank line is where a reader stops attributing the comment. + if node.start_position().row > previous_end + 1 { + break; + } + previous_end = node.end_position().row; + run.push(node); + sibling = node.next_named_sibling(); + } + run +} + +/// Collect every comment in one file. +/// +/// A file that does not parse is not an error and not silence either: the parse +/// tree of broken source still contains its comments, because the grammar's +/// error recovery keeps lexing. What a caller gets from a file it could not +/// read at all is an empty list, and the selection layer is what reports that. +pub(crate) fn collect(source: &str, language: Language) -> Vec { + let mut parser = Parser::new(); + if parser.set_language(&language.grammar()).is_err() { + return Vec::new(); + } + let Some(tree) = parser.parse(source, None) else { + return Vec::new(); + }; + + let kinds = language.comment_kinds(); + let mut nodes = Vec::new(); + let mut cursor = tree.walk(); + let mut pending = vec![tree.root_node()]; + while let Some(current) = pending.pop() { + if kinds.contains(¤t.kind()) { + nodes.push(current); + } + pending.extend(current.children(&mut cursor)); + } + nodes.sort_by_key(Node::start_byte); + + let lines: Vec<&str> = source.lines().collect(); + let comment_lines: BTreeSet = nodes + .iter() + .map(|node| node.start_position().row as u64 + 1) + .collect(); + + nodes + .iter() + .map(|&node| { + let row = node.start_position().row; + let line = row as u64 + 1; + let own_line = lines.get(row).is_some_and(|text| { + text.trim_start() + .starts_with(node_text(node, source).trim()) + }); + let in_run = comment_lines.contains(&line.saturating_sub(1)) + || comment_lines.contains(&(line + 1)); + let mut subject = BTreeSet::new(); + for code in introduced_code(node, kinds) { + subject.extend(subject_words(code, source)); + } + Comment { + line, + text: strip_markers(node_text(node, source)), + doc: language == Language::Rust && is_doc_comment(node, source), + own_line, + in_run, + subject, + } + }) + .collect() +} + +/// The words a comment contributes, filler removed. +fn comment_words(text: &str) -> Vec { + let mut words = Vec::new(); + for token in text.split(|c: char| !c.is_alphanumeric()) { + if token.is_empty() { + continue; + } + let lowered = token.to_ascii_lowercase(); + if FILLER.contains(&lowered.as_str()) { + continue; + } + words.push(lowered); + } + words +} + +/// Whether two words name the same thing. +/// +/// Exact match, then a shared stem, then a prefix of at least four characters -- +/// which is what lets `config` recognise the `CONF` in `DNSMASQ_CONF_FILE`. +/// Four rather than three because `set` would otherwise match `settings`, +/// `setup` and `setter` alike, and a comment saying `set` beside code that +/// settles something is not a restatement. +fn same_word(comment_word: &str, subject_word: &str) -> bool { + if comment_word == subject_word { + return true; + } + let stem = |word: &str| { + let word = word + .strip_suffix("ing") + .or_else(|| word.strip_suffix("ed")) + .or_else(|| word.strip_suffix("es")) + .or_else(|| word.strip_suffix('s')) + .unwrap_or(word); + word.strip_suffix('e').unwrap_or(word).to_owned() + }; + let (left, right) = (stem(comment_word), stem(subject_word)); + if left == right && !left.is_empty() { + return true; + } + let shorter = left.len().min(right.len()); + shorter >= 4 && (left.starts_with(&right) || right.starts_with(&left)) +} + +/// Does this comment say only what the code under it already says? +/// +/// The test is a subset, not a pattern: every word the comment contributes has +/// to be a word the following code already names. That is the whole rule, and it +/// is why there is no verb list here -- `// Stop and disable dnsmasq` is trivial +/// because `stop`, `disable` and `dnsmasq` are all in the code, not because +/// "stop" is on a list of boring verbs. A comment carrying one word the code +/// does not have is a comment that says something, whatever the word is. +pub(crate) fn is_trivial(comment: &Comment) -> bool { + if comment.doc || !comment.own_line || comment.in_run { + return false; + } + if comment.subject.is_empty() { + return false; + } + // A separator is not a remark about the code and is not judged as one. Its + // words restate the section by design -- that is what a heading does -- so + // a subset test calls every one of them trivial. Whether a tree wants them + // is a question about house style, which is a `comment_regexp` a repository + // writes if it wants to, and not a verdict this check should reach on its + // own. + if comment.text.contains("---") || comment.text.contains("===") { + return false; + } + if comment.text.contains(['─', '━', '═', '┄', '│']) { + return false; + } + // A worked example is not a restatement: `192.168.1.1/24 -> network = + // 192.168.1.0` shares every token with the code and says the one thing the + // code does not, which is what the answer comes out as. + if comment.text.contains('=') || comment.text.contains('→') || comment.text.contains("->") { + return false; + } + // A parenthesised aside is a qualification -- `(optional, comma-separated)` + // -- and the words inside it are the part the code does not carry. + if comment.text.contains('(') && comment.text.contains(')') { + return false; + } + let words = comment_words(&comment.text); + if words.is_empty() { + return false; + } + if words + .iter() + .any(|word| EXPLANATORY.contains(&word.as_str())) + { + return false; + } + words.iter().all(|word| { + comment + .subject + .iter() + .any(|subject| same_word(word, subject)) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn rust(source: &str) -> Vec { + collect(source, Language::Rust) + } + + /// The reason this module exists. `grep-regex` finds `// TODO` in both of + /// these lines; a comment rule must find it in one. + #[test] + fn a_comment_marker_inside_a_string_is_not_a_comment() { + let found = rust("fn f() {\n let marker = \"// TODO: not a comment\";\n}\n"); + assert!(found.is_empty(), "{found:?}"); + } + + /// `///` starts with `//`, which is how a prefix test loses a public item's + /// documentation. + #[test] + fn a_doc_comment_is_not_an_ordinary_comment() { + let found = rust("/// The outer EAP method.\npub struct Config;\n"); + assert_eq!(found.len(), 1); + assert!(found[0].doc); + assert!(!is_trivial(&found[0])); + } + + #[test] + fn an_inner_doc_comment_is_a_doc_comment() { + let found = rust("//! Module docs.\npub struct Config;\n"); + assert_eq!(found.len(), 1); + assert!(found[0].doc); + } + + /// The literal is part of what the code says, so a comment repeating it is + /// repeating the code. + #[test] + fn a_comment_restating_a_call_and_its_literal_is_trivial() { + let found = rust( + "fn f() {\n // Stop and disable dnsmasq\n systemd::stop(\"dnsmasq\");\n systemd::disable(\"dnsmasq\");\n}\n", + ); + assert_eq!(found.len(), 1, "{found:?}"); + assert!(is_trivial(&found[0]), "{:?}", found[0]); + } + + /// The run stops where a reader stops attributing the comment. + #[test] + fn a_blank_line_ends_the_code_a_comment_is_about() { + let found = rust( + "fn f() {\n // Stop dnsmasq\n systemd::stop(\"dnsmasq\");\n\n reload_relay();\n}\n", + ); + assert!(!found[0].subject.contains("reload"), "{:?}", found[0]); + } + + /// One word the code does not have, and the comment is saying something. + #[test] + fn a_comment_carrying_a_word_the_code_lacks_is_kept() { + let found = rust( + "fn f() {\n // Validate the config offline before prompting.\n validate_config(&runtime);\n}\n", + ); + assert_eq!(found.len(), 1); + assert!(!is_trivial(&found[0]), "{:?}", found[0]); + } + + /// A reason is not a restatement, however few words it has. + #[test] + fn an_explanatory_clause_is_kept_even_when_its_words_are_in_the_code() { + let found = rust( + "fn f() {\n // Stop dnsmasq because the relay holds the port.\n systemd::stop(\"dnsmasq\");\n}\n", + ); + assert!(!is_trivial(&found[0]), "{:?}", found[0]); + } + + /// Judging one line of a multi-line comment reads half a sentence. + #[test] + fn a_comment_in_a_run_is_not_judged_alone() { + let found = rust( + "fn f() {\n // Stop dnsmasq.\n // The relay holds the port open otherwise.\n systemd::stop(\"dnsmasq\");\n}\n", + ); + assert!( + found.iter().all(|comment| !is_trivial(comment)), + "{found:?}" + ); + } + + #[test] + fn a_trailing_comment_is_not_judged_as_an_introduction() { + let found = rust("fn f() {\n systemd::stop(\"dnsmasq\"); // Stop dnsmasq\n}\n"); + assert_eq!(found.len(), 1); + assert!(!found[0].own_line); + assert!(!is_trivial(&found[0])); + } + + #[test] + fn python_comments_are_read_with_the_python_grammar() { + let found = collect("# Load the config\nload_config()\n", Language::Python); + assert_eq!(found.len(), 1, "{found:?}"); + assert!(is_trivial(&found[0]), "{:?}", found[0]); + } + + #[test] + fn a_python_shebang_is_not_judged_against_the_code_below_it() { + let found = collect("#!/usr/bin/env python3\nload_config()\n", Language::Python); + assert!( + found.iter().all(|comment| !is_trivial(comment)), + "{found:?}" + ); + } + + #[test] + fn an_identifier_splits_on_case_and_underscore() { + let mut words = BTreeSet::new(); + identifier_words("set_zone_target", &mut words); + identifier_words("setZoneTarget", &mut words); + assert!(words.contains("set") && words.contains("zone") && words.contains("target")); + } + + /// `config` and `CONF` are the same word here; `set` and `settings` are not. + #[test] + fn a_shared_prefix_counts_only_when_it_is_long_enough_to_mean_something() { + assert!(same_word("config", "conf")); + assert!(same_word("zones", "zone")); + assert!(!same_word("set", "settings")); + } + + /// A heading restates its section by design, so a subset test calls every + /// separator trivial. Judging house style is not this check's job. + #[test] + fn a_separator_is_not_judged_as_a_remark_about_the_code() { + let found = rust("fn f() {\n // --- ICMP rules ---\n add_icmp_rules();\n}\n"); + assert!(!is_trivial(&found[0]), "{:?}", found[0]); + } + + /// Shares every token with the code, and says the one thing the code does + /// not: what the answer comes out as. + #[test] + fn a_worked_example_is_not_a_restatement() { + let found = rust( + "fn f() {\n // 192.168.1.1/24 = network 192.168.1.0\n let network = network_of(address);\n}\n", + ); + assert!(!is_trivial(&found[0]), "{:?}", found[0]); + } + + #[test] + fn a_parenthesised_aside_is_the_part_the_code_does_not_carry() { + let found = rust( + "fn f() {\n // Ports (optional, comma-separated)\n let ports = prompt(\"Ports\");\n}\n", + ); + assert!(!is_trivial(&found[0]), "{:?}", found[0]); + } + + #[test] + fn a_language_is_chosen_by_extension_and_nothing_else() { + assert_eq!(Language::for_path("src/scan.rs"), Some(Language::Rust)); + assert_eq!(Language::for_path("scripts/x.py"), Some(Language::Python)); + assert_eq!(Language::for_path("README.md"), None); + } +} diff --git a/src/config.rs b/src/config.rs index 256492d..fafa9bc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -78,6 +78,15 @@ pub(crate) const BUNDLED: &[(&str, &str)] = &[ pub(crate) enum Check { /// `regexp`: a regex over file contents that must find zero hits. Regexp, + /// `comment_regexp`: the same regex, over the COMMENTS of a parsed file + /// rather than over its bytes. A separate check and not a `files.*` knob, + /// because it answers a different question about a different subject: a + /// pattern that must not appear anywhere is not the pattern that must not + /// appear in prose a reader is asked to trust. + CommentRegexp, + /// `trivial_comments`: a comment that says only what the code under it + /// already says. + TrivialComments, /// `forbidden_literals` / `forbidden_literals_from`: literals produced at /// runtime -- a machine's own identity, or a command's output -- each of /// which must appear nowhere in the selected files. @@ -101,8 +110,10 @@ pub(crate) enum Check { impl Check { /// Evaluation order, and the only enumeration of the checks anywhere. - pub(crate) const ALL: [Self; 9] = [ + pub(crate) const ALL: [Self; 11] = [ Self::Regexp, + Self::CommentRegexp, + Self::TrivialComments, Self::ForbiddenLiterals, Self::MaxLines, Self::PathRegexp, @@ -118,6 +129,8 @@ impl Check { pub(crate) const fn as_str(self) -> &'static str { match self { Self::Regexp => "regexp", + Self::CommentRegexp => "comment_regexp", + Self::TrivialComments => "trivial_comments", Self::ForbiddenLiterals => "forbidden_literals", Self::MaxLines => "max_lines", Self::PathRegexp => "path_regexp", @@ -139,6 +152,8 @@ impl Check { matches!( self, Self::Regexp + | Self::CommentRegexp + | Self::TrivialComments | Self::ForbiddenLiterals | Self::MaxLines | Self::PathRegexp @@ -310,6 +325,21 @@ pub(crate) struct Rule { /// A regex over file contents that must find zero hits. #[serde(default)] pub regexp: Option, + /// A regex over the COMMENTS of a parsed file. Same dialect as `regexp` and + /// a different subject: the text is the comment with its markers stripped, + /// so a pattern never has to know whether the language spells one `//` or + /// `#`, and `let marker = "// TODO";` is not a comment however it reads. + /// + /// Documentation comments are excluded. `///` is an artefact rustdoc + /// publishes, not a remark about the code, and a rule that cannot tell the + /// two apart is one whose fix deletes a public item's documentation. + #[serde(default)] + pub comment_regexp: Option, + /// A comment that contributes no word the code beneath it does not already + /// name. There is no pattern to write: the check compares the comment + /// against the identifiers and literals of the statement it introduces. + #[serde(default)] + pub trivial_comments: Option, /// A regex matched against tracked file PATHS rather than their contents. #[serde(default)] pub path_regexp: Option, @@ -471,6 +501,8 @@ impl Rule { id: id.to_owned(), message: None, regexp: None, + comment_regexp: None, + trivial_comments: None, path_regexp: None, require_regexp: None, max_lines: None, @@ -504,6 +536,8 @@ impl Rule { // would instead of quietly becoming a different check. match check { Check::Regexp => rule.regexp = Some(String::new()), + Check::CommentRegexp => rule.comment_regexp = Some(String::new()), + Check::TrivialComments => rule.trivial_comments = Some(true), Check::PathRegexp => rule.path_regexp = Some(String::new()), Check::RequireRegexp => rule.require_regexp = Some(String::new()), Check::MaxLines => rule.max_lines = Some(0), @@ -524,6 +558,12 @@ impl Rule { if self.regexp.is_some() { return Some(Check::Regexp); } + if self.comment_regexp.is_some() { + return Some(Check::CommentRegexp); + } + if self.trivial_comments.is_some() { + return Some(Check::TrivialComments); + } if self.path_regexp.is_some() { return Some(Check::PathRegexp); } @@ -707,6 +747,7 @@ impl Rule { pub(crate) fn expression(&self) -> Option<&str> { self.regexp .as_deref() + .or(self.comment_regexp.as_deref()) .or(self.path_regexp.as_deref()) .or(self.require_regexp.as_deref()) } @@ -730,6 +771,10 @@ impl Rule { fn validate(&self) -> Result<()> { let set: Vec<&str> = [ self.regexp.is_some().then_some("regexp"), + self.comment_regexp.is_some().then_some("comment_regexp"), + self.trivial_comments + .is_some() + .then_some("trivial_comments"), self.path_regexp.is_some().then_some("path_regexp"), self.require_regexp.is_some().then_some("require_regexp"), self.max_lines.is_some().then_some("max_lines"), @@ -751,8 +796,9 @@ impl Rule { let Some(check) = self.check() else { return Err(Fatal::new(format!( "rule {:?}: nothing says what it checks. Set one of: regexp, \ - path_regexp, require_regexp, max_lines, encoding, allowed_scripts, \ - forbidden_literals, forbidden_literals_from, builtin, exec", + comment_regexp, trivial_comments, path_regexp, require_regexp, \ + max_lines, encoding, allowed_scripts, forbidden_literals, \ + forbidden_literals_from, builtin, exec", self.id ))); }; @@ -780,6 +826,15 @@ impl Rule { "`forbidden_literals` names a built-in source and `forbidden_literals_from` \ names a command; one rule cannot have both", )?; + // Writing the field is what declares the check, so `false` is a rule + // that names a check and switches it off -- which reads as enforcement + // in `upheld.toml` and enforces nothing. Deleting the rule is the way to + // not run it. + self.refuse( + self.trivial_comments == Some(false), + "`trivial_comments = false` declares the check and then runs nothing. \ + Delete the rule instead, so no claim can name it", + )?; if check.requires_files() && self.files.is_none() { return Err(Fatal::new(format!( diff --git a/src/guard/unicode.rs b/src/guard/unicode.rs index 378498a..64c8ff1 100644 --- a/src/guard/unicode.rs +++ b/src/guard/unicode.rs @@ -553,8 +553,8 @@ mod tests { #[test] fn a_blob_that_is_not_text_is_told_apart_from_one_that_is_binary() { - // The direction that matters: an undecodable blob used to be skipped in - // silence, so a file nobody read was counted as a file with nothing in + // The direction that matters: an undecodable blob is `Unreadable` and + // not a skip, because a file nobody read is not a file with nothing in // it. Binary is the one honest skip -- there are no lines in it for a // codepoint to hide in. assert!(matches!(decode_for_scan(b"plain\n"), Decoded::Text(_))); diff --git a/src/main.rs b/src/main.rs index 9cca270..97aaf0a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,6 +24,7 @@ mod audit; mod catalog; mod check; +mod comments; mod config; mod engine; mod error; diff --git a/src/pins.rs b/src/pins.rs index 03fb696..a66a9f1 100644 --- a/src/pins.rs +++ b/src/pins.rs @@ -501,15 +501,14 @@ pub(crate) fn stale(request: &Request<'_>) -> Result> { })); } - // COULD NOT LOOK, which is exit 2, and it used to be exit 0. + // COULD NOT LOOK, which is exit 2 and never exit 0. // - // `remote_refs` returns `Ok(None)` for a remote it could not reach and says - // in a comment that this is never a pass -- and then the caller made it one. - // The pin went into `unchecked`, `unchecked` was printed to stderr and - // dropped, `stale` returned `Ok(None)`, and `guard::run` counted the guard - // among the ones that passed and exited 0. A network that was down, a token - // that had expired, a remote that had been renamed: every one of them read - // as a pin that was up to date. + // `remote_refs` returns `Ok(None)` for a remote it could not reach, and the + // whole weight of that answer rests here: a pin in `unchecked` has to reach + // the caller as a failure to establish anything. Report it to stderr and + // return `Ok(None)` instead and `guard::run` counts this guard among the + // ones that passed -- which turns a network that is down, a token that has + // expired and a remote nobody can resolve into a pin that is up to date. // // A `Fatal` rather than a `Refusal`, because this is not a violation: the // repository may be perfectly pinned. It is this run failing to establish diff --git a/src/scan.rs b/src/scan.rs index ffa85ae..44cd083 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -155,6 +155,8 @@ impl<'a> Scan<'a> { } failures.extend(match check { Check::Regexp => self.pattern_failures(rule)?, + Check::CommentRegexp => self.comment_pattern_failures(rule)?, + Check::TrivialComments => self.trivial_comment_failures(rule)?, Check::ForbiddenLiterals => self.literal_failures(rule)?, Check::MaxLines => self.size_failures(rule)?, Check::PathRegexp => self.path_failures(rule)?, @@ -226,6 +228,97 @@ impl<'a> Scan<'a> { Ok(failures) } + // -- comments ----------------------------------------------------------- + + /// Every comment in the files a rule selects, with the ones the language + /// cannot be read for left out. + /// + /// A selected file in a language no grammar here knows is skipped and not + /// reported: `files.include = ["src"]` on a mixed tree is a normal thing to + /// write, and a Markdown file under it is not an unreadable one. What IS + /// reported is a rule that selects nothing parseable at all, because that is + /// a rule whose author believes it runs. + fn comments_of(&self, rule: &Rule) -> Result> { + let files = self.select(rule)?; + let mut parsed = 0_usize; + let mut found = Vec::new(); + for file in &files { + let Some(language) = crate::comments::Language::for_path(file) else { + continue; + }; + let path = self.root.join(file); + let Ok(source) = std::fs::read_to_string(&path) else { + // Consistent with every other check here: a file that could not + // be read is recorded and the scan carries on, because a checker + // that skips what it could not open is claiming a tree it never + // examined. + self.unreadable + .borrow_mut() + .insert(format!("{}: could not be read as text", path.display())); + continue; + }; + parsed += 1; + for comment in crate::comments::collect(&source, language) { + found.push((file.clone(), comment)); + } + } + if parsed == 0 && !files.is_empty() { + return Err(Fatal::new(format!( + "rule {:?}: selects {} file(s) and none of them is Rust or Python, so the \ + check reads no comments at all. Narrow `files.glob` to the languages it \ + is meant for", + rule.id, + files.len() + ))); + } + Ok(found) + } + + fn comment_pattern_failures(&self, rule: &Rule) -> Result> { + let pattern = rule.expression().unwrap_or_default(); + let matcher = Regex::new(pattern) + .map_err(|error| Fatal::new(format!("rule {:?}: {error}", rule.id)))?; + let hits: Vec = self + .comments_of(rule)? + .into_iter() + .filter(|(_, comment)| !comment.doc && matcher.is_match(&comment.text)) + .map(|(path, comment)| Hit { + path, + line: Some(comment.line), + text: comment.text, + }) + .collect(); + if hits.is_empty() { + return Ok(Vec::new()); + } + Ok(vec![Failure::new( + &rule.id, + rule.message(), + body_for(&hits, self.redact()), + )]) + } + + fn trivial_comment_failures(&self, rule: &Rule) -> Result> { + let hits: Vec = self + .comments_of(rule)? + .into_iter() + .filter(|(_, comment)| crate::comments::is_trivial(comment)) + .map(|(path, comment)| Hit { + path, + line: Some(comment.line), + text: comment.text, + }) + .collect(); + if hits.is_empty() { + return Ok(Vec::new()); + } + Ok(vec![Failure::new( + &rule.id, + rule.message(), + body_for(&hits, self.redact()), + )]) + } + // -- forbidden literals ------------------------------------------------- fn literal_failures(&self, rule: &Rule) -> Result> { @@ -640,12 +733,12 @@ impl<'a> Scan<'a> { .map_err(|error| Fatal::at(&self.root.join(relative), error))?; let text = match String::from_utf8(bytes) { Ok(text) => text, - // A non-UTF-8 file used to be SILENTLY SKIPPED here -- a file - // nobody read, reported as clean, which is the - // `explicit-unknown` failure by name. Now: bytes an `encoding` - // rule declares are decoded under that declaration and their - // scripts read; bytes nothing declares are a file this check - // cannot look at, which is exit-2 territory, never a pass. + // A non-UTF-8 file is never silently skipped here: a file nobody + // read, reported as clean, is the `explicit-unknown` failure by + // name. Bytes an `encoding` rule declares are decoded under that + // declaration and their scripts read; bytes nothing declares are + // a file this check cannot look at, which is exit-2 territory + // and never a pass. Err(error) => { let raw = error.into_bytes(); let covering = declared_encodings diff --git a/src/selection.rs b/src/selection.rs index 288c932..0829bbb 100644 --- a/src/selection.rs +++ b/src/selection.rs @@ -447,11 +447,11 @@ fn by_walking(root: &Path, roots: &[PathBuf], overrides: &Override) -> (Vec unreadable.push(error.to_string()), Ok(entry) => { if !entry.file_type().is_some_and(|kind| kind.is_file()) { diff --git a/src/shim.rs b/src/shim.rs index 681d9d4..dea08c8 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -753,10 +753,10 @@ fn consult(root: &Path, rule: &Rule, subject: &Subject) -> Result "{} refused a {} subject: {report}", rule.id, subject.kind ))), - // The write result used to be dropped with `.ok()`, and a subject that - // never arrived is the one case where a 0 means nothing at all: the - // checker approved whatever part of it got through, which is not what - // this invocation is about to publish. + // The write result is an answer, not something to drop with `.ok()`: a + // subject that never arrived is the one case where a 0 means nothing at + // all, because the checker approved whatever part of it got through and + // that is not what this invocation is about to publish. (_, Err(error)) => Err(Fatal::new(format!( "{} did not take the whole {} subject ({error}), so its answer is not about what \ would be published: {report}", @@ -1178,11 +1178,10 @@ pub(crate) fn run( }; }; - // Only the rules that name THIS command line. A checker used to be - // consulted by every shim -- a check written for a pull-request body was - // also asked about a branch name on `git push` and a tarball on `npm - // publish` -- because the only thing selecting it was `kind = "command"`, - // which says nothing about which command. + // Only the rules that name THIS command line. Selecting a checker by + // anything coarser -- a `kind` saying it stands in front of some command, + // without saying which -- asks a check written for a pull-request body + // about a branch name on `git push` and a tarball on `npm publish`. // Two kinds stand in front of a command, and both are scoped by the same // `command.before`. An `exec` checker is a program this repository names. // A text-capable BUILT-IN is one the binary already carries -- and a diff --git a/tests/config_cli.rs b/tests/config_cli.rs new file mode 100644 index 0000000..1be1b1c --- /dev/null +++ b/tests/config_cli.rs @@ -0,0 +1,255 @@ +//! CLI-level tests for the load-time refusals in `config::validate`. +//! +//! The unit tests beside `config::load` call the loader directly, which proves +//! the loader refuses and proves nothing about whether anything reaches the +//! loader. Every refusal here exists to stop a seam from running with nothing +//! checked, so the fact under test is the seam's: `uphold shim` must die on a +//! policy that declares a shim no checker names rather than exec through to the +//! command, and a rule whose only declared place is one no seam reads must stop +//! the binary at whichever entry point loaded it. +//! +//! That is the half a direct call to `load` cannot see. A future entry point +//! that skipped `config::load`, or a `shim_command` that treated a policy error +//! as "nothing to stand in front of this" and exec'd anyway, would leave every +//! unit test green while the command ran unchecked -- which is the exact shape +//! these refusals were written to make impossible. + +#![expect( + clippy::let_underscore_must_use, + clippy::tests_outside_test_module, + clippy::unwrap_used, + reason = "A CLI test asserts on the outcome; a panic in the harness that builds the fixture IS the failure report, and there is no caller to hand a Result to" +)] + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A repository whose policy is exactly `policy`, with a stub `faux` on PATH. +/// +/// The stub is what makes the shim cases falsifiable. A refusal that arrives as +/// a non-zero exit proves little on its own -- the command could have run and +/// failed -- so the stub announces itself on stdout, and "the command did not +/// run" is then something a test can assert rather than assume. +fn workspace(policy: &str) -> PathBuf { + static NEXT: AtomicUsize = AtomicUsize::new(0); + let root = std::env::temp_dir().join(format!( + "uphold-config-cli-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join("policy")).unwrap(); + std::fs::create_dir_all(root.join("bin")).unwrap(); + std::fs::write(root.join("policy/principles.toml"), policy).unwrap(); + + // The checker in the accepted policy below is this binary consulting + // itself, so it has to be on PATH under its own name for the multicall + // entry to dispatch it. + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), root.join("bin/uphold")).unwrap(); + + let stub = root.join("bin/faux"); + std::fs::write(&stub, "#!/bin/sh\necho \"faux ran: $*\"\n").unwrap(); + let mut permissions = std::fs::metadata(&stub).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o755); + std::fs::set_permissions(&stub, permissions).unwrap(); + + // `discover` walks up until a repository root, so the fixture has to be + // one; otherwise it climbs out of the temporary directory and finds + // whatever policy the machine running the suite happens to carry. + Command::new("git") + .args(["init", "-q", "-b", "main"]) + .current_dir(&root) + .stdout(Stdio::null()) + .status() + .unwrap(); + root +} + +fn uphold(root: &Path, args: &[&str]) -> Output { + let path = format!( + "{}:{}", + root.join("bin").display(), + std::env::var("PATH").unwrap_or_default() + ); + Command::new(env!("CARGO_BIN_EXE_uphold")) + .args(args) + .current_dir(root) + .env("PATH", path) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap() +} + +fn code(output: &Output) -> i32 { + output.status.code().unwrap() +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +/// The policy every accepted case here is a variation of: one checker, one +/// shim, each naming the other. +const PAIRED: &str = r#" +[rule.no-published-markers] +message = "remove the marker" +exec = "uphold guard --text -" + +[rule.no-published-markers.command] +before = ["faux"] + +[[shim]] +command = "faux" +match = ["pr:create"] +text_flags = ["-t", "--title"] +scope = "always" +"#; + +/// The rule under both cases below: a built-in that reads a push range, whose +/// only declared place is one no shim consults. +const PUSH_GUARD_IN_FRONT_OF_A_COMMAND: &str = r#" +[rule.push] +builtin = "prevent-public-push" + +[rule.push.command] +before = ["faux"] + +[[shim]] +command = "faux" +match = ["pr:create"] +text_flags = ["-t"] +scope = "always" +"#; + +/// `command.before` on a check no shim consults, refused by the real binary. +/// +/// `shim::run` filters the rules it consults to `exec` checkers and +/// text-capable built-ins, so a built-in that reads a push range and declares +/// `command.before` and nothing else is consulted by nothing and runs nowhere +/// -- and the "nothing says where it runs" refusal is satisfied by the very +/// field that cannot be used, so the check meant to catch a rule with no place +/// is the one this rule walks past. +#[test] +fn a_command_place_no_seam_reads_stops_the_binary() { + let root = workspace(PUSH_GUARD_IN_FRONT_OF_A_COMMAND); + // Asked of an entry point that only loads and prints, so what fails is the + // load and not a check downstream of it. + let output = uphold(&root, &["rules", "--effective"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("read by nothing"), "{text}"); + assert!(text.contains("push"), "{text}"); + // The refusal names the seams the rule COULD have declared, so the cure is + // in the message rather than in a document the reader has to go and find. + assert!(text.contains("git.hooks"), "{text}"); +} + +/// The same rule, and the seam it would have run at is silent about it. +/// +/// This is the half the loader exists for. `uphold shim` collects the subjects +/// of the invocation, consults the rules that stand in front of the command, +/// finds none -- because the built-in is not one `shim::run` consults -- and +/// execs. Nothing on that path can report the rule, so the only place it can be +/// said is at load, and this asserts the binary says it there instead of +/// running the command. +#[test] +fn the_shim_refuses_rather_than_running_a_command_that_rule_could_not_guard() { + let root = workspace(PUSH_GUARD_IN_FRONT_OF_A_COMMAND); + let output = uphold(&root, &["shim", "faux", "pr", "create", "-t", "A title"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "the command ran under a policy that could not be loaded: {}", + stdout(&output) + ); + assert!( + stderr(&output).contains("read by nothing"), + "{}", + stderr(&output) + ); +} + +/// A `[[shim]]` no checker names, refused before the command it fronts runs. +/// +/// The failure it prevents is the worst shape this tool has: the shim collects +/// the pull-request body, iterates an empty list of checkers, refuses nothing +/// and execs -- a publication that passed because nothing looked at it, +/// reported as a pass. So the assertion is not merely that the exit is 2 but +/// that the stub never printed. +#[test] +fn a_shim_named_by_no_checker_never_reaches_the_command() { + let root = workspace( + r#" + [[shim]] + command = "faux" + match = ["pr:create"] + text_flags = ["-t"] + scope = "always" + "#, + ); + let output = uphold(&root, &["shim", "faux", "pr", "create", "-t", "A title"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "an unchecked invocation reached the command: {}", + stdout(&output) + ); + let text = stderr(&output); + assert!(text.contains("named by no checker"), "{text}"); + assert!(text.contains("faux"), "{text}"); +} + +/// And the mirror: the shim is the only thing that invokes a checker, so a +/// `command.before` naming a command no `[[shim]]` declares is a rule that runs +/// nowhere -- which reads exactly like a rule that passes. +#[test] +fn a_checker_naming_a_command_no_shim_declares_stops_the_binary() { + let root = workspace( + r#" + [rule.no-published-markers] + message = "remove the marker" + exec = "uphold guard --text -" + + [rule.no-published-markers.command] + before = ["faux", "glab mr create"] + + [[shim]] + command = "faux" + match = ["pr:create"] + text_flags = ["-t"] + scope = "always" + "#, + ); + let output = uphold(&root, &["rules", "--effective"]); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("glab"), "{text}"); + assert!(text.contains("no `[[shim]]` declares"), "{text}"); +} + +/// The control, without which every assertion above passes for a policy that +/// simply does not parse. +/// +/// A paired checker and shim loads, and the shim runs the command it fronts -- +/// the half that is easy to lose, because a loader that refused everything +/// would satisfy each refusal test in this file. +#[test] +fn a_checker_and_the_shim_that_invokes_it_load_and_run() { + let root = workspace(PAIRED); + let listed = uphold(&root, &["rules", "--effective"]); + assert_eq!(code(&listed), 0, "{}", stderr(&listed)); + assert!( + stdout(&listed).contains("no-published-markers"), + "{}", + stdout(&listed) + ); + + let ran = uphold(&root, &["shim", "faux", "pr", "create", "-t", "A title"]); + assert_eq!(code(&ran), 0, "{}", stderr(&ran)); + assert!(stdout(&ran).contains("faux ran:"), "{}", stdout(&ran)); +} diff --git a/uphold_check.py b/uphold_check.py index 2c66b70..c87ed49 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -61,11 +61,11 @@ HERE = Path(__file__).resolve().parent DECLARATION_RELPATH = Path("policy") / "upheld.toml" -# The declaration is the only file this still reads out of a consumer's tree, -# and `--oscal` is the only mode that reads it. Everything that used to be here -# -- the pre-commit and lefthook scanners, the published-id table, the content -# policy reader -- answered "which rules run here", and `uphold check` answers -# that now, out of the loader that decides it. +# The declaration is the only file this reads out of a consumer's tree, and +# `--oscal` is the only mode that reads it. Anything answering "which rules run +# here" -- the pre-commit and lefthook scanners, the published-id table, the +# content policy reader -- belongs to `uphold check` instead, out of the loader +# that decides it, rather than to a second reader here that can disagree. class Refused(Exception):