diff --git a/crates/batten/src/trust.rs b/crates/batten/src/trust.rs index 118e09601..8b58f0b12 100644 --- a/crates/batten/src/trust.rs +++ b/crates/batten/src/trust.rs @@ -2627,15 +2627,64 @@ const RULE_NON_PREDICATE: &[(&str, &str)] = &[ ), ]; +/// Rule columns and nested keys whose ARRIVAL can only add refusals, each with +/// why (CLOUD-1394). +/// +/// **Declared, never inferred, and the counterexamples are why.** The tempting +/// structural rule — *an optional key whose absence preserved the prior +/// behaviour can only narrow* — is false of this repository three times over, +/// and each one is already written down. [`Rule::bypass_env`] arrives on a row +/// and makes it SUPPRESSIBLE (`batten.toml:415-430` says the smell is correct +/// "in its own terms, since the key makes a row suppressible"). +/// [`Rule::key_shape`] arrives and resolves a non-matching subject to absent, +/// which its own doc calls could-not-look and therefore allow — "it narrows +/// what counts as a subject; it never denies", which is a narrower PREDICATE +/// and a weaker GATE. [`Rule::when_present`] narrows a `mediated_call` row to +/// the calls that named a projection, so the calls it stops gating are no +/// longer refused. +/// +/// So the ranking is per-key and it is written here with its argument, exactly +/// as [`RULE_NON_PREDICATE`] writes its exemptions. Everything not named stays +/// unrankable and keeps firing, which is the direction the gate has always been +/// right about: a partial ranking that read "unrankable" as "narrowing" would +/// convert a false refusal into a false pass. +const RULE_NARROWING_ON_ARRIVAL: &[(&str, &str)] = &[ + ( + "max_age", + "a receipt this row already accepted can only stop qualifying by aging \ + out. Absent, existence is the verdict — the column's own words, \"no \ + committed row changes meaning by this column arriving\" — so what \ + arrives is a bound, never an admission", + ), + ( + "requires_field", + "a receipt field that says the required value is satisfied and an absent \ + one fails OPEN, so the column can only add the `Refuted` deny it \ + introduces. Nothing this row accepted before is accepted less", + ), +]; + /// Predicate columns of one rule that the working tree changed. /// -/// **A change, never a ranking.** Whether a narrowed glob is weaker than the one +/// **A change, and one ranking.** Whether a narrowed glob is weaker than the one /// it replaced is a judgement about two patterns, and this module does not make /// judgements. That the predicate moved at all is a byte comparison, and it is /// the fact that used to be reported nowhere: a rule whose glob matches nothing /// keeps its id and its severity, so the comparison called it unchanged while it /// gated nothing. /// +/// **The one exception is an ARRIVAL, and only a declared one** (CLOUD-1394). +/// Where the base said nothing and the working tree fills in a key named by +/// [`RULE_NARROWING_ON_ARRIVAL`], the change can only add refusals, and charging +/// it the groomed `Weakens:` pair a loosening costs is a false refusal that the +/// repository has twice paid by shaping the config around the gate instead +/// (`batten.toml:925-933`, `:951-960`, both splitting a check into a new +/// `receipt` row because a new row raises nothing). Every other difference — +/// a changed value, a removed key, a resized list, an UNDECLARED key arriving — +/// is unrankable and still fires. That asymmetry is deliberate: a ranking that +/// read "unrankable" as "narrowing" would turn this gate's one false refusal +/// into a false pass. +/// /// Read off the rule's own serialization rather than a hand-kept column list, /// which would drift the next time [`Rule`] grows one. The tokens are digests: /// a config's patterns are the consumer's, and a pointer names *which* column @@ -2661,7 +2710,7 @@ fn rule_predicate_weakenings(base: &Rule, working: &Rule) -> Vec { from.get(column).unwrap_or(&serde_json::Value::Null), to.get(column).unwrap_or(&serde_json::Value::Null), ); - (was != now).then(|| { + (was != now && !arrival_is_narrowing(column, was, now)).then(|| { Weakening::new( WeakeningKind::RulePredicateChanged, format!("rule[{}].{column}", base.id), @@ -2673,6 +2722,80 @@ fn rule_predicate_weakenings(base: &Rule, working: &Rule) -> Vec { .collect() } +/// Whether the difference between two column values is a declared arrival. +/// +/// Split from [`rule_predicate_weakenings`] rather than inlined so the ranking +/// has a name a test can reach, and so the comparison above stays one +/// expression. +fn arrival_is_narrowing(column: &str, was: &serde_json::Value, now: &serde_json::Value) -> bool { + if was.is_null() { + // The whole column arrived, so the key that arrived is the column. + return !now.is_null() && declared_narrowing(column); + } + arrivals_only(was, now).is_some_and(|keys| keys.iter().all(|key| declared_narrowing(key))) +} + +/// Whether [`RULE_NARROWING_ON_ARRIVAL`] names this key. +fn declared_narrowing(key: &str) -> bool { + RULE_NARROWING_ON_ARRIVAL + .iter() + .any(|(narrowing, _)| *narrowing == key) +} + +/// The keys the working tree FILLS where the base said nothing, or `None` the +/// moment anything else moved. +/// +/// `None` is the unrankable answer and it is the common one: a changed scalar, +/// a key the base carried and the head dropped, a list that changed length, a +/// type that changed. Only a strictly additive difference has a direction this +/// module can state without judging two values against each other. +/// +/// **Absent and `null` are one reading**, which [`column_token`] already takes: +/// a rule column carries `skip_serializing_if`, so a fresh optional field is +/// simply missing from the base object, while a nested row like +/// [`crate::facts::CaptureQuery`] carries none and serializes the same field as +/// `null`. A ranking that saw only one of those spellings would rank the rule +/// and not the row inside it. +fn arrivals_only(base: &serde_json::Value, head: &serde_json::Value) -> Option> { + use serde_json::Value; + if base == head { + return Some(Vec::new()); + } + match (base, head) { + (Value::Object(from), Value::Object(to)) => { + // A key the base carried and the head no longer does is a removal, + // never an arrival — and `null` on either side is "said nothing". + if from + .iter() + .any(|(key, was)| !was.is_null() && !to.contains_key(key)) + { + return None; + } + let mut arrived = Vec::new(); + for (key, now) in to { + let was = from.get(key).unwrap_or(&Value::Null); + if was == now { + continue; + } + if was.is_null() { + arrived.push(key.clone()); + } else { + arrived.extend(arrivals_only(was, now)?); + } + } + Some(arrived) + } + (Value::Array(from), Value::Array(to)) if from.len() == to.len() => { + let mut arrived = Vec::new(); + for (was, now) in from.iter().zip(to.iter()) { + arrived.extend(arrivals_only(was, now)?); + } + Some(arrived) + } + _ => None, + } +} + /// One column's value as a stable token: a digest, or `absent`. /// /// Never the value itself. A rule's patterns are the consumer's config, and §6's @@ -3796,6 +3919,125 @@ mod tests { assert!(weakenings(&base, &base).is_empty()); } + fn receipt_rule(id: &str, extra: &str) -> String { + format!( + "\n[[rule]]\nid = \"{id}\"\nkind = \"receipt\"\nscope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"gh pr ready\"\nchecks = [\"verify\"]\nkey = \"head\"\nreason = \"r\"\n{extra}" + ) + } + + #[test] + fn an_arriving_declared_narrowing_key_is_not_charged_as_a_weakening() { + // CLOUD-1394's measured case, in the shape CLOUD-1387 ran into: a column + // arrives that the base said nothing about, and its arrival can only add + // refusals. The gate used to charge that the groomed `Weakens:` pair a + // loosening costs, which is a false refusal and is why the repository + // twice split a check into a new row rather than touch an existing one. + let base = config(&receipt_rule("r", "")); + let working = config(&receipt_rule("r", "max_age = 3600\n")); + assert!( + weakenings(&base, &working).is_empty(), + "an arriving `max_age` is a bound, never an admission: {:?}", + weakenings(&base, &working) + ); + } + + #[test] + fn the_declared_key_moving_after_it_arrived_is_still_a_predicate_change() { + // The discriminating mirror, and the arm that makes the one above worth + // anything: a case asserting only the narrowing would pass under a gate + // that stopped firing at all. Raising a bound admits receipts the row + // refused, which is the direction nothing here ranks. + let base = config(&receipt_rule("r", "max_age = 3600\n")); + let working = config(&receipt_rule("r", "max_age = 86400\n")); + let found = only(&base, &working); + assert_eq!(found.kind, WeakeningKind::RulePredicateChanged); + assert_eq!(found.key, "rule[r].max_age"); + } + + #[test] + fn a_declared_key_departing_is_a_predicate_change_in_the_other_direction() { + // Arrival is the ranked half and removal is not: dropping the bound + // restores every receipt it was expiring. + let base = config(&receipt_rule("r", "max_age = 3600\n")); + let working = config(&receipt_rule("r", "")); + assert_eq!(only(&base, &working).key, "rule[r].max_age"); + } + + #[test] + fn an_arriving_undeclared_key_is_still_charged() { + // `bypass_env` is the measured counterexample the ranking is declared + // rather than inferred for (`batten.toml:415-430`): optional, absent + // before, absence-preserving — and it makes the row SUPPRESSIBLE. An + // "added optional key is a narrowing" rule would have admitted it. + let base = config(&receipt_rule("r", "")); + let working = config(&receipt_rule("r", "bypass_env = \"BATTEN_X_BYPASS\"\n")); + assert_eq!(only(&base, &working).key, "rule[r].bypass_env"); + } + + #[test] + fn a_declared_key_arriving_inside_a_nested_row_is_ranked_too() { + // The shape CLOUD-1387 actually has: the column is a LIST of rows and + // what arrives is a field on one of them. A nested row carries no + // `skip_serializing_if`, so the base spells the same absence `null` + // rather than by omission, and both readings have to rank. + let json = |text: &str| -> serde_json::Value { + serde_json::from_str(text).expect("a test literal parses") + }; + assert_eq!( + arrivals_only( + &json(r#"[{"id": "a", "max_age": null}]"#), + &json(r#"[{"id": "a", "max_age": 60}]"#), + ), + Some(vec!["max_age".to_owned()]), + "a `null` the head fills is an arrival" + ); + assert_eq!( + arrivals_only( + &json(r#"[{"id": "a"}]"#), + &json(r#"[{"id": "a", "max_age": 60}]"#) + ), + Some(vec!["max_age".to_owned()]), + "and so is a key the base omitted" + ); + // Everything else is unrankable, which is the direction that keeps + // firing: a changed value, a departure, a resized list. + assert_eq!( + arrivals_only(&json(r#"[{"id": "a"}]"#), &json(r#"[{"id": "b"}]"#)), + None + ); + assert_eq!( + arrivals_only( + &json(r#"[{"id": "a", "max_age": 60}]"#), + &json(r#"[{"id": "a"}]"#) + ), + None + ); + assert_eq!( + arrivals_only(&json("[]"), &json(r#"[{"id": "a"}]"#)), + None, + "a row joining a list is not one row's key arriving" + ); + } + + #[test] + fn every_narrowing_key_names_a_real_column_and_a_reason() { + // `RULE_NON_PREDICATE`'s census, for the table that ranks rather than + // exempts — and the failure direction is worse here: an exemption that + // names nothing merely never fires, while a NARROWING key that names + // nothing is a name a later column could accidentally acquire. + let source = concat!(include_str!("rules.rs"), include_str!("facts.rs")); + for (key, reason) in RULE_NARROWING_ON_ARRIVAL { + assert!( + source.contains(&format!("pub {key}:")), + "`{key}` is ranked as a narrowing but no rule or fact row declares it" + ); + assert!( + !reason.trim().is_empty(), + "`{key}` is ranked as a narrowing without saying why" + ); + } + } + #[test] fn a_lowered_severity_is_not_also_reported_as_a_predicate_change() { // `severity` is exempt from the predicate columns because it has a rank diff --git a/crates/batten/tests/it/config_lint.rs b/crates/batten/tests/it/config_lint.rs index f6c0c7af6..48fce2b56 100644 --- a/crates/batten/tests/it/config_lint.rs +++ b/crates/batten/tests/it/config_lint.rs @@ -667,6 +667,66 @@ fn the_keys_cloud_721_added_reach_the_lint_with_their_own_pointers() { ); } +#[test] +fn an_arriving_narrowing_key_is_clean_while_the_same_row_widened_is_not() { + // CLOUD-1394, over the compiled binary against a REAL base ref, because the + // acceptance turns on the engine building the comparison rather than on the + // module's own view of it. Both arms live in one case on purpose: an + // assertion that the narrowing is clean passes trivially under a gate that + // stopped firing at all, which is the failure this replaces rather than a + // variant of it. + let row = |extra: &str| { + format!( + "version = 1\n\n[[rule]]\nid = \"ready-receipt\"\nkind = \"receipt\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"gh pr ready\"\n\ + checks = [\"verify\"]\nkey = \"head\"\nreason = \"run verify\"\n{extra}" + ) + }; + + // The narrowing: a bound arrives where the base declared none, so a receipt + // this row already accepted can only stop qualifying by aging out. + let narrowed = pr_fixture("lint-1394-narrowed", &row(""), &row("max_age = 3600\n")); + let output = lint(&narrowed, &["--config-from", "origin/main"]); + assert_eq!( + output.status.code(), + Some(0), + "an arriving `max_age` is not a weakening; got: {}", + stdout(&output) + ); + assert_eq!(stdout(&output), "config-lint: 0 smell(s)\n"); + + // The mirror, same row and same column: raising the bound admits receipts + // the base refused, and nothing here ranks that. + let widened = pr_fixture( + "lint-1394-widened", + &row("max_age = 3600\n"), + &row("max_age = 86400\n"), + ); + let output = lint(&widened, &["--config-from", "origin/main"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!( + stdout(&output), + "batten.toml:rule[ready-receipt].max_age rule-predicate-changed\n\ + config-lint: 1 smell(s)\n" + ); + + // And the counterexample the ranking is DECLARED rather than inferred for: + // `bypass_env` is optional, absent before and absence-preserving, and it + // makes the row suppressible (`batten.toml:415-430`). + let suppressible = pr_fixture( + "lint-1394-bypass", + &row(""), + &row("bypass_env = \"BATTEN_READY_BYPASS\"\n"), + ); + let output = lint(&suppressible, &["--config-from", "origin/main"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!( + stdout(&output), + "batten.toml:rule[ready-receipt].bypass_env rule-predicate-changed\n\ + config-lint: 1 smell(s)\n" + ); +} + #[test] fn the_reverse_edit_of_those_keys_is_clean() { // The direction half, at the same boundary the verdict is taken: a widened