diff --git a/.serena/memories/core.md b/.serena/memories/core.md index c51cb4e71..d21ad1f2e 100644 --- a/.serena/memories/core.md +++ b/.serena/memories/core.md @@ -1403,6 +1403,24 @@ judge_fingerprint`, its own domain tag), so a caller can reference content it exactly what this surface could rebuild. `Module` holds no `source` field and hand-writes `Debug`, so a policy body has nowhere to live past compilation (rule 4). +- `pattern.rs` — the `[[pattern]]` table (CLOUD-885): named regular expressions a + policy module references by id, never writes inline. **The lever is cost, not + prohibition** — "do not regex things that are not regular" is a judgement and + rule 3 refuses a gate over one, but _where a pattern lives_ is decidable. So a + regex costs a config row and an id while the same question over an + already-parsed document costs a field access, and the cheap path becomes the + correct one without a translator reasoning about regularity. Consumer-owned for + `verbs`' stated reason (rule 1): a tracker-key expression is a consumer + identifier. Two consequences fall out rather than being designed in — + duplication becomes _unwritable_ (measured: one concept, 19 spellings across 17 + shell programs), and the inventory becomes reviewable data (§11). `policy.rs` + closes it at both ends: an inline literal is refused, and so is a reference no + row declares, because that resolves to UNDEFINED and Rego reads undefined as + "does not hold" — a silent gate. The same failure by deletion is + `trust::WeakeningKind::PatternRemoved`. Two defects here were found by an AST + probe rather than a reading: a backtick literal is a `RawString` node and not a + `String`, and a REFERENCE contains a literal (`patterns["x"]` is a `RefBrack` + indexed by a string), so a naive sweep refuses the sanctioned form. - `provision.rs` — the `[[provision]]` manifest (CLOUD-90): pinned tools fetched and cached out of tree. §9's check/fix pair — `provision status` (read) is freshness, `provision apply [-n]` (write) is the fix. **The provisioned binary diff --git a/Cargo.toml b/Cargo.toml index 82d7a351e..74d20b7b5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -241,6 +241,39 @@ gix = { version = "0.86", default-features = false, features = [ # code, in the harder place to review, to avoid a feature that costs a lockfile # edge. The first tree-scoped preset was written string-only and hit the wall on # the second rule, which is when this was reconsidered. +# +# `semver` IS NOT HERE, AND THAT IS THIS ROW'S OWN RULE APPLIED TO ITSELF. It +# was added beside `regex` on the reasoning that version-comparison gates will +# want it — and no module in the tree calls it. A feature nothing calls is +# `input.tree.tracked` documented and never built (CLOUD-845): it reads as a +# capability, it is checked by nothing, and the first consumer discovers whether +# it actually works. So it lands with its first consumer rather than ahead of +# one. The measurement is recorded on CLOUD-885 either way — `semver` is +# `dep:semver` alone and cleared both gates — so re-adding it costs a line, not +# a re-derivation. +# +# `glob` IS REFUSED BY CONSTRUCTION, not by preference, and stating it here is +# what stops it being re-litigated as an oversight: `glob = ["dep:globset"]`, and +# `globset` is one of the nine names `evaluator-closure-check` decides on. The +# gate that defends the security claim already answers this. A module needing +# path matching takes it from the row's `sources` glob (CLOUD-850), resolved by +# the engine outside the module — which is the better shape anyway, since the +# engine bounds acquisition by declaration. +# +# `time` IS DEFERRED, AND THE BLOCKER IS UPSTREAM'S DECLARATION rather than this +# pin (CLOUD-885). `time = ["dep:chrono", "dep:chrono-tz"]` and regorus declares +# both with `default-features = true` — every published version carrying the +# feature does, checked against the index. chrono's default `clock` reaches +# `iana-time-zone`, which on Apple targets reaches `core-foundation-sys`, which +# `macos-link-check`'s `FRAMEWORK_CRATES` names; enabling it fails that gate at +# exit 1 on exactly that crate. Cargo's feature unification is ADDITIVE, so a +# direct `chrono = { default-features = false }` here would not subtract what +# regorus requested — this is not fixable from our manifest. CLOUD-885 carries +# the re-open predicate as a command. +# +# `semver` IS NOT HERE, on the same rule (CLOUD-885): no module calls it, and a +# feature nothing calls is `input.tree.tracked` documented and never built +# (CLOUD-845). It lands with its first consumer. regorus = { version = "0.11", default-features = false, features = [ "ast", "std", diff --git a/batten.toml b/batten.toml index be9188df4..fac0bc1ab 100644 --- a/batten.toml +++ b/batten.toml @@ -1657,6 +1657,23 @@ no_fix_reason = "an IO crate reaching the evaluator is closed where it was enabl # # Mediated-call scoped, because the predicate is over a command line. The other # four families of that guard stay in bash and its header says which and why. +# The short flag cluster `git commit` accepts a message source in — `-m`, `-am`, +# `-F`, `-C`, `-c` — as a shape rather than a literal (CLOUD-885). +# +# DECLARED HERE RATHER THAN IN THE MODULE, and this row is the mechanism proving +# itself on consumer #1. An expression is a consumer fact: written into +# `policy/run-shape.rego` it would be a pattern with no home, invisible to +# `config show`, and the second module wanting the same shape would re-spell it. +# The engine refuses an inline regex at load, so this row is not a convention — +# it is the only way to write the predicate at all. +# +# The anchor is the predicate. `contains` over the flag's tail cannot say +# "letters", so `-x=mfoo` carried an `m` and read as naming a message source, +# letting through a commit that still blocks on $EDITOR. +[[pattern]] +id = "short-message-flag-cluster" +regex = "^-[A-Za-z]*[mFCc]" + [[rule]] id = "commit-message-obtainable" kind = "policy" diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index 79c1f35dc..f0c6a00c8 100644 --- a/crates/batten/src/config.rs +++ b/crates/batten/src/config.rs @@ -162,6 +162,18 @@ pub struct Config { /// lookup are [`crate::verbs`]. #[serde(default, rename = "verb", skip_serializing_if = "Vec::is_empty")] pub verbs: Vec, + /// The named-regex table (CLOUD-885): every expression a policy module may + /// apply, declared once and referenced by id. + /// + /// Consumer-specific by nature for exactly [`Config::verbs`]'s reason — a + /// tracker-key expression is a consumer identifier, so it lives here and + /// never in the crate (non-negotiable rule 1). A module reaches it at + /// `data.batten.patterns[""]`; writing one inline is refused at load, + /// which is what prices a one-off pattern against a field access over an + /// already-parsed document. The type and its validation are + /// [`crate::pattern`]. + #[serde(default, rename = "pattern", skip_serializing_if = "Vec::is_empty")] + pub patterns: Vec, /// The per-path-class redirect table (CLOUD-280): what to run instead, /// keyed by what is protected rather than by the verb reaching for it. /// @@ -616,6 +628,12 @@ fn parse_ungated(text: &str, source: &str) -> Result { // too: `batten.local.toml` may add verb rows, and a raise-only override that // adds an inert one has still written something that cannot mean anything. crate::verbs::validate(&config.verbs)?; + // The named-regex table, at parse for the identical reason (CLOUD-885): a + // malformed expression is a config fault, and refusing it here means + // `config lint` and `doctor` catch it rather than a mediated call + // discovering it at adjudication, which is the worst time and the wrong exit + // class (house style §8). + crate::pattern::validate(&config.patterns)?; crate::redirect::validate(&config.redirects)?; // And the marker table, for the identical reason in the identical shape // (CLOUD-253). Both tables arrived in one commit; CLOUD-242 wired one of @@ -766,6 +784,7 @@ impl Config { strictness: None, fail_on_warning: None, rules: Vec::new(), + patterns: Vec::new(), scope: Vec::new(), protected: Vec::new(), unlanded: Vec::new(), @@ -1029,6 +1048,7 @@ mod tests { /// [`parse_ungated`] that does it. Deleting a call fails the test below. const VALIDATED_AT_LOAD: &[(&str, &str)] = &[ ("verbs", "crate::verbs::validate("), + ("patterns", "crate::pattern::validate("), ("redirects", "crate::redirect::validate("), ("markers", "crate::markers::validate("), // The LOCATED form (CLOUD-773): the loaders hold the config text, so a diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index cc2c414fb..c863ec18c 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -2037,6 +2037,19 @@ impl Policy { // still is at `batten check`/`enforce`, which is where a tree rule // is evaluated and where `verify` and CI both reach it — so the // module is still refused before it can matter, one surface over. + // + // AND THE MODULE CHECKS DO NOT RUN HERE EITHER (CLOUD-885), which is + // the same finding reached from the other side. Narrowing the rows + // above removes the tree modules; this removes the AST walk over the + // ones that remain. `check_no_inline_regex` and + // `check_tree_paths_are_emittable` read a module through + // `get_ast_as_json`, which serialises every rule of every module — + // and their answer is a property of the module TEXT, so it is fixed + // for the life of the load and identical on every surface. CI + // measured re-deriving it per call at `wired` 14.03 ms -> 22.98 ms + // (1.638x). Both are the same rule: the mediated call loads and + // decides; a config fault is reported where config faults are + // reported. bundles: crate::policy::load( root, &resolved @@ -2045,6 +2058,8 @@ impl Policy { .filter(|rule| rule.scope == RuleScope::MediatedCall) .cloned() .collect::>(), + &resolved.patterns, + crate::policy::ModuleChecks::SkipOnHotPath, reference, )?, }) @@ -5318,7 +5333,8 @@ mod tests { Policy { harness: Harness::ExitCode, facts: Vec::new(), - bundles: crate::policy::load(&dir, &[row], None).expect("load"), + bundles: crate::policy::load(&dir, &[row], &[], crate::policy::ModuleChecks::Run, None) + .expect("load"), shapes: Vec::new(), fail_on_warning: false, verbs: Vec::new(), diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 60fdf232d..8a5965e01 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -43,6 +43,7 @@ pub mod lint; pub mod markers; pub mod output; pub mod outputs; +pub mod pattern; pub mod policy; pub mod provision; pub mod receipt; @@ -440,7 +441,7 @@ fn run_baseline( ) -> Result { let root = anchor(); let config = resolve::resolve(&root, overrides)?; - let scan = rules::run_static(&config.rules, &config.provisions, &root)?; + let scan = rules::run_static(&config.rules, &config.provisions, &config.patterns, &root)?; if prune { let Some(existing) = baseline::load(&root)? else { @@ -776,7 +777,12 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R // included (CLOUD-97 never once evaluated in this repository for exactly // that reason). Withholding is honest here because `record` below folds // `not_evaluated` into the store, where a withheld rule's findings HOLD. - let scan = rules::run_recorded(&config.rules, &config.provisions, Path::new("."))?; + let scan = rules::run_recorded( + &config.rules, + &config.provisions, + &config.patterns, + Path::new("."), + )?; if !scan.not_evaluated.is_empty() { // Never silent: a rule that did not look must say so, or a clean-looking // record is the false green. The COUNT carries that on the default rung @@ -1353,7 +1359,13 @@ impl SuiteReport { fn run_policy_test(json: bool, overrides: &Overrides, out: &mut dyn Write) -> Result { let root = Path::new("."); let config = resolve::resolve(root, overrides)?; - let bundles = policy::load(root, &config.rules, overrides.config_from.as_deref())?; + let bundles = policy::load( + root, + &config.rules, + &config.patterns, + policy::ModuleChecks::Run, + overrides.config_from.as_deref(), + )?; // The same walk the tree engine hoists, so a suite's input carries the same // `tracked` a real `check` would hand the bundle (CLOUD-845). Resolved once // here rather than per row, for the reason `rules::run` gives. @@ -3767,12 +3779,26 @@ fn apply_baseline( Ok(kept) } +/// One of the two rule-running surfaces, as [`run_rules`] takes it. +/// +/// Named rather than written inline because the pattern table joined the +/// argument list (CLOUD-885) and a four-argument fn pointer is past what +/// `clippy::type_complexity` will read. The alias is also the clearer spelling: +/// the three tables and the root are what a runner needs, and saying so once +/// beats repeating it at both call sites. +type RuleRunner = fn( + &[rules::Rule], + &[provision::Provision], + &[pattern::NamedPattern], + &Path, +) -> Result; + fn run_rules( out: &mut dyn Write, err: &mut dyn Write, mode: Mode, overrides: &Overrides, - runner: fn(&[rules::Rule], &[provision::Provision], &Path) -> Result, + runner: RuleRunner, surface: Surface, json: bool, ) -> Result { @@ -3801,7 +3827,7 @@ fn run_rules( // store's resolve pass fail-closed (CLOUD-81), and the enforce surface now // journals (CLOUD-529), so dropping it here would let a rule that never // looked resolve every finding it covers. - let scan = runner(&config.rules, &config.provisions, &root)?; + let scan = runner(&config.rules, &config.provisions, &config.patterns, &root)?; let mut findings = scan.findings.clone(); // Declared budgets are gates, evaluated here rather than only under `policy diff --git a/crates/batten/src/lint.rs b/crates/batten/src/lint.rs index 3ca5106f6..d3cd07759 100644 --- a/crates/batten/src/lint.rs +++ b/crates/batten/src/lint.rs @@ -462,7 +462,15 @@ pub fn run(dir: &Path, base_ref: Option<&str>, today: crate::waiver::Date) -> Re // reports nothing about a set it never saw. let bundles = config::parse(&text, &path.display().to_string()) .ok() - .map(|config| crate::policy::load(dir, &config.rules, base_ref)) + .map(|config| { + crate::policy::load( + dir, + &config.rules, + &config.patterns, + crate::policy::ModuleChecks::Run, + base_ref, + ) + }) .and_then(std::result::Result::ok) .unwrap_or_default(); smells( diff --git a/crates/batten/src/pattern.rs b/crates/batten/src/pattern.rs new file mode 100644 index 000000000..69b86966d --- /dev/null +++ b/crates/batten/src/pattern.rs @@ -0,0 +1,135 @@ +//! Named regular expressions, declared in the one committed authority +//! (CLOUD-885). +//! +//! # Why a pattern may not be written inline in a module +//! +//! Enabling `regorus`'s `regex` builtin gave a policy module the matching +//! vocabulary [`crate::rules::RuleKind::Forbid`] has carried since CLOUD-283. +//! It also handed 86 queued bash migrations the closest-looking thing to the +//! `grep -E`/`sed -E`/`awk` they are being translated from, and a translator +//! reaches for whatever is cheapest to write. +//! +//! **The lever is cost, not prohibition.** A rule saying "do not regex things +//! that are not regular" cannot be a gate: "is this pattern parsing something +//! non-regular" is a judgement, and non-negotiable rule 3 says a gate resolves +//! to a command and an exit code over an object it decides. What *is* decidable +//! is where a pattern lives. So a regex costs a declaration — an id and a row in +//! `batten.toml` — while asking the same question of a parsed document or a +//! resolved invocation costs a field access. One-off patterns get priced; shared +//! ones stay cheap after the first. +//! +//! # Three things fall out, and none of them is friction for its own sake +//! +//! **Non-negotiable rule 1.** `CLOUD-[0-9]+` is a *consumer* identifier. Written +//! into a module under `crates/batten` it is a repo-agnosticism violation of +//! exactly the kind rule 1 names; written here it is where consumer facts +//! belong. [`crate::verbs::MutatingVerb`] already states this argument for the +//! mutating-verb table — "consumer-specific by nature, so it lives here and +//! never in the crate" — and this is the same table shape for the same reason. +//! +//! **Duplication becomes unwritable rather than detectable.** Measured on the +//! tree this engine gates, 2026-08-22 with comments stripped: 82 of 140 shell +//! programs use `grep -E`/`sed -E`/`awk`/`=~` over 338 sites, and one concept — +//! a tracker key — carries **19 distinct spellings across 17 programs**. +//! `CLOUD-[0-9]+` alone sits at 15 sites in 9 of them, with a `CLOUD-*` GLOB +//! spelling beside the regexes and one program holding two variants of its own +//! pattern 52 lines apart. Those are correct regexes over a genuinely regular +//! thing, duplicated until they drifted. A named declaration has one home, so +//! the second author finds the first one's work instead of re-deriving it. +//! +//! **The inventory becomes reviewable data** (house style §11). Every pattern +//! the policy set can apply is readable out of one file, which is not true of +//! any tree where they are written inline. +//! +//! # What a module sees +//! +//! The set is projected into the evaluator's `data` document, so a reference +//! reads `data.batten.patterns["tracker-key"]`. It is deliberately NOT on +//! `input`: `input` is the subject under adjudication and changes per call, +//! while this is configuration, fixed for the life of the load. + +use std::collections::BTreeSet; + +use serde::{Deserialize, Serialize}; + +use crate::error::UsageError; + +/// One declared regular expression. +/// +/// Mirrors [`crate::verbs::MutatingVerb`]'s shape deliberately: a consumer-owned +/// table in the one committed authority, keyed by an id the config author picks. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, schemars::JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct NamedPattern { + /// The name a module references it by — the key under + /// `data.batten.patterns`. + /// + /// Unique across the table. A repeated id would make + /// `data.batten.patterns["x"]` ambiguous, and "which declaration answered + /// me" is not a question a reviewer should have to resolve — the same + /// reasoning [`crate::policy`] applies to two rows registering one module. + pub id: String, + /// The expression itself. + /// + /// Compiled at load by [`validate`], so a malformed pattern is a config + /// fault at exit `1` rather than an evaluation failure on the mediated path + /// — house style §8's placement, and the same one `forbid`'s `regex` column + /// already takes. + pub regex: String, +} + +/// Refuse a malformed pattern table, at load. +/// +/// Two rules, and both are about the table rather than about any predicate that +/// might use it: +/// +/// * an id is non-empty and unique; +/// * the expression compiles. +/// +/// # Errors +/// +/// A [`UsageError`] (exit `1`) naming the offending id. The expression itself is +/// a **declaration** — a literal the config author wrote, the class `config +/// show` exists to echo — so naming it is inside non-negotiable rule 4, and +/// without it a refusal over a malformed pattern could not be acted on. +pub fn validate(patterns: &[NamedPattern]) -> anyhow::Result<()> { + let mut seen: BTreeSet<&str> = BTreeSet::new(); + for pattern in patterns { + if pattern.id.trim().is_empty() { + return Err(UsageError::raise(String::from( + "pattern: `id` cannot be blank — it is the name a module references, \ + and an empty one names nothing", + ))); + } + if !seen.insert(pattern.id.as_str()) { + return Err(UsageError::raise(format!( + "pattern `{}` is declared twice; one concept, one spelling — \ + `data.batten.patterns[\"{}\"]` cannot resolve to two expressions", + pattern.id, pattern.id + ))); + } + regex::Regex::new(&pattern.regex).map_err(|err| { + UsageError::raise(format!( + "pattern `{}`: `{}` is not a valid expression: {err}", + pattern.id, pattern.regex + )) + })?; + } + Ok(()) +} + +/// The table as the evaluator's `data` document carries it. +/// +/// `{"batten": {"patterns": {"": ""}}}` — a flat map, because a +/// module's whole use of it is one lookup by name. +#[must_use] +pub fn data_document(patterns: &[NamedPattern]) -> serde_json::Value { + let mut map = serde_json::Map::new(); + for pattern in patterns { + map.insert( + pattern.id.clone(), + serde_json::Value::from(pattern.regex.as_str()), + ); + } + serde_json::json!({ "batten": { "patterns": serde_json::Value::Object(map) } }) +} diff --git a/crates/batten/src/policy.rs b/crates/batten/src/policy.rs index 1e0cce550..98cb4c484 100644 --- a/crates/batten/src/policy.rs +++ b/crates/batten/src/policy.rs @@ -515,6 +515,31 @@ pub fn engines_constructed() -> usize { ENGINES_CONSTRUCTED.load(Ordering::Relaxed) } +/// Whether a `load` re-derives the AST-borne config checks. +/// +/// **A placement decision, and it was measured rather than reasoned.** The two +/// checks below read a module's AST through `Engine::get_ast_as_json`, which +/// serialises every rule of every module in the bundle. Their answer is a +/// property of the module TEXT, so it is identical on every surface and fixed +/// for the life of the load — but `hook` calls `load` once per mediated call, +/// so running them there re-derives a constant answer inside CLOUD-689's 100ms +/// budget. CI measured the cost as `wired` p50 14.03ms -> 22.98ms, a 1.638x +/// regression against a 1.30x gate, on a branch whose local `verify` was green. +/// +/// So they run where a config fault is REPORTED — `check`, `enforce`, `config +/// lint`, `doctor` — which is house style §8's placement independently of the +/// cost, and never on the adjudication path. A module with an inline pattern is +/// refused by this repository's own gate chain exactly as a `no-docs-tree` +/// violation is; what the mediated call must do is load and decide. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ModuleChecks { + /// Re-derive them: the caller is a surface that reports config faults. + Run, + /// Skip them: the caller is the mediated path, where the answer is already + /// known and the budget is per call. + SkipOnHotPath, +} + /// Load, compile and smoke-test every module the rule set registers. /// /// Boundary I/O, called once per process from the config resolution path — never @@ -531,7 +556,19 @@ pub fn engines_constructed() -> usize { /// faults. Every one of those is a config error at load rather than a surprise /// at the gate, which is the whole reason this function drives a query it throws /// away. -pub fn load(root: &Path, rules: &[Rule], reference: Option<&str>) -> Result> { +pub fn load( + root: &Path, + rules: &[Rule], + patterns: &[crate::pattern::NamedPattern], + checks: ModuleChecks, + reference: Option<&str>, +) -> Result> { + // The table is validated at PARSE, beside `verbs` and `redirects` and for + // their reason (`config.rs`'s `VALIDATED_AT_LOAD` census asserts the call + // site exists). Validating again here would be a second authority for one + // question, which is the shape `rules-drift` exists to refuse. + let pattern_data = crate::pattern::data_document(patterns); + let declared_patterns: BTreeSet<&str> = patterns.iter().map(|p| p.id.as_str()).collect(); let mut bundles = Vec::new(); let mut seen: BTreeSet<&str> = BTreeSet::new(); // Every predicate id published so far, and the module that published it — @@ -607,7 +644,7 @@ pub fn load(root: &Path, rules: &[Rule], reference: Option<&str>) -> Result) -> Result) -> Result) -> Result) -> Result Result { +pub fn compile( + id: &str, + sources: &[(String, String)], + // The declared pattern table as a `data` document (CLOUD-885). Handed in + // rather than read here, so `compile` stays a pure function of what it is + // given and the one authority for the table remains `Config`. + data: &serde_json::Value, +) -> Result { // ONE ENGINE FOR THE WHOLE BUNDLE. `add_policy` once per file into it, so // the bundle compiles once and a helper defined in one module is callable // from another. This is how Conftest and OPA load a policy directory, and // constructing the engine outside the loop is the entire fix: it used to sit // inside it, which is why there was no composed rule set to speak of. let mut engine = new_engine(); + // BEFORE `add_policy`, so a module compiled against the table cannot be + // compiled against an empty one. An unusable data document is a config + // fault at exit 1, not a silent empty map — a module reading + // `data.batten.patterns["x"]` against an absent table gets UNDEFINED, and + // Rego reads undefined as "this rule body does not hold", which is + // CLOUD-251's vacuous pass with a regex in it. + engine + .add_data( + regorus::Value::from_json_str(&data.to_string()).map_err(|err| { + UsageError::raise(format!("the declared pattern table is not usable: {err}")) + })?, + ) + .map_err(|err| { + UsageError::raise(format!( + "the declared pattern table could not be loaded: {err}" + )) + })?; let mut modules = Vec::new(); for (path, source) in sources { engine @@ -1134,6 +1201,114 @@ fn check_tree_paths_are_emittable(rule: &Rule, bundle: &Bundle, source: &str) -> Ok(()) } +/// Refuse a regex written inline in a module (CLOUD-885). +/// +/// **The lever is cost, not prohibition**, and this is the half that applies it. +/// "Do not regex things that are not regular" cannot be a gate — that is a +/// judgement, and non-negotiable rule 3 says a gate resolves to a command and an +/// exit code over an object it decides. Where a pattern LIVES is decidable, so +/// that is what is decided: a regex costs an id and a row in `batten.toml`, +/// while the same question asked of a parsed document costs a field access. The +/// cheap path becomes the correct one without anyone having to reason about +/// regularity. +/// +/// Three things follow, and [`crate::pattern`] carries the argument for each: +/// rule 1 (a tracker-key expression is a consumer identifier and belongs in the +/// consumer's config), duplication becoming unwritable rather than merely +/// detectable, and the pattern inventory becoming reviewable data (§11). +/// +/// **`test_` rules are exempt**, and it is a real case rather than a hatch: a +/// test legitimately matches a declared pattern against a literal subject +/// (`regex.match(patterns.key, "CLOUD-1")`), which this check would otherwise +/// read as an inline pattern. The prefix is the one `policy test` already keys +/// on, so no second convention is introduced. +/// +/// # Errors +/// +/// A [`UsageError`] (exit `1`) naming the module and the remedy (CLOUD-437). +/// **The expression is named**, and that is inside rule 4 rather than an +/// exception to it: a pattern is a declaration the config author wrote — the +/// class `config show` exists to echo — not content read out of a subject file. +fn check_no_inline_regex( + rule: &Rule, + bundle: &Bundle, + declared: &BTreeSet<&str>, + source: &str, +) -> Result<()> { + let Some(described) = describe(&bundle.engine) else { + // Could-not-look on the AST is not a refusal, for the reason the sibling + // checks state: `load` has already compiled this module, so a shape this + // reader does not recognise is its own limitation, and failing the config + // over it would make a reader upgrade a breaking change. + return Ok(()); + }; + // A REFERENCE TO AN UNDECLARED ID IS THE SAME DEFECT ONE STEP LATER, and + // refusing the inline form without refusing this would leave the hole the + // mechanism exists to close: `data.batten.patterns["typo"]` resolves to + // UNDEFINED, Rego reads undefined as "this rule body does not hold", so the + // module loads clean, evaluates clean and gates nothing. That is CLOUD-251's + // vacuous pass, and it is exactly what `WeakeningKind::PatternRemoved` + // describes arriving by a different route — a typo rather than a deletion. + // Same shape as `check_tree_paths_are_emittable`: refuse a reference the + // engine cannot satisfy, at load, against the table rather than a list. + // A VENDORED PRESET IS EXEMPT, and it is the rule's own scope rather than a + // hatch in it. The declaration requirement exists because a pattern written + // into a module smuggles a CONSUMER fact into a place consumer facts may not + // live (non-negotiable rule 1) — which is why `Config::verbs` states the + // same argument for its table. A preset ships INSIDE the crate, so rule 1 + // already forces its patterns to be repo-agnostic: `shell-hygiene`'s + // `\$\{?BASH_SOURCE|\$0` names no consumer and could not, or the preset + // itself would fail the rule. + // + // It is also unsatisfiable as a demand. A preset is compiled in; a consumer + // cannot add a `[[pattern]]` row on its behalf, and the preset cannot read + // one — so refusing it would make a vendored bundle unloadable with no fix + // available, which is the wrongly-refusing gate AGENTS.md calls a defect. + // Caught by `the_committed_delegating_rule_*` on the first run against a + // preset that uses one. + if rule.preset.is_some() { + return Ok(()); + } + for module in &described { + for rule_ast in &module.rules { + for referenced in &rule_ast.pattern_refs { + if declared.contains(referenced.as_str()) { + continue; + } + let mut known: Vec<&str> = declared.iter().copied().collect(); + known.sort_unstable(); + return Err(UsageError::raise(format!( + "rule `{}` registers `{source}`, whose module {} references \ +`data.batten.patterns[\"{referenced}\"]`, which no `[[pattern]]` row declares — the \ +reference would be undefined and the predicate silent. Declared: {}", + rule.id, + module.path, + if known.is_empty() { + String::from("none") + } else { + known.join(", ") + }, + ))); + } + if rule_ast.name.starts_with("test_") { + continue; + } + let Some(pattern) = rule_ast.inline_regex.first() else { + continue; + }; + return Err(UsageError::raise(format!( + "rule `{}` registers `{source}`, whose module {} writes the regex `{pattern}` \ +inline in `{}`. Declare it once as a `[[pattern]]` row and reference it as \ +`data.batten.patterns[\"\"]`: an expression is a consumer fact, so it belongs in \ +the config rather than in a module (rule 1), and a named pattern has one home, which \ +is what stops one concept acquiring several spellings", + rule.id, module.path, rule_ast.name, + ))); + } + } + Ok(()) +} + /// The keys `rules::tree_document` emits, derived from the fact model. /// /// Named once here so the refusal above and the engine agree by construction @@ -1318,6 +1493,18 @@ struct DescribedRule { /// literals beside it. Paths only: a reference is a NAME, never a value, so /// rule 4 has nothing to say about carrying it. input_paths: Vec, + /// Every string literal this rule hands to a `regex.*` builtin (CLOUD-885). + /// + /// A subset of [`Self::literals`], separated because the question is + /// different: that field binds a predicate id to the rule raising it, this + /// one asks whether a pattern was written inline instead of declared. + inline_regex: Vec, + /// Every `data.batten.patterns[""]` this rule references (CLOUD-885). + /// + /// The ids only. A reference the config does not declare resolves to + /// undefined, which Rego reads as "this rule body does not hold" — so this + /// is what makes a typo a refusal rather than a silent disarm. + pattern_refs: Vec, } /// Read every module's rule names, spans and literals off the compiled AST. @@ -1363,11 +1550,17 @@ fn describe(engine: ®orus::Engine) -> Option> { collect_literals(rule, &mut literals); let mut input_paths = Vec::new(); collect_input_paths(rule, &mut input_paths); + let mut inline_regex = Vec::new(); + collect_inline_regex(rule, &mut inline_regex); + let mut pattern_refs = Vec::new(); + collect_pattern_refs(rule, &mut pattern_refs); rules.push(DescribedRule { name, head_line, literals, input_paths, + inline_regex, + pattern_refs, }); } described.push(Described { @@ -1451,6 +1644,126 @@ fn collect_input_paths(value: &serde_json::Value, found: &mut Vec) { } } +/// Every string literal a rule hands to a `regex.*` builtin (CLOUD-885). +/// +/// A call is `{"Call": {"fcn": , "params": [, …]}}`, and `fcn` +/// resolves through [`reference_path`] exactly as an input reference does — +/// `regex.match` is a `RefDot` over the var `regex`. +/// +/// **Every parameter, and the recursion into each is what closes the hole.** +/// The builtins disagree on argument order — `regex.match(pattern, value)` +/// against `regex.replace(s, pattern, value)` — so a per-builtin position table +/// would be a second thing to keep in step with upstream. Reading them all is +/// wider, and the width is load-bearing rather than lazy: recursing means +/// `regex.match(concat("", ["CLOUD", "-[0-9]+"]), s)` is caught too, which a +/// direct-parameter check would wave through. +/// +/// A reference to `data.batten.patterns["x"]` carries no literal, so the +/// sanctioned form passes by construction rather than by exemption. +fn collect_inline_regex(value: &serde_json::Value, found: &mut Vec) { + match value { + serde_json::Value::Object(object) => { + if let Some(call) = object.get("Call") + && let Some(name) = call.get("fcn").and_then(reference_path) + && name.starts_with("regex.") + && let Some(params) = call.get("params").and_then(serde_json::Value::as_array) + { + for param in params { + // A REFERENCE IS NOT A LITERAL, even though it contains one. + // `data.batten.patterns["x"]` is a `RefBrack` whose index is + // the string `"x"`, so a naive literal sweep reads the + // sanctioned form as the refused one and no module can load + // at all. Skipping the subtree is right rather than + // convenient: the id is a NAME, and the expression it names + // lives in the config, which is the property being enforced. + if reference_path(param) + .is_some_and(|path| path.starts_with("data.batten.patterns.")) + { + continue; + } + // BOTH SPELLINGS. A Rego backtick literal serialises as + // `RawString`, not `String`, and it is the spelling a regex + // is almost always written in — backticks are what let a + // pattern carry backslashes unescaped. Reading only + // `collect_literals` let every realistic inline pattern + // through, which is what the AST probe measured rather than + // what this reader assumed. + collect_string_values(param, "String", found); + collect_string_values(param, "RawString", found); + } + } + for child in object.values() { + collect_inline_regex(child, found); + } + } + serde_json::Value::Array(items) => { + for item in items { + collect_inline_regex(item, found); + } + } + _ => {} + } +} + +/// Every pattern id a rule reaches through `data.batten.patterns[…]` +/// (CLOUD-885). +/// +/// [`reference_path`] already resolves a string-literal index, so +/// `data.batten.patterns["x"]` arrives as the dotted path +/// `data.batten.patterns.x` and the id is its last segment. A VARIABLE index is +/// deliberately not resolved — that path is not statically knowable, and +/// answering `None` for it is could-not-look rather than a guess, which is the +/// same posture `reference_path` already takes. +fn collect_pattern_refs(value: &serde_json::Value, found: &mut Vec) { + const PREFIX: &str = "data.batten.patterns."; + match value { + serde_json::Value::Object(object) => { + if let Some(path) = reference_path(value) + && let Some(rest) = path.strip_prefix(PREFIX) + && !rest.is_empty() + { + found.push(rest.split('.').next().unwrap_or(rest).to_owned()); + } + for child in object.values() { + collect_pattern_refs(child, found); + } + } + serde_json::Value::Array(items) => { + for item in items { + collect_pattern_refs(item, found); + } + } + _ => {} + } +} + +/// Every `{"": {"value": "…"}}` node in a subtree, for one node kind. +/// +/// Rego has two literal spellings and regorus gives them different nodes: +/// `"x"` is a `String` and `` `x` `` is a `RawString`. A reader that knows only +/// one of them is blind to the other, and for a regex the backtick form is the +/// usual one, since it carries backslashes unescaped. +fn collect_string_values(value: &serde_json::Value, kind: &str, found: &mut Vec) { + match value { + serde_json::Value::Object(object) => { + if let Some(node) = object.get(kind) + && let Some(text) = node.get("value").and_then(serde_json::Value::as_str) + { + found.push(text.to_owned()); + } + for child in object.values() { + collect_string_values(child, kind, found); + } + } + serde_json::Value::Array(items) => { + for item in items { + collect_string_values(item, kind, found); + } + } + _ => {} + } +} + /// Every string literal in a rule's AST subtree. fn collect_literals(value: &serde_json::Value, found: &mut Vec) { match value { diff --git a/crates/batten/src/resolve.rs b/crates/batten/src/resolve.rs index eb8364868..14e91a839 100644 --- a/crates/batten/src/resolve.rs +++ b/crates/batten/src/resolve.rs @@ -294,6 +294,10 @@ pub struct Resolved { /// The mutating-verb table, consumer data the authority supplies. #[serde(rename = "verb")] pub verbs: Vec, + /// The named-regex table (CLOUD-885), consumer data the authority supplies — + /// carried for [`Resolved::verbs`]'s reason and layered the same way. + #[serde(rename = "pattern")] + pub patterns: Vec, /// The per-path-class redirect table (CLOUD-280), authority rows plus any a /// local file **added**. Local rows append after committed ones, and the /// lookup takes the first match, so an uncommitted file can add a class the @@ -1146,6 +1150,7 @@ fn assemble( unlanded: paths.unlanded, epoch: repo.epoch.clone(), verbs: repo.verbs.clone(), + patterns: repo.patterns.clone(), redirects: tables.redirects, facts: tables.facts, markers: repo.markers.clone(), @@ -1207,6 +1212,7 @@ fn attribution( ("unlanded", paths.unlanded_source.clone()), ("epoch", authority_set(repo.epoch.is_some())), ("verb", authority_set(!repo.verbs.is_empty())), + ("pattern", authority_set(!repo.patterns.is_empty())), ("marker", authority_set(!repo.markers.is_empty())), ( "exec_pattern", diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index da2946414..205605ca8 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -2790,6 +2790,9 @@ pub const SPAWNING_VERB: &str = "batten enforce"; pub fn run_static( rules: &[Rule], _provisions: &[crate::provision::Provision], + // The declared pattern table (CLOUD-885), riding beside `provisions` for the + // same reason it does: a second config table the rule set evaluates against. + patterns: &[crate::pattern::NamedPattern], root: &Path, ) -> anyhow::Result { // POLICY BUNDLES ARE LOADED HERE, on the read surface, and that is @@ -2801,7 +2804,13 @@ pub fn run_static( // process or reach the network, a property CLOUD-831 gates rather than // asserts. So admitting it here makes `check` MORE capable without making it // less honest, and the spawning refusal below is untouched. - let bundles = crate::policy::load(root, rules, None)?; + let bundles = crate::policy::load( + root, + rules, + patterns, + crate::policy::ModuleChecks::Run, + None, + )?; // Refuse before any work: the read-only surface must not even begin a run // it cannot complete honestly. for rule in rules { @@ -2862,13 +2871,20 @@ pub fn run_static( pub fn run_recorded( rules: &[Rule], provisions: &[crate::provision::Provision], + patterns: &[crate::pattern::NamedPattern], root: &Path, ) -> anyhow::Result { let (evaluable, withheld): (Vec<&Rule>, Vec<&Rule>) = rules .iter() .partition(|rule| !rule.kind.carries_ambient_authority()); let evaluable: Vec = evaluable.into_iter().cloned().collect(); - let bundles = crate::policy::load(root, &evaluable, None)?; + let bundles = crate::policy::load( + root, + &evaluable, + patterns, + crate::policy::ModuleChecks::Run, + None, + )?; let mut scan = run(&evaluable, provisions, root, &bundles)?; for rule in withheld { // `RuleSkipped`, not a variant of its own. The distinction between "the @@ -2898,6 +2914,7 @@ pub fn run_recorded( pub fn run_all( rules: &[Rule], provisions: &[crate::provision::Provision], + patterns: &[crate::pattern::NamedPattern], root: &Path, ) -> anyhow::Result { // Refuse before any work, the shape `run_static` above already uses: the @@ -2913,7 +2930,13 @@ pub fn run_all( ))); } } - let bundles = crate::policy::load(root, rules, None)?; + let bundles = crate::policy::load( + root, + rules, + patterns, + crate::policy::ModuleChecks::Run, + None, + )?; run(rules, provisions, root, &bundles) } @@ -5844,11 +5867,11 @@ mod tests { /// found; [`Scan::not_evaluated`] has its own tests, so shadowing keeps the /// suite reading as it did before that half existed. fn run_static(rules: &[Rule], root: &Path) -> anyhow::Result> { - super::run_static(rules, &[], root).map(|scan| scan.findings) + super::run_static(rules, &[], &[], root).map(|scan| scan.findings) } fn run_all(rules: &[Rule], root: &Path) -> anyhow::Result> { - super::run_all(rules, &[], root).map(|scan| scan.findings) + super::run_all(rules, &[], &[], root).map(|scan| scan.findings) } fn forbid(id: &str, glob: &str, pattern: &str) -> Rule { @@ -6022,7 +6045,8 @@ mod tests { let dir = temp_dir("scan-skipped"); write(&dir, "src/a.rs", "fine\n"); - let clean = super::run_static(&[forbid("looked", "**/*.rs", "TODO")], &[], &dir).unwrap(); + let clean = + super::run_static(&[forbid("looked", "**/*.rs", "TODO")], &[], &[], &dir).unwrap(); assert!(clean.findings.is_empty()); assert!( clean.not_evaluated.is_empty(), @@ -6030,7 +6054,8 @@ mod tests { ); let skipped = - super::run_static(&[forbid("never-looked", "**/*.md", "TODO")], &[], &dir).unwrap(); + super::run_static(&[forbid("never-looked", "**/*.md", "TODO")], &[], &[], &dir) + .unwrap(); assert!(skipped.findings.is_empty()); assert_eq!( skipped.not_evaluated.get("never-looked"), @@ -6046,6 +6071,7 @@ mod tests { ..forbid("switched-off", "**/*.rs", "TODO") }], &[], + &[], &dir, ) .unwrap(); diff --git a/crates/batten/src/trust.rs b/crates/batten/src/trust.rs index 1b90efe40..ceae9439a 100644 --- a/crates/batten/src/trust.rs +++ b/crates/batten/src/trust.rs @@ -540,6 +540,16 @@ pub enum WeakeningKind { /// A `[[verb]]` row is gone, so a mutating tool call is no longer mediated /// at the `PreToolUse` boundary (CLOUD-36). VerbRemoved, + /// A `[[pattern]]` row is gone, so every module referencing it resolves to + /// **undefined** (CLOUD-885). + /// + /// **This is a silent disarm rather than a load failure**, which is what + /// makes it belong on this table. Rego reads an undefined reference as "this + /// rule body does not hold", so a module whose pattern was deleted loads + /// clean, evaluates clean, and gates nothing — CLOUD-251's vacuous pass, + /// reachable from a local override. Removing one row can silence several + /// predicates at once, since a declared pattern is shared by design. + PatternRemoved, /// An agent-sourced fact's declared command changed (CLOUD-776). /// /// The one weakening on this table whose payoff is a FORGED FACT rather than @@ -637,6 +647,7 @@ impl WeakeningKind { WeakeningKind::MinVersionLowered, WeakeningKind::EpochPathRemoved, WeakeningKind::VerbRemoved, + WeakeningKind::PatternRemoved, WeakeningKind::FactCommandChanged, WeakeningKind::FactRemoved, WeakeningKind::MarkerRemoved, @@ -676,6 +687,7 @@ impl WeakeningKind { WeakeningKind::MinVersionLowered => "min-version-lowered", WeakeningKind::EpochPathRemoved => "epoch-path-removed", WeakeningKind::VerbRemoved => "verb-removed", + WeakeningKind::PatternRemoved => "pattern-removed", WeakeningKind::FactCommandChanged => "fact-command-changed", WeakeningKind::FactRemoved => "fact-removed", WeakeningKind::MarkerRemoved => "marker-removed", @@ -788,6 +800,10 @@ pub const CENSUS: &[FieldCoverage] = &[ field: "verbs", coverage: Coverage::Compared(&[WeakeningKind::VerbRemoved]), }, + FieldCoverage { + field: "patterns", + coverage: Coverage::Compared(&[WeakeningKind::PatternRemoved]), + }, FieldCoverage { field: "facts", coverage: Coverage::Compared(&[ @@ -1083,6 +1099,16 @@ fn entry_weakenings(base: &Config, working: &Config) -> Vec { "verb", )); + // The named-regex table (CLOUD-885): removing a row does not fail a load, it + // makes every reference to it undefined, and Rego reads undefined as "does + // not hold". So the predicates go quiet rather than red. + found.extend(removed_entries( + WeakeningKind::PatternRemoved, + &pattern_entries(base), + &pattern_entries(working), + "pattern", + )); + // The agent-sourced facts (CLOUD-776). Removal is reported and is a // tightening; a CHANGED command is the dangerous direction, because the same // string is both what the agent is told to run and what the record is checked @@ -1253,6 +1279,14 @@ fn verb_entries(config: &Config) -> Vec { .collect() } +/// The declared pattern ids, so [`removed_entries`] can compare them. +/// +/// The **id** and never the expression: a removal is identified by the name a +/// module references, which is also what keeps this a pointer (rule 4). +fn pattern_entries(config: &Config) -> Vec { + config.patterns.iter().map(|row| row.id.clone()).collect() +} + /// The ids of a table, collected so [`removed_entries`] can compare them. fn ids(entries: impl Iterator) -> Vec { entries.collect() @@ -2259,6 +2293,34 @@ mod tests { assert!(weakenings(&working, &base).is_empty()); } + #[test] + fn removing_a_declared_pattern_is_a_weakening() { + // NOT A LOAD FAILURE, which is the whole reason this is on the table. + // Every module referencing a deleted pattern resolves to UNDEFINED, and + // Rego reads undefined as "this rule body does not hold" — so the + // predicates go quiet rather than red, and one removed row can silence + // several at once, because a declared pattern is shared by design + // (CLOUD-885). That is CLOUD-251's vacuous pass reachable from a local + // override. + let base = config( + "\n[[pattern]]\nid = \"tracker-key\"\nregex = \"CLOUD-[0-9]+\"\n\n\ + [[pattern]]\nid = \"sha\"\nregex = \"[0-9a-f]{7,40}\"\n", + ); + let working = config("\n[[pattern]]\nid = \"tracker-key\"\nregex = \"CLOUD-[0-9]+\"\n"); + assert_eq!( + only(&base, &working), + Weakening::new( + WeakeningKind::PatternRemoved, + "pattern[sha]", + "present", + "absent", + ) + ); + // The other direction is a tightening: adding a pattern arms nothing on + // its own, since a module has to reference it. + assert!(weakenings(&working, &base).is_empty()); + } + fn verb_row(verb: &str, subcommand: Option<&str>) -> String { let qualifier = subcommand.map_or_else(String::new, |sub| format!("subcommand = \"{sub}\"\n")); diff --git a/crates/batten/tests/document_read_count.rs b/crates/batten/tests/document_read_count.rs index db0e6178c..1e71fe127 100644 --- a/crates/batten/tests/document_read_count.rs +++ b/crates/batten/tests/document_read_count.rs @@ -95,6 +95,7 @@ fn rows_declaring_one_path_read_it_once() { row("third", &["config.toml"]), ], &[], + &[], &root, ) .expect("the read surface runs the rows"); @@ -117,7 +118,7 @@ fn rows_declaring_one_path_read_it_once() { let before = rules::documents_acquired(); fs::write(root.join("other.toml"), "stray = true\n").expect("fixture"); write_bundles(&root, &["fourth"]); - let _ = rules::run_static(&[row("fourth", &["other.toml"])], &[], &root); + let _ = rules::run_static(&[row("fourth", &["other.toml"])], &[], &[], &root); assert!( rules::documents_acquired() > before, "the counter moves for a path not already cached, so the delta above \ @@ -148,7 +149,7 @@ fn a_glob_source_resolves_against_the_walk_rather_than_being_read_literally() { })) .expect("a row declaring a selector"); - let scan = rules::run_static(&[globbed], &[], &root).expect("the selector resolves"); + let scan = rules::run_static(&[globbed], &[], &[], &root).expect("the selector resolves"); assert_eq!( scan.findings.len(), 1, diff --git a/crates/batten/tests/identity_churn.rs b/crates/batten/tests/identity_churn.rs index 9d60919b4..e9a080c21 100644 --- a/crates/batten/tests/identity_churn.rs +++ b/crates/batten/tests/identity_churn.rs @@ -76,7 +76,7 @@ impl Scan { /// written against the whole matched line, and they still pass, so the /// engine demonstrably picks the same span the test used to. fn of(root: &Path, rules: &[Rule]) -> Self { - let findings = rules::run_static(rules, &[], root) + let findings = rules::run_static(rules, &[], &[], root) .expect("scan the tree") .findings; let identities = identity::count_occurrences( diff --git a/crates/batten/tests/policy_engine_count.rs b/crates/batten/tests/policy_engine_count.rs index 902911f2e..005187831 100644 --- a/crates/batten/tests/policy_engine_count.rs +++ b/crates/batten/tests/policy_engine_count.rs @@ -41,7 +41,8 @@ fn a_bundle_of_n_modules_constructs_exactly_one_engine() { .collect(); let before = policy::engines_constructed(); - let bundle = policy::compile("policy-many", &sources).expect("five modules, one bundle"); + let bundle = policy::compile("policy-many", &sources, &serde_json::json!({})) + .expect("five modules, one bundle"); let after = policy::engines_constructed(); assert_eq!( diff --git a/crates/batten/tests/policy_modules.rs b/crates/batten/tests/policy_modules.rs index 8261bf01c..b45e73768 100644 --- a/crates/batten/tests/policy_modules.rs +++ b/crates/batten/tests/policy_modules.rs @@ -223,7 +223,14 @@ fn scratch(name: &str) -> std::path::PathBuf { fn a_module_denies_on_a_fact_and_is_silent_otherwise() { let root = scratch("denies"); let path = module_file(&root, "writes.rego", DENIES_WRITES); - let bundles = policy::load(&root, &[row("policy-writes", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-writes", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); assert_eq!(bundles.len(), 1); let denied = policy::deny(&bundles[0], r#"{"call":{"operation":"write"}}"#); @@ -248,7 +255,14 @@ fn a_module_denies_on_a_fact_and_is_silent_otherwise() { fn an_unparseable_input_is_could_not_look_and_never_an_empty_deny_set() { let root = scratch("couldnotlook"); let path = module_file(&root, "writes.rego", DENIES_WRITES); - let bundles = policy::load(&root, &[row("policy-writes", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-writes", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); let answer = policy::deny(&bundles[0], "{not json"); assert!( @@ -265,8 +279,14 @@ fn a_cyclic_module_is_refused_at_load() { // in `load`, this module compiles clean here and faults at the gate. let root = scratch("cyclic"); let path = module_file(&root, "cyclic.rego", CYCLIC); - let err = policy::load(&root, &[row("policy-cyclic", &path)], None) - .expect_err("a cycle is a config error, not a runtime surprise"); + let err = policy::load( + &root, + &[row("policy-cyclic", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("a cycle is a config error, not a runtime surprise"); let text = format!("{err}"); assert!( text.contains("cyclic.rego"), @@ -288,8 +308,14 @@ fn a_cyclic_module_is_refused_at_load() { #[test] fn a_module_that_cannot_be_read_is_refused_at_load() { let root = scratch("absent"); - let err = policy::load(&root, &[row("policy-absent", "nowhere.rego")], None) - .expect_err("a registration naming no file decides nothing and must not load"); + let err = policy::load( + &root, + &[row("policy-absent", "nowhere.rego")], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("a registration naming no file decides nothing and must not load"); assert!(format!("{err}").contains("nowhere.rego")); } @@ -300,11 +326,128 @@ fn two_rows_registering_one_module_are_refused_at_load() { // answer. Same reasoning as the duplicate derived-value name (CLOUD-773). let root = scratch("duplicate"); let path = module_file(&root, "writes.rego", DENIES_WRITES); - let err = policy::load(&root, &[row("first", &path), row("second", &path)], None) - .expect_err("one module, two registrations"); + let err = policy::load( + &root, + &[row("first", &path), row("second", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("one module, two registrations"); assert!(format!("{err}").contains("already registers")); } +/// A pattern written inline, which is what a bash translation reaches for. +const INLINE_REGEX: &str = r#" +package batten + +import rego.v1 + +deny contains "names a tracker key" if { + regex.match(`CLOUD-[0-9]+`, input.call.command) +} +"#; + +/// The same predicate against a declared pattern — the sanctioned form. +const DECLARED_REGEX: &str = r#" +package batten + +import rego.v1 + +deny contains "names a tracker key" if { + regex.match(data.batten.patterns["tracker-key"], input.call.command) +} +"#; + +/// A literal smuggled past a direct-parameter check by building it up. +const SMUGGLED_REGEX: &str = r#" +package batten + +import rego.v1 + +deny contains "names a tracker key" if { + regex.match(concat("", ["CLOUD", "-[0-9]+"]), input.call.command) +} +"#; + +fn tracker_key() -> batten::pattern::NamedPattern { + batten::pattern::NamedPattern { + id: String::from("tracker-key"), + regex: String::from("CLOUD-[0-9]+"), + } +} + +#[test] +fn a_regex_written_inline_is_refused_and_a_declared_one_decides() { + // THE LEVER IS COST, NOT PROHIBITION. "Do not regex things that are not + // regular" cannot be a gate — that is a judgement, and non-negotiable rule 3 + // says a gate resolves to a command and an exit code over an object it + // decides. Where a pattern LIVES is decidable, so a regex costs an id and a + // config row while the same question over a parsed document costs a field + // access. The cheap path becomes the correct one without anyone reasoning + // about regularity. + let root = scratch("inline-regex"); + let inline = module_file(&root, "inline.rego", INLINE_REGEX); + let err = policy::load( + &root, + &[row("keys", &inline)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("a regex written inline must not load"); + let rendered = format!("{err}"); + assert!( + rendered.contains("CLOUD-[0-9]+"), + "the refusal names the expression, a declaration and not content: {rendered}" + ); + assert!( + rendered.contains("[[pattern]]"), + "the refusal names its remedy (CLOUD-437): {rendered}" + ); + + // THE DISCRIMINATING HALF, and it is two claims rather than one. A check + // that refused every `regex.*` call would satisfy the assertions above and + // be useless, so the declared form must LOAD — and it must also DECIDE, + // because a module whose pattern resolved to undefined would load clean and + // gate nothing, which is CLOUD-251's vacuous pass with a regex in it. So + // this asserts the deny, not merely the load. + let declared = module_file(&root, "declared.rego", DECLARED_REGEX); + let bundles = policy::load( + &root, + &[row("keys", &declared)], + &[tracker_key()], + policy::ModuleChecks::Run, + None, + ) + .expect("a declared pattern is the sanctioned form"); + let doc = r#"{"call":{"command":"git commit -m CLOUD-885"}}"#; + let Look::Is(violations) = policy::deny(&bundles[0], doc) else { + panic!("the declared pattern must reach the module through `data`"); + }; + assert_eq!( + violations, + vec![unattributed("names a tracker key")], + "the projected table decides, rather than resolving to undefined" + ); + + // THE SMUGGLE. A check reading only direct parameters would wave this + // through; recursing into each parameter is what closes it. + let smuggled = module_file(&root, "smuggled.rego", SMUGGLED_REGEX); + let err = policy::load( + &root, + &[row("keys", &smuggled)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("a literal assembled inside the call is still a literal"); + assert!( + format!("{err}").contains("[[pattern]]"), + "the smuggled form gets the same refusal: {err}" + ); +} + #[test] fn a_module_holds_no_source_and_cannot_leak_one_through_debug() { // Rule 4 is structural here rather than careful: `Module` has no `source` @@ -312,7 +455,14 @@ fn a_module_holds_no_source_and_cannot_leak_one_through_debug() { // re-derived it. This asserts the rendering, which is the reachable half. let root = scratch("pointer"); let path = module_file(&root, "writes.rego", DENIES_WRITES); - let bundles = policy::load(&root, &[row("policy-writes", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-writes", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); let rendered = format!("{:?}", bundles[0]); assert!(rendered.contains("policy-writes"), "the pointer is present"); assert!( @@ -369,8 +519,14 @@ fn no_evaluator_feature_admits_io() { // The control first. If this does not deny, nothing below discriminates. let included = module_file(&root, "included.rego", REACHES_AN_INCLUDED_BUILTIN); - let bundles = policy::load(&root, &[row("policy-included", &included)], None) - .expect("a module over an in-closure builtin loads"); + let bundles = policy::load( + &root, + &[row("policy-included", &included)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("a module over an in-closure builtin loads"); assert_eq!( policy::deny(&bundles[0], "{}"), Look::Is(vec![unattributed( @@ -385,7 +541,13 @@ fn no_evaluator_feature_admits_io() { // property the doc claims; both arms are accepted here and the assertion is // over the outcome that matters. let network = module_file(&root, "network.rego", REACHES_THE_NETWORK); - match policy::load(&root, &[row("policy-network", &network)], None) { + match policy::load( + &root, + &[row("policy-network", &network)], + &[], + policy::ModuleChecks::Run, + None, + ) { Err(refused) => { let text = format!("{refused}"); assert!( @@ -409,7 +571,13 @@ fn no_evaluator_feature_admits_io() { // manifest pins out. Same shape: a test covering one of the two would report // the pin held while half of it drifted. let schema = module_file(&root, "schema.rego", REACHES_JSONSCHEMA); - match policy::load(&root, &[row("policy-schema", &schema)], None) { + match policy::load( + &root, + &[row("policy-schema", &schema)], + &[], + policy::ModuleChecks::Run, + None, + ) { Err(refused) => { let text = format!("{refused}"); assert!( @@ -439,7 +607,14 @@ fn no_evaluator_feature_admits_io() { fn one_module_carries_two_predicates_that_deny_under_their_own_ids() { let root = scratch("two-predicates"); let path = module_file(&root, "two.rego", TWO_PREDICATES); - let bundles = policy::load(&root, &[row("policy-two", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-two", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); let denied = policy::deny(&bundles[0], r#"{"call":{"operation":"write"}}"#); let Look::Is(violations) = denied else { @@ -492,7 +667,14 @@ fn one_module_carries_two_predicates_that_deny_under_their_own_ids() { fn a_bare_string_deny_still_reports_under_the_registering_row() { let root = scratch("bare-string"); let path = module_file(&root, "writes.rego", DENIES_WRITES); - let bundles = policy::load(&root, &[row("policy-writes", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-writes", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); assert!( bundles[0].declared().is_empty(), @@ -523,8 +705,14 @@ fn a_bare_string_deny_still_reports_under_the_registering_row() { fn an_undeclared_violation_id_is_refused_at_load() { let root = scratch("undeclared"); let path = module_file(&root, "undeclared.rego", UNDECLARED_ID); - let err = policy::load(&root, &[row("policy-undeclared", &path)], None) - .expect_err("an id the module does not publish cannot be attributed"); + let err = policy::load( + &root, + &[row("policy-undeclared", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("an id the module does not publish cannot be attributed"); let text = format!("{err}"); assert!( text.contains("never-declared"), @@ -556,6 +744,8 @@ fn two_modules_declaring_one_id_are_refused_at_load() { let err = policy::load( &root, &[row("policy-a", &first), row("policy-b", &second)], + &[], + policy::ModuleChecks::Run, None, ) .expect_err("one id, two publishers"); @@ -585,7 +775,14 @@ fn two_modules_declaring_one_id_are_refused_at_load() { fn a_waiver_over_one_predicate_does_not_suppress_its_sibling() { let root = scratch("waiver-sibling"); let path = module_file(&root, "two.rego", TWO_PREDICATES); - let bundles = policy::load(&root, &[row("policy-two", &path)], None).expect("load"); + let bundles = policy::load( + &root, + &[row("policy-two", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load"); let Look::Is(violations) = policy::deny(&bundles[0], r#"{"call":{"operation":"write"}}"#) else { @@ -636,8 +833,14 @@ violation contains {"rule": "only-on-a-write", "msg": "reached later"} if { } "#; let path = module_file(&root, "late.rego", source); - let bundles = policy::load(&root, &[row("policy-late", &path)], None) - .expect("load cannot reach this violation, so it loads"); + let bundles = policy::load( + &root, + &[row("policy-late", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("load cannot reach this violation, so it loads"); let answer = policy::deny(&bundles[0], r#"{"call":{"operation":"write"}}"#); assert!( @@ -668,7 +871,7 @@ fn a_predicate_severity_naming_an_unpublished_id_is_refused_at_load() { .into_iter() .collect(), ); - let err = policy::load(&root, &[rule], None) + let err = policy::load(&root, &[rule], &[], policy::ModuleChecks::Run, None) .expect_err("a severity aimed at an id nothing publishes decides nothing"); let text = format!("{err}"); assert!(text.contains("no-such-predicate"), "names the key: {text}"); @@ -692,7 +895,8 @@ fn severity_resolves_per_predicate_and_falls_back_to_the_row() { .into_iter() .collect(), ); - let bundles = policy::load(&root, &[rule.clone()], None).expect("load"); + let bundles = + policy::load(&root, &[rule.clone()], &[], policy::ModuleChecks::Run, None).expect("load"); assert_eq!( rule.severity_for(Some("no-stray-artifact")), @@ -747,8 +951,14 @@ violation contains {"rule": "no-force-push", "msg": "a force push at the trunk"} } "#; let path = module_file(&root, "git.rego", source); - let bundles = - policy::load(&root, &[row("policy-git", &path)], None).expect("a sub-package module loads"); + let bundles = policy::load( + &root, + &[row("policy-git", &path)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("a sub-package module loads"); assert!( bundles[0].declared().contains("no-force-push"), @@ -812,6 +1022,7 @@ violation contains {"rule": "no-protected-write", "msg": "a write under a protec ("shared.rego".to_owned(), helper.to_owned()), ("consumer.rego".to_owned(), consumer.to_owned()), ], + &serde_json::json!({}), ) .expect("two modules compose into one rule set"); @@ -876,6 +1087,7 @@ fn a_bundle_of_n_modules_is_one_engine_by_construction() { "package batten.c\nimport rego.v1\nrules contains \"c\"\n".to_owned(), ), ], + &serde_json::json!({}), ) .expect("three modules, one bundle"); diff --git a/crates/batten/tests/policy_presets.rs b/crates/batten/tests/policy_presets.rs index fce2da8f8..bc4f897ed 100644 --- a/crates/batten/tests/policy_presets.rs +++ b/crates/batten/tests/policy_presets.rs @@ -54,8 +54,14 @@ fn scratch(name: &str) -> PathBuf { #[test] fn a_preset_predicate_denies_and_is_green_by_turns() { let root = scratch("denies"); - let bundles = policy::load(&root, &[preset_row("trunk", "trunk-based")], None) - .expect("a vendored preset loads"); + let bundles = policy::load( + &root, + &[preset_row("trunk", "trunk-based")], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("a vendored preset loads"); let Look::Is(violations) = policy::deny(&bundles[0], &call("git push --force origin topic")) else { @@ -92,8 +98,14 @@ fn a_preset_predicate_denies_and_is_green_by_turns() { #[test] fn the_commit_hygiene_preset_decides_both_ways() { let root = scratch("hygiene"); - let bundles = policy::load(&root, &[preset_row("hygiene", "commit-hygiene")], None) - .expect("the preset loads"); + let bundles = policy::load( + &root, + &[preset_row("hygiene", "commit-hygiene")], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("the preset loads"); let Look::Is(violations) = policy::deny(&bundles[0], &call("git commit --allow-empty -m x")) else { @@ -114,8 +126,14 @@ fn the_commit_hygiene_preset_decides_both_ways() { #[test] fn an_unknown_preset_name_is_refused_at_load() { let root = scratch("unknown"); - let err = policy::load(&root, &[preset_row("typo", "trunk-basd")], None) - .expect_err("a name this binary does not ship"); + let err = policy::load( + &root, + &[preset_row("typo", "trunk-basd")], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("a name this binary does not ship"); let text = format!("{err}"); assert!( text.contains("trunk-basd"), @@ -131,7 +149,8 @@ fn an_unknown_preset_name_is_refused_at_load() { #[test] fn enabling_no_preset_yields_no_preset_predicates() { let root = scratch("opt-in"); - let bundles = policy::load(&root, &[], None).expect("no rows, no bundles"); + let bundles = policy::load(&root, &[], &[], policy::ModuleChecks::Run, None) + .expect("no rows, no bundles"); assert!( bundles.is_empty(), "a consumer who enables nothing gets nothing, which is what keeps this \ @@ -163,8 +182,14 @@ fn a_preset_id_colliding_with_an_in_repo_id_is_refused_at_load() { })) .expect("an in-repo row"); - let err = policy::load(&root, &[preset_row("trunk", "trunk-based"), mine], None) - .expect_err("one id, two publishers across the boundary"); + let err = policy::load( + &root, + &[preset_row("trunk", "trunk-based"), mine], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect_err("one id, two publishers across the boundary"); let text = format!("{err}"); assert!(text.contains("no-force-push"), "names the id: {text}"); assert!( @@ -184,8 +209,14 @@ fn every_advertised_preset_name_actually_loads() { let names = policy::preset_names(); assert!(!names.is_empty(), "the binary ships at least one preset"); for name in names { - policy::load(&root, &[preset_row("row", name)], None) - .unwrap_or_else(|err| panic!("the advertised preset `{name}` does not load: {err}")); + policy::load( + &root, + &[preset_row("row", name)], + &[], + policy::ModuleChecks::Run, + None, + ) + .unwrap_or_else(|err| panic!("the advertised preset `{name}` does not load: {err}")); } } @@ -269,7 +300,14 @@ fn presets_are_inside_the_rule_one_glob() { fn every_shipped_preset_publishes_its_ids() { let root = scratch("published"); for name in policy::preset_names() { - let bundles = policy::load(&root, &[preset_row("row", name)], None).expect("loads"); + let bundles = policy::load( + &root, + &[preset_row("row", name)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("loads"); assert!( !bundles[0].declared().is_empty(), "the preset `{name}` publishes no rule id, so nothing it denies could \ @@ -295,7 +333,14 @@ fn every_shipped_preset_publishes_its_ids() { fn every_shipped_preset_passes_its_own_suite() { let root = scratch("suites"); for name in policy::preset_names() { - let bundles = policy::load(&root, &[preset_row("row", name)], None).expect("loads"); + let bundles = policy::load( + &root, + &[preset_row("row", name)], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("loads"); let Look::Is(suite) = policy::test(&bundles[0], "{}").expect("the suite runs") else { panic!("the preset `{name}` has a suite that could not run at all"); }; diff --git a/crates/batten/tests/policy_test_suite.rs b/crates/batten/tests/policy_test_suite.rs index 926bef6e7..e39d51d08 100644 --- a/crates/batten/tests/policy_test_suite.rs +++ b/crates/batten/tests/policy_test_suite.rs @@ -61,8 +61,12 @@ fn row(id: &str, module: &str) -> Rule { /// Compile `source` as a single-module bundle and run its suite. fn suite_of(source: &str) -> Suite { - let bundle = policy::compile("fixture", &[("fixture.rego".to_owned(), source.to_owned())]) - .expect("the fixture compiles"); + let bundle = policy::compile( + "fixture", + &[("fixture.rego".to_owned(), source.to_owned())], + &serde_json::json!({}), + ) + .expect("the fixture compiles"); match policy::test(&bundle, "{}").expect("the suite runs") { Look::Is(suite) => suite, Look::IsNot | Look::CouldNotLook => panic!("the suite did not run"), @@ -471,8 +475,14 @@ fn a_registered_module_with_tests_still_loads_and_denies() { // only surface at a mediated call. let dir = Fixture::new("policy-test-still-denies").build(); fs::write(dir.join("probe.rego"), CORRECT).expect("write module"); - let bundles = policy::load(Path::new(&dir), &[row("probe", "probe.rego")], None) - .expect("the bundle loads"); + let bundles = policy::load( + Path::new(&dir), + &[row("probe", "probe.rego")], + &[], + policy::ModuleChecks::Run, + None, + ) + .expect("the bundle loads"); let bundle = bundles.first().expect("one bundle"); let Look::Is(violations) = policy::deny(bundle, r#"{"call": {"command": "git push --force"}}"#) else { diff --git a/crates/batten/tests/policy_tree.rs b/crates/batten/tests/policy_tree.rs index 6333e238b..801dc99ac 100644 --- a/crates/batten/tests/policy_tree.rs +++ b/crates/batten/tests/policy_tree.rs @@ -67,7 +67,7 @@ fn write_bundle(root: &Path, source: &str) { } fn scan(root: &Path, rules: &[Rule]) -> rules::Scan { - rules::run_static(rules, &[], root).expect("the read surface runs a policy row") + rules::run_static(rules, &[], &[], root).expect("the read surface runs a policy row") } /// (a) A tree-scoped bundle denies on a fixture that violates. @@ -186,7 +186,7 @@ fn check_still_refuses_a_spawning_kind() { })) .expect("a command row"); - let err = rules::run_static(&[command], &[], &root) + let err = rules::run_static(&[command], &[], &[], &root) .expect_err("a read-effect verb will not run a configured command"); let text = format!("{err}"); assert!( @@ -242,6 +242,7 @@ fn an_empty_bundle_root_is_refused_at_load() { let err = rules::run_static( &[tree_row("repo-policy", "policy/", &["config.toml"])], &[], + &[], &root, ) .expect_err("a folder with no modules enables nothing"); @@ -381,8 +382,14 @@ violation contains {"rule": "reads-a-ghost", "msg": "x"} if { "#, ); - let err = batten::policy::load(&root, &[tree_row("repo-policy", "policy/", &[])], None) - .expect_err("a module reading a key the engine cannot emit is refused at load"); + let err = batten::policy::load( + &root, + &[tree_row("repo-policy", "policy/", &[])], + &[], + batten::policy::ModuleChecks::Run, + None, + ) + .expect_err("a module reading a key the engine cannot emit is refused at load"); let message = format!("{err}"); assert!( message.contains("nonesuch"), @@ -428,8 +435,14 @@ violation contains {"rule": "reads-real-keys", "msg": "z"} if { "#, ); - batten::policy::load(&root, &[tree_row("repo-policy", "policy/", &[])], None) - .expect("every key this module reads is one the engine emits"); + batten::policy::load( + &root, + &[tree_row("repo-policy", "policy/", &[])], + &[], + batten::policy::ModuleChecks::Run, + None, + ) + .expect("every key this module reads is one the engine emits"); } /// The third of `missing`'s three causes, split out as a CONFIG FAULT @@ -457,6 +470,7 @@ fn a_document_with_no_parser_is_refused_rather_than_skipped() { let err = rules::run_static( &[tree_row("repo-policy", "policy/", &["CLAUDE.md"])], &[], + &[], &root, ) .expect_err("a declared document this build cannot parse is a config fault"); @@ -513,6 +527,7 @@ fn an_absent_unsupported_document_is_still_a_parser_fault() { let err = rules::run_static( &[tree_row("repo-policy", "policy/", &["CLAUDE.md"])], &[], + &[], &root, ) .expect_err("the extension is decided before the tree is consulted"); @@ -555,8 +570,14 @@ violation contains {"rule": "reads-a-ghost", "msg": "x"} if { "#, ); - let err = batten::policy::load(&root, &[tree_row("repo-policy", "policy/", &[])], None) - .expect_err("a bracket reference is a reference"); + let err = batten::policy::load( + &root, + &[tree_row("repo-policy", "policy/", &[])], + &[], + batten::policy::ModuleChecks::Run, + None, + ) + .expect_err("a bracket reference is a reference"); assert!( format!("{err}").contains("nonesuch"), "the refusal names the bracketed key: {err}" @@ -585,8 +606,14 @@ violation contains {"rule": "reads-real-keys", "msg": "x"} if { "#, ); - batten::policy::load(&root, &[tree_row("repo-policy", "policy/", &[])], None) - .expect("`documents` is emitted, however it is spelled"); + batten::policy::load( + &root, + &[tree_row("repo-policy", "policy/", &[])], + &[], + batten::policy::ModuleChecks::Run, + None, + ) + .expect("`documents` is emitted, however it is spelled"); } // --- the lines fact (CLOUD-846) --------------------------------------------- diff --git a/crates/batten/tests/policy_whole_set.rs b/crates/batten/tests/policy_whole_set.rs index 93cd45cd1..fe597acaf 100644 --- a/crates/batten/tests/policy_whole_set.rs +++ b/crates/batten/tests/policy_whole_set.rs @@ -43,6 +43,7 @@ fn a_healthy_set_sweeps_clean_and_reaches_every_module() { "package batten.b\nimport rego.v1\nrules contains \"b\"\nviolation contains {\"rule\": \"b\", \"msg\": \"m\"} if { input.call.operation == \"read\" }\n", ), ], + &serde_json::json!({}), ) .expect("a healthy set compiles"); @@ -86,6 +87,7 @@ fn a_module_the_sweep_never_entered_is_reported() { "package elsewhere\nimport rego.v1\nnever_reached contains \"x\" if { true }\n", ), ], + &serde_json::json!({}), ) .expect("both compile — being unreachable is not a compile error"); @@ -121,6 +123,7 @@ fn two_contradicting_complete_rules_are_refused_by_the_sweep() { module("one", "package batten.c\nimport rego.v1\nverdict := 1\n"), module("two", "package batten.c\nimport rego.v1\nverdict := 2\n"), ], + &serde_json::json!({}), ); // The conflict may be caught by `compile`'s own smoke query — which is the // correct earliest place — or by the driven sweep. Either is a refusal at @@ -160,6 +163,7 @@ fn a_cyclic_set_is_refused_by_the_sweep() { "package batten.cyc\nimport rego.v1\nright contains x if { left[x] }\n", ), ], + &serde_json::json!({}), ); match bundle { Err(refused) => { @@ -193,6 +197,7 @@ fn the_analysis_carries_no_byte_of_any_policy_body() { "package batten.p\nimport rego.v1\nrules contains \"p\"\nviolation contains {{\"rule\": \"p\", \"msg\": \"{DISTINCTIVE}\"}} if {{ input.call.operation == \"write\" }}\n" ), )], + &serde_json::json!({}), ) .expect("compiles"); diff --git a/crates/batten/tests/primitives.rs b/crates/batten/tests/primitives.rs index c0e497228..6966f7e29 100644 --- a/crates/batten/tests/primitives.rs +++ b/crates/batten/tests/primitives.rs @@ -868,16 +868,27 @@ fn the_acceptance_runner_is_the_landed_rule_engine() { let repo = repo("acceptance-runner"); repo.write("a.txt", "content\n"); - let err = batten::rules::run_static(&config.rules, &config.provisions, &repo.dir).unwrap_err(); + let err = batten::rules::run_static( + &config.rules, + &config.provisions, + &config.patterns, + &repo.dir, + ) + .unwrap_err(); assert!( err.downcast_ref::().is_some(), "the read-effect surface refuses a spawning rule loudly, never skips it" ); assert!( - batten::rules::run_all(&config.rules, &config.provisions, &repo.dir) - .expect("the spawning surface runs it") - .findings - .is_empty(), + batten::rules::run_all( + &config.rules, + &config.provisions, + &config.patterns, + &repo.dir + ) + .expect("the spawning surface runs it") + .findings + .is_empty(), "an acceptance item that exits 0 produces no finding" ); } diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index b6c92139c..0927c95f9 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -108,7 +108,7 @@ checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" [[package]] name = "batten" -version = "0.0.103" +version = "0.0.104" dependencies = [ "anyhow", "clap", diff --git a/hk.pkl b/hk.pkl index 7c39f6a78..c65399718 100644 --- a/hk.pkl +++ b/hk.pkl @@ -411,6 +411,7 @@ local gate = new Mapping { "crates/batten/src/judge.rs", "crates/batten/src/markers.rs", "crates/batten/src/outputs.rs", + "crates/batten/src/pattern.rs", "crates/batten/src/provision.rs", "crates/batten/src/redirect.rs", "crates/batten/src/rules.rs", diff --git a/policy/run-shape.rego b/policy/run-shape.rego index 68fcfcba6..a7906cb38 100644 --- a/policy/run-shape.rego +++ b/policy/run-shape.rego @@ -28,7 +28,7 @@ rules contains "commit-names-no-message-source" # the SCRUBBING and the SPLITTING rather than the flag table, which is where a # raw-string module goes quietly wrong. `@` delimits each sed script because the # rows themselves are `|`-separated. -#MUTANT message-flag-unchecked|s@some c in {"m", "F", "C", "c"}@some c in {"ZZZZ"}@|every form that CAN obtain a message stays allowed +#MUTANT message-flag-cluster-unreferenced|s@short-message-flag-cluster@zzz-no-such-pattern@|every form that CAN obtain a message stays allowed #MUTANT list-not-split|s@^elements :=.*@elements := [scrubbed]@|a compound list is judged per element #MUTANT heredoc-body-judged|s@ j < i@ j < -1@|a git commit inside a heredoc body is prose #MUTANT double-quoted-span-judged|s@^scrubbed := quoted_out(single_scrubbed.*@scrubbed := single_scrubbed@|a quoted span carrying a list separator is not a list @@ -191,12 +191,16 @@ names_a_message_source(stage) if { # A short cluster — `-m`, `-am`, `-F`, `-C`, `-c`: one `-`, then letters, at # least one of which selects a message source. +# +# `regex.match` RATHER THAN `contains`, and the difference is a verdict rather +# than a spelling (CLOUD-885). The predicate is "a cluster of LETTERS, one of +# which is a message flag", and `contains` over the tail cannot say "letters": +# `-x=mfoo` carries an `m` and read as naming a message source, so a commit that +# will still block on $EDITOR was allowed through. The anchored class is the +# predicate the comment above already claimed. names_a_message_source(stage) if { some t in tokens(stage) - startswith(t, "-") - not startswith(t, "--") - some c in {"m", "F", "C", "c"} - contains(substring(t, 1, -1), c) + regex.match(data.batten.patterns["short-message-flag-cluster"], t) } # --------------------------------------------------------------------------- @@ -223,3 +227,17 @@ test_a_later_element_is_judged_too if { test_another_tool_is_not_judged if { count(violation) == 0 with input as {"call": {"command": "hg commit"}} } + +test_a_short_cluster_names_a_message_source if { + count(violation) == 0 with input as {"call": {"command": "git commit -am x"}} +} + +# THE DISCRIMINATING CASE for the `regex.match` above (CLOUD-885). `-x=mfoo` is +# not a flag cluster — it carries an `m`, which is all the previous `contains` +# over the tail could see, so a commit that still blocks on $EDITOR was allowed. +# A test that only covered `-m` and `-am` passes under both spellings and proves +# nothing about the change. +test_a_non_cluster_carrying_m_is_not_a_message_source if { + some v in violation with input as {"call": {"command": "git commit -x=mfoo"}} + v.rule == "commit-names-no-message-source" +} diff --git a/schema/batten.schema.json b/schema/batten.schema.json index e3dc226b2..4c1794af7 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -167,6 +167,13 @@ "null" ] }, + "pattern": { + "description": "The named-regex table (CLOUD-885): every expression a policy module may\napply, declared once and referenced by id.\n\nConsumer-specific by nature for exactly [`Config::verbs`]'s reason — a\ntracker-key expression is a consumer identifier, so it lives here and\nnever in the crate (non-negotiable rule 1). A module reaches it at\n`data.batten.patterns[\"\"]`; writing one inline is refused at load,\nwhich is what prices a one-off pattern against a field access over an\nalready-parsed document. The type and its validation are\n[`crate::pattern`].", + "type": "array", + "items": { + "$ref": "#/$defs/NamedPattern" + } + }, "protected": { "description": "Paths whose modification is guarded. A plain include set — no `!`\nentries — evaluated independently of [`Config::scope`] and\n[`Config::unlanded`]. CLOUD-31's config-trust diff defends this set.", "type": "array", @@ -856,6 +863,25 @@ "effect" ] }, + "NamedPattern": { + "description": "One declared regular expression.\n\nMirrors [`crate::verbs::MutatingVerb`]'s shape deliberately: a consumer-owned\ntable in the one committed authority, keyed by an id the config author picks.", + "type": "object", + "properties": { + "id": { + "description": "The name a module references it by — the key under\n`data.batten.patterns`.\n\nUnique across the table. A repeated id would make\n`data.batten.patterns[\"x\"]` ambiguous, and \"which declaration answered\nme\" is not a question a reviewer should have to resolve — the same\nreasoning [`crate::policy`] applies to two rows registering one module.", + "type": "string" + }, + "regex": { + "description": "The expression itself.\n\nCompiled at load by [`validate`], so a malformed pattern is a config\nfault at exit `1` rather than an evaluation failure on the mediated path\n— house style §8's placement, and the same one `forbid`'s `regex` column\nalready takes.", + "type": "string" + } + }, + "additionalProperties": false, + "required": [ + "id", + "regex" + ] + }, "OperandScope": { "description": "Which operands of a matched invocation are write targets.\n\n[`OperandScope::All`] is the default, and that is load-bearing rather than\nincidental: it is the reading that fails toward *refusing*, and it is the one\na move needs — guarding only the source would miss the direction that\ndestroys the destination, which\n`hook::tests::every_operand_is_a_candidate_so_a_destination_is_guarded_too`\npins. [`OperandScope::Last`] is the narrowing a destination-only program\nneeds, where copying a guarded file *out* of the guarded set is a read and\nrefusing it is the false positive that gets a guard switched off.", "oneOf": [ diff --git a/tests/run-shape.bats b/tests/run-shape.bats index 09e74cf8c..0ff83f99c 100644 --- a/tests/run-shape.bats +++ b/tests/run-shape.bats @@ -53,6 +53,14 @@ setup() { echo 'scope = "mediated_call"' echo 'module = "policy/run-shape.rego"' echo 'severity = "deny"' + # The declared pattern the module references (CLOUD-885). An inline regex + # is refused at load, so this row is not fixture decoration — without it + # the module's reference is undefined and every allow case flips to a + # denial, which is the silent-disarm the engine now refuses outright. + echo + echo "[[pattern]]" + echo 'id = "short-message-flag-cluster"' + echo 'regex = "^-[A-Za-z]*[mFCc]"' } >"$REPO/batten.toml" # No global or system config: a contributor's own git settings must not be # able to change a verdict here (CLOUD-282). @@ -68,8 +76,21 @@ hook() { # hook (cd "$REPO" && printf '%s' "$envelope" | "$BIN" hook --harness claude-code) } -denied() { [[ "$1" == *'"permissionDecision":"deny"'* ]]; } -allowed() { [[ "$1" != *'"deny"'* ]]; } +# BOTH HELPERS ASSERT THE EXIT STATUS, and for `allowed` that is the whole +# assertion rather than a belt-and-braces addition. `batten hook` prints NOTHING +# on an allow — the JSON is emitted only to deny, and the exit status is 0 either +# way, because the contract is that the harness reads the decision and not the +# code. So `[[ "$1" != *deny* ]]` over an EMPTY string is true, and every allow +# case in this file went green on any output at all, including the output of a +# binary that died before it judged anything. Measured on this branch: allow and +# deny both exit 0, so a non-zero status is exactly and only the crash. +# +# That is CLOUD-251's vacuous pass wearing a test's clothes, and it was suite- +# wide rather than one case: nothing here referenced `$status` at all. Fixed in +# the helper so all fourteen cases gain it at once. `$status` is the global bats +# `run` sets, so this reads the status of the call the case just made. +denied() { [ "$status" -eq 0 ] && [[ "$1" == *'"permissionDecision":"deny"'* ]]; } +allowed() { [ "$status" -eq 0 ] && [[ "$1" != *'"deny"'* ]]; } # --- the predicate ---------------------------------------------------------- @@ -99,6 +120,26 @@ allowed() { [[ "$1" != *'"deny"'* ]]; } done } +@test "THE MEASURED SHAPE: a token carrying an m is not a flag cluster" { + # CLOUD-885. The rule reads "one `-`, then LETTERS, at least one of which + # selects a message source". Before `regex.match` it was spelled as + # `contains` over the flag's tail, which cannot say "letters" — so `-x=mfoo` + # carried an `m`, read as naming a message source, and a commit that still + # blocks on $EDITOR went through. + # + # This is the discriminating case rather than another `-m`: the suite as it + # stood covered `-m`, `-am`, `-F`, `-C` and the long forms, and every one of + # them passes under BOTH spellings. Reproduced against the old spelling in a + # fixture before this landed, which is the evidence the case exists for. + run hook 'git commit -x=mfoo' + denied "$output" + + # The other direction, so the anchor is proven and not just the class: a + # message flag must be reached from the START of the cluster. `-vm` is one. + run hook 'git commit -vm "a message"' + allowed "$output" +} + # --- the list, which is where a raw-string module goes silent --------------- @test "a compound list is judged per element, not by its first word" {