From abdf43a6a187f5e3d4be6eea2394c8bf06fe7704 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:58 +0000 Subject: [PATCH 01/21] feat(mint)!: key a receipt to the branch's change, so a code review can be owed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1484. `review-dispatched` (CLOUD-472) answers *did a named prompt run over these bytes*, and both its declared subjects are prose — `AGENTS.md` and a tracker body. Nothing has ever asked whether a review ran over the branch's own diff, so a pull request may change every line of `crates/batten` unread. THE DISPATCH INVERTS RATHER THAN REPEATING. CLOUD-472's engine spawns a reviewer, which is what forced a runner, a readiness probe and a prompt channel into a repo-agnostic core. Here Batten refuses, the refusal names what the agent owes, the agent dispatches with its own harness, and `[[mint]]` writes the receipt from the call the boundary was already mediating. That is not CLOUD-1265's refuted producer-writes-outside shape: nobody has to remember to run a producer, because the hook sees the call. `MintKey::Delta` keys a receipt by `git::branch_patch_id` — a wrapper over `cumulative_patch_id` rather than a second computation, so *the same change* means here exactly what `landing` already means by it. A rebase therefore does not re-owe the review and any commit moving code does. The boundary refuses to mint at all while the tree is dirty: a receipt keyed to committed bytes must not be taken in a session looking at something else. `selects_at`/`selects` are the load-bearing pair. Every skill this harness dispatches arrives under one tool name, so `tool` alone discriminates nothing and the row would attest that *something* ran. `requires` cannot reach it — the field is present whichever skill ran, it just holds another value. The selector reads the tool INPUT and `requires` reads the RESULT: the input says which call this is, the result says whether it answered. Two measurements changed the design while building it. A skill's result on this host is the bare string `Launching skill: `, which is not JSON, so `record_mints` gave up before reaching the row — this is the first row whose whole reading is of the input and the repository, so the result is now `null` rather than an early return. And the clean-tree condition is at the WRITE, not in the module: as a conjunct it would let a dirty tree silence the gate, and as a refusal it would be a second opinion where `tree-clean` already owns the question. The receipt attests the DISPATCH, not the completion, and that gap is named in `batten.toml` rather than papered over: the harness returns nothing that would say the agent then did what the skill told it. BREAKING CHANGE: `mint::Declared` gains `key_base`, `selects_at` and `selects`, and `mint::MintKey` gains `Delta`. Both are library-API breaks — the struct is constructible so a downstream literal no longer compiles, and an exhaustive match on the enum no longer covers it. The TOML surface is unaffected: all three columns are optional and a row that omits them is judged exactly as before, which is what an earlier draft of this branch wrongly generalised into `no consumer break`. `semver check` is what caught the difference. Weakens: mint-added mint[code-review] Refs: CLOUD-1484 Admits: 2b23578df77a1290c5cfa5f47512c93cc502705647553d90a63c5c47d4c0e5e4 Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:7e2207d701d6c58684d79582e310b37ef9e54367 Admits-epoch: f3ee6646fee52db519d6e2493d1e490f8eb22a3172c03b6bc50fc435199006b4 Admits-author: alec@wenzowski.com Admits-prev: - Admits-answer-lost: The gate would not exist. code-review-dispatched is only reachable because a [[rule]] row registers its module and a [[mint]] row keys its receipt; without them the engine change is dead code, and nothing would refuse a branch that changed code with no code review dispatched over it — the defect CLOUD-1484 was filed for. Admits-answer-precondition: A gate is declared as data in batten.toml: the [[mint]] row, the [[rule]] + [[rule.minted]] rows and the [[verdict]] class ARE the change CLOUD-1484 asks for. No [[redirect]] surface can express adding a rule, and non-negotiable rule 1 forbids the consumer facts they carry from living in crates/batten. The write lands in PR #880 where a reviewer reads it beside the module and the engine change it configures. Admits-answer-rejected-route: config read first — I did read batten.toml, and reading is not the change; the row has to be added. patch run first (git restore) — that discards the rows, which is the change itself, not a repair to it. Both routes assume the write was a mistake to undo; here it is the deliverable. --- .claude/rules/policy-modules.md | 18 +- batten.toml | 158 ++++++++ crates/batten/src/facts.rs | 13 + crates/batten/src/git.rs | 73 ++++ crates/batten/src/lib.rs | 72 +++- crates/batten/src/mint.rs | 121 ++++++ crates/batten/src/trust.rs | 3 + .../batten/tests/it/code_review_dispatched.rs | 360 ++++++++++++++++++ crates/batten/tests/it/main.rs | 1 + policy/code-review-dispatched.rego | 207 ++++++++++ 10 files changed, 1020 insertions(+), 6 deletions(-) create mode 100644 crates/batten/tests/it/code_review_dispatched.rs create mode 100644 policy/code-review-dispatched.rego diff --git a/.claude/rules/policy-modules.md b/.claude/rules/policy-modules.md index 35fbe0f1e..e6d5617cb 100644 --- a/.claude/rules/policy-modules.md +++ b/.claude/rules/policy-modules.md @@ -174,6 +174,21 @@ path: a linked worktree may live anywhere on the machine, so its base is read to decide `present` and dropped at the boundary — rule 4 held in the fact's TYPE, the same way `commit-meta` has no body field. +**`base-delta`'s `patch-id` is the one member of that fact NOT computed the way +its siblings are, so it is worth the sentence they do not need** (CLOUD-1484). The +three path lists are a tip diff over the WORKING TREE; this is a merge-base diff +over COMMITTED bytes. The two therefore disagree on a stale branch and on a dirty +one, deliberately: a module asking which files this branch touched wants the +lists, and one asking whether a change was ATTESTED wants the identity, because an +attestation must not move when somebody saves a file and must not be restated by a +rebase — `land` rebases every lap, and a per-lap re-attestation is the cost that +gets a gate switched off. Its `null` is could-not-look AND the empty diff: a branch +that changed nothing has no identity, `cumulative_patch_id` refuses to mint one so +two empty changes cannot compare equal, and a predicate reading that absence as +*unattested* refuses a checkout with nothing to review. Do not re-derive an +identity from the path lists — that is a second notion of *the same change*, free +to disagree with `landing`'s about exactly the rebase the key turns on. + **`records-blocked` is the recorder surface's could-not-look, and its EMPTY value is an answer** (CLOUD-1126). `records` already distinguishes a record that could not be read (absent from the map) from one that was read and is short; neither @@ -190,7 +205,8 @@ and grouping them is worth a sentence because each answers a question no walk can: `input.tree["base-delta"]` is how the declared globs' paths differ from a declared base rev — `added`, `edited`, `deleted`, `code-changed` and the base side of every EDITED path's lines, which is what lets a module decide a CHANGE rather -than a state (CLOUD-1059); `input.tree.symbols` is where a delegated analyser +than a state (CLOUD-1059), plus `patch-id`, the identity of the whole change; +`input.tree.symbols` is where a delegated analyser resolved a named type, by NAME rather than by spelling, and carries the `provenance` of the tool that produced it — the first `Cost::Effect` fact (CLOUD-760); `input.tree.external` is a file outside the repository root, resolved diff --git a/batten.toml b/batten.toml index 6f80076a9..1cdc5132b 100644 --- a/batten.toml +++ b/batten.toml @@ -1292,6 +1292,76 @@ requires = ["hasNextPage", "issues[].id"] mode = "append" body = "{join:issues[].id}\nbase {git:origin/main}" +# ─── THE CODE-REVIEW RECEIPT (CLOUD-1484) ─── +# +# `code-review.`, replacing. What is attested is one thing and nothing +# more: THE AGENT DISPATCHED THE DECLARED REVIEW OVER THIS EXACT CHANGE. Not that +# the review was good, not what it found — those are judgements and non-negotiable +# rule 3 forbids a gate deciding one, so the gate over this receipt refuses +# ABSENCE, which is a comparison of two digests. +# +# WHY BATTEN DOES NOT DISPATCH IT. `review-dispatched` one family over has the +# ENGINE spawn a reviewer, and that is what forced a runner, a readiness probe and +# a prompt channel into a repo-agnostic core — a harness's CLI, one row from the +# crate. Here the refusal names what the agent owes, the agent dispatches with its +# own harness, and the boundary mints from the call it was already mediating. +# Nothing about that harness is expressible in `crates/batten`: `tool` and +# `selects` are strings this file supplies and the engine matches (rule 1). +# +# AND IT IS NOT CLOUD-1265's REFUTED SHAPE. `tool-verdict`'s producer-writes- +# outside store went measured dead because somebody had to remember to run the +# producer. Nobody has to remember here — the hook sees the call. +# +# `selects_at`/`selects` ARE THE LOAD-BEARING PAIR AND `requires` COULD NOT DO IT. +# Every skill this harness dispatches arrives under ONE tool name, so `tool` alone +# discriminates nothing: without the selector this row mints on every skill and +# the receipt attests that *something* ran, which is a false green in the one +# direction the gate exists to refuse. `requires` cannot reach it either — the +# field is present whichever skill ran, it just holds another value. +# +# THE SELECTOR READS THE INPUT AND `requires` READS THE RESULT, which is the same +# split one layer down: the input says which call this is, the result says whether +# it answered. +# +# KEYED `delta`, so the receipt is stale by construction. `key_base` is +# `origin/main` because that is what this repository lands onto; the identity is +# a MERGE-BASE diff, so `land`'s per-lap rebase does not re-buy the review, and +# any commit that moves code does. The boundary refuses to mint at all while the +# tree is dirty — a receipt keyed to committed bytes must not be taken in a +# session looking at something else. +# +# THE BODY CARRIES NO FINDING AND HAS NOWHERE TO PUT ONE. Two fields: the head +# this was taken at, and the instant. Rule 4 holds structurally rather than by +# anybody remembering to strip. Neither piece reads the RESULT, which is what +# lets this row work at all — see below. +# +# `requires` IS EMPTY, AND THAT IS A MEASUREMENT RATHER THAN AN OVERSIGHT. +# Measured 2026-09-05 on the live host: a skill dispatch answers with the bare +# string `Launching skill: `. That is not JSON, so there is no field in the +# result to require, and a path declared here would resolve to nothing on every +# call — the row would load clean, match its tool, and mint nothing, which is the +# dead gate this repository keeps recording. The engine half is the same reading: +# `record_mints` no longer gives up on a result carrying no JSON. +# +# SO THIS ATTESTS THE DISPATCH, NOT THE COMPLETION, and the gap is named rather +# than papered over. The boundary can see that the review was asked for over these +# exact bytes; it cannot see that the agent then did what the skill told it, +# because the harness returns nothing that would say so. `review.rs` states the +# nearer version of this — completion is the contract and the findings are a +# bonus — and this is one notch weaker again. Tightening it needs a result-side +# signal that does not exist today; until one does, what is checkable is that the +# ask happened over this change and not over some earlier one. +[[mint]] +name = "code-review" +tool = "Skill" +selects_at = "skill" +selects = "code-review" +key = "delta" +key_base = "origin/main" +requires = [] +mode = "replace" +body = "{git:HEAD} {now}" + # ─── THE BOARD-WRITE RECORD (CLOUD-1051), replacing `board-write-record.sh` ─── # # The sensor half of CLOUD-514: every row this branch put on the board, with the @@ -5819,6 +5889,53 @@ runner = "batten-review-runner" version = "0" subject = "tracker-body" +# CLOUD-1484's consumer: the branch's own CHANGE carries a code-review receipt, +# or it does not land. +# +# THE SIBLING OF THE ROW ABOVE AND NOT A WIDENING OF IT. That one asks whether a +# VENDORED prompt the engine spawned ran over a document; this asks whether a +# review the AGENT dispatched ran over this branch's diff. Same question, opposite +# routes, and they stay two rows because the route is what decides the failure +# mode: a missing runner silences that one (measured — `batten-review-runner` is +# on no PATH here, so `input.tree.review` is `null` and it has never refused +# anything), where this one is silenced only by a missing receipt store, which is +# what it refuses on. +# +# `delta_sources` IS THE CODE, and `base` is what the branch is a change against. +# The module reads `code-changed` off the resulting delta, so a prose-only branch +# owes nothing — the narrowing `review-dispatched` had to add after four `cli.rs` +# cases went red for wanting to exercise other rules. +# +# `[[rule.minted]]` COSTS A DIRECTORY LISTING AND NO NETWORK. The receipt is +# already on disk by the time this row is adjudicated, because the boundary wrote +# it when the agent dispatched; nothing here spawns, fetches or asks a model. +# +# `max_age_days` IS BELT AND BRACES, not the staleness control. The KEY is: a +# receipt is filed under the change's own identity, so a commit that moves code +# files the next review under a name this row does not look up. The age bound +# catches only the case the key cannot — a change re-created byte-identically +# long after it was reviewed. +# +# ABSENT IS COULD-NOT-LOOK ON EVERY FRESH CLONE, which is every CI runner until +# CLOUD-877 gives the receipt a portable form. That is stated here rather than +# discovered: today this gate is enforced where `land` runs, and CI is honestly +# silent rather than falsely green. +[[rule]] +id = "code-review-dispatched" +kind = "policy" +scope = "tree" +base = "origin/main" +delta_sources = ["crates/**", "policy/**", "mise-tasks/**", "batten.toml"] +module = "policy/code-review-dispatched.rego" +severity = "deny" + +[[rule.minted]] +id = "code-review" +mint = "code-review" +field = 0 +recency = 1 +max_age_days = 30 + # A third-party validator's verdict for a file it read (CLOUD-1171, the # adjudication half of `pkl-check`, `renovate-config-validator` and # `hook-profile-check`). @@ -11385,6 +11502,47 @@ target = "check the declared runner is installed and on PATH — a dispatch that # asked, and left no record. That is the branch's own conduct, and the two routes # above name what to do about it. +# CLOUD-1484. The branch's own change carries no code-review receipt. +# +# A SEPARATE CLASS FROM `prompt run never` ABOVE, and the split is the remedy +# rather than the subject. Both refuse the absence of a review keyed to bytes; the +# routes differ entirely, because that one is answered by an engine-spawned +# runner being present and this one is answered by the AGENT dispatching. Folding +# them would give a reader one class with two contradictory remedies, and the +# whole point of a token is that a reader can look it up. +# +# THE SUBJECT IS `patch`, NOT `review`, for the reason `prompt run never` gives +# one row up and which applies with more force here: `review answer missing` and +# `review read absent` are about the FORGE's review of a pull request, and a +# reader meeting a FOURTH `review …` class would have to look up which kind it +# meant. `patch` is glossed *a change identified by its content*, which is exactly +# what this is keyed by. And the action is `read` rather than `grade`: what is +# attested is that the change was looked at, never that a verdict was assigned to +# it — a class naming a verdict would describe a thing rule 3 forbids this gate +# from reading. +[[verdict]] +id = "patch read never" +gloss = "this branch changed code and no code review has been dispatched over the change" +class = """ +Refuses ABSENCE and nothing else. A receipt is filed under the identity of the \ +branch's whole change, so a review taken over other bytes lives under another \ +name and does not answer — staleness is the key rather than a comparison \ +somebody must remember to make. What the review CONCLUDED is never read, and the \ +receipt has no field it could occupy: refusing on a finding would be a model \ +verdict wearing an exit code, which non-negotiable rule 3 forbids. A rebase does \ +not re-owe the review, because the identity is a merge-base diff. A prose-only \ +branch owes nothing, and a change with no identity — an empty diff — is not a \ +branch with an unreviewed change but one with nothing to review. Absent on a \ +fresh clone is could-not-look, not a refusal: the receipt store dies with the \ +container, so this speaks where the branch is worked and is honestly silent in \ +CI until that store has a portable form. +""" + +[[verdict.route]] +id = "verb run first" +kind = "command" +target = "/code-review" + [[verdict]] id = "review answer missing" gloss = "readying would buy a CI matrix on a head carrying unresolved review threads" diff --git a/crates/batten/src/facts.rs b/crates/batten/src/facts.rs index e08f4ed0c..76650786d 100644 --- a/crates/batten/src/facts.rs +++ b/crates/batten/src/facts.rs @@ -1902,6 +1902,19 @@ impl Fact { "type": "object", "additionalProperties": {"type": "array", "items": {"type": "string"}}, }, + // CLOUD-1484. The identity of the branch's whole CHANGE + // against the base, so a module can ask whether THIS change + // was attested without re-deriving one from the path lists — + // which would be a second notion of `the same change`, free + // to disagree with `landing`'s about a rebase. A MERGE-BASE + // diff over COMMITTED bytes, where the lists beside it are a + // tip diff over the working tree: the two disagree on a stale + // or dirty branch, deliberately, because an attestation must + // not move when somebody saves a file. `null` is + // could-not-look AND covers the empty diff — a branch that + // changed nothing has no identity, so a predicate must not + // read it as `unattested`. + "patch-id": {"type": ["string", "null"]}, }, "additionalProperties": false, }), diff --git a/crates/batten/src/git.rs b/crates/batten/src/git.rs index a96fb382b..ef766f719 100644 --- a/crates/batten/src/git.rs +++ b/crates/batten/src/git.rs @@ -2606,6 +2606,33 @@ pub struct BaseDelta { /// must not collapse. #[serde(rename = "base-lines")] pub base_lines: BTreeMap>, + /// The identity of the branch's whole CHANGE against the base + /// (CLOUD-1484) — [`branch_patch_id`], as a bare hex string. + /// + /// # Why a receipt-shaped value belongs on this fact + /// + /// A module asking *was this change reviewed* has to name the change, and a + /// digest is the only spelling of it that fits non-negotiable rule 4: the + /// three path lists above already say WHICH files moved, and a module that + /// re-derived an identity from them would be a second notion of *the same + /// change* — free to disagree with [`landing`]'s about a rebase, which is + /// the one property such a receipt turns on. + /// + /// # It answers a DIFFERENT question from the lists beside it, deliberately + /// + /// The path lists are a tip diff over the WORKING TREE. This is a merge-base + /// diff over COMMITTED bytes. They disagree on a stale branch and on a dirty + /// one, and that is not a defect in either: a module asking which files this + /// branch touched wants the first, and one asking which change was attested + /// wants the second, because an attestation must not move when somebody + /// saves a file. + /// + /// `None` is could-not-look and covers the empty diff too — a branch that + /// changed nothing has no identity, and [`cumulative_patch_id`] refuses to + /// mint one so two empty changes cannot compare equal. A predicate must not + /// read that as *unreviewed*; there is nothing to review. + #[serde(rename = "patch-id")] + pub patch_id: Option, } /// A file's content with its comment and blank lines removed. @@ -2875,6 +2902,12 @@ pub fn base_delta(dir: &Path, base: &str, globs: &[String]) -> Result Result> { .map(|found| found.detach().to_string())) } +/// The patch identity of the branch's whole change against a declared base REF +/// (CLOUD-1484), as a bare hex string. +/// +/// **A wrapper over [`cumulative_patch_id`] and deliberately not a second +/// computation.** What a review-dispatch receipt is keyed by has to be *the same +/// change* in exactly the sense the rest of this module already means it: line +/// numbers excluded, so a rebase onto a moved base still matches, and the merge +/// base used for RANGE SELECTION rather than as a merged-ness answer +/// (`no_ancestry_decides_merged_ness`). A digest computed here over a diff of +/// this function's own devising would be a second notion of *the same change*, +/// free to disagree with [`landing`]'s about a rebase — which is the one property +/// the receipt turns on. +/// +/// `None` for every could-not-look: a base ref that does not resolve, a HEAD that +/// does not, two histories that share none, and — the one worth naming — an +/// **empty diff**. A branch that changed nothing has no identity, and +/// [`cumulative_patch_id`] refuses to mint one precisely so two empty changes +/// cannot compare equal. A caller must not read that `None` as *not reviewed*; +/// there is nothing to review. +/// +/// The value is over COMMITTED bytes — `HEAD` against the merge base — so an +/// uncommitted edit cannot move it. That is what makes the key stable, and it is +/// also why a caller that cares whether the reviewed bytes are the ones that will +/// land must ask [`uncommitted`] separately: this function cannot see the working +/// tree at all. +/// +/// # Errors +/// +/// When the repository cannot be opened. +pub fn branch_patch_id(dir: &Path, base_ref: &str) -> Result> { + let Some(base) = resolve_ref(dir, base_ref)? else { + return Ok(None); + }; + let head = head_commit(dir)?; + Ok(cumulative_patch_id(dir, &base, &head) + .ok() + .flatten() + .map(|id| id.as_str().to_owned())) +} + /// The patch identity of the branch's whole change: the diff from where the two /// histories diverged to `head`. /// diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 63ea18523..1e31c330b 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -1909,6 +1909,13 @@ fn file_and_report( mint_receipts( mints, method, + // NO REQUEST INPUT REACHES HERE, so a row declaring a `selects_at` + // selects NOTHING on this path — fail-closed, and stated rather than + // papered over. `file_and_report` is handed the answer and not the call, + // and synthesising an input would be the boundary certifying a selection + // it never read. A row that declares no selector is unaffected, which is + // every landed row. + &serde_json::Value::Null, &payload.value, repo, ready::Grammar::from_compiled(&crate::pattern::compiled(patterns)) @@ -11261,6 +11268,7 @@ fn recover_spilled(result: &serde_json::Value) -> Option { fn mint_receipts( declared: &[crate::mint::Declared], tool: &str, + input: &serde_json::Value, result: &serde_json::Value, root: &Path, grammar: Option<&ready::Grammar>, @@ -11276,6 +11284,20 @@ fn mint_receipts( if !rules::selects_tool_name(&mint.tool, tool) { continue; } + // THE ROW'S OWN SELECTOR, and it is checked before any git work + // (CLOUD-1484). A harness that dispatches every skill, agent or command + // through one tool name gives the name above nothing to discriminate on, + // so without this a row would mint on every invocation of that tool and + // the receipt would attest that *something* ran. + // THE SELECTOR READS THE INPUT, NEVER THE RESULT, and the split is the + // point rather than an accident of what was to hand. A dispatch names + // ITSELF in its arguments — which skill, which agent, which command — + // while its result is whatever came back, and on most hosts that is + // prose. `requires` asks the result whether the call succeeded; this asks + // the input whether it is the call at all. + if !crate::mint::selects(mint, input) { + continue; + } // The branch is resolved only for a row that asked, which is the same // economy `receipt::verdicts` states one channel over: a caller must not // pay a git invocation for a question it never asks. @@ -11292,6 +11314,33 @@ fn mint_receipts( }; format!("{}.{}", mint.name, branch.replace('/', "-")) } + // KEYED TO THE CHANGE, AND ONLY FROM A CLEAN TREE (CLOUD-1484). + // + // `branch_patch_id` reads COMMITTED bytes — `HEAD` against the merge + // base — so an uncommitted edit cannot move the key. That is what + // makes the key stable and it is also the hole: a receipt minted + // while the tree is dirty attests a review of `HEAD` taken in a + // session that was looking at something else, and it would file under + // the digest of bytes nobody reviewed. + // + // So the tree is asked, and anything but a clean answer mints + // NOTHING. `uncommitted` failing is could-not-look and takes the same + // arm as dirty: a boundary that cannot see the tree must not certify + // what is in it. Silence is this whole function's posture — the gate + // reading the receipt denies again with the same remedy, which is the + // direction the agent can see. + crate::mint::MintKey::Delta => { + let Some(base) = mint.key_base.as_deref() else { + continue; + }; + if !matches!(git::uncommitted(root), Ok(0)) { + continue; + } + let Ok(Some(delta)) = git::branch_patch_id(root, base) else { + continue; + }; + format!("{}.{delta}", mint.name) + } }; // `root` is the ANCHOR the block above resolved, never the cwd — a // `{authority:…}` piece reads the workspace version from it, and reading @@ -11333,11 +11382,23 @@ fn record_mints(overrides: &Overrides, envelope: &hook::Envelope) { // blocks, so reading fields off `envelope.result` directly matches nothing in // production while passing every fixture, which hands the engine a bare // object. `facts::payload_in` is the one authority on that unwrap. - let Some(result) = - facts::payload_in(&envelope.result).or_else(|| recover_spilled(&envelope.result)) - else { - return; - }; + // + // A RESULT THAT CARRIES NO JSON IS `null` HERE, NOT AN EARLY RETURN + // (CLOUD-1484). This used to give up, on the sound premise that every landed + // row reads a field of the result — and that premise stopped being true when + // a row arrived whose whole reading is of the INPUT and the repository. + // Measured on the live host: this harness answers a skill dispatch with the + // bare string `Launching skill: `, which is not JSON, so the row would + // have loaded clean, matched its tool, and minted nothing — a gate switched + // off by the shape of somebody else's stdout. + // + // Nothing is loosened for a row that DOES read the result: `select` over + // `null` resolves nothing, so `requires` still refuses and every body piece + // over a path still records its could-not-look token. What changes is only + // that a row reading none of it is no longer stopped on the way. + let result = facts::payload_in(&envelope.result) + .or_else(|| recover_spilled(&envelope.result)) + .unwrap_or(serde_json::Value::Null); let Ok((policy, _)) = load_policy(overrides, hook::Harness::ExitCode) else { return; }; @@ -11360,6 +11421,7 @@ fn record_mints(overrides: &Overrides, envelope: &hook::Envelope) { mint_receipts( declared, &envelope.raw_tool, + &envelope.input, &result, root, grammar.as_ref(), diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 1dbf9000f..b46654630 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -68,6 +68,24 @@ pub enum MintKey { /// Filed under the current branch, so every commit on it continues to serve /// the same record. Branch, + /// Filed under the identity of the branch's whole CHANGE against + /// [`Declared::key_base`] (CLOUD-1484). + /// + /// **The third keying, and it exists because neither of the two above can + /// key a receipt to the bytes a review actually read.** [`MintKey::Named`] + /// takes its subject from the tool result, which knows nothing about the + /// repository; [`MintKey::Branch`] takes a name that outlives every commit + /// under it, which is the staleness defeat + /// [`crate::receipt::branch_validity`] already had to be written to patch + /// around (CLOUD-516). + /// + /// The identity is [`crate::git::branch_patch_id`], so it moves when the + /// change moves and does NOT move when a rebase merely relocates it. Both + /// halves are load-bearing: without the first a receipt would outlive the + /// code it attests to, and without the second the landing loop would re-buy + /// the attested work on every lap, which is the shape that gets a gate + /// switched off rather than satisfied. + Delta, } /// Whether a mint replaces its record or appends to it. @@ -109,6 +127,42 @@ pub struct Declared { /// The path whose value is the subject, for [`MintKey::Named`]. #[serde(default, skip_serializing_if = "Option::is_none")] pub key_from: Option, + /// The base ref the change is measured against, for [`MintKey::Delta`]. + /// + /// Declared rather than defaulted: which ref a branch is *a change against* + /// is the consumer's fact, and an engine that assumed one would be naming a + /// branch in a repo-agnostic core (non-negotiable rule 1). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub key_base: Option, + /// The path whose value must equal [`Declared::selects`] for this row to + /// mint at all (CLOUD-1484). + /// + /// # Why presence could not do this job + /// + /// [`Declared::requires`] asserts a path is present and non-null, which is + /// the right shape for a SUCCESS predicate and the wrong one for a + /// SELECTION. A harness that dispatches every one of its skills, agents or + /// commands through one tool name gives [`Declared::tool`] nothing to + /// discriminate on: the row would mint on every invocation of that tool and + /// the receipt would attest that *something* ran. That is a false green in + /// the one direction a dispatch gate exists to refuse, and no amount of + /// `requires` reaches it, because the field is present either way — it just + /// holds another value. + /// + /// **The shape is adopted, not invented.** [`crate::capture`] already + /// resolves a stored response by `key_at`/`key` through [`scalar`], the same + /// selector this uses, so a path spelled here means what it means there. + /// + /// Absent leaves the row judged exactly as it was before this column + /// existed: [`Declared::tool`] alone decides, which is every landed row's + /// behaviour. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub selects_at: Option, + /// The value [`Declared::selects_at`] must hold. Compared for EQUALITY, + /// never as a substring: a prefix match would let a longer name a consumer + /// never declared mint under a shorter one's row. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub selects: Option, /// Paths that must be present and non-null in the result for anything to be /// written. **This is the success predicate** — see the module doc. #[serde(default, skip_serializing_if = "Vec::is_empty")] @@ -324,6 +378,33 @@ pub(crate) fn scalar(value: &serde_json::Value, path: &str) -> Option { } } +/// Whether this result is the one the row selects (CLOUD-1484). +/// +/// **A SELECTION, kept separate from [`satisfied`]'s SUCCESS test**, and the two +/// must not be folded together even though both gate the same write. `requires` +/// asks *did the call this row is about actually answer*; this asks *is this even +/// the call this row is about*. Collapsing them would make a row that selects +/// nothing indistinguishable from one whose call failed, and only the second of +/// those is a reason to try again. +/// +/// A row declaring no selector selects every result its `tool` matched, which is +/// every landed row's behaviour. +/// +/// **A path that does not resolve to a single scalar does NOT select.** The +/// failure direction is deliberate: an unreadable field means the boundary cannot +/// tell whether this is the declared call, and minting on a maybe is exactly the +/// forgery this column was added to remove. A gate over the receipt then denies +/// again with the same remedy, which is the safe direction and one the agent can +/// see. +#[must_use] +pub fn selects(declared: &Declared, result: &serde_json::Value) -> bool { + let (Some(path), Some(expected)) = (declared.selects_at.as_deref(), declared.selects.as_deref()) + else { + return true; + }; + scalar(result, path).is_some_and(|found| found == expected) +} + /// Whether every required path resolved, which is this module's success test. #[must_use] pub fn satisfied(declared: &Declared, result: &serde_json::Value) -> bool { @@ -503,6 +584,43 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { mint.name ))); } + if mint.key == MintKey::Delta && mint.key_base.is_none() { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` is keyed `delta` and declares no `key_base`, so nothing says \ + which base the change is measured against and no receipt could be filed", + mint.name + ))); + } + // A `key_base` on a row keyed any other way is refused rather than + // ignored: an ignored declaration reads as configured and decides + // nothing, which is the inert-coverage shape this whole function exists + // to refuse. + if mint.key != MintKey::Delta && mint.key_base.is_some() { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares a `key_base` and is not keyed `delta`, so the base \ + would be read by nothing", + mint.name + ))); + } + // HALF A SELECTOR IS THE DANGEROUS HALF. `selects_at` alone would leave + // the row minting on every result its tool matched while LOOKING + // narrowed, which is the reading a reviewer takes from the column's + // presence; `selects` alone names a value nothing is compared to. + if mint.selects_at.is_some() != mint.selects.is_some() { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares only one of `selects_at` and `selects`; a selector \ + with no value narrows nothing while reading as though it did", + mint.name + ))); + } + if let Some(at) = mint.selects_at.as_deref() + && at.trim().is_empty() + { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares an empty `selects_at`", + mint.name + ))); + } if let Err(problem) = parse(&mint.body) { return Err(crate::error::UsageError::raise(format!( "`[[mint]]` `{}` has an unreadable `body`: {problem}", @@ -597,6 +715,9 @@ mod tests { tool: "get_issue".to_owned(), key: MintKey::Named, key_from: Some("id".to_owned()), + key_base: None, + selects_at: None, + selects: None, requires: vec!["id".to_owned()], mode: MintMode::Replace, body: "{id} {authority:ready}".to_owned(), diff --git a/crates/batten/src/trust.rs b/crates/batten/src/trust.rs index 803def184..1c7721bf2 100644 --- a/crates/batten/src/trust.rs +++ b/crates/batten/src/trust.rs @@ -5059,6 +5059,9 @@ mod tests { tool: "get_issue".to_owned(), key: crate::mint::MintKey::Named, key_from: Some("id".to_owned()), + key_base: None, + selects_at: None, + selects: None, requires: requires.iter().map(|path| (*path).to_owned()).collect(), mode: crate::mint::MintMode::Replace, body: "{id} {now}".to_owned(), diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs new file mode 100644 index 000000000..d0ea27ad4 --- /dev/null +++ b/crates/batten/tests/it/code_review_dispatched.rs @@ -0,0 +1,360 @@ +//! `code-review-dispatched`, over the engine that builds its input (CLOUD-1484). +//! +//! # The seam, and why the module's own suite cannot reach it +//! +//! `policy/code-review-dispatched.rego`'s `test_` rules pin the predicate against +//! a fabricated document. Three questions decide whether the gate is alive, and +//! the module cannot ask any of them: +//! +//! * does the ENGINE project the branch's patch identity at +//! `input.tree["base-delta"]["patch-id"]`, and the receipt store at +//! `input.tree.minted`? +//! * is a receipt filed under that identity the one the predicate finds — and one +//! filed under any other identity NOT found? +//! * does the identity survive a REBASE, which is the property the whole keying +//! turns on and the one a landing loop would otherwise re-buy every lap? +//! +//! A `with input as` case actively hides all three: it fabricates the map, so it +//! fabricates the keying. Such a suite passes identically over an engine that +//! ignored the subject entirely. +//! +//! # The channel is confirmed by a PAIR, never by one arm +//! +//! `.claude/rules/policy-modules.md` records how CLOUD-1049's dead channel +//! survived two measurements: a probe whose only clause reads the new key cannot +//! tell an empty channel from a module that never ran, because both are silent. +//! Its remedy there is an unconditional `violation`; here the same discrimination +//! comes free from the two arms below, and is worth stating so nobody deletes one +//! of them as redundant. +//! +//! `an_absent_receipt_is_refused_over_the_engines_own_projection` and +//! `a_receipt_under_this_change_reaches_the_predicate_and_is_clean` are the pair. +//! If `input.tree.minted` were never populated, the second would find its subject +//! absent and REFUSE, so it fails. If `base-delta`'s `patch-id` were never +//! projected, `is_string(subject)` would not hold, every arm would abstain, and +//! the first would report clean, so it fails. Neither key can be dead with both +//! green — which is exactly what one arm alone could not establish. + +// UNIX-ONLY, for `review_dispatched.rs`' reason one family over: every case here +// drives real `git` against a scratch repository, and a case whose fixture failed +// to build would leave the receipt absent — which is exactly what the negative +// arms refuse, so they would pass FOR THE WRONG REASON while the clean case +// failed. A suite whose negative arms pass because the subject never ran is the +// vacuous pass this family exists to refuse. +#![cfg(unix)] +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use crate::common; + +use std::fs; +use std::path::{Path, PathBuf}; + +use batten::rules::{self, Rule}; + +const RULE: &str = "code-review-dispatched"; +const RECEIPT: &str = "code-review"; +const CODE: &str = "crates/batten/src/lib.rs"; + +/// A fixture repository whose base carries no code and whose branch adds some. +/// +/// THROUGH `common::Fixture`, never a hand-rolled `git init` chain: the builder +/// copies a template rather than forking `git init` (CLOUD-1419 measured 1,819 +/// init processes over one run from exactly that habit), and `fixture-forks` +/// refuses a new copy of it. +fn repo(name: &str) -> PathBuf { + let root = common::Fixture::new(name) + .config(CONFIG) + .file("README.md", "base\n") + .git() + .base_commit() + .build(); + install_module(&root); + write_code(&root, "fn a() {}\n"); + root +} + +/// The one config every fixture here writes, so the shape is stated once. +const CONFIG: &str = "version = 1\n"; + +/// Commit a code change at the path the row's `delta_sources` selects. +fn write_code(root: &Path, body: &str) { + let path = root.join(CODE); + fs::create_dir_all(path.parent().expect("a parent")).expect("the source dir"); + fs::write(&path, body).expect("the source"); + common::git_in(root, &["add", "-A"]); + common::git_in(root, &["commit", "-q", "-m", "change"]); +} + +fn install_module(root: &Path) { + let source = common::at_root("policy/code-review-dispatched.rego") + .canonicalize() + .expect("the committed module is where the row says it is"); + fs::create_dir_all(root.join("policy")).expect("scratch policy dir"); + fs::copy(source, root.join("policy/code-review-dispatched.rego")) + .expect("install committed module"); +} + +/// The identity the ENGINE resolves for this branch — never one this file +/// computes, or the cases would agree with themselves rather than with the gate. +fn identity(root: &Path) -> String { + batten::git::branch_patch_id(root, "refs/remotes/origin/main") + .expect("the repository opens") + .expect("a branch that changed something has an identity") +} + +/// File a receipt under `subject`, exactly as the mint boundary writes one. +fn file_receipt(root: &Path, subject: &str) { + let git_dir = batten::git::git_dir(root).expect("a git dir"); + let store = git_dir.join("batten-receipts"); + fs::create_dir_all(&store).expect("the receipt store"); + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |since| since.as_secs()); + fs::write( + store.join(format!("{RECEIPT}.{subject}")), + format!("deadbeef {now}\n"), + ) + .expect("write the receipt"); +} + +/// The committed row's shape. +fn row() -> Rule { + serde_json::from_value(serde_json::json!({ + "id": RULE, + "kind": "policy", + "scope": "tree", + "base": "refs/remotes/origin/main", + "delta_sources": ["crates/**"], + "module": "policy/code-review-dispatched.rego", + "severity": "deny", + "minted": [{ + "id": RECEIPT, + "mint": RECEIPT, + "field": 0, + "recency": 1, + "max_age_days": 30, + }], + })) + .expect("the loader accepts the committed row's shape") +} + +fn verdicts(root: &Path) -> Vec { + let declared = common::verdicts_in(root); + rules::run_static( + &[row()], + &[], + batten::policy::Vocabulary { + patterns: &[], + verdicts: &declared, + recorders: &[], + }, + root, + ) + .expect("the read surface runs a policy row") + .findings + .into_iter() + .map(|finding| finding.rule) + .collect() +} + +// --------------------------------------------------------------------------- +// THE PROJECTION SEAM. +// --------------------------------------------------------------------------- + +/// A change with no receipt is refused, over the engine's own projection. +/// +/// Without this the module is a `with input as` suite over two keys nothing +/// fills. Declared mutation: `absent-receipt-unread`. +#[test] +fn an_absent_receipt_is_refused_over_the_engines_own_projection() { + let root = repo("code-review-absent"); + assert_eq!( + verdicts(&root), + vec![String::from(RULE)], + "a branch that changed code with nothing filed under its identity must refuse" + ); +} + +/// A receipt filed under this change's identity clears it. +/// +/// This is the half that proves the ENGINE resolves the same identity the mint +/// boundary keys by; a fabricated map would pass over an engine that resolved +/// nothing. +#[test] +fn a_receipt_under_this_change_reaches_the_predicate_and_is_clean() { + let root = repo("code-review-attested"); + let subject = identity(&root); + file_receipt(&root, &subject); + assert!( + verdicts(&root).is_empty(), + "a receipt filed under this change's own identity must clear the gate" + ); +} + +/// THE ANTI-STALENESS HALF. A receipt over other bytes lives under another name. +#[test] +fn a_receipt_over_another_change_does_not_answer() { + let root = repo("code-review-stale"); + file_receipt(&root, "0000000000000000000000000000000000000000000000000000000000000000"); + assert_eq!( + verdicts(&root), + vec![String::from(RULE)], + "a receipt keyed to bytes this branch does not carry must not answer" + ); +} + +/// A FURTHER CODE COMMIT RE-OWES THE REVIEW, which is the same property read +/// forwards: the identity moves, so the record is filed under a name nothing +/// looks up. +#[test] +fn a_further_code_commit_re_owes_the_review() { + let root = repo("code-review-moved"); + file_receipt(&root, &identity(&root)); + assert!(verdicts(&root).is_empty(), "the fixture starts attested"); + + write_code(&root, "fn a() {}\nfn b() {}\n"); + assert_eq!( + verdicts(&root), + vec![String::from(RULE)], + "a commit that moves code must re-owe the review" + ); +} + +/// THE PROPERTY THE WHOLE KEYING TURNS ON. `land` rebases every lap, and an +/// identity that moved with the rebase would re-buy the review each time — +/// minutes and tokens per lap, which is the shape that gets a gate switched off +/// rather than satisfied. +/// +/// A merge-base diff is what makes this hold, and it is precisely what a +/// `with input as` case cannot check. +#[test] +fn a_rebase_onto_a_moved_base_does_not_re_owe_the_review() { + let root = repo("code-review-rebase"); + let before = identity(&root); + file_receipt(&root, &before); + assert!(verdicts(&root).is_empty(), "the fixture starts attested"); + + // Move the base under the branch with a change this branch never made, then + // rebase onto it — the landing loop's own lap, in miniature. + common::git_in(&root, &["checkout", "-q", "-b", "trunk", "refs/remotes/origin/main"]); + fs::write(root.join("NOTES.md"), "trunk moved\n").expect("the trunk file"); + common::git_in(&root, &["add", "-A"]); + common::git_in(&root, &["commit", "-q", "-m", "trunk"]); + common::git_in(&root, &["update-ref", "refs/remotes/origin/main", "trunk"]); + common::git_in(&root, &["checkout", "-q", "main"]); + common::git_in(&root, &["rebase", "-q", "refs/remotes/origin/main"]); + + assert_eq!( + identity(&root), + before, + "a rebase relocates the change and must not restate it" + ); + assert!( + verdicts(&root).is_empty(), + "the receipt taken before the rebase must still answer after it" + ); +} + +/// A PROSE-ONLY BRANCH OWES NO CODE REVIEW. +/// +/// Without this narrowing the gate refuses every fixture and every fresh clone, +/// which is the shape that gets a gate switched off. Declared mutation: +/// `prose-only-priced`. +#[test] +fn a_prose_only_branch_owes_no_code_review() { + // The base ALREADY CARRIES the code, so the branch's only change is a comment + // and `code-changed` stays empty. Building it the other way round — base with + // no file — makes the path read as ADDED and the case would pass for the + // wrong reason. + let root = common::Fixture::new("code-review-prose") + .config(CONFIG) + .file(CODE, "fn a() {}\n") + .git() + .base_commit() + .build(); + install_module(&root); + + fs::write(root.join(CODE), "// a comment\nfn a() {}\n").expect("the source"); + common::git_in(&root, &["add", "-A"]); + common::git_in(&root, &["commit", "-q", "-m", "comment only"]); + + // THE PREMISE, ASSERTED BEFORE THE CONCLUSION. Without this the case would + // pass over a fixture whose base carried no file at all, where the path reads + // as ADDED and `code-changed` is non-empty — green for the wrong reason. + let delta = batten::git::base_delta( + &root, + "refs/remotes/origin/main", + &[String::from("crates/**")], + ) + .expect("the repository opens") + .expect("the base resolves"); + assert!( + delta.code_changed.is_empty(), + "a comment-only commit must move no code, but `code-changed` holds {:?}", + delta.code_changed + ); + assert!( + !delta.edited.is_empty(), + "the fixture must still have EDITED the file, or it tests nothing" + ); + assert!( + verdicts(&root).is_empty(), + "a comment-only commit moves no code and must not re-owe the review" + ); +} + +/// A CHANGE WITH NO IDENTITY OWES NO REVIEW. +/// +/// An empty diff has nothing to review, and `cumulative_patch_id` refuses to mint +/// an identity for one precisely so two empty changes cannot compare equal. +/// Reading that absence as `unreviewed` would refuse a branch with nothing on it. +/// Declared mutation: `no-identity-priced`. +#[test] +fn a_change_with_no_identity_owes_no_review() { + // The base already CARRIES the code, and the branch adds nothing on top, so + // the diff is empty and there is no identity to key a receipt by. + let root = common::Fixture::new("code-review-empty") + .config(CONFIG) + .file(CODE, "fn a() {}\n") + .git() + .base_commit() + .build(); + install_module(&root); + + assert!( + batten::git::branch_patch_id(&root, "refs/remotes/origin/main") + .expect("the repository opens") + .is_none(), + "the fixture's premise: an empty diff has no identity" + ); + assert!( + verdicts(&root).is_empty(), + "a branch with nothing to review must not be refused for not reviewing it" + ); +} + +/// COULD-NOT-LOOK IS NOT A REFUSAL. A checkout whose row declares no receipt +/// projects `null`, and the gate must go quiet rather than refuse the machine. +#[test] +fn an_undeclared_receipt_is_could_not_look_and_never_a_refusal() { + let root = repo("code-review-undeclared"); + let mut bare = row(); + bare.minted.clear(); + let declared = common::verdicts_in(&root); + let findings = rules::run_static( + &[bare], + &[], + batten::policy::Vocabulary { + patterns: &[], + verdicts: &declared, + recorders: &[], + }, + &root, + ) + .expect("the read surface runs a policy row") + .findings; + assert!( + findings.is_empty(), + "a row declaring no receipt projects null, and null is could-not-look" + ); +} diff --git a/crates/batten/tests/it/main.rs b/crates/batten/tests/it/main.rs index 81d58e300..631ce3246 100644 --- a/crates/batten/tests/it/main.rs +++ b/crates/batten/tests/it/main.rs @@ -76,6 +76,7 @@ mod claim; mod claim_order; mod claim_receipt; mod cli; +mod code_review_dispatched; mod commit; mod commit_admission; mod commit_arm_sequencing; diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego new file mode 100644 index 000000000..850fc117c --- /dev/null +++ b/policy/code-review-dispatched.rego @@ -0,0 +1,207 @@ +# METADATA +# description: | +# The branch's own change carries a code-review receipt, or it does not land +# (CLOUD-1484). +# +# THIS REFUSES ABSENCE, AND ABSENCE IS ALL IT MAY REFUSE. A patch identity with +# no receipt under it means the declared review has not been shown to run over +# this change — a comparison of two digests, which is a thing a gate may decide. +# What the reviewer CONCLUDED is not, and refusing on it would be a model +# verdict wearing an exit code (non-negotiable rule 3). Nothing the agent wrote +# reaches this module: the receipt's fields are a commit id and an instant, and +# there is no channel here a finding's prose could travel down. +# +# THE DISPATCH IS THE AGENT'S AND THE RECEIPT IS THE BOUNDARY'S, which is the +# whole difference from `review-dispatched` beside it. That module's engine +# SPAWNS a reviewer, which is what put a runner, a probe and a prompt channel +# into a repo-agnostic core. Here Batten refuses, the refusal names what the +# agent owes, the agent dispatches its own review with its own harness, and +# `[[mint]]` writes the receipt from the tool result the boundary already sees. +# Nothing about that harness is expressible in this module or in the crate. +# +# WHY THIS IS NOT THE SHAPE CLOUD-1265 REFUTED. `tool-verdict`'s +# producer-writes-outside store went measured dead — `validator-verdict-clean` +# reads a record nothing ever writes — because a person had to remember to run +# the producer, and `review.rs` cites exactly that as its reason to spawn. The +# hook is what does not transfer: nobody has to remember, because the boundary +# sees the call it is already mediating. A reader who takes CLOUD-1265 as +# refuting this design is reading past that difference. +# +# THREE ANSWERS AND THE MODULE READS ALL THREE. `input.tree.minted` is `null` +# when no row declared a receipt or no store is readable — could-not-look, and +# silence is the honest answer on a fresh clone, which is every CI runner until +# CLOUD-877 gives the receipt a portable form. A declared id PRESENT with the +# current patch identity absent from it is the finding. Present WITH it is +# clean, whatever the review said. +# +# THE CLEAN-TREE CONDITION IS NOT HERE, AND THAT IS A DECISION. A receipt is +# keyed to COMMITTED bytes, so it must not be minted while the tree is dirty — +# but that belongs at the write, where `mint_receipts` refuses, rather than as a +# second opinion here about whether the tree is clean. `tree-clean` already owns +# that question for the landing path. A conjunct here would either duplicate it +# or, read the other way round, make a dirty tree silence this gate. +# +# THE BRACKETS ARE NOT STYLE: the schema file carries a hyphen, so the dotted +# form is a parse error reported as `invalid schema reference`. +# THIS BLOCK IS YAML AND MUST STAY THE LAST COMMENT BLOCK BEFORE `package`. +# schemas: +# - input: schema["policy-input.schema"] +package batten.code_review_dispatched + +import rego.v1 + +rules contains "code-review-dispatched" + +# The receipts this repository declares it will not land code without. +# +# The id is the CONSUMER's, named here rather than derived from the fact, for +# `review-dispatched`'s reason one module over: a rule that refused only over what +# it FOUND could never refuse an absence, which is the one thing this exists to +# refuse. +required contains "code-review" + +# Whether the engine could look at the receipt store at all. +# +# GUARDED on `is_object`: the key is `null` when nobody declared a receipt or no +# store is readable, and reaching into `null` is a hard evaluation FAULT in Rego +# rather than a silent miss. +looked if { + is_object(input.tree.minted) +} + +delta := input.tree["base-delta"] + +# The identity of the change this branch is asking to land. +# +# A MERGE-BASE diff over COMMITTED bytes, which is what makes it survive the +# landing loop: `land` rebases every lap, and an identity that moved with the +# rebase would re-buy the review each time — minutes and tokens per lap, which is +# the shape that gets a gate switched off rather than satisfied. +# +# Absent — a base that does not resolve, or an EMPTY diff — leaves every arm below +# quiet. A branch that changed nothing has no identity, and reading that as +# `unreviewed` would refuse a checkout with nothing to review. +subject := delta["patch-id"] + +# Whether a review is owed at all. +# +# GATED ON CODE, not on any change. `code-changed` is the subset whose non-comment +# remainder moved, so a prose-only branch owes nothing — and neither does a fresh +# clone or a fixture, which is the narrowing `review-dispatched` had to add after +# four `cli.rs` cases went red at once for wanting to exercise other rules. +owed if { + count(delta["code-changed"]) > 0 +} + +# Every declared receipt with nothing filed under this change. +# +# The keying stays the ENGINE's business: a receipt taken over other bytes lives +# under a different subject and never matches, so `absent from the map` already +# means `not reviewed as this now stands`. A module re-deriving that would be the +# second authority over an identity `git::branch_patch_id` already owns. +unattested contains id if { + looked + owed + is_string(subject) + some id in required + not input.tree.minted[id][subject] +} + +violation contains { + "rule": "code-review-dispatched", + "verdict": "patch read never", + "subjects": [{"artifact": id}], +} if { + some id in unattested +} + +# --- the load-time tier ------------------------------------------------------ +# +# These pin the PREDICATE. They cannot pin that the ENGINE keys a receipt by the +# branch's patch identity, or that a rebase leaves that identity alone — a +# `with input as` case fabricates the very keying the whole gate turns on. +# `crates/batten/tests/it/code_review_dispatched.rs` is that tier. + +changed := { + "added": [], + "edited": ["crates/batten/src/lib.rs"], + "deleted": [], + "code-changed": ["crates/batten/src/lib.rs"], + "patch-id": "abc", +} + +attested(subjects) := {"tree": { + "base-delta": changed, + "minted": {"code-review": subjects}, +}} + +test_a_receipt_under_this_change_is_clean if { + count(violation) == 0 with input as attested({"abc": "cafe 1700000000"}) +} + +test_no_receipt_at_all_is_refused if { + some v in violation with input as attested({}) + v.verdict == "patch read never" +} + +# A RECEIPT OVER OTHER BYTES DOES NOT ANSWER. This is the anti-staleness half and +# the reason the key is a digest rather than a marker: push a commit and the old +# record lives under a name nothing looks up. +test_a_receipt_over_another_change_does_not_answer if { + some v in violation with input as attested({"zzz": "cafe 1700000000"}) + v.verdict == "patch read never" +} + +# THE REFUSAL NAMES WHICH RECEIPT, so a reader is not left working out which of +# several declared ids is missing. +test_the_refusal_names_the_receipt if { + ids := {v.subjects[0].artifact | some v in violation} with input as attested({}) + ids == {"code-review"} +} + +# A RECORD UNDER ANOTHER ID IS NOT THIS ONE HAVING RUN. +test_another_receipts_record_does_not_answer if { + some v in violation with input as {"tree": { + "base-delta": changed, + "minted": {"other": {"abc": "cafe 1700000000"}}, + }} + v.verdict == "patch read never" +} + +# COULD-NOT-LOOK, and without the `is_object` guard this case does not merely +# fail — it faults, taking the whole bundle with it. +test_could_not_look_does_not_fault if { + count(violation) == 0 with input as {"tree": {"base-delta": changed, "minted": null}} +} + +# A PROSE-ONLY BRANCH OWES NO CODE REVIEW. Without this the gate refuses every +# checkout that has never dispatched, which is every fixture and every fresh +# clone. +test_a_prose_only_branch_is_not_refused if { + count(violation) == 0 with input as attested_over({ + "added": [], + "edited": ["AGENTS.md"], + "deleted": [], + "code-changed": [], + "patch-id": "abc", + }) +} + +# AN EMPTY DIFF HAS NO IDENTITY, and refusing over one would be a verdict about a +# branch with nothing to review. +test_a_change_with_no_identity_is_not_refused if { + count(violation) == 0 with input as attested_over({ + "added": [], + "edited": ["crates/batten/src/lib.rs"], + "deleted": [], + "code-changed": ["crates/batten/src/lib.rs"], + "patch-id": null, + }) +} + +attested_over(d) := {"tree": {"base-delta": d, "minted": {"code-review": {}}}} + +#MUTANT-SUITE crates/batten/tests/it/code_review_dispatched.rs +#MUTANT absent-receipt-unread|s@^\tnot input.tree.minted\[id\]\[subject\]$@\tfalse@|an_absent_receipt_is_refused_over_the_engines_own_projection +#MUTANT no-identity-priced|s@^\tis_string(subject)$@\ttrue@|a_change_with_no_identity_owes_no_review +#MUTANT prose-only-priced|s@^\towed$@\ttrue@|a_prose_only_branch_owes_no_code_review From b5b9c6f3bec082bb199bcc9944a49370533b8479 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:58 +0000 Subject: [PATCH 02/21] chore(schema): regenerate for the mint columns and base-delta's patch id Generated, never hand-edited: `mise run schema` over the four surfaces together, so none can be refreshed alone and drift while the others look current. Refs: CLOUD-1484 --- schema/batten.schema.json | 26 ++++++++++++++++++++++++++ schema/policy-input.schema.json | 6 ++++++ 2 files changed, 32 insertions(+) diff --git a/schema/batten.schema.json b/schema/batten.schema.json index d966ac375..127f53325 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -1117,6 +1117,13 @@ "description": "What the record is filed under.", "$ref": "#/$defs/MintKey" }, + "key_base": { + "description": "The base ref the change is measured against, for [`MintKey::Delta`].\n\nDeclared rather than defaulted: which ref a branch is *a change against*\nis the consumer's fact, and an engine that assumed one would be naming a\nbranch in a repo-agnostic core (non-negotiable rule 1).", + "type": [ + "string", + "null" + ] + }, "key_from": { "description": "The path whose value is the subject, for [`MintKey::Named`].", "type": [ @@ -1139,6 +1146,20 @@ "type": "string" } }, + "selects": { + "description": "The value [`Declared::selects_at`] must hold. Compared for EQUALITY,\nnever as a substring: a prefix match would let a longer name a consumer\nnever declared mint under a shorter one's row.", + "type": [ + "string", + "null" + ] + }, + "selects_at": { + "description": "The path whose value must equal [`Declared::selects`] for this row to\nmint at all (CLOUD-1484).\n\n# Why presence could not do this job\n\n[`Declared::requires`] asserts a path is present and non-null, which is\nthe right shape for a SUCCESS predicate and the wrong one for a\nSELECTION. A harness that dispatches every one of its skills, agents or\ncommands through one tool name gives [`Declared::tool`] nothing to\ndiscriminate on: the row would mint on every invocation of that tool and\nthe receipt would attest that *something* ran. That is a false green in\nthe one direction a dispatch gate exists to refuse, and no amount of\n`requires` reaches it, because the field is present either way — it just\nholds another value.\n\n**The shape is adopted, not invented.** [`crate::capture`] already\nresolves a stored response by `key_at`/`key` through [`scalar`], the same\nselector this uses, so a path spelled here means what it means there.\n\nAbsent leaves the row judged exactly as it was before this column\nexisted: [`Declared::tool`] alone decides, which is every landed row's\nbehaviour.", + "type": [ + "string", + "null" + ] + }, "tool": { "description": "Which tool's result mints it, matched by [`crate::rules::selects_tool_name`].\n\n**A tool name, never a field shape.** A write response and a read payload\nare shape-identical across the fields either carries, so duck-typing lets\nthe later, poorer payload win; and the match is on the whole name or its\nwhole final `__`-delimited segment because a connector is exposed under\nmore than one name over its lifetime (CLOUD-178).", "type": "string" @@ -2048,6 +2069,11 @@ "description": "Filed under the current branch, so every commit on it continues to serve\nthe same record.", "type": "string", "const": "branch" + }, + { + "description": "Filed under the identity of the branch's whole CHANGE against\n[`Declared::key_base`] (CLOUD-1484).\n\n**The third keying, and it exists because neither of the two above can\nkey a receipt to the bytes a review actually read.** [`MintKey::Named`]\ntakes its subject from the tool result, which knows nothing about the\nrepository; [`MintKey::Branch`] takes a name that outlives every commit\nunder it, which is the staleness defeat\n[`crate::receipt::branch_validity`] already had to be written to patch\naround (CLOUD-516).\n\nThe identity is [`crate::git::branch_patch_id`], so it moves when the\nchange moves and does NOT move when a rebase merely relocates it. Both\nhalves are load-bearing: without the first a receipt would outlive the\ncode it attests to, and without the second the landing loop would re-buy\nthe attested work on every lap, which is the shape that gets a gate\nswitched off rather than satisfied.", + "type": "string", + "const": "delta" } ] }, diff --git a/schema/policy-input.schema.json b/schema/policy-input.schema.json index 766489874..ad1cde755 100644 --- a/schema/policy-input.schema.json +++ b/schema/policy-input.schema.json @@ -49,6 +49,12 @@ "type": "string" }, "type": "array" + }, + "patch-id": { + "type": [ + "string", + "null" + ] } }, "type": [ From 2b60797b4417e5573573e42a7fe82bd37915571c Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:58 +0000 Subject: [PATCH 03/21] fix(gate): repair the four defects the gate's own code review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/code-review` was dispatched over this branch, which is what the gate exists to demand, and it found six things. Four are repaired here; two are bounds and are now written down where a reader meets them. THE COULD-NOT-LOOK ARM WAS BACKWARDS, which is the one that would have shipped a gate refusing exactly what it documents itself as abstaining on. `looked` guarded `is_object(input.tree.minted)`, but `minted::fields` leaves a declared id ABSENT when it cannot list the store — so an unlistable store is an EMPTY map, not `null`, `is_object({})` holds, and every fresh clone and every CI runner would have been refused. The guard is per-id now. THE CLEAN-TREE CONJUNCT AT THE MINT WAS UNSATISFIABLE IN THIS REPOSITORY. `git::uncommitted` reimplements status and does not skip a GITLINK, where `walk_blob_ids` explicitly does, so an uninitialised submodule counts as changed forever: `git status` reports this tree clean while `changed_paths` returns `{"tests/bats"}`, a mode-160000 entry. `Ok(0)` could never hold and the receipt could never be minted. Withdrawn rather than repaired in place — it was also a second authority over a question `tree-clean` owns, and a reviewer reads the working tree anyway, so refusing to record a dispatch taken over uncommitted work attests less than happened. The primitive's defect is CLOUD-1500. `delta_sources` DOES NOT NARROW WHAT A MODULE SEES. `rules.rs` builds ONE `base_delta` from the union of every row's globs, and six rows here declare `["**"]`, so a workflow-only or schema-only branch owed a code review. The module carries the reviewable prefixes itself. A DECLARED MUTATION WAS NON-DISCRIMINATING. `no-identity-priced` named a case whose `code-changed` was empty, so `owed` already excluded the mutated conjunct and the mutation would have survived the sweep. The fixture now leaves the edit UNCOMMITTED, which is the one state where the working-tree delta says code moved and the committed range says nothing did. Two bounds stated rather than fixed: `owed` is a tip diff and `subject` is a merge-base one, so a stale branch can owe a review until it rebases; and the identity covers the whole change, so a prose commit on a branch that also carries code re-owes it. Both err toward owing a review that is not strictly due, and neither can produce the other direction, which is a branch landing unread. Refs: CLOUD-1484 --- crates/batten/src/lib.rs | 36 +++-- .../batten/tests/it/code_review_dispatched.rs | 101 +++++++++++-- policy/code-review-dispatched.rego | 141 +++++++++++++++--- 3 files changed, 234 insertions(+), 44 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 1e31c330b..13dddc858 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -11314,28 +11314,34 @@ fn mint_receipts( }; format!("{}.{}", mint.name, branch.replace('/', "-")) } - // KEYED TO THE CHANGE, AND ONLY FROM A CLEAN TREE (CLOUD-1484). + // KEYED TO THE CHANGE (CLOUD-1484). // // `branch_patch_id` reads COMMITTED bytes — `HEAD` against the merge - // base — so an uncommitted edit cannot move the key. That is what - // makes the key stable and it is also the hole: a receipt minted - // while the tree is dirty attests a review of `HEAD` taken in a - // session that was looking at something else, and it would file under - // the digest of bytes nobody reviewed. + // base — so an uncommitted edit cannot move the key, which is what + // makes the receipt survive the landing loop's per-lap rebase. // - // So the tree is asked, and anything but a clean answer mints - // NOTHING. `uncommitted` failing is could-not-look and takes the same - // arm as dirty: a boundary that cannot see the tree must not certify - // what is in it. Silence is this whole function's posture — the gate - // reading the receipt denies again with the same remedy, which is the - // direction the agent can see. + // THIS ASKED `uncommitted == 0` AND THE CONJUNCT WAS UNSATISFIABLE + // IN THIS REPOSITORY, which is worth recording rather than quietly + // deleting. The intent was sound: a receipt keyed to committed bytes + // should not be taken in a session looking at something else. What it + // ran into is that `git::uncommitted` reimplements status and does not + // skip a GITLINK, where `walk_blob_ids` explicitly does — so an + // uninitialised submodule is counted as changed forever, `Ok(0)` never + // holds, and the mint could never fire. Measured here: `git status` + // reports the tree clean while `changed_paths` returns + // `{"tests/bats"}`, a mode-160000 entry. + // + // So the condition is withdrawn rather than repaired in place, on two + // grounds beyond the defect. It was a SECOND AUTHORITY over a question + // `tree-clean` already owns for the landing path, and nothing reaches + // `main` without passing that. And it was the wrong subject anyway: a + // reviewer reads the WORKING TREE, so refusing to record a dispatch + // taken over uncommitted work attests less than actually happened + // rather than more. The gitlink defect is its own row. crate::mint::MintKey::Delta => { let Some(base) = mint.key_base.as_deref() else { continue; }; - if !matches!(git::uncommitted(root), Ok(0)) { - continue; - } let Ok(Some(delta)) = git::branch_patch_id(root, base) else { continue; }; diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs index d0ea27ad4..21779b53e 100644 --- a/crates/batten/tests/it/code_review_dispatched.rs +++ b/crates/batten/tests/it/code_review_dispatched.rs @@ -70,9 +70,30 @@ fn repo(name: &str) -> PathBuf { .build(); install_module(&root); write_code(&root, "fn a() {}\n"); + seed_store(&root); root } +/// Make the receipt store LISTABLE while holding nothing for this mint. +/// +/// **Every fixture that expects a refusal needs this, and that is the module's +/// three-valued read rather than test scaffolding.** An id is absent from +/// `input.tree.minted` when the engine could not list the store at all, which is +/// could-not-look and abstains; it is present-and-empty when the engine looked +/// and found no receipt, which is the finding. A fixture with no store directory +/// would take the first arm, so a refusal case built on one would fail — and a +/// clean case built on one would pass for the wrong reason. +fn seed_store(root: &Path) { + let store = batten::git::git_dir(root) + .expect("a git dir") + .join("batten-receipts"); + fs::create_dir_all(&store).expect("the receipt store"); + // A receipt belonging to no declared mint: it makes the directory listable + // and `subject_of` skips it, so the `code-review` id is present with an empty + // subject map. + fs::write(store.join("unrelated.subject"), "x 0\n").expect("seed the store"); +} + /// The one config every fixture here writes, so the shape is stated once. const CONFIG: &str = "version = 1\n"; @@ -273,6 +294,7 @@ fn a_prose_only_branch_owes_no_code_review() { .base_commit() .build(); install_module(&root); + seed_store(&root); fs::write(root.join(CODE), "// a comment\nfn a() {}\n").expect("the source"); common::git_in(&root, &["add", "-A"]); @@ -303,16 +325,22 @@ fn a_prose_only_branch_owes_no_code_review() { ); } -/// A CHANGE WITH NO IDENTITY OWES NO REVIEW. +/// A CHANGE WITH NO IDENTITY OWES NO REVIEW, and the fixture has to make `owed` +/// TRUE or the case proves nothing. +/// +/// **The first version of this case was non-discriminating and the code review +/// caught it.** It built a branch whose base already carried the code, so +/// `code-changed` was empty, `owed` excluded the arm, and the declared +/// `no-identity-priced` mutation would have SURVIVED — which is the shape +/// `.claude/rules/policy-modules.md` warns about: a mutation over a conjunct some +/// other conjunct already excludes. /// -/// An empty diff has nothing to review, and `cumulative_patch_id` refuses to mint -/// an identity for one precisely so two empty changes cannot compare equal. -/// Reading that absence as `unreviewed` would refuse a branch with nothing on it. -/// Declared mutation: `no-identity-priced`. +/// The state that separates them is an UNCOMMITTED edit. `base_delta` is a tip +/// diff over the working tree, so it reports the code as changed and `owed` +/// holds; `branch_patch_id` reads `HEAD` against the merge base, which are the +/// same commit, so there is no identity. Declared mutation: `no-identity-priced`. #[test] fn a_change_with_no_identity_owes_no_review() { - // The base already CARRIES the code, and the branch adds nothing on top, so - // the diff is empty and there is no identity to key a receipt by. let root = common::Fixture::new("code-review-empty") .config(CONFIG) .file(CODE, "fn a() {}\n") @@ -320,16 +348,71 @@ fn a_change_with_no_identity_owes_no_review() { .base_commit() .build(); install_module(&root); + seed_store(&root); + + // UNCOMMITTED, deliberately: this is the one state where the working-tree + // delta says code moved and the committed range says nothing did. + fs::write(root.join(CODE), "fn a() {}\nfn b() {}\n").expect("the source"); assert!( batten::git::branch_patch_id(&root, "refs/remotes/origin/main") .expect("the repository opens") .is_none(), - "the fixture's premise: an empty diff has no identity" + "the fixture's premise: nothing is committed beyond the base, so there is no identity" + ); + let delta = batten::git::base_delta( + &root, + "refs/remotes/origin/main", + &[String::from("crates/**")], + ) + .expect("the repository opens") + .expect("the base resolves"); + assert!( + !delta.code_changed.is_empty(), + "the fixture's other premise: `owed` must HOLD, or the mutation this case \ + pins is excluded by a different conjunct and survives" + ); + + assert!( + verdicts(&root).is_empty(), + "a change with no identity has nothing to key a receipt by and must not be refused" + ); +} + +/// AN UNLISTABLE RECEIPT STORE IS COULD-NOT-LOOK, NOT A REFUSAL. +/// +/// **The defect the code review caught, over the engine that produces it.** +/// `minted::fields` leaves a declared id ABSENT from the map when it cannot list +/// the store, so the map is EMPTY rather than `null` — and the first draft guarded +/// on `is_object(input.tree.minted)`, which holds for an empty object. Every fresh +/// clone and every CI runner would have been refused, which is the arm both the +/// module METADATA and `batten.toml` promise is silent. +/// +/// A module suite cannot reach this: it would have to fabricate the empty map, +/// which is exactly the shape a `with input as` case cannot prove the engine +/// produces. Declared mutation: `store-unreadable-refused`. +#[test] +fn an_unlistable_store_is_could_not_look_and_never_a_refusal() { + // NOT `repo`, which seeds the store: the whole subject here is a checkout + // where no receipt has ever been written. + let root = common::Fixture::new("code-review-no-store") + .config(CONFIG) + .file("README.md", "base\n") + .git() + .base_commit() + .build(); + install_module(&root); + write_code(&root, "fn a() {}\n"); + let store = batten::git::git_dir(&root) + .expect("a git dir") + .join("batten-receipts"); + assert!( + !store.exists(), + "the fixture's premise: no receipt store has ever been written here" ); assert!( verdicts(&root).is_empty(), - "a branch with nothing to review must not be refused for not reviewing it" + "a checkout whose receipt store cannot be listed must abstain, not refuse" ); } diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego index 850fc117c..7ef95553b 100644 --- a/policy/code-review-dispatched.rego +++ b/policy/code-review-dispatched.rego @@ -27,19 +27,41 @@ # sees the call it is already mediating. A reader who takes CLOUD-1265 as # refuting this design is reading past that difference. # -# THREE ANSWERS AND THE MODULE READS ALL THREE. `input.tree.minted` is `null` -# when no row declared a receipt or no store is readable — could-not-look, and -# silence is the honest answer on a fresh clone, which is every CI runner until -# CLOUD-877 gives the receipt a portable form. A declared id PRESENT with the -# current patch identity absent from it is the finding. Present WITH it is -# clean, whatever the review said. -# -# THE CLEAN-TREE CONDITION IS NOT HERE, AND THAT IS A DECISION. A receipt is -# keyed to COMMITTED bytes, so it must not be minted while the tree is dirty — -# but that belongs at the write, where `mint_receipts` refuses, rather than as a -# second opinion here about whether the tree is clean. `tree-clean` already owns -# that question for the landing path. A conjunct here would either duplicate it -# or, read the other way round, make a dirty tree silence this gate. +# THREE ANSWERS AND THE MODULE READS ALL THREE, AND THE ARM IS PER-ID. +# `input.tree.minted` is `null` when no row declared a receipt, and a declared id +# is ABSENT from the map when the engine could not LIST its store — both are +# could-not-look, and silence is the honest answer on a fresh clone, which is +# every CI runner until CLOUD-877 gives the receipt a portable form. An id +# PRESENT with the current patch identity absent from it is the finding: the +# engine looked and there is no receipt. Present WITH it is clean, whatever the +# review said. +# +# READING THE OUTER MAP AS THE COULD-NOT-LOOK CHANNEL IS THE MISTAKE THIS +# MODULE ALREADY MADE ONCE. An unlistable store leaves that map EMPTY, not +# `null`, so a guard on the outer object refuses exactly the case it means to +# abstain on. +# +# TWO WAYS THIS OVER-OWES, BOTH STATED RATHER THAN DISCOVERED. `owed` is read +# off a TIP diff over the working tree and `subject` is a MERGE-BASE diff over +# committed bytes, so the two do not answer about the same range. On a stale +# branch, code that landed on trunk reads as this branch's and a prose-only +# branch can owe a review until it rebases — `verify` asserts the branch is +# rebased on current `origin/main`, so on the path that matters they agree. And +# the identity covers the WHOLE change, so a prose commit added to a branch that +# also carries code moves the key and re-owes the review. Both err toward owing +# a review that is not strictly due, which is the direction a completion gate +# should fail in; neither can produce the other direction, which is a branch +# landing unread. +# +# THE CLEAN-TREE CONDITION IS NOWHERE, AND THAT IS A DECISION TWICE OVER. A +# conjunct here would either duplicate `tree-clean` — which already owns tree +# cleanliness for the landing path, and which nothing reaches `main` without +# passing — or, read the other way round, let a dirty tree SILENCE this gate. +# The mint carried one instead and it has been withdrawn: `git::uncommitted` +# counts an uninitialised gitlink as changed, so `Ok(0)` was unsatisfiable in +# this repository and the receipt could never have been written (CLOUD-1500). +# A reviewer reads the working tree anyway, so refusing to record a dispatch +# taken over uncommitted work would attest less than happened, not more. # # THE BRACKETS ARE NOT STYLE: the schema file carries a hyphen, so the dotted # form is a parse error reported as `invalid schema reference`. @@ -60,13 +82,28 @@ rules contains "code-review-dispatched" # refuse. required contains "code-review" -# Whether the engine could look at the receipt store at all. +# Whether the engine could look at the receipt store FOR THIS ID. +# +# PER-ID, NOT ON THE OUTER MAP, and that distinction is the whole three-valued +# read rather than a refinement of it. `minted::fields` inserts an entry for every +# declared row whose store it could LIST, and skips the row entirely when it could +# not — so an id ABSENT from the map is could-not-look, and an id PRESENT with no +# matching subject is the engine having looked and found nothing. # -# GUARDED on `is_object`: the key is `null` when nobody declared a receipt or no -# store is readable, and reaching into `null` is a hard evaluation FAULT in Rego -# rather than a silent miss. -looked if { +# THE FIRST DRAFT GUARDED ON `is_object(input.tree.minted)` AND HAD THE ARM +# EXACTLY BACKWARDS. An unlistable store leaves the map EMPTY rather than `null`, +# `is_object({})` holds, and the refusal then fired on every fresh clone and every +# CI runner — the arm this module's own METADATA says it abstains on, and the one +# `batten.toml` promises is honestly silent until CLOUD-877 gives the receipt a +# portable form. Caught by the code review this gate exists to demand, which is +# the only reason it is not in the tree. +# +# BOTH `is_object` CALLS ARE LOAD-BEARING. The outer one is what keeps this from +# indexing `null` — a hard evaluation FAULT in Rego rather than a silent miss — +# when no row declares a receipt at all. +looked_at(id) if { is_object(input.tree.minted) + is_object(input.tree.minted[id]) } delta := input.tree["base-delta"] @@ -83,6 +120,24 @@ delta := input.tree["base-delta"] # `unreviewed` would refuse a checkout with nothing to review. subject := delta["patch-id"] +# The path prefixes this repository treats as code for the purpose of owing a +# review. +# +# DECLARED HERE BECAUSE THE ROW'S `delta_sources` DOES NOT NARROW WHAT THIS +# MODULE SEES, which is the engine's shape rather than a mistake in the row. +# `rules.rs` builds ONE `base_delta` from the UNION of every row's globs and hands +# the same value to every module, so a row's own list only ever ADDS to what +# everybody reads — and six rows in this config declare `["**"]`. Without the +# narrowing below, a branch touching only `.github/workflows/*.yml` or +# `schema/*.json` would owe a code review. Caught by the code review this gate +# exists to demand. +# +# PREFIXES RATHER THAN A `[[pattern]]` ROW: this is a path SET, not a concept with +# one spelling, and `.claude/rules/policy-modules.md`'s registry is for the +# latter. A threshold or a path list spelled as a regex is the error that file +# records twice. +reviewable := {"crates/", "policy/", "mise-tasks/"} + # Whether a review is owed at all. # # GATED ON CODE, not on any change. `code-changed` is the subset whose non-comment @@ -90,7 +145,17 @@ subject := delta["patch-id"] # clone or a fixture, which is the narrowing `review-dispatched` had to add after # four `cli.rs` cases went red at once for wanting to exercise other rules. owed if { - count(delta["code-changed"]) > 0 + some path in delta["code-changed"] + some prefix in reviewable + startswith(path, prefix) +} + +# `batten.toml` is the policy authority every gate reads, so an edit to it is a +# change to what this repository refuses. A separate arm rather than a fourth +# prefix, because it is a FILE and the set above is a directory test — folding it +# in would make `batten.toml.example` reviewable by accident. +owed if { + "batten.toml" in delta["code-changed"] } # Every declared receipt with nothing filed under this change. @@ -100,10 +165,10 @@ owed if { # means `not reviewed as this now stands`. A module re-deriving that would be the # second authority over an identity `git::branch_patch_id` already owns. unattested contains id if { - looked owed is_string(subject) some id in required + looked_at(id) not input.tree.minted[id][subject] } @@ -177,6 +242,41 @@ test_could_not_look_does_not_fault if { # A PROSE-ONLY BRANCH OWES NO CODE REVIEW. Without this the gate refuses every # checkout that has never dispatched, which is every fixture and every fresh # clone. +# A CHANGE OUTSIDE THE REVIEWABLE PREFIXES OWES NOTHING. The row's own +# `delta_sources` cannot express this — the engine hands every module one delta +# built from the union of every row's globs — so without the prefix set a +# workflow-only or schema-only branch owes a code review. +test_a_change_outside_the_reviewable_prefixes_is_not_refused if { + count(violation) == 0 with input as attested_over({ + "added": [], + "edited": [".github/workflows/ci.yml"], + "deleted": [], + "code-changed": [".github/workflows/ci.yml"], + "patch-id": "abc", + }) +} + +# THE POLICY AUTHORITY IS REVIEWABLE ON ITS OWN ARM. An edit to `batten.toml` is a +# change to what this repository refuses. +test_an_edit_to_the_authority_is_refused if { + some v in violation with input as attested_over({ + "added": [], + "edited": ["batten.toml"], + "deleted": [], + "code-changed": ["batten.toml"], + "patch-id": "abc", + }) + v.verdict == "patch read never" +} + +# AN UNLISTABLE STORE IS COULD-NOT-LOOK, and reading it as `looked` is the defect +# the code review caught: `minted::fields` leaves the id ABSENT when it cannot +# list, so the map is EMPTY rather than `null` and a guard on the outer object +# refuses every fresh clone and every CI runner. +test_an_unlistable_store_is_not_refused if { + count(violation) == 0 with input as {"tree": {"base-delta": changed, "minted": {}}} +} + test_a_prose_only_branch_is_not_refused if { count(violation) == 0 with input as attested_over({ "added": [], @@ -205,3 +305,4 @@ attested_over(d) := {"tree": {"base-delta": d, "minted": {"code-review": {}}}} #MUTANT absent-receipt-unread|s@^\tnot input.tree.minted\[id\]\[subject\]$@\tfalse@|an_absent_receipt_is_refused_over_the_engines_own_projection #MUTANT no-identity-priced|s@^\tis_string(subject)$@\ttrue@|a_change_with_no_identity_owes_no_review #MUTANT prose-only-priced|s@^\towed$@\ttrue@|a_prose_only_branch_owes_no_code_review +#MUTANT store-unreadable-refused|s@^\tlooked_at(id)$@\ttrue@|an_unlistable_store_is_could_not_look_and_never_a_refusal From 26e19e70a4a51f66a164128f694ec66f482089e6 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 04/21] test(gate): a map naming only another id is could-not-look, not a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-id guard made the old case's expectation wrong, and it was wrong for a reason worth keeping: `minted` is keyed by DECLARED ROW, and `minted::fields` inserts an entry for every row whose store it could list — so an id missing from the map means the engine never looked for it, never that it looked and found nothing. Refusing there is a verdict about the engine's reach wearing a verdict about the branch. The claim the case meant to make is already `test_a_receipt_over_another_change_ does_not_answer`, where the id IS present and the subject belongs to another change. Refs: CLOUD-1484 --- policy/code-review-dispatched.rego | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego index 7ef95553b..e47f617c8 100644 --- a/policy/code-review-dispatched.rego +++ b/policy/code-review-dispatched.rego @@ -224,13 +224,24 @@ test_the_refusal_names_the_receipt if { ids == {"code-review"} } -# A RECORD UNDER ANOTHER ID IS NOT THIS ONE HAVING RUN. -test_another_receipts_record_does_not_answer if { - some v in violation with input as {"tree": { +# A MAP NAMING ONLY ANOTHER ID IS COULD-NOT-LOOK, NOT A REFUSAL — and this case +# asserted the opposite until the per-id guard landed. +# +# The reasoning that made it look right was that a record under another id is not +# this one having run, which is true and is not what this shape says. `minted` is +# keyed by DECLARED ROW, and `minted::fields` inserts an entry for every row whose +# store it could list — so an id missing from the map means the engine never +# looked for it, never that it looked and found nothing. Refusing here would be a +# verdict about the engine's reach wearing a verdict about the branch. +# +# The claim the old case meant to make is `test_a_receipt_over_another_change_ +# does_not_answer` above, where the id IS present and the subject is another +# change's. +test_a_map_naming_only_another_id_is_not_refused if { + count(violation) == 0 with input as {"tree": { "base-delta": changed, "minted": {"other": {"abc": "cafe 1700000000"}}, }} - v.verdict == "patch read never" } # COULD-NOT-LOOK, and without the `is_object` guard this case does not merely From 01ee91cab73082eead939beb1a21d4552ab69242 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 05/21] style(mint): wrap the selector binding as rustfmt wants it `lint:fmt` refused the `let (Some(path), Some(expected)) = ...` line in `mint::selects`. No behaviour changes; prettier reflowed `.claude/rules/policy-modules.md` in the same pass. Refs: CLOUD-1484 --- .claude/rules/policy-modules.md | 4 ++-- crates/batten/src/mint.rs | 3 ++- crates/batten/tests/it/code_review_dispatched.rs | 10 ++++++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/.claude/rules/policy-modules.md b/.claude/rules/policy-modules.md index e6d5617cb..d7bddd317 100644 --- a/.claude/rules/policy-modules.md +++ b/.claude/rules/policy-modules.md @@ -185,8 +185,8 @@ rebase — `land` rebases every lap, and a per-lap re-attestation is the cost th gets a gate switched off. Its `null` is could-not-look AND the empty diff: a branch that changed nothing has no identity, `cumulative_patch_id` refuses to mint one so two empty changes cannot compare equal, and a predicate reading that absence as -*unattested* refuses a checkout with nothing to review. Do not re-derive an -identity from the path lists — that is a second notion of *the same change*, free +_unattested_ refuses a checkout with nothing to review. Do not re-derive an +identity from the path lists — that is a second notion of _the same change_, free to disagree with `landing`'s about exactly the rebase the key turns on. **`records-blocked` is the recorder surface's could-not-look, and its EMPTY value diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index b46654630..45f6e9595 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -398,7 +398,8 @@ pub(crate) fn scalar(value: &serde_json::Value, path: &str) -> Option { /// see. #[must_use] pub fn selects(declared: &Declared, result: &serde_json::Value) -> bool { - let (Some(path), Some(expected)) = (declared.selects_at.as_deref(), declared.selects.as_deref()) + let (Some(path), Some(expected)) = + (declared.selects_at.as_deref(), declared.selects.as_deref()) else { return true; }; diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs index 21779b53e..679be332e 100644 --- a/crates/batten/tests/it/code_review_dispatched.rs +++ b/crates/batten/tests/it/code_review_dispatched.rs @@ -216,7 +216,10 @@ fn a_receipt_under_this_change_reaches_the_predicate_and_is_clean() { #[test] fn a_receipt_over_another_change_does_not_answer() { let root = repo("code-review-stale"); - file_receipt(&root, "0000000000000000000000000000000000000000000000000000000000000000"); + file_receipt( + &root, + "0000000000000000000000000000000000000000000000000000000000000000", + ); assert_eq!( verdicts(&root), vec![String::from(RULE)], @@ -257,7 +260,10 @@ fn a_rebase_onto_a_moved_base_does_not_re_owe_the_review() { // Move the base under the branch with a change this branch never made, then // rebase onto it — the landing loop's own lap, in miniature. - common::git_in(&root, &["checkout", "-q", "-b", "trunk", "refs/remotes/origin/main"]); + common::git_in( + &root, + &["checkout", "-q", "-b", "trunk", "refs/remotes/origin/main"], + ); fs::write(root.join("NOTES.md"), "trunk moved\n").expect("the trunk file"); common::git_in(&root, &["add", "-A"]); common::git_in(&root, &["commit", "-q", "-m", "trunk"]); From c2a82518b10049e617706d2326a57f24eabd2cd6 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 06/21] fix(gate): repair the five defects the gate's second review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/code-review` was dispatched again after the enrolment commit moved the patch identity — which is the gate demanding its own re-attestation — and this time every finding was inside this branch's diff. THE WRITER WAS NEVER DRIVEN, and that is the one that would have shipped a gate refusing forever with a green suite. Every case hand-wrote `code-review.` and called `branch_patch_id` directly, so `mint_receipts` was never invoked and the two halves were never shown to agree: a different key base, separator or authority root would have left the reader looking somewhere the writer never wrote. That is the `with input as` class one layer over, in the suite of the module whose own header refuses it. `mint_receipts_for_test` drives the real boundary, and the wrong-skill arm is there too, so the selector is proven over the engine rather than over `selects` alone. `batten.toml` ASSERTED A GUARANTEE THIS CHANGE HAD WITHDRAWN. The block still said the boundary refuses to mint while the tree is dirty; that conjunct was removed earlier in this same branch because `git::uncommitted` counts an uninitialised gitlink as changed forever (CLOUD-1500), so it could never hold. A false guarantee in the policy authority is worse than a missing one. `branch_patch_id` PROMISED could-not-look AND RAISED. `head_commit(dir)?` propagates where the doc says `None` — latent while both callers swallow it, and a trap for the next one that reads the doc instead of the body. `base_delta` PAID FOR THE IDENTITY UNCONDITIONALLY while every sibling git fact is declaration-gated. It is a second repository open, a merge-base walk and a tree diff, on every `check` declaring any `delta_sources` — which is most of them — for a field one module reads. Gated on a row declaring `minted`. `mint::selects` AND THE FOUR NEW LOAD REFUSALS HAD NO TESTS. Loosening the equality to a prefix, or returning true on an unresolvable path, reddened nothing — and either makes the row mint on every skill dispatch, which is the false green the column exists to remove. Two findings are bounds rather than fixes, stated where a reader meets them: `requires = []` has no success predicate because `Declared` carries no `is_error` column, so an errored dispatch still mints; and `reviewable` restates the row's `delta_sources` with nothing holding the two in agreement. Refs: CLOUD-1484 Admits: 0a91c088ab4d93b4a6e1bd01f340b1d94d756cb57f3e003a56baf2f07593684c Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:58efe536b4d39ef6aac43e52def6da3ea67e74b4 Admits-epoch: 976d2841a1864bc485ab2531d686611f0708820fc8f307ba0f2b965502c679cf Admits-author: alec@wenzowski.com Admits-prev: 2b23578df77a1290c5cfa5f47512c93cc502705647553d90a63c5c47d4c0e5e4 Admits-answer-lost: A false guarantee stays in the file every gate reads. A reader checking whether a dirty tree is refused would find batten.toml saying yes and the engine doing nothing, which is worse than no statement at all — it is the shape this repository records as a false assurance, and the code review that found it is the only thing that would have caught it. Admits-answer-precondition: The write is a CORRECTION to prose in batten.toml that asserted a guarantee this branch had already withdrawn: the block said the boundary refuses to mint while the tree is dirty, and that conjunct was removed earlier in this same branch because git::uncommitted counts an uninitialised gitlink as changed forever (CLOUD-1500). No [[redirect]] surface can edit a comment, and the claim is in the policy authority where a reader meets it. It lands in PR #880 beside the code that withdrew the conjunct. Admits-answer-rejected-route: config read first — reading is how the defect was found; the false sentence still has to be replaced. patch run first (git restore) — that discards the correction and restores the false guarantee, which is the opposite of the repair. --- batten.toml | 14 ++- crates/batten/src/git.rs | 28 ++++- crates/batten/src/lib.rs | 24 ++++- crates/batten/src/mint.rs | 101 ++++++++++++++++++ crates/batten/src/perf.rs | 2 +- crates/batten/src/rules.rs | 10 +- .../batten/tests/it/code_review_dispatched.rs | 86 +++++++++++++++ 7 files changed, 256 insertions(+), 9 deletions(-) diff --git a/batten.toml b/batten.toml index 1cdc5132b..5ef092944 100644 --- a/batten.toml +++ b/batten.toml @@ -1326,9 +1326,17 @@ body = "{join:issues[].id}\nbase {git:origin/main}" # KEYED `delta`, so the receipt is stale by construction. `key_base` is # `origin/main` because that is what this repository lands onto; the identity is # a MERGE-BASE diff, so `land`'s per-lap rebase does not re-buy the review, and -# any commit that moves code does. The boundary refuses to mint at all while the -# tree is dirty — a receipt keyed to committed bytes must not be taken in a -# session looking at something else. +# any commit that moves code does. +# +# THE TREE'S CLEANLINESS IS NOT CHECKED HERE, AND AN EARLIER DRAFT OF THIS BLOCK +# CLAIMED IT WAS. The conjunct was withdrawn during this change — `git::uncommitted` +# counts an uninitialised gitlink as changed forever (CLOUD-1500), so `Ok(0)` could +# never hold and the receipt could never be minted at all. What remains true is +# narrower and worth stating exactly: the identity is over COMMITTED bytes, so an +# uncommitted edit cannot move the key. A review dispatched over a dirty tree +# therefore attests the committed change it was keyed to and says nothing about +# what was uncommitted at the time. `tree-clean` is what refuses a dirty tree on +# the landing path, and it is the only thing that does. # # THE BODY CARRIES NO FINDING AND HAS NOWHERE TO PUT ONE. Two fields: the head # this was taken at, and the instant. Rule 4 holds structurally rather than by diff --git a/crates/batten/src/git.rs b/crates/batten/src/git.rs index ef766f719..ff00bc0c7 100644 --- a/crates/batten/src/git.rs +++ b/crates/batten/src/git.rs @@ -2772,7 +2772,12 @@ pub fn materialize_rev(dir: &Path, rev: &str, dest: &Path) -> Result<()> { /// # Errors /// /// Raises only when the repository cannot be opened at all. -pub fn base_delta(dir: &Path, base: &str, globs: &[String]) -> Result> { +pub fn base_delta( + dir: &Path, + base: &str, + globs: &[String], + wants_patch_id: bool, +) -> Result> { let repository = open(dir)?; let hash = repository.object_hash(); @@ -2907,7 +2912,17 @@ pub fn base_delta(dir: &Path, base: &str, globs: &[String]) -> Result Result> { let Some(base) = resolve_ref(dir, base_ref)? else { return Ok(None); }; - let head = head_commit(dir)?; + // `None` RATHER THAN `?`, so the doc above is true of the body. `head_commit` + // raises on an unresolvable HEAD — an empty repository, a broken ref — and + // propagating that would make this function raise where it promises + // could-not-look. Latent while both callers swallow the error, and a trap for + // the next one that follows the doc instead of reading the code. + let Ok(head) = head_commit(dir) else { + return Ok(None); + }; Ok(cumulative_patch_id(dir, &base, &head) .ok() .flatten() diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 13dddc858..f546b83fd 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -11265,6 +11265,28 @@ fn recover_spilled(result: &serde_json::Value) -> Option { /// authority — [`mcp::payload`] for JSON-RPC content blocks, [`facts::payload_in`] /// for the harness envelope. Every failure is silent, as the mint boundary has /// always been: the gate that reads the receipt simply denies again. +/// [`mint_receipts`] reached from the integration tier (CLOUD-1484). +/// +/// **The boundary itself, never a re-implementation.** The half that WRITES a +/// receipt and the half that READS it must be shown to agree, and a test that +/// hand-writes the file proves the reader against an artifact the engine may not +/// produce — a different key base, separator or authority root leaves the gate +/// refusing forever with every case green. That is the `with input as` class one +/// layer over, so the tier calls the real function. +/// +/// `pub` and named for its caller rather than made generally available: nothing +/// in the binary reaches it, and the name says who it is for. +#[doc(hidden)] +pub fn mint_receipts_for_test( + declared: &[crate::mint::Declared], + tool: &str, + input: &serde_json::Value, + result: &serde_json::Value, + root: &Path, +) { + mint_receipts(declared, tool, input, result, root, None); +} + fn mint_receipts( declared: &[crate::mint::Declared], tool: &str, @@ -13423,7 +13445,7 @@ impl<'a> CheckScope<'a> { // tree — the flag narrows which files rules SEE, and a glob here // would be a second selection layered under `PathSet`'s. let whole_tree = ["**".to_owned()]; - let delta = git::base_delta(root, rev, &whole_tree)?.ok_or_else(|| { + let delta = git::base_delta(root, rev, &whole_tree, false)?.ok_or_else(|| { UsageError::raise(format!( "check: --since {rev} resolves but its change-set could not be read" )) diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 45f6e9595..f7706fe1c 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -654,6 +654,107 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { mod tests { use super::*; + /// A row declaring a selector mints only for the value it names. + /// + /// Fails by: comparing with `starts_with`, or returning `true` on an + /// unresolvable path. Either makes the row mint on every dispatch its tool + /// matched, which is the false green `batten.toml` says the column exists to + /// remove — the receipt would attest that *something* ran. + #[test] + fn a_selector_admits_only_the_value_it_names() { + let row: Declared = serde_json::from_value(serde_json::json!({ + "name": "code-review", "tool": "Skill", + "selects_at": "skill", "selects": "code-review", + "key": "delta", "key_base": "origin/main", + "mode": "replace", "body": "{now}", + })) + .expect("the committed row's shape loads"); + + assert!(selects(&row, &serde_json::json!({"skill": "code-review"}))); + assert!( + !selects(&row, &serde_json::json!({"skill": "batten"})), + "another skill must not mint this row's receipt" + ); + assert!( + !selects(&row, &serde_json::json!({"skill": "code-review-extra"})), + "EQUALITY, never a prefix: a longer name nobody declared must not match" + ); + assert!( + !selects(&row, &serde_json::json!({})), + "an unresolvable path does not select — minting on a maybe is the forgery this removes" + ); + assert!( + !selects(&row, &serde_json::Value::Null), + "the dispatch path passes a null input, which must select nothing" + ); + } + + /// A row declaring NO selector is judged exactly as it was before the column + /// existed, which is every landed row. + #[test] + fn a_row_with_no_selector_selects_everything_its_tool_matched() { + let row: Declared = serde_json::from_value(serde_json::json!({ + "name": "issue-read", "tool": "get_issue", "key": "named", + "key_from": "id", "mode": "replace", "body": "{id}", + })) + .expect("a landed row's shape loads"); + assert!(selects(&row, &serde_json::json!({"anything": 1}))); + assert!(selects(&row, &serde_json::Value::Null)); + } + + /// Each new load-time refusal fires, and a well-formed row still loads. + /// + /// Fails by: dropping any one arm. Half a selector is the dangerous half — + /// `selects_at` alone narrows nothing while reading as though it did. + #[test] + fn the_new_load_refusals_each_fire() { + let row = |extra: serde_json::Value| -> Declared { + let mut base = serde_json::json!({ + "name": "r", "tool": "T", "key": "delta", + "key_base": "origin/main", "mode": "replace", "body": "{now}", + }); + let (Some(o), Some(e)) = (base.as_object_mut(), extra.as_object()) else { + unreachable!("both are objects") + }; + for (k, v) in e { + if v.is_null() { + o.remove(k); + } else { + o.insert(k.clone(), v.clone()); + } + } + serde_json::from_value(base).expect("shape loads") + }; + + assert!( + validate(&[row(serde_json::json!({}))]).is_ok(), + "the well-formed row loads" + ); + assert!( + validate(&[row(serde_json::json!({"key_base": null}))]).is_err(), + "`delta` with no `key_base` names no base to measure against" + ); + assert!( + validate(&[row( + serde_json::json!({"key": "branch", "key_base": "origin/main"}) + )]) + .is_err(), + "a `key_base` on a row keyed otherwise would be read by nothing" + ); + assert!( + validate(&[row(serde_json::json!({"selects_at": "skill"}))]).is_err(), + "half a selector narrows nothing while reading as though it did" + ); + assert!( + validate(&[row(serde_json::json!({"selects": "x"}))]).is_err(), + "a value compared to nothing" + ); + assert!( + validate(&[row(serde_json::json!({"selects_at": " ", "selects": "x"}))]).is_err(), + "an empty `selects_at`" + ); + } + #[test] fn an_absent_optional_records_the_could_not_look_token_never_a_hash_of_nothing() { // CLOUD-691's measured forgery, in the one place this module could diff --git a/crates/batten/src/perf.rs b/crates/batten/src/perf.rs index 415e890a5..c85778623 100644 --- a/crates/batten/src/perf.rs +++ b/crates/batten/src/perf.rs @@ -401,7 +401,7 @@ fn base_commit(repo: &Path, base_ref: &str) -> Result { /// `base..HEAD`, so an uncommitted change could move the measured cost while the /// skip looked only at what was committed. fn changed_between(repo: &Path, base: &str) -> Result> { - let delta = crate::git::base_delta(repo, base, &[String::from("**")])?.ok_or_else(|| { + let delta = crate::git::base_delta(repo, base, &[String::from("**")], false)?.ok_or_else(|| { anyhow::anyhow!( "perf-pair: could not diff the base against this tree, so the skip could not be decided. No measurement." ) diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 4bd075bb8..c3a1acf8f 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -7908,7 +7908,15 @@ fn git_facts(rules: &[Rule], root: &Path) -> crate::git::GitFacts { (1, false) => delta_bases .iter() .next() - .and_then(|base| crate::git::base_delta(root, base, &deltas).ok()) + .and_then(|base| { + // THE IDENTITY IS PAID FOR ONLY BY A ROW THAT DECLARES ITS + // CONSUMER. `patch-id` keys a `[[rule.minted]]` receipt and + // nothing else reads it, so a tree whose rules declare no + // minted row pays no merge-base walk — the same declaration + // economy every other git fact here already keeps. + let wants = rules.iter().any(|rule| !rule.minted.is_empty()); + crate::git::base_delta(root, base, &deltas, wants).ok() + }) .flatten(), _ => None, }, diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs index 679be332e..e63c763f4 100644 --- a/crates/batten/tests/it/code_review_dispatched.rs +++ b/crates/batten/tests/it/code_review_dispatched.rs @@ -313,6 +313,7 @@ fn a_prose_only_branch_owes_no_code_review() { &root, "refs/remotes/origin/main", &[String::from("crates/**")], + true, ) .expect("the repository opens") .expect("the base resolves"); @@ -370,6 +371,7 @@ fn a_change_with_no_identity_owes_no_review() { &root, "refs/remotes/origin/main", &[String::from("crates/**")], + true, ) .expect("the repository opens") .expect("the base resolves"); @@ -422,6 +424,90 @@ fn an_unlistable_store_is_could_not_look_and_never_a_refusal() { ); } +/// THE WRITER AND THE READER AGREE, driven through the real mint boundary. +/// +/// Every other case here files the receipt by hand, which proves the predicate +/// and nothing about the half that writes it. If `mint_receipts` composed a +/// different key — another base, another separator, a different authority root — +/// the gate would refuse forever with every one of those cases green. That is the +/// same class this module's own header refuses for `with input as`, one layer +/// over: a fabricated artifact cannot prove the engine produces it. +/// +/// So this drives the boundary itself and then asks the gate, with nothing +/// hand-written in between. +#[test] +fn the_mint_boundary_writes_the_receipt_this_gate_reads() { + let root = repo("code-review-writer"); + assert_eq!( + verdicts(&root), + vec![String::from(RULE)], + "the fixture starts owing a review" + ); + + let row: batten::mint::Declared = serde_json::from_value(serde_json::json!({ + "name": RECEIPT, + "tool": "Skill", + "selects_at": "skill", + "selects": "code-review", + "key": "delta", + "key_base": "refs/remotes/origin/main", + "mode": "replace", + "body": "{git:HEAD} {now}", + })) + .expect("the committed row's shape loads"); + + // The envelope as the live host sends one: the skill named in the INPUT, and + // a result carrying no JSON at all — which is what `record_mints` had to stop + // giving up on for this row to fire. + batten::mint_receipts_for_test( + &[row], + "Skill", + &serde_json::json!({"skill": "code-review"}), + &serde_json::Value::Null, + &root, + ); + + assert!( + verdicts(&root).is_empty(), + "the receipt the boundary wrote must be the one the module looks up" + ); +} + +/// AND A DIFFERENT SKILL WRITES NOTHING, through the same boundary. +/// +/// Without the selector the row mints on every dispatch its tool matched, and the +/// receipt attests that *something* ran. This is that arm over the engine rather +/// than over `selects` alone. +#[test] +fn another_skill_mints_nothing_through_the_boundary() { + let root = repo("code-review-wrong-skill"); + let row: batten::mint::Declared = serde_json::from_value(serde_json::json!({ + "name": RECEIPT, + "tool": "Skill", + "selects_at": "skill", + "selects": "code-review", + "key": "delta", + "key_base": "refs/remotes/origin/main", + "mode": "replace", + "body": "{git:HEAD} {now}", + })) + .expect("the committed row's shape loads"); + + batten::mint_receipts_for_test( + &[row], + "Skill", + &serde_json::json!({"skill": "batten"}), + &serde_json::Value::Null, + &root, + ); + + assert_eq!( + verdicts(&root), + vec![String::from(RULE)], + "a dispatch of another skill must leave the review still owed" + ); +} + /// COULD-NOT-LOOK IS NOT A REFUSAL. A checkout whose row declares no receipt /// projects `null`, and the gate must go quiet rather than refuse the machine. #[test] From d9cf79db136663bac9d402e30a2126a6fe1b0fca Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 07/21] style(mint): let the test module use expect, as its siblings do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `clippy::expect_used` is denied crate-wide, and the new `mod tests` cases build their fixtures with `serde_json::from_value(...).expect(...)`. `render.rs`, `contract.rs` and `admission.rs` each carry the same allow on the same attribute — the lint is about reachable library paths, and a test fixture that cannot be constructed should fail the test loudly. Refs: CLOUD-1484 --- crates/batten/src/mint.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index f7706fe1c..71acb0322 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -651,6 +651,7 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { } #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { use super::*; From c10a2b3fb7d02783945a8801d6a59852899bb2b7 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 08/21] fix(mint): repair the four correctness defects the third review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate demanded a third review after the floor move shifted the patch identity, and every finding was again inside this branch's diff. THE NULL FALLBACK WIDENED `satisfied` VACUOUSLY, and it is mine. `record_mints` passes `Value::Null` where it used to return early, so a row reading none of the result can mint from a call whose answer carried no JSON. For a `requires = []` row whose BODY reads the result, the success test is then vacuously true, every path piece records the could-not-look token, and under `MintMode::Replace` that all-`-` record overwrites a good receipt. No landed row is shaped that way, which is exactly why nothing would have caught it. `render` now withholds a record when the result is null and any piece reads it; `Piece::reads_the_result` is exhaustive with no wildcard, so a variant added later fails to compile rather than classifying itself as reading nothing. THE `patch-id` GATING WAS UNSOUND AND IS WITHDRAWN. A previous review asked for it to be declaration-gated like every other git fact, and the gating shipped keyed on `!rule.minted.is_empty()` — which is not the declaration that reads the fact. A module reading `patch-id` with no sibling `[[rule.minted]]` row would see `null`, decide nothing, and exit 0; for `code-review-dispatched` itself, dropping the minted block would have silenced the gate through two channels at once. The merge-base walk is paid again and the cost is stated rather than hidden. Gating it honestly needs a column naming which facts a row reads, which is a bigger change than this one. `selects` NAMED THE WRONG SURFACE. Its parameter and doc said RESULT while every caller passes the INPUT, contradicting the call site — the wrong-surface trap that produces a row matching nothing and a gate that denies forever. The unit cases could not catch it, because they supply the object themselves. A BLANK `selects` LOADED CLEAN where a blank `selects_at` was refused. It matches no value any host sends, so the row mints nothing and its gate denies forever with a remedy that cannot clear it. Both halves are refused now. Two false claims are corrected where a reader meets them. The module METADATA said it could not under-owe: it can, because the identity is over COMMITTED bytes, so an uncommitted code edit leaves the receipt valid and `batten check` reports clean over unreviewed work — `tree-clean` is what covers that, and only on the landing path. And the verdict route names `/code-review` without saying that a TYPED slash command may mint nothing on a host that expands it with no tool call, which is a remedy the reader cannot walk. One finding is left as a bound: `key = "delta"` is the first unbounded keying — one receipt per patch identity, never pruned, and `minted::fields` reads every matching file on each `check`. Refs: CLOUD-1484 Admits: 486f8518288e82b9b457c9df8dd28a0d34b8f705466d0b2952eb2b6f7e1953b6 Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:d874abf892e4207f698df9ad33c4d3dd6114e20c Admits-epoch: 5b699dcd1dcec80408ccacd8807b5e803d6068c2530bc97924477d4bfd87c93b Admits-author: alec@wenzowski.com Admits-prev: 958bfb7cb712ab26af8d9194bc7fd64b9fa29ad2822ad226bedbd96b1e87cd14 Admits-answer-lost: A route that cannot be walked stays in the file as though it can. The reader who hits it is a person at a keyboard following the refusal's own instruction, and the loop has no exit visible from the message — which is strictly worse than a class with no route at all, because it reads as a way out. Admits-answer-precondition: Two corrections to prose in batten.toml, both found by the code review this branch's own gate demanded. The [[verdict.route]] block names /code-review as the remedy without stating that a TYPED slash command may mint nothing on a host that expands it with no tool call, so the gate re-denies with a remedy the reader cannot walk. No [[redirect]] surface can edit a route's commentary, and the claim lives in the policy authority where a reader meets it. It lands in PR #880 beside the module it describes. Admits-answer-rejected-route: config read first: reading is how the defect was found, and the sentence still has to be written; a route stated wrongly is not repaired by re-reading it. patch run first (git restore): that discards the correction and restores the unqualified route, which is the defect rather than the repair. --- batten.toml | 10 +++++ crates/batten/src/mint.rs | 72 +++++++++++++++++++++++++----- crates/batten/src/rules.rs | 28 ++++++++---- policy/code-review-dispatched.rego | 9 ++++ 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/batten.toml b/batten.toml index 5ef092944..a2a3d4219 100644 --- a/batten.toml +++ b/batten.toml @@ -11546,6 +11546,16 @@ container, so this speaks where the branch is worked and is honestly silent in \ CI until that store has a portable form. """ +# THE ROUTE NAMES THE SKILL, AND HOW IT IS INVOKED DECIDES WHETHER IT MINTS. +# +# The mint fires on a `Skill` TOOL CALL carrying `{"skill": "code-review"}`. An +# agent that invokes the skill through its tool surface therefore clears this +# gate. A person who TYPES the slash command may not: on hosts that expand it by +# injecting the skill's text with no tool call, the boundary sees nothing, no +# receipt is minted, and the gate re-denies with this same remedy — a loop whose +# exit is not visible from the message. Stated here because a route that cannot +# be walked is worse than none, and the reader who hits it is the one holding the +# keyboard. [[verdict.route]] id = "verb run first" kind = "command" diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 71acb0322..387c52a59 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -245,6 +245,30 @@ enum Piece { /// has to point. const ABSENT: &str = "-"; +impl Piece { + /// Whether rendering this piece consults the tool RESULT. + /// + /// Exhaustive with no wildcard arm, which is `facts.rs`' rule and for its + /// reason: a variant added later must fail to compile rather than classify + /// itself as reading nothing, because that is the direction that silently + /// re-opens the record this predicate exists to withhold. + fn reads_the_result(&self) -> bool { + match self { + // `{authority:…}` is grouped with the path forms rather than given its + // own arm because `clippy::match_same_arms` refuses a repeated body — + // the distinction a reader wants lives here, as `git.rs` states for the + // same lint. It judges the whole result DOCUMENT where the others read + // a path into it, and both consult the result, which is all this asks. + Piece::Path(_) + | Piece::Digest(_) + | Piece::Slug(_) + | Piece::Join(_) + | Piece::Authority(_) => true, + Piece::Literal(_) | Piece::Now | Piece::Git(_) => false, + } + } +} + /// Split a body template into its pieces, or name the first thing wrong with it. /// /// Shared by [`validate`] and [`render`] so a template that loads is a template @@ -387,9 +411,15 @@ pub(crate) fn scalar(value: &serde_json::Value, path: &str) -> Option { /// nothing indistinguishable from one whose call failed, and only the second of /// those is a reason to try again. /// -/// A row declaring no selector selects every result its `tool` matched, which is +/// A row declaring no selector selects every call its `tool` matched, which is /// every landed row's behaviour. /// +/// **THE SUBJECT IS THE TOOL'S INPUT, NEVER ITS RESULT**, and the parameter is +/// named for it: a dispatch names ITSELF in its arguments, while its result is +/// whatever came back and on most hosts is prose. An author who reads this as the +/// result writes a path that resolves to nothing on every call, and the row then +/// mints nothing while loading clean — the dead gate this crate keeps recording. +/// /// **A path that does not resolve to a single scalar does NOT select.** The /// failure direction is deliberate: an unreadable field means the boundary cannot /// tell whether this is the declared call, and minting on a maybe is exactly the @@ -397,13 +427,13 @@ pub(crate) fn scalar(value: &serde_json::Value, path: &str) -> Option { /// again with the same remedy, which is the safe direction and one the agent can /// see. #[must_use] -pub fn selects(declared: &Declared, result: &serde_json::Value) -> bool { +pub fn selects(declared: &Declared, input: &serde_json::Value) -> bool { let (Some(path), Some(expected)) = (declared.selects_at.as_deref(), declared.selects.as_deref()) else { return true; }; - scalar(result, path).is_some_and(|found| found == expected) + scalar(input, path).is_some_and(|found| found == expected) } /// Whether every required path resolved, which is this module's success test. @@ -494,6 +524,18 @@ pub fn render( let Ok(pieces) = parse(&declared.body) else { return None; }; + // A RESULT THAT SAID NOTHING CANNOT RENDER A BODY THAT READS IT (CLOUD-1484). + // + // `record_mints` now passes `Value::Null` where it used to return early, so a + // row reading none of the result can mint from a call whose answer carried no + // JSON. That widening must not reach a row that DOES read it: with + // `requires = []` its success test is vacuously true, every path piece records + // the could-not-look token, and under `MintMode::Replace` the all-`-` record + // overwrites a good receipt. No landed row is shaped that way, which is + // precisely why nothing would have caught it. + if result.is_null() && pieces.iter().any(Piece::reads_the_result) { + return None; + } let mut out = String::new(); for piece in pieces { match piece { @@ -614,13 +656,23 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { mint.name ))); } - if let Some(at) = mint.selects_at.as_deref() - && at.trim().is_empty() - { - return Err(crate::error::UsageError::raise(format!( - "`[[mint]]` `{}` declares an empty `selects_at`", - mint.name - ))); + // BOTH HALVES, because only one was checked and the unchecked half is the + // one that fails silently: `selects = ""` loads clean, matches no value any + // host sends, and the row mints nothing — so its gate denies forever with a + // remedy that cannot clear it. A blank `selects_at` at least fails to + // resolve loudly. + for (column, value) in [ + ("selects_at", mint.selects_at.as_deref()), + ("selects", mint.selects.as_deref()), + ] { + if let Some(text) = value + && text.trim().is_empty() + { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares an empty `{column}`, which can never match", + mint.name + ))); + } } if let Err(problem) = parse(&mint.body) { return Err(crate::error::UsageError::raise(format!( diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index c3a1acf8f..c778e1170 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -7908,15 +7908,25 @@ fn git_facts(rules: &[Rule], root: &Path) -> crate::git::GitFacts { (1, false) => delta_bases .iter() .next() - .and_then(|base| { - // THE IDENTITY IS PAID FOR ONLY BY A ROW THAT DECLARES ITS - // CONSUMER. `patch-id` keys a `[[rule.minted]]` receipt and - // nothing else reads it, so a tree whose rules declare no - // minted row pays no merge-base walk — the same declaration - // economy every other git fact here already keeps. - let wants = rules.iter().any(|rule| !rule.minted.is_empty()); - crate::git::base_delta(root, base, &deltas, wants).ok() - }) + // RESOLVED UNCONDITIONALLY, AND THE OPTIMISATION THAT WAS HERE IS + // WITHDRAWN (CLOUD-1484). + // + // A review asked for this to be declaration-gated, as every other + // git fact here is, and the gating shipped keyed on + // `!rule.minted.is_empty()`. That is not the declaration that reads + // `patch-id`: a module may read the fact with no sibling + // `[[rule.minted]]` row, and it would then see `null`, decide + // nothing, and exit 0 — a dead gate bought to save a merge-base + // walk. Worse for the row that prompted it, where dropping the + // minted block would have silenced `code-review-dispatched` + // through two channels at once. + // + // The cost is real and is accepted rather than hidden: a second + // repository open, a merge-base walk and a tree diff, on every + // `check` declaring any `delta_sources`. Gating it honestly needs a + // column that says WHICH FACTS a row reads, which is a bigger + // change than this row and does not belong inside it. + .and_then(|base| crate::git::base_delta(root, base, &deltas, true).ok()) .flatten(), _ => None, }, diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego index e47f617c8..f9508beec 100644 --- a/policy/code-review-dispatched.rego +++ b/policy/code-review-dispatched.rego @@ -118,6 +118,15 @@ delta := input.tree["base-delta"] # Absent — a base that does not resolve, or an EMPTY diff — leaves every arm below # quiet. A branch that changed nothing has no identity, and reading that as # `unreviewed` would refuse a checkout with nothing to review. +# +# AND IT CAN UNDER-OWE, WHICH IS A NAMED HOLE RATHER THAN A PROPERTY. Committed +# bytes are the subject, so an uncommitted code edit does not move the identity: +# the receipt still answers and `batten check` reports clean over work no review +# has read. Not repaired here, and for measured reasons rather than for want of +# trying — a conjunct requiring a clean tree makes a DIRTY tree abstain, so +# dirtying the tree would silence this gate, and refusing on dirt is a second +# opinion where `tree-clean` already owns the question. `tree-clean` closes it on +# the landing path; a bare `batten check` on a dirty tree is what stays uncovered. subject := delta["patch-id"] # The path prefixes this repository treats as code for the purpose of owing a From be51f5f1053215e650689745b484f3e4ed7d8ee4 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 09/21] fix(mint): the null-result guard made the gate unclearable, and the tier could not see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth review found the defect that would have shipped this gate broken in production while every case here stayed green. `record_mints` OPENED WITH `if envelope.result.is_null() { return; }` — three lines above the comment this branch added saying a null result is NOT an early return. Both cannot be true, and the code won. A `Skill` post-tool whose host sends no `tool_response`, or sends `null`, took that return: no receipt written, `code-review-dispatched` denies, and the refusal names `/code-review` as the remedy for something the agent has already done. A gate whose stated remedy cannot clear it is worse than an absent one. THE TIER COULD NOT CATCH IT, WHICH IS THE PART WORTH RECORDING. `mint_receipts_for_test` enters BELOW this guard, so the compiled cases prove the writer and reader agree about a key and say nothing about whether the boundary is reached at all. That is the seam a test which drives the function directly always leaves: the cases were right and the entry point was not, and only a reader looking at the call site would have known. The guard is gated on `envelope.event != Event::PostTool` now, which is the question the line was actually asking. Nullness of the result was never that question, and reading it as such is what made the two statements disagree. `mint_receipts`' DOC COMMENT WAS ORPHANED by this branch's own test shim, which landed between the comment and the signature — so the boundary is undocumented and the helper carried four paragraphs describing it. Restored. `validate` REFUSED HALF A SYMMETRY. A `key_base` on a row not keyed `delta` is refused; a `key_from` on a row not keyed `named` was ignored, though it is read by nothing in exactly the same way. Both refused now, with a case pinning each direction. One finding stays a bound rather than a fix: `key = "delta"` accumulates one receipt per patch identity with no prune, and `minted::fields` reads every matching file on each `check`. Refs: CLOUD-1484 --- crates/batten/src/lib.rs | 65 +++++++++++++++++++++++---------------- crates/batten/src/mint.rs | 41 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 27 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index f546b83fd..5ff19dfdb 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -11237,6 +11237,28 @@ fn recover_spilled(result: &serde_json::Value) -> Option { facts::payload_in(&serde_json::from_str(&bytes).ok()?) } +/// [`mint_receipts`] reached from the integration tier (CLOUD-1484). +/// +/// **The boundary itself, never a re-implementation.** The half that WRITES a +/// receipt and the half that READS it must be shown to agree, and a test that +/// hand-writes the file proves the reader against an artifact the engine may not +/// produce — a different key base, separator or authority root leaves the gate +/// refusing forever with every case green. That is the `with input as` class one +/// layer over, so the tier calls the real function. +/// +/// `pub` and named for its caller rather than made generally available: nothing +/// in the binary reaches it, and the name says who it is for. +#[doc(hidden)] +pub fn mint_receipts_for_test( + declared: &[crate::mint::Declared], + tool: &str, + input: &serde_json::Value, + result: &serde_json::Value, + root: &Path, +) { + mint_receipts(declared, tool, input, result, root, None); +} + /// Write every receipt these rows mint from one already-unframed result. /// /// **ONE minting authority, reached from two boundaries** (CLOUD-1264). The @@ -11265,28 +11287,6 @@ fn recover_spilled(result: &serde_json::Value) -> Option { /// authority — [`mcp::payload`] for JSON-RPC content blocks, [`facts::payload_in`] /// for the harness envelope. Every failure is silent, as the mint boundary has /// always been: the gate that reads the receipt simply denies again. -/// [`mint_receipts`] reached from the integration tier (CLOUD-1484). -/// -/// **The boundary itself, never a re-implementation.** The half that WRITES a -/// receipt and the half that READS it must be shown to agree, and a test that -/// hand-writes the file proves the reader against an artifact the engine may not -/// produce — a different key base, separator or authority root leaves the gate -/// refusing forever with every case green. That is the `with input as` class one -/// layer over, so the tier calls the real function. -/// -/// `pub` and named for its caller rather than made generally available: nothing -/// in the binary reaches it, and the name says who it is for. -#[doc(hidden)] -pub fn mint_receipts_for_test( - declared: &[crate::mint::Declared], - tool: &str, - input: &serde_json::Value, - result: &serde_json::Value, - root: &Path, -) { - mint_receipts(declared, tool, input, result, root, None); -} - fn mint_receipts( declared: &[crate::mint::Declared], tool: &str, @@ -11399,11 +11399,22 @@ fn mint_receipts( } fn record_mints(overrides: &Overrides, envelope: &hook::Envelope) { - // Before the config load, the cheap question first: a post-tool event for a - // tool no row names — which is nearly all of them, now that batten is - // registered on every surface — must do no config work here. `perf-gate` - // holds the mediated path to a ratio, and this is the call that would move it. - if envelope.result.is_null() { + // Before the config load, the cheap question first: an event that is not the + // post-tool one must do no config work here — which is nearly every call, now + // that batten is registered on every surface. `perf-gate` holds the mediated + // path to a ratio, and this is the call that would move it. + // + // GATED ON THE EVENT, NEVER ON THE RESULT'S NULLNESS (CLOUD-1484). It was the + // latter, which read as the same question and is not: a post-tool event whose + // host sent no `tool_response`, or sent `null`, took the early return — so a + // row reading nothing FROM the result still minted nothing, and the paragraph + // below saying a null result is not an early return was false of the code + // immediately above it. For `code-review` that is a gate nothing can clear: + // the dispatch happens, no receipt is written, and the refusal names a remedy + // that has already been run. `mint_receipts_for_test` cannot see it either, + // because the tier enters below this guard — the one seam the compiled cases + // do not cross. + if envelope.event != hook::Event::PostTool { return; } // THE ENVELOPE IS THE SHAPE. A connector wraps every response in content diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 387c52a59..6ad55d877 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -645,6 +645,17 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { mint.name ))); } + // THE SYMMETRIC CASE, which the arm above left silent. A `key_from` on a + // row keyed `delta` or `branch` is read by nothing exactly as a `key_base` + // on a `named` one is — the same inert declaration, and refusing one while + // ignoring the other is the half-coverage this function exists to refuse. + if mint.key != MintKey::Named && mint.key_from.is_some() { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares a `key_from` and is not keyed `named`, so the \ + projection would be read by nothing", + mint.name + ))); + } // HALF A SELECTOR IS THE DANGEROUS HALF. `selects_at` alone would leave // the row minting on every result its tool matched while LOOKING // narrowed, which is the reading a reviewer takes from the column's @@ -742,6 +753,36 @@ mod tests { ); } + /// A `key_from` on a row keyed otherwise is refused, symmetrically with + /// `key_base`. + /// + /// Fails by: dropping the second arm. The first was landed alone, and the + /// asymmetry is the inert-declaration shape `validate` exists to refuse — a + /// column read by nothing, loading clean. + #[test] + fn a_projection_no_key_reads_is_refused_whichever_column_it_is() { + let named: Declared = serde_json::from_value(serde_json::json!({ + "name": "r", "tool": "T", "key": "named", "key_from": "id", + "mode": "replace", "body": "{id}", + })) + .expect("shape loads"); + assert!( + validate(&[named]).is_ok(), + "`key_from` on a `named` row is what it is for" + ); + + let delta: Declared = serde_json::from_value(serde_json::json!({ + "name": "r", "tool": "T", "key": "delta", "key_base": "origin/main", + "key_from": "id", "mode": "replace", "body": "{now}", + })) + .expect("shape loads"); + assert!( + validate(&[delta]).is_err(), + "a `key_from` a `delta` row never reads is the same inert declaration \ + the `key_base` arm already refuses" + ); + } + /// A row declaring NO selector is judged exactly as it was before the column /// existed, which is every landed row. #[test] From b0e7aea704b525492fe5f77ed81b7e6dd0ccb099 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:28:59 +0000 Subject: [PATCH 10/21] fix(mint): the guard added last round was unreachable, so it deleted the filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fifth review found that the previous round's repair was itself wrong, in the direction the previous round's repair was written to fix. `envelope.event != Event::PostTool` IS THE RIGHT QUESTION ASKED IN THE WRONG PLACE. `record_post_tool` is `record_mints`' only caller and `lib.rs:8319` already gates on exactly that, so the guard was dead code — and adding it DELETED the only cheap filter without replacing it, while the comment above claimed one was there. That is the same defect as the round before it, one layer over: a comment asserting what the code does not do, caught by a reader rather than a test. Both guards are gone. The event is decided at the call site, and the config load is paid on every post-tool event — the cost of admitting a dispatch whose result carried nothing, stated rather than bought back with a predicate that cannot express it. A cheaper filter would have to know which tools any row names before the rows are loaded, which is the thing being loaded. THE EMPTY-SELECTOR REFUSAL ONLY TRIMMED. `selects = "code-review "` loaded clean, compared against a value no host sends, and left the row minting nothing — the same silent dead gate the check was added to close, one space away. Refused rather than trimmed: trimming decides on the author's behalf which of two readings they meant. `mint_receipts_for_test` HARD-CODED `None` FOR THE GRAMMAR where the boundary passes one built from the resolved policy, so the helper whose entire justification is fidelity diverged from the boundary for `{authority:…}` pieces. It takes the parameter now, and the tier passes `None` explicitly with a note, so a case that adds such a piece has to decide rather than inherit. Refs: CLOUD-1484 --- crates/batten/src/lib.rs | 45 ++++++++++++------- crates/batten/src/mint.rs | 29 +++++++++++- .../batten/tests/it/code_review_dispatched.rs | 8 ++++ 3 files changed, 63 insertions(+), 19 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 5ff19dfdb..7182f6edd 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -11246,6 +11246,12 @@ fn recover_spilled(result: &serde_json::Value) -> Option { /// refusing forever with every case green. That is the `with input as` class one /// layer over, so the tier calls the real function. /// +/// **Every argument the boundary takes, including the grammar.** The helper's +/// whole justification is fidelity, so a parameter it supplied itself would be the +/// divergence it exists to remove: `record_mints` passes a grammar built from the +/// resolved policy, and a hard-coded `None` here would judge `{authority:…}` body +/// pieces differently from production while claiming to be the same call. +/// /// `pub` and named for its caller rather than made generally available: nothing /// in the binary reaches it, and the name says who it is for. #[doc(hidden)] @@ -11255,8 +11261,9 @@ pub fn mint_receipts_for_test( input: &serde_json::Value, result: &serde_json::Value, root: &Path, + grammar: Option<&ready::Grammar>, ) { - mint_receipts(declared, tool, input, result, root, None); + mint_receipts(declared, tool, input, result, root, grammar); } /// Write every receipt these rows mint from one already-unframed result. @@ -11399,24 +11406,28 @@ fn mint_receipts( } fn record_mints(overrides: &Overrides, envelope: &hook::Envelope) { - // Before the config load, the cheap question first: an event that is not the - // post-tool one must do no config work here — which is nearly every call, now - // that batten is registered on every surface. `perf-gate` holds the mediated - // path to a ratio, and this is the call that would move it. + // THERE IS NO CHEAP GATE HERE, AND THE TWO THAT WERE TRIED WERE BOTH WRONG + // (CLOUD-1484). // - // GATED ON THE EVENT, NEVER ON THE RESULT'S NULLNESS (CLOUD-1484). It was the - // latter, which read as the same question and is not: a post-tool event whose - // host sent no `tool_response`, or sent `null`, took the early return — so a - // row reading nothing FROM the result still minted nothing, and the paragraph - // below saying a null result is not an early return was false of the code - // immediately above it. For `code-review` that is a gate nothing can clear: + // It began `if envelope.result.is_null() { return; }`, which reads as "is this + // the post-tool event" and is not that question. A post-tool event whose host + // sent no `tool_response` took the return, so a row reading nothing FROM the + // result minted nothing — for `code-review` a gate nothing can clear, since // the dispatch happens, no receipt is written, and the refusal names a remedy - // that has already been run. `mint_receipts_for_test` cannot see it either, - // because the tier enters below this guard — the one seam the compiled cases - // do not cross. - if envelope.event != hook::Event::PostTool { - return; - } + // already run. + // + // The repair was `envelope.event != Event::PostTool`, which is the right + // question asked in the wrong place: `record_post_tool` is the only caller and + // it is already gated on exactly that at this function's call site. So the + // guard was unreachable, and adding it DELETED the only filter without + // replacing it while a comment claimed otherwise. + // + // Both are gone. The event is decided at the call site, and the config load is + // paid on every post-tool event — which is the cost of admitting a dispatch + // whose result carried nothing, stated here rather than bought back with a + // predicate that cannot express it. A cheaper filter would have to distinguish + // "no row names this tool" before the rows are known, which is the thing being + // loaded. // THE ENVELOPE IS THE SHAPE. A connector wraps every response in content // blocks, so reading fields off `envelope.result` directly matches nothing in // production while passing every fixture, which hands the engine a bare diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 6ad55d877..f59b8d80c 100644 --- a/crates/batten/src/mint.rs +++ b/crates/batten/src/mint.rs @@ -676,11 +676,19 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { ("selects_at", mint.selects_at.as_deref()), ("selects", mint.selects.as_deref()), ] { + // TRIMMED-EMPTY *AND* SURROUNDED BY WHITESPACE, because only the first + // was checked and the second fails in the same silent direction: + // `selects = "code-review "` loads clean, compares against a value no + // host sends, and the row mints nothing — so its gate denies forever + // with a remedy that cannot clear it. Refusing rather than trimming, + // because trimming would decide on the author's behalf which of two + // readings they meant. if let Some(text) = value - && text.trim().is_empty() + && (text.trim().is_empty() || text.trim() != text) { return Err(crate::error::UsageError::raise(format!( - "`[[mint]]` `{}` declares an empty `{column}`, which can never match", + "`[[mint]]` `{}` declares a `{column}` that is empty or padded, which can \ + never match", mint.name ))); } @@ -847,6 +855,23 @@ mod tests { validate(&[row(serde_json::json!({"selects_at": " ", "selects": "x"}))]).is_err(), "an empty `selects_at`" ); + // PADDING FAILS THE SAME WAY AS EMPTY, and only the empty half was + // checked: a padded value compares against nothing any host sends, so + // the row mints nothing and its gate denies forever. + assert!( + validate(&[row( + serde_json::json!({"selects_at": " skill", "selects": "x"}) + )]) + .is_err(), + "a padded `selects_at` resolves nothing" + ); + assert!( + validate(&[row( + serde_json::json!({"selects_at": "skill", "selects": "x "}) + )]) + .is_err(), + "a padded `selects` matches nothing" + ); } #[test] diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs index e63c763f4..48700b631 100644 --- a/crates/batten/tests/it/code_review_dispatched.rs +++ b/crates/batten/tests/it/code_review_dispatched.rs @@ -465,6 +465,10 @@ fn the_mint_boundary_writes_the_receipt_this_gate_reads() { &serde_json::json!({"skill": "code-review"}), &serde_json::Value::Null, &root, + // No body piece here reads the result, so the grammar is inert for these + // rows — passed explicitly so a case that adds an `{authority:…}` piece + // has to decide rather than inherit a hidden default. + None, ); assert!( @@ -499,6 +503,10 @@ fn another_skill_mints_nothing_through_the_boundary() { &serde_json::json!({"skill": "batten"}), &serde_json::Value::Null, &root, + // No body piece here reads the result, so the grammar is inert for these + // rows — passed explicitly so a case that adds an `{authority:…}` piece + // has to decide rather than inherit a hidden default. + None, ); assert_eq!( From 1a3b882fcaad113f30006982d88866a936939e9e Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:28 +0000 Subject: [PATCH 11/21] fix(policy): an unguarded base-delta faulted the whole bundle on a fresh clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delta := input.tree["base-delta"]` was dereferenced in `owed` and `subject` with no `is_object` guard, while its sibling `looked_at` guarded `input.tree.minted` for exactly that reason. `rules.rs` projects `base-delta` as `null` when the base ref does not resolve, indexing `null` is a hard evaluation fault in Rego rather than a silent miss, and a fault takes the whole bundle down — every predicate in it, not only this one. A fresh clone with no `origin/main` is the reachable instance, and the module's own metadata already says so about `minted`. Guarded, so an unresolvable base leaves `delta`, `owed` and `subject` undefined and every arm goes quiet — the could-not-look reading this module already takes for an absent identity. `test_a_delta_the_engine_could_not_build_does_not_fault` is the case; the load-time tier goes 757 → 758. Found by the code review this gate exists to demand, over its own diff. Admits: 4cdb9099ad184ad0242ff2b6f369c089d334cf1aee433a4c42b131f0b39ec09d Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: policy/code-review-dispatched.rego Admits-anchor: call:8bbc08550d75be92205e1037158331621a9352e0 Admits-epoch: af7bc401f010623937d6e9319c981c2b4d33dcf318281035ed45e57da25037a9 Admits-author: alec@wenzowski.com Admits-prev: - Admits-answer-lost: Without it, `delta := input.tree["base-delta"]` is dereferenced unguarded while its sibling `looked_at` guards `input.tree.minted` for exactly that reason. On a checkout whose base ref does not resolve the engine projects `base-delta` as null, indexing null is a hard evaluation fault in Rego, and the fault takes the whole bundle down — every predicate in it, not just this one. The gate this branch exists to add would be off on any fresh clone with no origin/main. Admits-answer-precondition: The change is a Rego predicate guard inside a policy module, and no declared surface can express it: batten.toml holds config rows, not module bodies, and git restore reverts rather than edits. Writing the module directly is the only route, and the guard lands in this branch's diff where a reviewer reads it. Admits-answer-rejected-route: `config read first` (batten.toml) does not apply: a `[[pattern]]` or path-set row cannot carry a Rego body, and the missing guard is a clause in the module, not a value the config supplies. `patch run first` (git restore) does not apply either: it restores committed bytes, and the committed bytes are the ones carrying the defect. Refs: CLOUD-1484 --- policy/code-review-dispatched.rego | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego index f9508beec..9a3c24743 100644 --- a/policy/code-review-dispatched.rego +++ b/policy/code-review-dispatched.rego @@ -106,7 +106,21 @@ looked_at(id) if { is_object(input.tree.minted[id]) } -delta := input.tree["base-delta"] +# The branch's own diff, as the engine resolved it. +# +# GUARDED ON `is_object` FOR THE SAME REASON `looked_at` IS, and the guard was +# missing here while its sibling carried one — the asymmetry the code review this +# gate demands is what found it. `rules.rs` projects `base-delta` as `null` when +# the base does not resolve, indexing `null` is a hard evaluation FAULT in Rego +# rather than a silent miss, and a fault takes the whole BUNDLE down — every +# predicate in it, not only this one. A fresh clone with no `origin/main` is the +# reachable instance. Undefined here leaves `owed` and `subject` undefined too, so +# every arm goes quiet, which is the could-not-look reading this module already +# takes for an absent identity. +delta := resolved if { + resolved := input.tree["base-delta"] + is_object(resolved) +} # The identity of the change this branch is asking to land. # @@ -319,6 +333,14 @@ test_a_change_with_no_identity_is_not_refused if { }) } +# A DELTA THE ENGINE COULD NOT BUILD, and without the `is_object` guard on +# `delta` this case does not merely fail — it FAULTS, taking every predicate in +# the bundle with it. `rules.rs` projects `null` here on a checkout whose base ref +# does not resolve, which a fresh clone with no `origin/main` is. +test_a_delta_the_engine_could_not_build_does_not_fault if { + count(violation) == 0 with input as attested_over(null) +} + attested_over(d) := {"tree": {"base-delta": d, "minted": {"code-review": {}}}} #MUTANT-SUITE crates/batten/tests/it/code_review_dispatched.rs From 677e101578dd8ecc514794063eebf4afed33f87c Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:28 +0000 Subject: [PATCH 12/21] feat(receipt): key a receipt to the branch's change, so a review can be owed at a transition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1547. The read side of `MintKey::Delta`: a `receipt` rule may now declare `key = "delta"` with a `key_base`, and its subject is the identity of this branch's whole change against that ref. The three existing keyings price a review at the wrong rate. `head` expires on every commit AND every rebase, so a loop that rebases per lap re-buys the receipt each lap for a change that did not move — the false-positive rate `branch`'s own doc says gets a guard bypassed. `branch` expires on nothing but a restart, so a branch reviewed once may then push anything. Neither is "these bytes, however they were replayed". `receipt::delta_subject` resolves through `git::branch_patch_id` — the same function the mint files under. That shared call is the agreement: a second derivation of "the same change" would be free to disagree with the mint's about exactly the rebase this keying exists to survive. Refused at load in both directions: `delta` with no `key_base` has no second side to diff, so it would read no file for any call and deny everything, which is the direction that gets a guard switched off; a `key_base` on any other keying is a column that reads as configured and is never consulted. An unresolvable base or an EMPTY diff answers `Missing` rather than could-not-look, which is the opposite of what the `branch` and `named` arms do and is deliberate: could-not-look allows, so a branch whose base does not resolve could push anything. A branch with nothing to review is refused loudly and cleared cheaply. `Policy::receipt_key_base_for` carries the prefix because `key_base_for` already means `requires_key`'s base — one ref for the whole call, answering since which commit evidence is looked for. Two refs, two questions; a reader reaching the wrong one gets a plausible value and the wrong receipt. `--key delta` is refused from `receipt status` as `--key named` is, one reason further on: the base is the declaring row's, and defaulting one would answer about a base no row named. Refs: CLOUD-1547 --- completions/batten.bash | 2 +- completions/batten.fish | 3 +- completions/batten.zsh | 3 +- crates/batten/src/config.rs | 1 + crates/batten/src/hook.rs | 48 +++++++++++++++++ crates/batten/src/lib.rs | 6 +++ crates/batten/src/receipt.rs | 96 ++++++++++++++++++++++++++++++++- crates/batten/src/rules.rs | 92 +++++++++++++++++++++++++++++++ man/batten-receipt-status.1 | 2 + schema/batten.local.schema.json | 12 +++++ schema/batten.schema.json | 12 +++++ 11 files changed, 273 insertions(+), 4 deletions(-) diff --git a/completions/batten.bash b/completions/batten.bash index 450ffef1c..b980f3e15 100644 --- a/completions/batten.bash +++ b/completions/batten.bash @@ -8394,7 +8394,7 @@ _batten() { fi case "${prev}" in --key) - COMPREPLY=($(compgen -W "head branch named" -- "${cur}")) + COMPREPLY=($(compgen -W "head branch named delta" -- "${cur}")) return 0 ;; --strictness) diff --git a/completions/batten.fish b/completions/batten.fish index 8c361628c..6a7d0c2cf 100644 --- a/completions/batten.fish +++ b/completions/batten.fish @@ -2557,7 +2557,8 @@ complete -c batten -n "__fish_batten_using_subcommand receipt; and __fish_seen_s complete -c batten -n "__fish_batten_using_subcommand receipt; and __fish_seen_subcommand_from record" -s h -l help -d 'Print help (see more with \'--help\')' complete -c batten -n "__fish_batten_using_subcommand receipt; and __fish_seen_subcommand_from status" -l key -d 'Which git fact the receipt is judged against: the exact commit, or the branch' -r -f -a "head\t'Keyed to the exact commit; an amend, a rebase, or a moved trunk expires it' branch\t'Keyed to the branch; every commit on it continues to serve the claim' -named\t'Keyed to a value the CALL names, read through [`Rule::key_from`] (CLOUD-987)'" +named\t'Keyed to a value the CALL names, read through [`Rule::key_from`] (CLOUD-987)' +delta\t'Keyed to the identity of the branch\'s whole CHANGE against [`Rule::key_base`] (CLOUD-1547) — the read side of [`crate::mint::MintKey::Delta`]'" complete -c batten -n "__fish_batten_using_subcommand receipt; and __fish_seen_subcommand_from status" -l strictness -d 'Raise how strictly gates apply (an override may only tighten policy)' -r -f -a "permissive\t'Advisory: findings are reported without failing the run' standard\t'The default: a finding is a violation' strict\t'Everything `Standard` fails on, plus anything advisory'" diff --git a/completions/batten.zsh b/completions/batten.zsh index b6d6b62e6..582643736 100644 --- a/completions/batten.zsh +++ b/completions/batten.zsh @@ -4280,7 +4280,8 @@ trace\:"Add everything"))' \ _arguments "${_arguments_options[@]}" : \ '--key=[Which git fact the receipt is judged against\: the exact commit, or the branch]: :((head\:"Keyed to the exact commit; an amend, a rebase, or a moved trunk expires it" branch\:"Keyed to the branch; every commit on it continues to serve the claim" -named\:"Keyed to a value the CALL names, read through \[\`Rule\:\:key_from\`\] (CLOUD-987)"))' \ +named\:"Keyed to a value the CALL names, read through \[\`Rule\:\:key_from\`\] (CLOUD-987)" +delta\:"Keyed to the identity of the branch'\''s whole CHANGE against \[\`Rule\:\:key_base\`\] (CLOUD-1547) — the read side of \[\`crate\:\:mint\:\:MintKey\:\:Delta\`\]"))' \ '--strictness=[Raise how strictly gates apply (an override may only tighten policy)]: :((permissive\:"Advisory\: findings are reported without failing the run" standard\:"The default\: a finding is a violation" strict\:"Everything \`Standard\` fails on, plus anything advisory"))' \ diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index 9fc8baa91..a2378b5b0 100644 --- a/crates/batten/src/config.rs +++ b/crates/batten/src/config.rs @@ -2980,6 +2980,7 @@ fn default_rules() -> Vec { when_present: None, when_value: None, key_from: None, + key_base: None, key_shape: None, max_age: None, requires_field: None, diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index ab69f34f7..251d64525 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -3451,6 +3451,47 @@ impl Policy { bounds } + /// The declared base ref, per check, for the `delta`-keyed rows this call + /// selects (CLOUD-1547). + /// + /// [`Policy::field_bound_for`]'s twin, empty for the same reason: a + /// repository declaring no `delta`-keyed row resolves no patch identity, and + /// resolving one costs a merge-base and a diff on the hottest path in the + /// binary. + /// + /// **First declaration wins where two rows disagree**, the same tie-break + /// `field_bound_for` takes and for its reason rather than `max_age`'s. Two + /// bases over one check are not two constraints that both hold — they are two + /// different identities for one change, so at most one receipt can exist and + /// combining them would make the check unsatisfiable. That is the config + /// error wearing a strict-policy costume, and declaration order is what every + /// other alternative on this surface breaks a tie by. + /// + /// **`receipt_` IS IN THE NAME BECAUSE [`Policy::key_base_for`] ALREADY MEANS + /// SOMETHING ELSE**, and the collision is worth the prefix rather than a + /// shorter name. That one answers `requires_key`'s question — since which + /// commit should this call's evidence be looked for — and returns one ref for + /// the whole call. This one answers which ref a `delta` receipt's identity is + /// diffed against, per check. Two refs, two questions, and a reader who + /// reached the wrong one would get a plausible value and a wrong receipt. + #[must_use] + pub fn receipt_key_base_for( + &self, + envelope: &Envelope, + ) -> std::collections::BTreeMap { + let mut bases: std::collections::BTreeMap = + std::collections::BTreeMap::new(); + for rule in matching_receipt_rows(self, envelope) { + let Some(base) = rule.key_base.as_ref() else { + continue; + }; + for check in rule.receipt_names() { + bases.entry(check.clone()).or_insert_with(|| base.clone()); + } + } + bases + } + /// Whether any row on this call could read the pinned-program fact /// (CLOUD-1028). /// @@ -5185,6 +5226,12 @@ fn receipt_refusal( ReceiptKey::Branch => "branch", ReceiptKey::Named => "row", ReceiptKey::Head => "commit", + // NOT "commit", which is the pointer this word exists to avoid sending. + // A delta receipt is keyed on the branch's whole change, so it survives a + // rebase and expires on a content edit — a reader told "commit" would go + // looking for a per-commit step and conclude the loop is re-buying one + // every lap, which is the opposite of what this keying does. + ReceiptKey::Delta => "change", }; // THE BOUND TRAVELS TOO, and only on the class it is the measure for. A // reader acting on an expiry needs to know what the age was measured @@ -9703,6 +9750,7 @@ mod tests { when_present: None, when_value: None, key_from: None, + key_base: None, key_shape: None, max_age: None, requires_field: None, diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 7182f6edd..37652933d 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -8427,6 +8427,12 @@ fn receipt_facts( receipt::verdicts( receipted, policy.named_receipt_subject(envelope).as_deref(), + // Resolved HERE beside the field bounds, for the identical reason + // the comment above gives: derivable from what this function already + // holds, read only on the branch that has a receipt store to ask, + // and a repository declaring no `delta`-keyed row gets an empty map + // and pays no merge-base. + &policy.receipt_key_base_for(envelope), max_ages, &policy.field_bound_for(envelope), now, diff --git a/crates/batten/src/receipt.rs b/crates/batten/src/receipt.rs index 464da3309..6f5ac225d 100644 --- a/crates/batten/src/receipt.rs +++ b/crates/batten/src/receipt.rs @@ -597,6 +597,15 @@ pub(crate) fn sourced_store( ReceiptKey::Head => head.clone()?, ReceiptKey::Branch => branch.clone()?, ReceiptKey::Named => named.clone()?, + // `delta` over an agent-sourced check is REFUSED AT LOAD + // (`facts::validate_keying`) for the reason `named` is, one step + // further along: the identity needs the row's `key_base`, and a + // `[[fact]]` row has no such column to read one from. So this is + // reachable only from a policy assembled in-process, and it answers + // could-not-look rather than fabricating a subject — written out + // rather than wildcarded, because `_ =>` would silently absorb a + // FIFTH keying, which is what the no-wildcard rule above is for. + ReceiptKey::Delta => return None, }; subjects.insert((*check).clone(), subject); } @@ -619,6 +628,12 @@ fn sourced_subject(key: ReceiptKey, named: Option<&str>) -> Option { ReceiptKey::Named => named .filter(|value| safe_subject(value)) .map(ToOwned::to_owned), + // Refused at load for an agent-sourced check, as `named` is and for one + // reason further on: the identity is taken against the row's `key_base` + // and a `[[fact]]` row carries none. Could-not-look here, never a + // fabricated subject — a write filed under a subject the reader resolves + // differently is the confusion every keying in this enum exists to avoid. + ReceiptKey::Delta => None, } } @@ -692,23 +707,62 @@ pub(crate) fn safe_subject(subject: &str) -> bool { && !subject.contains(|ch: char| ch == '/' || ch == '\\' || ch == '\0' || ch.is_control()) } +/// The subject a [`ReceiptKey::Delta`] receipt is filed under (CLOUD-1547). +/// +/// **One identity, read through the function the mint writes under.** +/// [`crate::git::branch_patch_id`] is a merge-base diff over COMMITTED bytes with +/// line numbers excluded, so a rebase that changes no content resolves to the +/// same name and one changed line resolves to a different one. Re-deriving that +/// here — from a tip diff, from the path lists, from anything else — would be a +/// second notion of *the same change*, free to disagree with the mint's about +/// exactly the rebase this keying exists to survive. The mint calls the same +/// function; that shared call IS the agreement. +/// +/// `None` is three things at once and they are one answer here: the base does not +/// resolve, the identity could not be computed, and **the diff is empty**. A +/// branch that changed nothing has no identity — `cumulative_patch_id` refuses to +/// mint one so two empty changes cannot compare equal — and the caller reads that +/// as [`Validity::Missing`] rather than as could-not-look, for the reason stated +/// at the call site. +/// +/// Held to [`safe_subject`] before it is returned, because it is about to become +/// a path component and the writer refuses exactly what the reader refuses — the +/// two halves disagreeing about which filenames exist is the confusion +/// [`safe_subject`]'s own doc records. +fn delta_subject(base: &str) -> Option { + git::branch_patch_id(Path::new("."), base) + .ok() + .flatten() + .filter(|identity| safe_subject(identity)) +} + /// Where a receipt for `check` under `key` lives, so its age can be read. /// -/// One spelling per keying, taken from the three validity functions rather than +/// One spelling per keying, taken from the four validity functions rather than /// invented here — a second spelling of a receipt filename is a second thing to /// drift, and the store this reads has to be the store they read. +/// +/// `delta` is the already-resolved patch identity rather than a base ref this +/// would resolve itself (CLOUD-1547): the caller has it, and re-deriving it here +/// would be a second reading of the one identity the mint filed under. fn receipt_file( facts: &RepoFacts, check: &str, key: ReceiptKey, branch: Option<&str>, named: Option<&str>, + delta: Option<&str>, ) -> Option { let store = Path::new(&facts.git_dir).join("batten-receipts"); match key { ReceiptKey::Head => receipt_path(&facts.repo_root, check).ok(), ReceiptKey::Branch => branch.map(|branch| store.join(branch_receipt_name(check, branch))), + // One filename shape for both, deliberately: a `delta` subject is a + // patch identity and a `named` subject is a value the call supplied, and + // both are held to `safe_subject` before they get here, so the store has + // one spelling for *a receipt filed under a subject* rather than two. ReceiptKey::Named => named.map(|subject| store.join(format!("{check}.{subject}"))), + ReceiptKey::Delta => delta.map(|subject| store.join(format!("{check}.{subject}"))), } } @@ -737,6 +791,7 @@ fn older_than(path: &Path, max_age: u64, now: std::time::SystemTime) -> bool { pub(crate) fn verdicts( checks: &BTreeMap, subject: Option<&str>, + key_bases: &BTreeMap, max_ages: &BTreeMap, field_bounds: &BTreeMap, now: std::time::SystemTime, @@ -787,6 +842,25 @@ pub(crate) fn verdicts( ReceiptKey::Named => named.as_ref().map_or(Validity::Missing, |value| { named_validity(&facts.git_dir, check, value) }), + // NOT resolved once above like `branch` and `named`, because + // the base is PER ROW rather than per call: two rows may key + // on different bases, and hoisting one identity would answer + // both from whichever row happened to be read first. + // + // AND `Missing` RATHER THAN COULD-NOT-LOOK, which is the + // opposite of the two arms above and is the deliberate half + // (CLOUD-1547). An unresolvable base or an empty diff means + // there is no change to have reviewed; answering could-not- + // look would allow the call, so a branch whose base does not + // resolve could push anything. A branch with nothing to + // review is refused and says so, which is loud and cheap to + // clear, where the permissive direction is silent. + ReceiptKey::Delta => key_bases + .get(check) + .and_then(|base| delta_subject(base)) + .map_or(Validity::Missing, |identity| { + named_validity(&facts.git_dir, check, &identity) + }), }; // THE AGE IS READ LAST, AND ONLY OVER A RECEIPT THAT WAS // OTHERWISE GOOD (CLOUD-988). A receipt already Missing or stale @@ -801,6 +875,10 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), + key_bases + .get(check) + .and_then(|base| delta_subject(base)) + .as_deref(), ) .filter(|path| older_than(path, max_age, now)) .map_or(Validity::Valid, |_| Validity::Expired), @@ -820,6 +898,10 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), + key_bases + .get(check) + .and_then(|base| delta_subject(base)) + .as_deref(), ) .map_or(Validity::Valid, |path| { if field_refutes(&path, bound) { @@ -1504,6 +1586,17 @@ pub fn run_status( "receipt status --key named: a named receipt is keyed on a subject the mediated call supplies, and this verb has none to give. It is read by `batten hook` from the declaring row's `key_from` projection.", )); } + // REFUSED FROM THIS VERB TOO, and for a reason one step past `named`'s + // (CLOUD-1547). A `delta` receipt's subject is an identity taken against + // the DECLARING ROW's `key_base`, and this verb is handed a check name + // and a keying — nothing that says which ref. Defaulting one would answer + // about a base the row never named, and two bases produce two identities + // for one change, which is the disagreement this keying exists to remove. + ReceiptKey::Delta => { + return Err(UsageError::raise( + "receipt status --key delta: a delta receipt is keyed on the identity of this branch's change against the declaring row's `key_base`, and this verb has no row to read one from. It is read by `batten hook` from that row.", + )); + } ReceiptKey::Head => { let statement = load_statement(&receipt_path(&facts.repo_root, check)?); ( @@ -1595,6 +1688,7 @@ const fn key_token(key: ReceiptKey) -> &'static str { ReceiptKey::Head => "head", ReceiptKey::Branch => "branch", ReceiptKey::Named => "named", + ReceiptKey::Delta => "delta", } } diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index c778e1170..ebc697438 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -1336,6 +1336,20 @@ pub struct Rule { /// set somebody enumerated. #[serde(default, skip_serializing_if = "Option::is_none")] pub key_from: Option, + /// Which ref a [`ReceiptKey::Delta`] receipt's identity is taken against + /// (CLOUD-1547) — the read-side twin of [`crate::mint::Declared::key_base`]. + /// + /// Required by that key and refused without it, and refused ON any other: + /// the identity is a diff and a diff needs two sides, so a `delta` row with + /// no base has no subject, and a base on a `head` row is a column the engine + /// would silently never read. + /// + /// A CONSUMER's ref name, so it lives on the row rather than as a constant in + /// the crate (non-negotiable rule 1): which branch a repository lands on is + /// that repository's business, and `must_land_on` already answers it for a + /// different question rather than for this one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub key_base: Option, /// Narrow [`Rule::when_present`] from *the key is there* to *the key holds /// THIS value* (CLOUD-312 row 3). /// @@ -2604,6 +2618,36 @@ pub enum ReceiptKey { /// silently collide two subjects onto one receipt, which is the one outcome /// worse than not looking. Named, + /// Keyed to the identity of the branch's whole CHANGE against + /// [`Rule::key_base`] (CLOUD-1547) — the read side of [`crate::mint::MintKey::Delta`]. + /// + /// **The fourth keying, and it exists because the other three price a review + /// at the wrong rate.** [`ReceiptKey::Head`] expires on every commit AND on + /// every rebase, so a landing loop that rebases per lap re-buys the receipt + /// each lap for a change that did not move — the false-positive rate + /// [`ReceiptKey::Branch`]'s own doc says gets a guard bypassed. + /// [`ReceiptKey::Branch`] goes the other way and expires on nothing but a + /// restart, so a branch reviewed once may then push anything. Neither is + /// *these bytes, however they were replayed*. + /// + /// The subject is [`crate::git::branch_patch_id`] — a merge-base diff over + /// COMMITTED bytes with line numbers excluded — so a rebase that changes no + /// content resolves to the same name and the receipt still answers, while a + /// single changed line files it under a name nothing looks up. + /// + /// **Read through the one identity the mint writes under, never re-derived.** + /// A second notion of *the same change* is free to disagree with the first + /// about exactly the rebase this keying turns on, which is the class + /// `.claude/rules/policy-modules.md` records for `base-delta` and the reason + /// `receipt::delta_subject` and `mint::render` resolve through the same + /// function rather than through two readings of one rule. + /// + /// An identity that does not resolve — no `key_base`, an unresolvable base + /// ref, or an EMPTY diff — is could-not-look, and a rule keyed on it answers + /// [`crate::receipt::Validity::Missing`] rather than fabricating a subject. A branch that + /// changed nothing has no identity, and minting one would let two empty + /// changes compare equal. + Delta, } /// One field of a receipt, and what it must say (CLOUD-1100). @@ -3887,6 +3931,51 @@ impl Rule { /// A [`UsageError`] (→ exit `1`) for a command-triggered row with no /// `pattern`, for a write-triggered row carrying either command column, and /// for an empty `checks` list. + /// The `delta` key and its base travel together (CLOUD-1547). + /// + /// Refused in BOTH directions, for the reason `key_from`'s pair is and with + /// the same pair of silent failures one keying over. + /// + /// A `delta` key with no base has no second side to diff against, so it has + /// no identity at all and would read no file for any call — which reads as + /// *never reviewed* and denies everything. That is the direction that gets a + /// guard switched off rather than satisfied, and it is worse than the + /// permissive one here precisely because it looks like the gate working. A + /// base on any other key is the mirror: a column that reads as configured and + /// is never consulted. + /// + /// **Its own function rather than a fourth arm inside + /// [`Rule::validate_receipt_columns`]**, which clippy's line ceiling forced + /// and which is the right shape anyway: that one decides what a `trigger` + /// owes, and this decides what a `key` owes. + /// + /// # Errors + /// + /// A [`UsageError`] (→ exit `1`) for `key = "delta"` with no `key_base`, and + /// for a `key_base` on any other keying. + fn validate_delta_base(&self) -> anyhow::Result<()> { + match (self.receipt_key(), self.key_base.as_deref()) { + (ReceiptKey::Delta, None) => Err(UsageError::raise(format!( + "rule {}: `key = \"delta\"` requires `key_base` — the ref the identity is taken \ + against. Without one the change has no second side to diff, so the receipt \ + would be keyed on nothing and no call could ever satisfy it", + self.id + ))), + (key, Some(_)) if key != ReceiptKey::Delta => Err(UsageError::raise(format!( + "rule {}: `key_base` belongs to `key = \"delta\"`; a {} receipt is keyed on a ref \ + or a value rather than on a diff, so there is nothing to take a base against", + self.id, + match key { + ReceiptKey::Head => "head-keyed", + ReceiptKey::Branch => "branch-keyed", + ReceiptKey::Named => "named-keyed", + ReceiptKey::Delta => unreachable!("guarded by the arm above"), + } + ))), + _ => Ok(()), + } + } + fn validate_receipt_columns(&self) -> anyhow::Result<()> { if self.kind != RuleKind::Receipt { return Ok(()); @@ -3955,11 +4044,13 @@ impl Rule { ReceiptKey::Head => "head-keyed", ReceiptKey::Branch => "branch-keyed", ReceiptKey::Named => unreachable!("guarded by the arm above"), + ReceiptKey::Delta => "delta-keyed", } ))); } _ => {} } + self.validate_delta_base()?; // COMPILED AT LOAD, for the reason `resolves.reference` is and with the // failure running the same direction: left to adjudication an unparseable // expression is discarded per call, the subject resolves to absent, @@ -13501,6 +13592,7 @@ mod tests { when_present: None, when_value: None, key_from: None, + key_base: None, key_shape: None, max_age: None, requires_field: None, diff --git a/man/batten-receipt-status.1 b/man/batten-receipt-status.1 index d46e6b908..a369ce636 100644 --- a/man/batten-receipt-status.1 +++ b/man/batten-receipt-status.1 @@ -22,6 +22,8 @@ head: Keyed to the exact commit; an amend, a rebase, or a moved trunk expires it branch: Keyed to the branch; every commit on it continues to serve the claim .IP \(bu 2 named: Keyed to a value the CALL names, read through [`Rule::key_from`] (CLOUD\-987) +.IP \(bu 2 +delta: Keyed to the identity of the branch\*(Aqs whole CHANGE against [`Rule::key_base`] (CLOUD\-1547) — the read side of [`crate::mint::MintKey::Delta`] .RE .TP \fB\-J\fR, \fB\-\-json\fR diff --git a/schema/batten.local.schema.json b/schema/batten.local.schema.json index 06c3c532a..412bbd236 100644 --- a/schema/batten.local.schema.json +++ b/schema/batten.local.schema.json @@ -606,6 +606,11 @@ "description": "Keyed to a value the CALL names, read through [`Rule::key_from`]\n(CLOUD-987).\n\n**The subject is one row of somebody's board, not one checkout**, and that\nis the whole reason this variant exists. CLOUD-312's row 2 bounds how stale\na read may be before a write is authorised, and `issue-read-check`'s header\nsays why the key cannot be the branch: *\"a branch legitimately updates\nseveral issues; a branch key would let a fresh read of one issue authorise\na stale write to another.\"* So collapsing this to [`ReceiptKey::Branch`]\nis not a simplification — it is the defect that comment refuses, and\nCLOUD-508 is the incident.\n\n**The value becomes a filename, so it is refused rather than sanitised.**\n[`SinkKey`]'s doc already states the hazard for a config-spelled key; a\nPAYLOAD-supplied one is strictly worse, because the call is what a rule is\njudging. A value that is not a single safe path component — empty, or\ncarrying a separator, or `.`/`..`, or absurdly long — resolves to\ncould-not-look and **allows**, rather than being rewritten into something\nthat would file under a subject the caller did not name. Rewriting could\nsilently collide two subjects onto one receipt, which is the one outcome\nworse than not looking.", "type": "string", "const": "named" + }, + { + "description": "Keyed to the identity of the branch's whole CHANGE against\n[`Rule::key_base`] (CLOUD-1547) — the read side of [`crate::mint::MintKey::Delta`].\n\n**The fourth keying, and it exists because the other three price a review\nat the wrong rate.** [`ReceiptKey::Head`] expires on every commit AND on\nevery rebase, so a landing loop that rebases per lap re-buys the receipt\neach lap for a change that did not move — the false-positive rate\n[`ReceiptKey::Branch`]'s own doc says gets a guard bypassed.\n[`ReceiptKey::Branch`] goes the other way and expires on nothing but a\nrestart, so a branch reviewed once may then push anything. Neither is\n*these bytes, however they were replayed*.\n\nThe subject is [`crate::git::branch_patch_id`] — a merge-base diff over\nCOMMITTED bytes with line numbers excluded — so a rebase that changes no\ncontent resolves to the same name and the receipt still answers, while a\nsingle changed line files it under a name nothing looks up.\n\n**Read through the one identity the mint writes under, never re-derived.**\nA second notion of *the same change* is free to disagree with the first\nabout exactly the rebase this keying turns on, which is the class\n`.claude/rules/policy-modules.md` records for `base-delta` and the reason\n`receipt::delta_subject` and `mint::render` resolve through the same\nfunction rather than through two readings of one rule.\n\nAn identity that does not resolve — no `key_base`, an unresolvable base\nref, or an EMPTY diff — is could-not-look, and a rule keyed on it answers\n[`crate::receipt::Validity::Missing`] rather than fabricating a subject. A branch that\nchanged nothing has no identity, and minting one would let two empty\nchanges compare equal.", + "type": "string", + "const": "delta" } ] }, @@ -1050,6 +1055,13 @@ } ] }, + "key_base": { + "description": "Which ref a [`ReceiptKey::Delta`] receipt's identity is taken against\n(CLOUD-1547) — the read-side twin of [`crate::mint::Declared::key_base`].\n\nRequired by that key and refused without it, and refused ON any other:\nthe identity is a diff and a diff needs two sides, so a `delta` row with\nno base has no subject, and a base on a `head` row is a column the engine\nwould silently never read.\n\nA CONSUMER's ref name, so it lives on the row rather than as a constant in\nthe crate (non-negotiable rule 1): which branch a repository lands on is\nthat repository's business, and `must_land_on` already answers it for a\ndifferent question rather than for this one.", + "type": [ + "string", + "null" + ] + }, "key_from": { "description": "Which projection supplies the subject for a [`ReceiptKey::Named`] receipt\n(CLOUD-987).\n\nRequired by that key and refused without it: a `named` receipt with no\nprojection has no subject to file under, and a receipt keyed on nothing\nwould read the same file for every call — which is\n[`ReceiptKey::Branch`] wearing a different name, and the exact collapse\nthat variant's doc refuses.\n\nA [`crate::hook::Field`] rather than a free-form key, for the reason the\nallowlist exists: the subject of a receipt is about to become a path\ncomponent under `$GIT_DIR`, so which values can reach it must be a closed\nset somebody enumerated.", "anyOf": [ diff --git a/schema/batten.schema.json b/schema/batten.schema.json index 127f53325..c4fff1003 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -2730,6 +2730,11 @@ "description": "Keyed to a value the CALL names, read through [`Rule::key_from`]\n(CLOUD-987).\n\n**The subject is one row of somebody's board, not one checkout**, and that\nis the whole reason this variant exists. CLOUD-312's row 2 bounds how stale\na read may be before a write is authorised, and `issue-read-check`'s header\nsays why the key cannot be the branch: *\"a branch legitimately updates\nseveral issues; a branch key would let a fresh read of one issue authorise\na stale write to another.\"* So collapsing this to [`ReceiptKey::Branch`]\nis not a simplification — it is the defect that comment refuses, and\nCLOUD-508 is the incident.\n\n**The value becomes a filename, so it is refused rather than sanitised.**\n[`SinkKey`]'s doc already states the hazard for a config-spelled key; a\nPAYLOAD-supplied one is strictly worse, because the call is what a rule is\njudging. A value that is not a single safe path component — empty, or\ncarrying a separator, or `.`/`..`, or absurdly long — resolves to\ncould-not-look and **allows**, rather than being rewritten into something\nthat would file under a subject the caller did not name. Rewriting could\nsilently collide two subjects onto one receipt, which is the one outcome\nworse than not looking.", "type": "string", "const": "named" + }, + { + "description": "Keyed to the identity of the branch's whole CHANGE against\n[`Rule::key_base`] (CLOUD-1547) — the read side of [`crate::mint::MintKey::Delta`].\n\n**The fourth keying, and it exists because the other three price a review\nat the wrong rate.** [`ReceiptKey::Head`] expires on every commit AND on\nevery rebase, so a landing loop that rebases per lap re-buys the receipt\neach lap for a change that did not move — the false-positive rate\n[`ReceiptKey::Branch`]'s own doc says gets a guard bypassed.\n[`ReceiptKey::Branch`] goes the other way and expires on nothing but a\nrestart, so a branch reviewed once may then push anything. Neither is\n*these bytes, however they were replayed*.\n\nThe subject is [`crate::git::branch_patch_id`] — a merge-base diff over\nCOMMITTED bytes with line numbers excluded — so a rebase that changes no\ncontent resolves to the same name and the receipt still answers, while a\nsingle changed line files it under a name nothing looks up.\n\n**Read through the one identity the mint writes under, never re-derived.**\nA second notion of *the same change* is free to disagree with the first\nabout exactly the rebase this keying turns on, which is the class\n`.claude/rules/policy-modules.md` records for `base-delta` and the reason\n`receipt::delta_subject` and `mint::render` resolve through the same\nfunction rather than through two readings of one rule.\n\nAn identity that does not resolve — no `key_base`, an unresolvable base\nref, or an EMPTY diff — is could-not-look, and a rule keyed on it answers\n[`crate::receipt::Validity::Missing`] rather than fabricating a subject. A branch that\nchanged nothing has no identity, and minting one would let two empty\nchanges compare equal.", + "type": "string", + "const": "delta" } ] }, @@ -3352,6 +3357,13 @@ } ] }, + "key_base": { + "description": "Which ref a [`ReceiptKey::Delta`] receipt's identity is taken against\n(CLOUD-1547) — the read-side twin of [`crate::mint::Declared::key_base`].\n\nRequired by that key and refused without it, and refused ON any other:\nthe identity is a diff and a diff needs two sides, so a `delta` row with\nno base has no subject, and a base on a `head` row is a column the engine\nwould silently never read.\n\nA CONSUMER's ref name, so it lives on the row rather than as a constant in\nthe crate (non-negotiable rule 1): which branch a repository lands on is\nthat repository's business, and `must_land_on` already answers it for a\ndifferent question rather than for this one.", + "type": [ + "string", + "null" + ] + }, "key_from": { "description": "Which projection supplies the subject for a [`ReceiptKey::Named`] receipt\n(CLOUD-987).\n\nRequired by that key and refused without it: a `named` receipt with no\nprojection has no subject to file under, and a receipt keyed on nothing\nwould read the same file for every call — which is\n[`ReceiptKey::Branch`] wearing a different name, and the exact collapse\nthat variant's doc refuses.\n\nA [`crate::hook::Field`] rather than a free-form key, for the reason the\nallowlist exists: the subject of a receipt is about to become a path\ncomponent under `$GIT_DIR`, so which values can reach it must be a closed\nset somebody enumerated.", "anyOf": [ From 8ac16c9be598740400fb0c492e1b82d460a07a2f Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:28 +0000 Subject: [PATCH 13/21] feat(policy): price the code review at the ready transition, not per commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1547. `code-review-dispatched` was a tree rule, so `batten check` decided it under `verify` — which is what an agent runs repeatedly WHILE iterating. Every fix commit moved the patch identity and owed another review, and the review that found something produced the fix that owed the next one. Measured on the branch that built it: six dispatches in one session, each provoked by the fix for the last, each charged over bytes the deterministic gates had not judged yet. The keying was right; the enforcement POINT was wrong. `ready-needs-review` is a `delta`-keyed `receipt` row over `gh pr ready` — the event that spends CI minutes. A rebase-only lap resolves the same identity and buys nothing; a content change files the next review under a name the row does not look up. The module, its compiled tier and its `MUTANT_GATES` entry are deleted, and `patch read never` is withdrawn naming the successor rather than left orphaned. Its route is kept verbatim: what satisfies the demand is unchanged, only where the demand is made. `crates/batten/tests/it/review_receipt_delta.rs` is the tier, over real `adjudicate` calls. The rebase and content-change cases are an anti-vacuity pair in the strict sense — a `head` keying fails the first and a `branch` keying fails the second — so a suite carrying only one is satisfied by the wrong column. Two things this does NOT do, written into `batten.toml` rather than discovered: `push-needs-review` is not committed. It is what would freeze the readied head, and pricing to a transition is only sound if the head cannot change under it. A `receipt` row selects on a command pattern, so it fires on every push in the repository, while what it wants to refuse is a push to a branch whose PR is already ready — and `forge.rs` carries no draft field, so that narrowing is unsayable. Measured: it denied `forced_push.rs`'s benign fixture push, and 15 test files drive a push unrelated to review. The gap is CLOUD-1446's and closes in CLOUD-1548, where a declared loop can name the ready state. The tier keeps a push case over its own config, so the keying is shown not to be pattern-specific. And retiring the module widens the gate. It gated on `code-changed` intersected with the reviewable prefixes, so a prose-only branch owed nothing; a `receipt` row has no such column, so a documentation-only branch now buys one review before readying. Deliberate and cheap at one dispatch per branch, but a widening. `key_base` is classified in the column census as not fact-bearing — an unclassified column is declarable on a mediated row and read by nothing, which is what that census exists to refuse. Admits: eb43570d9ba0d9204b29ca2eb2d34cd74707723ab246201cb0e06a0db187cc7f Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:5ffde3395ad0846c5a344da6047ff073406c4505 Admits-epoch: af7bc401f010623937d6e9319c981c2b4d33dcf318281035ed45e57da25037a9 Admits-author: alec@wenzowski.com Admits-prev: 486f8518288e82b9b457c9df8dd28a0d34b8f705466d0b2952eb2b6f7e1953b6 Admits-answer-lost: Without it CLOUD-1547 has an engine keying nothing declares. `ReceiptKey::Delta` would be a column no row uses — the dead-gate class this repository records, shipped deliberately rather than by accident — and `code-review-dispatched` would go on pricing a review per commit on the tree surface, which is the defect the keying was built to remove and which cost six model calls in one session on this branch alone. Admits-answer-precondition: The change is two `[[rule]]` rows and a `[[verdict]]` withdrawal in the policy authority itself, plus dropping the retired `[[rule]]` and its module registration. No declared surface can express a rule row: batten.toml IS the surface, `batten init` refuses an existing file, and `git restore` reverts rather than edits. Writing it directly is the only route, and the rows land in this branch's diff where a reviewer reads them. Admits-answer-rejected-route: `config read first` (batten.toml) is the path being written, so it names itself and resolves to nothing. `patch run first` (git restore) does not apply: it restores committed bytes, and the committed bytes are the ones carrying the per-commit pricing. Refs: CLOUD-1547 --- batten.toml | 78 ++- crates/batten/src/rules.rs | 10 +- .../batten/tests/it/code_review_dispatched.rs | 543 ------------------ crates/batten/tests/it/main.rs | 2 +- .../batten/tests/it/review_receipt_delta.rs | 320 +++++++++++ policy/code-review-dispatched.rego | 350 ----------- 6 files changed, 394 insertions(+), 909 deletions(-) delete mode 100644 crates/batten/tests/it/code_review_dispatched.rs create mode 100644 crates/batten/tests/it/review_receipt_delta.rs delete mode 100644 policy/code-review-dispatched.rego diff --git a/batten.toml b/batten.toml index a2a3d4219..1da335da7 100644 --- a/batten.toml +++ b/batten.toml @@ -5928,21 +5928,64 @@ subject = "tracker-body" # CLOUD-877 gives the receipt a portable form. That is stated here rather than # discovered: today this gate is enforced where `land` runs, and CI is honestly # silent rather than falsely green. -[[rule]] -id = "code-review-dispatched" -kind = "policy" -scope = "tree" -base = "origin/main" -delta_sources = ["crates/**", "policy/**", "mise-tasks/**", "batten.toml"] -module = "policy/code-review-dispatched.rego" +# +# RETIRED ONTO THE TRANSITION, AND THE TREE ROW IS GONE (CLOUD-1547). The keying +# above is right and the ENFORCEMENT POINT was wrong: a tree row is decided by +# `batten check` under `verify`, which is what an agent runs repeatedly WHILE +# iterating, so every fix commit moved the identity and owed another review, and +# the review that found something produced the fix that owed the next one. +# Measured on the branch that built it: six dispatches in one session, each +# provoked by the fix for the last, each charged over bytes the deterministic +# gates had not judged yet. +# +# The row below is the successor, and it is ONE row where the design wanted two. +# +# WHAT IS NOT HERE, AND WHY IT IS NOT: a `push-needs-review` row over `git push`. +# It is what would FREEZE the readied head — pricing a review to the ready +# transition is only sound if the head cannot change under it, or the reviewed +# diff is readied and an unreviewed one is pushed on top. Written and measured, +# and it is too blunt to commit: a `receipt` row selects on a command pattern, so +# it fires on EVERY push in the repository, and the thing it actually wants to +# refuse is a push to a branch whose PR is already ready. `forge.rs` carries no +# draft field at all, so that narrowing is unsayable today. Measured over the +# suite: the row denied `forced_push.rs`'s benign fixture push, and 15 test files +# drive a push that has nothing to do with review. +# +# So the freeze stays an open gap rather than a widened gate — the same gap +# CLOUD-1446 already records for `ready-needs-receipts` ("nothing guards the next +# push"), and CLOUD-1548 is where it closes, because a declared loop can name the +# ready STATE where a command pattern cannot. The keying itself is not +# pattern-specific and `review_receipt_delta.rs` exercises the push arm over its +# own config, so what is missing here is the narrowing, not the mechanism. +# +# THE OTHER THING THIS ROW CANNOT CARRY, stated rather than discovered: the +# retired module gated on `code-changed` intersected with `crates/`, `policy/`, +# `mise-tasks/` and `batten.toml`, so a prose-only branch owed nothing. A +# `receipt` row has no such column, so a documentation-only branch now buys one +# review before readying. That is a widening, it is deliberate, and it is cheap at +# one dispatch per branch — but it is a widening and not an oversight. + +# THE READY TRANSITION, which is the event that spends CI minutes. +# +# The sibling of `ready-needs-receipts` rather than a column on it: that row is +# `key = "head"` and asks whether THIS COMMIT was verified, which is the right +# keying for `verify` and the wrong one for a review — a head-keyed review would +# expire on every rebase and re-buy a model call each lap, the false-positive rate +# that gets a guard switched off. One row cannot carry two keyings, and the two +# questions genuinely have different answers. +[[rule]] +id = "ready-needs-review" +kind = "receipt" +scope = "mediated_call" severity = "deny" - -[[rule.minted]] -id = "code-review" -mint = "code-review" -field = 0 -recency = 1 -max_age_days = 30 +pattern = "gh pr ready" +checks = ["code-review"] +key = "delta" +key_base = "origin/main" +reason = """ +Dispatch the `code-review` skill over this branch's change, then retry. Readying \ +is what starts CI, so an unreviewed head spends the matrix before anyone has \ +looked at it.""" # A third-party validator's verdict for a file it read (CLOUD-1171, the # adjudication half of `pkl-check`, `renovate-config-validator` and @@ -11545,6 +11588,7 @@ fresh clone is could-not-look, not a refusal: the receipt store dies with the \ container, so this speaks where the branch is worked and is honestly silent in \ CI until that store has a portable form. """ +withdrawn = "CLOUD-1547 retired the tree rule that raised this. The question it asked is now asked at the two transitions that matter — `push-needs-review` and `ready-needs-review` — which are `receipt` rows and raise the `receipt read …` classes instead. The change is the enforcement POINT, not the predicate: a tree row is decided by `batten check` under `verify`, so it charged a model call per fix commit while the work was still being iterated, where a transition charges one per diff actually offered." # THE ROUTE NAMES THE SKILL, AND HOW IT IS INVOKED DECIDES WHETHER IT MINTS. # @@ -11556,6 +11600,12 @@ CI until that store has a portable form. # exit is not visible from the message. Stated here because a route that cannot # be walked is worse than none, and the reader who hits it is the one holding the # keyboard. +# +# A TOMBSTONE STILL OWES A ROUTE, and this one is still walkable — which is why it +# is kept verbatim rather than repointed at the successor rows. A reader meeting +# the historical token needs the same thing the successors ask for: dispatch the +# skill. What changed underneath them is where the demand is made, not what +# satisfies it. [[verdict.route]] id = "verb run first" kind = "command" diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index ebc697438..9651f9d99 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -183,6 +183,7 @@ const RECEIPT_PERMITS: &[&str] = &[ "checks_any", "key", "key_from", + "key_base", "key_shape", "max_age", "requires_field", @@ -3202,6 +3203,12 @@ pub const COLUMN_CENSUS: &[ColumnCensus] = &[ field: "key_from", declares: Declares::NotFactBearing("names where a receipt's key comes from"), }, + ColumnCensus { + field: "key_base", + declares: Declares::NotFactBearing( + "names the ref a delta-keyed receipt's identity is taken against", + ), + }, ColumnCensus { field: "when_value", declares: Declares::NotFactBearing("a condition over a fact another column declared"), @@ -4368,7 +4375,7 @@ impl Rule { /// 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); 53] { + fn columns(&self) -> [(&'static str, bool); 54] { [ // In the census because it is now per-kind, which is what makes // "required by every kind but the judge" a fact the existing @@ -4391,6 +4398,7 @@ impl Rule { ("when_present", self.when_present.is_some()), ("when_value", self.when_value.is_some()), ("key_from", self.key_from.is_some()), + ("key_base", self.key_base.is_some()), ("key_shape", self.key_shape.is_some()), ("max_age", self.max_age.is_some()), ("requires_field", self.requires_field.is_some()), diff --git a/crates/batten/tests/it/code_review_dispatched.rs b/crates/batten/tests/it/code_review_dispatched.rs deleted file mode 100644 index 48700b631..000000000 --- a/crates/batten/tests/it/code_review_dispatched.rs +++ /dev/null @@ -1,543 +0,0 @@ -//! `code-review-dispatched`, over the engine that builds its input (CLOUD-1484). -//! -//! # The seam, and why the module's own suite cannot reach it -//! -//! `policy/code-review-dispatched.rego`'s `test_` rules pin the predicate against -//! a fabricated document. Three questions decide whether the gate is alive, and -//! the module cannot ask any of them: -//! -//! * does the ENGINE project the branch's patch identity at -//! `input.tree["base-delta"]["patch-id"]`, and the receipt store at -//! `input.tree.minted`? -//! * is a receipt filed under that identity the one the predicate finds — and one -//! filed under any other identity NOT found? -//! * does the identity survive a REBASE, which is the property the whole keying -//! turns on and the one a landing loop would otherwise re-buy every lap? -//! -//! A `with input as` case actively hides all three: it fabricates the map, so it -//! fabricates the keying. Such a suite passes identically over an engine that -//! ignored the subject entirely. -//! -//! # The channel is confirmed by a PAIR, never by one arm -//! -//! `.claude/rules/policy-modules.md` records how CLOUD-1049's dead channel -//! survived two measurements: a probe whose only clause reads the new key cannot -//! tell an empty channel from a module that never ran, because both are silent. -//! Its remedy there is an unconditional `violation`; here the same discrimination -//! comes free from the two arms below, and is worth stating so nobody deletes one -//! of them as redundant. -//! -//! `an_absent_receipt_is_refused_over_the_engines_own_projection` and -//! `a_receipt_under_this_change_reaches_the_predicate_and_is_clean` are the pair. -//! If `input.tree.minted` were never populated, the second would find its subject -//! absent and REFUSE, so it fails. If `base-delta`'s `patch-id` were never -//! projected, `is_string(subject)` would not hold, every arm would abstain, and -//! the first would report clean, so it fails. Neither key can be dead with both -//! green — which is exactly what one arm alone could not establish. - -// UNIX-ONLY, for `review_dispatched.rs`' reason one family over: every case here -// drives real `git` against a scratch repository, and a case whose fixture failed -// to build would leave the receipt absent — which is exactly what the negative -// arms refuse, so they would pass FOR THE WRONG REASON while the clean case -// failed. A suite whose negative arms pass because the subject never ran is the -// vacuous pass this family exists to refuse. -#![cfg(unix)] -#![allow(clippy::unwrap_used, clippy::expect_used)] - -use crate::common; - -use std::fs; -use std::path::{Path, PathBuf}; - -use batten::rules::{self, Rule}; - -const RULE: &str = "code-review-dispatched"; -const RECEIPT: &str = "code-review"; -const CODE: &str = "crates/batten/src/lib.rs"; - -/// A fixture repository whose base carries no code and whose branch adds some. -/// -/// THROUGH `common::Fixture`, never a hand-rolled `git init` chain: the builder -/// copies a template rather than forking `git init` (CLOUD-1419 measured 1,819 -/// init processes over one run from exactly that habit), and `fixture-forks` -/// refuses a new copy of it. -fn repo(name: &str) -> PathBuf { - let root = common::Fixture::new(name) - .config(CONFIG) - .file("README.md", "base\n") - .git() - .base_commit() - .build(); - install_module(&root); - write_code(&root, "fn a() {}\n"); - seed_store(&root); - root -} - -/// Make the receipt store LISTABLE while holding nothing for this mint. -/// -/// **Every fixture that expects a refusal needs this, and that is the module's -/// three-valued read rather than test scaffolding.** An id is absent from -/// `input.tree.minted` when the engine could not list the store at all, which is -/// could-not-look and abstains; it is present-and-empty when the engine looked -/// and found no receipt, which is the finding. A fixture with no store directory -/// would take the first arm, so a refusal case built on one would fail — and a -/// clean case built on one would pass for the wrong reason. -fn seed_store(root: &Path) { - let store = batten::git::git_dir(root) - .expect("a git dir") - .join("batten-receipts"); - fs::create_dir_all(&store).expect("the receipt store"); - // A receipt belonging to no declared mint: it makes the directory listable - // and `subject_of` skips it, so the `code-review` id is present with an empty - // subject map. - fs::write(store.join("unrelated.subject"), "x 0\n").expect("seed the store"); -} - -/// The one config every fixture here writes, so the shape is stated once. -const CONFIG: &str = "version = 1\n"; - -/// Commit a code change at the path the row's `delta_sources` selects. -fn write_code(root: &Path, body: &str) { - let path = root.join(CODE); - fs::create_dir_all(path.parent().expect("a parent")).expect("the source dir"); - fs::write(&path, body).expect("the source"); - common::git_in(root, &["add", "-A"]); - common::git_in(root, &["commit", "-q", "-m", "change"]); -} - -fn install_module(root: &Path) { - let source = common::at_root("policy/code-review-dispatched.rego") - .canonicalize() - .expect("the committed module is where the row says it is"); - fs::create_dir_all(root.join("policy")).expect("scratch policy dir"); - fs::copy(source, root.join("policy/code-review-dispatched.rego")) - .expect("install committed module"); -} - -/// The identity the ENGINE resolves for this branch — never one this file -/// computes, or the cases would agree with themselves rather than with the gate. -fn identity(root: &Path) -> String { - batten::git::branch_patch_id(root, "refs/remotes/origin/main") - .expect("the repository opens") - .expect("a branch that changed something has an identity") -} - -/// File a receipt under `subject`, exactly as the mint boundary writes one. -fn file_receipt(root: &Path, subject: &str) { - let git_dir = batten::git::git_dir(root).expect("a git dir"); - let store = git_dir.join("batten-receipts"); - fs::create_dir_all(&store).expect("the receipt store"); - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map_or(0, |since| since.as_secs()); - fs::write( - store.join(format!("{RECEIPT}.{subject}")), - format!("deadbeef {now}\n"), - ) - .expect("write the receipt"); -} - -/// The committed row's shape. -fn row() -> Rule { - serde_json::from_value(serde_json::json!({ - "id": RULE, - "kind": "policy", - "scope": "tree", - "base": "refs/remotes/origin/main", - "delta_sources": ["crates/**"], - "module": "policy/code-review-dispatched.rego", - "severity": "deny", - "minted": [{ - "id": RECEIPT, - "mint": RECEIPT, - "field": 0, - "recency": 1, - "max_age_days": 30, - }], - })) - .expect("the loader accepts the committed row's shape") -} - -fn verdicts(root: &Path) -> Vec { - let declared = common::verdicts_in(root); - rules::run_static( - &[row()], - &[], - batten::policy::Vocabulary { - patterns: &[], - verdicts: &declared, - recorders: &[], - }, - root, - ) - .expect("the read surface runs a policy row") - .findings - .into_iter() - .map(|finding| finding.rule) - .collect() -} - -// --------------------------------------------------------------------------- -// THE PROJECTION SEAM. -// --------------------------------------------------------------------------- - -/// A change with no receipt is refused, over the engine's own projection. -/// -/// Without this the module is a `with input as` suite over two keys nothing -/// fills. Declared mutation: `absent-receipt-unread`. -#[test] -fn an_absent_receipt_is_refused_over_the_engines_own_projection() { - let root = repo("code-review-absent"); - assert_eq!( - verdicts(&root), - vec![String::from(RULE)], - "a branch that changed code with nothing filed under its identity must refuse" - ); -} - -/// A receipt filed under this change's identity clears it. -/// -/// This is the half that proves the ENGINE resolves the same identity the mint -/// boundary keys by; a fabricated map would pass over an engine that resolved -/// nothing. -#[test] -fn a_receipt_under_this_change_reaches_the_predicate_and_is_clean() { - let root = repo("code-review-attested"); - let subject = identity(&root); - file_receipt(&root, &subject); - assert!( - verdicts(&root).is_empty(), - "a receipt filed under this change's own identity must clear the gate" - ); -} - -/// THE ANTI-STALENESS HALF. A receipt over other bytes lives under another name. -#[test] -fn a_receipt_over_another_change_does_not_answer() { - let root = repo("code-review-stale"); - file_receipt( - &root, - "0000000000000000000000000000000000000000000000000000000000000000", - ); - assert_eq!( - verdicts(&root), - vec![String::from(RULE)], - "a receipt keyed to bytes this branch does not carry must not answer" - ); -} - -/// A FURTHER CODE COMMIT RE-OWES THE REVIEW, which is the same property read -/// forwards: the identity moves, so the record is filed under a name nothing -/// looks up. -#[test] -fn a_further_code_commit_re_owes_the_review() { - let root = repo("code-review-moved"); - file_receipt(&root, &identity(&root)); - assert!(verdicts(&root).is_empty(), "the fixture starts attested"); - - write_code(&root, "fn a() {}\nfn b() {}\n"); - assert_eq!( - verdicts(&root), - vec![String::from(RULE)], - "a commit that moves code must re-owe the review" - ); -} - -/// THE PROPERTY THE WHOLE KEYING TURNS ON. `land` rebases every lap, and an -/// identity that moved with the rebase would re-buy the review each time — -/// minutes and tokens per lap, which is the shape that gets a gate switched off -/// rather than satisfied. -/// -/// A merge-base diff is what makes this hold, and it is precisely what a -/// `with input as` case cannot check. -#[test] -fn a_rebase_onto_a_moved_base_does_not_re_owe_the_review() { - let root = repo("code-review-rebase"); - let before = identity(&root); - file_receipt(&root, &before); - assert!(verdicts(&root).is_empty(), "the fixture starts attested"); - - // Move the base under the branch with a change this branch never made, then - // rebase onto it — the landing loop's own lap, in miniature. - common::git_in( - &root, - &["checkout", "-q", "-b", "trunk", "refs/remotes/origin/main"], - ); - fs::write(root.join("NOTES.md"), "trunk moved\n").expect("the trunk file"); - common::git_in(&root, &["add", "-A"]); - common::git_in(&root, &["commit", "-q", "-m", "trunk"]); - common::git_in(&root, &["update-ref", "refs/remotes/origin/main", "trunk"]); - common::git_in(&root, &["checkout", "-q", "main"]); - common::git_in(&root, &["rebase", "-q", "refs/remotes/origin/main"]); - - assert_eq!( - identity(&root), - before, - "a rebase relocates the change and must not restate it" - ); - assert!( - verdicts(&root).is_empty(), - "the receipt taken before the rebase must still answer after it" - ); -} - -/// A PROSE-ONLY BRANCH OWES NO CODE REVIEW. -/// -/// Without this narrowing the gate refuses every fixture and every fresh clone, -/// which is the shape that gets a gate switched off. Declared mutation: -/// `prose-only-priced`. -#[test] -fn a_prose_only_branch_owes_no_code_review() { - // The base ALREADY CARRIES the code, so the branch's only change is a comment - // and `code-changed` stays empty. Building it the other way round — base with - // no file — makes the path read as ADDED and the case would pass for the - // wrong reason. - let root = common::Fixture::new("code-review-prose") - .config(CONFIG) - .file(CODE, "fn a() {}\n") - .git() - .base_commit() - .build(); - install_module(&root); - seed_store(&root); - - fs::write(root.join(CODE), "// a comment\nfn a() {}\n").expect("the source"); - common::git_in(&root, &["add", "-A"]); - common::git_in(&root, &["commit", "-q", "-m", "comment only"]); - - // THE PREMISE, ASSERTED BEFORE THE CONCLUSION. Without this the case would - // pass over a fixture whose base carried no file at all, where the path reads - // as ADDED and `code-changed` is non-empty — green for the wrong reason. - let delta = batten::git::base_delta( - &root, - "refs/remotes/origin/main", - &[String::from("crates/**")], - true, - ) - .expect("the repository opens") - .expect("the base resolves"); - assert!( - delta.code_changed.is_empty(), - "a comment-only commit must move no code, but `code-changed` holds {:?}", - delta.code_changed - ); - assert!( - !delta.edited.is_empty(), - "the fixture must still have EDITED the file, or it tests nothing" - ); - assert!( - verdicts(&root).is_empty(), - "a comment-only commit moves no code and must not re-owe the review" - ); -} - -/// A CHANGE WITH NO IDENTITY OWES NO REVIEW, and the fixture has to make `owed` -/// TRUE or the case proves nothing. -/// -/// **The first version of this case was non-discriminating and the code review -/// caught it.** It built a branch whose base already carried the code, so -/// `code-changed` was empty, `owed` excluded the arm, and the declared -/// `no-identity-priced` mutation would have SURVIVED — which is the shape -/// `.claude/rules/policy-modules.md` warns about: a mutation over a conjunct some -/// other conjunct already excludes. -/// -/// The state that separates them is an UNCOMMITTED edit. `base_delta` is a tip -/// diff over the working tree, so it reports the code as changed and `owed` -/// holds; `branch_patch_id` reads `HEAD` against the merge base, which are the -/// same commit, so there is no identity. Declared mutation: `no-identity-priced`. -#[test] -fn a_change_with_no_identity_owes_no_review() { - let root = common::Fixture::new("code-review-empty") - .config(CONFIG) - .file(CODE, "fn a() {}\n") - .git() - .base_commit() - .build(); - install_module(&root); - seed_store(&root); - - // UNCOMMITTED, deliberately: this is the one state where the working-tree - // delta says code moved and the committed range says nothing did. - fs::write(root.join(CODE), "fn a() {}\nfn b() {}\n").expect("the source"); - - assert!( - batten::git::branch_patch_id(&root, "refs/remotes/origin/main") - .expect("the repository opens") - .is_none(), - "the fixture's premise: nothing is committed beyond the base, so there is no identity" - ); - let delta = batten::git::base_delta( - &root, - "refs/remotes/origin/main", - &[String::from("crates/**")], - true, - ) - .expect("the repository opens") - .expect("the base resolves"); - assert!( - !delta.code_changed.is_empty(), - "the fixture's other premise: `owed` must HOLD, or the mutation this case \ - pins is excluded by a different conjunct and survives" - ); - - assert!( - verdicts(&root).is_empty(), - "a change with no identity has nothing to key a receipt by and must not be refused" - ); -} - -/// AN UNLISTABLE RECEIPT STORE IS COULD-NOT-LOOK, NOT A REFUSAL. -/// -/// **The defect the code review caught, over the engine that produces it.** -/// `minted::fields` leaves a declared id ABSENT from the map when it cannot list -/// the store, so the map is EMPTY rather than `null` — and the first draft guarded -/// on `is_object(input.tree.minted)`, which holds for an empty object. Every fresh -/// clone and every CI runner would have been refused, which is the arm both the -/// module METADATA and `batten.toml` promise is silent. -/// -/// A module suite cannot reach this: it would have to fabricate the empty map, -/// which is exactly the shape a `with input as` case cannot prove the engine -/// produces. Declared mutation: `store-unreadable-refused`. -#[test] -fn an_unlistable_store_is_could_not_look_and_never_a_refusal() { - // NOT `repo`, which seeds the store: the whole subject here is a checkout - // where no receipt has ever been written. - let root = common::Fixture::new("code-review-no-store") - .config(CONFIG) - .file("README.md", "base\n") - .git() - .base_commit() - .build(); - install_module(&root); - write_code(&root, "fn a() {}\n"); - let store = batten::git::git_dir(&root) - .expect("a git dir") - .join("batten-receipts"); - assert!( - !store.exists(), - "the fixture's premise: no receipt store has ever been written here" - ); - assert!( - verdicts(&root).is_empty(), - "a checkout whose receipt store cannot be listed must abstain, not refuse" - ); -} - -/// THE WRITER AND THE READER AGREE, driven through the real mint boundary. -/// -/// Every other case here files the receipt by hand, which proves the predicate -/// and nothing about the half that writes it. If `mint_receipts` composed a -/// different key — another base, another separator, a different authority root — -/// the gate would refuse forever with every one of those cases green. That is the -/// same class this module's own header refuses for `with input as`, one layer -/// over: a fabricated artifact cannot prove the engine produces it. -/// -/// So this drives the boundary itself and then asks the gate, with nothing -/// hand-written in between. -#[test] -fn the_mint_boundary_writes_the_receipt_this_gate_reads() { - let root = repo("code-review-writer"); - assert_eq!( - verdicts(&root), - vec![String::from(RULE)], - "the fixture starts owing a review" - ); - - let row: batten::mint::Declared = serde_json::from_value(serde_json::json!({ - "name": RECEIPT, - "tool": "Skill", - "selects_at": "skill", - "selects": "code-review", - "key": "delta", - "key_base": "refs/remotes/origin/main", - "mode": "replace", - "body": "{git:HEAD} {now}", - })) - .expect("the committed row's shape loads"); - - // The envelope as the live host sends one: the skill named in the INPUT, and - // a result carrying no JSON at all — which is what `record_mints` had to stop - // giving up on for this row to fire. - batten::mint_receipts_for_test( - &[row], - "Skill", - &serde_json::json!({"skill": "code-review"}), - &serde_json::Value::Null, - &root, - // No body piece here reads the result, so the grammar is inert for these - // rows — passed explicitly so a case that adds an `{authority:…}` piece - // has to decide rather than inherit a hidden default. - None, - ); - - assert!( - verdicts(&root).is_empty(), - "the receipt the boundary wrote must be the one the module looks up" - ); -} - -/// AND A DIFFERENT SKILL WRITES NOTHING, through the same boundary. -/// -/// Without the selector the row mints on every dispatch its tool matched, and the -/// receipt attests that *something* ran. This is that arm over the engine rather -/// than over `selects` alone. -#[test] -fn another_skill_mints_nothing_through_the_boundary() { - let root = repo("code-review-wrong-skill"); - let row: batten::mint::Declared = serde_json::from_value(serde_json::json!({ - "name": RECEIPT, - "tool": "Skill", - "selects_at": "skill", - "selects": "code-review", - "key": "delta", - "key_base": "refs/remotes/origin/main", - "mode": "replace", - "body": "{git:HEAD} {now}", - })) - .expect("the committed row's shape loads"); - - batten::mint_receipts_for_test( - &[row], - "Skill", - &serde_json::json!({"skill": "batten"}), - &serde_json::Value::Null, - &root, - // No body piece here reads the result, so the grammar is inert for these - // rows — passed explicitly so a case that adds an `{authority:…}` piece - // has to decide rather than inherit a hidden default. - None, - ); - - assert_eq!( - verdicts(&root), - vec![String::from(RULE)], - "a dispatch of another skill must leave the review still owed" - ); -} - -/// COULD-NOT-LOOK IS NOT A REFUSAL. A checkout whose row declares no receipt -/// projects `null`, and the gate must go quiet rather than refuse the machine. -#[test] -fn an_undeclared_receipt_is_could_not_look_and_never_a_refusal() { - let root = repo("code-review-undeclared"); - let mut bare = row(); - bare.minted.clear(); - let declared = common::verdicts_in(&root); - let findings = rules::run_static( - &[bare], - &[], - batten::policy::Vocabulary { - patterns: &[], - verdicts: &declared, - recorders: &[], - }, - &root, - ) - .expect("the read surface runs a policy row") - .findings; - assert!( - findings.is_empty(), - "a row declaring no receipt projects null, and null is could-not-look" - ); -} diff --git a/crates/batten/tests/it/main.rs b/crates/batten/tests/it/main.rs index 631ce3246..c5cb13a65 100644 --- a/crates/batten/tests/it/main.rs +++ b/crates/batten/tests/it/main.rs @@ -76,7 +76,6 @@ mod claim; mod claim_order; mod claim_receipt; mod cli; -mod code_review_dispatched; mod commit; mod commit_admission; mod commit_arm_sequencing; @@ -209,6 +208,7 @@ mod repetition; mod retirement_doctrine; mod review_answered; mod review_dispatched; +mod review_receipt_delta; mod rule_cost_census; mod rule_cost_rung; mod rules_builtin_claims; diff --git a/crates/batten/tests/it/review_receipt_delta.rs b/crates/batten/tests/it/review_receipt_delta.rs new file mode 100644 index 000000000..44c01255e --- /dev/null +++ b/crates/batten/tests/it/review_receipt_delta.rs @@ -0,0 +1,320 @@ +//! A `delta`-keyed receipt is filed under the identity of the branch's CHANGE, +//! and read back under the same one (CLOUD-1547). +//! +//! # Why this tier, and why a unit test over `delta_subject` would prove nothing +//! +//! Stated because the temptation is real: `delta_subject` is a short function and +//! a unit test over it would assert that a filename contains whatever +//! `branch_patch_id` returned. That is not the question. The question is whether +//! the WRITE half and the READ half resolve the SAME subject — the mint files a +//! receipt at the mediated boundary and the receipt row looks one up there, and a +//! defect in either would leave a gate that loads clean and decides nothing. +//! `fact_record_keying.rs` is this suite's sibling for that reason and this one +//! follows its shape: nothing here writes a receipt by hand and nothing inspects +//! a path to decide a case. Every verdict comes from a real `adjudicate` call. +//! +//! # The three properties, and why each needs its own case +//! +//! * **The receipt is found.** A change with a receipt filed under its identity +//! is allowed. Without this the suite could pass over an engine that refused +//! everything. +//! * **A rebase does not re-owe it.** This is the whole reason the keying exists. +//! `land` rebases every lap, and a keying that moved with the rebase would buy +//! a model call per lap — the cost that gets a gate switched off rather than +//! satisfied. A `head`-keyed receipt fails this case, which is what makes it +//! discriminating rather than decorative. +//! * **A content change DOES re-owe it.** The other direction, and the one that +//! makes the gate worth having: without it a branch reviewed once could push +//! anything. A `branch`-keyed receipt fails this case. +//! +//! The last two are an anti-vacuity pair in the strict sense — no single keying +//! satisfies both, so a suite carrying only one of them is silently satisfied by +//! the wrong column. That is `fact_record_keying.rs`'s own lesson, one keying +//! over. +//! +//! # The push arm is exercised here and NOT declared in `batten.toml` +//! +//! Worth stating, because a reader who checks the committed config will find one +//! row and two arms here and reasonably suspect drift. +//! +//! The keying is not pattern-specific — a `receipt` row selects on a command and +//! resolves the same subject whichever command that is — and the push arm is what +//! shows that, over this file's own config. What this repository declares is only +//! the ready row, because a committed `push-needs-review` fires on EVERY push: +//! measured over the suite, it denied `forced_push.rs`'s benign fixture push, and +//! 15 test files drive a push that has nothing to do with review. What it wants +//! to refuse is a push to a branch whose PR is already READY, and `forge.rs` +//! carries no draft field, so that narrowing is unsayable today (CLOUD-1446 for +//! the gap, CLOUD-1548 for where it closes). +//! +//! So the arm here proves the mechanism generalises; it does not claim the +//! repository is frozen against a mid-ready push. It is not. + +// UNIX-ONLY, for `fact_record_keying.rs`' reason: every case drives real `git` +// against a scratch repository, and a fixture that failed to build would leave +// the receipt absent — which is what the negative arms refuse, so they would pass +// FOR THE WRONG REASON while the allow cases failed. +#![cfg(unix)] +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use crate::common; + +use std::path::{Path, PathBuf}; +use std::process::Output; + +use common::{git_in, run_with_stdin, scratch, stdout, write}; + +const RECEIPT: &str = "code-review"; +const CODE: &str = "crates/batten/src/lib.rs"; + +/// A fixture carrying both committed rows, keyed as the committed config keys +/// them. +/// +/// `key` varies ONLY where a case is establishing that the alternative keyings +/// fail — the discrimination the module doc calls an anti-vacuity pair. Every +/// other case takes the committed `delta`. +fn fixture(name: &str, key: &str) -> PathBuf { + let dir = scratch(name); + // `key_base` is refused on any keying but `delta` (`Rule::validate_delta_base`), + // so the alternative-keying fixtures must omit it — which is the load-time + // half of the same pair these cases exercise at adjudication. + let base = if key == "delta" { + "key_base = \"refs/remotes/origin/main\"\n" + } else { + "" + }; + write( + &dir, + "batten.toml", + &format!( + "version = 1\n\n\ + [[rule]]\nid = \"push-needs-review\"\nkind = \"receipt\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"git push\"\n\ + checks = [\"{RECEIPT}\"]\nkey = \"{key}\"\n{base}\ + reason = \"dispatch the code-review skill\"\n\n\ + [[rule]]\nid = \"ready-needs-review\"\nkind = \"receipt\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"gh pr ready\"\n\ + checks = [\"{RECEIPT}\"]\nkey = \"{key}\"\n{base}\ + reason = \"dispatch the code-review skill\"\n" + ), + ); + // `git_in` blanks global and system config for CLOUD-282's reason: a + // contributor's own git settings must not change a verdict here. + git_in(&dir, &["init", "-q", "-b", "main", "."]); + write(&dir, "README.md", "base\n"); + git_in(&dir, &["add", "-A"]); + git_in(&dir, &["commit", "-q", "-m", "base"]); + // The base ref the rows name. A real remote is not needed and would be a + // network dependency in a gate; what the identity reads is a REF, and a local + // one resolves identically. + git_in(&dir, &["update-ref", "refs/remotes/origin/main", "HEAD"]); + dir +} + +/// Commit a code change, so the branch has an identity at all. +fn change(dir: &Path, body: &str) { + let path = dir.join(CODE); + std::fs::create_dir_all(path.parent().expect("a parent")).expect("the source dir"); + std::fs::write(&path, body).expect("the source"); + git_in(dir, &["add", "-A"]); + git_in(dir, &["commit", "-q", "-m", "change"]); +} + +/// The identity the ENGINE resolves — never one this file computes, or the cases +/// would agree with themselves rather than with the gate. +fn identity(dir: &Path) -> String { + batten::git::branch_patch_id(dir, "refs/remotes/origin/main") + .expect("the repository opens") + .expect("a branch that changed something has an identity") +} + +/// File a receipt under `subject`, in the store both halves read. +/// +/// Written directly rather than through a `[[mint]]` envelope BECAUSE the mint's +/// own tier already drives that path: what is under test here is the READ side +/// resolving the same subject, and routing through the writer would make a +/// failure ambiguous between the two halves. +fn file_receipt(dir: &Path, subject: &str) { + let store = dir.join(".git/batten-receipts"); + std::fs::create_dir_all(&store).expect("the receipt store"); + std::fs::write(store.join(format!("{RECEIPT}.{subject}")), "deadbeef 0\n") + .expect("write the receipt"); +} + +fn call(dir: &Path, command: &str) -> Output { + let envelope = serde_json::json!({ + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": command}, + }); + run_with_stdin( + dir, + &["adjudicate", "--harness", "claude-code"], + &envelope.to_string(), + ) +} + +fn push(dir: &Path) -> Output { + call(dir, "git push -u origin HEAD") +} + +fn ready(dir: &Path) -> Output { + call(dir, "gh pr ready 999") +} + +/// BOTH HELPERS ASSERT THE EXIT STATUS, for `fact_record_keying.rs`' measured +/// reason: the hook prints nothing on an allow and exits 0 either way, so a +/// substring check over an empty string is true — including the empty output of a +/// binary that died before it judged anything. +fn denied(output: &Output) -> String { + let text = stdout(output); + assert_eq!(output.status.code(), Some(0), "the hook itself ran: {text}"); + assert!( + text.contains("\"permissionDecision\":\"deny\""), + "expected a deny: {text}" + ); + text +} + +fn allowed(output: &Output) { + let text = stdout(output); + assert_eq!(output.status.code(), Some(0), "the hook itself ran: {text}"); + assert!(!text.contains("\"deny\""), "expected an allow: {text}"); +} + +// --------------------------------------------------------------------------- +// THE SUBJECT SEAM: the write and the read resolve one identity. +// --------------------------------------------------------------------------- + +/// A change with a receipt under its identity is allowed, and one without is not. +/// +/// The pair, in one case, because either alone is satisfied by a broken engine: +/// an engine that never resolved a subject would refuse both, and one that +/// ignored the receipt store would allow both. Declared mutation: +/// `delta-receipt-unread`. +#[test] +fn a_receipt_under_this_change_answers_and_nothing_else_does() { + let dir = fixture("review-delta-subject", "delta"); + change(&dir, "fn a() {}\n"); + + denied(&ready(&dir)); + file_receipt(&dir, &identity(&dir)); + allowed(&ready(&dir)); +} + +/// A receipt filed under some OTHER identity does not answer. +/// +/// The forgery control: without it the row would be satisfied by any receipt in +/// the store, which is `ReceiptKey::Branch` wearing a subject. +#[test] +fn a_receipt_over_another_change_does_not_answer() { + let dir = fixture("review-delta-other", "delta"); + change(&dir, "fn a() {}\n"); + file_receipt(&dir, "0000000000000000000000000000000000000000"); + + let text = denied(&ready(&dir)); + assert!(text.contains("receipt read missing"), "{text}"); +} + +// --------------------------------------------------------------------------- +// THE ANTI-VACUITY PAIR: no other keying satisfies both of these. +// --------------------------------------------------------------------------- + +/// A REBASE THAT CHANGES NO CONTENT DOES NOT RE-OWE THE REVIEW. +/// +/// The property the whole keying exists for. `land` rebases every lap, so a +/// keying that moved here would buy a model call per lap — the cost that gets a +/// gate switched off rather than satisfied. A `head`-keyed receipt fails this +/// case, which is what makes it discriminating. Declared mutation: +/// `rebase-repriced`. +#[test] +fn a_rebase_that_changes_no_content_still_answers() { + let dir = fixture("review-delta-rebase", "delta"); + change(&dir, "fn a() {}\n"); + file_receipt(&dir, &identity(&dir)); + allowed(&ready(&dir)); + + // Advance the base and replay onto it. The tree the branch produces is + // unchanged, so the merge-base diff — and therefore the identity — is too, + // while every commit SHA on the branch is new. + let before = common::git_in(&dir, &["rev-parse", "HEAD"]); + git_in( + &dir, + &["checkout", "-q", "-b", "trunk", "refs/remotes/origin/main"], + ); + write(&dir, "UNRELATED.md", "moved\n"); + git_in(&dir, &["add", "-A"]); + git_in(&dir, &["commit", "-q", "-m", "trunk moves"]); + git_in(&dir, &["update-ref", "refs/remotes/origin/main", "HEAD"]); + git_in(&dir, &["checkout", "-q", "main"]); + git_in(&dir, &["rebase", "-q", "refs/remotes/origin/main"]); + let after = common::git_in(&dir, &["rev-parse", "HEAD"]); + + assert_ne!( + before.trim(), + after.trim(), + "the rebase must actually move HEAD, or this case asserts nothing" + ); + allowed(&ready(&dir)); +} + +/// A CONTENT CHANGE DOES RE-OWE IT. +/// +/// The other direction, and the one that makes the gate worth having: without it +/// a branch reviewed once could push anything afterwards. A `branch`-keyed +/// receipt fails this case. +#[test] +fn one_further_code_change_re_owes_the_review() { + let dir = fixture("review-delta-moved", "delta"); + change(&dir, "fn a() {}\n"); + file_receipt(&dir, &identity(&dir)); + allowed(&ready(&dir)); + + change(&dir, "fn a() {}\nfn b() {}\n"); + denied(&ready(&dir)); +} + +// --------------------------------------------------------------------------- +// THE FREEZE INVARIANT: the push arm is what makes the ready arm sound. +// --------------------------------------------------------------------------- + +/// THE KEYING IS NOT PATTERN-SPECIFIC: the same receipt answers at a push. +/// +/// Over THIS FILE's config, not the committed one — see the module header for why +/// the repository declares only the ready row. What this establishes is that a +/// consumer who can narrow the push (a declared loop that knows the PR is ready) +/// gets the freeze from the same mechanism, with no second keying. +/// Declared mutation: `push-ungated`. +#[test] +fn a_push_carrying_an_unreviewed_change_is_refused() { + let dir = fixture("review-delta-push", "delta"); + change(&dir, "fn a() {}\n"); + + denied(&push(&dir)); + file_receipt(&dir, &identity(&dir)); + allowed(&push(&dir)); + + // And what the freeze would buy a consumer who declares it: once reviewed, + // a further change cannot reach the remote without its own review. + change(&dir, "fn a() {}\nfn b() {}\n"); + denied(&push(&dir)); +} + +// --------------------------------------------------------------------------- +// COULD-NOT-LOOK IS A REFUSAL HERE, DELIBERATELY. +// --------------------------------------------------------------------------- + +/// A BRANCH WITH NO IDENTITY IS REFUSED RATHER THAN WAVED THROUGH. +/// +/// The opposite of what the `branch` and `named` arms do, and the deliberate +/// half. An unresolvable base or an empty diff means there is no change to have +/// reviewed; answering could-not-look would ALLOW, so a branch whose base does +/// not resolve could push anything. Refusing is loud and cheap to clear. +#[test] +fn a_branch_with_no_change_at_all_is_refused_rather_than_allowed() { + let dir = fixture("review-delta-empty", "delta"); + // No `change` call: the branch sits at the base, so the merge-base diff is + // empty and `branch_patch_id` mints no identity. + denied(&ready(&dir)); +} diff --git a/policy/code-review-dispatched.rego b/policy/code-review-dispatched.rego deleted file mode 100644 index 9a3c24743..000000000 --- a/policy/code-review-dispatched.rego +++ /dev/null @@ -1,350 +0,0 @@ -# METADATA -# description: | -# The branch's own change carries a code-review receipt, or it does not land -# (CLOUD-1484). -# -# THIS REFUSES ABSENCE, AND ABSENCE IS ALL IT MAY REFUSE. A patch identity with -# no receipt under it means the declared review has not been shown to run over -# this change — a comparison of two digests, which is a thing a gate may decide. -# What the reviewer CONCLUDED is not, and refusing on it would be a model -# verdict wearing an exit code (non-negotiable rule 3). Nothing the agent wrote -# reaches this module: the receipt's fields are a commit id and an instant, and -# there is no channel here a finding's prose could travel down. -# -# THE DISPATCH IS THE AGENT'S AND THE RECEIPT IS THE BOUNDARY'S, which is the -# whole difference from `review-dispatched` beside it. That module's engine -# SPAWNS a reviewer, which is what put a runner, a probe and a prompt channel -# into a repo-agnostic core. Here Batten refuses, the refusal names what the -# agent owes, the agent dispatches its own review with its own harness, and -# `[[mint]]` writes the receipt from the tool result the boundary already sees. -# Nothing about that harness is expressible in this module or in the crate. -# -# WHY THIS IS NOT THE SHAPE CLOUD-1265 REFUTED. `tool-verdict`'s -# producer-writes-outside store went measured dead — `validator-verdict-clean` -# reads a record nothing ever writes — because a person had to remember to run -# the producer, and `review.rs` cites exactly that as its reason to spawn. The -# hook is what does not transfer: nobody has to remember, because the boundary -# sees the call it is already mediating. A reader who takes CLOUD-1265 as -# refuting this design is reading past that difference. -# -# THREE ANSWERS AND THE MODULE READS ALL THREE, AND THE ARM IS PER-ID. -# `input.tree.minted` is `null` when no row declared a receipt, and a declared id -# is ABSENT from the map when the engine could not LIST its store — both are -# could-not-look, and silence is the honest answer on a fresh clone, which is -# every CI runner until CLOUD-877 gives the receipt a portable form. An id -# PRESENT with the current patch identity absent from it is the finding: the -# engine looked and there is no receipt. Present WITH it is clean, whatever the -# review said. -# -# READING THE OUTER MAP AS THE COULD-NOT-LOOK CHANNEL IS THE MISTAKE THIS -# MODULE ALREADY MADE ONCE. An unlistable store leaves that map EMPTY, not -# `null`, so a guard on the outer object refuses exactly the case it means to -# abstain on. -# -# TWO WAYS THIS OVER-OWES, BOTH STATED RATHER THAN DISCOVERED. `owed` is read -# off a TIP diff over the working tree and `subject` is a MERGE-BASE diff over -# committed bytes, so the two do not answer about the same range. On a stale -# branch, code that landed on trunk reads as this branch's and a prose-only -# branch can owe a review until it rebases — `verify` asserts the branch is -# rebased on current `origin/main`, so on the path that matters they agree. And -# the identity covers the WHOLE change, so a prose commit added to a branch that -# also carries code moves the key and re-owes the review. Both err toward owing -# a review that is not strictly due, which is the direction a completion gate -# should fail in; neither can produce the other direction, which is a branch -# landing unread. -# -# THE CLEAN-TREE CONDITION IS NOWHERE, AND THAT IS A DECISION TWICE OVER. A -# conjunct here would either duplicate `tree-clean` — which already owns tree -# cleanliness for the landing path, and which nothing reaches `main` without -# passing — or, read the other way round, let a dirty tree SILENCE this gate. -# The mint carried one instead and it has been withdrawn: `git::uncommitted` -# counts an uninitialised gitlink as changed, so `Ok(0)` was unsatisfiable in -# this repository and the receipt could never have been written (CLOUD-1500). -# A reviewer reads the working tree anyway, so refusing to record a dispatch -# taken over uncommitted work would attest less than happened, not more. -# -# THE BRACKETS ARE NOT STYLE: the schema file carries a hyphen, so the dotted -# form is a parse error reported as `invalid schema reference`. -# THIS BLOCK IS YAML AND MUST STAY THE LAST COMMENT BLOCK BEFORE `package`. -# schemas: -# - input: schema["policy-input.schema"] -package batten.code_review_dispatched - -import rego.v1 - -rules contains "code-review-dispatched" - -# The receipts this repository declares it will not land code without. -# -# The id is the CONSUMER's, named here rather than derived from the fact, for -# `review-dispatched`'s reason one module over: a rule that refused only over what -# it FOUND could never refuse an absence, which is the one thing this exists to -# refuse. -required contains "code-review" - -# Whether the engine could look at the receipt store FOR THIS ID. -# -# PER-ID, NOT ON THE OUTER MAP, and that distinction is the whole three-valued -# read rather than a refinement of it. `minted::fields` inserts an entry for every -# declared row whose store it could LIST, and skips the row entirely when it could -# not — so an id ABSENT from the map is could-not-look, and an id PRESENT with no -# matching subject is the engine having looked and found nothing. -# -# THE FIRST DRAFT GUARDED ON `is_object(input.tree.minted)` AND HAD THE ARM -# EXACTLY BACKWARDS. An unlistable store leaves the map EMPTY rather than `null`, -# `is_object({})` holds, and the refusal then fired on every fresh clone and every -# CI runner — the arm this module's own METADATA says it abstains on, and the one -# `batten.toml` promises is honestly silent until CLOUD-877 gives the receipt a -# portable form. Caught by the code review this gate exists to demand, which is -# the only reason it is not in the tree. -# -# BOTH `is_object` CALLS ARE LOAD-BEARING. The outer one is what keeps this from -# indexing `null` — a hard evaluation FAULT in Rego rather than a silent miss — -# when no row declares a receipt at all. -looked_at(id) if { - is_object(input.tree.minted) - is_object(input.tree.minted[id]) -} - -# The branch's own diff, as the engine resolved it. -# -# GUARDED ON `is_object` FOR THE SAME REASON `looked_at` IS, and the guard was -# missing here while its sibling carried one — the asymmetry the code review this -# gate demands is what found it. `rules.rs` projects `base-delta` as `null` when -# the base does not resolve, indexing `null` is a hard evaluation FAULT in Rego -# rather than a silent miss, and a fault takes the whole BUNDLE down — every -# predicate in it, not only this one. A fresh clone with no `origin/main` is the -# reachable instance. Undefined here leaves `owed` and `subject` undefined too, so -# every arm goes quiet, which is the could-not-look reading this module already -# takes for an absent identity. -delta := resolved if { - resolved := input.tree["base-delta"] - is_object(resolved) -} - -# The identity of the change this branch is asking to land. -# -# A MERGE-BASE diff over COMMITTED bytes, which is what makes it survive the -# landing loop: `land` rebases every lap, and an identity that moved with the -# rebase would re-buy the review each time — minutes and tokens per lap, which is -# the shape that gets a gate switched off rather than satisfied. -# -# Absent — a base that does not resolve, or an EMPTY diff — leaves every arm below -# quiet. A branch that changed nothing has no identity, and reading that as -# `unreviewed` would refuse a checkout with nothing to review. -# -# AND IT CAN UNDER-OWE, WHICH IS A NAMED HOLE RATHER THAN A PROPERTY. Committed -# bytes are the subject, so an uncommitted code edit does not move the identity: -# the receipt still answers and `batten check` reports clean over work no review -# has read. Not repaired here, and for measured reasons rather than for want of -# trying — a conjunct requiring a clean tree makes a DIRTY tree abstain, so -# dirtying the tree would silence this gate, and refusing on dirt is a second -# opinion where `tree-clean` already owns the question. `tree-clean` closes it on -# the landing path; a bare `batten check` on a dirty tree is what stays uncovered. -subject := delta["patch-id"] - -# The path prefixes this repository treats as code for the purpose of owing a -# review. -# -# DECLARED HERE BECAUSE THE ROW'S `delta_sources` DOES NOT NARROW WHAT THIS -# MODULE SEES, which is the engine's shape rather than a mistake in the row. -# `rules.rs` builds ONE `base_delta` from the UNION of every row's globs and hands -# the same value to every module, so a row's own list only ever ADDS to what -# everybody reads — and six rows in this config declare `["**"]`. Without the -# narrowing below, a branch touching only `.github/workflows/*.yml` or -# `schema/*.json` would owe a code review. Caught by the code review this gate -# exists to demand. -# -# PREFIXES RATHER THAN A `[[pattern]]` ROW: this is a path SET, not a concept with -# one spelling, and `.claude/rules/policy-modules.md`'s registry is for the -# latter. A threshold or a path list spelled as a regex is the error that file -# records twice. -reviewable := {"crates/", "policy/", "mise-tasks/"} - -# Whether a review is owed at all. -# -# GATED ON CODE, not on any change. `code-changed` is the subset whose non-comment -# remainder moved, so a prose-only branch owes nothing — and neither does a fresh -# clone or a fixture, which is the narrowing `review-dispatched` had to add after -# four `cli.rs` cases went red at once for wanting to exercise other rules. -owed if { - some path in delta["code-changed"] - some prefix in reviewable - startswith(path, prefix) -} - -# `batten.toml` is the policy authority every gate reads, so an edit to it is a -# change to what this repository refuses. A separate arm rather than a fourth -# prefix, because it is a FILE and the set above is a directory test — folding it -# in would make `batten.toml.example` reviewable by accident. -owed if { - "batten.toml" in delta["code-changed"] -} - -# Every declared receipt with nothing filed under this change. -# -# The keying stays the ENGINE's business: a receipt taken over other bytes lives -# under a different subject and never matches, so `absent from the map` already -# means `not reviewed as this now stands`. A module re-deriving that would be the -# second authority over an identity `git::branch_patch_id` already owns. -unattested contains id if { - owed - is_string(subject) - some id in required - looked_at(id) - not input.tree.minted[id][subject] -} - -violation contains { - "rule": "code-review-dispatched", - "verdict": "patch read never", - "subjects": [{"artifact": id}], -} if { - some id in unattested -} - -# --- the load-time tier ------------------------------------------------------ -# -# These pin the PREDICATE. They cannot pin that the ENGINE keys a receipt by the -# branch's patch identity, or that a rebase leaves that identity alone — a -# `with input as` case fabricates the very keying the whole gate turns on. -# `crates/batten/tests/it/code_review_dispatched.rs` is that tier. - -changed := { - "added": [], - "edited": ["crates/batten/src/lib.rs"], - "deleted": [], - "code-changed": ["crates/batten/src/lib.rs"], - "patch-id": "abc", -} - -attested(subjects) := {"tree": { - "base-delta": changed, - "minted": {"code-review": subjects}, -}} - -test_a_receipt_under_this_change_is_clean if { - count(violation) == 0 with input as attested({"abc": "cafe 1700000000"}) -} - -test_no_receipt_at_all_is_refused if { - some v in violation with input as attested({}) - v.verdict == "patch read never" -} - -# A RECEIPT OVER OTHER BYTES DOES NOT ANSWER. This is the anti-staleness half and -# the reason the key is a digest rather than a marker: push a commit and the old -# record lives under a name nothing looks up. -test_a_receipt_over_another_change_does_not_answer if { - some v in violation with input as attested({"zzz": "cafe 1700000000"}) - v.verdict == "patch read never" -} - -# THE REFUSAL NAMES WHICH RECEIPT, so a reader is not left working out which of -# several declared ids is missing. -test_the_refusal_names_the_receipt if { - ids := {v.subjects[0].artifact | some v in violation} with input as attested({}) - ids == {"code-review"} -} - -# A MAP NAMING ONLY ANOTHER ID IS COULD-NOT-LOOK, NOT A REFUSAL — and this case -# asserted the opposite until the per-id guard landed. -# -# The reasoning that made it look right was that a record under another id is not -# this one having run, which is true and is not what this shape says. `minted` is -# keyed by DECLARED ROW, and `minted::fields` inserts an entry for every row whose -# store it could list — so an id missing from the map means the engine never -# looked for it, never that it looked and found nothing. Refusing here would be a -# verdict about the engine's reach wearing a verdict about the branch. -# -# The claim the old case meant to make is `test_a_receipt_over_another_change_ -# does_not_answer` above, where the id IS present and the subject is another -# change's. -test_a_map_naming_only_another_id_is_not_refused if { - count(violation) == 0 with input as {"tree": { - "base-delta": changed, - "minted": {"other": {"abc": "cafe 1700000000"}}, - }} -} - -# COULD-NOT-LOOK, and without the `is_object` guard this case does not merely -# fail — it faults, taking the whole bundle with it. -test_could_not_look_does_not_fault if { - count(violation) == 0 with input as {"tree": {"base-delta": changed, "minted": null}} -} - -# A PROSE-ONLY BRANCH OWES NO CODE REVIEW. Without this the gate refuses every -# checkout that has never dispatched, which is every fixture and every fresh -# clone. -# A CHANGE OUTSIDE THE REVIEWABLE PREFIXES OWES NOTHING. The row's own -# `delta_sources` cannot express this — the engine hands every module one delta -# built from the union of every row's globs — so without the prefix set a -# workflow-only or schema-only branch owes a code review. -test_a_change_outside_the_reviewable_prefixes_is_not_refused if { - count(violation) == 0 with input as attested_over({ - "added": [], - "edited": [".github/workflows/ci.yml"], - "deleted": [], - "code-changed": [".github/workflows/ci.yml"], - "patch-id": "abc", - }) -} - -# THE POLICY AUTHORITY IS REVIEWABLE ON ITS OWN ARM. An edit to `batten.toml` is a -# change to what this repository refuses. -test_an_edit_to_the_authority_is_refused if { - some v in violation with input as attested_over({ - "added": [], - "edited": ["batten.toml"], - "deleted": [], - "code-changed": ["batten.toml"], - "patch-id": "abc", - }) - v.verdict == "patch read never" -} - -# AN UNLISTABLE STORE IS COULD-NOT-LOOK, and reading it as `looked` is the defect -# the code review caught: `minted::fields` leaves the id ABSENT when it cannot -# list, so the map is EMPTY rather than `null` and a guard on the outer object -# refuses every fresh clone and every CI runner. -test_an_unlistable_store_is_not_refused if { - count(violation) == 0 with input as {"tree": {"base-delta": changed, "minted": {}}} -} - -test_a_prose_only_branch_is_not_refused if { - count(violation) == 0 with input as attested_over({ - "added": [], - "edited": ["AGENTS.md"], - "deleted": [], - "code-changed": [], - "patch-id": "abc", - }) -} - -# AN EMPTY DIFF HAS NO IDENTITY, and refusing over one would be a verdict about a -# branch with nothing to review. -test_a_change_with_no_identity_is_not_refused if { - count(violation) == 0 with input as attested_over({ - "added": [], - "edited": ["crates/batten/src/lib.rs"], - "deleted": [], - "code-changed": ["crates/batten/src/lib.rs"], - "patch-id": null, - }) -} - -# A DELTA THE ENGINE COULD NOT BUILD, and without the `is_object` guard on -# `delta` this case does not merely fail — it FAULTS, taking every predicate in -# the bundle with it. `rules.rs` projects `null` here on a checkout whose base ref -# does not resolve, which a fresh clone with no `origin/main` is. -test_a_delta_the_engine_could_not_build_does_not_fault if { - count(violation) == 0 with input as attested_over(null) -} - -attested_over(d) := {"tree": {"base-delta": d, "minted": {"code-review": {}}}} - -#MUTANT-SUITE crates/batten/tests/it/code_review_dispatched.rs -#MUTANT absent-receipt-unread|s@^\tnot input.tree.minted\[id\]\[subject\]$@\tfalse@|an_absent_receipt_is_refused_over_the_engines_own_projection -#MUTANT no-identity-priced|s@^\tis_string(subject)$@\ttrue@|a_change_with_no_identity_owes_no_review -#MUTANT prose-only-priced|s@^\towed$@\ttrue@|a_prose_only_branch_owes_no_code_review -#MUTANT store-unreadable-refused|s@^\tlooked_at(id)$@\ttrue@|an_unlistable_store_is_could_not_look_and_never_a_refusal From 2e1d7ec2133458f0ca3d77c396b9cc552f4d0487 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:28 +0000 Subject: [PATCH 14/21] test(receipt): build the delta-keying fixture through the builder, not a git init chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fixture-forks` refused `review_receipt_delta.rs:104` and was right: the fixture hand-rolled `git init` + `add` + `commit` + `update-ref`, which is the habit CLOUD-1419 measured at 1,819 init processes and 4.49s over one run. `common::Fixture` copies a published template instead of forking `git init`, and its `base_commit` already pins `refs/remotes/origin/main` — the very ref these rows name — so the chain was not only expensive but redundant. 4881/4881 unchanged. Refs: CLOUD-1547 --- .../batten/tests/it/review_receipt_delta.rs | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/crates/batten/tests/it/review_receipt_delta.rs b/crates/batten/tests/it/review_receipt_delta.rs index 44c01255e..503657238 100644 --- a/crates/batten/tests/it/review_receipt_delta.rs +++ b/crates/batten/tests/it/review_receipt_delta.rs @@ -63,7 +63,7 @@ use crate::common; use std::path::{Path, PathBuf}; use std::process::Output; -use common::{git_in, run_with_stdin, scratch, stdout, write}; +use common::{git_in, run_with_stdin, stdout, write}; const RECEIPT: &str = "code-review"; const CODE: &str = "crates/batten/src/lib.rs"; @@ -75,7 +75,6 @@ const CODE: &str = "crates/batten/src/lib.rs"; /// fail — the discrimination the module doc calls an anti-vacuity pair. Every /// other case takes the committed `delta`. fn fixture(name: &str, key: &str) -> PathBuf { - let dir = scratch(name); // `key_base` is refused on any keying but `delta` (`Rule::validate_delta_base`), // so the alternative-keying fixtures must omit it — which is the load-time // half of the same pair these cases exercise at adjudication. @@ -84,10 +83,14 @@ fn fixture(name: &str, key: &str) -> PathBuf { } else { "" }; - write( - &dir, - "batten.toml", - &format!( + // THROUGH `common::Fixture`, never a hand-rolled `git init` chain: the builder + // copies a template rather than forking `git init` (CLOUD-1419 measured 1,819 + // init processes over one run from exactly that habit), and `fixture-forks` + // refuses a new copy of it. `base_commit` also pins `refs/remotes/origin/main`, + // which is the ref these rows name — no remote is needed, because what the + // identity reads is a REF and a local one resolves identically. + common::Fixture::new(name) + .config(&format!( "version = 1\n\n\ [[rule]]\nid = \"push-needs-review\"\nkind = \"receipt\"\n\ scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"git push\"\n\ @@ -97,19 +100,11 @@ fn fixture(name: &str, key: &str) -> PathBuf { scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"gh pr ready\"\n\ checks = [\"{RECEIPT}\"]\nkey = \"{key}\"\n{base}\ reason = \"dispatch the code-review skill\"\n" - ), - ); - // `git_in` blanks global and system config for CLOUD-282's reason: a - // contributor's own git settings must not change a verdict here. - git_in(&dir, &["init", "-q", "-b", "main", "."]); - write(&dir, "README.md", "base\n"); - git_in(&dir, &["add", "-A"]); - git_in(&dir, &["commit", "-q", "-m", "base"]); - // The base ref the rows name. A real remote is not needed and would be a - // network dependency in a gate; what the identity reads is a REF, and a local - // one resolves identically. - git_in(&dir, &["update-ref", "refs/remotes/origin/main", "HEAD"]); - dir + )) + .file("README.md", "base\n") + .git() + .base_commit() + .build() } /// Commit a code change, so the branch has an identity at all. From 506edc9fe01a654ad2fb5783d17d9c175e91b56e Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 19:16:57 +0000 Subject: [PATCH 15/21] chore(waiver): suppress filed-here, whose class has no walkable route here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1547 is implemented and closed by this PR, so its §1 names these eight paths because they are the files this change edits. That is precisely what `filed-here.rego`'s `closes` arm exempts, and the exemption cannot fire: the `pr-closes` record is minted from a `gh pr view --jq .body` call and there is no `gh` on this host, so `closes` is empty. Empty is not `closes_unreadable` either — nothing attempted the call, so the blocked store has nothing to record. CLOUD-1126 named that shape; CLOUD-1481 carries the residual. The class's own override route is spent without suppressing. Nine admissions were issued and spent against these subjects and the findings did not move. The consumption is NOT missing — `admission::Anchor::Finding` exists for exactly this and `apply_admissions` suppresses stored findings by fingerprint. What fails is the anchor resolver: `lib.rs:5428` matches stored findings on `(rule, path)` and falls back to `Anchor::Call` on zero matches, so the spend produced `Admits-anchor: call:2dee1726…` for a tree finding, and nothing queries a call anchor for one. The ambiguity arm refuses precisely to avoid "an override that appears to work"; the zero arm walks into it. CLOUD-1551 carries that, with why the match returned zero as its open question. So all four declared routes are unavailable here: the fix is committed, the body says `Closes CLOUD-1547` and cannot be read, filing from a clean tree cannot apply to a row already filed and already implemented, and the override binds an anchor nothing reads. A `[[waiver]]` is the blunt instrument that route existed to avoid, which is why it names both rows, carries a reason, and expires 2026-10-11 rather than switching the rule off. It names `filed-here` rather than `filed-over-own-diff` because a waiver keys on a rule id and that is one of the two predicates the rule publishes, so it is wider than intended and also silences `filed-unrefined`. Stated on the row. Admits: 09521541a8bd8da59b93ddfb949fd6af188f81dfae43ca4b5775397dd437e9a2 Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:2dee17263fa63c4951cc4b4b3e802d08f6e20195 Admits-epoch: 1ab5aef43a9bb3a8da9b8c2adf82a92d26c626ded0af816a694267fc8337d576 Admits-author: alec@wenzowski.com Admits-prev: 36aa4e297aebc8c5fa0490acac99a36be208cd6938f3ff623419c2ff6c939433 Admits-answer-lost: A committed waiver whose stated cause is false. It says filed-here.rego has no admission conjunct and nothing reads the store; measured, admission::Anchor::Finding exists for exactly this and apply_admissions suppresses stored findings by fingerprint. The real cause is the anchor resolver falling back to a call anchor on zero matches. A waiver is the one artefact whose job is to say why a gate is suppressed and when to stop, so a wrong cause in it is the false-pointer failure scanning.md records, committed. Admits-answer-precondition: The change corrects the reason string and comment on a [[waiver]] row in the policy authority. batten.toml IS the surface a waiver lives on; batten init refuses an existing file and git restore reverts rather than edits, so writing it directly is the only route. It lands in this branch diff where a reviewer reads it. Admits-answer-rejected-route: config read first names batten.toml, the path being written, so it resolves to nothing. patch run first (git restore) would restore committed bytes that do not carry the waiver at all. Weakens: waiver-added waiver[filed-over-own-diff] --- batten.toml | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/batten.toml b/batten.toml index 1da335da7..66b5f3072 100644 --- a/batten.toml +++ b/batten.toml @@ -7010,6 +7010,48 @@ expires = "2026-10-31" # The expiry is short on purpose. PR #842 carries CLOUD-1387's fix; this should # be removed by whoever lands it, and the date is what forces the question if # they do not. +# CLOUD-1547's own row, refused by the gate it is landing beside — and the two +# reasons it cannot be cleared are both mechanism defects rather than facts about +# this branch. +# +# The row is NOT a punt: this PR implements CLOUD-1547 and closes it, so its §1 +# names these paths because they are the files this change edits. That is exactly +# what `closes` exempts. The exemption cannot fire here because the `pr-closes` +# record is minted from a `gh pr view --jq .body` call and there is **no `gh` on +# this host** — so `closes` is empty, and empty is not `closes_unreadable` either, +# because nothing ever attempted the call for the blocked store to record. +# CLOUD-1126 named that shape and CLOUD-1481 carries the residual. +# +# AND THE CLASS'S OWN OVERRIDE ROUTE IS SPENT WITHOUT SUPPRESSING. `issue file +# same` declares `path admit first`; nine admissions were issued and spent against +# these subjects and the findings did not move. The consumption is NOT missing — +# `admission::Anchor::Finding` exists for exactly this and `apply_admissions` +# suppresses stored findings by fingerprint. What fails is the ANCHOR RESOLVER: +# `lib.rs:5428` matches stored findings on `(rule, path)`, and on ZERO matches it +# falls back to `Anchor::Call`. Measured here, with the findings live and being +# reported in the same session: `Admits-anchor: call:2dee1726…`, and nothing +# queries a call anchor for a tree finding. The ambiguity arm refuses precisely +# to avoid "an override that appears to work"; the zero arm walks into it. +# CLOUD-1551, which also records why the match returned zero as the open question. +# +# A `[[waiver]]` and never `severity = "allow"`: this names the rule, states why, +# and LAPSES. Short on purpose — it should be removed by whoever lands CLOUD-1551, +# and the date is what forces the question if they do not. UNNARROWED because the +# eight findings are eight paths under one row, and a `path =` would need eight +# rows that all lapse together anyway. +# +# IT NAMES THE RULE AND SO IT IS WIDER THAN THE PREDICATE IT IS FOR, which is a +# cost rather than a choice. A waiver keys on a `[[rule]]` id — `config-lint` +# refuses `filed-over-own-diff` as `waiver-names-no-rule`, because that is one of +# the two PREDICATES `filed-here` publishes. So this also suppresses +# `filed-unrefined`, which reports nothing today and would be silenced if it +# started to. Stated rather than absorbed: whoever lands CLOUD-1551 removes this, +# and until then an unrefined row this branch files goes unreported. +[[waiver]] +rule = "filed-here" +reason = "CLOUD-1547 is implemented and closed by this PR, so `closes` is the exemption that applies; it cannot fire because `pr-closes` is minted from a `gh pr view` call and there is no `gh` on this host (CLOUD-1126, residual on CLOUD-1481). The class's own `path admit first` override route does not consume — eight admissions were spent and the findings did not move, because the anchor resolver falls back to a `call:` anchor on zero stored-finding matches and nothing queries one for a tree finding (CLOUD-1551). Remove this with CLOUD-1551." +expires = "2026-10-11" + [[waiver]] rule = "claim-before-code" reason = "CLOUD-1387: `captured::reduce` selects the first capture whose bytes MENTION the key rather than the one it is the subject of, so a response quoting the row shadows its real payload and `present` answers false over a row that is on a project. Fix is open in PR #842; remove this waiver with it." From 87a9a11327c6e0f46e2ecbb96eb3a1763e2b5d44 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 16/21] fix(lint): a waiver may name a predicate id, which is the spelling that suppresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1553. `waiver-names-no-rule` resolved a waiver's `rule` against `config.rules` alone. A policy rule publishes PREDICATE ids from its module and a policy finding carries the predicate id, so `waiver::apply` and `config lint` disagreed about which name a `[[waiver]]` holds — and each was self-consistent: rule = "filed-over-own-diff" suppresses (8 audit lines) refused by the lint rule = "filed-here" suppresses nothing clean to the lint Measured on this tree minutes apart by changing only that value. For any policy rule whose module publishes an id of its own, no value satisfied both halves, and the spelling the lint blessed was the dead one — which is the "exemption somebody is relying on" CLOUD-208 opened this smell to catch, produced by the catcher. The bundles were already in hand for the set analysis below and simply never consulted. `Bundle::declared` is the module's own published set — the same authority `attribute` resolves a violation's id against — so this acquires nothing and adds no second list that can drift. The fix deliberately does NOT go the other way. Making `waiver::apply` match the rule id instead would let a waiver over `filed-here` suppress every predicate the module publishes, so waiving one finding class would silently waive the rest. That is a weakening of the raise-only invariant, and it is the widening this branch had to accept and write a comment about before the repair. Second arm: bundles are EMPTY when a module will not load, which a config judged away from its own tree always is — `repo_with_committed_config` is exactly that, the committed `batten.toml` with no `policy/*.rego` beside it. With no bundle to ask, a predicate id is indistinguishable from a typo, so the smell abstains rather than resuming the inversion. Narrow on purpose: it abstains only where the config declares a policy rule and nothing resolved, so a config with no policy rules keeps the smell's full reach. The `filed-over-own-diff` waiver added last commit takes the narrow spelling now. It suppresses that predicate alone and leaves `filed-unrefined` deciding, so the widening its comment apologised for is gone. A wrongly refusing gate is a defect to repair rather than a ticket to file, so this lands here rather than being left for CLOUD-1553 to pick up. Admits: 0a3b4f556f824a4a46783cdac0ad57aae4141253de32752d0a30ac7e5173175c Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: batten.toml Admits-anchor: call:3b21e71b75f10a06de5851c15536534d709d2f64 Admits-epoch: ff75259a4c4c2868d11c45ce05bb1768588122d48b8579c106ae1495d71c21a0 Admits-author: alec@wenzowski.com Admits-prev: 09521541a8bd8da59b93ddfb949fd6af188f81dfae43ca4b5775397dd437e9a2 Admits-answer-lost: A committed waiver whose stated cause is false. It says filed-here.rego has no admission conjunct and nothing reads the store; measured, admission::Anchor::Finding exists for exactly this and apply_admissions suppresses stored findings by fingerprint. The real cause is the anchor resolver falling back to a call anchor on zero matches. A waiver is the one artefact whose job is to say why a gate is suppressed and when to stop, so a wrong cause in it is the false-pointer failure scanning.md records, committed. Admits-answer-precondition: The change corrects the reason string and comment on a [[waiver]] row in the policy authority. batten.toml IS the surface a waiver lives on; batten init refuses an existing file and git restore reverts rather than edits, so writing it directly is the only route. It lands in this branch diff where a reviewer reads it. Admits-answer-rejected-route: config read first names batten.toml, the path being written, so it resolves to nothing. patch run first (git restore) would restore committed bytes that do not carry the waiver at all. Refs: CLOUD-1553 --- batten.toml | 19 +++++++++++-------- crates/batten/src/lint.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/batten.toml b/batten.toml index 66b5f3072..f51e74abf 100644 --- a/batten.toml +++ b/batten.toml @@ -7040,15 +7040,18 @@ expires = "2026-10-31" # eight findings are eight paths under one row, and a `path =` would need eight # rows that all lapse together anyway. # -# IT NAMES THE RULE AND SO IT IS WIDER THAN THE PREDICATE IT IS FOR, which is a -# cost rather than a choice. A waiver keys on a `[[rule]]` id — `config-lint` -# refuses `filed-over-own-diff` as `waiver-names-no-rule`, because that is one of -# the two PREDICATES `filed-here` publishes. So this also suppresses -# `filed-unrefined`, which reports nothing today and would be silenced if it -# started to. Stated rather than absorbed: whoever lands CLOUD-1551 removes this, -# and until then an unrefined row this branch files goes unreported. +# IT NAMES THE PREDICATE, NOT THE RULE, and that is the narrow spelling rather +# than a slip. `waiver::apply` filters FINDINGS and a policy finding carries the +# predicate id, so this suppresses `filed-over-own-diff` and leaves +# `filed-unrefined` — the rule's other predicate — still deciding. +# +# `config-lint` refused exactly this spelling until the same change repaired it: +# `waiver-names-no-rule` read only `config.rules`, so it blessed `filed-here` +# (which suppresses nothing) and refused this one (which does). CLOUD-1553 is +# that inversion; the fix is in `lint.rs` in this commit, because a wrongly +# refusing gate is a defect to repair rather than a ticket to file. [[waiver]] -rule = "filed-here" +rule = "filed-over-own-diff" reason = "CLOUD-1547 is implemented and closed by this PR, so `closes` is the exemption that applies; it cannot fire because `pr-closes` is minted from a `gh pr view` call and there is no `gh` on this host (CLOUD-1126, residual on CLOUD-1481). The class's own `path admit first` override route does not consume — eight admissions were spent and the findings did not move, because the anchor resolver falls back to a `call:` anchor on zero stored-finding matches and nothing queries one for a tree finding (CLOUD-1551). Remove this with CLOUD-1551." expires = "2026-10-11" diff --git a/crates/batten/src/lint.rs b/crates/batten/src/lint.rs index 794a12b99..8b410def5 100644 --- a/crates/batten/src/lint.rs +++ b/crates/batten/src/lint.rs @@ -447,6 +447,45 @@ pub fn smells( .iter() .find(|rule| rule.id == *waiver.rule.get_ref()) { + // A PREDICATE ID IS A WAIVABLE NAME TOO, and reading only `rules` + // here inverted this smell for every policy rule whose module + // publishes an id of its own (CLOUD-1553). + // + // `waiver::apply` filters FINDINGS, and a policy finding carries the + // predicate id rather than the row's. Measured on this repository: + // `rule = "filed-over-own-diff"` suppressed and was refused here, + // while `rule = "filed-here"` was clean here and suppressed nothing — + // so no value satisfied both halves, and the one this smell blessed + // was the dead one. That is precisely the "exemption someone is + // relying on" CLOUD-208 opened it to catch, produced by the catcher. + // + // The bundles are already in hand for the set analysis below, so this + // acquires nothing: `Bundle::declared` is the module's own published + // set, which is the same authority `attribute` resolves a violation's + // id against. Reading it here rather than re-deriving from the module + // source keeps one authority over what a bundle declares. + None if bundles + .iter() + .any(|bundle| bundle.declared().contains(waiver.rule.get_ref())) => {} + // COULD-NOT-LOOK, and it is the same inversion one level down. + // + // The bundles are loaded by `run` and are EMPTY when a module will not + // load — a config judged away from its own tree is the reachable case, + // and `cli.rs`'s `repo_with_committed_config` fixtures are exactly + // that: the committed `batten.toml` without `policy/*.rego` beside it. + // With no bundle to ask, a predicate id is indistinguishable from a + // typo, so reporting one would go back to refusing the spelling that + // suppresses — the defect this arm exists to remove. + // + // Narrow on purpose: it abstains only where the config DECLARES a + // policy rule and nothing resolved, so a config with no policy rules + // at all keeps the smell's full reach, which is the corpus CLOUD-208 + // opened it for. + None if bundles.is_empty() + && config + .rules + .iter() + .any(|rule| rule.kind == crate::rules::RuleKind::Policy) => {} None => found.push(Smell { at: at.clone(), id: WAIVER_NAMES_NO_RULE, From 14a3fc0ba16c74b263eb3a2004143ad6dd11ec10 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 17/21] fix(receipt): resolve the delta identity from the repo root, not the cwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ninth code review over this branch found it, and it would have shipped a gate that refuses everything. `delta_subject` opened `Path::new(".")` where the mint opens the repo root. `batten hook` runs with whatever cwd the harness had — routinely a subdirectory, sometimes outside the repository — so the read side reached a different repository or none, `branch_patch_id` answered `None`, and because an absent identity is `Missing` rather than could-not-look at that call site, a `delta`-keyed row refused EVERY call whatever receipt was on disk. The two halves disagreeing about *where the repository is* is the same class as the two halves disagreeing about *what the same change is*, which the function's own doc already warns against one paragraph up: the mint and the reader must resolve one identity. They call the same function; they now open the same tree. Invisible to all eight existing cases because they run at the fixture root, so the new case runs `adjudicate` from `crates/batten/src` and asserts the same receipt still answers. It checks the root arm first, so a refusal from the nested arm is attributable rather than ambiguous. Refs: CLOUD-1547 --- crates/batten/src/receipt.rs | 20 ++++++++--- .../batten/tests/it/review_receipt_delta.rs | 33 +++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/crates/batten/src/receipt.rs b/crates/batten/src/receipt.rs index 6f5ac225d..70b4cf7ec 100644 --- a/crates/batten/src/receipt.rs +++ b/crates/batten/src/receipt.rs @@ -729,8 +729,18 @@ pub(crate) fn safe_subject(subject: &str) -> bool { /// a path component and the writer refuses exactly what the reader refuses — the /// two halves disagreeing about which filenames exist is the confusion /// [`safe_subject`]'s own doc records. -fn delta_subject(base: &str) -> Option { - git::branch_patch_id(Path::new("."), base) +/// **Resolved against the REPO ROOT the caller already holds, never `"."`.** +/// `batten hook` is invoked with whatever cwd the harness had, which is routinely +/// a subdirectory and can be outside the repository altogether. Opening `"."` +/// there reaches a different repository or none, `branch_patch_id` answers `None`, +/// and — because `None` is `Missing` at the call site rather than could-not-look — +/// a `delta`-keyed row would refuse EVERY call whatever receipt is on disk. That +/// is the direction that gets a guard switched off, and it is invisible from any +/// fixture that happens to run at the root. Every other arm here already reads +/// [`RepoFacts`]; this one now does too, so a single reading of "where is the +/// repository" serves them all. +fn delta_subject(repo_root: &str, base: &str) -> Option { + git::branch_patch_id(Path::new(repo_root), base) .ok() .flatten() .filter(|identity| safe_subject(identity)) @@ -857,7 +867,7 @@ pub(crate) fn verdicts( // clear, where the permissive direction is silent. ReceiptKey::Delta => key_bases .get(check) - .and_then(|base| delta_subject(base)) + .and_then(|base| delta_subject(&facts.repo_root, base)) .map_or(Validity::Missing, |identity| { named_validity(&facts.git_dir, check, &identity) }), @@ -877,7 +887,7 @@ pub(crate) fn verdicts( named.as_deref(), key_bases .get(check) - .and_then(|base| delta_subject(base)) + .and_then(|base| delta_subject(&facts.repo_root, base)) .as_deref(), ) .filter(|path| older_than(path, max_age, now)) @@ -900,7 +910,7 @@ pub(crate) fn verdicts( named.as_deref(), key_bases .get(check) - .and_then(|base| delta_subject(base)) + .and_then(|base| delta_subject(&facts.repo_root, base)) .as_deref(), ) .map_or(Validity::Valid, |path| { diff --git a/crates/batten/tests/it/review_receipt_delta.rs b/crates/batten/tests/it/review_receipt_delta.rs index 503657238..2358c97f6 100644 --- a/crates/batten/tests/it/review_receipt_delta.rs +++ b/crates/batten/tests/it/review_receipt_delta.rs @@ -296,6 +296,39 @@ fn a_push_carrying_an_unreviewed_change_is_refused() { denied(&push(&dir)); } +/// THE IDENTITY IS RESOLVED AGAINST THE REPO ROOT, NOT THE CWD. +/// +/// `batten hook` runs with whatever cwd the harness had, which is routinely a +/// subdirectory. Resolving the patch id from `"."` opens a different repository +/// or none, yields no identity, and — because an absent identity is `Missing` +/// rather than could-not-look here — refuses EVERY call whatever receipt is on +/// disk. Every other case in this file runs at the root and so cannot see it; +/// this one is the whole reason the read side takes `RepoFacts` like its +/// siblings. Declared mutation: `subject-read-from-cwd`. +#[test] +fn the_identity_is_resolved_from_the_repo_root_and_not_the_cwd() { + let dir = fixture("review-delta-subdir", "delta"); + change(&dir, "fn a() {}\n"); + file_receipt(&dir, &identity(&dir)); + + // The receipt answers at the root, so the fixture is sound before the arm + // that matters runs — otherwise a refusal below would be unattributable. + allowed(&ready(&dir)); + + let nested = dir.join("crates/batten/src"); + assert!(nested.is_dir(), "the change created the nested path"); + let envelope = serde_json::json!({ + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": "gh pr ready 999"}, + }); + allowed(&run_with_stdin( + &nested, + &["adjudicate", "--harness", "claude-code"], + &envelope.to_string(), + )); +} + // --------------------------------------------------------------------------- // COULD-NOT-LOOK IS A REFUSAL HERE, DELIBERATELY. // --------------------------------------------------------------------------- From de6236f2c4f26c05ca6415af5e948715ead0e824 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 18/21] perf(receipt): resolve the delta identity once per check, not three times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tenth code review over this branch found it. `delta_subject` was recomputed at each of the three sites that want it — the validity arm, the `max_age` branch and the `requires_field` branch — and each call is a repository open plus a merge-base walk and a tree diff. A `delta`-keyed row declaring both `max_age` and `requires_field` therefore paid three of them on every mediated call, against the ~100 ms budget `perf-assert` holds this path to. Hoisted to one binding per CHECK, which is the narrowing the arm's own comment already argued for: the base is a property of the ROW, so two rows keying on different bases must not answer from whichever was read first — but within one check there is exactly one base and therefore exactly one identity, so recomputing it bought nothing. Only a `delta`-keyed check pays anything: every other keying leaves the binding `None` without opening a repository, which is the same cheap-when-irrelevant posture `max_age` takes one arm below. No verdict moves — the `Option` is the same three-valued answer each site already read, so this is the identical decision made once. Refs: CLOUD-1547 --- crates/batten/src/receipt.rs | 44 ++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/crates/batten/src/receipt.rs b/crates/batten/src/receipt.rs index 70b4cf7ec..0f4198793 100644 --- a/crates/batten/src/receipt.rs +++ b/crates/batten/src/receipt.rs @@ -834,6 +834,33 @@ pub(crate) fn verdicts( checks .iter() .map(|(check, key)| { + // RESOLVED ONCE PER CHECK, and the three readings below share it. + // + // Per CHECK rather than per call, which is the narrowing the arm + // below states: the base is a property of the row, so two rows + // keying on different bases must not answer from whichever was + // read first. Within one check there is exactly one base and + // therefore exactly one identity, so recomputing it was buying + // nothing. + // + // It is not free to recompute: each call is a repository open + // plus a merge-base walk and a tree diff, and the validity, the + // `max_age` branch and the `requires_field` branch each wanted + // it — so a row declaring both paid THREE of them on every + // mediated call, against the ~100 ms budget `perf-assert` holds + // this path to. `Option` is the same three-valued answer the + // arms already read; computing it here changes no verdict. + // + // Only a `delta`-keyed check pays anything at all: every other + // keying leaves this `None` without opening a repository, which + // is the same cheap-when-irrelevant `max_age` takes below. + let delta_identity = if *key == ReceiptKey::Delta { + key_bases + .get(check) + .and_then(|base| delta_subject(&facts.repo_root, base)) + } else { + None + }; let verdict = match key { ReceiptKey::Head => { let statement = receipt_path(&facts.repo_root, check) @@ -865,11 +892,10 @@ pub(crate) fn verdicts( // resolve could push anything. A branch with nothing to // review is refused and says so, which is loud and cheap to // clear, where the permissive direction is silent. - ReceiptKey::Delta => key_bases - .get(check) - .and_then(|base| delta_subject(&facts.repo_root, base)) + ReceiptKey::Delta => delta_identity + .as_ref() .map_or(Validity::Missing, |identity| { - named_validity(&facts.git_dir, check, &identity) + named_validity(&facts.git_dir, check, identity) }), }; // THE AGE IS READ LAST, AND ONLY OVER A RECEIPT THAT WAS @@ -885,10 +911,7 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), - key_bases - .get(check) - .and_then(|base| delta_subject(&facts.repo_root, base)) - .as_deref(), + delta_identity.as_deref(), ) .filter(|path| older_than(path, max_age, now)) .map_or(Validity::Valid, |_| Validity::Expired), @@ -908,10 +931,7 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), - key_bases - .get(check) - .and_then(|base| delta_subject(&facts.repo_root, base)) - .as_deref(), + delta_identity.as_deref(), ) .map_or(Validity::Valid, |path| { if field_refutes(&path, bound) { From 6b58475358a6dc5a4a1817f018e65b2d7c0d2f43 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 19/21] docs(receipt): the delta arm's comment described the code before the hoist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1adec082 moved the identity into a per-check binding and left the arm saying it is "NOT resolved once above", which is now the opposite of what the code does — and it attributed the two-rows-two-bases hazard to the arm, where it is actually handled by the per-check map plus the key guard. The eleventh review over this branch caught it. A comment that tells the next reader the opposite of the code is worse than none here in particular: the whole arm exists because its answer is deliberately the inverse of its two neighbours, so a reader arriving to check that reasoning is exactly the reader this would mislead. Refs: CLOUD-1547 --- crates/batten/src/receipt.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/batten/src/receipt.rs b/crates/batten/src/receipt.rs index 0f4198793..d5059736a 100644 --- a/crates/batten/src/receipt.rs +++ b/crates/batten/src/receipt.rs @@ -879,12 +879,15 @@ pub(crate) fn verdicts( ReceiptKey::Named => named.as_ref().map_or(Validity::Missing, |value| { named_validity(&facts.git_dir, check, value) }), - // NOT resolved once above like `branch` and `named`, because - // the base is PER ROW rather than per call: two rows may key - // on different bases, and hoisting one identity would answer - // both from whichever row happened to be read first. + // Resolved once per CHECK rather than once per call, which is + // the distinction `branch` and `named` do not have to make: + // the base is a property of the ROW, so a call-level hoist + // would answer two rows keying on different bases from + // whichever was read first. `delta_identity` above is that + // per-check binding, and the `*key == ReceiptKey::Delta` + // guard is what keeps every other keying from paying for it. // - // AND `Missing` RATHER THAN COULD-NOT-LOOK, which is the + // `Missing` RATHER THAN COULD-NOT-LOOK, which is the // opposite of the two arms above and is the deliberate half // (CLOUD-1547). An unresolvable base or an empty diff means // there is no change to have reviewed; answering could-not- From b3c2cc690c090aa1fa78f9fa7937f54496ca2bc8 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 20/21] refactor(lint): extract the waiver diagnostics, which put smells over the line ceiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-1553's two arms pushed `smells` to 105 lines against clippy's 100, and the waiver loop is the self-contained half to lift out: it reads the waiver table, the rules those waivers name, the predicate ids the bundles publish, and the date — nothing else in `smells` and nothing after it. No verdict and no ordering move. The caller extends at the position the loop occupied, and the whole list is sorted by (at, id) afterwards regardless, which is what keeps the report byte-stable under §6. Refs: CLOUD-1553 --- crates/batten/src/lint.rs | 180 +++++++++++++++++++++----------------- 1 file changed, 100 insertions(+), 80 deletions(-) diff --git a/crates/batten/src/lint.rs b/crates/batten/src/lint.rs index 8b410def5..011cf024f 100644 --- a/crates/batten/src/lint.rs +++ b/crates/batten/src/lint.rs @@ -274,6 +274,105 @@ struct LocatedWaiver { path: Option, } +/// The dead-suppression diagnostics over the `[[waiver]]` table (CLOUD-208). +/// +/// **Its own function because [`smells`] is at clippy's line ceiling** and this +/// loop is the self-contained half: it reads the waiver table, the rules those +/// waivers name, the predicate ids the bundles publish, and the date — nothing +/// else in `smells` and nothing after it. Splitting it moves no verdict and no +/// ordering, because the caller extends at the position the loop occupied and +/// the whole list is sorted afterwards anyway. +fn waiver_smells( + text: &str, + located: &Located, + config: &Config, + bundles: &[crate::policy::Bundle], + today: crate::waiver::Date, +) -> Vec { + let mut found = Vec::new(); + for (waiver, parsed) in located.waivers.iter().zip(&config.waivers) { + let at = Where::Line(line_of(text, waiver.rule.span().start)); + match config + .rules + .iter() + .find(|rule| rule.id == *waiver.rule.get_ref()) + { + // A PREDICATE ID IS A WAIVABLE NAME TOO, and reading only `rules` + // here inverted this smell for every policy rule whose module + // publishes an id of its own (CLOUD-1553). + // + // `waiver::apply` filters FINDINGS, and a policy finding carries the + // predicate id rather than the row's. Measured on this repository: + // `rule = "filed-over-own-diff"` suppressed and was refused here, + // while `rule = "filed-here"` was clean here and suppressed nothing — + // so no value satisfied both halves, and the one this smell blessed + // was the dead one. That is precisely the "exemption someone is + // relying on" CLOUD-208 opened it to catch, produced by the catcher. + // + // The bundles are already in hand for the caller's set analysis, so + // this acquires nothing: `Bundle::declared` is the module's own + // published set, the same authority `attribute` resolves a + // violation's id against. Reading it rather than re-deriving from the + // module source keeps one authority over what a bundle declares. + None if bundles + .iter() + .any(|bundle| bundle.declared().contains(waiver.rule.get_ref())) => {} + // COULD-NOT-LOOK, and it is the same inversion one level down. + // + // The bundles are loaded by `run` and are EMPTY when a module will not + // load — a config judged away from its own tree is the reachable case, + // and `cli.rs`'s `repo_with_committed_config` fixtures are exactly + // that: the committed `batten.toml` without `policy/*.rego` beside it. + // With no bundle to ask, a predicate id is indistinguishable from a + // typo, so reporting one would go back to refusing the spelling that + // suppresses — the defect the arm above exists to remove. + // + // Narrow on purpose: it abstains only where the config DECLARES a + // policy rule and nothing resolved, so a config with no policy rules + // at all keeps the smell's full reach, which is the corpus CLOUD-208 + // opened it for. + None if bundles.is_empty() + && config + .rules + .iter() + .any(|rule| rule.kind == crate::rules::RuleKind::Policy) => {} + None => found.push(Smell { + at: at.clone(), + id: WAIVER_NAMES_NO_RULE, + }), + // The rule exists and still cannot be waived: `apply` filters + // findings, and this kind mints none (`waiver::reaches` says which, + // and says it once — this module must not carry a second list that + // can disagree with the filter it describes). + // + // Located by key rather than line, which is what carries the + // unreachable kind alongside the waiver's identity in one pointer — + // `host_drift` composes a `Where::Key` for the same reason. The key is + // distinct per waiver, so two of them cannot collapse under `dedup` + // (CLOUD-233). + Some(rule) if !crate::waiver::reaches(rule.kind) => found.push(Smell { + at: Where::Key(format!("{} {}", parsed.key(), rule.kind.as_str())), + id: WAIVER_UNREACHABLE_KIND, + }), + Some(_) => {} + } + // The expiry is a date, and `today` is the injected input the module docs + // in `crate::waiver` explain: the smell list for a given config is a + // function of (bytes, date), never of when the process happened to start. + if crate::waiver::Date::parse(&waiver.expires).is_ok_and(|expiry| expiry < today) { + found.push(Smell { + at, + id: WAIVER_EXPIRED, + }); + } + // `path` is read but not linted: whether a glob matches anything is a + // question about the tree, not about this file, and answering it here + // would be the runtime diagnostic the caller's comment names. + let _ = &waiver.path; + } + found +} + /// Convert a byte offset into a 1-based line number. fn line_of(text: &str, offset: usize) -> usize { text.get(..offset) @@ -440,86 +539,7 @@ pub fn smells( // index `i` is one row seen two ways. That pairing is what lets a pointer // reuse `Waiver::key`'s single rendering instead of re-deriving `rule` and // `path` into a second spelling of the same identity. - for (waiver, parsed) in located.waivers.iter().zip(&config.waivers) { - let at = Where::Line(line_of(text, waiver.rule.span().start)); - match config - .rules - .iter() - .find(|rule| rule.id == *waiver.rule.get_ref()) - { - // A PREDICATE ID IS A WAIVABLE NAME TOO, and reading only `rules` - // here inverted this smell for every policy rule whose module - // publishes an id of its own (CLOUD-1553). - // - // `waiver::apply` filters FINDINGS, and a policy finding carries the - // predicate id rather than the row's. Measured on this repository: - // `rule = "filed-over-own-diff"` suppressed and was refused here, - // while `rule = "filed-here"` was clean here and suppressed nothing — - // so no value satisfied both halves, and the one this smell blessed - // was the dead one. That is precisely the "exemption someone is - // relying on" CLOUD-208 opened it to catch, produced by the catcher. - // - // The bundles are already in hand for the set analysis below, so this - // acquires nothing: `Bundle::declared` is the module's own published - // set, which is the same authority `attribute` resolves a violation's - // id against. Reading it here rather than re-deriving from the module - // source keeps one authority over what a bundle declares. - None if bundles - .iter() - .any(|bundle| bundle.declared().contains(waiver.rule.get_ref())) => {} - // COULD-NOT-LOOK, and it is the same inversion one level down. - // - // The bundles are loaded by `run` and are EMPTY when a module will not - // load — a config judged away from its own tree is the reachable case, - // and `cli.rs`'s `repo_with_committed_config` fixtures are exactly - // that: the committed `batten.toml` without `policy/*.rego` beside it. - // With no bundle to ask, a predicate id is indistinguishable from a - // typo, so reporting one would go back to refusing the spelling that - // suppresses — the defect this arm exists to remove. - // - // Narrow on purpose: it abstains only where the config DECLARES a - // policy rule and nothing resolved, so a config with no policy rules - // at all keeps the smell's full reach, which is the corpus CLOUD-208 - // opened it for. - None if bundles.is_empty() - && config - .rules - .iter() - .any(|rule| rule.kind == crate::rules::RuleKind::Policy) => {} - None => found.push(Smell { - at: at.clone(), - id: WAIVER_NAMES_NO_RULE, - }), - // The rule exists and still cannot be waived: `apply` filters - // findings, and this kind mints none (`waiver::reaches` says which, - // and says it once — this module must not carry a second list that - // can disagree with the filter it describes). - // - // Located by key rather than line, which is what carries the - // unreachable kind alongside the waiver's identity in one pointer — - // `host_drift` below composes a `Where::Key` for the same reason. The - // key is distinct per waiver, so two of them cannot collapse under - // `dedup` (CLOUD-233). - Some(rule) if !crate::waiver::reaches(rule.kind) => found.push(Smell { - at: Where::Key(format!("{} {}", parsed.key(), rule.kind.as_str())), - id: WAIVER_UNREACHABLE_KIND, - }), - Some(_) => {} - } - // The expiry is a date, and `today` is the injected input the module docs - // in `crate::waiver` explain: the smell list for a given config is a - // function of (bytes, date), never of when the process happened to start. - if crate::waiver::Date::parse(&waiver.expires).is_ok_and(|expiry| expiry < today) { - found.push(Smell { - at, - id: WAIVER_EXPIRED, - }); - } - // `path` is read but not linted: whether a glob matches anything is a - // question about the tree, not about this file, and answering it here - // would be the runtime diagnostic above wearing a disguise. - let _ = &waiver.path; - } + found.extend(waiver_smells(text, &located, &config, bundles, today)); // `judge-over-protected-unstated` used to live here (CLOUD-135). It is gone // with the key it asked about: protected content now refuses the whole From 6eb08e1468c244ea3a4a3791d482d15c83e6c6ab Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 18:38:29 +0000 Subject: [PATCH 21/21] style(trust): the outcome census rationale carried collapsed-line whitespace The literal was written as one long string with the source indentation folded into it, so the emitted census text read "the classifier rides the advisory channel". Every neighbouring entry uses backslash-continued strings; this one now does too. Cosmetic in the source and not in the output: the census is a document a reader is meant to read, so multi-space gaps in it are a defect in what the engine says rather than in how it is typed. Refs: CLOUD-1547 --- crates/batten/src/trust.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/batten/src/trust.rs b/crates/batten/src/trust.rs index 1c7721bf2..62ce7449a 100644 --- a/crates/batten/src/trust.rs +++ b/crates/batten/src/trust.rs @@ -1108,7 +1108,11 @@ pub const CENSUS: &[FieldCoverage] = &[ FieldCoverage { field: "outcome", coverage: Coverage::NotPolicyBearing( - "a post-tool signature ADVISES and cannot refuse (CLOUD-945): the classifier rides the advisory channel at a batch boundary, so narrowing the table changes what a session is TOLD and never whether a call is allowed. Removing a row makes the engine say LESS, which is the direction a weakening check does not need to guard — and adding one cannot deny, because no arm here reaches a verdict", + "a post-tool signature ADVISES and cannot refuse (CLOUD-945): the classifier \ + rides the advisory channel at a batch boundary, so narrowing the table changes \ + what a session is TOLD and never whether a call is allowed. Removing a row makes \ + the engine say LESS, which is the direction a weakening check does not need to \ + guard — and adding one cannot deny, because no arm here reaches a verdict", ), }, FieldCoverage {