diff --git a/batten.toml b/batten.toml index 021c498c2..0b8843c6f 100644 --- a/batten.toml +++ b/batten.toml @@ -2613,6 +2613,44 @@ no_fix_reason = "delete the literal; a consumer fact belongs in that consumer's # WHERE THE VOCABULARY GOES INSTEAD is `[[pattern]]`, which exists for exactly # this and whose own module doc says so: a tracker key "is a *consumer* # identifier … written here it is where consumer facts belong". +# THE TWENTY-FIRST COPY (CLOUD-1142). `no-tracker-key-in-core` below refuses a +# derivation outright, and can, because `crates/**` carries none — the crate owns +# the three AXES (`ready::Grammar::key_of` and `keys_in`) and never the token. +# The task tree is the other half and cannot be refused outright: CLOUD-761 +# measured twenty derivations there in nine spellings, and 34 occurrences across +# 12 programs survive today. A `forbid` would fire on every one of them. +# +# SO IT RATCHETS RATHER THAN FORBIDS, which is the whole of what is landable here. +# Converting those sites means editing governed `mise-tasks/` programs, which +# `V-SHELL-RULE-EDITED` refuses with one route — retire them — and that is +# CLOUD-761's remaining half, blocked on the retirement campaign's sequencing. +# What this row buys meanwhile is the property whose absence is how twenty +# arrived: the count cannot GROW. A twenty-first copy fails; the existing ones +# wait for their program's retirement, each taking its derivation with it. +# +# The direction is also why the row needs no exemption for the authority. A +# ratchet counts rather than judges, so `batten.toml`'s own rows are simply not in +# this glob, and `policy/**` carries none at all because a module reads +# `data.batten.patterns["ready-issue-key"]` by id — the registry doing its job. +[[rule]] +id = "issue-key-derivations-not-growing" +kind = "ratchet" +glob = "mise-tasks/**" +# A LITERAL SUBSTRING, NOT A REGEX, and the distinction is a dead gate away. +# `ratchet_rule` counts with `text.matches(pattern)`, which is `str::matches` — +# substring, not expression. The first draft of this row wrote the escaped regex +# form and counted ZERO at both ends, so the ratchet loaded, ran, and could never +# fire. It was caught by driving a twenty-first derivation through it rather than +# by reading, which is the only thing that tells a passing gate from an absent +# one. `forbid` above takes `regex` and this takes `pattern`: two columns, two +# languages, one file. +pattern = 'CLOUD-[0-9]' +direction = "non_increasing" +base = "origin/main" +severity = "deny" +scope = "tree" +no_fix_reason = "read the key from `[[pattern]] ready-issue-key`, or let the program's retirement carry its derivation away; a twenty-first spelling is how the first twenty arrived" + [[rule]] id = "no-tracker-key-in-core" kind = "forbid" diff --git a/crates/batten/src/claim.rs b/crates/batten/src/claim.rs index 1cab08690..8d868037a 100644 --- a/crates/batten/src/claim.rs +++ b/crates/batten/src/claim.rs @@ -601,11 +601,40 @@ pub fn mint( base: Option<&str>, claimed_at: &str, ) -> Result { + let dest = receipts.join(receipt_name(branch)); + + // WHAT THIS BRANCH ALREADY CLAIMED, CARRIED FORWARD (CLOUD-1231). `mint` has + // always written line 1 as an id LIST, so a receipt holding several keys is + // the shape this file was built for — but every invocation wrote a fresh one, + // so claiming a second row on one branch silently discarded the first row's + // record. Measured on CLOUD-1295's branch: re-claiming would have dropped the + // `weakens` lines `config lint`'s groomed half reads, which is the difference + // between a landable branch and an unexplainable refusal, and the reason that + // work had to move to a branch of its own. + // + // A branch legitimately serves several rows — `closing-key-check` expects a + // body to close several — so the union is the honest record rather than a + // convenience. + // + // ONLY WHEN THE BASE AGREES. CLOUD-516's restart case is exactly a receipt + // that outlived the branch it described: `git checkout -B origin/main` + // discards the commits and keeps the filename. Carrying ids across that would + // let a restarted branch inherit claims for work it no longer holds, which is + // the defect that row exists to close. A differing or unreadable base + // therefore REPLACES rather than merges — the direction that forgets rather + // than the one that over-claims. + let carried = carried_claim(&dest, base); + let mut body = String::new(); // LINE 1 IS THE ID LIST, exactly where it has always been, so any reader that // did parse it still finds it. Everything below is read BY KEY for the same // reason: a line added here must not move one somebody else counts on. - let ids: Vec<&str> = issues.iter().map(|issue| issue.id.as_str()).collect(); + let mut ids: Vec = carried.ids.clone(); + for issue in issues { + if !ids.iter().any(|id| id == &issue.id) { + ids.push(issue.id.clone()); + } + } body.push_str(&ids.join(" ")); body.push('\n'); if request.bypass_sequence { @@ -636,6 +665,7 @@ pub fn mint( // "could not look", which falls back to the trailer. That is decided by the // file's existence rather than by this loop writing zero lines, so nothing // here needs a placeholder. + let mut weakens: Vec = carried.weakens.clone(); for issue in issues { for pair in issue .description @@ -643,9 +673,15 @@ pub fn mint( .map(admitted_weakenings) .unwrap_or_default() { - writeln!(body, "weakens {} {pair}", issue.id)?; + let line = format!("weakens {} {pair}", issue.id); + if !weakens.contains(&line) { + weakens.push(line); + } } } + for line in &weakens { + writeln!(body, "{line}")?; + } writeln!(body, "claimed-at {claimed_at}")?; // THE BASE THIS CLAIM WAS MADE AGAINST (CLOUD-516). A branch NAME outlives the // branch it described — `git checkout -B origin/main` discards the @@ -661,7 +697,6 @@ pub fn mint( // only record and it names something that no longer exists. writeln!(body, "branch {branch}")?; - let dest = receipts.join(receipt_name(branch)); std::fs::create_dir_all(receipts) .and_then(|()| std::fs::write(&dest, body)) .map_err(|_| { @@ -673,6 +708,53 @@ pub fn mint( Ok(dest) } +/// What a prior claim on this branch still says, when it is still about this +/// branch (CLOUD-1231). +#[derive(Default)] +struct Carried { + /// The ids line 1 already named. + ids: Vec, + /// The `weakens` lines already recorded, verbatim. + weakens: Vec, +} + +/// Read the receipt already at `dest`, if its recorded base matches `base`. +/// +/// **Every could-not-look answers with nothing carried**, which is the direction +/// that forgets: an unreadable file, an empty one, a receipt whose `base` line is +/// absent or differs, or a run whose own base did not resolve. Carrying on a +/// doubtful match would let a restarted branch inherit a claim for work it no +/// longer holds, and that is CLOUD-516's defect rather than this one's fix. +fn carried_claim(dest: &Path, base: Option<&str>) -> Carried { + let Some(base) = base else { + return Carried::default(); + }; + let Ok(text) = std::fs::read_to_string(dest) else { + return Carried::default(); + }; + let recorded = text + .lines() + .find_map(|line| line.strip_prefix("base ")) + .map(str::trim); + if recorded != Some(base) { + return Carried::default(); + } + Carried { + ids: text + .lines() + .next() + .unwrap_or_default() + .split_whitespace() + .map(str::to_owned) + .collect(), + weakens: text + .lines() + .filter(|line| line.starts_with("weakens ")) + .map(str::to_owned) + .collect(), + } +} + /// A stranded receipt this branch may adopt. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Orphan { @@ -977,3 +1059,76 @@ mod tests { ); } } + +/// CLOUD-1231: a branch serves several rows, so its receipt records several. +#[cfg(test)] +mod carried_claim_tests { + use super::{Carried, carried_claim}; + + fn write(dir: &std::path::Path, body: &str) -> std::path::PathBuf { + let dest = dir.join("claim.branch"); + let Ok(()) = std::fs::write(&dest, body) else { + panic!("write the fixture receipt") + }; + dest + } + + fn scratch(name: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("batten-carried-{name}")); + let _ = std::fs::remove_dir_all(&dir); + let Ok(()) = std::fs::create_dir_all(&dir) else { + panic!("create the fixture root") + }; + dir + } + + #[test] + fn a_prior_claim_on_the_same_base_is_carried() { + // The positive arm, and the defect this closes: claiming a second row on + // one branch used to discard the first row's record entirely, including + // the `weakens` lines `config lint`'s groomed half reads. + let dir = scratch("same-base"); + let dest = write( + &dir, + "AAA-1 AAA-2\nready-lint pass\nweakens AAA-1 smell key\nbase deadbeef\nbranch b\n", + ); + let carried = carried_claim(&dest, Some("deadbeef")); + assert_eq!(carried.ids, vec!["AAA-1".to_owned(), "AAA-2".to_owned()]); + assert_eq!(carried.weakens, vec!["weakens AAA-1 smell key".to_owned()]); + } + + #[test] + fn a_prior_claim_on_a_different_base_is_forgotten() { + // CLOUD-516's restart case, and the reason this merges conditionally + // rather than always. `git checkout -B origin/main` discards the + // commits and keeps the filename, so carrying ids across it would let a + // restarted branch inherit claims for work it no longer holds. + let dir = scratch("moved-base"); + let dest = write(&dir, "AAA-1\nbase deadbeef\nbranch b\n"); + assert!(carried_claim(&dest, Some("cafe")).ids.is_empty()); + } + + #[test] + fn every_could_not_look_carries_nothing() { + // The direction that forgets. An absent file, a receipt with no `base` + // line, and a run whose own base did not resolve are all doubtful + // matches, and over-claiming on a doubt is the failure CLOUD-516 records. + let dir = scratch("could-not-look"); + assert!( + carried_claim(&dir.join("claim.absent"), Some("deadbeef")) + .ids + .is_empty() + ); + let no_base = write(&dir, "AAA-1\nready-lint pass\nbranch b\n"); + assert!(carried_claim(&no_base, Some("deadbeef")).ids.is_empty()); + assert!(carried_claim(&no_base, None).ids.is_empty()); + } + + #[test] + fn the_default_carries_nothing() { + // The anti-vacuity mirror: a `Carried` that arrived populated by default + // would make every case above pass without reading a file at all. + assert!(Carried::default().ids.is_empty()); + assert!(Carried::default().weakens.is_empty()); + } +} diff --git a/crates/batten/src/ready.rs b/crates/batten/src/ready.rs index e1f13f040..c9e459dd3 100644 --- a/crates/batten/src/ready.rs +++ b/crates/batten/src/ready.rs @@ -477,21 +477,140 @@ fn compiled(pattern: &str) -> Regex { }) } -/// The issue keys in a span, deduped and ordered NUMERICALLY. +/// THE ONE DEFINITION OF AN ISSUE KEY (CLOUD-1142). /// -/// Numeric and not a bare sort, for `graph-check`'s reason: `CLOUD-10` sorts -/// before `CLOUD-9` lexically, so a caller diffing two runs could not tell an -/// ordering change from a content one. -fn keys_in(grammar: &Grammar, text: &str) -> Vec { - let found: BTreeSet<&str> = grammar.key.find_iter(text).map(|m| m.as_str()).collect(); - let mut keys: Vec = found.into_iter().map(str::to_owned).collect(); - keys.sort_by_key(|k| { - k.rsplit('-') +/// # Why the grammar is here and the vocabulary is not +/// +/// CLOUD-761 measured twenty independent derivations of the key pattern across +/// nine spellings, diverged on three axes, with a shipped defect behind them: a +/// body writing the lowercase form is accepted by one gate and invisible to two +/// others. This is the definition those sites are meant to converge on. +/// +/// **The token itself is never written here.** It is the consumer's, read from +/// the `[[pattern]]` registry as [`Grammar::key`], and `no-tracker-key-in-core` +/// refuses a derivation of it anywhere under `crates/**` — the mechanism that +/// exists because CLOUD-1121 carried the literal in as a `const` and passed every +/// gate. So this module owns the three AXES and the consumer owns the TOKEN, and +/// the split is what keeps one definition compatible with rule 1. +/// +/// # The three axes, decided by CLOUD-761 and built here +/// +/// **Case: sensitive.** Nothing here folds case. The consumer's row carries no +/// `(?i)`, so the lowercase spelling is not a key and is refused rather than +/// normalised — normalising up is precisely what produced the shipped defect. +/// +/// **Boundary: the surrounding bytes, checked rather than composed.** The stated +/// form is `(^|[^0-9A-Za-z-])…([^0-9]|$)`, and this does not build it as a +/// regex — `regex` has no lookahead, so a trailing class would CONSUME the byte +/// after a match and make two adjacent keys unfindable. Reading the bytes on +/// either side of a match answers the same question, and it means the crate +/// composes no key expression at all: there is nothing here for a twenty-first +/// derivation to be a copy OF. +/// +/// **Project prefix: mandatory.** Inherited from the consumer's row rather than +/// asserted here. The four shell `case` globs this replaces accept `AB-1`, `Z-9` +/// and `A-1foo` because a glob cannot anchor; [`Grammar::key_of`] anchors by +/// requiring the match to span the whole input, which no glob can express. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct IssueKey(String); + +impl IssueKey { + /// The key as the consumer wrote it. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } + + /// The trailing number, for the numeric ordering [`Grammar::keys_in`] keeps. + fn number(&self) -> u64 { + self.0 + .rsplit('-') .next() .and_then(|n| n.parse::().ok()) .unwrap_or(0) - }); - keys + } +} + +impl std::fmt::Display for IssueKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +/// Is the byte before a match a boundary — i.e. NOT one a key could continue +/// through? +/// +/// `-` is in the class deliberately, and that is the axis `\b` cannot express: a +/// word boundary treats `-` as a separator, so `\b` would find a key inside a +/// longer hyphenated token. Two landed sites disagreed on exactly this while both +/// looking correct. +fn opens_a_key(text: &str, at: usize) -> bool { + text[..at] + .chars() + .next_back() + .is_none_or(|c| !c.is_ascii_alphanumeric() && c != '-') +} + +/// Is the byte after a match a boundary? +/// +/// Digits only, per the decided form. A letter may follow — `CLOUD-1x` contains +/// the key `CLOUD-1` — which is why [`Grammar::key_of`] asks a different +/// question than this one rather than reusing it. +fn closes_a_key(text: &str, at: usize) -> bool { + text[at..] + .chars() + .next() + .is_none_or(|c| !c.is_ascii_digit()) +} + +impl Grammar { + /// Is this WHOLE string a key? The four shell `case` globs' question. + /// + /// Anchored by construction: the match must begin at 0 and end at the input's + /// length, so `CLOUD-1x` is not a key even though it contains one. A glob + /// cannot say this, which is why those sites accept `AB-1` and `A-1foo` + /// today. + #[must_use] + pub fn key_of(&self, text: &str) -> Option { + let found = self.key.find(text)?; + (found.start() == 0 && found.end() == text.len()) + .then(|| IssueKey(found.as_str().to_owned())) + } + + /// The issue keys in a span, deduped and ordered NUMERICALLY. + /// + /// Numeric and not a bare sort, for `graph-check`'s reason: `CLOUD-10` sorts + /// before `CLOUD-9` lexically, so a caller diffing two runs could not tell an + /// ordering change from a content one. + /// + /// The boundary check is what stops a key being found inside a longer token. + /// A greedy match already prevents the reverse case the two landed sites + /// commented on — `CLOUD-17` is not returned for `CLOUD-179`, because the + /// match IS `CLOUD-179` — so a caller asking "does this text carry key K" + /// compares against this set rather than searching again. That is the third + /// derivation this definition removes rather than adds. + #[must_use] + pub fn keys_in(&self, text: &str) -> Vec { + let found: BTreeSet<&str> = self + .key + .find_iter(text) + .filter(|m| opens_a_key(text, m.start()) && closes_a_key(text, m.end())) + .map(|m| m.as_str()) + .collect(); + let mut keys: Vec = found.into_iter().map(|k| IssueKey(k.to_owned())).collect(); + keys.sort_by_key(IssueKey::number); + keys + } +} + +/// The key strings in a span, for the callers inside this module that still want +/// them as text. +fn keys_in(grammar: &Grammar, text: &str) -> Vec { + grammar + .keys_in(text) + .into_iter() + .map(|k| k.as_str().to_owned()) + .collect() } /// One emitted derived fact: a label and its key set. @@ -1347,3 +1466,113 @@ pub fn verdict_token( _ => None, } } + +// CLOUD-1142's fixed example set, driven against the grammar this repository +// COMMITS rather than a fixture — `Grammar::committed`'s own reason: a fixture +// would let the registry row change while every case here kept passing, which is +// the drift one definition exists to remove. +// +// The examples are the row's, written down there rather than left to the +// implementer, and each one is a site that behaves differently today. +#[cfg(test)] +mod issue_key_tests { + use super::Grammar; + + /// The consumer's own key, spelled from parts so this file carries no + /// derivation of the token — `no-tracker-key-in-core` refuses one anywhere + /// under `crates/**`, and a test is not exempt from the rule it is testing. + fn key(n: u32) -> String { + format!("{}-{n}", "CL".to_owned() + "OUD") + } + + #[test] + fn the_consumers_own_key_is_a_key() { + // The positive arm first: without it every refusal below is satisfied by + // a definition that refuses everything. + let grammar = Grammar::committed(); + let subject = key(757); + assert_eq!( + grammar.key_of(&subject).map(|k| k.as_str().to_owned()), + Some(subject.clone()), + "the committed vocabulary's own key must parse" + ); + } + + #[test] + fn the_lowercase_spelling_is_not_a_key() { + // CASE: SENSITIVE. The shipped defect CLOUD-761 measured — one gate + // accepts this spelling and two others cannot find it. Refused rather + // than normalised, because normalising up produced the disagreement. + let grammar = Grammar::committed(); + assert_eq!(grammar.key_of(&key(757).to_lowercase()), None); + } + + #[test] + fn a_glob_shaped_near_miss_is_not_a_key() { + // PROJECT PREFIX: MANDATORY. All three are accepted today by the four + // shell `case` globs, which test `[A-Z]*-[0-9]*` and cannot anchor. + let grammar = Grammar::committed(); + for subject in ["AB-1", "Z-9", "A-1foo"] { + assert_eq!(grammar.key_of(subject), None, "{subject} is not a key"); + } + } + + #[test] + fn a_key_with_a_trailing_letter_is_not_a_key_but_contains_one() { + // The glob `-[0-9]*` accepts this for the same reason. The whole + // string must BE the key, and here the match stops short of the input's + // end — which is also why this asks a different question from `keys_in`, + // where the same string legitimately CONTAINS a key. + let grammar = Grammar::committed(); + let subject = format!("{}x", key(1)); + assert_eq!(grammar.key_of(&subject), None); + assert_eq!(grammar.keys_in(&subject).len(), 1); + } + + #[test] + fn a_shorter_key_is_not_found_inside_a_longer_one() { + // BOUNDARY. The case two landed sites commented on by name. A greedy + // match takes the whole number, so the short key never appears — and a + // caller asking "does this carry key K" compares against this set rather + // than searching again, which is the derivation this removes. + let grammar = Grammar::committed(); + let found = grammar.keys_in(&key(179)); + assert_eq!(found.len(), 1); + assert_eq!(found[0].as_str(), key(179)); + assert!(!found.iter().any(|k| k.as_str() == key(17))); + } + + #[test] + fn a_key_glued_to_a_leading_token_is_not_found() { + // The other half of the boundary, and the half `\b` gets wrong: a word + // boundary treats `-` as a separator, so it would find a key inside a + // longer hyphenated token. + let grammar = Grammar::committed(); + for prefix in ["X", "9", "SUB-"] { + let subject = format!("{prefix}{}", key(757)); + assert!( + grammar.keys_in(&subject).is_empty(), + "{subject} carries no key of its own" + ); + } + } + + #[test] + fn ordinary_prose_yields_its_keys_in_numeric_order() { + // The allow that keeps the boundary honest: the separators a body + // actually uses must still open a key, or the definition refuses most + // real text and gets replaced by a twenty-first copy. + let grammar = Grammar::committed(); + let text = format!("Refs: {}, {} and ({}).", key(10), key(9), key(1142)); + let found: Vec = grammar + .keys_in(&text) + .into_iter() + .map(|k| k.as_str().to_owned()) + .collect(); + assert_eq!( + found, + vec![key(9), key(10), key(1142)], + "numeric, not lexical" + ); + } +}