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 304654ec1..63ea18523 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -365,7 +365,16 @@ 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, + 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 { identity, @@ -1091,8 +1100,154 @@ 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 { +/// 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"; + +/// 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, + 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, + // 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, + // 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)?; // **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 @@ -1116,41 +1271,73 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R // 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("."), - )?; - 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(", ")), - )?; - } + // 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 { + // 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() + } + }; + report_withheld(&scan, mode, err)?; + + // THE WRITE PHASE STARTS HERE, and so does the lock. Everything above is a + // READ, so nothing above needs one writer. + // + // 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 { @@ -1172,17 +1359,25 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R // 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 @@ -1190,6 +1385,14 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R // 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 { @@ -1203,6 +1406,17 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R 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. @@ -1214,19 +1428,7 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R )?; } - // 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 @@ -5392,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, @@ -5410,6 +5640,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 { @@ -10514,9 +10748,120 @@ 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. +/// 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 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); + 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 + // not turn that into a verdict about the turn. + return; + }; + // 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) { + args.push("--strictness".to_owned()); + args.push(value.get_name().to_owned()); + } + } + if overrides.fail_on_warning { + args.push("--fail-on-warning".to_owned()); + } + if let Some(reference) = overrides.config_from.as_deref() { + args.push("--config-from".to_owned()); + args.push(reference.to_owned()); + } + if let Some(dir) = overrides.config_in.as_deref() { + 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. + exec::detached(&exe, &args, &[(DRAIN_MARKER, "1")]); } /// The `completion.unlanded` verdict for this branch, or nothing (CLOUD-1163). @@ -13473,6 +13818,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 2dcba19bf..4bd075bb8 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,7 +6183,7 @@ fn run_static_inner( )); } } - run(rules, &[], root, &bundles, vocabulary, scope, now) + run(rules, &[], root, &bundles, vocabulary, opts) } /// Run only the rules that cannot spawn a process, and report the ones that can @@ -6191,13 +6207,31 @@ 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. +/// **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 +/// 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 /// @@ -6208,28 +6242,41 @@ 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, + // 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() .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, 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, + 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 @@ -6270,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, @@ -6285,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 @@ -6300,7 +6352,7 @@ 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, opts) } /// Run every rule in `rules` against the tree rooted at `root`, returning all @@ -6321,13 +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 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 @@ -6399,8 +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. - let symbols = symbols_fact(rules, root); - let review = review_fact(rules, root); + 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 @@ -6506,6 +6567,7 @@ fn run( records: &records, records_blocked: &records_blocked, git: &git, + surface, symbols: &symbols, review: &review, state: state.as_ref(), @@ -6537,6 +6599,51 @@ 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<'_>, + 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"); + } + 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). /// @@ -6570,6 +6677,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, inputs.surface) { + 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. @@ -6867,6 +7007,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>, @@ -8434,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 @@ -13233,6 +13423,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, 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 + ); +} 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. diff --git a/crates/batten/tests/it/stop_posture.rs b/crates/batten/tests/it/stop_posture.rs index 40f2f17b0..63ee4eb9a 100644 --- a/crates/batten/tests/it/stop_posture.rs +++ b/crates/batten/tests/it/stop_posture.rs @@ -974,3 +974,219 @@ 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(); + // 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}" + ); +} + +/// 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}" + ); +}