diff --git a/.claude/rules/policy-modules.md b/.claude/rules/policy-modules.md index 35fbe0f1e..d7bddd317 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..f51e74abf 100644 --- a/batten.toml +++ b/batten.toml @@ -1292,6 +1292,84 @@ 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 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 +# 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 +5897,96 @@ 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. +# +# 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" +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 # `hook-profile-check`). @@ -6842,6 +7010,51 @@ 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 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-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" + [[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." @@ -11385,6 +11598,64 @@ 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. +""" +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. +# +# 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. +# +# 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" +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/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/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..ff00bc0c7 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. @@ -2745,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(); @@ -2875,6 +2907,22 @@ 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); + }; + // `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() + .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/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 63ea18523..37652933d 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)) @@ -8420,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, @@ -11230,6 +11243,35 @@ 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. +/// +/// **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)] +pub fn mint_receipts_for_test( + declared: &[crate::mint::Declared], + tool: &str, + input: &serde_json::Value, + result: &serde_json::Value, + root: &Path, + grammar: Option<&ready::Grammar>, +) { + mint_receipts(declared, tool, input, result, root, grammar); +} + /// Write every receipt these rows mint from one already-unframed result. /// /// **ONE minting authority, reached from two boundaries** (CLOUD-1264). The @@ -11261,6 +11303,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 +11319,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 +11349,39 @@ fn mint_receipts( }; format!("{}.{}", mint.name, branch.replace('/', "-")) } + // 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, which is what + // makes the receipt survive the landing loop's per-lap rebase. + // + // 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; + }; + 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 @@ -11322,22 +11412,49 @@ 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() { - return; - } + // THERE IS NO CHEAP GATE HERE, AND THE TWO THAT WERE TRIED WERE BOTH WRONG + // (CLOUD-1484). + // + // 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 + // 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 // 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 +11477,7 @@ fn record_mints(overrides: &Overrides, envelope: &hook::Envelope) { mint_receipts( declared, &envelope.raw_tool, + &envelope.input, &result, root, grammar.as_ref(), @@ -13355,7 +13473,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/lint.rs b/crates/batten/src/lint.rs index 794a12b99..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,47 +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()) - { - 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 diff --git a/crates/batten/src/mint.rs b/crates/batten/src/mint.rs index 1dbf9000f..f59b8d80c 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")] @@ -191,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 @@ -324,6 +402,40 @@ 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 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 +/// 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, input: &serde_json::Value) -> bool { + let (Some(path), Some(expected)) = + (declared.selects_at.as_deref(), declared.selects.as_deref()) + else { + return true; + }; + scalar(input, 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 { @@ -412,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 { @@ -503,6 +627,72 @@ 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 + ))); + } + // 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 + // 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 + ))); + } + // 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()), + ] { + // 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() != text) + { + return Err(crate::error::UsageError::raise(format!( + "`[[mint]]` `{}` declares a `{column}` that is empty or padded, which can \ + never match", + mint.name + ))); + } + } if let Err(problem) = parse(&mint.body) { return Err(crate::error::UsageError::raise(format!( "`[[mint]]` `{}` has an unreadable `body`: {problem}", @@ -532,9 +722,158 @@ pub fn validate(mints: &[Declared]) -> anyhow::Result<()> { } #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] 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 `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] + 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`" + ); + // 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] 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 @@ -597,6 +936,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/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/receipt.rs b/crates/batten/src/receipt.rs index 464da3309..d5059736a 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,72 @@ 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. +/// **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)) +} + /// 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 +801,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, @@ -769,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) @@ -787,6 +879,27 @@ pub(crate) fn verdicts( ReceiptKey::Named => named.as_ref().map_or(Validity::Missing, |value| { named_validity(&facts.git_dir, check, value) }), + // 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. + // + // `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 => delta_identity + .as_ref() + .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 +914,7 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), + delta_identity.as_deref(), ) .filter(|path| older_than(path, max_age, now)) .map_or(Validity::Valid, |_| Validity::Expired), @@ -820,6 +934,7 @@ pub(crate) fn verdicts( *key, branch.as_ref().map(|(branch, _)| branch.as_str()), named.as_deref(), + delta_identity.as_deref(), ) .map_or(Validity::Valid, |path| { if field_refutes(&path, bound) { @@ -1504,6 +1619,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 +1721,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 4bd075bb8..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", @@ -1336,6 +1337,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 +2619,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). @@ -3158,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"), @@ -3887,6 +3938,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 +4051,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, @@ -4277,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 @@ -4300,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()), @@ -7908,7 +8007,25 @@ 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()) + // 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, }, @@ -13483,6 +13600,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/trust.rs b/crates/batten/src/trust.rs index 803def184..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 { @@ -5059,6 +5063,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/main.rs b/crates/batten/tests/it/main.rs index 81d58e300..c5cb13a65 100644 --- a/crates/batten/tests/it/main.rs +++ b/crates/batten/tests/it/main.rs @@ -208,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..2358c97f6 --- /dev/null +++ b/crates/batten/tests/it/review_receipt_delta.rs @@ -0,0 +1,348 @@ +//! 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, 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 { + // `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 { + "" + }; + // 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\ + 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" + )) + .file("README.md", "base\n") + .git() + .base_commit() + .build() +} + +/// 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)); +} + +/// 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. +// --------------------------------------------------------------------------- + +/// 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/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 d966ac375..c4fff1003 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" } ] }, @@ -2704,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" } ] }, @@ -3326,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": [ 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": [