diff --git a/batten.toml b/batten.toml index 2346aade9..1d4b5200e 100644 --- a/batten.toml +++ b/batten.toml @@ -8029,9 +8029,22 @@ target = ".claude/rules/commits.md" id = "hook wire duplicate" gloss = "a registration on a merged surface outside this repository does not reach the mediator" class = """ -`PreToolUse` is ONE entry — the engine — and CLOUD-312 measured the alternative: six task-runner launches per Bash call, 1.247s serial to do milliseconds of policy, ~93% of it startup. A second decider registered beside the mediator re-introduces that cost and, worse, splits the verdict across two authorities that can disagree about one call. The wiring this judges is the launcher's own merged settings, which live outside this repository — CLOUD-1167's fact is what makes the question expressible at all, and the row that declares the file is the whole bound on what the module may read. +`PreToolUse` is ONE entry — the engine — and CLOUD-312 measured the alternative: six task-runner launches per Bash call, 1.247s serial to do milliseconds of policy, ~93% of it startup. A second decider registered beside the mediator re-introduces that cost and, worse, splits the verdict across two authorities that can disagree about one call. The wiring this judges is the launcher's own merged settings, which live outside this repository — and `batten wiring reclaim` removes the registration from them, so this IS repairable from here. It is a REPAIR AND NOT A FIX: a launcher that writes those registrations at session start writes them again next session, so the same refusal recurs and the same command answers it each time. CLOUD-1167's fact is what makes the question expressible at all, and the row that declares the file is the whole bound on what the module may read. """ +# THE COMMAND FIRST, because a reader who follows only the first route must reach +# the thing that clears the refusal (CLOUD-1339). Both routes were `document` +# before and neither document names the verb — measured 2026-09-02, that cost one +# session three commits with `hooks-wiring-check` switched off, a false claim +# about this repository's capabilities written into two commit messages on +# main-bound history, and a three-option menu put to a human that one command +# already answered. The predicate was right every time; only the remedy was +# missing, which is the shape CLOUD-122's contract exists to refuse. +[[verdict.route]] +id = "verb run first" +kind = "command" +target = "batten wiring reclaim" + [[verdict.route]] id = "prose read first" kind = "document" diff --git a/crates/batten/src/doctor.rs b/crates/batten/src/doctor.rs index 8a86c762f..4b73ffc09 100644 --- a/crates/batten/src/doctor.rs +++ b/crates/batten/src/doctor.rs @@ -663,9 +663,21 @@ const SIBLING_REGISTERED: &str = "hook-wiring-sibling-registered"; /// The same, on a surface the host MERGES rather than the committed one. /// /// Its own id because the remedy differs and the committed one's does not reach -/// it: a merged surface is under `$HOME`, so editing the repository cannot +/// it: a merged surface is under `$HOME`, so editing the REPOSITORY cannot /// remove the registration — the same reason [`MERGED_REGISTRATION`] is separate /// from [`EVENT_REGISTERED_N_TIMES`]. +/// +/// **That is a statement about editing tracked files, not about this +/// repository's reach, and it was read as the latter** (CLOUD-1339). +/// `batten wiring reclaim` removes a merged registration, and +/// `crates/batten/tests/it/wiring_reclaim.rs` drives it against a real `$HOME` — +/// so the remedy exists and ships here. Measured 2026-09-02, a session read this +/// comment beside the refusal, concluded the condition was unfixable from the +/// repository, switched the gate off for three commits and wrote that conclusion +/// into two commit messages. The verb is a REPAIR rather than a fix — a launcher +/// that registers at session start registers again next session — but a repair +/// is not nothing, and it is what the `hook wire duplicate` verdict now routes +/// to first. const MERGED_SIBLING: &str = "hook-wiring-merged-sibling"; /// The wiring file is there and is not readable as the JSON object it must be. /// @@ -911,11 +923,27 @@ fn reaches_engine(entry: &str, harness: hook::Harness) -> bool { } // (a) The derived argv appears as a contiguous run immediately after a token // whose file stem is the binary's name. - let derived = ["hook", "--harness", harness.as_str()]; + // + // DERIVED FROM THE `SURFACE` ROW, NOT SPELLED HERE (CLOUD-1191). This read + // `["hook", "--harness", harness.as_str()]` — a literal independent of the + // declaration — so it answered "does the wiring name THIS STRING" where the + // question is "does the wiring name the declared mediation path". Those + // differ exactly when it matters: against a settings file naming a command + // the surface no longer declares, the literal version returns `true` and + // reports stale wiring as healthy. The one diagnostic built for that failure + // was blind to it. + // + // No mediation row means no declared path for a registration to reach, so + // nothing reaches the engine. That is `false` — loud — rather than a literal + // fallback, which would be the spelling this change removes. + let Some(mut derived) = crate::surface::mediation_argv() else { + return false; + }; + derived.push(harness.as_str().to_owned()); tokens.iter().enumerate().any(|(at, token)| { Path::new(token) .file_stem() - .is_some_and(|stem| stem == "batten") + .is_some_and(|stem| stem == crate::surface::BINARY) && tokens.len() >= at + 1 + derived.len() && tokens[at + 1..=at + derived.len()] == derived }) @@ -1150,6 +1178,42 @@ mod tests { dir } + /// `reaches_engine` answers about the DECLARED mediation path, not a + /// literal — so wiring naming a command the surface no longer declares is + /// drift rather than health (CLOUD-1191). + /// + /// **The second assertion is the whole row.** Before the derivation, the + /// check matched the literal `"hook"`, so a settings file naming a renamed + /// or removed verb still returned `true`: the one diagnostic built for this + /// failure reported it healthy, while the invocation itself became an + /// unknown subcommand — clap error, exit `1`, which every host reads as + /// allow. Fail-open, reported green. + /// + /// Fails by: reverting `reaches_engine` to a literal argv. + #[test] + fn wiring_naming_an_undeclared_path_does_not_reach_the_engine() { + let harness = hook::Harness::ClaudeCode; + let live = hook::wiring_command(harness); + assert!( + reaches_engine(&live, harness), + "the command this build emits must reach the engine: {live}" + ); + + // The same shape with a verb the surface does not declare. This is what a + // half-done rename leaves in a committed wiring file. `adjudicate` is the + // path CLOUD-1192 proposes, so this is the exact string that row's + // rename would strand if it landed without this derivation. + let stale = format!( + "{} adjudicate --harness {}", + crate::surface::BINARY, + harness.as_str() + ); + assert!( + !reaches_engine(&stale, harness), + "wiring naming an undeclared verb must read as drift, not health: {stale}" + ); + } + #[test] fn a_missing_config_is_named_rather_than_lumped_in() { let dir = scratch("missing-config"); @@ -1793,8 +1857,21 @@ mod tests { "a legitimate spelling must stay green — {spelling}" ); } + // DERIVED, because a literal here is the fourth spelling CLOUD-1191 + // removed — this assertion hardcoded `hook` and went red on the rename, + // which is the derivation catching its own test rather than the test + // catching the derivation. + let argv = crate::surface::mediation_argv().expect("declared"); assert!( - reaches_engine("/opt/bin/batten.exe hook --harness claude-code", harness), + reaches_engine( + &format!( + "/opt/bin/{}.exe {} {}", + crate::surface::BINARY, + argv.join(" "), + harness.as_str() + ), + harness + ), "the stem match is what lets a Windows image pass" ); // And the drift case the existing suite pins stays drift. diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index 1d70d5d86..a03457f95 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -1164,8 +1164,25 @@ const CLAUDE_SPELLINGS: &[(Event, &str)] = &[ /// the one module allowed to. The search was an install concern. What the /// paragraph above still says is unchanged, and it is what keeps the emitter able /// to serve a consumer that does need one. +/// +/// **The argv is DERIVED from the `SURFACE` row, never spelled here** +/// (CLOUD-1191). This was `format!("batten hook --harness {}", …)`, one of three +/// independent spellings; renaming the row left this one behind, and the +/// resulting unknown subcommand is a clap error — exit `1` — which every host +/// reads as allow. The disagreement would not have broken loudly, it would have +/// turned enforcement off everywhere while `doctor` reported green. +/// +/// A surface declaring no mediation row yields the binary and the harness with +/// no verb between them — a command that matches nothing, so every registration +/// reports as drift. That is the LOUD direction, and it is why there is no +/// literal fallback here: emitting `"hook"` when the declaration is gone would +/// be the fourth spelling this removes, and it would report healthy. pub(crate) fn wiring_command(harness: Harness) -> String { - format!("batten hook --harness {}", harness.as_str()) + let argv = crate::surface::mediation_argv().unwrap_or_default(); + let mut parts = vec![crate::surface::BINARY.to_owned()]; + parts.extend(argv); + parts.push(harness.as_str().to_owned()); + parts.join(" ") } /// Render one harness's registrations as the JSON its host reads. diff --git a/crates/batten/src/spec.rs b/crates/batten/src/spec.rs index f8038f444..2cb2f4564 100644 --- a/crates/batten/src/spec.rs +++ b/crates/batten/src/spec.rs @@ -560,13 +560,21 @@ mod tests { // verb ("structurally incapable, not merely well-behaved") cannot be // made about a verb whose whole job is adjudicating someone else's // write. Pinned here so the correction cannot be undone by a row edit. + // DERIVED FROM THE ROW (CLOUD-1191), so the pin survives the rename it + // is pinning against. Spelled `hook` until CLOUD-1192 moved the path to + // `adjudicate`; a literal here would have gone quietly green on that + // rename by asking about a path the surface no longer declares — + // asserting nothing, in the unsafe direction this case exists to stop. + let path = crate::surface::mediation() + .expect("the surface declares a mediation row") + .path; let allowlist = read_only_allowlist(&spec()); assert!( - !allowlist.iter().any(|entry| entry.path == "hook"), + !allowlist.iter().any(|entry| entry.path == path), "the mediation entrypoint leaked into the read-only allowlist: {allowlist:?}" ); - assert_eq!(effect_for("hook"), Effect::Unclassified); - assert!(!effect_for("hook").is_read_only()); + assert_eq!(effect_for(path), Effect::Unclassified); + assert!(!effect_for(path).is_read_only()); } #[test] diff --git a/crates/batten/src/surface.rs b/crates/batten/src/surface.rs index 6274ac6b6..be141eb78 100644 --- a/crates/batten/src/surface.rs +++ b/crates/batten/src/surface.rs @@ -1872,6 +1872,70 @@ const ROOT_FLAGS: &[FlagDecl] = &[ }, ]; +/// The binary's own name, as a wiring entry spells it. +/// +/// Here rather than beside each consumer for [`mediation`]'s reason: the +/// generator emits it and the diagnostic matches a token's file stem against it, +/// and those two agreeing is what makes a registration reach the engine. +pub const BINARY: &str = "batten"; + +/// The stable id of the row that adjudicates a mediated tool call. +/// +/// **The anchor is the id and never the path**, which is [`CommandDecl::id`]'s +/// whole contract: the path is "the one thing about a row that is expected to +/// change", so a derivation keyed on it re-breaks on exactly the rename it +/// exists to survive. +pub const MEDIATION_ID: &str = "hook"; + +/// The mediation row, resolved from [`SURFACE`] by [`MEDIATION_ID`]. +/// +/// **One authority for how the mediator is invoked** (CLOUD-1191). Before this, +/// the argv was spelled independently in three places with nothing linking them +/// — the row here, the generator in [`crate::hook::wiring_command`], and the +/// diagnostic in `doctor`'s `reaches_engine` — plus five committed wiring files +/// carrying it as data. +/// +/// # Why a disagreement between them is worse than ordinary drift +/// +/// It is a **silent fail-open**. An unknown subcommand is a clap error, which is +/// [`crate::exit::ExitCode::Usage`] (`1`), and `exit.rs` states the consequence +/// as a design property: every host reads anything but `0`/`2` as "the hook +/// itself failed, let the call through". So three literals that disagree do not +/// break loudly — they turn enforcement off across every harness while `doctor` +/// reports green. Renaming the row was measured safe on paper and would have +/// done exactly that. +/// +/// Returns `None` only if the row is absent, which +/// [`tests::the_mediation_row_resolves`] refuses. Callers treat `None` as "no +/// declared mediation path" and fail loud rather than falling back to a literal +/// — a fallback would reintroduce the fourth spelling this exists to remove. +#[must_use] +pub fn mediation() -> Option<&'static CommandDecl> { + SURFACE.iter().find(|row| row.id == MEDIATION_ID) +} + +/// The mediation row's argv as the wiring spells it: the path, then each +/// required flag as `--long`. +/// +/// Derived rather than formatted, so a change to the row's `path` or to its +/// required flags moves the emitted wiring and the diagnostic's expectation in +/// the same build. +/// The value each required flag takes is the CALLER's — the harness — so this +/// stops at the flag and the caller appends it. That keeps the one thing that +/// varies per registration out of a function whose whole job is the part that +/// does not. +#[must_use] +pub fn mediation_argv() -> Option> { + let row = mediation()?; + let mut argv = vec![row.path.to_owned()]; + for flag in row.flags.iter().filter(|flag| flag.required) { + if let Some(long) = flag.long { + argv.push(format!("--{long}")); + } + } + Some(argv) +} + /// The command tree: every subcommand, with its summary, effect, and flags. /// /// Order is declaration order and does not matter — [`command`] groups rows by @@ -4033,6 +4097,50 @@ mod tests { assert_eq!(seen.len(), total, "a command path is declared twice"); } + /// The mediation row resolves, which is what lets every consumer treat a + /// `None` from [`mediation`] as "the surface declares none" rather than as a + /// bug it has to guard. + /// + /// Fails by: renaming or deleting the row's `id`. That is the one edit + /// [`MEDIATION_ID`] does not survive, and it must be loud — the id is the + /// stable anchor precisely so the `path` can move without touching it. + #[test] + fn the_mediation_row_resolves() { + let row = mediation().expect("the surface declares a mediation row"); + assert_eq!(row.id, MEDIATION_ID); + assert!( + row.flags.iter().any(|flag| flag.required), + "the mediation row must carry a required flag, or `mediation_argv` \ + emits a bare path and every registration reads as drift" + ); + } + + /// The emitted argv follows the row's `path`, not a literal. + /// + /// **This is the case that would have caught the defect.** With three + /// independent spellings, renaming the row left the generator and the + /// diagnostic behind, and the resulting unknown subcommand exits `1` — which + /// every host reads as allow. Fails by: reverting either consumer to a + /// `"hook"` literal. + #[test] + fn the_emitted_argv_is_the_rows_path_and_its_required_flags() { + let row = mediation().expect("declared"); + let argv = mediation_argv().expect("declared"); + assert_eq!( + argv.first().map(String::as_str), + Some(row.path), + "the argv must open with the row's path" + ); + for flag in row.flags.iter().filter(|flag| flag.required) { + if let Some(long) = flag.long { + assert!( + argv.iter().any(|word| word == &format!("--{long}")), + "a required flag the row declares is missing from the argv: {long}" + ); + } + } + } + #[test] fn every_declared_parent_is_itself_declared() { // `config show` without a `config` row would build a subcommand tree diff --git a/crates/batten/tests/it/bypass_scrub.rs b/crates/batten/tests/it/bypass_scrub.rs index ad600d276..307afa93b 100644 --- a/crates/batten/tests/it/bypass_scrub.rs +++ b/crates/batten/tests/it/bypass_scrub.rs @@ -175,6 +175,150 @@ fn the_hatch_is_load_bearing() { ); } +/// THE THIRD CHANNEL, AND THE ONE THIS FILE ALREADY KNEW ABOUT. +/// +/// `the_hatch_is_load_bearing` above had to stop observing the hatch through a +/// protected-path refusal, and its comment says why in as many words: *"that +/// class declares an override route and the boundary honours a spent admission +/// for it, so the variable stopped being its way out."* So this suite recorded +/// that an ADMISSION had replaced the variable for that class — and then left the +/// admission store ambient, which is the half nobody closed. +/// +/// The two scrubs this file asserts are walks over ENVIRONMENT VARIABLES, and an +/// admission is a signed record in the state store rather than a knowable string. +/// CLOUD-1051 made it that way on purpose. So the channel that replaced the +/// scrubbed ones is unreachable from either walk by construction, and the state +/// root has to be redirected explicitly instead. +/// +/// A FIXTURE HOME CANNOT ESCAPE THIS, which is why the redirect belongs in +/// `common::batten()` rather than in each case. The state segment is derived from +/// the repository root, so a suite whose subject is the COMMITTED config — this +/// one, `mediated_verbs.rs`, `gh_guard.rs`, `pipeline_shapes.rs`, +/// `refusal_ceiling.rs` — runs against the real root and therefore reads the real +/// repository's own segment. `mediated_admission.rs` is unaffected for exactly +/// the same reason, in the other direction: its fixture is a scratch repo, so it +/// has always had a segment of its own. +#[test] +fn the_ambient_state_root_never_reaches_the_binary_under_test() { + // READ THROUGH `common`, NEVER BY NAMING THE VARIABLES HERE. That module is + // the one place they may be spelled, which + // `primitives::no_suite_sets_the_state_dir_variables_itself` enforces — a + // case that re-typed them to assert the redirect would become the copy that + // audit exists to refuse, while claiming there are none. + let redirected = common::state_roots(&common::batten_at_real_root()); + assert_eq!( + redirected.len(), + 2, + "both state roots must be redirected, on every platform: {redirected:?}" + ); + let scratch_root = common::scratch_state_root(); + for (name, value) in &redirected { + assert_eq!( + value.as_path(), + scratch_root, + "{name} must point at the suite's own state root, never the developer's" + ); + } +} + +/// THE ANTI-VACUITY MIRROR for the case above, and the same shape +/// `the_hatch_is_load_bearing` uses one screen up: it shows an admission +/// genuinely disarming the committed policy, so redirecting the store it lives in +/// is load-bearing rather than tidy. +/// +/// Over the REAL repository root, deliberately — a scratch fixture would prove +/// the mechanism and miss the defect, because the defect IS that these suites +/// share the real repository's segment. Contained only because the case above now +/// holds: every spawn here writes into the suite's own state root, so issuing and +/// spending a real admission cannot touch the developer's store. +/// +/// MEASURED 2026-09-02, before the redirect landed. A spent admission for +/// `batten.toml` — taken by hand, for unrelated work, hours earlier in the same +/// session — turned +/// `cli.rs::the_committed_protected_paths_fire_on_a_mutating_verb` green-side: +/// `mv batten.toml elsewhere.toml` answered exit `0` where that case demands `2`. +/// The suite reported that the committed protected-path policy refuses a write +/// while a record on the machine was admitting it. +#[test] +fn an_admission_in_the_store_disarms_the_committed_protected_gate() { + let root = at_root("batten.toml"); + let root = root.parent().expect("the committed config has a parent"); + let payload = serde_json::json!({ + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "tool_input": {"command": "mv batten.toml elsewhere.toml"}, + }) + .to_string(); + + let refused = run(common::batten_at_real_root().current_dir(root), &payload); + assert_eq!( + refused, + Some(2), + "the committed policy must refuse on its own, or the comparison below proves nothing" + ); + + let answers = "precondition=the suite is demonstrating that this channel admits, which is \ + the property the redirect above exists to contain\n\ + lost=nothing: this admission is spent inside the suite's own state root and \ + is unreachable from any other process\n\ + rejected-route=every declared route is a real remedy for a real write; this \ + case is not making one, it is proving the channel is load-bearing\n"; + let issued = common::run_with_stdin_at_real_root( + root, + &[ + "override", + "request", + "--rule", + "protected-mutation", + "--verdict", + "path write refused", + "--subject", + "batten.toml", + ], + answers, + ); + assert!( + issued.status.success(), + "the request must issue: {}", + String::from_utf8_lossy(&issued.stderr) + ); + let admission = String::from_utf8_lossy(&issued.stdout) + .split_whitespace() + .last() + .unwrap_or_default() + .to_owned(); + assert!(!admission.is_empty(), "the request must name an address"); + + let spent = common::run_at_real_root( + root, + &[ + "override", + "spend", + "--admission", + &admission, + "--rule", + "protected-mutation", + "--verdict", + "path write refused", + "--subject", + "batten.toml", + ], + ); + assert!( + spent.status.success(), + "the admission must spend: {}", + String::from_utf8_lossy(&spent.stderr) + ); + + let admitted = run(common::batten_at_real_root().current_dir(root), &payload); + assert_eq!( + admitted, + Some(0), + "a spent admission admits the write — which is why the store must never be the \ + developer's" + ); +} + #[expect( clippy::disallowed_types, reason = "stays, and test-only: the subject of `the_hatch_is_load_bearing` is what \ diff --git a/crates/batten/tests/it/cli.rs b/crates/batten/tests/it/cli.rs index 7213b3a33..6d09562f4 100644 --- a/crates/batten/tests/it/cli.rs +++ b/crates/batten/tests/it/cli.rs @@ -114,7 +114,7 @@ fn repo_with_gh_policy(name: &str) -> PathBuf { /// points at `crates/batten/`, which has no `batten.toml` — that is the /// no-authority case, which several tests want. fn run_hook_in(dir: &std::path::Path, harness: &str, payload: &str, bypass: bool) -> Output { - let mut command = batten(); + let mut command = common::batten_at_real_root(); command .current_dir(dir) .args(["hook", "--harness", harness]) @@ -151,7 +151,7 @@ fn run_hook_with_env( payload: &str, env: &[(&str, &str)], ) -> Output { - let mut command = batten(); + let mut command = common::batten_at_real_root(); command .current_dir(dir) .args(["hook", "--harness", harness]) diff --git a/crates/batten/tests/it/common/mod.rs b/crates/batten/tests/it/common/mod.rs index 0c055a4d2..9e7ab49d6 100644 --- a/crates/batten/tests/it/common/mod.rs +++ b/crates/batten/tests/it/common/mod.rs @@ -235,6 +235,98 @@ pub(crate) fn batten() -> Command { command } +/// The state root a suite whose subject is the REAL repository must run under. +/// +/// **A THIRD SUPPRESSION CHANNEL, AND THE ONE NEITHER SCRUB IN [`batten`] CAN +/// SEE.** That function removes every `BATTEN_` variable the surface declares and +/// every bypass name beside it, and both of those are walks over ENVIRONMENT +/// VARIABLES. An **admission** is not one: CLOUD-1051 retired +/// `BATTEN_FILED_HERE_BYPASS` and its siblings precisely so that suppressing a +/// refusal would cost a signed record in the state store rather than a knowable +/// string anyone could export. So the channel that replaced the scrubbed ones is +/// unreachable from the scrub by construction — the same shape as `BATTEN_BIN`, +/// and for the same reason. +/// +/// **Measured 2026-09-02, and it is a false green in the unsafe direction.** A +/// spent admission for `batten.toml` in the DEVELOPER'S OWN store turned +/// `cli.rs::the_committed_protected_paths_fire_on_a_mutating_verb` green-side: +/// `mv batten.toml elsewhere.toml` answered exit `0` where the case demands `2`, +/// and the case reported that the committed protected-path policy refuses a +/// write while a record on that machine was admitting it. +/// +/// **WHY IT IS NOT THE DEFAULT IN [`batten`], WHICH WAS THE FIRST SHAPE TRIED.** +/// A fixture suite may spawn a child that WRITES the store and then read it back +/// IN-PROCESS — `admission.rs`'s `a_correctly_answered_override_completes_end_to_end` +/// does exactly that, through `admission::load`, which resolves the root from the +/// PARENT's environment. Redirecting only the child splits the two and the case +/// fails looking for a record the child filed elsewhere. That case is not the +/// defect: its subject is a scratch repo, so it already has a state segment of +/// its own and cannot collide with the real one. +/// +/// The defect is narrower than "every spawn", and naming it precisely is what +/// keeps this from being a change to how every suite runs: **a suite whose +/// subject is the COMMITTED configuration must drive the binary at the real +/// repository root, and the state segment is derived from that root — so it, and +/// only it, shares the developer's own segment.** Hence a helper the real-root +/// suites apply, rather than a default every fixture suite inherits. +/// +/// **Per PROCESS**, because nextest runs each case in its own process: state one +/// spawn writes is still there for the next spawn in the same case, and no case +/// can reach another's. Under `target/`, so `cargo clean` collects it, and +/// resolved once so the directory is created on the first use rather than every. +pub(crate) fn scratch_state_root() -> &'static Path { + static ROOT: std::sync::OnceLock = std::sync::OnceLock::new(); + ROOT.get_or_init(|| { + let dir = target_tmp().join(format!("state-{}", std::process::id())); + fs::create_dir_all(&dir).expect("create the scratch state root"); + dir + }) +} + +/// The state ROOTS a command has been pointed at, as `(name, value)` pairs. +/// +/// Here rather than in the asserting suite because this module is the one place +/// the variables may be named at all — +/// `primitives::no_suite_sets_the_state_dir_variables_itself` enforces exactly +/// that, and it is right to: CLOUD-619's defect was fourteen copies of the name, +/// one of which was POSIX-only and redirected nothing on Windows. A suite +/// checking the redirect must therefore ask this module rather than re-type the +/// names, or it becomes the fifteenth copy while asserting that there are none. +/// +/// `LOCALAPPDATA` is deliberately absent: [`state_dir`] points it at a `cache` +/// subdirectory rather than at the root, so including it would make a caller +/// compare two different things under one name. +#[must_use] +#[expect( + clippy::disallowed_types, + reason = "stays with the harness spawn it reads: the subject is what a child's environment carries, which is a property of the command rather than of anything in-process" +)] +pub(crate) fn state_roots(command: &Command) -> Vec<(String, PathBuf)> { + command + .get_envs() + .filter_map(|(name, value)| { + let name = name.to_str()?.to_owned(); + (name == "XDG_DATA_HOME" || name == "APPDATA") + .then(|| Some((name, PathBuf::from(value?))))? + }) + .collect() +} + +/// `batten`, pointed at a state root of the suite's own — for the suites whose +/// subject is the committed configuration and which therefore run at the real +/// repository root. See [`scratch_state_root`] for what this contains and why it +/// is not the default. +#[must_use] +#[expect( + clippy::disallowed_types, + reason = "stays with the harness spawn it configures, exactly as `state_home` does: the state root a child resolves is a property of that child's environment" +)] +pub(crate) fn batten_at_real_root() -> Command { + let mut command = batten(); + state_dir(&mut command, scratch_state_root()); + command +} + /// Run `batten` with `args` in `dir`. #[must_use] pub(crate) fn run(dir: &Path, args: &[&str]) -> Output { @@ -245,6 +337,20 @@ pub(crate) fn run(dir: &Path, args: &[&str]) -> Output { .expect("run batten") } +/// [`run`] for a suite whose subject is the committed configuration, so its state +/// root is the suite's own rather than the developer's. +/// +/// See [`scratch_state_root`] for the defect this closes and why it is a separate +/// entry point rather than [`batten`]'s default. +#[must_use] +pub(crate) fn run_at_real_root(dir: &Path, args: &[&str]) -> Output { + batten_at_real_root() + .args(args) + .current_dir(dir) + .output() + .expect("run batten") +} + /// Point Batten's OS data directory at `home`, on **every** platform. /// /// A third hermeticity behaviour, in the module whose header already names the @@ -370,10 +476,29 @@ pub(crate) fn state_dir<'a>(command: &'a mut Command, dir: &Path) -> &'a mut Com /// of agreement with [`batten`]. #[must_use] pub(crate) fn run_with_stdin(dir: &Path, args: &[&str], input: &str) -> Output { + stdin_run(batten(), dir, args, input) +} + +/// [`run_with_stdin`] for a suite whose subject is the committed configuration, +/// so its state root is the suite's own rather than the developer's. +/// +/// See [`scratch_state_root`] for the defect this exists to close and for why it +/// is a separate entry point rather than [`batten`]'s default. +#[must_use] +pub(crate) fn run_with_stdin_at_real_root(dir: &Path, args: &[&str], input: &str) -> Output { + stdin_run(batten_at_real_root(), dir, args, input) +} + +#[expect( + clippy::disallowed_types, + reason = "stays, and test-only: this IS the spawn-and-pipe harness, and taking the command lets the two entry points above share one body rather than drifting apart — the founding reason this module exists" +)] +#[must_use] +fn stdin_run(mut command: Command, dir: &Path, args: &[&str], input: &str) -> Output { use std::io::Write as _; use std::process::Stdio; - let mut child = batten() + let mut child = command .args(args) .current_dir(dir) .stdin(Stdio::piped()) diff --git a/crates/batten/tests/it/gh_guard.rs b/crates/batten/tests/it/gh_guard.rs index 37266dddf..737a0ddf5 100644 --- a/crates/batten/tests/it/gh_guard.rs +++ b/crates/batten/tests/it/gh_guard.rs @@ -77,7 +77,7 @@ use crate::common; use std::path::PathBuf; -use common::{run_with_stdin, stdout}; +use common::{run_with_stdin_at_real_root, stdout}; /// The four rule ids that carry the `gh` lifecycle, as `batten.toml` declares /// them. @@ -113,7 +113,7 @@ fn bash_payload(command: &str) -> String { /// `"permissionDecision": "deny"`, and that an allowed command produces no /// document at all. The exit-code adapter collapses both into a status. fn decision(command: &str) -> String { - stdout(&run_with_stdin( + stdout(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "claude-code"], &bash_payload(command), @@ -282,7 +282,7 @@ fn an_allowed_command_emits_no_decision() { #[test] fn unparseable_input_fails_open() { - let out = stdout(&run_with_stdin( + let out = stdout(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "claude-code"], "not json", @@ -296,7 +296,7 @@ fn unparseable_input_fails_open() { /// Run one command with an environment variable set, and return the document. fn decision_with_env(command: &str, key: &str, value: &str) -> String { stdout( - &common::batten() + &common::batten_at_real_root() .args(["hook", "--harness", "claude-code"]) .current_dir(root()) .env(key, value) diff --git a/crates/batten/tests/it/mediated_verbs.rs b/crates/batten/tests/it/mediated_verbs.rs index 97698e70d..27a7d9cc3 100644 --- a/crates/batten/tests/it/mediated_verbs.rs +++ b/crates/batten/tests/it/mediated_verbs.rs @@ -34,7 +34,7 @@ use crate::common; use std::path::PathBuf; -use common::{run, run_with_stdin, stderr}; +use common::{run, run_with_stdin_at_real_root, stderr}; /// A protected path this repository declares, and one it does not. /// @@ -64,7 +64,7 @@ fn bash_payload(command: &str) -> String { /// `exit-code` rather than `claude-code`: the code *is* the whole channel there, /// so a verdict is read from the status without parsing a decision document. fn verdict(command: &str) -> Option { - run_with_stdin( + run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(command), @@ -104,7 +104,7 @@ fn assert_allowed(command: &str) { /// `_memory`; no other row's text does. A caller that stops emitting that token /// fails this, which is the direction worth protecting. fn assert_not_refused_as_a_write(command: &str) { - let refusal = stderr(&run_with_stdin( + let refusal = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(command), @@ -278,7 +278,7 @@ fn the_deny_names_the_whole_action_and_the_serena_tool_to_use_instead() { // gets must still name the surface that owns the file, and for a subcommand // row it must name the action rather than only the front-end — a refusal // saying `git` would read as a ban on every use of version control. - let refusal = stderr(&run_with_stdin( + let refusal = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(&format!("git mv {GUARDED} .serena/memories/renamed.md")), @@ -286,7 +286,7 @@ fn the_deny_names_the_whole_action_and_the_serena_tool_to_use_instead() { assert!(refusal.contains("git mv"), "names the action: {refusal}"); assert!(refusal.contains(GUARDED), "names where: {refusal}"); - let edit = stderr(&run_with_stdin( + let edit = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(&format!("sed -i s/a/b/ {GUARDED}")), @@ -334,7 +334,7 @@ fn the_deny_names_the_whole_action_and_the_serena_tool_to_use_instead() { /// route. Both directions, or neither means anything — CLOUD-418. #[test] fn a_registered_module_gets_its_own_route_and_not_the_memory_one() { - let module = stderr(&run_with_stdin( + let module = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload("sed -i s/a/b/ policy/shell-retirement.rego"), @@ -362,7 +362,7 @@ fn a_registered_module_gets_its_own_route_and_not_the_memory_one() { // THE MIRROR. Without it the assertions above pass over a build that simply // stopped naming the Serena tools anywhere. - let memory = stderr(&run_with_stdin( + let memory = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(&format!("sed -i s/a/b/ {GUARDED}")), @@ -446,7 +446,7 @@ fn no_byte_of_the_mediated_command_reaches_either_stream() { // the rule, the action, the path and the remedy — never the command line, // which is the caller's own text and could carry anything. let canary = "CANARY-SECRET-VALUE"; - let output = run_with_stdin( + let output = run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload(&format!("sed -i s/a/{canary}/ {GUARDED}")), @@ -478,7 +478,7 @@ fn write_payload(tool: &str, path: &str) -> String { } fn write_verdict(tool: &str, path: &str) -> Option { - run_with_stdin( + run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &write_payload(tool, path), @@ -813,7 +813,7 @@ fn bash_payload_in(cwd: &std::path::Path, command: &str) -> String { } fn verdict_in(cwd: &std::path::Path, command: &str) -> Option { - run_with_stdin( + run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &bash_payload_in(cwd, command), @@ -953,7 +953,7 @@ fn read_payload(path: &str) -> String { } fn read_verdict(path: &str) -> Option { - run_with_stdin( + run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &read_payload(path), @@ -969,7 +969,7 @@ fn a_generic_read_of_a_memory_is_refused_and_names_the_tool_that_answers() { Some(2), "a generic file read of a memory is refused" ); - let refusal = stderr(&run_with_stdin( + let refusal = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &read_payload(GUARDED), @@ -1015,6 +1015,6 @@ fn a_write_to_a_memory_is_still_refused_as_a_write() { \"tool_input\":{{\"file_path\":{}}}}}", serde_json::to_string(GUARDED).expect("a path is encodable") ); - let run = run_with_stdin(&root(), &["hook", "--harness", "exit-code"], &payload); + let run = run_with_stdin_at_real_root(&root(), &["hook", "--harness", "exit-code"], &payload); assert_eq!(run.status.code(), Some(2), "a write is still a write"); } diff --git a/crates/batten/tests/it/pipeline_shapes.rs b/crates/batten/tests/it/pipeline_shapes.rs index e96c9972d..c56e6af3d 100644 --- a/crates/batten/tests/it/pipeline_shapes.rs +++ b/crates/batten/tests/it/pipeline_shapes.rs @@ -30,7 +30,7 @@ use crate::common; use std::path::PathBuf; -use common::{run, run_with_stdin, stderr}; +use common::{run, run_with_stdin_at_real_root, stderr}; fn root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../..") @@ -45,7 +45,7 @@ fn payload(command: &str) -> String { } fn verdict(command: &str) -> Option { - run_with_stdin( + run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &payload(command), @@ -68,7 +68,7 @@ fn assert_allowed(command: &str) { /// shapes render three causes, and the substitution family asserts that the /// cause points at the operand a caller can act on. fn cause(command: &str) -> String { - stderr(&run_with_stdin( + stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &payload(command), @@ -198,7 +198,7 @@ fn the_refusal_states_the_principle_rather_than_naming_one_command() { // CLOUD-199's second instance happened because an agent complied with the // narrower wording exactly and made the same error on the next command. The // cause therefore has to generalise, and the remedy has to be the row's. - let refusal = stderr(&run_with_stdin( + let refusal = stderr(&run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &payload("mise run verify | tail -6"), diff --git a/crates/batten/tests/it/preset_segments.rs b/crates/batten/tests/it/preset_segments.rs index f2cfb8d77..4f005a21d 100644 --- a/crates/batten/tests/it/preset_segments.rs +++ b/crates/batten/tests/it/preset_segments.rs @@ -34,7 +34,7 @@ use crate::common; use std::path::PathBuf; -use common::{run_with_stdin, stderr}; +use common::{run_with_stdin_at_real_root, stderr}; fn root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../..") @@ -50,7 +50,7 @@ fn payload(command: &str) -> String { /// The exit code and the refusal text together, because both are asserted. fn adjudicate(command: &str) -> (Option, String) { - let outcome = run_with_stdin( + let outcome = run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &payload(command), diff --git a/crates/batten/tests/it/refusal_ceiling.rs b/crates/batten/tests/it/refusal_ceiling.rs index e9de3348f..e387bbefe 100644 --- a/crates/batten/tests/it/refusal_ceiling.rs +++ b/crates/batten/tests/it/refusal_ceiling.rs @@ -21,7 +21,7 @@ use crate::common; use std::path::PathBuf; -use common::{run_with_stdin, stderr}; +use common::{run_with_stdin_at_real_root, stderr}; fn root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../..") @@ -37,7 +37,7 @@ fn payload(command: &str) -> String { /// The refusal text a mediated call produces, or `None` where it was allowed. fn refusal(command: &str) -> Option { - let run = run_with_stdin( + let run = run_with_stdin_at_real_root( &root(), &["hook", "--harness", "exit-code"], &payload(command), diff --git a/crates/batten/tests/it/shell_write_advisory.rs b/crates/batten/tests/it/shell_write_advisory.rs index 76ee7b012..954b9def3 100644 --- a/crates/batten/tests/it/shell_write_advisory.rs +++ b/crates/batten/tests/it/shell_write_advisory.rs @@ -31,7 +31,7 @@ use crate::common; use std::path::PathBuf; -use common::{Fixture, run_with_stdin, stderr, stdout}; +use common::{Fixture, run_with_stdin_at_real_root, stderr, stdout}; /// The repository root, whose committed `batten.toml` registers both modules. fn root() -> PathBuf { @@ -66,7 +66,8 @@ fn bash_payload(command: &str) -> String { /// case reading one stream would pass against a build that silently stopped /// delivering, which is the thing this file is here to catch. fn reported(payload: &str) -> String { - let answer = run_with_stdin(&root(), &["hook", "--harness", "claude-code"], payload); + let answer = + run_with_stdin_at_real_root(&root(), &["hook", "--harness", "claude-code"], payload); format!("{}{}", stdout(&answer), stderr(&answer)) } @@ -82,7 +83,8 @@ fn signals(payload: &str) -> bool { #[test] fn a_write_to_a_governed_shell_path_signals_without_refusing() { let payload = write_payload("Write", "mise-tasks/ready-lint.sh"); - let answer = run_with_stdin(&root(), &["hook", "--harness", "exit-code"], &payload); + let answer = + run_with_stdin_at_real_root(&root(), &["hook", "--harness", "exit-code"], &payload); assert_eq!( answer.status.code(), Some(0), @@ -248,7 +250,7 @@ fn an_advised_and_denied_call_emits_only_the_refusal() { .file("policy/shell-write-advisory.rego", &module); let payload = write_payload("Write", "mise-tasks/ready-lint.sh"); - let answer = run_with_stdin( + let answer = run_with_stdin_at_real_root( bench.path(), &["hook", "--harness", "claude-code"], &payload, @@ -282,7 +284,8 @@ fn an_advised_and_denied_call_emits_only_the_refusal() { #[test] fn an_advised_and_allowed_call_still_speaks() { let payload = write_payload("Write", "mise-tasks/ready-lint.sh"); - let answer = run_with_stdin(&root(), &["hook", "--harness", "exit-code"], &payload); + let answer = + run_with_stdin_at_real_root(&root(), &["hook", "--harness", "exit-code"], &payload); let reported = format!( "{}{}", String::from_utf8_lossy(&answer.stdout),