From 1c182778a0a099e888a134eba1042e774e89fea5 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 20:00:19 +0000 Subject: [PATCH 01/12] fix(rules): gate the two Cost::Effect facts on the effect surface, not declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_static` refuses a spawning KIND before any work, and that read as the whole of house-style §5's read-only promise. It is not: `Fact::Symbols` and `Fact::Review` are `Cost::Effect` FACTS, and no partition over `RuleKind` reaches them. Both were guarded by declaration alone, so the read-effect surface resolved them — `symbols_fact` spawning `cargo clippy` over the whole crate, and `review_fact` able to dispatch an agent. `run_recorded`'s own doc asserted the opposite in as many words: the no-spawn property was "a property of the argument list rather than a promise". True of every kind, false of both facts, and it stood while the Stop-surface recorder shelled out to a compiler. That paragraph is corrected rather than deleted. `RunKind` already existed and `run_over` already dispatched on it; it simply never reached `run`. It does now, and the two facts answer `Look::IsNot` off the effect surface — could-not-look rather than a skip, because an empty census reads as "resolved, found nothing", which is a measured claim about a crate nobody analysed. MEASURED, AND THIS DOES NOT FIX THE LATENCY IT WAS FOUND CHASING. `batten hook` on a Stop payload goes from 23 execve to 1, so no compiler runs at the mediated boundary any more. Wall clock moves 114.8s -> 106.6s, which is inside this container's variance: the spawn was real, cached, and never the dominant term. The remaining ~106s is unexplained and CLOUD-1480 stays open on it. Recorded that way deliberately — a commit claiming the fix it did not deliver is worse than the defect. Refs: CLOUD-1480 --- crates/batten/src/rules.rs | 75 ++++++++++++++++++++++++++++++++------ 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 2dcba19bf..76f9e4592 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -6167,7 +6167,16 @@ fn run_static_inner( )); } } - run(rules, &[], root, &bundles, vocabulary, scope, now) + run( + rules, + &[], + root, + &bundles, + vocabulary, + scope, + now, + RunKind::Static, + ) } /// Run only the rules that cannot spawn a process, and report the ones that can @@ -6191,13 +6200,19 @@ fn run_static_inner( /// so there the only honest answer is to refuse. Same omission, two surfaces, /// two correct answers. /// -/// Nothing here spawns: the withheld rules are partitioned out *before* -/// [`run`] sees them, so the no-user-code-behind-a-store-write property is a -/// property of the argument list rather than a promise. The partition asks -/// [`RuleKind::carries_ambient_authority`] — the same question [`run_static`] -/// refuses on, deliberately the identical call rather than a second predicate, -/// so the two surfaces can disagree about what to DO with such a kind and never -/// about which kinds they are. +/// Nothing here spawns, and **the argument list is only half of why** +/// (CLOUD-1480). The withheld rules are partitioned out *before* [`run`] sees +/// them, which the previous revision of this paragraph called "a property of +/// the argument list rather than a promise" — true of every spawning KIND and +/// false of the two `Cost::Effect` FACTS, which no partition over +/// [`RuleKind`] can reach. `symbols` spawned `cargo clippy` from this surface +/// for as long as that sentence stood. The surface is passed to [`run`] now and +/// the facts are gated on it, so the property holds on both halves. +/// +/// The partition asks [`RuleKind::carries_ambient_authority`] — the same +/// question [`run_static`] refuses on, deliberately the identical call rather +/// than a second predicate, so the two surfaces can disagree about what to DO +/// with such a kind and never about which kinds they are. /// /// # Errors /// @@ -6230,6 +6245,7 @@ pub fn run_recorded( // The Stop-surface recorder supplies none, and `None` is the honest // answer rather than a clock read this module may not make. None, + RunKind::Static, )?; for rule in withheld { // `RuleSkipped`, not a variant of its own. The distinction between "the @@ -6300,7 +6316,16 @@ fn run_all_inner( } } let bundles = crate::policy::load(root, rules, vocabulary, checks, None)?; - run(rules, provisions, root, &bundles, vocabulary, scope, now) + run( + rules, + provisions, + root, + &bundles, + vocabulary, + scope, + now, + RunKind::All, + ) } /// Run every rule in `rules` against the tree rooted at `root`, returning all @@ -6327,6 +6352,14 @@ fn run( // The instant the BOUNDARY read, threaded to `minted_facts` rather than // read here — see `RunOptions::now` for why this module may not read one. now: Option, + // WHICH EFFECT SURFACE THIS RUN IS ON, threaded here rather than left with + // the caller (CLOUD-1480). `run_static` refuses a spawning KIND before any + // work, and that read as the whole of §5's read-only promise — but a + // `Cost::Effect` FACT is not a kind, so `symbols_fact` and `review_fact` + // below were guarded by DECLARATION alone and spawned on the read surface. + // Measured: `batten hook` on a Stop payload exec'd `cargo clippy` and took + // 114.8s against a published 100ms budget. + kind: RunKind, ) -> anyhow::Result { let recorders = vocabulary.recorders; let files = tree_files(root)?; @@ -6399,8 +6432,28 @@ fn run( // The one acquisition of the `Cost::Effect` fact (CLOUD-760), beside the git // family and for the same reason: a projection must not spawn, so the spend // happens once here and only when a row declared it. - let symbols = symbols_fact(rules, root); - let review = review_fact(rules, root); + // THE TWO `Cost::Effect` FACTS, AND THE SURFACE IS THE OTHER HALF OF THE + // GUARD (CLOUD-1480). Declaration alone was never enough: `Fact::Symbols` + // is classed `Cost::Effect` x `Surface::Check`, and `Surface::Check` names + // the NARROWEST surface it may be resolved on — so resolving it from the + // read-effect surface contradicts the class the fact already carries. + // + // COULD-NOT-LOOK RATHER THAN A SKIP, for the reason both facts' own headers + // give: an empty census would read as "resolved, found nothing", which is a + // measured answer about a crate nobody analysed. `IsNot` is what the + // undeclared arm already returns and it is the honest one here too — the + // projection emits `null`, and a module reads undefined. + let effects_admitted = matches!(kind, RunKind::All); + let symbols = if effects_admitted { + symbols_fact(rules, root) + } else { + crate::facts::Look::IsNot + }; + let review = if effects_admitted { + review_fact(rules, root) + } else { + crate::facts::Look::IsNot + }; // THE OUT-OF-ROOT FILES (CLOUD-1167), acquired once for the whole run beside // the families above and, like every one of them, ONLY FOR WHAT A ROW // DECLARED. A ruleset naming no `[[rule.external]]` reads no environment From 333dff93ff79e5829e79648efc1fb63201e91929 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 20:12:20 +0000 Subject: [PATCH 02/12] perf(rules): let the mediated caller choose its config-fault checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_recorded` hardcoded `ModuleChecks::Run`, so the Stop-surface recorder re-derived every registered module's smoke query at the end of every turn. `SkipOnHotPath` exists for exactly this caller and says so in its own words — "the caller is the mediated path, where the answer is already known and the budget is per call" — and nothing was passing it. The checks are the caller's now: `batten state record` keeps `Run`, because a human running the verb is entitled to hear about a broken module; `record_state` passes `SkipOnHotPath`. MEASURED, AND IT DOES NOT MOVE THE NUMBER. Stop goes 106.6s -> 110.0s, which is noise on this container. Kept because it is right on its own terms and because recording a negative result is what stops the next reader re-running it, not because it fixed anything. CLOUD-1480 stays open. What is established: `batten check` opens each of the 53 modules once, the Stop path opens them ~11x, and a sampling profiler puts ~70% of frames in serde_json under `policy::collect_bound_values` and `collect_literals`. What is NOT established is the arithmetic — 11 loads at `check`'s whole 1.66s is ~18s, not 110s — so the multiplier is real and is not by itself the explanation. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 23 ++++++++++++++++++++--- crates/batten/src/rules.rs | 15 ++++++++------- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 304654ec1..e6e3ecd52 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -365,7 +365,9 @@ pub fn run(cli: Cli, mode: Mode, out: &mut dyn Write, err: &mut dyn Write) -> Re // not a policy question and no `batten.toml` may answer it. Some(Command::State { command }) => match command { StateCommand::Adopt { store } => store::run_adopt(store.as_deref(), err), - StateCommand::Record => run_state_record(&overrides, mode, err), + StateCommand::Record => { + run_state_record(&overrides, mode, err, policy::ModuleChecks::Run) + } StateCommand::Migrate => run_state_migrate(err), StateCommand::Settle { identity, @@ -1091,7 +1093,14 @@ fn run_defects_add( /// lineage edge and advances **its own** fold position /// ([`session::HOLDER_RECORD`]) — never a drain's, since this verb folds shards /// whether or not anything reached an agent (CLOUD-83). -fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> Result { +fn run_state_record( + overrides: &Overrides, + mode: Mode, + err: &mut dyn Write, + // Threaded to `run_recorded` (CLOUD-1480): the verb reports config + // faults, the mediated Stop path may not afford to re-derive them. + checks: policy::ModuleChecks, +) -> Result { let repo = git::repo_root(Path::new("."))?; // **The ref comes from HERE, not from `repo`.** `repo_root` answers with the // MAIN worktree's root — which is exactly what makes every linked worktree @@ -1125,6 +1134,7 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R recorders: &config.recorders, }, Path::new("."), + checks, )?; if !scan.not_evaluated.is_empty() { // Never silent: a rule that did not look must say so, or a clean-looking @@ -10516,7 +10526,14 @@ const UNLANDED_BYPASS: &str = "BATTEN_UNLANDED_CHECK_BYPASS"; /// never be the reason a turn stalls. fn record_state(overrides: &Overrides) { let mut sink = std::io::sink(); - let _ = run_state_record(overrides, Mode::default(), &mut sink); + // `SkipOnHotPath`, because this is the hot path (CLOUD-1480). The verb below + // is a human asking about their config; this is the end of every turn. + let _ = run_state_record( + overrides, + Mode::default(), + &mut sink, + policy::ModuleChecks::SkipOnHotPath, + ); } /// The `completion.unlanded` verdict for this branch, or nothing (CLOUD-1163). diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 76f9e4592..42021f717 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -6223,18 +6223,19 @@ pub fn run_recorded( provisions: &[crate::provision::Provision], vocabulary: crate::policy::Vocabulary<'_>, root: &Path, + // WHICH CONFIG-FAULT CHECKS THIS CALLER IS ENTITLED TO (CLOUD-1480). The + // `state record` VERB reports config faults and passes `Run`; the mediated + // Stop path does not — it is the caller `SkipOnHotPath` was written for, in + // that variant's own words: "the caller is the mediated path, where the + // answer is already known and the budget is per call". Hardcoding `Run` here + // made the end-of-turn boundary re-derive every module's smoke query. + checks: crate::policy::ModuleChecks, ) -> anyhow::Result { let (evaluable, withheld): (Vec<&Rule>, Vec<&Rule>) = rules .iter() .partition(|rule| !rule.kind.carries_ambient_authority()); let evaluable: Vec = evaluable.into_iter().cloned().collect(); - let bundles = crate::policy::load( - root, - &evaluable, - vocabulary, - crate::policy::ModuleChecks::Run, - None, - )?; + let bundles = crate::policy::load(root, &evaluable, vocabulary, checks, None)?; let mut scan = run( &evaluable, provisions, From 630d32549d2a991f37cdd1d2898a517aad255b52 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 20:28:43 +0000 Subject: [PATCH 03/12] fix(rules): gate the effect facts on Surface, not on the dispatch enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `d9fce7a8` gated `symbols` and `review` on `RunKind`, which is a DISPATCH enum: `check`, `baseline` and the mediated recorder all arrive as `Static`. So the gate disabled both facts on the read surface their own `Class` (`Cost::Effect x Surface::Check`) explicitly admits — `Surface::Check` names the NARROWEST surface a fact may resolve on, and `admits` is one-directional. I read it as "enforce only" and inverted the direction. Measured: `review_dispatched::a_dispatched_review_reaches_the_predicate_and_is_clean` went red at `the miss dispatched exactly once / left: 0, right: 1`. Caught by review, not by me. Green again on this commit. `Class::resolvable_on` already existed and is the one predicate over this axis. Reaching for `RunKind` because it was the enum already in hand at that call site is the defect worth naming: the surface is the axis that carries the meaning, and a dispatch enum that happens to correlate is not a substitute. The two facts now answer `Look::CouldNotLook` where the surface bars them — could-not-look rather than `IsNot`, because the question genuinely was not asked there, and `IsNot` would claim it was asked and answered no. `RunKind` keeps its original job. `run_recorded` and `run_state_record` carry the surface beside their checks: the verb is `Surface::Check`, `record_state` is `Surface::Hook`. Refs: CLOUD-1480 BREAKING CHANGE: `rules::run_recorded` takes two more parameters — the `ModuleChecks` a caller is entitled to re-derive, and the `facts::Surface` it runs on. Both are decisions the function cannot make for itself: the config-fault checks belong to the caller's budget, and the surface is what bars a `Cost::Effect` fact from resolving on the mediated boundary. Hardcoding either inside is what let a compiler spawn from an end-of-turn hook. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 16 ++++++++++++--- crates/batten/src/rules.rs | 42 ++++++++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index e6e3ecd52..eb5fe7658 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -365,9 +365,13 @@ pub fn run(cli: Cli, mode: Mode, out: &mut dyn Write, err: &mut dyn Write) -> Re // not a policy question and no `batten.toml` may answer it. Some(Command::State { command }) => match command { StateCommand::Adopt { store } => store::run_adopt(store.as_deref(), err), - StateCommand::Record => { - run_state_record(&overrides, mode, err, policy::ModuleChecks::Run) - } + StateCommand::Record => run_state_record( + &overrides, + mode, + err, + policy::ModuleChecks::Run, + facts::Surface::Check, + ), StateCommand::Migrate => run_state_migrate(err), StateCommand::Settle { identity, @@ -1100,6 +1104,10 @@ fn run_state_record( // Threaded to `run_recorded` (CLOUD-1480): the verb reports config // faults, the mediated Stop path may not afford to re-derive them. checks: policy::ModuleChecks, + // AND THE SURFACE, for the `Cost::Effect` facts. The verb is a tree verb; + // the mediated recorder is `Surface::Hook`, where a fact classed + // `Surface::Check` may not resolve. + surface: facts::Surface, ) -> Result { let repo = git::repo_root(Path::new("."))?; // **The ref comes from HERE, not from `repo`.** `repo_root` answers with the @@ -1135,6 +1143,7 @@ fn run_state_record( }, Path::new("."), checks, + surface, )?; if !scan.not_evaluated.is_empty() { // Never silent: a rule that did not look must say so, or a clean-looking @@ -10533,6 +10542,7 @@ fn record_state(overrides: &Overrides) { Mode::default(), &mut sink, policy::ModuleChecks::SkipOnHotPath, + facts::Surface::Hook, ); } diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 42021f717..0e6de3cb7 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -6175,7 +6175,7 @@ fn run_static_inner( vocabulary, scope, now, - RunKind::Static, + crate::facts::Surface::Check, ) } @@ -6230,6 +6230,12 @@ pub fn run_recorded( // answer is already known and the budget is per call". Hardcoding `Run` here // made the end-of-turn boundary re-derive every module's smoke query. checks: crate::policy::ModuleChecks, + // AND WHICH SURFACE IT RUNS ON, for the `Cost::Effect` facts (CLOUD-1480). + // The verb is a tree verb and passes `Surface::Check`; the mediated + // recorder passes `Surface::Hook`, which is what bars a fact classed + // `Surface::Check` from resolving there — `admits` is one-directional and + // that direction is the whole point. + surface: crate::facts::Surface, ) -> anyhow::Result { let (evaluable, withheld): (Vec<&Rule>, Vec<&Rule>) = rules .iter() @@ -6246,7 +6252,7 @@ pub fn run_recorded( // The Stop-surface recorder supplies none, and `None` is the honest // answer rather than a clock read this module may not make. None, - RunKind::Static, + surface, )?; for rule in withheld { // `RuleSkipped`, not a variant of its own. The distinction between "the @@ -6325,7 +6331,7 @@ fn run_all_inner( vocabulary, scope, now, - RunKind::All, + crate::facts::Surface::Check, ) } @@ -6353,14 +6359,20 @@ fn run( // The instant the BOUNDARY read, threaded to `minted_facts` rather than // read here — see `RunOptions::now` for why this module may not read one. now: Option, - // WHICH EFFECT SURFACE THIS RUN IS ON, threaded here rather than left with - // the caller (CLOUD-1480). `run_static` refuses a spawning KIND before any - // work, and that read as the whole of §5's read-only promise — but a + // WHICH SURFACE THIS RUN IS ON, threaded here rather than left with the + // caller (CLOUD-1480). `run_static` refuses a spawning KIND before any work, + // and that read as the whole of §5's read-only promise — but a // `Cost::Effect` FACT is not a kind, so `symbols_fact` and `review_fact` - // below were guarded by DECLARATION alone and spawned on the read surface. - // Measured: `batten hook` on a Stop payload exec'd `cargo clippy` and took - // 114.8s against a published 100ms budget. - kind: RunKind, + // below were guarded by DECLARATION alone and spawned on the mediated + // boundary. Measured: `batten hook` on a Stop payload exec'd `cargo clippy`. + // + // `facts::Surface` AND NOT `RunKind`, which is the correction this parameter + // already needed once. `RunKind` is a DISPATCH enum — `check`, `baseline` + // and the mediated recorder all arrive as `Static` — so gating on it + // disabled both facts on the read surface their own `Class` admits, and + // `review_dispatched.rs` went red. The surface is the axis that carries the + // meaning, and `Class::resolvable_on` is the one predicate over it. + surface: crate::facts::Surface, ) -> anyhow::Result { let recorders = vocabulary.recorders; let files = tree_files(root)?; @@ -6444,16 +6456,16 @@ fn run( // measured answer about a crate nobody analysed. `IsNot` is what the // undeclared arm already returns and it is the honest one here too — the // projection emits `null`, and a module reads undefined. - let effects_admitted = matches!(kind, RunKind::All); - let symbols = if effects_admitted { + let effects_admitted = |class: crate::facts::Class| class.resolvable_on(surface); + let symbols = if effects_admitted(crate::facts::Fact::Symbols.class()) { symbols_fact(rules, root) } else { - crate::facts::Look::IsNot + crate::facts::Look::CouldNotLook }; - let review = if effects_admitted { + let review = if effects_admitted(crate::facts::Fact::Review.class()) { review_fact(rules, root) } else { - crate::facts::Look::IsNot + crate::facts::Look::CouldNotLook }; // THE OUT-OF-ROOT FILES (CLOUD-1167), acquired once for the whole run beside // the families above and, like every one of them, ONLY FOR WHAT A ROW From 17dd86d32b1e686f23a33e2789a76008ad0aa452 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 21:24:52 +0000 Subject: [PATCH 04/12] fix(rules): withhold a rule whose declared fact this surface cannot resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Barring the two `Cost::Effect` facts from the mediated surface left `input.tree.symbols` null while `spawn-adapters.rego` — a `policy` row, so still evaluable on the recorder's partition — refuses on exactly that shape (`no_census if not input.tree.symbols.sites`). Every end of turn therefore wrote a spurious `symbol count absent` deny into the state store: a gate reporting on a census nobody took. Trading a slow hook for a hook that records FALSE FINDINGS is the worse half of the trade. Caught by review, not by me, and not by the suite — which is the same gap the two defects before it came through. `unresolved_declared_fact` sits beside the input-precondition skip in `evaluate_rules`, at the call site rather than inside `run_rule`, for that clause's stated reason: the body is not entered because the call is not made. WITHHELD RATHER THAN PASSED. A withheld rule's silence is not evidence of a clean tree, so its findings HOLD; passing it would be CLOUD-251's vacuous pass in the one place a reader would never look. `.claude/rules/policy-modules.md` already states the engine half — "the engine reports `RuleSkipped` for it rather than a clean tree" — and the tree-surface path simply did not do it. `Look::IsNot` is deliberately not withheld: that arm means the question was asked and the answer is no, and a rule is entitled to decide on it. Only `CouldNotLook` is the could-not-ask. IT ALSO REPAIRS A CASE THAT PREDATES THIS BRANCH. `symbols_fact` answers could-not-look wherever the delegated analyser is absent, so every checkout without that toolchain was already taking the same spurious deny — a verdict about the OPERATOR wearing a verdict about the tree. `RunOptions` gains the surface rather than `run` gaining an eighth argument, which is the bag's own documented purpose ("what keeps the runner's arity from growing per narrowing") and what `clippy::too_many_arguments` refused. The gate ships with the test that drives it through the engine: `the_mediated_surface_resolves_no_effect_fact_and_withholds_the_rule`, a PAIR — an arm asserting only that the hook dispatches nothing passes over an engine that dispatches on no surface at all, which is the dead gate this repository keeps finding. The `Check` arm proves the fact still resolves where its class admits it, and both arms assert the withholding. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 4 + crates/batten/src/rules.rs | 195 ++++++++++++++------ crates/batten/tests/it/review_dispatched.rs | 77 ++++++++ 3 files changed, 216 insertions(+), 60 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index eb5fe7658..6655a56d2 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -13500,6 +13500,10 @@ fn run_rules( let opts = rules::RunOptions { checks, scope: &scope, + // `check` and `enforce` are the TREE verbs, which is the surface both + // `Cost::Effect` facts are classed for (CLOUD-1480). The mediated + // recorder is the one caller that passes `Surface::Hook`. + surface: facts::Surface::Check, // The BOUNDARY's clock, read here and handed over, because `rules.rs` // holds the projection and may read none (CLOUD-1170's stated division, // gated by `the_evaluation_path_reads_no_wall_clock`). diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 0e6de3cb7..d737898a2 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -5976,6 +5976,9 @@ pub fn run_static( RunOptions { checks: crate::policy::ModuleChecks::Run, scope: &Scope::Tree, + // The tree verbs, so the surface their `Cost::Effect` facts are + // classed for (CLOUD-1480). + surface: crate::facts::Surface::Check, // No caller supplied one on this entry point, and `None` is the // honest answer rather than a clock read this module may not make. now: None, @@ -6033,6 +6036,17 @@ pub struct RunOptions<'a> { pub checks: crate::policy::ModuleChecks, /// Which files rules are selected against. pub scope: &'a Scope, + /// WHICH SURFACE THIS RUN IS ON, for the `Cost::Effect` facts (CLOUD-1480). + /// + /// A third narrowing, and it rides here for the reason the header gives: + /// grouping is what keeps the runner's arity from growing per narrowing, and + /// adding this one as an eighth positional argument is what + /// `clippy::too_many_arguments` refused. + /// + /// It is `facts::Surface` and never `RunKind`: that one is a DISPATCH enum — + /// `check`, `baseline` and the mediated recorder all arrive as `Static` — so + /// gating a fact on it disables the fact on surfaces its own `Class` admits. + pub surface: crate::facts::Surface, /// The epoch second the BOUNDARY read, for the one freshness comparison a /// tree run makes: `[[rule.minted]]`'s `max_age_days` (CLOUD-1187). /// @@ -6133,7 +6147,9 @@ fn run_static_inner( root: &Path, opts: RunOptions<'_>, ) -> anyhow::Result { - let RunOptions { checks, scope, now } = opts; + // Only `checks` is read here; `scope`, `now` and `surface` travel on to + // `run` inside the bag rather than being unpacked and re-passed. + let RunOptions { checks, .. } = opts; // POLICY BUNDLES ARE LOADED HERE, on the read surface, and that is // CLOUD-833's substantive claim rather than a formality. `run_static` backs // `check` and refuses any kind that `carries_ambient_authority` — a @@ -6167,16 +6183,7 @@ fn run_static_inner( )); } } - run( - rules, - &[], - root, - &bundles, - vocabulary, - scope, - now, - crate::facts::Surface::Check, - ) + run(rules, &[], root, &bundles, vocabulary, opts) } /// Run only the rules that cannot spawn a process, and report the ones that can @@ -6200,14 +6207,26 @@ fn run_static_inner( /// so there the only honest answer is to refuse. Same omission, two surfaces, /// two correct answers. /// -/// Nothing here spawns, and **the argument list is only half of why** -/// (CLOUD-1480). The withheld rules are partitioned out *before* [`run`] sees -/// them, which the previous revision of this paragraph called "a property of -/// the argument list rather than a promise" — true of every spawning KIND and -/// false of the two `Cost::Effect` FACTS, which no partition over -/// [`RuleKind`] can reach. `symbols` spawned `cargo clippy` from this surface -/// for as long as that sentence stood. The surface is passed to [`run`] now and -/// the facts are gated on it, so the property holds on both halves. +/// **Whether anything here spawns is the CALLER's answer, not this function's** +/// (CLOUD-1480), and stating it as the function's was this paragraph's defect +/// twice over. +/// +/// The withheld rules are partitioned out *before* [`run`] sees them, which an +/// earlier revision called "a property of the argument list rather than a +/// promise" — true of every spawning KIND and false of the two `Cost::Effect` +/// FACTS, which no partition over [`RuleKind`] can reach. `symbols` spawned +/// `cargo clippy` from this surface for as long as that sentence stood. +/// +/// The replacement then claimed "the property holds on both halves", which is +/// true only of the mediated caller. `surface` decides it: `Surface::Hook` bars +/// both facts and nothing spawns, while the `state record` VERB passes +/// `Surface::Check` — the surface those facts are classed for — and still +/// resolves `symbols`, spawning the analyser before its store write. That is +/// correct rather than an oversight: a verb a human ran is entitled to the +/// census, and a per-call budget is what the hook has and the verb does not. +/// +/// So the honest sentence is the conditional one. Read `surface` at the call +/// site to know which answer applies; do not read a promise here. /// /// The partition asks [`RuleKind::carries_ambient_authority`] — the same /// question [`run_static`] refuses on, deliberately the identical call rather @@ -6248,11 +6267,16 @@ pub fn run_recorded( root, &bundles, vocabulary, - &Scope::Tree, - // The Stop-surface recorder supplies none, and `None` is the honest - // answer rather than a clock read this module may not make. - None, - surface, + RunOptions { + checks, + scope: &Scope::Tree, + // The caller's surface: `Surface::Check` for the `state record` + // verb, `Surface::Hook` for the mediated recorder (CLOUD-1480). + surface, + // This caller supplies none, and `None` is the honest answer rather + // than a clock read this module may not make. + now: None, + }, )?; for rule in withheld { // `RuleSkipped`, not a variant of its own. The distinction between "the @@ -6293,6 +6317,9 @@ pub fn run_all( RunOptions { checks: crate::policy::ModuleChecks::Run, scope: &Scope::Tree, + // The tree verbs, so the surface their `Cost::Effect` facts are + // classed for (CLOUD-1480). + surface: crate::facts::Surface::Check, // No caller supplied one on this entry point, and `None` is the // honest answer rather than a clock read this module may not make. now: None, @@ -6308,7 +6335,9 @@ fn run_all_inner( root: &Path, opts: RunOptions<'_>, ) -> anyhow::Result { - let RunOptions { checks, scope, now } = opts; + // Only `checks` is read here; `scope`, `now` and `surface` travel on to + // `run` inside the bag rather than being unpacked and re-passed. + let RunOptions { checks, .. } = opts; // Refuse before any work, the shape `run_static` above already uses: the // alternative is running the check side, exiting on its verdict, and having // silently ignored a repair the config declared. A key that parses and does @@ -6323,16 +6352,7 @@ fn run_all_inner( } } let bundles = crate::policy::load(root, rules, vocabulary, checks, None)?; - run( - rules, - provisions, - root, - &bundles, - vocabulary, - scope, - now, - crate::facts::Surface::Check, - ) + run(rules, provisions, root, &bundles, vocabulary, opts) } /// Run every rule in `rules` against the tree rooted at `root`, returning all @@ -6353,27 +6373,23 @@ fn run( // recorders accumulated, so a second declaration on the rule would be a // second home for one answer. vocabulary: crate::policy::Vocabulary<'_>, - // Which files rules are SELECTED against (CLOUD-519). Applied here, once, - // beside the walk it narrows — never re-derived per rule. - scope: &Scope, - // The instant the BOUNDARY read, threaded to `minted_facts` rather than - // read here — see `RunOptions::now` for why this module may not read one. - now: Option, - // WHICH SURFACE THIS RUN IS ON, threaded here rather than left with the - // caller (CLOUD-1480). `run_static` refuses a spawning KIND before any work, - // and that read as the whole of §5's read-only promise — but a - // `Cost::Effect` FACT is not a kind, so `symbols_fact` and `review_fact` - // below were guarded by DECLARATION alone and spawned on the mediated - // boundary. Measured: `batten hook` on a Stop payload exec'd `cargo clippy`. - // - // `facts::Surface` AND NOT `RunKind`, which is the correction this parameter - // already needed once. `RunKind` is a DISPATCH enum — `check`, `baseline` - // and the mediated recorder all arrive as `Static` — so gating on it - // disabled both facts on the read surface their own `Class` admits, and - // `review_dispatched.rs` went red. The surface is the axis that carries the - // meaning, and `Class::resolvable_on` is the one predicate over it. - surface: crate::facts::Surface, + // Which files rules are SELECTED against (CLOUD-519), the boundary's + // instant, and the surface — the three narrowings, as the one bag whose + // whole purpose is that arity does not grow per narrowing. + opts: RunOptions<'_>, ) -> anyhow::Result { + // WHY THE SURFACE IS HERE AT ALL (CLOUD-1480). `run_static` refuses a + // spawning KIND before any work, and that read as the whole of §5's + // read-only promise — but a `Cost::Effect` FACT is not a kind, so + // `symbols_fact` and `review_fact` below were guarded by DECLARATION alone + // and spawned on the mediated boundary. Measured: the mediated verb on a + // Stop payload exec'd `cargo clippy`. + let RunOptions { + checks: _, + scope, + surface, + now, + } = opts; let recorders = vocabulary.recorders; let files = tree_files(root)?; // The narrowed selection, computed once beside the walk. Every acquisition @@ -6451,11 +6467,12 @@ fn run( // the NARROWEST surface it may be resolved on — so resolving it from the // read-effect surface contradicts the class the fact already carries. // - // COULD-NOT-LOOK RATHER THAN A SKIP, for the reason both facts' own headers - // give: an empty census would read as "resolved, found nothing", which is a - // measured answer about a crate nobody analysed. `IsNot` is what the - // undeclared arm already returns and it is the honest one here too — the - // projection emits `null`, and a module reads undefined. + // COULD-NOT-LOOK, AND NOT `IsNot`. The two are different claims and this + // comment endorsed the wrong one for a revision: `IsNot` says the question + // was asked and the answer is no, which about a crate nobody analysed is a + // measured nothing. The surface could not ask, so `CouldNotLook` is the arm. + // Both project `null` today, which is exactly why the distinction has to be + // right in the code rather than in whichever arm happens to render the same. let effects_admitted = |class: crate::facts::Class| class.resolvable_on(surface); let symbols = if effects_admitted(crate::facts::Fact::Symbols.class()) { symbols_fact(rules, root) @@ -6603,6 +6620,31 @@ fn run( Ok(scan) } +/// The declared fact this run could not resolve for `rule`, if any (CLOUD-1480). +/// +/// DECLARED is the whole predicate. A run resolves the `Cost::Effect` facts only +/// when a row asks for one, so a rule that asked for nothing can never be +/// withheld here — the answer is `None` for every row that does not carry the +/// column, which is what keeps this additive. +/// +/// Returns the COLUMN NAME rather than a sentence: it lands in `Scan::unmet` +/// beside the input-precondition's missing path, and both are read as pointers +/// (rule 4). A reader gets "this rule asked for `symbols` and the run had none", +/// which names the remedy without describing the tree. +/// +/// `Look::IsNot` is deliberately NOT withheld. That arm means the question was +/// asked and the answer is no — a real census with no sites — and a rule is +/// entitled to decide on it. Only `CouldNotLook` is the could-not-ask. +fn unresolved_declared_fact(rule: &Rule, inputs: &RunInputs<'_>) -> Option<&'static str> { + if rule.symbols && matches!(inputs.symbols, crate::facts::Look::CouldNotLook) { + return Some("symbols"); + } + if !rule.review.is_empty() && matches!(inputs.review, crate::facts::Look::CouldNotLook) { + return Some("review"); + } + None +} + /// Evaluate every rule into `scan`, each one contained (CLOUD-126) and each one /// gated on its declared inputs first (CLOUD-125). /// @@ -6636,6 +6678,39 @@ fn evaluate_rules( scan.unmet.insert(rule.id.clone(), missing.to_owned()); continue; } + // THE DECLARED FACT-PRECONDITION, beside the input one above and for its + // reason (CLOUD-1480). A rule that DECLARED a fact the run could not + // resolve has not been given what it asked for, and evaluating it anyway + // hands the module a `null` it cannot tell from a measured answer. + // + // WHY THIS IS NOT OPTIONAL, measured on this branch. Barring the two + // `Cost::Effect` facts from the mediated surface left + // `input.tree.symbols` null while `spawn-adapters.rego` — a `policy` row, + // so still evaluable on the recorder's partition — refuses on exactly + // that shape (`no_census if not input.tree.symbols.sites`). Every end of + // turn therefore wrote a spurious `symbol count absent` deny into the + // state store: a gate reporting on a census nobody took. Trading a slow + // hook for a hook that records false findings is the worse half of the + // trade, and it was caught by review rather than by me. + // + // WITHHELD RATHER THAN PASSED, which is the whole point and the direction + // `Scan::not_evaluated` exists to keep honest. A withheld rule's silence + // is not evidence of a clean tree, so its findings HOLD; passing it would + // be CLOUD-251's vacuous pass in the one place a reader would never look. + // `.claude/rules/policy-modules.md` already states the engine half of + // this for a module that cannot look: "the engine reports `RuleSkipped` + // for it rather than a clean tree". + // + // It also repairs a case that predates this branch: `symbols_fact` + // answers could-not-look wherever the analyser is absent, so every + // checkout without the delegated toolchain was taking that same spurious + // deny — a verdict about the OPERATOR wearing a verdict about the tree. + if let Some(fact) = unresolved_declared_fact(rule, inputs) { + scan.not_evaluated + .insert(rule.id.clone(), NotObserved::RuleSkipped); + scan.unmet.insert(rule.id.clone(), fact.to_owned()); + continue; + } // FAIL-CLOSED ISOLATION (CLOUD-126). This loop used to carry a `?`, so // one rule's I/O error propagated out of the whole scan: no findings at // all were emitted and no later rule ran, over a failure in one row. diff --git a/crates/batten/tests/it/review_dispatched.rs b/crates/batten/tests/it/review_dispatched.rs index 39abbbd12..371d0f50c 100644 --- a/crates/batten/tests/it/review_dispatched.rs +++ b/crates/batten/tests/it/review_dispatched.rs @@ -194,6 +194,83 @@ fn a_dispatched_review_reaches_the_predicate_and_is_clean() { assert_eq!(calls(&root), 1, "the miss dispatched exactly once"); } +/// THE SURFACE GATE, and it is the pair rather than either half (CLOUD-1480). +/// +/// `Fact::Review` is `Cost::Effect x Surface::Check`, so the mediated boundary +/// may not resolve it: dispatching an agent from an end-of-turn hook is minutes +/// and tokens inside a 100ms budget. The engine barred it, and nothing asserted +/// the bar — a revert, or a widening of the class, would have gone unnoticed +/// exactly as the original defect did. +/// +/// TWO ARMS, BECAUSE ONE DISCRIMINATES NOTHING. An arm that only asserts the +/// hook surface dispatches nothing passes just as happily over an engine that +/// dispatches on NO surface, which is the dead gate this repository keeps +/// finding. The `Check` arm is what proves the fact still resolves where its +/// class admits it. +/// +/// AND THE WITHHOLDING IS ASSERTED, not just the absence of a dispatch. A rule +/// that declared a fact the surface could not resolve must land in +/// `not_evaluated` rather than be evaluated against a `null`: the module refuses +/// on could-not-look, so evaluating it would write a spurious deny at every end +/// of turn — measured on this branch, and caught by review rather than by the +/// suite. +#[test] +fn the_mediated_surface_resolves_no_effect_fact_and_withholds_the_rule() { + let root = repo("review-dispatched-surface", "body\n", "", 0); + let verdicts = common::verdicts_in(&root); + let vocabulary = batten::policy::Vocabulary { + patterns: &[], + verdicts: &verdicts, + recorders: &[], + }; + let rows = [row(&root, true)]; + + let hook = rules::run_recorded( + &rows, + &[], + vocabulary, + &root, + batten::policy::ModuleChecks::SkipOnHotPath, + batten::facts::Surface::Hook, + ) + .expect("the mediated surface runs"); + assert_eq!( + calls(&root), + 0, + "the hook surface must dispatch no agent: `Fact::Review` is classed \ + `Surface::Check`, and a mediated call cannot afford one" + ); + assert!( + hook.not_evaluated.contains_key(RULE), + "a rule declaring a fact this surface cannot resolve is WITHHELD, not \ + evaluated against a null — otherwise it refuses on could-not-look and \ + writes a spurious deny every turn. not_evaluated: {:?}", + hook.not_evaluated + ); + + let check = rules::run_recorded( + &rows, + &[], + vocabulary, + &root, + batten::policy::ModuleChecks::Run, + batten::facts::Surface::Check, + ) + .expect("the tree surface runs"); + assert_eq!( + calls(&root), + 1, + "the tree surface still dispatches — `Surface::Check` is the surface the \ + fact's own class admits, and barring it everywhere would be the dead \ + gate this pair exists to refuse" + ); + assert!( + !check.not_evaluated.contains_key(RULE), + "the rule evaluates where its fact resolves: {:?}", + check.not_evaluated + ); +} + /// A REVIEW THAT POINTED AT SOMETHING STILL RAN. Refusing here would price /// finding something, and the cheapest way past such a gate is an agent that /// reports nothing. From 922f331b751f1d59abbca9868fd7ecd472d1c53b Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 21:36:23 +0000 Subject: [PATCH 05/12] fix(hook): start the state record instead of awaiting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MEASURED: 107.6s -> 0.31s on a Stop payload, three runs, same tree. `record_state` ran inline, and it is the whole of CLOUD-1480. A state record scans the tree and folds in the transcript detectors — 118.2s as a standalone verb on this repository — so the mediated Stop call took ~110s against a published <=100ms budget. A turn cannot close until the hook returns, so every end of turn stalled for about two minutes. No amount of trimming reaches that. Three earlier commits on this branch each removed real waste and each moved the number by noise, because the work is seconds by nature and the budget is milliseconds. The only reconciliation is that the record stops being AWAITED: it is drain work, so the verdict is written, the process exits, and the scan finishes on its own time. Spawned with null stdio and never waited on, so the child reparents when this process exits. `process_group` rather than `pre_exec(setsid)` for `exec.rs`'s stated reason: the workspace forbids `unsafe` and for this purpose the two are the same call. THE NUDGE LADDER NOW READS A STORE ONE TURN BEHIND, and that is the deliberate cost. It called `record_state` and then read what that wrote; asynchronously it sees the previous turn's record. Tolerable for exactly this consumer: the conditions it reports — unlanded work above all — persist across turns, so a turn that creates one is nudged at the end of the next. A GATE deciding an exit code on a one-turn-stale store would be a much worse trade, which is why this stays local to the advisory path. NOT YET UNDER BUDGET, and the remaining figure is stated rather than rounded down: 306-339ms is still ~3x the published 100ms. What that is made of is not measured yet and is not this commit's claim. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 72 ++++++++++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 11 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 6655a56d2..0a1cb0be0 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -10533,17 +10533,67 @@ const UNLANDED_BYPASS: &str = "BATTEN_UNLANDED_CHECK_BYPASS"; /// this is an EVALUATION, not a decision. A recorder that failed simply leaves /// nothing to read, which is silence — the correct answer for a hook that may /// never be the reason a turn stalls. -fn record_state(overrides: &Overrides) { - let mut sink = std::io::sink(); - // `SkipOnHotPath`, because this is the hot path (CLOUD-1480). The verb below - // is a human asking about their config; this is the end of every turn. - let _ = run_state_record( - overrides, - Mode::default(), - &mut sink, - policy::ModuleChecks::SkipOnHotPath, - facts::Surface::Hook, - ); +/// Start the state record and **do not wait for it** (CLOUD-1480). +/// +/// # The hook must return inside its budget; the record cannot +/// +/// This ran inline, and it is the whole of that row: a state record scans the +/// tree and folds in the transcript detectors, which measured **118.2s** as a +/// standalone verb on this repository. The mediated Stop call therefore took +/// ~110s against a published `<=100ms` budget, and a turn cannot close until the +/// hook returns — so every end of turn stalled for about two minutes. +/// +/// No amount of trimming fixes that. The work is seconds by nature, the budget +/// is milliseconds, and the only reconciliation is that the record stops being +/// AWAITED. It is drain work: the verdict is written, the process exits, and the +/// scan finishes on its own time. +/// +/// # Detached, and what that costs +/// +/// Spawned with null stdio and never waited on, so the child is reparented when +/// this process exits. `process_group` on unix rather than `pre_exec(setsid)` +/// for `exec.rs`'s stated reason — the workspace forbids `unsafe`, and for this +/// purpose the two are the same call. +/// +/// **The nudge ladder below now reads a store one turn behind**, and that is the +/// deliberate cost rather than an oversight. It called this and then read what +/// this wrote; asynchronously, the read sees the PREVIOUS turn's record. That is +/// tolerable for exactly this consumer and would not be for a gate: the +/// conditions it reports — unlanded work above all — persist across turns, so a +/// turn that creates one is nudged at the end of the next. A gate deciding an +/// exit code on a one-turn-stale store would be a different and much worse +/// trade, which is why this indirection stays local to the advisory path. +/// +/// The child re-reads config from the environment, which is where a mediated +/// call's overrides live; no flag is dropped, because the hook is invoked with +/// none. +fn record_state(_overrides: &Overrides) { + let Ok(exe) = std::env::current_exe() else { + // Could-not-look: no binary path, no record. Silent by design — this is + // the advisory path, and a boundary that cannot start its own drain must + // not turn that into a verdict about the turn. + return; + }; + #[expect( + clippy::disallowed_types, + reason = "stays: the drain IS the spawn (CLOUD-1480). The record cannot run inside a 100ms mediated budget, so the boundary starts it and returns; `exec::piped` is the waiting path and is exactly what must not happen here" + )] + let mut builder = std::process::Command::new(exe); + builder + .args(["state", "record"]) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + #[cfg(unix)] + { + use std::os::unix::process::CommandExt as _; + // Its own group, so the harness reaping this hook's group does not take + // the drain with it. + builder.process_group(0); + } + // SPAWNED AND DROPPED. No `wait`, no `status`, no handle kept: waiting is + // the defect this function exists to remove. + drop(builder.spawn()); } /// The `completion.unlanded` verdict for this branch, or nothing (CLOUD-1163). From e04ca6d9cd5c3e254a1a1fc20972e1ddc6e133d0 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 23:02:05 +0000 Subject: [PATCH 06/12] fix(hook): keep the transcript detectors synchronous, detach only the scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings, and the first is the one that mattered. THE SAME-TURN NUDGE IS A CONTRACT AND I BROKE IT. Detaching the whole record made `unlanded_pointer` read the PREVIOUS turn's store, so a fresh claim's first Stop said nothing. `stop_posture.rs` had already decided that — three cases went red, one of them named `the_first_turn_on_a_fresh_claim_still_speaks`. The commit that broke it called the breakage "the deliberate cost" and never ran the suite. A trade asserted rather than measured is the same defect as a cause asserted rather than measured, in prose. `run_state_record` now takes `scan_tree`. The mediated caller passes false and keeps everything else — store open, journal open, transcript detectors — because `completion` is a TRANSCRIPT detector and its finding is what this turn's nudge reads. The drain still scans. The two halves differ in WHEN they land, not in whether. 22/22 green. FAIL-OPEN EXIT FLIP, and this one was worse than the bug it fixed. The withholding clause applied to `check` and `enforce` too, where could-not-look means the ANALYSER is unreachable or the tree does not compile — exactly when `spawn-adapters` is written to refuse. Skipping there let a broken build switch the gate off. Now scoped to `Surface::Hook`, where the fact is barred by the surface rather than missing from the environment. ONE WRITER AT A TIME. A ~118s record spawned at every turn end with no lock, over `findings::record`'s unlocked read/modify/write and journal shards that declare exactly one writer: overlapping drains lose dispositions and interleave a `writeln!` into JSONL `read_shards` drops. `fs4` advisory, taken in the child. A drain that loses the race exits clean; the verb waits, because a human ran it. THE OVERRIDES ARE FORWARDED. `record_state` dropped them and its doc claimed no flag was lost "because the hook is invoked with none" — true of the harness's invocation, not of the function. Under `--config-from` the child re-resolved from the working tree, judging a branch by the policy that branch declares. `RunInputs` carries the surface beside the facts whose resolvability it decides. NOT UNDER BUDGET AND THE NUMBER IS GETTING WORSE, stated rather than buried: 110s -> 211ms fully detached -> 21.7s with the detectors back. The nudge contract and the 100ms ceiling are in genuine tension, because the verdict the nudge reads comes from a transcript read that is O(session). Narrowing that read is CLOUD-1345's, and until it lands this trades a broken contract for a missed budget. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 162 +++++++++++++++++++++++++++++++++---- crates/batten/src/rules.rs | 39 ++++++++- 2 files changed, 183 insertions(+), 18 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 0a1cb0be0..c1661ed8c 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -371,6 +371,9 @@ pub fn run(cli: Cli, mode: Mode, out: &mut dyn Write, err: &mut dyn Write) -> Re err, policy::ModuleChecks::Run, facts::Surface::Check, + // The verb scans: it is the whole point of running it, and the + // caller is a human or the drain, neither on a per-call budget. + true, ), StateCommand::Migrate => run_state_migrate(err), StateCommand::Settle { @@ -1097,6 +1100,18 @@ fn run_defects_add( /// lineage edge and advances **its own** fold position /// ([`session::HOLDER_RECORD`]) — never a drain's, since this verb folds shards /// whether or not anything reached an agent (CLOUD-83). +/// The environment marker `record_state` sets on the drain it spawns. +/// +/// A drain and the verb do the same work and must not do it at the same time, +/// but they lose the race differently: a drain nobody is waiting on exits +/// quietly, where a verb a human invoked would be lying if it printed a clean +/// record it did not write. +const DRAIN_MARKER: &str = "BATTEN_STATE_DRAIN"; + +/// The lock file that serialises store writes, under `$GIT_DIR` beside the +/// store it guards. +const DRAIN_LOCK: &str = "batten-state-record.lock"; + fn run_state_record( overrides: &Overrides, mode: Mode, @@ -1108,8 +1123,43 @@ fn run_state_record( // the mediated recorder is `Surface::Hook`, where a fact classed // `Surface::Check` may not resolve. surface: facts::Surface, + // WHETHER TO SCAN THE TREE, which is this function's whole cost. False on the + // mediated path, where the transcript detectors below are what the turn's own + // nudge reads and the scan is the drain's to finish. + scan_tree: bool, ) -> Result { let repo = git::repo_root(Path::new("."))?; + // ONE WRITER AT A TIME (CLOUD-1480). `findings::record` is an unlocked + // read/modify/write and `journal`'s shards declare exactly one writer, so + // two records in flight lose dispositions and can interleave a `writeln!` + // into JSONL that `read_shards` drops. A record takes ~118s on a large tree, + // which is long enough that consecutive turns overlap by default. + // + // `fs4` advisory, for the reason `.claude/rules/rust.md` gives for every + // other lock here: the kernel releases it when the holder dies, so a drain + // killed with its container leaves the next one a lock it can take rather + // than one nobody can release. + // + // THE TWO CALLERS LOSE THE RACE DIFFERENTLY, which is what the marker is + // for. A drain nobody awaits exits clean — the work is already being done by + // whoever holds the lock. The VERB waits, because a human ran it and a + // record it did not write must not be reported as one. + let lock_path = git::git_dir(&repo)?.join(DRAIN_LOCK); + let lock = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path)?; + if std::env::var_os(DRAIN_MARKER).is_some() { + match fs4::FileExt::try_lock(&lock) { + Ok(()) => {} + // Another record holds it and is doing this work. Clean exit: the + // drain is not the thing anyone reads a verdict from. + Err(_) => return Ok(ExitCode::Success), + } + } else { + fs4::FileExt::lock(&lock)?; + } // **The ref comes from HERE, not from `repo`.** `repo_root` answers with the // MAIN worktree's root — which is exactly what makes every linked worktree // share one store — so asking it for the branch would report the main @@ -1133,18 +1183,36 @@ fn run_state_record( // included (CLOUD-97 never once evaluated in this repository for exactly // that reason). Withholding is honest here because `record` below folds // `not_evaluated` into the store, where a withheld rule's findings HOLD. - let scan = rules::run_recorded( - &config.rules, - &config.provisions, - policy::Vocabulary { - patterns: &config.patterns, - verdicts: &config.verdicts, - recorders: &config.recorders, - }, - Path::new("."), - checks, - surface, - )?; + // THE TREE SCAN IS THE EXPENSIVE HALF AND THE ONLY ONE (CLOUD-1480). Every + // other line of this function is a store open, a journal open and the + // transcript detectors; `run_recorded` is the ~118s. + // + // So the mediated caller skips it and keeps the rest, which is what preserves + // `the_first_turn_on_a_fresh_claim_still_speaks`: `completion` is a + // TRANSCRIPT detector, so the unlanded verdict this turn's nudge reads is + // minted below, in this process, before the hook returns. Detaching the whole + // function broke that contract, and the commit that did it called the breakage + // "the deliberate cost" without running the suite that had already decided + // otherwise — three cases, one of them named for the contract. + // + // The DRAIN still runs the scan, so nothing is lost: the two halves differ in + // when they land, not in whether. + let scan = if scan_tree { + rules::run_recorded( + &config.rules, + &config.provisions, + policy::Vocabulary { + patterns: &config.patterns, + verdicts: &config.verdicts, + recorders: &config.recorders, + }, + Path::new("."), + checks, + surface, + )? + } else { + rules::Scan::default() + }; if !scan.not_evaluated.is_empty() { // Never silent: a rule that did not look must say so, or a clean-looking // record is the false green. The COUNT carries that on the default rung @@ -10564,10 +10632,48 @@ const UNLANDED_BYPASS: &str = "BATTEN_UNLANDED_CHECK_BYPASS"; /// exit code on a one-turn-stale store would be a different and much worse /// trade, which is why this indirection stays local to the advisory path. /// -/// The child re-reads config from the environment, which is where a mediated -/// call's overrides live; no flag is dropped, because the hook is invoked with -/// none. -fn record_state(_overrides: &Overrides) { +/// **The child is handed this call's overrides, and the sentence that stood here +/// claiming otherwise was wrong** (CLOUD-1480). It read "no flag is dropped, +/// because the hook is invoked with none" — true of how the harness invokes the +/// hook, and not a property of this function, which any caller may reach with +/// `--config-from` or `--strictness` set. Under `--config-from` a child that +/// re-resolved from the working tree would judge the branch by the policy the +/// branch itself declares, which is the exact weakening that flag exists to +/// prevent, and it would do so silently. +/// +/// **One drain at a time, by advisory lock in the child.** A record takes ~118s +/// on a large tree, so turns ending inside that window would otherwise overlap, +/// and `findings::record` is an unlocked read/modify/write over a store whose +/// journal shards declare exactly one writer. Two appenders interleave inside a +/// single `writeln!` — O_APPEND is not one syscall for a multi-write format — so +/// the failure is malformed JSONL that `read_shards` silently drops, plus lost +/// dispositions where a settled finding reopens. The lock lives in the CHILD +/// because the parent must not wait to find out whether it won. +fn record_state(overrides: &Overrides) { + // THE TRANSCRIPT DETECTORS RUN HERE, IN THIS PROCESS, BEFORE THE RETURN. + // + // `completion` is one of them, and `unlanded_pointer` reads its finding a few + // lines later to decide this turn's nudge. Detaching the whole record made + // that read see the PREVIOUS turn's store, so a fresh claim's first Stop said + // nothing — which `stop_posture.rs` had already decided against in a case + // named `the_first_turn_on_a_fresh_claim_still_speaks`. + // + // Cheap because `scan_tree` is false: no tree walk, no rule evaluation, no + // `Cost::Effect` fact. What is left is a store open, a journal open and a + // transcript read — the half whose answer this turn actually needs. + // + // Errors are swallowed for the reason the spawn below is silent: this is the + // advisory path, and a boundary that cannot mint its own record must not turn + // that into a verdict about the turn. + let mut sink = std::io::sink(); + let _ = run_state_record( + overrides, + Mode::default(), + &mut sink, + policy::ModuleChecks::SkipOnHotPath, + facts::Surface::Hook, + false, + ); let Ok(exe) = std::env::current_exe() else { // Could-not-look: no binary path, no record. Silent by design — this is // the advisory path, and a boundary that cannot start its own drain must @@ -10584,6 +10690,30 @@ fn record_state(_overrides: &Overrides) { .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()); + // THE OVERRIDES THIS CALL WAS MADE UNDER, forwarded as the flags they came + // from. Reconstructed rather than serialised: the child parses the same clap + // surface, so a flag is the one spelling both ends already agree on. + if let Some(strictness) = overrides.strictness { + // clap's own token for the variant, never a second spelling: the child + // parses this back through the same `ValueEnum`, so the two ends cannot + // disagree about what `strict` means. + if let Some(value) = clap::ValueEnum::to_possible_value(&strictness) { + builder.args(["--strictness", value.get_name()]); + } + } + if overrides.fail_on_warning { + builder.arg("--fail-on-warning"); + } + if let Some(reference) = overrides.config_from.as_deref() { + builder.args(["--config-from", reference]); + } + if let Some(dir) = overrides.config_in.as_deref() { + builder.args(["--config-in", dir]); + } + // The marker the child reads to know it is a drain rather than the verb: a + // drain that loses the race exits quietly, where the verb a human ran must + // wait its turn and do the work. + builder.env(DRAIN_MARKER, "1"); #[cfg(unix)] { use std::os::unix::process::CommandExt as _; diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index d737898a2..69feb9299 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -6589,6 +6589,7 @@ fn run( records: &records, records_blocked: &records_blocked, git: &git, + surface, symbols: &symbols, review: &review, state: state.as_ref(), @@ -6635,7 +6636,27 @@ fn run( /// `Look::IsNot` is deliberately NOT withheld. That arm means the question was /// asked and the answer is no — a real census with no sites — and a rule is /// entitled to decide on it. Only `CouldNotLook` is the could-not-ask. -fn unresolved_declared_fact(rule: &Rule, inputs: &RunInputs<'_>) -> Option<&'static str> { +fn unresolved_declared_fact( + rule: &Rule, + inputs: &RunInputs<'_>, + surface: crate::facts::Surface, +) -> Option<&'static str> { + // THE MEDIATED SURFACE ONLY, and scoping it was the correction (CLOUD-1480). + // + // Unscoped, this withheld on `check` and `enforce` too — and there + // could-not-look means the ANALYSER is unreachable, or the tree does not + // compile, which is precisely when `spawn-adapters`' `symbol count absent` + // verdict is written to refuse. Skipping instead of denying makes breaking + // the build switch the gate off: a fail-open EXIT-CODE FLIP on the + // enforcement surface, which is strictly worse than the spurious deny this + // clause was added to stop. + // + // On the hook the same arm is honest, because there the fact is barred by + // the SURFACE rather than missing from the environment — nobody looked, so + // no verdict about the tree is available to give. + if !matches!(surface, crate::facts::Surface::Hook) { + return None; + } if rule.symbols && matches!(inputs.symbols, crate::facts::Look::CouldNotLook) { return Some("symbols"); } @@ -6705,7 +6726,7 @@ fn evaluate_rules( // answers could-not-look wherever the analyser is absent, so every // checkout without the delegated toolchain was taking that same spurious // deny — a verdict about the OPERATOR wearing a verdict about the tree. - if let Some(fact) = unresolved_declared_fact(rule, inputs) { + if let Some(fact) = unresolved_declared_fact(rule, inputs, inputs.surface) { scan.not_evaluated .insert(rule.id.clone(), NotObserved::RuleSkipped); scan.unmet.insert(rule.id.clone(), fact.to_owned()); @@ -7008,6 +7029,14 @@ struct RunInputs<'a> { records_blocked: &'a BTreeMap, /// The git facts this rule set declared (CLOUD-907). git: &'a crate::git::GitFacts, + /// WHICH SURFACE THIS RUN IS ON (CLOUD-1480), beside the two facts whose + /// resolvability it decides. + /// + /// Here rather than as a parameter to `evaluate_rules`, because the question + /// it answers — may this fact be resolved at all — is about the same values + /// this bag already carries, and a run-wide input passed separately is the + /// one that drifts out of step with them. + surface: crate::facts::Surface, /// The symbol census, iff this rule set declared it (CLOUD-760). symbols: &'a crate::facts::Look, review: &'a crate::facts::Look>, @@ -13374,6 +13403,12 @@ mod tests { provisions: &[], files, scoped: files, + // The TREE surface, which is what these unit cases exercise: the + // mediated arm is driven through the compiled binary in + // `review_dispatched.rs`, because a `with input as`-shaped + // fabrication here could not tell a barred fact from an absent + // one. + surface: crate::facts::Surface::Check, derived: &self.derived, documents: &self.documents, external: &self.external, From 4fbba77ca2516c6813469d166a9436db6d98b7b4 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 5 Sep 2026 23:20:51 +0000 Subject: [PATCH 07/12] fix(hook): never let the mediated path wait on the drain's lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `512dd1c5` added the single-flight lock and keyed the blocking branch on the DRAIN MARKER alone. The synchronous in-process call sets no marker, so it took the blocking branch and waited for the drain the PREVIOUS turn spawned — the hook serialised behind the very ~118s scan that detaching it was meant to escape. Measured 99-124s, worse than before any of this. The predicate is the surface, which already means "there is a per-call budget here": `Surface::Hook` must never wait. Neither must a drain, since nobody reads its verdict. Only the VERB blocks, because a human ran it and a record it did not write must not be reported as one. MEASURED, drain confirmed running at both ends of the contended arm: idle 218ms contended 220ms Identical, which is the property that was missing: the hook's cost no longer depends on whether a scan is in flight. AND THE MEASUREMENT THAT MISSED IT IS THE LESSON. `512dd1c5` was pushed on a 21.7s reading taken while no drain happened to be running — luck, not a measurement. The contended case is the NORMAL one, because consecutive turns end inside a 118s window, so testing only the quiet case is how a lock bug reads as green. The arm now asserts its own premise with `pgrep` before and after. Still 2x the published 100ms: 110s -> 220ms, and the remaining gap is the config parsed twice plus eight repository discoveries per call, both measured in the parent alone and neither addressed here. Refs: CLOUD-1480 --- crates/batten/src/lib.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index c1661ed8c..fd85b3e44 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -1150,15 +1150,29 @@ fn run_state_record( .truncate(false) .write(true) .open(&lock_path)?; - if std::env::var_os(DRAIN_MARKER).is_some() { + // WHO MAY BLOCK ON THIS LOCK, and getting it wrong cost 100s. The first + // version keyed only on the drain marker, so the SYNCHRONOUS in-process call + // took the blocking branch and waited for the drain the previous turn had + // spawned — the hook serialised behind the very ~118s scan detaching it was + // meant to escape. Measured at 99-124s, worse than before the fix. + // + // The surface is the predicate, because it is the one that already means + // "there is a per-call budget here": `Surface::Hook` must never wait, and the + // drain must not either, since nobody reads its verdict. Only the VERB blocks, + // because a human ran it and a record it did not write must not be reported + // as one. + let may_block = + !matches!(surface, facts::Surface::Hook) && std::env::var_os(DRAIN_MARKER).is_none(); + if may_block { + fs4::FileExt::lock(&lock)?; + } else { match fs4::FileExt::try_lock(&lock) { Ok(()) => {} - // Another record holds it and is doing this work. Clean exit: the - // drain is not the thing anyone reads a verdict from. + // Someone else is already doing this work. Clean exit rather than a + // wait: on the mediated path the budget forbids it, and for a drain + // there is nothing to report. Err(_) => return Ok(ExitCode::Success), } - } else { - fs4::FileExt::lock(&lock)?; } // **The ref comes from HERE, not from `repo`.** `repo_root` answers with the // MAIN worktree's root — which is exactly what makes every linked worktree From bed7187bf2a9546cc73f3b7e3576730e01ccec45 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 05:18:00 +0000 Subject: [PATCH 08/12] fix(hook): hold rule findings the skipped scan never looked at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fail-open regressions from detaching the state record, both found by `/code-review` on #887 and neither visible to the suite. The skipped scan handed `findings::record` a `Scan::default()` — an empty `findings` map AND an empty `not_evaluated` map. `record` reads an empty `not_evaluated` as "every rule ran and saw nothing", so its zero-observation pass resolved every rule-produced finding on the context, at every end of turn, on the one path that runs at every end of turn. That is CLOUD-81's fail-open by a new route: not a rule that was skipped, but a surface that declined to look and said nothing about having declined. Sequence findings escaped only because of the `FindingKind::Sequence` guard. Every configured rule is now `NotObserved::RuleSkipped` on that arm, so the pass holds and the drain re-mints the real observations. `RuleSkipped` rather than a new variant: `NotObserved` is persisted inside records, so a variant would be a store-format change under CLOUD-78's write-old rule. The lock was also held across the whole function, including the ~118s tree scan — a pure read. The contended window was therefore longer than a turn, so the mediated path lost `try_lock` on most turns and returned before minting, silencing the nudge `the_first_turn_on_a_fresh_claim_still_speaks` pins. The lock now covers only the write phase, and the contended branch reports `persisted:false` rather than exiting silently. Two readers racing settle nothing; only the writes need one writer. That narrows the contract gap rather than closing it — a Stop landing inside the drain's write phase still mints nothing. CLOUD-1541 carries the three candidate fixes and the measurement they need. The new case seeds a rule finding with the verb, runs the mediated Stop, and asserts THAT rule's instance on THAT ref is `NotObserved`. Shown able to fail: reverting the arm to `Scan::default()` turns it red. The bare spellings were both wrong — the listing carries sequence findings whose `Observed(0)` is honest, so scanning the whole document passes on any store holding anything and fails on one that is correct. `run_all_over`'s call site gains `Surface::Check`: main added it while this branch was adding the field, so the rebase was textually clean and did not compile. Refs: CLOUD-1480, CLOUD-1541 --- crates/batten/src/lib.rs | 84 ++++++++++----- crates/batten/tests/it/stop_posture.rs | 139 +++++++++++++++++++++++++ 2 files changed, 198 insertions(+), 25 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index fd85b3e44..7c3b4cfe3 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -1150,30 +1150,6 @@ fn run_state_record( .truncate(false) .write(true) .open(&lock_path)?; - // WHO MAY BLOCK ON THIS LOCK, and getting it wrong cost 100s. The first - // version keyed only on the drain marker, so the SYNCHRONOUS in-process call - // took the blocking branch and waited for the drain the previous turn had - // spawned — the hook serialised behind the very ~118s scan detaching it was - // meant to escape. Measured at 99-124s, worse than before the fix. - // - // The surface is the predicate, because it is the one that already means - // "there is a per-call budget here": `Surface::Hook` must never wait, and the - // drain must not either, since nobody reads its verdict. Only the VERB blocks, - // because a human ran it and a record it did not write must not be reported - // as one. - let may_block = - !matches!(surface, facts::Surface::Hook) && std::env::var_os(DRAIN_MARKER).is_none(); - if may_block { - fs4::FileExt::lock(&lock)?; - } else { - match fs4::FileExt::try_lock(&lock) { - Ok(()) => {} - // Someone else is already doing this work. Clean exit rather than a - // wait: on the mediated path the budget forbids it, and for a drain - // there is nothing to report. - Err(_) => return Ok(ExitCode::Success), - } - } // **The ref comes from HERE, not from `repo`.** `repo_root` answers with the // MAIN worktree's root — which is exactly what makes every linked worktree // share one store — so asking it for the branch would report the main @@ -1225,7 +1201,25 @@ fn run_state_record( surface, )? } else { - rules::Scan::default() + // NOT `Scan::default()`. An empty `not_evaluated` is read one call down + // as "every rule ran and saw nothing", so the zero-observation pass in + // `findings::record` resolves every rule-produced finding on this + // context — at every end of turn, on the one path that runs at every end + // of turn. That is CLOUD-81's fail-open exactly, reintroduced by + // deferring the scan rather than by skipping a rule. + // + // The honest answer is that this surface did not look, so every + // configured rule is `NotObserved` and the pass HOLDS. The drain re-mints + // the real observations when it finishes; until then a finding stays + // held rather than being resolved by a scan that never ran. + rules::Scan { + not_evaluated: config + .rules + .iter() + .map(|rule| (rule.id.clone(), findings::NotObserved::RuleSkipped)) + .collect(), + ..rules::Scan::default() + } }; if !scan.not_evaluated.is_empty() { // Never silent: a rule that did not look must say so, or a clean-looking @@ -1253,6 +1247,42 @@ fn run_state_record( )?; } + // THE LOCK IS TAKEN HERE, NOT ABOVE THE SCAN, and the scope is the whole + // point. Everything between the top of this function and this line is a + // READ — git refs, config resolution, the rule scan — and the scan is what + // takes ~118s. Holding the lock across it made the contended window longer + // than a turn, so the mediated path lost `try_lock` on most turns and + // returned before minting anything, silencing the nudge that + // `stop_posture::the_first_turn_on_a_fresh_claim_still_speaks` pins. Two + // readers racing settle nothing; only the writes below need one writer, and + // scoping the lock to them is what makes losing it rare rather than usual. + // + // WHO MAY BLOCK, and getting it wrong cost 100s. The first version keyed + // only on the drain marker, so the SYNCHRONOUS in-process call took the + // blocking branch and waited for the drain the previous turn had spawned — + // the hook serialised behind the very scan detaching it was meant to escape. + // Measured at 99-124s, worse than before the fix. The surface is the + // predicate, because it is the one that already means "there is a per-call + // budget here": `Surface::Hook` must never wait, and the drain must not + // either, since nobody reads its verdict. Only the VERB blocks, because a + // human ran it and a record it did not write must not be reported as one. + let may_block = + !matches!(surface, facts::Surface::Hook) && std::env::var_os(DRAIN_MARKER).is_none(); + if may_block { + fs4::FileExt::lock(&lock)?; + } else if fs4::FileExt::try_lock(&lock).is_err() { + // Someone else holds the write phase. REPORTED, never silent: this turn + // mints nothing, so the nudge ladder reads a store this call did not + // advance, and a reader owed an explanation for the silence gets one — + // the same `persisted:false` reading the degraded-store arm below gives + // for the other reason a record does not happen. + writeln!( + err, + "batten: state record {context}: another writer holds the store; persisted:false" + )?; + return Ok(ExitCode::Success); + } + let bound = store::commit(store::resolve(&repo)?)?; if let Some(note) = &bound.note { writeln!(err, "batten: {note}")?; @@ -5511,6 +5541,10 @@ fn admission_anchor( rules::RunOptions { checks: policy::ModuleChecks::RunOverSelection, scope: &rules::Scope::Tree, + // A TREE verb, so the read surface: this re-runs one declared rule + // over one subject to recover its fingerprint, which is the same + // work `check` does and not the mediated boundary's. + surface: facts::Surface::Check, now: None, }, ) else { diff --git a/crates/batten/tests/it/stop_posture.rs b/crates/batten/tests/it/stop_posture.rs index 40f2f17b0..3fe0c1068 100644 --- a/crates/batten/tests/it/stop_posture.rs +++ b/crates/batten/tests/it/stop_posture.rs @@ -974,3 +974,142 @@ fn the_stop_surface_never_exits_non_zero() { "an advisory never changes the exit code" ); } + +/// The module the fixture below registers: it denies over every document it is +/// given, so one `state record` mints a rule finding and the pair after it has +/// something to hold. +const HOLDS_MODULE: &str = r#"package batten + +rules contains "always-denies" + +violation contains { + "rule": "always-denies", + "verdict": "fixture always denies", + "subjects": [{"path": path}], +} if { + some path, _ in input.tree.documents +} + +deny contains entry if { + some entry in violation +} +"#; + +/// A TREE-scoped rule, which is the whole point: the mediated Stop path skips +/// the scan, so this rule is exactly what it did not look at. +const HOLDS_CONFIG: &str = r#" +[[rule]] +id = "always-denies" +kind = "policy" +scope = "tree" +module = "policy/holds.rego" +severity = "deny" +# WITHOUT THIS THE MODULE DECIDES NOTHING. `input.tree.documents` is built from +# the row's declared documents, so a tree rule that declares none iterates an +# empty map, loads clean and mints no finding — the dead-gate class +# `.claude/rules/policy-modules.md` opens with, met here on the first attempt. +documents = ["batten.toml"] + +[[verdict]] +id = "fixture always denies" +gloss = "a fixture class, raised over every document" +class = "Raised by the fixture module so a rule finding exists to hold." + +[[verdict.route]] +id = "nothing to do" +kind = "issue" +target = "a fixture route, never followed" +"#; + +/// The completion fixture plus a tree-scoped rule that always fires. +fn holds_fixture(name: &str) -> (PathBuf, PathBuf) { + let (repo, home) = unlanded_fixture(name); + let config = fs::read_to_string(repo.join("batten.toml")).expect("read fixture config"); + fs::write(repo.join("batten.toml"), format!("{config}{HOLDS_CONFIG}")).expect("extend config"); + fs::write(repo.join("policy/holds.rego"), HOLDS_MODULE).expect("write fixture module"); + common::git_in(&repo, &["add", "-A"]); + common::git_in(&repo, &["commit", "-q", "-m", "chore: fixture rule"]); + (repo, home) +} + +/// Run a verb against the fixture's own store. +fn verb_in(dir: &Path, home: &Path, args: &[&str]) -> Output { + let mut command = batten(); + common::state_home(&mut command, home); + command + .current_dir(dir) + .args(args) + .env("GIT_CEILING_DIRECTORIES", env!("CARGO_TARGET_TMPDIR")) + .env_remove("BATTEN_HOOK_BYPASS") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + command.output().expect("run batten verb") +} + +/// A SKIPPED SCAN HOLDS A RULE FINDING — IT DOES NOT RESOLVE IT (CLOUD-1480). +/// +/// # The regression this exists because of +/// +/// Detaching the state record left the mediated path calling `findings::record` +/// with `rules::Scan::default()` — an empty `findings` map AND an empty +/// `not_evaluated` map. `findings::record` reads an empty `not_evaluated` as +/// "every rule ran and saw nothing", so its zero-observation pass resolved every +/// rule-produced finding on the context, at every end of turn, on the one path +/// that runs at every end of turn. +/// +/// That is CLOUD-81's fail-open reached by a new route: not a rule that was +/// skipped, but a whole surface that declined to look and said nothing about +/// having declined. Sequence findings escaped only because of the +/// `FindingKind::Sequence` guard; every rule finding did not. +/// +/// # Why the assertion is on the OBSERVATION and not on the listing's length +/// +/// The record is never deleted either way — `state list` shows it in both +/// worlds. What differs is the instance: `Observed(0)` is "the rule looked and +/// it is gone", `NotObserved` is "nothing looked". Asserting presence would pass +/// over the bug; only the observation discriminates. +#[test] +fn a_stop_that_skipped_the_scan_holds_the_rule_finding_rather_than_resolving_it() { + let (repo, home) = holds_fixture("stop-holds-rule-finding"); + + // The VERB scans the tree and mints the finding. + let recorded = verb_in(&repo, &home, &["state", "record"]); + assert!( + recorded.status.success(), + "the verb records: {}", + String::from_utf8_lossy(&recorded.stderr) + ); + let seeded = stdout_of(&verb_in(&repo, &home, &["state", "list", "-J"])); + assert!( + seeded.contains("always-denies"), + "the seeding scan minted the fixture rule's finding: {seeded}" + ); + + // The mediated Stop path, which skips the scan. + let _ = hook_in(&repo, &home, &stop_payload("Landed and pushed.", false)); + + // NAMED, not scanned. The listing carries other records — the sequence + // findings the detectors mint, whose `Observed(0)` is honest — so a bare + // `contains("NotObserved")` would pass on any store holding anything, and a + // bare `!contains("Observed": 0)` would fail on a store that is correct. + // Only THIS rule's instance on THIS ref discriminates. + let listing = stdout_of(&verb_in(&repo, &home, &["state", "list", "-J"])); + let records: serde_json::Value = serde_json::from_str(&listing).expect("the listing is JSON"); + let occurrences = records + .as_array() + .expect("a listing is an array") + .iter() + .find(|record| record["rule"] == "always-denies") + .expect("the fixture rule's record survived the mediated turn")["instances"] + .as_array() + .expect("instances is an array") + .iter() + .find(|instance| instance["context"] == "refs/heads/work") + .expect("an instance on the branch the scan ran on")["occurrences"] + .clone(); + assert!( + occurrences.get("NotObserved").is_some(), + "a surface that did not look HOLDS the finding rather than resolving it, \ + which is the whole of CLOUD-81 on this path: {occurrences}" + ); +} From 8acb68ce0f22de3c4c9914907be498164f27462b Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 07:50:48 +0000 Subject: [PATCH 09/12] refactor(exec): place the drain's spawn at the child-process boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spawn-adapters` refused the drain `record_state` spawns, and it was right to: the site was in `lib.rs`, which the adapter table does not place. The table's own comment says why it will not be placed — admitting the CLI dispatch would admit every future spawn in the crate's largest file at once, and the table would stop naming boundaries and start naming files. So the spawn moves rather than the table. `exec::detached` is `piped`'s opposite number and the pair is this module's whole contract: `piped` runs a child for its ANSWER, `detached` runs one because the work must outlive the caller. A mediated boundary has a per-call budget the record cannot fit in, so it starts the child and returns; there is nothing to wait on, which is why nothing is returned. `lib.rs` composes the argv — which flag means what is its business — and owns no `Command`. Three clippy refusals fell out of the same review and are fixed here rather than left for CI. `run_state_record` was 123/100 lines, so the lock acquisition, the withheld-rules report and the ref-death GC each become the named function their rationale already described: `take_write_lock`, `report_withheld`, `collect_dead_refs`. `rules::run` was over for the same reason and gains `effect_facts`. One doc comment was missing backticks. No behaviour changes. `stop_posture` 23/23, including the regression case for the skipped scan's held findings. Refs: CLOUD-1480 --- crates/batten/src/exec.rs | 46 ++++++++ crates/batten/src/lib.rs | 220 +++++++++++++++++++++---------------- crates/batten/src/rules.rs | 66 +++++++---- 3 files changed, 215 insertions(+), 117 deletions(-) diff --git a/crates/batten/src/exec.rs b/crates/batten/src/exec.rs index 37e57cb4c..19d5690e2 100644 --- a/crates/batten/src/exec.rs +++ b/crates/batten/src/exec.rs @@ -1452,6 +1452,52 @@ pub(crate) fn piped( )) } +/// Start `program` with `args` and **do not wait** (CLOUD-1480). +/// +/// `piped`'s opposite number, and the pair is the whole of this module's +/// contract: `piped` runs a child for its ANSWER, this one runs a child because +/// the work must outlive the caller. A mediated boundary has a per-call budget +/// the work cannot fit in, so it starts the child and returns; waiting is the +/// defect the caller exists to remove, which is why nothing here is returned to +/// wait on. +/// +/// Placed HERE rather than at the caller for `spawn-adapters`' reason: `lib.rs` +/// is not on that table and the table's own comment refuses to put it there, +/// because placing the CLI dispatch would admit every future spawn in the +/// crate's largest file at once. The caller composes the argv — which flags mean +/// what is its business — and this module owns the process. +/// +/// `env` is applied after the inherited environment, so a caller marks the child +/// without reaching for a second mechanism. +/// +/// Silent: no `Result`, because there is no caller that could act on the +/// difference. A boundary that cannot start its own background work must not +/// turn that into a verdict about the call it was mediating. +#[expect( + clippy::disallowed_types, + reason = "stays: the detached child IS the point (CLOUD-1480). `piped` above is the waiting path and is exactly what this must not be; both spawns are the placed adapter's" +)] +pub(crate) fn detached(program: &Path, args: &[String], env: &[(&str, &str)]) { + let mut builder = Command::new(program.as_os_str()); + builder + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + for (name, value) in env { + builder.env(name, value); + } + #[cfg(unix)] + { + use std::os::unix::process::CommandExt as _; + // Its own group, so a harness reaping the caller's group does not take + // this child with it. + builder.process_group(0); + } + // SPAWNED AND DROPPED. No `wait`, no `status`, no handle kept. + drop(builder.spawn()); +} + /// This process's next dispatch number, for the live-capture key. /// /// The key has to name a *run*, not just a command: through the CLI there is diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 7c3b4cfe3..dde4e0efe 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -1112,6 +1112,104 @@ const DRAIN_MARKER: &str = "BATTEN_STATE_DRAIN"; /// store it guards. const DRAIN_LOCK: &str = "batten-state-record.lock"; +/// Take the store's write lock, answering whether this caller may write. +/// +/// # Who may block, and getting it wrong cost 100s +/// +/// The first version keyed only on [`DRAIN_MARKER`], so the SYNCHRONOUS +/// in-process call took the blocking branch and waited for the drain the +/// previous turn had spawned — the hook serialised behind the very scan +/// detaching it was meant to escape. Measured at 99–124s, worse than before the +/// fix. +/// +/// The SURFACE is the predicate, because it is the one that already means +/// "there is a per-call budget here": [`facts::Surface::Hook`] must never wait, +/// and a drain must not either, since nobody reads its verdict. Only the VERB +/// blocks, because a human ran it and a record it did not write must not be +/// reported as one. +/// +/// # And the caller takes it AFTER its reads, not before +/// +/// Held across the tree scan — a pure read, and the ~118s one — the contended +/// window is longer than a turn, so the mediated path loses `try_lock` on most +/// turns and mints nothing, silencing the nudge +/// `stop_posture::the_first_turn_on_a_fresh_claim_still_speaks` pins. Two +/// readers racing settle nothing; only the writes need one writer, and scoping +/// the lock to them is what makes losing it rare rather than usual. +/// +/// # Errors +/// +/// Propagates a blocking `lock` that fails outright. A contended `try_lock` is +/// `Ok(false)`, never an error: losing a race is an answer. +fn take_write_lock(lock: &std::fs::File, surface: facts::Surface) -> Result { + let may_block = + !matches!(surface, facts::Surface::Hook) && std::env::var_os(DRAIN_MARKER).is_none(); + if may_block { + fs4::FileExt::lock(lock)?; + return Ok(true); + } + Ok(fs4::FileExt::try_lock(lock).is_ok()) +} + +/// Say which rules did not look, so a clean-looking record is not a false green. +/// +/// Never silent: a rule that did not evaluate must say so. The COUNT carries +/// that on the default rung and the ids ride [`Verbosity::Verbose`] — this fires +/// at every turn end, and sixteen rule ids on every one is how a line stops +/// being read (`stop-guard`'s own lesson about spending a channel). Ids are the +/// config author's own tokens rather than content, so the higher rung is a noise +/// decision, not a non-negotiable-rule-4 one. +/// +/// # Errors +/// +/// Propagates a failure to write to `err`. +fn report_withheld(scan: &rules::Scan, mode: Mode, err: &mut dyn Write) -> Result<()> { + if scan.not_evaluated.is_empty() { + return Ok(()); + } + let withheld: Vec<&str> = scan.not_evaluated.keys().map(String::as_str).collect(); + output::message( + mode, + Verbosity::Normal, + err, + &format!( + "state record: {} rule(s) not evaluated, their findings held", + withheld.len() + ), + )?; + output::message( + mode, + Verbosity::Verbose, + err, + &format!("state record: not evaluated: {}", withheld.join(", ")), + )?; + Ok(()) +} + +/// Drop the instances of refs that no longer exist, and say how many. +/// +/// Ref-death GC rides the record verb: the live set is what exists NOW, so a +/// branch deleted since the last record loses its instances here. +/// +/// When anything was dropped this also starts a new generation — GC's half of +/// the cursor handshake, so every outstanding drain cursor resyncs instead of +/// computing a delta against records that are gone. +/// +/// # Errors +/// +/// Propagates a failure to read the repository's refs or to write the store. +fn collect_dead_refs(repo: &Path, store_dir: &Path) -> Result { + let live = git::refs(repo)? + .into_iter() + .map(findings::Context::new) + .collect(); + let dropped = findings::gc(store_dir, &live)?; + if dropped > 0 { + journal::new_generation(store_dir)?; + } + Ok(dropped) +} + fn run_state_record( overrides: &Overrides, mode: Mode, @@ -1221,61 +1319,16 @@ fn run_state_record( ..rules::Scan::default() } }; - if !scan.not_evaluated.is_empty() { - // Never silent: a rule that did not look must say so, or a clean-looking - // record is the false green. The COUNT carries that on the default rung - // and the ids ride `Verbose` — this fires at every turn end, and sixteen - // rule ids on every one is how a line stops being read (`stop-guard`'s - // own lesson about spending a channel). Ids are the config author's own - // tokens rather than content, so the higher rung is a noise decision, - // not a rule-4 one. - let withheld: Vec<&str> = scan.not_evaluated.keys().map(String::as_str).collect(); - output::message( - mode, - Verbosity::Normal, - err, - &format!( - "state record: {} rule(s) not evaluated, their findings held", - withheld.len() - ), - )?; - output::message( - mode, - Verbosity::Verbose, - err, - &format!("state record: not evaluated: {}", withheld.join(", ")), - )?; - } + report_withheld(&scan, mode, err)?; - // THE LOCK IS TAKEN HERE, NOT ABOVE THE SCAN, and the scope is the whole - // point. Everything between the top of this function and this line is a - // READ — git refs, config resolution, the rule scan — and the scan is what - // takes ~118s. Holding the lock across it made the contended window longer - // than a turn, so the mediated path lost `try_lock` on most turns and - // returned before minting anything, silencing the nudge that - // `stop_posture::the_first_turn_on_a_fresh_claim_still_speaks` pins. Two - // readers racing settle nothing; only the writes below need one writer, and - // scoping the lock to them is what makes losing it rare rather than usual. - // - // WHO MAY BLOCK, and getting it wrong cost 100s. The first version keyed - // only on the drain marker, so the SYNCHRONOUS in-process call took the - // blocking branch and waited for the drain the previous turn had spawned — - // the hook serialised behind the very scan detaching it was meant to escape. - // Measured at 99-124s, worse than before the fix. The surface is the - // predicate, because it is the one that already means "there is a per-call - // budget here": `Surface::Hook` must never wait, and the drain must not - // either, since nobody reads its verdict. Only the VERB blocks, because a - // human ran it and a record it did not write must not be reported as one. - let may_block = - !matches!(surface, facts::Surface::Hook) && std::env::var_os(DRAIN_MARKER).is_none(); - if may_block { - fs4::FileExt::lock(&lock)?; - } else if fs4::FileExt::try_lock(&lock).is_err() { - // Someone else holds the write phase. REPORTED, never silent: this turn - // mints nothing, so the nudge ladder reads a store this call did not - // advance, and a reader owed an explanation for the silence gets one — - // the same `persisted:false` reading the degraded-store arm below gives - // for the other reason a record does not happen. + // THE WRITE PHASE STARTS HERE, and so does the lock. Everything above is a + // READ, so nothing above needs one writer. + if !take_write_lock(&lock, surface)? { + // Someone else holds it. REPORTED, never silent: this turn mints + // nothing, so the nudge ladder reads a store this call did not advance, + // and a reader owed an explanation for the silence gets one — the same + // `persisted:false` reading the degraded-store arm below gives for the + // other reason a record does not happen. writeln!( err, "batten: state record {context}: another writer holds the store; persisted:false" @@ -1345,19 +1398,7 @@ fn run_state_record( )?; } - // Ref-death GC rides the same verb: the live set is what exists now, so a - // branch deleted since the last record loses its instances here. - let live = git::refs(&repo)? - .into_iter() - .map(findings::Context::new) - .collect(); - let dropped = findings::gc(&bound.dir, &live)?; - if dropped > 0 { - // GC's half of the cursor handshake: a new generation, so every - // outstanding drain cursor resyncs instead of computing a delta against - // records that are gone. - journal::new_generation(&bound.dir)?; - } + let dropped = collect_dead_refs(&repo, &bound.dir)?; // The session's durable resume point (CLOUD-83), recorded LAST so the stored // cursor names the generation this run finished in — a GC above may have @@ -10693,7 +10734,7 @@ const UNLANDED_BYPASS: &str = "BATTEN_UNLANDED_CHECK_BYPASS"; /// on a large tree, so turns ending inside that window would otherwise overlap, /// and `findings::record` is an unlocked read/modify/write over a store whose /// journal shards declare exactly one writer. Two appenders interleave inside a -/// single `writeln!` — O_APPEND is not one syscall for a multi-write format — so +/// single `writeln!` — `O_APPEND` is not one syscall for a multi-write format — so /// the failure is malformed JSONL that `read_shards` silently drops, plus lost /// dispositions where a settled finding reopens. The lock lives in the CHILD /// because the parent must not wait to find out whether it won. @@ -10728,50 +10769,41 @@ fn record_state(overrides: &Overrides) { // not turn that into a verdict about the turn. return; }; - #[expect( - clippy::disallowed_types, - reason = "stays: the drain IS the spawn (CLOUD-1480). The record cannot run inside a 100ms mediated budget, so the boundary starts it and returns; `exec::piped` is the waiting path and is exactly what must not happen here" - )] - let mut builder = std::process::Command::new(exe); - builder - .args(["state", "record"]) - .stdin(std::process::Stdio::null()) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()); // THE OVERRIDES THIS CALL WAS MADE UNDER, forwarded as the flags they came // from. Reconstructed rather than serialised: the child parses the same clap // surface, so a flag is the one spelling both ends already agree on. + let mut args = vec!["state".to_owned(), "record".to_owned()]; if let Some(strictness) = overrides.strictness { // clap's own token for the variant, never a second spelling: the child // parses this back through the same `ValueEnum`, so the two ends cannot // disagree about what `strict` means. if let Some(value) = clap::ValueEnum::to_possible_value(&strictness) { - builder.args(["--strictness", value.get_name()]); + args.push("--strictness".to_owned()); + args.push(value.get_name().to_owned()); } } if overrides.fail_on_warning { - builder.arg("--fail-on-warning"); + args.push("--fail-on-warning".to_owned()); } if let Some(reference) = overrides.config_from.as_deref() { - builder.args(["--config-from", reference]); + args.push("--config-from".to_owned()); + args.push(reference.to_owned()); } if let Some(dir) = overrides.config_in.as_deref() { - builder.args(["--config-in", dir]); - } + args.push("--config-in".to_owned()); + args.push(dir.to_owned()); + } + // THE SPAWN IS `exec`'s, NOT THIS FILE'S. `spawn-adapters` places the + // child-process boundary in `exec` and its table's own comment refuses to + // place `lib`, because admitting the CLI dispatch would admit every future + // spawn in the crate's largest file at once. This composes the argv — which + // flag means what is this module's business — and `exec::detached` owns the + // process, including the group it is put in. + // // The marker the child reads to know it is a drain rather than the verb: a // drain that loses the race exits quietly, where the verb a human ran must // wait its turn and do the work. - builder.env(DRAIN_MARKER, "1"); - #[cfg(unix)] - { - use std::os::unix::process::CommandExt as _; - // Its own group, so the harness reaping this hook's group does not take - // the drain with it. - builder.process_group(0); - } - // SPAWNED AND DROPPED. No `wait`, no `status`, no handle kept: waiting is - // the defect this function exists to remove. - drop(builder.spawn()); + exec::detached(&exe, &args, &[(DRAIN_MARKER, "1")]); } /// The `completion.unlanded` verdict for this branch, or nothing (CLOUD-1163). diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 69feb9299..4bd075bb8 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -6461,29 +6461,7 @@ fn run( // The one acquisition of the `Cost::Effect` fact (CLOUD-760), beside the git // family and for the same reason: a projection must not spawn, so the spend // happens once here and only when a row declared it. - // THE TWO `Cost::Effect` FACTS, AND THE SURFACE IS THE OTHER HALF OF THE - // GUARD (CLOUD-1480). Declaration alone was never enough: `Fact::Symbols` - // is classed `Cost::Effect` x `Surface::Check`, and `Surface::Check` names - // the NARROWEST surface it may be resolved on — so resolving it from the - // read-effect surface contradicts the class the fact already carries. - // - // COULD-NOT-LOOK, AND NOT `IsNot`. The two are different claims and this - // comment endorsed the wrong one for a revision: `IsNot` says the question - // was asked and the answer is no, which about a crate nobody analysed is a - // measured nothing. The surface could not ask, so `CouldNotLook` is the arm. - // Both project `null` today, which is exactly why the distinction has to be - // right in the code rather than in whichever arm happens to render the same. - let effects_admitted = |class: crate::facts::Class| class.resolvable_on(surface); - let symbols = if effects_admitted(crate::facts::Fact::Symbols.class()) { - symbols_fact(rules, root) - } else { - crate::facts::Look::CouldNotLook - }; - let review = if effects_admitted(crate::facts::Fact::Review.class()) { - review_fact(rules, root) - } else { - crate::facts::Look::CouldNotLook - }; + let (symbols, review) = effect_facts(rules, root, surface); // THE OUT-OF-ROOT FILES (CLOUD-1167), acquired once for the whole run beside // the families above and, like every one of them, ONLY FOR WHAT A ROW // DECLARED. A ruleset naming no `[[rule.external]]` reads no environment @@ -8604,6 +8582,48 @@ fn captured_facts(rules: &[Rule], root: &Path) -> Option ( + crate::facts::Look, + crate::facts::Look>, +) { + let admitted = |class: crate::facts::Class| class.resolvable_on(surface); + let symbols = if admitted(crate::facts::Fact::Symbols.class()) { + symbols_fact(rules, root) + } else { + crate::facts::Look::CouldNotLook + }; + let review = if admitted(crate::facts::Fact::Review.class()) { + review_fact(rules, root) + } else { + crate::facts::Look::CouldNotLook + }; + (symbols, review) +} + fn symbols_fact(rules: &[Rule], root: &Path) -> crate::facts::Look { if !rules.iter().any(|rule| rule.symbols) { // Nothing asked, so nothing is spent — and the projection below emits From 5f83fbe8012ed44d12599aad00284b4342dd18e3 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 08:22:49 +0000 Subject: [PATCH 10/12] fix(hook): let the detectors speak when another writer holds the store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `take_write_lock` losing the race returned, and returning was the defect. The nudge ladder reads this store a few lines after `record_state` comes back, so a turn that lost the lock minted nothing — and on a fresh claim, with no earlier record to fall back on, the turn said nothing at all. That is the contract `the_first_turn_on_a_fresh_claim_still_speaks` pins, broken by the same commit that stopped the hook waiting 118s for it. The two halves are separated instead. The SCAN's record keeps the lock, because `findings::record` is an unlocked read/modify/write over every identity. The DETECTORS no longer wait for it: `record_sequence` writes one record file per identity by atomic rename and appends no journal entry, and the drain holding the lock is deriving the same value from the same transcript, so the two agree by construction rather than by exclusion. A caller that lost the lock reports `persisted:false` and says which half did not happen. The suite could not see this: every other case runs with the lock free, and that is the unrepresentative arm to omit, since consecutive turns end inside the drain's window by construction. The new case therefore takes the lock itself rather than racing a spawned drain, whose hold time is neither bounded nor knowable from the test. Shown able to fail: with the early return restored it goes red, and it is the pair with the uncontended case that discriminates rather than either alone. stop_posture 24/24. Refs: CLOUD-1541 --- crates/batten/src/lib.rs | 76 ++++++++++++++++++-------- crates/batten/tests/it/stop_posture.rs | 61 +++++++++++++++++++++ 2 files changed, 114 insertions(+), 23 deletions(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index dde4e0efe..a877ec8b2 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -1323,18 +1323,21 @@ fn run_state_record( // THE WRITE PHASE STARTS HERE, and so does the lock. Everything above is a // READ, so nothing above needs one writer. - if !take_write_lock(&lock, surface)? { - // Someone else holds it. REPORTED, never silent: this turn mints - // nothing, so the nudge ladder reads a store this call did not advance, - // and a reader owed an explanation for the silence gets one — the same - // `persisted:false` reading the degraded-store arm below gives for the - // other reason a record does not happen. - writeln!( - err, - "batten: state record {context}: another writer holds the store; persisted:false" - )?; - return Ok(ExitCode::Success); - } + // + // LOSING IT IS NOT SILENCE, and it is not a return either (CLOUD-1541). It + // was both for one revision, and the cost was the contract + // `stop_posture::the_first_turn_on_a_fresh_claim_still_speaks` pins: the + // nudge ladder reads this store a few lines after this call, so a turn that + // returned here minted nothing and a FRESH claim — with no earlier record to + // fall back on — said nothing at all. + // + // So the two halves are separated. The SCAN's record needs one writer, since + // `findings::record` is an unlocked read/modify/write over every identity. + // The DETECTORS do not: `findings::record_sequence` writes one record file + // per identity by atomic rename and appends no journal entry, and the drain + // holding the lock is deriving the same value from the same transcript — so + // the two agree by construction rather than by exclusion. + let holds_lock = take_write_lock(&lock, surface)?; let bound = store::commit(store::resolve(&repo)?)?; if let Some(note) = &bound.note { @@ -1356,17 +1359,25 @@ fn run_state_record( // The worktree actually scanned, not the main root — this is metadata for a // human reading a report, and naming the wrong directory would misdirect it. let here = std::env::current_dir().ok(); - let recorded = findings::record( - &bound.dir, - &context, - &commit, - here.as_deref().and_then(Path::to_str), - &scan.findings, - schema, - // The rules that never looked. Without this the pass below reads their - // silence as "clean" and resolves every finding they cover (CLOUD-81). - &scan.not_evaluated, - )?; + // THE SCAN'S RECORD IS THE HALF THAT NEEDS THE LOCK. Skipped rather than + // resolved when this caller lost it: `Recorded::default()` is all zeroes, so + // the report below says nothing happened, which is true. + let recorded = if holds_lock { + findings::record( + &bound.dir, + &context, + &commit, + here.as_deref().and_then(Path::to_str), + &scan.findings, + schema, + // The rules that never looked. Without this the pass below reads + // their silence as "clean" and resolves every finding they cover + // (CLOUD-81). + &scan.not_evaluated, + )? + } else { + findings::Recorded::default() + }; // The transcript-substrate detectors (CLOUD-97, CLOUD-98), folded in beside // the rule scan rather than through it: their identities are sequences over @@ -1374,6 +1385,14 @@ fn run_state_record( // no vocabulary to express. They run AFTER `record` on purpose — that pass // resolves what this context no longer sees, and a raise written before it // would be reasoning about a store mid-update. + // + // AND THEY RUN WHETHER OR NOT THIS CALLER HOLDS THE LOCK (CLOUD-1541), which + // is the whole of that row's fix. The nudge ladder reads what this writes a + // few lines after the call returns, so skipping them is what silenced a + // fresh claim's first turn. They are safe unlocked for a reason the scan's + // record is not: one record file per identity, written by atomic rename, + // with no journal entry appended — and the holder is deriving the same value + // from the same transcript. register_transcript_detectors( &repo, &Recording { @@ -1387,6 +1406,17 @@ fn run_state_record( err, )?; + if !holds_lock { + // REPORTED, never silent: the detectors spoke, the scan's record did + // not, and a reader owed the difference gets the same `persisted:false` + // reading the degraded-store arm above gives for its own reason. + writeln!( + err, + "batten: state record {context}: another writer holds the store; persisted:false" + )?; + return Ok(ExitCode::Success); + } + // Fold any dispositions this worktree journalled since the last record. A // lost lock race is not a failure — the entries stay in the shard and the // next record folds them — so it reports and carries on. diff --git a/crates/batten/tests/it/stop_posture.rs b/crates/batten/tests/it/stop_posture.rs index 3fe0c1068..717b543a6 100644 --- a/crates/batten/tests/it/stop_posture.rs +++ b/crates/batten/tests/it/stop_posture.rs @@ -1113,3 +1113,64 @@ fn a_stop_that_skipped_the_scan_holds_the_rule_finding_rather_than_resolving_it( which is the whole of CLOUD-81 on this path: {occurrences}" ); } + +/// A CONTENDED STORE LOCK STILL SPEAKS (CLOUD-1541). +/// +/// # The regression this exists because of +/// +/// Detaching the state record put an advisory lock over the store's write +/// phase, and the mediated path may not wait on it — the 100ms budget forbids +/// it, and a revision that did wait measured 99–124s. So it `try_lock`s, and +/// the first version RETURNED when it lost. +/// +/// That silenced the contract `the_first_turn_on_a_fresh_claim_still_speaks` +/// pins. The nudge ladder reads this store a few lines after `record_state` +/// returns, so a turn that returned before the detectors ran minted nothing — +/// and on a FRESH claim there is no earlier record to fall back on, so the turn +/// said nothing at all. +/// +/// # Why the suite could not see it, which is the point of this case +/// +/// Every other case here runs with the lock free. The contended arm is the +/// unrepresentative one to omit, because consecutive turns end inside the +/// drain's window by construction — that window is the whole reason the lock +/// exists. +/// +/// # The lock is HELD BY THIS TEST, not by a spawned drain +/// +/// A real drain would make the case a race: it holds the lock for as long as its +/// scan takes, which is neither bounded nor knowable from here. Taking the lock +/// directly makes the contended state a precondition the case CREATES, which is +/// what `.claude/rules/rust.md` demands of a test whose environment would not +/// otherwise produce the failing condition. +#[test] +fn a_stop_whose_store_lock_is_held_elsewhere_still_speaks() { + let (repo, home) = unlanded_fixture("stop-unlanded-contended"); + + // The same path `run_state_record` derives: `$GIT_DIR` beside the store it + // guards. Spelled here rather than imported because the constant is private + // — and a rename would leave this holding the wrong file, which the + // assertion below would catch rather than pass over: with the lock free the + // case proves nothing new, so it is the PAIR with the case above that + // discriminates, not this one alone. + let lock_path = repo.join(".git").join("batten-state-record.lock"); + let held = fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path) + .expect("open the store's write lock"); + fs4::FileExt::lock(&held).expect("hold the store's write lock"); + + let output = hook_in(&repo, &home, &stop_payload("Landed and pushed.", false)); + let stdout = stdout_of(&output); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + + drop(fs4::FileExt::unlock(&held)); + + assert!( + stdout.contains(batten::completion::RULE_ID), + "the claim is still reported while another writer holds the store: \ + {stdout}\n--- stderr ---\n{stderr}" + ); +} From e77de5b7041dafe5ae845577a3dded43ff33e1b5 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 16:04:03 +0000 Subject: [PATCH 11/12] fix(override): anchor a policy predicate's mint on its finding, not the HEAD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `admission_anchor` narrows the scan it runs to `declared.id == rule`, and a refusal names the PREDICATE. So for every policy module whose predicate ids differ from its enabling row's id — which is every real one, since a row carries many predicates under one id (CLOUD-832) — that filter selected nothing, the scan produced no finding, the match count was zero, and the mint took the `Anchor::Call` fallback. That fallback is commented "never weaker than what shipped before". For a tree finding it is fatal: CLOUD-1125 moved every tree finding to a `Finding` anchor, `apply_admissions` builds only that token, and the two are deliberately tagged apart. The admission was therefore answered, spent, and queried by nothing — the silent no-op the fallback's own comment warns a mismatched anchor produces. Measured on this branch before the fix: admissions for `filed-over-own-diff` and `filed-and-left-open`, both predicates of the `filed-here` row, were issued, spent and committed, and `batten-check` reported both findings unchanged. The sanctioned exit from those two refusals did not exist. An empty exact match now widens to `policy` rows and no further, since only a policy row can publish an id that is not its own. Every typed kind keeps the narrow fast path the ~90s measurement bought. The suite could not see this: every existing case mints against `always-refuses`, whose fixture names the predicate the same string as the row enabling it, so the narrowing matched by coincidence. The new case brings a module whose predicate id differs, which is the whole point of it. Shown able to fail: restoring the old narrowing reports `Call { head: ... }` where the finding's anchor belongs. Both refusals this branch could not otherwise clear are articulated here rather than in commits of their own. CLOUD-1480 is the row this branch implements, so its §1 naming lib.rs is the point rather than a deferral; it cannot close until CLOUD-1522 and CLOUD-1524 buy the 120ms that would let a `stop` arm sit under the 100ms ceiling. CLOUD-1549 is independent work: an emission in `ready.rs`, which this branch does not open. The blocks ride this commit because the anchor is now the finding's own fingerprint rather than a HEAD, so which commit carries them binds nothing — and an empty commit is not available to carry them: `lease::a_real_branch_enumerates_more_than_a_handful_of_objects` drives `objects_to_send` over this repository's own HEAD~1..HEAD and needs a commit that touches a blob. Admits: 9b8f159ae7df0d67a192c2fdbf8b39cba753c2ed0014be1fb21d62075315dbdf Admits-rule: filed-over-own-diff Admits-verdict: issue file same Admits-subject: crates/batten/src/lib.rs Admits-anchor: finding:c09917225d6081ef1f1947ef77d70a183dca052a4300b8a69dd5476f11eddc8d Admits-epoch: 6d34530beb792fa2e55cbfb438285bd8e6d6e99fc56da11bc28e55776b3c7a98 Admits-author: alec@wenzowski.com Admits-prev: c13449500bf3ecddc8b11a97b83798e112d4b4a10c8da10462911dff89e30daf Admits-answer-lost: The Stop path stays at 110s on every turn. The fix is written, tested and green, and declining leaves it unlanded over a row that names the very file it fixes. Admits-answer-precondition: The class admits a row that DOCUMENTS the change being landed, so naming its files is the point rather than a deferral. CLOUD-1480 is that row: it is the issue this branch exists to implement, its §1 names crates/batten/src/lib.rs because lib.rs is its subject, and this PR is its architectural half. Nothing was spun onto the board from this diff — the row predates the work and specifies it. Admits-answer-rejected-route: Rejected `task run other` — "name it in closing form in the PR body, so the merge lands it". Closing CLOUD-1480 would be false: its Ready block requires a `stop` arm in `perf` under the 100ms ceiling, this branch measures 220ms, and `perf-assert` would refuse that arm at `perf-over-budget` until CLOUD-1522 and CLOUD-1524 buy back the remaining 120ms. Closing the key would announce released work that is half done, which is the false-Done class CLOUD-807 and CLOUD-1292 record. Admits: 0f70a835c756902534bff8b91aee43f26087c5d23ed092a5653617406de9665e Admits-rule: filed-and-left-open Admits-verdict: issue file held Admits-subject: CLOUD-1549 Admits-anchor: finding:f6f79e011f150e05db1a5fca79f5c7cda0a875a41d29466a620d19e6e14a2e5e Admits-epoch: 6d34530beb792fa2e55cbfb438285bd8e6d6e99fc56da11bc28e55776b3c7a98 Admits-author: alec@wenzowski.com Admits-prev: a9f9718f6e57dec965ac45e6c4bf55e2885e2d5932ad8d766d770769fea8b10b Admits-answer-lost: The next agent pays what I paid: a refusal naming a row that is fine, with the actual remedy — refresh the capture — appearing nowhere in the message. It cost this branch a full verify cycle and would have been a minute with the reading's provenance emitted. Admits-answer-precondition: The class admits a row needing a decision, a mechanism or an artifact that does not exist yet. CLOUD-1549 is all three: its source of truth is crates/batten/src/ready.rs, which this branch does not open, and its remedy needs a reading-age bound whose declaration surface — per recorder row, per board rule, or one value in [ready] — is an owner's decision nobody has made. The defect it records is in the board gates themselves rather than in this diff: a stale capture made filed-unrefined refuse CLOUD-1480 while that row had been Ready for hours. Admits-answer-rejected-route: Rejected `task run first` — "close the row you filed and fix it in this diff". The fix is an emission in ready.rs, a file this branch does not open, on a surface whose bound is undecided; adding it here would widen a PR about the Stop path into the board-gate surface and would settle an owner's config question by implementation rather than by asking. Refs: CLOUD-1125 --- crates/batten/src/lib.rs | 30 ++++++++- crates/batten/tests/it/admission.rs | 98 +++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index a877ec8b2..63ea18523 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -5594,12 +5594,40 @@ fn admission_anchor( // reader. Measured before this line existed: `override request` and // `override spend` each ran a full policy scan of the tree, ~90s on this // repository, on the path `land` mints inside. - let selected: Vec<_> = config + let exact: Vec<_> = config .rules .iter() .filter(|declared| declared.id == rule) .cloned() .collect(); + // A POLICY PREDICATE IS NOT A ROW ID, and narrowing as though it were made + // every policy admission a silent no-op (CLOUD-1087, CLOUD-1125). + // + // `filed-here` publishes `filed-over-own-diff`; the refusal names the + // PREDICATE, so that is what `--rule` carries here. Filtering on + // `declared.id == rule` therefore selected NOTHING, the scan below produced + // no finding, the match count was `0`, and the mint took the `head()` + // fallback. That is fatal rather than merely weaker: CLOUD-1125 moved every + // tree finding to a `Finding` anchor, `apply_admissions` builds only that + // token, and the two are tagged apart — so the admission was answered, + // spent, and queried by nothing, which is exactly what the fallback's own + // comment warns a `Call` anchor cannot do. Measured on this branch: two + // admissions issued, spent, committed and honoured by neither gate. + // + // Only a `policy` row can publish an id that is not its own, so an empty + // exact match widens to those rows and no further. Every typed kind keeps + // the narrow fast path it was given, which is what the ~90s measurement + // above is about. + let selected: Vec<_> = if exact.is_empty() { + config + .rules + .iter() + .filter(|declared| declared.kind == rules::RuleKind::Policy) + .cloned() + .collect() + } else { + exact + }; let Ok(scan) = rules::run_all_over( &selected, &config.provisions, diff --git a/crates/batten/tests/it/admission.rs b/crates/batten/tests/it/admission.rs index 726e1177d..12c587581 100644 --- a/crates/batten/tests/it/admission.rs +++ b/crates/batten/tests/it/admission.rs @@ -1195,3 +1195,101 @@ fn a_mediated_admission_spends_after_the_tree_moves_under_it() { common::stderr(&spent) ); } + +/// A second module in the same bundle, publishing a predicate whose id is NOT +/// the enabling row's. +/// +/// That inequality is the whole point. `ALWAYS` above names its predicate +/// `always-refuses`, the same string as the `[[rule]]` id enabling it, so every +/// case built on it matched a narrowing that filters rows by `declared.id == +/// rule` — and the suite stayed green over a mint that could not work for any +/// real policy module, where a row carries many predicates under one id +/// (CLOUD-832). +const UNDER_A_ROW: &str = r#" +package batten.admits.under + +import rego.v1 + +rules contains "predicate-under-a-row" + +violation contains { + "rule": "predicate-under-a-row", + "verdict": "always probe probe", + "subjects": [{"path": "b.rs"}], +} +"#; + +fn admits_fixture_with_predicate(name: &str) -> PathBuf { + let root = admits_fixture(name); + common::write(&root, "policy-admits/under.rego", UNDER_A_ROW); + common::git_in(&root, &["add", "-A"]); + common::git_in(&root, &["commit", "-qm", "a predicate under the row"]); + root +} + +/// A MINT FOR A POLICY PREDICATE ANCHORS THE FINDING, NOT THE CALL. +/// +/// # The defect this exists because of +/// +/// `admission_anchor` narrows the scan it runs to `declared.id == rule`, and a +/// refusal names the PREDICATE — so for every policy module whose predicate ids +/// differ from its enabling row's id, that filter selected NOTHING. The scan +/// produced no finding, the match count was `0`, and the mint took the +/// `Anchor::Call` fallback. +/// +/// That fallback is documented as "never weaker than what shipped before", and +/// for a tree finding it is fatal: CLOUD-1125 moved every tree finding to a +/// `Finding` anchor, `apply_admissions` builds only that token, and +/// `a_mediated_anchor_and_a_finding_anchor_are_not_interchangeable` above pins +/// that the two do not collide. So the admission was answered, spent, and +/// queried by nothing — the exact silent no-op the fallback's own comment warns +/// a mismatched anchor produces. +/// +/// Measured before the fix, on this repository: two admissions for +/// `filed-over-own-diff` and `filed-and-left-open` — both predicates of the +/// `filed-here` row — were issued, spent, committed, and honoured by neither +/// gate. `batten-check` reported both findings unchanged afterwards. +/// +/// # Why the existing cases could not see it +/// +/// Every one of them mints against `always-refuses`, where the row id and the +/// predicate id are the same string, so the narrowing matched by coincidence. +/// The fixture is the reason the suite was green, which is why this case brings +/// its own module rather than reusing that one. +#[test] +fn a_mint_for_a_policy_predicate_anchors_the_finding_not_the_call() { + use batten::admission::{Anchor, Record}; + + let root = admits_fixture_with_predicate("policy-predicate"); + let issued = common::run_with_stdin( + &root, + &[ + "override", + "request", + "--rule", + "predicate-under-a-row", + "--verdict", + "always probe probe", + "--subject", + "b.rs", + ], + "precondition=the refusal is the fixture's point\nlost=the finding is the subject\n\ + rejected-route=admits fix probe has nothing to change\n", + ); + let address = String::from_utf8_lossy(&issued.stdout).trim().to_owned(); + assert_eq!( + address.len(), + 64, + "an address was issued: {address:?} — {}", + common::stderr(&issued) + ); + + let path = batten::admission::record_path(&root, &address).expect("record path"); + let record: Record = + serde_json::from_slice(&std::fs::read(&path).expect("read")).expect("parse"); + assert!( + matches!(record.binding.anchor, Anchor::Finding(_)), + "a policy predicate's mint anchors the finding it answers, never the HEAD: {:?}", + record.binding.anchor + ); +} From b15fd0a08ef5ee69403de9b92aab93e92262fc3f Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sun, 6 Sep 2026 17:34:42 +0000 Subject: [PATCH 12/12] test(hook): assert the skipped scan does not RESOLVE, not which arm it takes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows reported `{"Observed":1}` — the seeded count, never advanced — against an assertion naming `NotObserved`. Both are correct trees, and the platform decides which: where the mediated call takes the store's write lock the skipped scan marks every rule `NotObserved` and the pass holds, and where it loses that lock the write phase does not run at all and the instance keeps what it was seeded with. `fs4`'s locking is mandatory on Windows, so a handle this process already holds does not re-acquire, and that is the arm CI took. Untouched is not resolved. The defect this case exists for is a rule finding being RESOLVED by a scan that never ran, so `Observed(0)` is what the assertion names now rather than one of the two ways of avoiding it. Still shown able to fail: restoring `Scan::default()` reports `{"Observed":0}` and the case goes red. Refs: CLOUD-1480 --- crates/batten/tests/it/stop_posture.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/crates/batten/tests/it/stop_posture.rs b/crates/batten/tests/it/stop_posture.rs index 717b543a6..63ee4eb9a 100644 --- a/crates/batten/tests/it/stop_posture.rs +++ b/crates/batten/tests/it/stop_posture.rs @@ -1107,10 +1107,26 @@ fn a_stop_that_skipped_the_scan_holds_the_rule_finding_rather_than_resolving_it( .find(|instance| instance["context"] == "refs/heads/work") .expect("an instance on the branch the scan ran on")["occurrences"] .clone(); - assert!( - occurrences.get("NotObserved").is_some(), - "a surface that did not look HOLDS the finding rather than resolving it, \ - which is the whole of CLOUD-81 on this path: {occurrences}" + // NOT RESOLVED — which is the defect — rather than one specific arm. + // + // Two outcomes are both correct here and the platform decides which. Where + // this call takes the store's write lock, the skipped scan marks every rule + // `NotObserved` and the pass HOLDS. Where it loses that lock, the write + // phase does not run at all and the instance keeps the value it was seeded + // with — untouched is not resolved either. Windows takes the second arm, + // because `fs4`'s locking is mandatory there and a handle this process + // already holds does not re-acquire. + // + // An assertion naming `NotObserved` alone therefore fails on a tree that is + // correct, which is what it did: CI reported `{"Observed":1}` — the seeded + // count, never advanced. The defect this case exists for is a rule finding + // being RESOLVED by a scan that never ran, and `Observed(0)` is the whole of + // it, so that is what the assertion names. + assert_ne!( + occurrences, + serde_json::json!({ "Observed": 0 }), + "a surface that did not look must never resolve the finding, which is \ + the whole of CLOUD-81 on this path: {occurrences}" ); }