diff --git a/.serena/memories/core.md b/.serena/memories/core.md index 22f655b54..6e0089ab2 100644 --- a/.serena/memories/core.md +++ b/.serena/memories/core.md @@ -623,6 +623,31 @@ repo config > default`, declared as data in `SETTINGS` (per-key env var/flag), crate↔config contract, hence the constant. Stated limits: no `cwd`, so an absolute or `..` path is compared as written, and expansion/substitution hide operands. Both under-deny, the sanctioned direction. +- `redirect.rs` — 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. `[[redirect]]` is `{glob, mutation}`, and `hook::protected_refusal` + consults it BEFORE the verb's own `redirect` — three tiers, table then verb + then `Fix::None`, where the last two are CLOUD-96's behaviour untouched, so the + floor is structural rather than careful (`Fix::declared(Option<&str>)` was + built for this seam). Matching is `rules::glob_match` — one glob semantics for + the engine — over the SAME normalised path `protected.contains` was asked + about, or the two tables would disagree about which path is under discussion. + **Declaration order decides, first match wins**, the tie-break `shape_rules` + already uses and for its stated reason: a reviewer reads a table top to bottom, + and any cleverer precedence is a rule about rules the config does not state. + A **sibling** table rather than a wider `protected`, which keeps `Vec` + so `trust::removed_entries`'s `protected[]` weakening keys are + byte-identical (asserted). Not policy-bearing — it changes what a refusal says, + never whether it fires — so no raise-only clamp applies; the local layer may + add a class and may not redefine a committed one, and since local rows append + after committed ones, first-match-wins means an uncommitted file can never + change what a committed row says. **The boundary worth knowing**: consumer #1 + declares `.github/workflows/**` and `batten.toml`, and deliberately NOT + `.serena/memories/**` — that class's remedy depends on the ACTION + (`write_memory` / `edit_memory` / `rename_memory` / `delete_memory`), so a path + row would override four correct per-verb answers with one weaker sentence. + Per-path beats per-verb only where the path fact dominates. It makes a message + specific; it does not make the named surface reachable (CLOUD-663). - `refusal.rs` — the refusal contract (CLOUD-122): ONE `Refusal` value — `{rule, reason, fix}` — constructed at every deny site and projected onto whatever channel a host reads, so the shape is never re-typed per harness. diff --git a/batten.toml b/batten.toml index c3892f156..f39384130 100644 --- a/batten.toml +++ b/batten.toml @@ -94,6 +94,36 @@ protected = [ ".github/workflows/**", ] +# --------------------------------------------------------------------------- +# What to run instead, per protected path class (CLOUD-280). +# +# The refusal contract (CLOUD-122) is that every deny names the fix. CLOUD-96 +# put that text on the `[[verb]]` row, which is right when the VERB knows the +# remedy and wrong when the PATH does: `rm` says "restore it with `git checkout +# --`", which is true of most files and actively misleading for a workflow, +# where the point is not recovering the bytes but that CI's definition of green +# changes under review. +# +# Consulted BEFORE the verb's own `redirect`; a class named here answers for +# every verb that reaches it. Declaration order decides, first match wins. +# +# WHY `.serena/memories/**` IS DELIBERATELY ABSENT, which is the interesting +# boundary of this table rather than an omission. Its remedy is not a property +# of the path alone: a write wants `write_memory`, an in-place edit wants +# `edit_memory`, a move wants `rename_memory` (the only route that rewrites +# `mem:` referrers), a delete wants `delete_memory`. The verb rows below already +# name the right tool per action, and a row here would OVERRIDE all four with +# one weaker sentence — the opposite of what this table is for. Per-path beats +# per-verb only where the path fact dominates; where the verb fact does, the +# fallback tier is the correct answer and not a leftover. +[[redirect]] +glob = ".github/workflows/**" +mutation = "change it in a pull request — these files are CI's definition of green, so the change has to be reviewed rather than restored" + +[[redirect]] +glob = "batten.toml" +mutation = "change it in a pull request — this file is the policy authority every gate reads, and `mise run config-lint` is what checks the edit before it lands" + # --------------------------------------------------------------------------- # The worktree pileup threshold (CLOUD-46). # diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index 5f2b7c7a4..8dde23974 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 per-path-class redirect table (CLOUD-280): what to run instead, + /// keyed by what is protected rather than by the verb reaching for it. + /// + /// Consulted before [`MutatingVerb::redirect`], which stays the fallback, so + /// the behaviour CLOUD-96 shipped is the floor rather than a regression. + /// Deliberately a sibling of [`Config::protected`] rather than a widening of + /// it: that set keeps its element type, so [`crate::trust`]'s + /// `protected[]` weakening keys are untouched. + /// + /// [`MutatingVerb::redirect`]: crate::verbs::MutatingVerb::redirect + #[serde(default, rename = "redirect", skip_serializing_if = "Vec::is_empty")] + pub redirects: Vec, /// Output predicates over a wrapped command's captured streams (CLOUD-117): /// literals that, found in `batten exec`'s output, promote a lying exit `0` /// to a violation. Consumer-specific by nature — which warning means @@ -420,6 +432,14 @@ pub struct OverrideConfig { skip_serializing_if = "Vec::is_empty" )] pub exec_patterns: Vec, + /// Redirects this file **adds**. A duplicate glob is refused. + /// + /// Needs no raise-only clamp, and that is a decision rather than an + /// oversight: a redirect changes what a refusal *says*, never whether it + /// fires, so there is no bar here to lower. Refusing a redefinition is + /// coherence with the other append-only tables. + #[serde(default, rename = "redirect", skip_serializing_if = "Vec::is_empty")] + pub redirects: Vec, /// Waivers this file adds, for rules the authority does not declare. A /// waiver over a committed rule lowers that bar and is refused. #[serde(default, rename = "waiver", skip_serializing_if = "Vec::is_empty")] @@ -497,6 +517,7 @@ 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)?; + 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 // them up and nobody checked the sibling, so an empty `token` — which @@ -650,6 +671,7 @@ impl Config { unlanded: Vec::new(), epoch: None, verbs: Vec::new(), + redirects: Vec::new(), markers: Vec::new(), exec: None, exec_patterns: Vec::new(), @@ -891,6 +913,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("), + ("redirects", "crate::redirect::validate("), ("markers", "crate::markers::validate("), ("rules", "crate::rules::validate("), ("exec_patterns", "crate::outputs::validate("), diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index b9ed11bb9..0bf036277 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -45,6 +45,7 @@ use serde::Serialize; use serde_json::Value; use crate::receipt::Validity; +use crate::redirect::{self, Redirect}; use crate::refusal::{Fix, Refusal}; use crate::resolve::Resolved; use crate::rules::{PathSet, ReceiptKey, ReceiptTrigger, Rule, RuleKind, RuleScope}; @@ -1492,6 +1493,12 @@ pub struct Policy { /// the cross product as rules would need one row per verb × path pair, and /// the config would restate what an intersection already says. protected: PathSet, + /// What to run instead, per protected path class (CLOUD-280). + /// + /// Message composition only — it never decides whether the gate fires, which + /// is why it sits beside `protected` rather than inside it and why no + /// raise-only clamp applies to it. + redirects: Vec, } impl Policy { @@ -1507,6 +1514,7 @@ impl Policy { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), } } @@ -1534,6 +1542,7 @@ impl Policy { fail_on_warning: resolved.fail_on_warning, verbs: resolved.verbs.clone(), protected: PathSet::includes("protected", &resolved.protected)?, + redirects: resolved.redirects.clone(), }) } @@ -1741,12 +1750,15 @@ fn adjudicated( && let Some(verb) = crate::verbs::classify(&policy.verbs, &envelope.tool) && policy.protected.contains(normalise(path)) { - return Decision::Deny(protected_refusal(&Target { - program: &envelope.tool, - subcommand: None, - path, - verb, - })); + return Decision::Deny(protected_refusal( + &policy.redirects, + &Target { + program: &envelope.tool, + subcommand: None, + path, + verb, + }, + )); } // The write-triggered receipt gate (CLOUD-444), reached whether or not this // call also carries a command — a write tool carries none, and the early @@ -2309,7 +2321,7 @@ fn protected_mutation(policy: &Policy, command: &str) -> Decision { if !policy.protected.contains(normalise(target.path)) { continue; } - return Decision::Deny(protected_refusal(&target)); + return Decision::Deny(protected_refusal(&policy.redirects, &target)); } } Decision::Allow @@ -2388,18 +2400,22 @@ fn normalise(path: &str) -> &str { /// typed, and naming it is the difference between an actionable refusal and a /// riddle. The file's contents never appear. /// -/// The fix is the verb's own declared `redirect`, and [`Fix::None`] where the -/// consumer declared none — stated rather than papered over with a catch-all -/// that pretends to be specific. That absence is the seam CLOUD-280 fills: the -/// useful redirect is a property of what is being protected, not of the verb -/// reaching for it, so a per-path-class table lands *here* and this fallback -/// becomes the third tier rather than the second. +/// The fix is three-tiered (CLOUD-280): the `[[redirect]]` row for this path +/// class, else the verb's own declared `redirect`, else [`Fix::None`] — stated +/// rather than papered over with a catch-all that pretends to be specific. The +/// useful remedy is a property of what is being protected, not of the verb +/// reaching for it, which is why the table is consulted first; the two fallbacks +/// are CLOUD-96's behaviour unchanged, so the floor cannot regress. +/// +/// It makes a refusal SPECIFIC; it does not make the named surface reachable. +/// CLOUD-663 was canceled on exactly that distinction — a redirect pointing at a +/// surface that is down is a defect in the surface. /// /// A subcommand-qualified row names the whole action, not just the front-end: /// ` ` is what the caller typed and what a reader has to /// recognise, and a refusal naming only the front-end would read as a ban on /// every use of it (CLOUD-442). -fn protected_refusal(target: &Target<'_>) -> Refusal { +fn protected_refusal(redirects: &[Redirect], target: &Target<'_>) -> Refusal { let action = match target.subcommand { Some(subcommand) => format!("{} {subcommand}", target.program), None => target.program.to_owned(), @@ -2407,7 +2423,21 @@ fn protected_refusal(target: &Target<'_>) -> Refusal { Refusal::new( PROTECTED_MUTATION, format!("`{action}` targets the protected path {}", target.path), - Fix::declared(target.verb.redirect.as_deref()), + // Three tiers, narrowest first (CLOUD-280): the path class the consumer + // declared, then the verb's own general remedy, then `Fix::None` — which + // renders an explicit "none declared" and names the gate. The two + // fallbacks are exactly CLOUD-96's behaviour, so this can only ever make + // a refusal more specific, never less. + // + // The lookup takes `normalise`d path, the same value + // `policy.protected.contains` was asked about, so the two tables cannot + // disagree about WHICH path is under discussion. The message keeps the + // path as the caller typed it, because that is the pointer they can act + // on. + Fix::declared( + redirect::resolve(redirects, normalise(target.path)) + .or(target.verb.redirect.as_deref()), + ), ) } @@ -2991,6 +3021,11 @@ mod tests { /// one protected glob. Both tables are the consumer's, so a test supplies /// them exactly as a `batten.toml` would. fn protected_policy(verbs: Vec) -> Policy { + protected_policy_with(verbs, Vec::new()) + } + + /// The same fixture with a declared `[[redirect]]` table (CLOUD-280). + fn protected_policy_with(verbs: Vec, redirects: Vec) -> Policy { Policy { shapes: Vec::new(), fail_on_warning: false, @@ -3000,6 +3035,7 @@ mod tests { &[".serena/memories/**".to_owned(), "batten.toml".to_owned()], ) .expect("the fixture protected set is well formed"), + redirects, } } @@ -3022,6 +3058,7 @@ mod tests { Policy { verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), shapes: vec![ shape("gh-pr-merge", "gh pr merge", None), shape( @@ -3543,6 +3580,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), }; assert_eq!( adjudicate( @@ -3568,6 +3606,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), }; assert_eq!( adjudicate( @@ -3587,6 +3626,7 @@ mod tests { fail_on_warning: true, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), }; assert!( matches!( @@ -3617,6 +3657,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), }; let reason = denial_text(adjudicate( &policy, @@ -3653,6 +3694,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), }; let reason = denial_text(adjudicate( &policy, @@ -3707,6 +3749,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), } } @@ -3746,6 +3789,7 @@ mod tests { fail_on_warning: false, verbs: Vec::new(), protected: PathSet::empty(), + redirects: Vec::new(), } } @@ -4373,6 +4417,143 @@ mod tests { assert!(reason.contains("surface that owns it"), "got: {reason}"); } + /// Adjudicate against the protected fixture with a declared redirect table. + fn guarded_with(redirects: Vec, command: &str) -> Decision { + adjudicate( + &protected_policy_with( + vec![verb("rm", Some("restore it with git")), verb("mv", None)], + redirects, + ), + &envelope(command), + false, + &None, + &None, + &crate::stop::StopFacts::default(), + ) + } + + fn redirect_row(glob: &str, mutation: &str) -> Redirect { + Redirect { + glob: glob.to_owned(), + mutation: mutation.to_owned(), + } + } + + #[test] + fn the_path_class_redirect_outranks_the_verbs_own() { + // Tier one (CLOUD-280). `rm` declares "restore it with git", which is + // true of most paths and useless for this one: agent memory has a write + // surface, and that is the fact the PATH knows and the verb cannot. + let refusal = denial(guarded_with( + vec![redirect_row( + ".serena/memories/**", + "write it through the memory surface that owns it", + )], + "rm .serena/memories/core.md", + )); + assert_eq!( + refusal.fix().declared_alternative(), + Some("write it through the memory surface that owns it"), + "the class the config declared beats the verb's general remedy" + ); + } + + #[test] + fn a_class_no_row_claims_falls_back_to_the_verbs_redirect() { + // Tier two, and the floor this issue promised not to regress: with a + // table declared but silent about this path, the answer is exactly what + // CLOUD-96 shipped. + let refusal = denial(guarded_with( + vec![redirect_row( + ".serena/memories/**", + "use the memory surface", + )], + "rm batten.toml", + )); + assert_eq!( + refusal.fix().declared_alternative(), + Some("restore it with git"), + "an unclaimed class leaves the verb's own redirect standing" + ); + } + + #[test] + fn neither_tier_declaring_anything_still_names_the_gate() { + // Tier three, also unchanged: `mv` declares no redirect and no row + // claims the path, so the absence is stated as a value. + let decision = guarded_with( + vec![redirect_row("somewhere/else/**", "irrelevant")], + "mv batten.toml elsewhere", + ); + let refusal = denial(decision.clone()); + assert_eq!(refusal.fix(), &Fix::None); + assert!(denial_text(decision).contains(PROTECTED_MUTATION)); + } + + #[test] + fn the_redirect_lookup_sees_the_same_path_the_protected_check_did() { + // Both tables must discuss ONE path. `./x` and `x` are the same file, and + // `protected.contains` is asked about the normalised form — so a lookup + // on the raw operand would guard the path and then fail to find the class + // that speaks for it, producing a deny whose fix silently fell back a + // tier for a spelling. + let refusal = denial(guarded_with( + vec![redirect_row("batten.toml", "change it in a reviewed PR")], + "rm ./batten.toml", + )); + assert_eq!( + refusal.fix().declared_alternative(), + Some("change it in a reviewed PR") + ); + // And the message still points at what the caller actually typed. + assert!( + refusal.reason().contains("./batten.toml"), + "got: {}", + refusal.reason() + ); + } + + #[test] + fn the_declared_order_decides_which_class_answers() { + // The tie-break, at the surface that consumes it rather than only in the + // table's own unit tests: two rows match, and the one declared first + // wins, because the config author orders the table. + let refusal = denial(guarded_with( + vec![ + redirect_row(".serena/memories/**", "the narrow answer"), + redirect_row("**", "the catch-all"), + ], + "rm .serena/memories/core.md", + )); + assert_eq!( + refusal.fix().declared_alternative(), + Some("the narrow answer") + ); + } + + #[test] + fn a_redirect_changes_the_message_and_never_the_verdict() { + // The claim that exempts this table from the raise-only clamp, asserted + // rather than assumed: the same command against the same policy is a + // deny with the table and a deny without it, and an ALLOW stays an allow. + // If a redirect could ever flip a verdict it would be policy-bearing and + // would need a clamp (the issue's stated assumption 1). + let rows = vec![redirect_row("**", "some remedy")]; + assert!(matches!( + guarded_with(rows.clone(), "rm batten.toml"), + Decision::Deny(_) + )); + assert!(matches!( + guarded_with(Vec::new(), "rm batten.toml"), + Decision::Deny(_) + )); + assert_eq!( + guarded_with(rows, "rm target/debug/scratch"), + Decision::Allow, + "a redirect matching every path cannot make an unprotected one deny" + ); + } + #[test] fn flags_are_never_treated_as_paths() { // And `--` ends option parsing, so a dash-leading operand after it is @@ -4425,6 +4606,7 @@ mod tests { fail_on_warning: false, verbs: vec![verb("rm", None)], protected: PathSet::empty(), + redirects: Vec::new(), }; assert_eq!( adjudicate( @@ -4503,6 +4685,7 @@ mod tests { verbs: verbs.clone(), protected: PathSet::includes("protected", &["guarded/**".to_owned()]) .expect("well formed"), + redirects: Vec::new(), }; let elsewhere = Policy { shapes: Vec::new(), @@ -4510,6 +4693,7 @@ mod tests { verbs, protected: PathSet::includes("protected", &["other/**".to_owned()]) .expect("well formed"), + redirects: Vec::new(), }; let call = envelope("rm guarded/thing"); assert!( diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index dfcd17dcc..9f79ab87d 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -44,6 +44,7 @@ pub mod output; pub mod outputs; pub mod provision; pub mod receipt; +pub mod redirect; pub mod refusal; pub mod render; pub mod resolve; diff --git a/crates/batten/src/redirect.rs b/crates/batten/src/redirect.rs new file mode 100644 index 000000000..41dbcabed --- /dev/null +++ b/crates/batten/src/redirect.rs @@ -0,0 +1,214 @@ +//! 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. +//! +//! CLOUD-96 shipped the sanctioned mutation on the `[[verb]]` row, because a +//! per-class one was not expressible then. That gets the refusal contract +//! (CLOUD-122) mostly right — `rm` has one obvious alternative most of the time +//! — but the *useful* remedy is a property of the target: `rm` against agent +//! memory should name the memory-write surface, `rm` against a committed +//! workflow should say to delete it in a PR, `rm` against generated output +//! should say to re-run the generator. One string on the verb has to be vague +//! enough to cover all three, and a vague fix is the un-actionable refusal the +//! contract exists to prevent. +//! +//! The duplication that forced this was measured rather than predicted. Once +//! CLOUD-312 made the write *tools* verbs and CLOUD-442 added the qualifier +//! columns, this repository's own table reached seventeen `redirect` strings of +//! which **ten** carried the same per-path clause verbatim — one fact about +//! `.serena/memories/**`, copy-pasted across every program that can reach it, +//! because there was nowhere else to put it. +//! +//! Load-bearing choices: +//! +//! * **A sibling table, not a wider `protected`.** The direct approach — +//! widening `protected` to `{glob, mutation}` — breaks +//! [`crate::trust::weakenings`], whose `protected[]` keys are how the +//! raise-only comparison names a removed guard. Those keys stay byte-identical +//! because `protected` keeps its element type; this table only says what to +//! *suggest*, so the two can never disagree about coverage. +//! * **Declaration order decides, first match wins.** The same tie-break +//! [`crate::hook`]'s shape rows use, and for the reason stated there: a +//! reviewer reads a table top to bottom, and any cleverer precedence is a rule +//! about rules that the config does not state. "Most specific glob" has no +//! cheap definition and would be one. +//! * **A redirect is not policy-bearing.** It changes what a refusal *says*, +//! never whether the refusal fires, so §8's raise-only clamp does not apply to +//! it — stated here rather than left to inference. What the local layer still +//! may not do is *redefine* a committed row, which is coherence with every +//! other table rather than a weakening this one could express. +//! * **It makes a message specific; it does not make a surface reachable.** +//! CLOUD-663 was canceled on exactly this distinction: a redirect naming a +//! surface that is down is a defect in the surface, not in the redirect, and +//! a second sanctioned writer would have hidden it permanently. +//! +//! The glob matcher is [`crate::rules::glob_match`] — one glob semantics for the +//! whole engine, never a second implementation. + +use anyhow::Result; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::error::UsageError; +use crate::rules::glob_match; + +/// One declared path class and the mutation sanctioned for it. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct Redirect { + /// The path class this redirect speaks for, in the engine's one glob + /// dialect. + /// + /// Deliberately independent of `protected`: this table answers "what should + /// they run instead", never "is this guarded". A glob here matching a path + /// no `protected` entry covers simply never comes up, which is the harmless + /// direction — the alternative, deriving protection from a redirect, is the + /// set-collapsing CLOUD-37 exists to prevent. + pub glob: String, + /// The sanctioned mutation for that class — the "run this instead" a deny + /// carries. + pub mutation: String, +} + +/// Reject a table that would make a refusal dishonest or silent. +/// +/// # Errors +/// +/// Returns a [`UsageError`] (→ exit `1`) for an empty `glob`, an empty +/// `mutation`, or a duplicated `glob`. The last is the load-bearing one: two +/// rows for one path class are two answers to one question, and silently taking +/// the first is how a corrected remedy gets lost behind a stale row — the same +/// refusal [`crate::verbs::validate`] gives a verb declared twice. +/// +/// An empty `mutation` is refused rather than treated as absent, because it +/// would render a fix clause that is present and says nothing, which reads worse +/// than the explicit "none declared" a row's absence already produces. +pub fn validate(table: &[Redirect]) -> Result<()> { + for (index, entry) in table.iter().enumerate() { + if entry.glob.trim().is_empty() { + return Err(UsageError::raise( + "redirect: `glob` must not be empty".to_owned(), + )); + } + if entry.mutation.trim().is_empty() { + return Err(UsageError::raise(format!( + "redirect {}: `mutation` must not be empty — a redirect that names nothing is a \ + fix clause that says nothing", + entry.glob + ))); + } + if table[..index].iter().any(|prior| prior.glob == entry.glob) { + return Err(UsageError::raise(format!( + "redirect {}: declared twice; a path class has one sanctioned mutation", + entry.glob + ))); + } + } + Ok(()) +} + +/// The mutation declared for `path`, if any row claims it. +/// +/// First match in declaration order — see the module doc for why that is the +/// tie-break and not "most specific". `None` means no row speaks for this path, +/// which leaves the caller to fall back to the verb's own redirect. +#[must_use] +pub fn resolve<'table>(table: &'table [Redirect], path: &str) -> Option<&'table str> { + table + .iter() + .find(|entry| glob_match(&entry.glob, path)) + .map(|entry| entry.mutation.as_str()) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] +mod tests { + use super::*; + + fn row(glob: &str, mutation: &str) -> Redirect { + Redirect { + glob: glob.to_owned(), + mutation: mutation.to_owned(), + } + } + + #[test] + fn a_declared_class_resolves_to_its_mutation() { + let table = [row("guarded/**", "use the surface that owns it")]; + assert_eq!( + resolve(&table, "guarded/thing.md"), + Some("use the surface that owns it") + ); + } + + #[test] + fn a_path_no_row_claims_resolves_to_nothing() { + // Not an error and not a default: "no row speaks for this path" is what + // lets the caller fall back to the verb's own redirect, which is the + // tier CLOUD-96 shipped and this table sits in front of. + let table = [row("guarded/**", "use the surface that owns it")]; + assert_eq!(resolve(&table, "elsewhere/thing.md"), None); + assert_eq!(resolve(&[], "guarded/thing.md"), None); + } + + #[test] + fn the_first_matching_row_wins_in_declaration_order() { + // The tie-break, as a test rather than a comment. Both globs match, and + // the narrower one is declared FIRST — which is the whole point: the + // config author orders the table, the engine does not rank it. + let table = [ + row("guarded/secrets/**", "rotate it, do not edit it"), + row("guarded/**", "use the surface that owns it"), + ]; + assert_eq!( + resolve(&table, "guarded/secrets/key.md"), + Some("rotate it, do not edit it") + ); + // And the later row still answers for everything the earlier one misses, + // so ordering narrow-first is a usable discipline rather than a trap. + assert_eq!( + resolve(&table, "guarded/notes.md"), + Some("use the surface that owns it") + ); + } + + #[test] + fn a_class_declared_twice_is_a_usage_error() { + // Two answers to one question. Taking the first silently is how a + // corrected remedy gets lost behind a stale row. + let table = [row("guarded/**", "first"), row("guarded/**", "second")]; + let err = validate(&table).unwrap_err(); + assert!(err.downcast_ref::().is_some()); + } + + #[test] + fn an_empty_glob_or_mutation_is_a_usage_error() { + assert!(validate(&[row("", "something")]).is_err()); + assert!(validate(&[row("guarded/**", "")]).is_err()); + // Whitespace is not a declaration either — it would render `Fix: .` + assert!(validate(&[row("guarded/**", " ")]).is_err()); + assert!(validate(&[row("guarded/**", "something")]).is_ok()); + } + + #[test] + fn two_distinct_classes_coexist() { + let table = [row("a/**", "first"), row("b/**", "second")]; + assert!(validate(&table).is_ok()); + assert_eq!(resolve(&table, "b/x"), Some("second")); + } + + #[test] + fn the_source_bakes_in_no_path_class() { + // Non-negotiable rule 1, in the `verbs::the_source_bakes_in_no_verb` + // idiom: which paths a repository protects, and what it wants run + // instead, are the consumer's policy. Asserted behaviourally — the same + // path must get opposite answers from two tables differing only in their + // rows, which a hardcoded class could not produce. + let declaring = [row("guarded/**", "the declared remedy")]; + let elsewhere = [row("other/**", "the declared remedy")]; + assert_eq!( + resolve(&declaring, "guarded/thing"), + Some("the declared remedy") + ); + assert_eq!(resolve(&elsewhere, "guarded/thing"), None); + } +} diff --git a/crates/batten/src/resolve.rs b/crates/batten/src/resolve.rs index 3bae8585b..eb9b65916 100644 --- a/crates/batten/src/resolve.rs +++ b/crates/batten/src/resolve.rs @@ -220,6 +220,12 @@ pub struct Resolved { /// The mutating-verb table, consumer data the authority supplies. #[serde(rename = "verb")] pub verbs: 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 + /// authority never named and can never change what a committed row says. + #[serde(rename = "redirect")] + pub redirects: Vec, /// The suppression-marker table, consumer data the authority supplies. #[serde(rename = "marker")] pub markers: Vec, @@ -579,6 +585,7 @@ pub fn resolve_with_env( rules_source: declared_by(present, !repo.rules.is_empty()), rules: repo.rules.clone(), exec_patterns: repo.exec_patterns.clone(), + redirects: repo.redirects.clone(), waivers: repo.waivers.clone(), }; @@ -675,6 +682,7 @@ struct Tables { rules: Vec, rules_source: Source, exec_patterns: Vec, + redirects: Vec, waivers: Vec, } @@ -742,6 +750,7 @@ fn apply_local( tables.rules_source = Source::LocalFile; } merge_local_patterns(&mut tables.exec_patterns, local.exec_patterns)?; + merge_local_redirects(&mut tables.redirects, local.redirects)?; merge_local_waivers(&mut tables.waivers, local.waivers, &repo.rules)?; // §8's three policy-bearing path sets, raise-only. Before CLOUD-239 these // were parsed and discarded: an author who wrote `protected` here got no @@ -865,6 +874,36 @@ fn merge_local_patterns( Ok(()) } +/// Add a local file's redirects to the committed ones, refusing a redefinition. +/// +/// The same append-only shape [`merge_local_patterns`] uses, keyed on `glob`, +/// and for coherence rather than for safety: a redirect is **not +/// policy-bearing** — it changes what a refusal says, never whether it fires — +/// so §8's raise-only clamp has no bar here to protect. What refusing a +/// redefinition buys is that a committed remedy cannot be quietly reworded by an +/// uncommitted file, which is a claim about provenance, not about strictness. +/// +/// Appending is what makes that hold: [`crate::redirect::resolve`] takes the +/// **first** matching row, so a local row can only ever answer for a class the +/// authority left unclaimed. +fn merge_local_redirects( + committed: &mut Vec, + local: Vec, +) -> Result<()> { + for entry in local { + if committed.iter().any(|row| row.glob == entry.glob) { + return Err(UsageError::raise(format!( + "redirect {}: {LOCAL_CONFIG_FILE} may not redefine a redirect from {}; an \ + override may only add path classes the authority does not claim (§8)", + entry.glob, + config::CONFIG_FILE, + ))); + } + committed.push(entry); + } + Ok(()) +} + /// Add a local file's waivers to the committed ones, refusing any that touch a /// committed rule. /// @@ -940,6 +979,7 @@ fn assemble( unlanded: paths.unlanded, epoch: repo.epoch.clone(), verbs: repo.verbs.clone(), + redirects: tables.redirects, markers: repo.markers.clone(), exec: repo.exec, exec_patterns: tables.exec_patterns, @@ -1001,6 +1041,7 @@ fn attribution( authority_set(!repo.exec_patterns.is_empty()), ), ("exec", authority_set(repo.exec.is_some())), + ("redirect", authority_set(!repo.redirects.is_empty())), ("waiver", authority_set(!repo.waivers.is_empty())), ("budget", authority_set(repo.budget.is_some())), ("must_land_on", authority_set(repo.must_land_on.is_some())), @@ -1047,6 +1088,66 @@ mod tests { None } + /// A committed authority declaring one redirect class. + const REDIRECT_AUTHORITY: &str = "version = 1\nprotected = [\"guarded/**\"]\n\n[[redirect]]\nglob = \"guarded/**\"\nmutation = \"use the surface that owns it\"\n"; + + #[test] + fn a_local_file_may_add_a_redirect_class_the_authority_does_not_claim() { + // The permitted direction, and the reason it needs no clamp: a redirect + // changes what a refusal SAYS, never whether it fires, so an added class + // lowers no bar. A session gating a scratch tree can name its own remedy + // without touching committed policy. + let dir = repo( + "redirect-local-add", + REDIRECT_AUTHORITY, + Some( + "version = 1\n\n[[redirect]]\nglob = \"vendor/**\"\nmutation = \"re-run the generator\"\n", + ), + ); + let resolved = resolve_with_env(&dir, &Overrides::default(), &no_env).unwrap(); + assert_eq!(resolved.redirects.len(), 2); + // Appended AFTER the committed rows, which is what makes first-match-wins + // safe: a local row can only ever answer for a class the authority left + // unclaimed. + assert_eq!(resolved.redirects[0].glob, "guarded/**"); + assert_eq!(resolved.redirects[1].glob, "vendor/**"); + assert_eq!( + crate::redirect::resolve(&resolved.redirects, "guarded/thing"), + Some("use the surface that owns it"), + "the committed remedy still answers for its own class" + ); + } + + #[test] + fn a_local_file_may_not_redefine_a_committed_redirect() { + // Not a strictness clamp — there is no bar here to lower — but a + // provenance one: a committed remedy must not be quietly reworded by an + // uncommitted file, the same refusal every other append-only table gives. + let dir = repo( + "redirect-local-redefine", + REDIRECT_AUTHORITY, + Some( + "version = 1\n\n[[redirect]]\nglob = \"guarded/**\"\nmutation = \"do whatever\"\n", + ), + ); + let err = resolve_with_env(&dir, &Overrides::default(), &no_env).unwrap_err(); + assert!(is_usage_error(&err), "got: {err}"); + assert!( + err.to_string().contains("guarded/**"), + "the refusal names the class: {err}" + ); + } + + #[test] + fn an_authority_declaring_no_redirect_resolves_an_empty_table() { + // Absent is not empty-and-wrong: a repository that names no path class + // simply falls through to the verb's own redirect, which is CLOUD-96's + // behaviour and the floor this table sits on top of. + let dir = repo("redirect-absent", "version = 1\n", None); + let resolved = resolve_with_env(&dir, &Overrides::default(), &no_env).unwrap(); + assert!(resolved.redirects.is_empty()); + } + fn is_usage_error(err: &anyhow::Error) -> bool { err.downcast_ref::().is_some() } diff --git a/crates/batten/src/trust.rs b/crates/batten/src/trust.rs index 3b4a35e82..14414f449 100644 --- a/crates/batten/src/trust.rs +++ b/crates/batten/src/trust.rs @@ -394,6 +394,42 @@ mod tests { ); } + #[test] + fn the_protected_weakening_key_survives_the_redirect_table() { + // CLOUD-280's load-bearing non-change. The obvious way to give a + // protected path its own redirect is to widen `protected` to a table of + // `{glob, mutation}` — and that breaks THIS key, because `removed_entries` + // renders `format!("{key}[{entry}]")` over a list of strings. A consumer + // reading `protected[b]` out of a trust report, and every gate keyed on + // that spelling, would start seeing something else. + // + // So the redirect landed as a sibling table and `protected` kept its + // element type. Asserted byte-for-byte, and with a redirect declared, so + // the assertion is about the shape that shipped rather than about a + // config the feature does not exist in. + let base = parse( + "version = 1\nprotected = [\"a\", \"b\"]\n\n[[redirect]]\nglob = \"b\"\nmutation = \"use the surface that owns it\"\n", + ); + let working = parse( + "version = 1\nprotected = [\"a\"]\n\n[[redirect]]\nglob = \"b\"\nmutation = \"use the surface that owns it\"\n", + ); + let found = weakenings(&base, &working); + assert_eq!(found.len(), 1, "one removed path, one weakening: {found:?}"); + assert_eq!( + found[0].key, "protected[b]", + "the key format is the signature this design preserves" + ); + // And the redirect table itself contributes no weakening in either + // direction: it is not policy-bearing, so adding or removing a row + // cannot lower a bar. + let with_row = parse( + "version = 1\nprotected = [\"a\"]\n\n[[redirect]]\nglob = \"a\"\nmutation = \"x\"\n", + ); + let without = parse("version = 1\nprotected = [\"a\"]\n"); + assert!(weakenings(&with_row, &without).is_empty()); + assert!(weakenings(&without, &with_row).is_empty()); + } + #[test] fn adding_a_protected_path_is_not_a_weakening() { let base = parse("version = 1\nprotected = [\"a\"]\n"); diff --git a/crates/batten/tests/cli.rs b/crates/batten/tests/cli.rs index 3b2f931c6..0c1a28a70 100644 --- a/crates/batten/tests/cli.rs +++ b/crates/batten/tests/cli.rs @@ -1349,6 +1349,61 @@ fn a_deny_with_no_safe_remedy_declares_it_rather_than_omitting_the_clause() { ); } +/// A gate whose protected class carries its own redirect, and a verb whose +/// general remedy is the wrong answer for it (CLOUD-280). +const PER_CLASS_REDIRECT_CONFIG: &str = r#"version = 1 +protected = ["guarded/**", "vendor/**"] + +[[redirect]] +glob = "guarded/**" +mutation = "change it in a pull request" + +[[verb]] +verb = "rm" +effect = "destructive" +redirect = "restore it with git" +"#; + +#[test] +fn a_deny_names_the_path_classs_own_mutation_over_the_verbs() { + // The three tiers over the compiled binary, because a refusal is a contract + // only where a host reads it. Same command, same verb, two paths: the class + // that declares a remedy gets it, and the class that does not falls back to + // the verb's — which is CLOUD-96's behaviour, asserted here so the floor is + // proven rather than assumed. + let dir = repo_with_config("refusal-per-class", PER_CLASS_REDIRECT_CONFIG); + + let claimed = run_hook_in( + &dir, + "exit-code", + &claude_payload("rm guarded/thing.md"), + false, + ); + assert_eq!(claimed.status.code(), Some(2), "the protected gate denies"); + let stderr = String::from_utf8_lossy(&claimed.stderr); + assert!( + stderr.contains("Fix: change it in a pull request"), + "the declared class answers, got: {stderr}" + ); + assert!( + !stderr.contains("restore it with git"), + "the verb's general remedy must not also appear, got: {stderr}" + ); + + let unclaimed = run_hook_in( + &dir, + "exit-code", + &claude_payload("rm vendor/thing.md"), + false, + ); + assert_eq!(unclaimed.status.code(), Some(2), "still denied"); + let stderr = String::from_utf8_lossy(&unclaimed.stderr); + assert!( + stderr.contains("Fix: restore it with git"), + "an unclaimed class leaves the verb's redirect standing, got: {stderr}" + ); +} + // --- the normalized event census (CLOUD-43) --------------------------------- // // The envelope carried an `event` field from CLOUD-202 and never dispatched on diff --git a/crates/batten/tests/config_schema.rs b/crates/batten/tests/config_schema.rs index a3649938a..36e828d70 100644 --- a/crates/batten/tests/config_schema.rs +++ b/crates/batten/tests/config_schema.rs @@ -427,6 +427,7 @@ fn the_override_schema_describes_only_keys_the_loader_honours() { "fail_on_warning", "min_batten_version", "protected", + "redirect", "rule", "scope", "strictness", diff --git a/hk.pkl b/hk.pkl index ebd620237..a50eb76fe 100644 --- a/hk.pkl +++ b/hk.pkl @@ -336,6 +336,7 @@ local gate = new Mapping { "crates/batten/src/markers.rs", "crates/batten/src/outputs.rs", "crates/batten/src/provision.rs", + "crates/batten/src/redirect.rs", "crates/batten/src/rules.rs", "crates/batten/src/severity.rs", "crates/batten/src/transcript.rs", diff --git a/schema/batten.local.schema.json b/schema/batten.local.schema.json index 41232e0cf..826a1f40a 100644 --- a/schema/batten.local.schema.json +++ b/schema/batten.local.schema.json @@ -32,6 +32,13 @@ "type": "string" } }, + "redirect": { + "description": "Redirects this file **adds**. A duplicate glob is refused.\n\nNeeds no raise-only clamp, and that is a decision rather than an\noversight: a redirect changes what a refusal *says*, never whether it\nfires, so there is no bar here to lower. Refusing a redefinition is\ncoherence with the other append-only tables.", + "type": "array", + "items": { + "$ref": "#/$defs/Redirect" + } + }, "rule": { "description": "Rules this file **adds**. Redefining a committed id is refused.", "type": "array", @@ -177,6 +184,25 @@ } ] }, + "Redirect": { + "description": "One declared path class and the mutation sanctioned for it.", + "type": "object", + "properties": { + "glob": { + "description": "The path class this redirect speaks for, in the engine's one glob\ndialect.\n\nDeliberately independent of `protected`: this table answers \"what should\nthey run instead\", never \"is this guarded\". A glob here matching a path\nno `protected` entry covers simply never comes up, which is the harmless\ndirection — the alternative, deriving protection from a redirect, is the\nset-collapsing CLOUD-37 exists to prevent.", + "type": "string" + }, + "mutation": { + "description": "The sanctioned mutation for that class — the \"run this instead\" a deny\ncarries.", + "type": "string" + } + }, + "additionalProperties": false, + "required": [ + "glob", + "mutation" + ] + }, "Rule": { "description": "One declarative rule from `batten.toml`'s `[[rule]]` array.\n\n`deny_unknown_fields` keeps the surface narrow (§8): a mistyped key is a hard\nerror, never a silently ignored setting that disables a gate. The struct is\nflat rather than an enum with `#[serde(flatten)]` precisely so this guarantee\nholds — `flatten` silently defeats `deny_unknown_fields`.\n`severity` is required by every kind but `judge`, which is refused it\n(CLOUD-445). The field is an `Option` so a judge row can omit it, so the\nderived `required` list cannot carry it — and a schema that merely dropped it\nwould stop flagging the missing key on the four kinds that must have one,\nmoving a check out of the editor with nothing taking its place.\n\nThis conditional puts it back, stated once here and derived into both\npublished schemas. It mirrors [`RuleKind::requires`] and\n[`RuleKind::permits`]; `tests::the_schema_conditional_matches_the_column_census`\nis what keeps the two from drifting.", "type": "object", diff --git a/schema/batten.schema.json b/schema/batten.schema.json index 692123868..ca0004678 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -174,6 +174,13 @@ "$ref": "#/$defs/Provision" } }, + "redirect": { + "description": "The per-path-class redirect table (CLOUD-280): what to run instead,\nkeyed by what is protected rather than by the verb reaching for it.\n\nConsulted before [`MutatingVerb::redirect`], which stays the fallback, so\nthe behaviour CLOUD-96 shipped is the floor rather than a regression.\nDeliberately a sibling of [`Config::protected`] rather than a widening of\nit: that set keeps its element type, so [`crate::trust`]'s\n`protected[]` weakening keys are untouched.\n\n[`MutatingVerb::redirect`]: crate::verbs::MutatingVerb::redirect", + "type": "array", + "items": { + "$ref": "#/$defs/Redirect" + } + }, "rule": { "description": "The declarative rules run against the repository. Absent or empty means\n\"no rules configured\" and nothing is reported. Which of these a given\nverb admits is the §5 effect split: `check` runs only non-spawning kinds\nand refuses the rest, `enforce` runs all of them (CLOUD-170).\n\nEvery rule pins its `severity` explicitly — the key is required, with no\nimplicit fallback — and carries a separate `scope` key whose vocabulary\nnever conflates with severity's (CLOUD-61). Both disciplines are\nenforced at parse time: omission or conflation is a usage error here,\nnever a value quietly assumed.", "type": "array", @@ -1024,6 +1031,25 @@ } ] }, + "Redirect": { + "description": "One declared path class and the mutation sanctioned for it.", + "type": "object", + "properties": { + "glob": { + "description": "The path class this redirect speaks for, in the engine's one glob\ndialect.\n\nDeliberately independent of `protected`: this table answers \"what should\nthey run instead\", never \"is this guarded\". A glob here matching a path\nno `protected` entry covers simply never comes up, which is the harmless\ndirection — the alternative, deriving protection from a redirect, is the\nset-collapsing CLOUD-37 exists to prevent.", + "type": "string" + }, + "mutation": { + "description": "The sanctioned mutation for that class — the \"run this instead\" a deny\ncarries.", + "type": "string" + } + }, + "additionalProperties": false, + "required": [ + "glob", + "mutation" + ] + }, "Rule": { "description": "One declarative rule from `batten.toml`'s `[[rule]]` array.\n\n`deny_unknown_fields` keeps the surface narrow (§8): a mistyped key is a hard\nerror, never a silently ignored setting that disables a gate. The struct is\nflat rather than an enum with `#[serde(flatten)]` precisely so this guarantee\nholds — `flatten` silently defeats `deny_unknown_fields`.\n`severity` is required by every kind but `judge`, which is refused it\n(CLOUD-445). The field is an `Option` so a judge row can omit it, so the\nderived `required` list cannot carry it — and a schema that merely dropped it\nwould stop flagging the missing key on the four kinds that must have one,\nmoving a check out of the editor with nothing taking its place.\n\nThis conditional puts it back, stated once here and derived into both\npublished schemas. It mirrors [`RuleKind::requires`] and\n[`RuleKind::permits`]; `tests::the_schema_conditional_matches_the_column_census`\nis what keeps the two from drifting.", "type": "object",