From b1fdcb8d4d173845a6e0b7214f13608c6e039d1c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 19:45:35 +0000 Subject: [PATCH] feat(hook): adjudicate mediated calls from a declarative rule table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hook`'s policy was a hardcoded `match (pair[0], pair[1])` in `hook.rs` carrying four `gh` shapes and their reason strings — the ad-hoc `if` chain of CLOUD-48's title. It now dispatches on `[[rule]]` rows, and this repo's four rows live in its own `batten.toml`. The engine stops carrying one consumer's vocabulary (non-negotiable rule 1): `mise run land`, `mise run ci-wait` and the `gh` lifecycle leave `crates/batten` entirely, and a reviewer can read what will be refused without reading Rust (§9). ## The schema delta `pattern` is reused rather than adding a shape column: for `forbid` it is a substring banned from files, for `shape` a banned command line whose first word is the effective program and the rest the adjacent non-flag words. One key, one meaning, two domains. New: `reason` (required by `shape` — a mediated deny is the whole explanation a caller gets, so an id alone is un-actionable, CLOUD-122), optional `policy_url`, and `contains`. `contains` earns its place on one real case: the landing directive lives inside a quoted `--body`, so it is not one of the words `pattern` matches, and without it every ordinary `gh pr comment` would be refused alongside the one that carries it. `glob` becomes optional, since a shape rule inspects no files, and `RuleScope::MediatedCall` is what routes a rule to the surface that evaluates it. Spelled `mediated_call`, not `command`, because `command` is already a `RuleKind` token and one word meaning two unrelated things across two keys in one table is the confusion the scope/severity separation exists to prevent. ## Validation by census, not a per-kind match `Rule::validate` was two match arms naming one required and one forbidden field each. That was correct for two kinds and three columns and fails silently the moment either grows — a new column appears in no arm, so every kind accepts it. Columns are now listed once (`Rule::columns`) and every kind is asked about all of them via `requires`/`permits`/`scopes`, with `every_optional_rule_field_is_classified_by_every_kind` failing if a column is added and left unplaced. Validation also moves to load time. It was defensible while the tree engine was the only runner; `hook` is a second one, and a malformed `mediated_call` row validated only by `check` is a row that loads, matches nothing at the mediation channel, and reads as coverage. ## What keeps `hook` unable to run configured code `Policy::from_resolved` filters on scope alone. That is safe because `RuleKind::scopes` pairs every spawning kind with `Tree` only, which `no_mediated_call_kind_spawns_a_process` pins over the whole cross product rather than a named pair. Resolving through `resolve` rather than `config::load` is deliberate: a `batten.local.toml` may only tighten, adding a shape row is a tightening, and a raise-only override model is worth nothing at a surface that ignores it. `--config-from` comes along for free. Absent authority is the empty policy — allow, silently, because `hook` mediates every call in whatever directory the agent is in. An authority that exists and cannot be read is exit 1, loud, never a deny: §7 spends 2 on the verdict alone. ## Corrections to the ticket, and one recorded negative The Ready block's acceptance (e) said an invalid `severity` exits 2. It cannot: 2 is the deny code, so a config typo would refuse every mediated call, which non-negotiable rule 5 and `no_failure_code_can_deny_a_mediated_call` forbid. Implemented as exit 1, matching every sibling config error. `the_schema_cannot_express_per_kind_requirements` records an honest gap rather than hiding it. `Rule` is a flat `deny_unknown_fields` struct, so schemars emits one `required` list and "required iff kind == shape" has nowhere to live; `glob` and `reason` are looser in the published schema than in the binary. The binary is the authority and still refuses, which is the safe direction, but it is asserted rather than left to be discovered. `hook` stays `Effect::Unclassified`. After this change `read` would be defensible — it reaches no user-supplied code — but the classification is load-bearing in exactly one place, the derived agent allowlist, and putting a deny-issuing mediator on it buys nothing an agent needs. The reason is now stated in `surface.rs` beside the row. Refs: CLOUD-48 --- .serena/memories/core.md | 24 +- batten.example.toml | 25 + batten.toml | 75 ++- crates/batten/src/config.rs | 13 +- crates/batten/src/hook.rs | 405 +++++++++++--- crates/batten/src/lib.rs | 60 ++- crates/batten/src/rules.rs | 510 +++++++++++++++--- crates/batten/src/surface.rs | 10 +- crates/batten/tests/cli.rs | 291 ++++++++-- crates/batten/tests/config_schema.rs | 64 +++ .../repos/shape-mediated-call/batten.toml.in | 13 + .../repos/shape-mediated-call/expected.in | 3 + .../repos/shape-mediated-call/lib.rs.in | 5 + schema/batten.schema.json | 43 +- 14 files changed, 1327 insertions(+), 214 deletions(-) create mode 100644 crates/batten/tests/fixtures/repos/shape-mediated-call/batten.toml.in create mode 100644 crates/batten/tests/fixtures/repos/shape-mediated-call/expected.in create mode 100644 crates/batten/tests/fixtures/repos/shape-mediated-call/lib.rs.in diff --git a/.serena/memories/core.md b/.serena/memories/core.md index 938ebf155..589c9a108 100644 --- a/.serena/memories/core.md +++ b/.serena/memories/core.md @@ -116,13 +116,25 @@ repo config > default`, declared as data in `SETTINGS` (per-key env var/flag), command-executing rule rather than silently skipping it. Each rule pins a required `severity` (`RuleSeverity`, no implicit fallback) and a separate `scope` (`RuleScope`, pinned default `tree`) — two axes that never conflate - (CLOUD-61); `any_blocking` is where severity meets the exit contract. + (CLOUD-61); `any_blocking` is where severity meets the exit contract. `scope` + is also the surface router (CLOUD-48): `tree` rules are the tree engine's, + `mediated_call` rules are `hook`'s, and `RuleKind::scopes` refuses a pairing no + surface would evaluate. Per-kind column agreement is a **census** + (`Rule::columns` × `requires`/`permits`), not a per-kind match — the match + named fields, so a new column landed in no arm and every kind accepted it. - `hook.rs` — the `hook` adjudicator (CLOUD-202): the normalized envelope, the - wrapper-lookthrough command parser, and the policy tables, ported from the - shell guards. Harness adapters decode/encode at the edges; the core is - harness-blind and fail-open. What varies per harness is the _channel_ a deny - travels over, never the number: the exit-code adapter denies with `2`, the - claude-code adapter with a `permissionDecision` document and exit `0`. + wrapper-lookthrough command parser, and the matcher. **The policy is config, + not code** (CLOUD-48): `Policy` is the `mediated_call`-scoped rows of the + _resolved_ config, so a `batten.local.toml` that adds a row is applied and + `--config-from` is inherited. The parser is quote-aware (CLOUD-269) — a quoted + operand survives as a word, which is what a path gate needs and what the old + `QUOTED` sentinel destroyed. Harness adapters decode/encode at the edges; the + core is harness-blind and fail-open. What varies per harness is the _channel_ a + deny travels over, never the number: the exit-code adapter denies with `2`, the + claude-code adapter with a `permissionDecision` document and exit `0`; + `Harness::ALL` is what keeps CLOUD-40's channel matrix total. Absent authority + is the empty policy (allow, silently); an authority that exists and cannot be + read is exit `1`, loud, never a deny. - `markers.rs` — counted suppression markers (CLOUD-36): how many times policy was waved through, and where. Tokens are config, never crate constants (rule 1); hits are pointer-only (`path:line` + marker id, rule 4) and `counts` diff --git a/batten.example.toml b/batten.example.toml index b31c066e6..56cdc1f15 100644 --- a/batten.example.toml +++ b/batten.example.toml @@ -103,6 +103,31 @@ pattern = "<<<<<<< " severity = "deny" scope = "tree" +# A shape rule refuses a banned COMMAND rather than a banned file, so it is +# adjudicated by `batten hook` against one mediated tool call at a time and is +# never evaluated by `check`. That is what `scope` selects: "tree" is the working +# tree, "mediated_call" is the single command being mediated. +# +# `pattern` is the command line as you would say it aloud. The first word is the +# program, matched *after* wrapper look-through — so this row also refuses +# `mise exec -- terraform apply` and `env TF_LOG=debug terraform apply`, which is +# the whole reason the look-through exists. The remaining words are the adjacent +# non-flag words that must follow, so `terraform apply -auto-approve` matches and +# `terraform plan apply-notes` does not. +# +# `reason` is REQUIRED here, unlike on a file rule. A file finding is a +# `path:line` a reader can open; a mediated deny is all the caller gets back, so +# a refusal that named only its id would leave nothing to act on. +[[rule]] +id = "no-unreviewed-apply" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "terraform apply" +reason = "apply runs from the pipeline, not a workstation — open a change request" +# Optional. Appended to the refusal so a caller can read the policy itself. +policy_url = "https://example.invalid/handbook/infrastructure" + # The governing config surface: the files whose bytes `batten config epoch` # hashes into one attributable value, so two records carrying the same epoch # were produced under provably the same rules. diff --git a/batten.toml b/batten.toml index 2050daae2..803a812de 100644 --- a/batten.toml +++ b/batten.toml @@ -22,10 +22,81 @@ version = 1 # is what actually forces that — an older build fails on the unknown key — and # this floor states the intent so the refusal names the version rather than the # key. -min_batten_version = "0.0.30" +min_batten_version = "0.0.42" # Batten gates this repository with its own engine — consumer #1 in practice, -# not just in principle. Each rule below runs under `batten check`. +# not just in principle. Rules below whose `scope` is "tree" run under `batten +# check`; rules whose `scope` is "mediated_call" are adjudicated by `batten hook` +# against one command at a time and are never evaluated by `check` (CLOUD-48). + +# --------------------------------------------------------------------------- +# The `gh` lifecycle, as config rather than Rust (CLOUD-48). +# +# These four rows were a hardcoded `match` inside `crates/batten/src/hook.rs`. +# Moving them here is the point of the change twice over: the engine stops +# carrying one consumer's task names (non-negotiable rule 1 — `git grep -Fn +# "mise run land" crates/` now finds nothing outside a test fixture), and a +# reviewer can read exactly what will be refused without reading Rust (§9). +# +# `pattern` for a shape rule is the banned command line as you would say it: the +# first word is the effective program — matched AFTER wrapper look-through, so +# `mise exec -- gh pr merge` and `env GH_PAGER= gh pr merge` both match — and the +# rest are the adjacent non-flag words that must follow it. +# +# `reason` is required on a shape rule, unlike on a file rule: a mediated deny +# reaches a model as the entire explanation, so a refusal that named only its id +# would be un-actionable (CLOUD-122). The crate appends the bypass hatch. +[[rule]] +id = "gh-pr-merge" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr merge" +reason = """ +`gh pr merge` (like the merge button) rewrites commits under new SHAs, \ +discarding the exact objects CI tested. Use `mise run land`, which comments \ +/fast-forward so main advances to this branch's already-passed commits.""" + +# The one row with a second condition. Only a comment carrying the landing +# directive is the lifecycle; an ordinary `gh pr comment` is not. The directive +# lives inside a quoted `--body`, so it is not one of the words `pattern` +# matches — `contains` looks at the segment as written, which is the only reason +# this key exists. +[[rule]] +id = "gh-pr-comment-fast-forward" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr comment" +contains = "fast-forward" +reason = """ +Commenting /fast-forward by hand only STARTS the merge — it does not wait for \ +it, and the usual pairing with a guessed `sleep` reports before the merge \ +lands. Use `mise run land` (backgrounded): it comments, then blocks until the \ +PR is MERGED or the fast-forward bot refuses.""" + +[[rule]] +id = "gh-pr-checks" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr checks" +reason = """ +Hand-watching CI is the ad-hoc shape `mise run ci-wait` replaces. It polls \ +check-runs for HEAD until every one is terminal, prints their conclusions, and \ +exits non-zero unless all are green — with no timeout to reintroduce the \ +VM-reap gap. Background it. (`gh pr view`/`list`/`create`, `gh pr ready`, `gh \ +api`, `gh run view` are NOT blocked.)""" + +[[rule]] +id = "gh-run-watch" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh run watch" +reason = """ +Hand-watching CI is the ad-hoc shape `mise run ci-wait` replaces. Background \ +it rather than holding a foreground poll open.""" # A conflict marker reaching a source file means a botched merge landed. The # shape is unambiguous and no compiler catches it in every file type, which is diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index b941fdc58..0b00e09eb 100644 --- a/crates/batten/src/config.rs +++ b/crates/batten/src/config.rs @@ -226,6 +226,13 @@ fn parse_ungated(text: &str, source: &str) -> Result { // matches every line of every file — still loaded clean. The completeness // test below is what stops the next table arriving orphaned the same way. crate::markers::validate(&config.markers)?; + // And the rule table, which used to be validated only by the runner that + // happened to evaluate it (CLOUD-48). That was defensible while the tree + // engine was the only runner; `batten hook` is now a second one, and a + // malformed `mediated_call` row validated only by `check` is a policy row + // that loads, matches nothing at the mediation channel, and reads as + // coverage. `run_rule` still calls `Rule::validate` as defence in depth. + crate::rules::validate(&config.rules)?; Ok(config) } @@ -371,15 +378,13 @@ mod tests { const VALIDATED_AT_LOAD: &[(&str, &str)] = &[ ("verbs", "crate::verbs::validate("), ("markers", "crate::markers::validate("), + ("rules", "crate::rules::validate("), ]; /// Tables proven well formed somewhere else, each with the reason. Listing /// an exemption is the point: a reader sees the justification rather than /// an absence, which is what an orphaned validator looks like. - const VALIDATED_BY_ITS_RUNNER: &[(&str, &str)] = &[( - "rules", - "Rule::validate fires in rules::run_rule when the rule is evaluated, not at load", - )]; + const VALIDATED_BY_ITS_RUNNER: &[(&str, &str)] = &[]; #[test] fn every_typed_config_table_has_a_validation_call_site() { diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index ecad3dbe3..86ebe0711 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -5,16 +5,21 @@ //! Three layers, deliberately separated: //! //! * [`Envelope`] — the one normalized shape every harness adapter decodes -//! into: `event`, `tool`, `input`, `cwd?`, `session?`. Harness-specific field -//! names live in the adapter for that harness, never here. -//! * the parser — quoted spans are neutralised, segments split on shell -//! separators, env prefixes and wrapper programs (`env`, `timeout`, -//! `mise exec -- …`) looked through, so policy judges the **effective** -//! program. Judging the wrapper token instead is the bug class CLOUD-181 -//! hardened the shell guards against; the port keeps that hard-won shape. -//! * the policy — a table from command shape to a deny with a fix pointer. -//! This slice carries the `gh` lifecycle table; the receipt, issue-key, -//! run-shape, and protected-path policies follow (CLOUD-202 items 3–5). +//! into: `event`, `tool`, and `command`. Harness-specific field names live in +//! the adapter for that harness, never here. (`cwd` and `session` are *not* +//! fields yet; the absence is why an absolute path argument cannot be resolved +//! against the repo root.) +//! * the parser — quoted spans become words rather than a sentinel, segments +//! split on unquoted shell separators, env prefixes and wrapper programs +//! (`env`, `timeout`, `mise exec -- …`) looked through, so policy judges the +//! **effective** program. Judging the wrapper token instead is the bug class +//! CLOUD-181 hardened the shell guards against; the port keeps that hard-won +//! shape, and CLOUD-269 extended it so a quoted operand survives as a word. +//! * the policy — **config, not code** (CLOUD-48). [`Policy`] is the +//! `mediated_call`-scoped rows of the resolved `batten.toml`, so the shapes a +//! repository refuses are readable without reading Rust (§9) and the engine +//! carries no consumer's task names (non-negotiable rule 1). This module owns +//! the matcher; the table lives in the consumer's config. //! //! **Posture: fail open.** Unreadable stdin, unparseable JSON, an envelope with //! no command — all resolve to [`Decision::Allow`]. A guard must never be the @@ -26,6 +31,10 @@ use serde::Serialize; use serde_json::Value; +use crate::resolve::Resolved; +use crate::rules::{Rule, RuleScope}; +use crate::severity::{self, ReportLevel, RuleSeverity}; + /// The harness adapters `batten hook` can speak. Each owns the decode of its /// host's payload into an [`Envelope`] and the encode of a [`Decision`] into /// what that host consumes; the core between them is harness-blind. @@ -62,9 +71,13 @@ impl Harness { } /// The normalized hook envelope — the shape the core adjudicates, whatever the -/// host called its fields. `session` is optional by design: some harnesses -/// expose two ids, some events none, so anything keyed on it degrades to -/// per-invocation (contracts doc). +/// host called its fields. +/// +/// Deliberately three fields and no more for now. A `session` id was described +/// here before it existed; anything keyed on one would have to degrade to +/// per-invocation anyway, since some harnesses expose two and some events none. +/// A `cwd` is the gap that bites: without it a path operand cannot be resolved +/// against the repo root, so an absolute path is compared as written. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Envelope { /// The lifecycle event, e.g. `PreToolUse`. @@ -118,79 +131,152 @@ pub fn decode(harness: Harness, raw: &str) -> Option { } } -/// Adjudicate an envelope against the policy tables. +/// The escape hatch, named once so the boundary and the reason text agree. +pub const BYPASS_ENV: &str = "BATTEN_GH_GUARD_BYPASS"; + +/// The mediated-call policy this run adjudicates against. /// -/// `bypass` is the caller-resolved escape hatch (the boundary reads -/// `BATTEN_GH_GUARD_BYPASS`, keeping this function pure and testable). +/// Built from the *resolved* config (§8), not the committed file alone, so a +/// `batten.local.toml` that **adds** a shape row is a gate the hook actually +/// applies — the raise-only override model is worth nothing at a surface that +/// ignores it — and `--config-from` is inherited for free. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct Policy { + shapes: Vec, + fail_on_warning: bool, +} + +impl Policy { + /// The policy that denies nothing. + /// + /// Not an error state: a repository with no authority, or a bypassed run, has + /// declared no mediated-call policy, and "nothing declared" means "nothing + /// denied". Mirrors `Config::declaring_nothing`. + #[must_use] + pub fn declaring_nothing() -> Policy { + Policy::default() + } + + /// Take the mediated-call rules out of a resolved config. + /// + /// Filters on `scope`, so the tree engine's rules are simply absent here + /// rather than skipped per-call, and a spawning kind can never reach this + /// surface — [`RuleKind::scopes`] pairs every spawning kind with + /// [`RuleScope::Tree`] alone, which is what keeps `hook` structurally unable + /// to execute a configured command. + #[must_use] + pub fn from_resolved(resolved: &Resolved) -> Policy { + Policy { + shapes: resolved + .rules + .iter() + .filter(|rule| rule.scope == RuleScope::MediatedCall) + .cloned() + .collect(), + fail_on_warning: resolved.fail_on_warning, + } + } + + /// Whether this policy can deny anything at all. + #[must_use] + pub fn is_empty(&self) -> bool { + self.shapes.is_empty() + } +} + +/// Adjudicate an envelope against the policy. +/// +/// Pure: no I/O, no environment, no clock. `bypass` is the caller-resolved +/// escape hatch (the boundary reads [`BYPASS_ENV`]), and the policy arrives as a +/// value, so every verdict is a function of config plus argv and nothing else. #[must_use] -pub fn adjudicate(envelope: &Envelope, bypass: bool) -> Decision { - if bypass || envelope.command.is_empty() { +pub fn adjudicate(policy: &Policy, envelope: &Envelope, bypass: bool) -> Decision { + if bypass || envelope.command.is_empty() || policy.is_empty() { return Decision::Allow; } - gh_lifecycle(&envelope.command) + shape_rules(policy, &envelope.command) } -/// The `gh` lifecycle policy, ported from `mise-tasks/gh-guard-check`: deny a -/// hand-rolled shape this repo's config routes through a task, allow reads and -/// everything unrecognised. -fn gh_lifecycle(command: &str) -> Decision { +/// The first shape row that matches the mediated command, in declaration order. +/// +/// Declaration order is the tie-break rather than "most specific wins": a +/// reviewer reads the table top to bottom, and any cleverer precedence would be +/// a rule about rules that the config does not state. +fn shape_rules(policy: &Policy, command: &str) -> Decision { for segment in segments(command) { let tokens: Vec<&str> = segment.words.iter().map(String::as_str).collect(); let Some(program_index) = effective_program(&tokens) else { continue; }; - if tokens[program_index] != "gh" { - continue; - } // Subcommand words with flags dropped. A value-taking flag leaves its - // value behind, but the blocked pairs are adjacent, so that never - // hides a real match (`gh -R o/r pr merge` still matches; `gh pr view + // value behind, but the blocked words are adjacent, so that never hides + // a real match (`gh -R o/r pr merge` still matches; `gh pr view // merge-fix` never does). let words: Vec<&str> = tokens[program_index + 1..] .iter() .copied() - .filter(|t| !t.starts_with('-')) + .filter(|token| !token.starts_with('-')) .collect(); - for pair in words.windows(2) { - let decision = match (pair[0], pair[1]) { - ("pr", "merge") => Some( - "Refused: `gh pr merge` is blocked — it (like the merge button) rewrites \ - commits under new SHAs, discarding the exact objects CI tested. Use `mise \ - run land`, which comments /fast-forward so main advances to this branch's \ - already-passed commits. Bypass with BATTEN_GH_GUARD_BYPASS=1.", - ), - // Only a comment carrying the landing directive; an ordinary - // `gh pr comment` is not the lifecycle. Tested against the raw - // text, since the directive lives inside the quoted `--body` - // and the word split does not care what a word contains. Scoped - // to this segment rather than the whole command, so an earlier - // `echo fast-forward` cannot make a later comment look like the - // landing directive. - ("pr", "comment") if segment.raw.contains("fast-forward") => Some( - "Refused: commenting /fast-forward by hand only STARTS the merge — it does \ - not wait for it, and the usual pairing with a guessed `sleep` reports \ - before the merge lands. Use `mise run land` (backgrounded): it comments, \ - then blocks until the PR is MERGED or the fast-forward bot refuses. Bypass \ - with BATTEN_GH_GUARD_BYPASS=1.", - ), - ("pr", "checks") | ("run", "watch") => Some( - "Refused: hand-watching CI is the ad-hoc shape `mise run ci-wait` replaces. \ - It polls check-runs for HEAD until every one is terminal, prints their \ - conclusions, and exits non-zero unless all are green — with no timeout to \ - reintroduce the VM-reap gap. Background it. (`gh pr view`/`list`/`create`, \ - `gh pr ready`, `gh api`, `gh run view` are NOT blocked.) Bypass with \ - BATTEN_GH_GUARD_BYPASS=1.", - ), - _ => None, + for rule in &policy.shapes { + if !blocks(rule.severity, policy.fail_on_warning) { + continue; + } + let Some((program, wanted)) = rule.shape() else { + continue; }; - if let Some(reason) = decision { - return Decision::Deny(reason.to_owned()); + if tokens[program_index] != program { + continue; + } + if !words + .windows(wanted.len().max(1)) + .any(|w| w == wanted.as_slice()) + { + continue; + } + // The extra literal is matched against the segment as written, + // because the thing it looks for lives inside a quoted argument and + // so is not one of the words above. + if let Some(needle) = rule.contains.as_deref() { + if !segment.raw.contains(needle) { + continue; + } } + return Decision::Deny(deny_reason(rule)); } } Decision::Allow } +/// Whether a rule at this severity blocks, once promotion has been applied. +/// +/// Routed through [`severity`] rather than matched here, so `allow` / `warn` / +/// `deny` mean the same thing at the mediation channel as in the checks +/// pipeline. One interpretation, two surfaces. +fn blocks(severity: RuleSeverity, fail_on_warning: bool) -> bool { + severity::promote(severity::row_for_rule(severity).report, fail_on_warning) == ReportLevel::Fail +} + +/// Compose a deny reason: the rule that refused, why, and where the policy lives. +/// +/// Pointer-only (rule 4) — it names the rule and the fix, never the mediated +/// command, which is the caller's own text and could carry anything. +fn deny_reason(rule: &Rule) -> String { + let mut reason = format!( + "Refused by {}: {}", + rule.id, + rule.reason.as_deref().unwrap_or("policy") + ); + if let Some(url) = rule.policy_url.as_deref() { + reason.push_str(" See "); + reason.push_str(url); + reason.push('.'); + } + reason.push_str(" Bypass with "); + reason.push_str(BYPASS_ENV); + reason.push_str("=1."); + reason +} + /// One shell-separated span of a mediated command, in the two forms policy needs. /// /// `words` is the span split into arguments with quoting resolved, so a quoted @@ -412,15 +498,50 @@ pub fn encode_claude_deny(event: &str, reason: &str) -> serde_json::Result) -> Rule { + Rule { + id: id.to_owned(), + kind: crate::rules::RuleKind::Shape, + glob: None, + severity: RuleSeverity::Deny, + scope: RuleScope::MediatedCall, + pattern: Some(pattern.to_owned()), + contains: contains.map(ToOwned::to_owned), + reason: Some(format!("use the sanctioned path for {id}")), + policy_url: None, + run: None, + } + } + + /// The `gh` lifecycle table as config, standing in for the rows this repo's + /// own `batten.toml` now carries. The policy left the crate in CLOUD-48, so + /// these tests supply it rather than assert against a baked-in table. + fn gh_policy() -> Policy { + Policy { + shapes: vec![ + shape("gh-pr-merge", "gh pr merge", None), + shape( + "gh-pr-comment-fast-forward", + "gh pr comment", + Some("fast-forward"), + ), + shape("gh-pr-checks", "gh pr checks", None), + shape("gh-run-watch", "gh run watch", None), + ], + fail_on_warning: false, + } + } + + fn envelope(command: &str) -> Envelope { + Envelope { + event: "PreToolUse".to_owned(), + tool: "Bash".to_owned(), + command: command.to_owned(), + } + } + fn adjudicate_command(command: &str) -> Decision { - adjudicate( - &Envelope { - event: "PreToolUse".to_owned(), - tool: "Bash".to_owned(), - command: command.to_owned(), - }, - false, - ) + adjudicate(&gh_policy(), &envelope(command), false) } fn is_deny(command: &str) -> bool { @@ -538,12 +659,10 @@ mod tests { #[test] fn bypass_allows_everything() { - let envelope = Envelope { - event: "PreToolUse".to_owned(), - tool: "Bash".to_owned(), - command: "gh pr merge".to_owned(), - }; - assert_eq!(adjudicate(&envelope, true), Decision::Allow); + assert_eq!( + adjudicate(&gh_policy(), &envelope("gh pr merge"), true), + Decision::Allow + ); } #[test] @@ -560,7 +679,139 @@ mod tests { // A payload with no command decodes to an empty command, which // adjudicates to Allow rather than erroring. let envelope = decode(Harness::ClaudeCode, "{}").expect("decodes"); - assert_eq!(adjudicate(&envelope, false), Decision::Allow); + assert_eq!(adjudicate(&gh_policy(), &envelope, false), Decision::Allow); + } + + #[test] + fn an_empty_policy_denies_nothing() { + // The default state, and the one most invocations are in: `hook` is + // registered once and mediates calls in directories that declare no + // policy at all. + assert_eq!( + adjudicate( + &Policy::declaring_nothing(), + &envelope("gh pr merge 42"), + false + ), + Decision::Allow + ); + } + + #[test] + fn the_deny_names_the_rule_and_its_reason() { + // Acceptance (c). The id is what a reviewer greps for in `batten.toml`; + // the reason is what the model acts on. + let Decision::Deny(reason) = adjudicate_command("gh pr merge 42") else { + panic!("a configured shape must deny"); + }; + assert!(reason.contains("gh-pr-merge"), "names the rule: {reason}"); + assert!(reason.contains("sanctioned path"), "names why: {reason}"); + assert!(reason.contains(BYPASS_ENV), "names the hatch: {reason}"); + } + + #[test] + fn the_deny_never_echoes_the_mediated_command() { + // Rule 4 at the mediation channel. The command is the caller's own text + // and can carry anything — a token, a path, a customer name — so a deny + // names the policy that refused, never the thing refused. + let secret = "gh pr merge --repo o/r-SENTINEL-9f3a"; + let Decision::Deny(reason) = adjudicate_command(secret) else { + panic!("a configured shape must deny"); + }; + assert!( + !reason.contains("SENTINEL"), + "the deny echoed the mediated command: {reason}" + ); + } + + #[test] + fn a_shape_rule_at_allow_is_configured_off() { + // `allow` is cargo-deny's "this rule is off", and it must mean the same + // thing here as in the checks pipeline — that is what routing through + // `severity::promote` buys. + let mut rule = shape("gh-pr-merge", "gh pr merge", None); + rule.severity = RuleSeverity::Allow; + let policy = Policy { + shapes: vec![rule], + fail_on_warning: false, + }; + assert_eq!( + adjudicate(&policy, &envelope("gh pr merge 42"), false), + Decision::Allow + ); + } + + #[test] + fn a_warn_shape_rule_blocks_only_once_promotion_is_on() { + let mut rule = shape("gh-pr-merge", "gh pr merge", None); + rule.severity = RuleSeverity::Warn; + let call = envelope("gh pr merge 42"); + + let advisory = Policy { + shapes: vec![rule.clone()], + fail_on_warning: false, + }; + assert_eq!( + adjudicate(&advisory, &call, false), + Decision::Allow, + "a warn row does not block a mediated call on its own" + ); + + let promoted = Policy { + shapes: vec![rule], + fail_on_warning: true, + }; + assert!( + matches!(adjudicate(&promoted, &call, false), Decision::Deny(_)), + "promotion applies at the mediation channel too" + ); + } + + #[test] + fn the_first_matching_row_wins_in_declaration_order() { + // Declaration order, not "most specific": a reviewer reads the table top + // to bottom, and any cleverer precedence would be a rule about rules the + // config never states. + let policy = Policy { + shapes: vec![ + shape("first", "gh pr merge", None), + shape("second", "gh pr merge", None), + ], + fail_on_warning: false, + }; + let Decision::Deny(reason) = adjudicate(&policy, &envelope("gh pr merge"), false) else { + panic!("must deny"); + }; + assert!(reason.contains("first"), "got: {reason}"); + } + + #[test] + fn an_extra_literal_condition_is_matched_against_the_command_as_written() { + // `contains` exists for exactly this pair: the directive lives inside a + // quoted argument, so it is not one of the words the shape matches. + assert!(matches!( + adjudicate_command("gh pr comment 7 --body /fast-forward"), + Decision::Deny(_) + )); + assert_eq!( + adjudicate_command("gh pr comment 7 --body thanks"), + Decision::Allow, + "an ordinary comment is not the lifecycle" + ); + } + + #[test] + fn a_policy_url_rides_the_deny_when_declared() { + let mut rule = shape("gh-pr-merge", "gh pr merge", None); + rule.policy_url = Some("https://example.invalid/policy".to_owned()); + let policy = Policy { + shapes: vec![rule], + fail_on_warning: false, + }; + let Decision::Deny(reason) = adjudicate(&policy, &envelope("gh pr merge"), false) else { + panic!("must deny"); + }; + assert!(reason.contains("example.invalid/policy"), "got: {reason}"); } #[test] diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 207897963..bdfa3445e 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -81,7 +81,7 @@ pub fn run(cli: Cli, out: &mut dyn Write) -> Result { Some(Command::Spec { format }) => run_spec(format, out), Some(Command::Doctor { json }) => run_doctor(json, out), Some(Command::Generate { command }) => run_generate(&command, out), - Some(Command::Hook { harness }) => run_hook(harness, out), + Some(Command::Hook { harness }) => run_hook(harness, &overrides, out), // The receipt verbs read their own git facts; the §8 config chain does // not apply — a receipt records policy (as a digest), it never resolves it. Some(Command::Receipt { command }) => match command { @@ -103,16 +103,68 @@ pub fn run(cli: Cli, out: &mut dyn Write) -> Result { /// the deny; the neutral exit-code adapter denies with [`ExitCode::Violation`], /// the same code a `check` violation returns, carrying its reason to stderr /// through [`Denial`] so the write stays at the binary boundary. -fn run_hook(harness: hook::Harness, out: &mut dyn Write) -> Result { +fn run_hook( + harness: hook::Harness, + overrides: &Overrides, + out: &mut dyn Write, +) -> Result { let mut raw = String::new(); if std::io::stdin().read_to_string(&mut raw).is_err() { return Ok(ExitCode::Success); } - let bypass = std::env::var_os("BATTEN_GH_GUARD_BYPASS").is_some_and(|v| !v.is_empty()); + let bypass = std::env::var_os(hook::BYPASS_ENV).is_some_and(|value| !value.is_empty()); let Some(envelope) = hook::decode(harness, &raw) else { return Ok(ExitCode::Success); }; - match hook::adjudicate(&envelope, bypass) { + // Only now is config touched. Ordering the cheap refusals first is §4's + // "cheap when irrelevant" applied to the hottest path in the binary — this + // runs on every mediated tool call — and it is also what keeps a bypassed or + // command-less call from being able to fail on an unrelated config error. + let policy = if bypass || envelope.command.is_empty() { + hook::Policy::declaring_nothing() + } else { + load_policy(overrides)? + }; + decide(harness, &envelope, &policy, bypass, out) +} + +/// Resolve the mediated-call policy for this run. +/// +/// **Absent authority is the empty policy, not an error.** `batten hook` is +/// registered once and then mediates every call in whatever directory the agent +/// happens to be in, most of which are not Batten repositories; refusing there +/// would make the guard the reason ordinary work stops. +/// +/// An authority that exists and **cannot be read** is the opposite case, and it +/// propagates: a policy file that does not parse means the rules the operator +/// wrote are not being applied, and silently allowing would be the false green +/// this engine exists to catch. It surfaces as a [`UsageError`] — exit `1`, loud +/// on stderr, and structurally not a deny, because §7 spends `2` on the verdict +/// alone. +fn load_policy(overrides: &Overrides) -> Result { + let here = std::path::Path::new("."); + if !here.join(config::CONFIG_FILE).exists() { + return Ok(hook::Policy::declaring_nothing()); + } + Ok(hook::Policy::from_resolved(&resolve::resolve( + here, overrides, + )?)) +} + +/// Map one decoded call onto its harness's decision channel. +/// +/// Split out of [`run_hook`] so the mapping is reachable without the process's +/// own stdin: `run_hook` owns the boundary (stdin, the bypass variable, the +/// config load) and this owns the contract CLOUD-40's matrix pins — including +/// the case where writing the decision document itself fails. +fn decide( + harness: hook::Harness, + envelope: &hook::Envelope, + policy: &hook::Policy, + bypass: bool, + out: &mut dyn Write, +) -> Result { + match hook::adjudicate(policy, envelope, bypass) { hook::Decision::Allow => Ok(ExitCode::Success), hook::Decision::Deny(reason) => match harness { hook::Harness::ClaudeCode => { diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index abb6a7dd0..b15206084 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -18,10 +18,13 @@ //! than skipping it, because a skipped gate that still exits `0` is the //! false-green Batten exists to catch. //! -//! Two kinds ship: [`RuleKind::Forbid`], a static banned-shape literal check, -//! and [`RuleKind::Command`], the dynamic escape hatch that runs a configured -//! command and reads its exit code as the predicate. Further kinds slot in as -//! new variants with one match arm each. Two properties are load-bearing and +//! Three kinds ship: [`RuleKind::Forbid`], a static banned-shape literal check +//! over files; [`RuleKind::Command`], the dynamic escape hatch that runs a +//! configured command and reads its exit code as the predicate; and +//! [`RuleKind::Shape`], a banned *command* shape adjudicated by `batten hook` +//! against one mediated call. [`RuleScope`] is what routes a rule to the surface +//! that evaluates it, and [`RuleKind::scopes`] pairs the two so a rule no +//! surface would ever run cannot load. Two properties are load-bearing and //! preserved by every kind added later: //! //! * **Pointer-only output** (non-negotiable rule 4): a finding is a @@ -64,15 +67,33 @@ pub enum RuleKind { /// declared in `batten.toml`, it runs only on the non-`read` surface (§5, /// CLOUD-170). Command, + /// A banned **command shape**: the mediated call `batten hook` is + /// adjudicating matches `pattern` read as an effective program plus the + /// adjacent words that must follow it. + /// + /// Distinct from [`RuleKind::Command`], which *runs* a declared command. + /// This one runs nothing — it is a string match over a command line the + /// host handed us, so it cannot spawn and stays on the read-safe surface. + Shape, } impl RuleKind { - /// Every kind the engine knows, so the spawn partition below is total. + /// Every kind the engine knows, so the partitions below are total. /// /// A new variant must be added here or [`tests::all_covers_every_kind`] /// fails — which is what keeps [`RuleKind::spawns_processes`] from silently /// defaulting a spawning kind to "safe". - pub const ALL: &'static [RuleKind] = &[RuleKind::Forbid, RuleKind::Command]; + pub const ALL: &'static [RuleKind] = &[RuleKind::Forbid, RuleKind::Command, RuleKind::Shape]; + + /// The stable lowercase token used in config and machine output (§6). + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + RuleKind::Forbid => "forbid", + RuleKind::Command => "command", + RuleKind::Shape => "shape", + } + } /// Whether running this kind can execute a process declared in /// `batten.toml`. @@ -85,10 +106,52 @@ impl RuleKind { #[must_use] pub const fn spawns_processes(self) -> bool { match self { - RuleKind::Forbid => false, + RuleKind::Forbid | RuleKind::Shape => false, RuleKind::Command => true, } } + + /// The columns this kind cannot load without. + #[must_use] + pub const fn requires(self) -> &'static [&'static str] { + match self { + RuleKind::Forbid => &["glob", "pattern"], + RuleKind::Command => &["glob", "run"], + // A shape row's `reason` is required, not optional: the deny it + // produces reaches a model as the whole explanation, and a refusal + // with nothing but an id is the un-actionable shape CLOUD-122 exists + // to prevent. + RuleKind::Shape => &["pattern", "reason"], + } + } + + /// Every column this kind accepts — a superset of [`RuleKind::requires`]. + /// + /// Anything outside this set is a usage error rather than an ignored key: a + /// `glob` on a shape rule selects files a shape rule never reads, so + /// accepting it would let a reviewer believe a rule is scoped when it is not. + #[must_use] + pub const fn permits(self) -> &'static [&'static str] { + match self { + RuleKind::Forbid => &["glob", "pattern"], + RuleKind::Command => &["glob", "run"], + RuleKind::Shape => &["pattern", "reason", "contains", "policy_url"], + } + } + + /// The scopes this kind may declare. + /// + /// The pairing is what makes the scope key a real router rather than a label: + /// a file kind scoped to the mediated call, or a shape kind scoped to the + /// tree, would be a rule that no surface ever evaluates — present, inert, and + /// reading as covered. + #[must_use] + pub const fn scopes(self) -> &'static [RuleScope] { + match self { + RuleKind::Forbid | RuleKind::Command => &[RuleScope::Tree], + RuleKind::Shape => &[RuleScope::MediatedCall], + } + } } /// Which file domain a rule evaluates over — *where a rule looks*, never what a @@ -110,21 +173,29 @@ impl RuleKind { #[serde(rename_all = "snake_case")] #[non_exhaustive] pub enum RuleScope { - /// The whole working tree: every file the walk yields. The only domain the - /// engine evaluates today, and the pinned default. + /// The whole working tree: every file the walk yields. The pinned default. #[default] Tree, + /// The single command `batten hook` is adjudicating — not a file domain at + /// all, which is why a rule declaring it reads no `glob`. + /// + /// Spelled `mediated_call` rather than `command` on purpose: `command` is + /// already a [`RuleKind`] token meaning "run this", and one word meaning two + /// unrelated things across two keys is the cross-vocabulary confusion + /// [`tests::severity_and_scope_vocabularies_do_not_cross`] exists to prevent. + MediatedCall, } impl RuleScope { /// Every scope the engine knows, so vocabulary tests stay total. - pub const ALL: &'static [RuleScope] = &[RuleScope::Tree]; + pub const ALL: &'static [RuleScope] = &[RuleScope::Tree, RuleScope::MediatedCall]; /// The stable lowercase token used in config and machine output (§6). #[must_use] pub const fn as_str(self) -> &'static str { match self { RuleScope::Tree => "tree", + RuleScope::MediatedCall => "mediated_call", } } } @@ -146,7 +217,11 @@ pub struct Rule { /// The glob selecting which files the rule inspects, matched against /// repo-relative paths (`/`-separated). `**` matches any run of path /// segments, `*` and `?` match within a single segment. - pub glob: String, + /// + /// Optional at the type level and required per-kind: a file kind cannot load + /// without it, and [`RuleKind::Shape`] — which inspects no files — rejects it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub glob: Option, /// What a match does: `deny` fails the run, `warn` reports without failing /// (until `--fail-on-warning` promotes it, CLOUD-49), `allow` switches the /// rule off (cargo-deny's model, CLOUD-61). @@ -161,10 +236,36 @@ pub struct Rule { /// [`RuleScope::Tree`], the pinned per-field default. #[serde(default)] pub scope: RuleScope, - /// The literal substring a [`RuleKind::Forbid`] rule bans from matched - /// files. Required by that kind, rejected by any other. + /// The literal shape this rule bans — one meaning, two domains. + /// + /// For [`RuleKind::Forbid`] it is a substring banned from matched *files*. + /// For [`RuleKind::Shape`] it is a banned *command line*: the first word is + /// the effective program (matched after wrapper look-through) and the rest + /// are the adjacent non-flag words that must follow it, so + /// `pattern = "gh pr merge"` reads exactly as a reviewer would say it aloud. + /// Required by both kinds, rejected by [`RuleKind::Command`]. #[serde(default, skip_serializing_if = "Option::is_none")] pub pattern: Option, + /// An extra literal that must appear in the mediated command **as written** + /// for a [`RuleKind::Shape`] rule to fire. Optional, that kind only. + /// + /// It exists for one real case rather than as a general escape hatch: a + /// landing directive lives inside a quoted `--body`, so it is not one of the + /// words the shape matches, and without this an ordinary `gh pr comment` + /// would be denied alongside the one that carries the directive. Matched + /// against the raw text of the same segment, quotes included. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub contains: Option, + /// Why this rule refuses, and what to do instead — the deny's whole text. + /// + /// Required by [`RuleKind::Shape`], where the refusal is all a caller gets; + /// optional elsewhere, where a `path:line` pointer plus the id already says + /// where to look. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason: Option, + /// An optional link to the policy this rule enforces, appended to the deny. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub policy_url: Option, /// The command template a [`RuleKind::Command`] rule runs. Required by that /// kind, rejected by any other. /// @@ -202,32 +303,110 @@ impl Rule { /// `deny_unknown_fields`), so the kind/field agreement that a tagged enum /// would give for free is asserted here instead — and a field belonging to /// another kind is an *error*, never ignored, so a rule can never half-apply. + /// Every optional column, paired with whether this rule carries it. + /// + /// The census is what keeps [`Rule::validate`] total. The previous version + /// hand-wrote one match arm per kind naming one required and one forbidden + /// field, which was correct for two kinds and two columns and goes stale + /// silently the moment either grows: a new column simply appears in no arm, + /// so every kind accepts it. Listing the columns once and asking each kind + /// about all of them makes that failure impossible, and + /// [`tests::every_optional_rule_field_is_classified_by_every_kind`] fails if + /// a column is added here without being placed. + fn columns(&self) -> [(&'static str, bool); 6] { + [ + ("glob", self.glob.is_some()), + ("pattern", self.pattern.is_some()), + ("run", self.run.is_some()), + ("contains", self.contains.is_some()), + ("reason", self.reason.is_some()), + ("policy_url", self.policy_url.is_some()), + ] + } + fn validate(&self) -> anyhow::Result<()> { - let (required, extra) = match self.kind { - RuleKind::Forbid => (self.pattern.is_some(), self.run.is_some().then_some("run")), - RuleKind::Command => ( - self.run.is_some(), - self.pattern.is_some().then_some("pattern"), - ), - }; - let (needs, kind) = match self.kind { - RuleKind::Forbid => ("pattern", "forbid"), - RuleKind::Command => ("run", "command"), - }; - if !required { + let kind = self.kind.as_str(); + for column in self.kind.requires() { + let present = self + .columns() + .into_iter() + .any(|(name, present)| name == *column && present); + if !present { + return Err(UsageError::raise(format!( + "rule {}: kind \"{kind}\" requires `{column}`", + self.id + ))); + } + } + for (name, present) in self.columns() { + if present && !self.kind.permits().contains(&name) { + return Err(UsageError::raise(format!( + "rule {}: `{name}` is not valid for kind \"{kind}\"", + self.id + ))); + } + } + // Scope routes a rule to the surface that evaluates it, so a pairing the + // engine cannot honour must be refused rather than accepted and never + // run. An inert rule reads as coverage. + if !self.kind.scopes().contains(&self.scope) { return Err(UsageError::raise(format!( - "rule {}: kind \"{kind}\" requires `{needs}`", - self.id + "rule {}: kind \"{kind}\" does not evaluate over scope \"{}\"", + self.id, + self.scope.as_str() ))); } - if let Some(extra) = extra { + if self.glob.as_deref().is_some_and(str::is_empty) { return Err(UsageError::raise(format!( - "rule {}: `{extra}` is not valid for kind \"{kind}\"", + "rule {}: `glob` must not be empty", self.id ))); } Ok(()) } + + /// The banned command shape: the effective program, then the adjacent words + /// that must follow it. + /// + /// `None` for any kind but [`RuleKind::Shape`], and for a `pattern` with no + /// program token. + #[must_use] + pub fn shape(&self) -> Option<(&str, Vec<&str>)> { + if self.kind != RuleKind::Shape { + return None; + } + let mut words = self.pattern.as_deref()?.split_whitespace(); + let program = words.next()?; + Some((program, words.collect())) + } +} + +/// Validate a whole `[[rule]]` table, and refuse a duplicated id. +/// +/// Called at **load** rather than only by a runner (`config::parse_ungated`), +/// because there are now two runners: the tree engine and `batten hook`. A rule +/// validated only by the surface that happens to run it is a rule the other +/// surface accepts malformed — and for the hook that means a policy row that +/// loads, matches nothing, and reads as coverage. +/// +/// Two rows for one id is a policy question with two answers, and silently +/// taking the first is how a tightening edit gets lost behind a stale row. Same +/// reasoning, and the same shape, as [`crate::verbs::validate`]. +/// +/// # Errors +/// +/// Returns a [`UsageError`] (→ exit `1`) for a malformed or duplicated rule. +pub fn validate(rules: &[Rule]) -> anyhow::Result<()> { + for (index, rule) in rules.iter().enumerate() { + rule.validate()?; + if rules[..index].iter().any(|prior| prior.id == rule.id) { + return Err(UsageError::raise(format!( + "rule {}: declared twice; a rule has one definition", + rule.id + ))); + } + } + Ok(()) } /// A single policy finding: the rule that fired and where, as a pointer only. @@ -330,13 +509,20 @@ fn run_rule( files: &[String], findings: &mut Vec, ) -> anyhow::Result<()> { - if rule.glob.is_empty() { - return Err(UsageError::raise(format!( - "rule {}: glob must not be empty", - rule.id - ))); - } + // Validation first, and it owns the empty-glob refusal now: the census in + // `Rule::validate` is the one place that knows which columns a kind needs, + // so a second check here could only disagree with it. rule.validate()?; + // A rule the tree engine does not evaluate is not this surface's business. + // `validate` has already refused an unevaluable kind/scope pairing, so a + // skip here is a rule another surface owns, never one nothing runs. + if rule.scope != RuleScope::Tree { + return Ok(()); + } + let Some(glob) = rule.glob.as_deref() else { + // Unreachable for a tree-scoped kind, whose census requires `glob`. + return Ok(()); + }; // An `allow` rule is configured off: a match is not a finding at all. It is // still validated above — a malformed rule is a config error even when off, // because "off" must never double as "unreadable" — but it matches nothing @@ -346,10 +532,7 @@ fn run_rule( if rule.severity == RuleSeverity::Allow { return Ok(()); } - let matched: Vec<&String> = files - .iter() - .filter(|path| glob_match(&rule.glob, path)) - .collect(); + let matched: Vec<&String> = files.iter().filter(|path| glob_match(glob, path)).collect(); // The glob is a gate before it is an argv source (§4 "cheap when // irrelevant"): no match means the rule is skipped entirely — for a command // rule, without ever spawning. @@ -363,6 +546,10 @@ fn run_rule( } } RuleKind::Command => command_rule(rule, root, &matched, findings)?, + // Unreachable: a shape rule is `mediated_call`-scoped, and the scope + // guard above returned before the walk. Stated rather than caught by a + // wildcard so adding a kind that *is* tree-scoped has to come here. + RuleKind::Shape => {} } Ok(()) } @@ -489,7 +676,11 @@ fn run_once( findings.push(Finding { rule: rule.id.clone(), severity: rule.severity, - path: rule.glob.clone(), + // The rule's glob is the tightest honest pointer for a finding a + // command condemned as a batch. A command kind cannot load without + // one, so the fallback is unreachable; naming the rule id rather + // than an empty string keeps it a usable pointer if it ever is not. + path: rule.glob.as_deref().unwrap_or(&rule.id).to_owned(), line: None, }); } @@ -811,27 +1002,46 @@ mod tests { fs::write(path, contents).unwrap(); } - fn forbid(id: &str, glob: &str, pattern: &str) -> Rule { + /// A rule with every column empty, for a test to fill in the one it means. + /// + /// Keeps the fixtures below from re-listing six `None`s each, so adding a + /// column touches this one place rather than every test. + fn blank(id: &str, kind: RuleKind) -> Rule { Rule { id: id.to_owned(), - kind: RuleKind::Forbid, - glob: glob.to_owned(), + kind, + glob: None, severity: RuleSeverity::Deny, - scope: RuleScope::Tree, - pattern: Some(pattern.to_owned()), + scope: kind.scopes()[0], + pattern: None, + contains: None, + reason: None, + policy_url: None, run: None, } } + fn forbid(id: &str, glob: &str, pattern: &str) -> Rule { + Rule { + glob: Some(glob.to_owned()), + pattern: Some(pattern.to_owned()), + ..blank(id, RuleKind::Forbid) + } + } + fn command(id: &str, glob: &str, run: &str) -> Rule { Rule { - id: id.to_owned(), - kind: RuleKind::Command, - glob: glob.to_owned(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, - pattern: None, + glob: Some(glob.to_owned()), run: Some(run.to_owned()), + ..blank(id, RuleKind::Command) + } + } + + fn shape(id: &str, pattern: &str, reason: &str) -> Rule { + Rule { + pattern: Some(pattern.to_owned()), + reason: Some(reason.to_owned()), + ..blank(id, RuleKind::Shape) } } @@ -922,6 +1132,154 @@ mod tests { assert!(findings.is_empty()); } + #[test] + fn every_optional_rule_field_is_classified_by_every_kind() { + // The census's own gate. Every column a rule can carry must be either + // required or permitted by at least one kind — a column classified by no + // kind is one every kind rejects, which is a field nobody can ever set. + // This is what the hand-written per-kind match could not give: it named + // fields, so a new one simply appeared in no arm. + let all = blank("probe", RuleKind::Forbid).columns(); + for (name, _) in all { + let classified = RuleKind::ALL + .iter() + .any(|kind| kind.permits().contains(&name)); + assert!( + classified, + "column `{name}` is permitted by no kind, so it can never be set" + ); + } + // And `requires` must be a subset of `permits` for every kind, or a kind + // would demand a column it then rejects. + for kind in RuleKind::ALL { + for required in kind.requires() { + assert!( + kind.permits().contains(required), + "{kind:?} requires `{required}` but does not permit it" + ); + } + } + } + + #[test] + fn no_mediated_call_kind_spawns_a_process() { + // What actually makes `hook`'s dispatch structurally unable to run a + // configured command, stated over the whole cross product rather than a + // named pair. `Policy::from_resolved` filters on scope alone, so this is + // the property that filter relies on. + for kind in RuleKind::ALL { + if kind.scopes().contains(&RuleScope::MediatedCall) { + assert!( + !kind.spawns_processes(), + "{kind:?} is adjudicable at the mediation channel and can spawn" + ); + } + } + } + + #[test] + fn kind_and_scope_vocabularies_do_not_cross() { + // The same separation `severity_and_scope_vocabularies_do_not_cross` + // enforces, applied to the pair that nearly collided: `command` is a + // kind, and naming a scope `command` too would make one word mean two + // unrelated things across two keys in the same table. + for kind in RuleKind::ALL { + for scope in RuleScope::ALL { + assert_ne!( + kind.as_str(), + scope.as_str(), + "the token {:?} names both a kind and a scope", + kind.as_str() + ); + } + } + } + + #[test] + fn a_kind_only_accepts_its_own_scopes() { + // A kind/scope pairing the engine cannot honour must be refused, never + // accepted and then never evaluated. An inert rule reads as coverage, + // which is the failure this whole file is written against. + for kind in RuleKind::ALL { + for scope in RuleScope::ALL { + let mut rule = blank("pairing", *kind); + rule.scope = *scope; + // Fill whatever this kind requires so the only possible + // complaint is the pairing. + for column in kind.requires() { + match *column { + "glob" => rule.glob = Some("**".to_owned()), + "pattern" => rule.pattern = Some("x".to_owned()), + "run" => rule.run = Some("true".to_owned()), + "reason" => rule.reason = Some("because".to_owned()), + other => panic!("unclassified required column `{other}`"), + } + } + let result = rule.validate(); + if kind.scopes().contains(scope) { + assert!(result.is_ok(), "{kind:?}/{scope:?} must load"); + } else { + let err = result.expect_err("an unevaluable pairing must be refused"); + assert!(err.downcast_ref::().is_some()); + } + } + } + } + + #[test] + fn a_shape_rule_requires_a_pattern_and_a_reason() { + let mut no_pattern = blank("s", RuleKind::Shape); + no_pattern.reason = Some("because".to_owned()); + assert!(no_pattern.validate().is_err(), "a shape needs a pattern"); + + let mut no_reason = blank("s", RuleKind::Shape); + no_reason.pattern = Some("gh pr merge".to_owned()); + // A mediated deny reaches a model as the whole explanation, so a row + // that would refuse with nothing but an id cannot load (CLOUD-122). + assert!(no_reason.validate().is_err(), "a shape needs a reason"); + } + + #[test] + fn a_shape_rule_rejects_a_glob_and_a_run() { + for column in ["glob", "run"] { + let mut rule = shape("s", "gh pr merge", "because"); + match column { + "glob" => rule.glob = Some("**".to_owned()), + _ => rule.run = Some("true".to_owned()), + } + let err = rule + .validate() + .expect_err("a shape rule inspects no files and runs nothing"); + assert!( + format!("{err}").contains(column), + "the refusal must name `{column}`" + ); + } + } + + #[test] + fn the_tree_engine_skips_a_mediated_call_rule_without_running_it() { + // Routing, from the tree side: a shape row contributes no finding and no + // error to `check`. It must not be refused either — `check` refusing a + // rule another surface owns would make every hook-using repo unable to + // run `check` at all. + let dir = temp_dir("rules-skip-mediated"); + write(&dir, "a.rs", "gh pr merge\n"); + let rule = shape("no-hand-merge", "gh pr merge", "use the landing path"); + let findings = run_static(std::slice::from_ref(&rule), &dir).unwrap(); + assert!(findings.is_empty(), "a shape rule is not a file rule"); + } + + #[test] + fn a_rule_id_declared_twice_is_a_usage_error() { + // Two rows for one id is a policy question with two answers, and taking + // the first silently is how a tightening edit gets lost behind a stale + // row. Same reasoning as the verb table's duplicate refusal. + let rules = [forbid("dup", "**", "a"), forbid("dup", "**", "b")]; + let err = validate(&rules).expect_err("a duplicated id must be refused"); + assert!(err.downcast_ref::().is_some()); + } + #[test] fn all_covers_every_kind() { // The spawn partition must be total. `ALL` is what the gate below @@ -930,12 +1288,12 @@ mod tests { // The match is exhaustive by the compiler; this asserts `ALL` agrees. for kind in RuleKind::ALL { match kind { - RuleKind::Forbid | RuleKind::Command => {} + RuleKind::Forbid | RuleKind::Command | RuleKind::Shape => {} } } assert_eq!( RuleKind::ALL.len(), - 2, + 3, "a new RuleKind must be added to RuleKind::ALL" ); } @@ -954,13 +1312,9 @@ mod tests { continue; } let rule = Rule { - id: "spawner".to_owned(), - kind: *kind, - glob: "**".to_owned(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, - pattern: None, + glob: Some("**".to_owned()), run: Some("true".to_owned()), + ..blank("spawner", *kind) }; let err = run_static(std::slice::from_ref(&rule), &dir).unwrap_err(); assert!( @@ -1095,41 +1449,29 @@ mod tests { let dir = temp_dir("cmd-schema"); write(&dir, "a.rs", "x\n"); let cases = [ + // command with no `run` Rule { - id: "a".into(), - kind: RuleKind::Command, - glob: "**".into(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, - pattern: None, - run: None, + glob: Some("**".into()), + ..blank("a", RuleKind::Command) }, + // command carrying a forbid-only column Rule { - id: "b".into(), - kind: RuleKind::Command, - glob: "**".into(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, + glob: Some("**".into()), pattern: Some("x".into()), run: Some("true".into()), + ..blank("b", RuleKind::Command) }, + // forbid with no `pattern` Rule { - id: "c".into(), - kind: RuleKind::Forbid, - glob: "**".into(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, - pattern: None, - run: None, + glob: Some("**".into()), + ..blank("c", RuleKind::Forbid) }, + // forbid carrying a command-only column Rule { - id: "d".into(), - kind: RuleKind::Forbid, - glob: "**".into(), - severity: RuleSeverity::Deny, - scope: RuleScope::Tree, + glob: Some("**".into()), pattern: Some("x".into()), run: Some("true".into()), + ..blank("d", RuleKind::Forbid) }, ]; for rule in cases { @@ -1166,7 +1508,7 @@ mod tests { // `ALL` stays total: a new variant must extend it or this stops compiling. for scope in RuleScope::ALL { match scope { - RuleScope::Tree => {} + RuleScope::Tree | RuleScope::MediatedCall => {} } } } diff --git a/crates/batten/src/surface.rs b/crates/batten/src/surface.rs index 708ab6d89..e81e098f1 100644 --- a/crates/batten/src/surface.rs +++ b/crates/batten/src/surface.rs @@ -413,7 +413,15 @@ pub const SURFACE: &[CommandDecl] = &[ // `hook` adjudicates another tool's call: its own execution only reads // stdin and config, but its *decision* mediates writes, so it is listed // unclassified rather than allowed to leak into the derived read-only - // allowlist (CLOUD-202). + // allowlist (CLOUD-202). Re-examined when CLOUD-48 made the policy config: + // `hook` still reaches no user-supplied code — `RuleKind::scopes` pairs every + // spawning kind with `RuleScope::Tree` alone, and `Policy::from_resolved` + // filters on scope, which `rules::tests::no_mediated_call_kind_spawns_a_process` + // pins. So `read` would now be *defensible*; it stays `Unclassified` because + // the classification is load-bearing in one place only — the derived + // allowlist — and putting a deny-issuing mediator on the agent's read-only + // list buys nothing an agent needs. §5's rule is that an unclassified command + // is listed with a stated reason rather than guessed; this is that reason. CommandDecl { path: "hook", about: "Adjudicate a mediated tool call read from stdin (a deny is exit 2, the one contract)", diff --git a/crates/batten/tests/cli.rs b/crates/batten/tests/cli.rs index 18346146a..fdaf37a4c 100644 --- a/crates/batten/tests/cli.rs +++ b/crates/batten/tests/cli.rs @@ -21,8 +21,72 @@ use common::{Fixture, batten, git_in, scratch}; /// The ambient bypass var is removed so a developer's shell can never flip a /// deny case; the `bypass` flag sets it explicitly for the case that wants it. fn run_hook(harness: &str, payload: &str, bypass: bool) -> Output { + run_hook_in( + &PathBuf::from(env!("CARGO_MANIFEST_DIR")), + harness, + payload, + bypass, + ) +} + +/// A fixture repo whose `batten.toml` carries the `gh` lifecycle shape rows. +/// +/// Since CLOUD-48 the policy is config, not Rust, so a hook test that wants a +/// deny has to supply one. Deliberately *not* this repo's own `batten.toml`: a +/// test that read the committed file would pass or fail with an edit to +/// production policy, and the committed rows have their own gate +/// (`the_committed_shape_rules_fire_on_every_banned_shape`). +fn repo_with_gh_policy(name: &str) -> PathBuf { + repo_with_config( + name, + r#"version = 1 +[[rule]] +id = "gh-pr-merge" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr merge" +reason = "use `mise run land`" + +[[rule]] +id = "gh-pr-comment-fast-forward" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr comment" +contains = "fast-forward" +reason = "use `mise run land`" + +[[rule]] +id = "gh-pr-checks" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr checks" +reason = "use `mise run ci-wait`" + +[[rule]] +id = "gh-run-watch" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh run watch" +reason = "use `mise run ci-wait`" +"#, + ) +} + +/// [`run_hook`], with the directory the policy is resolved from. +/// +/// The directory is now load-bearing: `hook` reads its authority from the cwd +/// (CLOUD-48), so a test that does not set one adjudicates against the empty +/// policy and allows everything. `run_hook` itself keeps the old signature and +/// points at `crates/batten/`, which has no `batten.toml` — that is the +/// no-authority case, which several tests want. +fn run_hook_in(dir: &std::path::Path, harness: &str, payload: &str, bypass: bool) -> Output { let mut command = batten(); command + .current_dir(dir) .args(["hook", "--harness", harness]) .env_remove("BATTEN_GH_GUARD_BYPASS") .stdin(Stdio::piped()) @@ -676,8 +740,9 @@ const MATRIX: &[Row] = &[ #[test] fn the_decision_channel_matrix_holds_for_every_harness() { + let dir = repo_with_gh_policy("matrix-policy"); for row in MATRIX { - let output = run_hook(row.harness, &claude_payload(row.command), false); + let output = run_hook_in(&dir, row.harness, &claude_payload(row.command), false); let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let at = format!("{}/{}", row.harness, row.case); @@ -736,8 +801,9 @@ fn a_quoted_invocation_denies_on_both_harness_channels() { // `gh pr merge`, and the sentinel parser let it through because the span // never became tokens. Checked on both channels so the tightening is // pinned wherever a host reads its decision. + let dir = repo_with_gh_policy("quoted-invocation"); for harness in ["claude-code", "exit-code"] { - let output = run_hook(harness, &claude_payload("gh \"pr\" \"merge\""), false); + let output = run_hook_in(&dir, harness, &claude_payload("gh \"pr\" \"merge\""), false); let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); if harness == "claude-code" { @@ -748,7 +814,7 @@ fn a_quoted_invocation_denies_on_both_harness_channels() { ); } else { assert_eq!(output.status.code(), Some(2), "{harness}"); - assert!(stderr.contains("Refused"), "{harness}: got {stderr}"); + assert!(stderr.contains("Refused by"), "{harness}: got {stderr}"); } } } @@ -783,30 +849,181 @@ fn a_malformed_harness_token_fails_loud_without_denying() { } #[test] -fn hook_reads_no_config_so_a_broken_one_cannot_block() { - // Pins *why* the config-failure leg above is unreachable rather than merely - // untested: `hook` never loads an authority, so the directory shapes that - // break every other verb are nothing to it. When that stops being true the - // deletion of this test is the seam where config became load-bearing. - let dir = repo_with_config("hook-broken-config", "this is not toml at all\n"); - let mut command = batten(); - command - .args(["hook", "--harness", "claude-code"]) - .current_dir(&dir) - .env_remove("BATTEN_GH_GUARD_BYPASS") - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); - let mut child = command.spawn().expect("spawn batten hook"); - child - .stdin - .take() - .expect("piped stdin") - .write_all(claude_payload("gh pr view 42").as_bytes()) - .expect("write payload"); - let output = child.wait_with_output().expect("run batten hook"); +fn hook_allows_when_no_authority_is_configured() { + // This replaces CLOUD-40's `hook_reads_no_config_so_a_broken_one_cannot_block` + // — the test written to be deleted here, marking the seam where config became + // load-bearing. + // + // `hook` is registered once and then mediates every call in whatever + // directory the agent is in, most of which are not Batten repositories. An + // absent authority is therefore the empty policy, not an error: nothing + // declared, nothing denied, silently. + let dir = repo_with_config("hook-no-authority", ""); + fs::remove_file(dir.join("batten.toml")).expect("remove the authority"); + let output = run_hook_in( + &dir, + "claude-code", + &claude_payload("gh pr merge 42"), + false, + ); assert_eq!(output.status.code(), Some(0)); - assert!(output.stdout.is_empty()); + assert!(output.stdout.is_empty(), "no authority denies nothing"); +} + +#[test] +fn hook_fails_open_and_loud_on_an_unloadable_authority() { + // The opposite case, and the one CLOUD-40 could not reach because `hook` + // loaded no config: an authority that EXISTS and cannot be read means the + // rules the operator wrote are not being applied. Allowing silently there + // would be the false green the engine exists to catch, so it is a usage + // error — loud on stderr, exit 1, and structurally not a deny, because §7 + // spends 2 on the verdict alone. + let dir = repo_with_config("hook-broken-authority", "this is not toml at all\n"); + for harness in ["claude-code", "exit-code"] { + let output = run_hook_in(&dir, harness, &claude_payload("gh pr view 42"), false); + let code = output.status.code(); + assert_eq!(code, Some(1), "{harness}: an unreadable authority is usage"); + assert_ne!(code, Some(2), "{harness}: must never deny"); + assert!(output.stdout.is_empty(), "{harness}: no decision document"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("batten.toml"), + "{harness}: the failure names the file, got: {stderr}" + ); + } +} + +#[test] +fn shape_scope_parses_independently_of_severity() { + // Acceptance (d). The two keys are separate axes, and a shape row proves it + // over the binary: `mediated_call` × each severity all load, and neither + // key's vocabulary is accepted in the other's slot. + for severity in ["deny", "warn", "allow"] { + let dir = repo_with_config( + &format!("shape-severity-{severity}"), + &format!( + "version = 1\n\n[[rule]]\nid = \"s\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"{severity}\"\n\ + pattern = \"gh pr merge\"\nreason = \"use the landing path\"\n" + ), + ); + let output = batten() + .args(["config", "show", "--json"]) + .current_dir(&dir) + .output() + .expect("run config show"); + assert_eq!( + output.status.code(), + Some(0), + "severity {severity} must load" + ); + let value: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("stdout is JSON"); + let rule = &value["rule"]["value"][0]; + assert_eq!(rule["severity"], severity); + assert_eq!(rule["scope"], "mediated_call"); + } + + // And crossing them is inexpressible, not silently reinterpreted. + for (key, value) in [("scope", "deny"), ("severity", "mediated_call")] { + let dir = repo_with_config( + &format!("shape-crossed-{key}"), + &format!( + "version = 1\n\n[[rule]]\nid = \"s\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\n\ + pattern = \"gh pr merge\"\nreason = \"r\"\n{key} = \"{value}\"\n" + ), + ); + assert_eq!( + batten() + .arg("check") + .current_dir(&dir) + .output() + .expect("run batten check") + .status + .code(), + Some(1), + "{key} = {value:?} must be a usage error" + ); + } +} + +#[test] +fn hook_refuses_an_invalid_severity_without_denying() { + // Acceptance (e), corrected: the ticket said exit 2, but 2 is the deny code, + // so a config typo would refuse every mediated call. Every sibling + // config error in the tree is exit 1, and non-negotiable rule 5 plus + // `no_failure_code_can_deny_a_mediated_call` forbid the alternative. + let dir = repo_with_config( + "shape-bad-severity", + "version = 1\n\n[[rule]]\nid = \"s\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"nope\"\n\ + pattern = \"gh pr merge\"\nreason = \"r\"\n", + ); + for harness in ["claude-code", "exit-code"] { + let output = run_hook_in(&dir, harness, &claude_payload("gh pr merge"), false); + let code = output.status.code(); + assert_eq!(code, Some(1), "{harness}: a bad severity is a usage error"); + assert_ne!(code, Some(2), "{harness}: must never deny"); + assert!(output.stdout.is_empty(), "{harness}: no decision document"); + } +} + +#[test] +fn hook_honours_a_shape_rule_a_local_override_added() { + // Why the policy resolves through `resolve` rather than `config::load`: the + // raise-only override model is worth nothing at a surface that ignores it. + // A `batten.local.toml` may only tighten, and adding a shape row is a + // tightening, so the hook must apply it. + let dir = repo_with_config("shape-local-override", "version = 1\n"); + with_local_config( + &dir, + "version = 1\n\n[[rule]]\nid = \"local-shape\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\n\ + pattern = \"npm publish\"\nreason = \"releases go through the release pipeline\"\n", + ); + let output = run_hook_in( + &dir, + "exit-code", + &claude_payload("npm publish --tag next"), + false, + ); + assert_eq!(output.status.code(), Some(2), "a local row must be applied"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("local-shape"), "got: {stderr}"); +} + +#[test] +fn the_committed_shape_rules_fire_on_every_banned_shape() { + // The obligation the fixture tests do not discharge. Every hook test above + // supplies its own policy, so after CLOUD-48 deleting a row from this repo's + // `batten.toml` would break none of them — and the guard chain in + // `.claude/settings.json` would silently lose a rule. `tests/cli.rs` states + // the standing rule: a rule is only pinned by a fixture that seeds the shape + // it bans, and any rule landing later owes one of its own. + let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../.."); + for command in [ + "gh pr merge 42", + "gh pr checks --watch", + "gh run watch 123", + "gh pr comment 7 --body /fast-forward", + ] { + let output = run_hook_in(&root, "exit-code", &claude_payload(command), false); + assert_eq!( + output.status.code(), + Some(2), + "the committed policy must still refuse {command:?}" + ); + } + // And the reads it must not refuse, from the same committed rows. + for command in ["gh pr view 42", "gh pr ready 42", "mise run land"] { + let output = run_hook_in(&root, "exit-code", &claude_payload(command), false); + assert_eq!( + output.status.code(), + Some(0), + "the committed policy must allow {command:?}" + ); + } } #[test] @@ -815,7 +1032,9 @@ fn hook_denies_a_blocked_shape_in_the_harness_channel() { // exit 0 — the channel the production shell guards already use. The // wrapper form is the load-bearing case: judging the wrapper token instead // of the effective program is the bug class CLOUD-181 hardened against. - let output = run_hook( + let dir = repo_with_gh_policy("deny-claude-channel"); + let output = run_hook_in( + &dir, "claude-code", &claude_payload("mise exec -- gh pr merge 42"), false, @@ -828,7 +1047,7 @@ fn hook_denies_a_blocked_shape_in_the_harness_channel() { ); assert!( stdout.contains("mise run land"), - "the deny must name the redirect" + "the deny must name the redirect the fixture policy declares" ); } @@ -865,11 +1084,12 @@ fn hook_exit_code_harness_denies_with_exit_2() { // The one contract, unmodified: 2 is the policy verdict, so a deny needs // no translation. The reason goes to stderr — the neutral channel for a // host whose only decision vocabulary is an exit status. - let output = run_hook("exit-code", &claude_payload("gh pr merge 42"), false); + let dir = repo_with_gh_policy("deny-exit-code-channel"); + let output = run_hook_in(&dir, "exit-code", &claude_payload("gh pr merge 42"), false); assert_eq!(output.status.code(), Some(2)); assert!(output.stdout.is_empty()); let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("Refused"), "got: {stderr}"); + assert!(stderr.contains("Refused by"), "got: {stderr}"); // A verdict is an answer, not a crash. The host hands this text back to the // model as the deny reason, so it must not wear the binary's error prefix. assert!( @@ -1542,7 +1762,16 @@ fn committed_rules_pin_severity_and_scope_explicitly() { ["allow", "warn", "deny"].contains(&severity), "{label}: severity {severity:?} outside the vocabulary" ); - assert_eq!(rule["scope"], "tree", "{label}: scope token"); + // Derived from the vocabulary rather than pinned to `tree`: the + // literal was correct while one scope existed, and would have had to + // be edited by every change that adds one — which is the edit a + // reviewer waves through. What matters is that the token is *in* the + // vocabulary, never that it is one particular member. + let scope = rule["scope"].as_str().expect("scope token"); + assert!( + ["tree", "mediated_call"].contains(&scope), + "{label}: scope {scope:?} outside the vocabulary" + ); } } } diff --git a/crates/batten/tests/config_schema.rs b/crates/batten/tests/config_schema.rs index 50fd2fa01..c8291fb69 100644 --- a/crates/batten/tests/config_schema.rs +++ b/crates/batten/tests/config_schema.rs @@ -274,3 +274,67 @@ fn the_gate_applies_to_every_verb_that_reads_config() { "config show skipped the gate" ); } + +#[test] +fn the_schema_accepts_a_mediated_call_rule() { + // The shape kind and its scope are part of the published vocabulary, so an + // editor validating against the schema must accept a row the binary accepts. + let schema = derived_schema(); + let validator = jsonschema::validator_for(&schema).expect("the schema compiles"); + let good = "version = 1\n\n[[rule]]\nid = \"s\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\n\ + pattern = \"gh pr merge\"\nreason = \"use the landing path\"\n"; + assert!( + validator.is_valid(&as_json(good)), + "the schema rejected a shape rule the binary accepts" + ); +} + +#[test] +fn the_schema_cannot_express_per_kind_requirements() { + // An honest negative, recorded rather than papered over (CLOUD-48). + // + // `Rule` is a flat `deny_unknown_fields` struct — deliberately, because a + // `#[serde(flatten)]` enum silently defeats that guarantee — so schemars + // emits ONE `required` list for every kind. "required iff kind == shape" + // therefore has nowhere to live, and two columns are looser in the schema + // than in the binary: `glob`, which a file kind cannot load without, and + // `reason`, which a shape kind cannot. + // + // The consequence is the mirror of `the_schema_refuses_what_the_binary_refuses`: + // an editor waves a config through that `batten check` then refuses. That is + // the safer direction of the two — the binary is the authority and it still + // refuses — but it is a real gap, so it is asserted here rather than left for + // someone to discover as a surprise. + let schema = derived_schema(); + let validator = jsonschema::validator_for(&schema).expect("the schema compiles"); + + for (label, config) in [ + // A forbid rule with no glob. + ( + "forbid without glob", + "version = 1\n\n[[rule]]\nid = \"f\"\nkind = \"forbid\"\n\ + severity = \"deny\"\npattern = \"x\"\n", + ), + // A shape rule with no reason. + ( + "shape without reason", + "version = 1\n\n[[rule]]\nid = \"s\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"gh pr merge\"\n", + ), + ] { + assert!( + validator.is_valid(&as_json(config)), + "{label}: the schema is expected to be looser here; if it now \ + refuses, per-kind requirements became expressible and this test \ + should be replaced by the positive one" + ); + // The binary is the authority, and it refuses. + let dir = repo_with_config(&format!("per-kind-{}", label.replace(' ', "-")), config); + assert_eq!( + check_in(&dir).status.code(), + Some(1), + "{label}: the binary must refuse what the schema let through" + ); + } +} diff --git a/crates/batten/tests/fixtures/repos/shape-mediated-call/batten.toml.in b/crates/batten/tests/fixtures/repos/shape-mediated-call/batten.toml.in new file mode 100644 index 000000000..443a30fda --- /dev/null +++ b/crates/batten/tests/fixtures/repos/shape-mediated-call/batten.toml.in @@ -0,0 +1,13 @@ +version = 1 + +# A shape rule is adjudicated by `batten hook` against one mediated command, not +# by the tree engine. This fixture pins the routing half of that: `check` walks +# past a `mediated_call` row and reports nothing, rather than erroring on a rule +# it cannot evaluate or — worse — evaluating it as if it selected files. +[[rule]] +id = "no-hand-merge" +kind = "shape" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr merge" +reason = "use the sanctioned landing path" diff --git a/crates/batten/tests/fixtures/repos/shape-mediated-call/expected.in b/crates/batten/tests/fixtures/repos/shape-mediated-call/expected.in new file mode 100644 index 000000000..fce10dcc0 --- /dev/null +++ b/crates/batten/tests/fixtures/repos/shape-mediated-call/expected.in @@ -0,0 +1,3 @@ +argv: check +exit: 0 +stdout: diff --git a/crates/batten/tests/fixtures/repos/shape-mediated-call/lib.rs.in b/crates/batten/tests/fixtures/repos/shape-mediated-call/lib.rs.in new file mode 100644 index 000000000..15d0af61a --- /dev/null +++ b/crates/batten/tests/fixtures/repos/shape-mediated-call/lib.rs.in @@ -0,0 +1,5 @@ +// A file with a line that would match if a shape rule were ever mistaken for a +// file rule: the pattern below is the shape this repo's rule bans. +fn main() { + // gh pr merge +} diff --git a/schema/batten.schema.json b/schema/batten.schema.json index 23013a460..e0bfd2878 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -174,9 +174,19 @@ "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`.", "type": "object", "properties": { + "contains": { + "description": "An extra literal that must appear in the mediated command **as written**\nfor a [`RuleKind::Shape`] rule to fire. Optional, that kind only.\n\nIt exists for one real case rather than as a general escape hatch: a\nlanding directive lives inside a quoted `--body`, so it is not one of the\nwords the shape matches, and without this an ordinary `gh pr comment`\nwould be denied alongside the one that carries the directive. Matched\nagainst the raw text of the same segment, quotes included.", + "type": [ + "string", + "null" + ] + }, "glob": { - "description": "The glob selecting which files the rule inspects, matched against\nrepo-relative paths (`/`-separated). `**` matches any run of path\nsegments, `*` and `?` match within a single segment.", - "type": "string" + "description": "The glob selecting which files the rule inspects, matched against\nrepo-relative paths (`/`-separated). `**` matches any run of path\nsegments, `*` and `?` match within a single segment.\n\nOptional at the type level and required per-kind: a file kind cannot load\nwithout it, and [`RuleKind::Shape`] — which inspects no files — rejects it.", + "type": [ + "string", + "null" + ] }, "id": { "description": "A stable identifier for the rule, surfaced in findings so a violation\npoints back at the policy that produced it.", @@ -187,7 +197,21 @@ "$ref": "#/$defs/RuleKind" }, "pattern": { - "description": "The literal substring a [`RuleKind::Forbid`] rule bans from matched\nfiles. Required by that kind, rejected by any other.", + "description": "The literal shape this rule bans — one meaning, two domains.\n\nFor [`RuleKind::Forbid`] it is a substring banned from matched *files*.\nFor [`RuleKind::Shape`] it is a banned *command line*: the first word is\nthe effective program (matched after wrapper look-through) and the rest\nare the adjacent non-flag words that must follow it, so\n`pattern = \"gh pr merge\"` reads exactly as a reviewer would say it aloud.\nRequired by both kinds, rejected by [`RuleKind::Command`].", + "type": [ + "string", + "null" + ] + }, + "policy_url": { + "description": "An optional link to the policy this rule enforces, appended to the deny.", + "type": [ + "string", + "null" + ] + }, + "reason": { + "description": "Why this rule refuses, and what to do instead — the deny's whole text.\n\nRequired by [`RuleKind::Shape`], where the refusal is all a caller gets;\noptional elsewhere, where a `path:line` pointer plus the id already says\nwhere to look.", "type": [ "string", "null" @@ -214,7 +238,6 @@ "required": [ "id", "kind", - "glob", "severity" ] }, @@ -230,6 +253,11 @@ "description": "A dynamic check: run the `run` template and treat its exit code as the\npredicate — `0` passes, non-zero is a violation. The sanctioned escape\nhatch for rules no static shape can express.\n\nIt is an **exit-code predicate, not a judge** (CLOUD-93): the command's\noutput is never parsed for meaning. Because it executes a process\ndeclared in `batten.toml`, it runs only on the non-`read` surface (§5,\nCLOUD-170).", "type": "string", "const": "command" + }, + { + "description": "A banned **command shape**: the mediated call `batten hook` is\nadjudicating matches `pattern` read as an effective program plus the\nadjacent words that must follow it.\n\nDistinct from [`RuleKind::Command`], which *runs* a declared command.\nThis one runs nothing — it is a string match over a command line the\nhost handed us, so it cannot spawn and stays on the read-safe surface.", + "type": "string", + "const": "shape" } ] }, @@ -237,9 +265,14 @@ "description": "Which file domain a rule evaluates over — *where a rule looks*, never what a\nmatch does (CLOUD-61).\n\nScope and severity are two independent keys on a [`Rule`], deliberately: the\nquestion \"which files does this gate watch\" and the question \"what happens\nwhen it matches\" ([`RuleSeverity`]) are different axes, and conflating them\nis the config bug this type makes inexpressible. Neither vocabulary\ndeserializes as the other, so a severity token in the `scope` key (or the\nreverse) is a usage error (exit `1`), never a silent reinterpretation.\n\nMarked `#[non_exhaustive]` like [`RuleKind`]: the git change-set / protected\n/ unlanded domains (CLOUD-36, CLOUD-37) slot in as new variants without a\nbreaking change. The default is pinned as data —\n[`tests::scope_default_is_pinned`] asserts it — so it is an explicit,\nper-field default rather than an implicit fallback buried in code.", "oneOf": [ { - "description": "The whole working tree: every file the walk yields. The only domain the\nengine evaluates today, and the pinned default.", + "description": "The whole working tree: every file the walk yields. The pinned default.", "type": "string", "const": "tree" + }, + { + "description": "The single command `batten hook` is adjudicating — not a file domain at\nall, which is why a rule declaring it reads no `glob`.\n\nSpelled `mediated_call` rather than `command` on purpose: `command` is\nalready a [`RuleKind`] token meaning \"run this\", and one word meaning two\nunrelated things across two keys is the cross-vocabulary confusion\n[`tests::severity_and_scope_vocabularies_do_not_cross`] exists to prevent.", + "type": "string", + "const": "mediated_call" } ] },