From 3609d9c833ca7ae6c942882af73f736636ad775a Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 21 Aug 2026 09:59:45 +0900 Subject: [PATCH] Let a baseline entry say who excused it and why #71 asked where a condition a matcher finds exactly but cannot judge should live, and proposed a new `disposition` on `Rule` that would emit a review finding at exit 0. Building it turned up `files.baseline`, which already does the mechanical half: path-scoped suppression, plus a stale-entry finding when an entry stops describing the tree. Shipping a second suppression concept beside that is the duplication #50 measured across 77 repositories, so the proposal is withdrawn and this is what was missing instead. WHAT A BASELINE COULD NOT SAY. It holds two different things and the format could express one: debt eight modules awaiting the same migration. One reason at the top of the file covers every entry, and the header is the right place exceptions the places a rule is simply wrong. `.ljust(` building a five-column table should take the declared dependency; `.ljust(` building a two-column key/value list is correct and a table reads worse. No pattern separates those For the second, the entry excusing it has to carry the judgement -- and the whole line was the path, so the judgement had nowhere to go but a comment nothing associates with an entry. An entry may now be signed: src/cli/top.py | alice | a two-column key/value list; a table reads worse `|` as the separator, because whitespace already separates the size baseline's count and a path may hold it, and `#` at line start already means a comment. `baselines_signed = true` at the top of the policy requires it. Off by default, which is not neutrality -- it is what every existing baseline file already is, and the two in this fleet stay green. A repository turns it on when its baselines stop being one homogeneous debt. Policy-level rather than per-rule, because "may an exemption be anonymous" is one answer a repository gives once; per rule it would be a setting every new baseline has to remember, which is the same as not having it. Unsigned entries report at the tier stale ones do, and for the same reason: both are a baseline that has stopped recording a decision somebody made. A signature is an addition to that record and not a way out of it -- a signed entry still goes stale, which is its own test. 554 tests pass, clippy and fmt clean, every lefthook pre-commit command run directly. --- docs/REFERENCE.md | 38 ++++++++++++++++ src/config.rs | 21 +++++++++ src/scan.rs | 114 +++++++++++++++++++++++++++++++++++++++++----- tests/scan_cli.rs | 107 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 12 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 103ca66..c7e90af 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -72,6 +72,44 @@ every git hook, and a repository whose own prose cites its issues would have every one of those citations refused — so the seam it belongs at is the command that publishes text to a forge, and only that one. +### A baseline entry may be asked to say who excused it and why + +`files.baseline` names a file of repository-relative paths a rule excuses, and +an entry that no longer matches is reported as stale -- an exemption that has +stopped describing the tree is the rule switched off for that path. + +A baseline holds two different things and the format could only express one: + +* **debt** -- eight modules awaiting the same migration. One reason at the top + of the file covers every entry, and the file's header is the right place for + it. +* **exceptions** -- the places a rule is simply wrong. `.ljust(` building a + five-column table should take the dependency; `.ljust(` building a two-column + key/value list is correct and a table would read worse. No pattern separates + those, so the entry excusing the second has to carry the judgement, and the + whole line was the path. + +So an entry may be signed: + +```text +# the places this rule is wrong +src/cli/top.py | alice | a two-column key/value list; a table reads worse +``` + +`path | owner | reason`, with `|` as the separator -- whitespace already +separates the size baseline's count and a path may hold it, and `#` at line +start already means a comment. + +Set `baselines_signed = true` at the top of the policy to require it. Off by +default, which is what every existing baseline file already is; a repository +turns it on when its baselines stop being one homogeneous debt. Unsigned +entries are then reported at the same tier as stale ones, and for the same +reason: both are a baseline that has stopped recording a decision somebody made. + +A signature is an addition to the record and not a way out of it. A reason says +why an entry is there; it says nothing about whether it still needs to be, so a +signed entry still goes stale. + ### A rule may not be about its own declaration A policy file is a tracked file, so a rule's `regexp` and `require_regexp` are diff --git a/src/config.rs b/src/config.rs index 46e75d7..a99079d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1480,6 +1480,23 @@ pub(crate) struct PolicyFile { pub rules: BTreeMap, #[serde(default, rename = "shim")] pub shims: Vec, + /// Whether every path-baseline entry must say who excused it and why. + /// + /// Off by default, and the default is not neutrality -- it is what every + /// existing baseline file already is. A repository turns it on when its + /// baselines stop being one homogeneous debt with a header explaining all + /// of it, and start holding entries that differ from each other. The + /// `.ljust(` case is the shape: one call site should take the dependency + /// and another is correct as it stands, and no pattern separates them, so + /// the entry that excuses the second has to carry the judgement or the + /// judgement is nowhere. + /// + /// Policy-level rather than per-rule, because "may an exemption be + /// anonymous" is one answer a repository gives once. Per-rule it would be a + /// setting every new baseline has to remember, which is the same as not + /// having it. + #[serde(default)] + pub baselines_signed: bool, } /// The ceiling on what one bundled set may install, declared in the set. @@ -1558,6 +1575,9 @@ pub(crate) struct Policy { /// exit 2. See [`PolicyFile::private_owners_optional`]. pub private_owners_optional: bool, pub redact_matches: bool, + /// Whether every path-baseline entry must be signed. See + /// [`PolicyFile::baselines_signed`]. + pub baselines_signed: bool, pub allowed_scripts: Vec, pub rules: Vec, pub shims: Vec, @@ -2084,6 +2104,7 @@ pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { private_owners_from: file.private_owners_from.clone(), private_owners_optional: file.private_owners_optional, redact_matches: file.redact_matches, + baselines_signed: file.baselines_signed, allowed_scripts: file.allowed_scripts, rules, shims: file.shims, diff --git a/src/scan.rs b/src/scan.rs index 9d1889c..5d9bd04 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -240,9 +240,10 @@ impl<'a> Scan<'a> { .filter(|path| baseline.contains(path)) .collect(); hits.retain(|hit| !baseline.contains(normalize_rel(&hit.path))); + failures.extend(baseline.unsigned_failure(rule, self.policy.baselines_signed)); failures.extend(stale_baseline_failure( rule, - &baseline, + &baseline.paths, &seen, STALE_BASELINE, )); @@ -492,9 +493,10 @@ impl<'a> Scan<'a> { .filter(|path| baseline.contains(path)) .collect(); hits.retain(|hit| !baseline.contains(normalize_rel(&hit.path))); + failures.extend(baseline.unsigned_failure(rule, self.policy.baselines_signed)); failures.extend(stale_baseline_failure( rule, - &baseline, + &baseline.paths, &seen, STALE_BASELINE, )); @@ -540,9 +542,10 @@ impl<'a> Scan<'a> { .iter() .map(|path| normalize_rel(path).to_owned()) .collect(); + failures.extend(baseline.unsigned_failure(rule, self.policy.baselines_signed)); failures.extend(stale_baseline_failure( rule, - &baseline, + &baseline.paths, &still_missing, STALE_REQUIRE_BASELINE, )); @@ -1156,24 +1159,58 @@ impl<'a> Scan<'a> { .collect() } - /// A path-only baseline: one repository-relative path per line. + /// A path-only baseline: one repository-relative path per line, optionally + /// signed. /// /// Paths rather than counts, deliberately. A count baseline is stricter, but /// a reformat moves a match count without anything real changing, and a rule /// whose baseline churns on unrelated edits is one people stop reading. A /// listed path may get worse internally; what it cannot do is let a NEW path /// start. - fn load_path_baseline(&self, relative: Option<&str>) -> Result> { + /// + /// A line may carry a signature after the path: + /// + /// ```text + /// src/cli/top.py | alice | a two-column key/value list; a table reads worse + /// ``` + /// + /// Optional here and required where the policy says so, because the two + /// things a baseline holds are not the same. A file listing eight modules + /// that have not been migrated yet needs one reason at the top and none per + /// line -- every entry is the same debt and the file's header says so. A + /// file listing the places a rule is WRONG needs one reason per line, and + /// nothing in the format could say it: the whole line was the path, so a + /// judgement about why this instance is the exception had nowhere to go + /// except a comment nothing associates with an entry. + /// + /// The separator is `|` rather than whitespace or `#`. Whitespace is the + /// size baseline's separator and a path may hold it; `#` at line start + /// already means a comment and overloading it would change what an existing + /// file means. + fn load_path_baseline(&self, relative: Option<&str>) -> Result { let Some(relative) = relative else { - return Ok(BTreeSet::new()); + return Ok(Baseline::default()); }; let text = crate::error::read_to_string(&self.root.join(relative))?; - Ok(text - .lines() - .map(str::trim) - .filter(|line| !line.is_empty() && !line.starts_with('#')) - .map(|line| normalize_rel(line).to_owned()) - .collect()) + let mut baseline = Baseline { + file: relative.to_owned(), + ..Baseline::default() + }; + for line in text.lines() { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + let mut parts = line.split('|').map(str::trim); + let path = normalize_rel(parts.next().unwrap_or_default()).to_owned(); + let owner = parts.next().unwrap_or_default(); + let reason = parts.next().unwrap_or_default(); + if owner.is_empty() || reason.is_empty() { + baseline.unsigned.push(path.clone()); + } + baseline.paths.insert(path); + } + Ok(baseline) } fn load_size_baseline(&self, relative: Option<&str>) -> Result> { @@ -1199,6 +1236,59 @@ impl<'a> Scan<'a> { } } +/// One rule's baseline: the paths it excuses, and which of them are unsigned. +/// +/// `file` is carried so a finding can name the file to edit. A report that says +/// "three entries are unsigned" and not where they are is a report whose reader +/// has to go looking for the thing it just read. +#[derive(Debug, Default)] +struct Baseline { + file: String, + paths: BTreeSet, + unsigned: Vec, +} + +impl Baseline { + fn is_empty(&self) -> bool { + self.paths.is_empty() + } + + fn contains(&self, path: &str) -> bool { + self.paths.contains(path) + } + + /// The entries carrying no owner and reason, when the policy asks for them. + /// + /// A finding rather than a load refusal, because a baseline is read here + /// and not at load: the path comes from the rule and the file from the + /// tree, and neither exists as text the loader has seen. It sits at the + /// same tier as a stale entry for the same reason -- both are a baseline + /// that has stopped describing a decision somebody made. + fn unsigned_failure(&self, rule: &Rule, required: bool) -> Vec { + if !required || self.unsigned.is_empty() { + return Vec::new(); + } + let body = self + .unsigned + .iter() + .map(|path| format!("{path}: no owner and reason")) + .collect::>() + .join("\n"); + vec![Failure::new( + format!("{} (unsigned baseline)", rule.id), + format!( + "This policy requires every baseline entry to say who excused it and why, and \ + these say neither. Write them as `path | owner | reason` in {}.\n\nA path on \ + its own records that a rule was switched off there and nothing about the \ + judgement behind it -- which is the difference between debt somebody is \ + carrying and a finding somebody silenced.", + self.file + ), + body, + )] + } +} + fn stale_baseline_failure( rule: &Rule, baseline: &BTreeSet, diff --git a/tests/scan_cli.rs b/tests/scan_cli.rs index b9074d7..6fe9186 100644 --- a/tests/scan_cli.rs +++ b/tests/scan_cli.rs @@ -482,6 +482,113 @@ fn a_baselined_path_is_allowed_and_a_new_one_is_not() { assert!(!text.contains("old.txt:"), "{text}"); } +#[test] +fn an_unsigned_baseline_entry_is_reported_where_the_policy_asks_for_signatures() { + // A baseline holds two different things and the format could only express + // one. Eight modules awaiting the same migration need one reason at the top + // of the file. A list of the places a rule is WRONG needs one reason per + // line -- and until now the whole line was the path, so the judgement had + // nowhere to go but a comment nothing associates with an entry. + let root = workspace(); + write( + &root, + "policy/principles.toml", + r#" + baselines_signed = true + + [rule.no-todo] + message = "no TODO" + regexp = 'TODO' + + [rule.no-todo.files] + exclude = ["policy/**"] + baseline = "policy/todo-baseline.txt" +"#, + ); + write( + &root, + "policy/todo-baseline.txt", + "# grandfathered\nold.txt\n", + ); + write(&root, "old.txt", "TODO: ancient\n"); + + let output = scan(&root); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("no-todo (unsigned baseline)"), "{text}"); + assert!(text.contains("old.txt: no owner and reason"), "{text}"); + // It names the file to edit: a report that says three entries are unsigned + // and not where they are sends its reader looking. + assert!(text.contains("policy/todo-baseline.txt"), "{text}"); + + // Signed, it passes -- and the entry still suppresses what it excused. + write( + &root, + "policy/todo-baseline.txt", + "# grandfathered\nold.txt | alice | the tracker this cites was closed; text stays\n", + ); + assert_eq!(code(&scan(&root)), 0); +} + +#[test] +fn an_unsigned_baseline_is_fine_where_the_policy_does_not_ask() { + // The default is not neutrality, it is what every existing baseline file + // already is. Turning this on is a repository saying its baselines have + // stopped being one homogeneous debt. + let root = workspace(); + write( + &root, + "policy/principles.toml", + r#" + [rule.no-todo] + message = "no TODO" + regexp = 'TODO' + + [rule.no-todo.files] + exclude = ["policy/**"] + baseline = "policy/todo-baseline.txt" +"#, + ); + write(&root, "policy/todo-baseline.txt", "old.txt\n"); + write(&root, "old.txt", "TODO: ancient\n"); + assert_eq!(code(&scan(&root)), 0); +} + +#[test] +fn a_signed_entry_still_goes_stale_when_it_stops_describing_the_tree() { + // The signature is an addition to the record, not a way out of it. A + // reason explains why an entry is there; it says nothing about whether it + // still needs to be. + let root = workspace(); + write( + &root, + "policy/principles.toml", + r#" + baselines_signed = true + + [rule.no-todo] + message = "no TODO" + regexp = 'TODO' + + [rule.no-todo.files] + exclude = ["policy/**"] + baseline = "policy/todo-baseline.txt" +"#, + ); + write( + &root, + "policy/todo-baseline.txt", + "paid.txt | alice | pre-existing, being migrated\n", + ); + write(&root, "paid.txt", "clean now\n"); + + let output = scan(&root); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + let text = stderr(&output); + assert!(text.contains("no-todo (stale baseline)"), "{text}"); + assert!(!text.contains("unsigned baseline"), "{text}"); +} + #[test] fn a_baseline_entry_that_no_longer_matches_is_reported_as_stale() { let root = workspace();