From 720992643e3d42a16f55eef45cd35ac18365d331 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 17:33:30 +0000 Subject: [PATCH 01/28] test(infra): ENG-4687 add pre-commit config invariant test (red) Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 240 +++++++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+) create mode 100644 tests/pre_commit_config.rs diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs new file mode 100644 index 0000000..73c9a5f --- /dev/null +++ b/tests/pre_commit_config.rs @@ -0,0 +1,240 @@ +//! ENG-4687: lock the structural invariants of `.pre-commit-config.yaml`. +//! +//! Why these tests exist +//! --------------------- +//! `.pre-commit-config.yaml` is the artifact of ENG-4687. Most of its +//! behavior can only be observed by invoking the `pre-commit` tool +//! itself (rustfmt failure prints a diff, hook installation creates a +//! `.git/hooks/pre-commit` shim). Those are not in scope for `cargo +//! test`. +//! +//! What IS in scope here is the small set of invariants whose silent +//! drift would turn the gate into a no-op or pull in inappropriate +//! Python-specific hooks. Mirroring `tests/ci_workflow.rs`, we lock +//! these with string-level assertions instead of adding a YAML parser +//! dev-dependency. +//! +//! Invariants locked: +//! - File exists at the crate root. +//! - `default_stages: [pre-commit]` is present. +//! - Top-level `exclude:` covers target/, data/, vendored .gpkg, +//! and *.rkyv. +//! - `pre-commit-hooks` is pinned to v5.0.0 (matches backend's +//! pinning convention). +//! - Required baseline hook ids are present. +//! - `cargo fmt --check` hook has the four contract attributes. +//! - `cargo clippy` hook is on `stages: [manual]`. +//! - Python-specific backend hooks/repos are ABSENT. + +use std::fs; +use std::path::PathBuf; + +fn config_path() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".pre-commit-config.yaml") +} + +fn read_config() -> String { + let p = config_path(); + fs::read_to_string(&p).unwrap_or_else(|e| panic!("failed to read {}: {e}", p.display())) +} + +// --------------------------------------------------------------------------- +// File presence +// --------------------------------------------------------------------------- + +#[test] +fn pre_commit_config_exists_at_crate_root() { + let p = config_path(); + assert!( + p.exists(), + ".pre-commit-config.yaml must exist at the crate root (got: {})", + p.display() + ); +} + +// --------------------------------------------------------------------------- +// default_stages +// --------------------------------------------------------------------------- + +#[test] +fn default_stages_is_pre_commit() { + let cfg = read_config(); + assert!( + cfg.contains("default_stages: [pre-commit]"), + "`.pre-commit-config.yaml` must declare `default_stages: [pre-commit]` \ + so unmarked hooks run on commit (not on push or manual). \ + Without it, hook stage routing is implementation-defined." + ); +} + +// --------------------------------------------------------------------------- +// Top-level exclude +// --------------------------------------------------------------------------- + +#[test] +fn top_level_exclude_covers_target() { + let cfg = read_config(); + assert!( + cfg.contains("^target/"), + "top-level `exclude:` must cover `^target/` so Cargo build output is skipped" + ); +} + +#[test] +fn top_level_exclude_covers_data_dir() { + let cfg = read_config(); + assert!( + cfg.contains("^data/"), + "top-level `exclude:` must cover `^data/` so generated graph archives are skipped" + ); +} + +#[test] +fn top_level_exclude_covers_rkyv() { + let cfg = read_config(); + assert!( + cfg.contains("\\.rkyv$"), + "top-level `exclude:` must cover `\\.rkyv$` so generated rkyv archives are skipped" + ); +} + +#[test] +fn top_level_exclude_covers_vendored_gpkg() { + let cfg = read_config(); + assert!( + cfg.contains("vendor/eurostat-marnet/.*\\.gpkg$"), + "top-level `exclude:` must cover `vendor/eurostat-marnet/.*\\.gpkg$` so the \ + intentionally vendored MARNET GeoPackages are skipped — without this, \ + `check-added-large-files` and `end-of-file-fixer` would trip on them." + ); +} + +// --------------------------------------------------------------------------- +// pre-commit-hooks revision pin +// --------------------------------------------------------------------------- + +#[test] +fn pre_commit_hooks_pinned_to_v5_0_0() { + let cfg = read_config(); + assert!( + cfg.contains("repo: https://github.com/pre-commit/pre-commit-hooks"), + "must include the upstream pre-commit/pre-commit-hooks repo" + ); + assert!( + cfg.contains("rev: v5.0.0"), + "pre-commit-hooks must be pinned to `rev: v5.0.0` (not master, not a \ + later version) to match the workspace convention in spotship/backend." + ); +} + +// --------------------------------------------------------------------------- +// Baseline hook ids present +// --------------------------------------------------------------------------- + +#[test] +fn baseline_hooks_present() { + let cfg = read_config(); + for needle in [ + "id: trailing-whitespace", + "id: end-of-file-fixer", + "id: check-json", + "id: check-toml", + "id: check-yaml", + "id: check-merge-conflict", + "id: check-case-conflict", + "id: detect-private-key", + ] { + assert!( + cfg.contains(needle), + "`.pre-commit-config.yaml` must include `{needle}` from the v5.0.0 baseline" + ); + } +} + +// --------------------------------------------------------------------------- +// cargo fmt --check hook contract +// --------------------------------------------------------------------------- + +#[test] +fn cargo_fmt_check_hook_contract() { + let cfg = read_config(); + + // Hook id and entry must appear. + assert!( + cfg.contains("id: cargo-fmt-check"), + "must define a local hook with `id: cargo-fmt-check`" + ); + assert!( + cfg.contains("entry: cargo fmt --check"), + "cargo-fmt-check hook must use `entry: cargo fmt --check` — without `--check`, \ + the hook would silently reformat files instead of failing on drift." + ); + + // The four contract attributes for a `cargo fmt`-style local hook. + // We assert their presence somewhere in the file rather than by + // YAML structure to keep the test parser-free, matching + // `tests/ci_workflow.rs`'s style. + for needle in [ + "language: system", + "pass_filenames: false", + "types: [rust]", + ] { + assert!( + cfg.contains(needle), + "cargo-fmt-check hook must include `{needle}` — without it the hook \ + would either need a managed Rust toolchain (`language: rust` isn't \ + supported for `cargo fmt`), pass filenames as positional args \ + (which `cargo fmt` rejects), or fire on every commit even when no \ + Rust file is staged." + ); + } +} + +// --------------------------------------------------------------------------- +// cargo clippy hook is manual-stage +// --------------------------------------------------------------------------- + +#[test] +fn cargo_clippy_hook_is_manual_stage() { + let cfg = read_config(); + assert!( + cfg.contains("id: cargo-clippy"), + "must define a local hook with `id: cargo-clippy`" + ); + assert!( + cfg.contains("entry: cargo clippy --no-deps -- -D warnings"), + "cargo-clippy hook must use `entry: cargo clippy --no-deps -- -D warnings` — \ + matches the planning request and the manual smoke-test scope." + ); + assert!( + cfg.contains("stages: [manual]"), + "cargo-clippy hook must be on `stages: [manual]` — clippy is too slow \ + for every commit, so it is opt-in via `pre-commit run --hook-stage manual`." + ); +} + +// --------------------------------------------------------------------------- +// Python-specific backend hooks are ABSENT +// --------------------------------------------------------------------------- + +#[test] +fn python_specific_backend_hooks_are_absent() { + let cfg = read_config(); + for forbidden in [ + // Python-runtime hooks from pre-commit-hooks v5 + "id: debug-statements", + "id: check-builtin-literals", + "id: check-docstring-first", + // Python tooling repos copied wholesale from backend + "ruff-pre-commit", + "django-upgrade", + "djLint", + ] { + assert!( + !cfg.contains(forbidden), + "`.pre-commit-config.yaml` must NOT include `{forbidden}` — this is a \ + Python-specific hook/repo from backend's config and does not apply to \ + a Rust crate. Catches accidental wholesale copy of backend config." + ); + } +} From a74e6e6370759de642eda978c8c5ef9dc323dab5 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 17:34:13 +0000 Subject: [PATCH 02/28] feat(infra): ENG-4687 add pre-commit config (cargo fmt --check + hygiene) Signed-off-by: Jimbo Freedman --- .pre-commit-config.yaml | 58 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 .pre-commit-config.yaml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..6c86341 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,58 @@ +# ENG-4687: local pre-commit gate for rustyroute. +# +# Mirrors the framework patterns from spotship/backend's config +# (top-level exclude, default_stages, rev: v5.0.0) and +# spotship/swiftsure's non-Python hygiene baseline. Substitutes +# `cargo fmt --check` for Python-specific linting. Clippy is opt-in +# via --hook-stage manual because it is too slow for every commit. CI +# (.github/workflows/ci.yaml) is the network-side enforcement; this +# file is the laptop-side early signal. +# +# Locked by tests/pre_commit_config.rs — see that file for the +# invariants asserted against silent drift. + +exclude: | + (?x)( + ^target/| + ^data/| + ^vendor/eurostat-marnet/.*\.gpkg$| + \.rkyv$ + ) +default_stages: [pre-commit] + +repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v5.0.0 + hooks: + - id: trailing-whitespace + args: [--markdown-linebreak-ext=md] + - id: end-of-file-fixer + - id: check-json + - id: check-toml + - id: check-yaml + args: ['--unsafe'] + - id: check-merge-conflict + - id: check-case-conflict + - id: check-added-large-files + - id: check-executables-have-shebangs + - id: check-vcs-permalinks + - id: forbid-new-submodules + - id: mixed-line-ending + args: [--fix=lf] + - id: detect-private-key + + - repo: local + hooks: + - id: cargo-fmt-check + name: cargo fmt --check + entry: cargo fmt --check + language: system + pass_filenames: false + types: [rust] + - id: cargo-clippy + name: cargo clippy --no-deps + entry: cargo clippy --no-deps -- -D warnings + language: system + pass_filenames: false + always_run: true + stages: [manual] From d7cb5920c6e6053203253ff689fc84b47604da97 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 17:34:36 +0000 Subject: [PATCH 03/28] docs: ENG-4687 document pre-commit install in CONTRIBUTING.md Signed-off-by: Jimbo Freedman --- CONTRIBUTING.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4babd34..35f68e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -71,6 +71,25 @@ See for the full text of the DCO. Before opening a pull request, run: +First-time setup — install the [pre-commit](https://pre-commit.com) +hook so format and file-hygiene checks run on every `git commit`: + +```sh +pre-commit install +``` + +To run the managed hooks explicitly (e.g. before pushing): + +```sh +pre-commit run --all-files +pre-commit run --hook-stage manual # runs cargo clippy --no-deps +``` + +The pre-commit gate runs `cargo fmt --check` plus lightweight file +hygiene (trailing whitespace, EOF newlines, YAML/TOML/JSON syntax, +merge-conflict markers, private keys). Clippy and the rest of the +full local validation suite remain manual: + ```sh cargo fmt --check cargo check From 30543a701c104692b877825f8230f445d042097b Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 17:35:07 +0000 Subject: [PATCH 04/28] style: ENG-4687 rustfmt pre_commit_config test (collapse array literal) Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 73c9a5f..a7a4941 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -174,11 +174,7 @@ fn cargo_fmt_check_hook_contract() { // We assert their presence somewhere in the file rather than by // YAML structure to keep the test parser-free, matching // `tests/ci_workflow.rs`'s style. - for needle in [ - "language: system", - "pass_filenames: false", - "types: [rust]", - ] { + for needle in ["language: system", "pass_filenames: false", "types: [rust]"] { assert!( cfg.contains(needle), "cargo-fmt-check hook must include `{needle}` — without it the hook \ From 4aa9bd6368c7d66bbcf07ec865f847351b320cfc Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 17:48:57 +0000 Subject: [PATCH 05/28] fix(infra): ENG-4687 extend cargo-fmt-check hook to cover downstream_consumer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests/downstream_consumer/ sub-package is its own [package] (not a workspace member), so `cargo fmt --check` from the root does not traverse into it. Without an explicit --manifest-path invocation the pre-commit gate would silently miss fmt drift in that crate — failing AC3 ("committing a file with bad formatting fails the hook") for any edit under tests/downstream_consumer/. Extend the cargo-fmt-check hook entry to run both invocations: cargo fmt --check && \ cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check Lock the new invariant in tests/pre_commit_config.rs and one-time-format the existing drift in tests/downstream_consumer/src/lib.rs. Found by peer review (Codex) on the initial implementation. Signed-off-by: Jimbo Freedman --- .pre-commit-config.yaml | 9 +++++++-- tests/downstream_consumer/src/lib.rs | 4 ++-- tests/pre_commit_config.rs | 21 +++++++++++++++++++-- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6c86341..2e6be94 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -44,8 +44,13 @@ repos: - repo: local hooks: - id: cargo-fmt-check - name: cargo fmt --check - entry: cargo fmt --check + # Runs `cargo fmt --check` for the root crate AND for the nested + # downstream_consumer sub-package (its own [package], not a + # workspace member — so the root `cargo fmt --check` does not + # traverse into it). Both manifests are checked together so a + # contributor cannot silently land fmt drift in either crate. + name: cargo fmt --check (root + downstream_consumer) + entry: bash -c 'cargo fmt --check && cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check' language: system pass_filenames: false types: [rust] diff --git a/tests/downstream_consumer/src/lib.rs b/tests/downstream_consumer/src/lib.rs index a22fe67..9e52ad5 100644 --- a/tests/downstream_consumer/src/lib.rs +++ b/tests/downstream_consumer/src/lib.rs @@ -12,8 +12,8 @@ pub fn exercise_public_api() -> (u32, u32, u32) { assert_eq!(loaded.resolution_km(), 50); // Path 2: Graph::from_bytes via the static BYTES_50KM const. - let static_g = Graph::from_bytes(rustyroute::data::BYTES_50KM) - .expect("Graph::from_bytes(BYTES_50KM)"); + let static_g = + Graph::from_bytes(rustyroute::data::BYTES_50KM).expect("Graph::from_bytes(BYTES_50KM)"); // Sanity: both observations agree on shape. assert_eq!(loaded.node_count(), static_g.node_count()); diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index a7a4941..b62171e 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -164,11 +164,28 @@ fn cargo_fmt_check_hook_contract() { cfg.contains("id: cargo-fmt-check"), "must define a local hook with `id: cargo-fmt-check`" ); + // The entry must run `cargo fmt --check` (with `--check`, not a + // bare `cargo fmt` that would silently reformat files instead of + // failing on drift). We assert the substring rather than the + // literal full entry because the hook also runs `cargo fmt + // --manifest-path tests/downstream_consumer/Cargo.toml --check` + // (a non-workspace sub-package — see below). assert!( - cfg.contains("entry: cargo fmt --check"), - "cargo-fmt-check hook must use `entry: cargo fmt --check` — without `--check`, \ + cfg.contains("cargo fmt --check"), + "cargo-fmt-check hook entry must include `cargo fmt --check` — without `--check`, \ the hook would silently reformat files instead of failing on drift." ); + // The nested `tests/downstream_consumer/` package is its own + // `[package]` (not a workspace member), so the root `cargo fmt + // --check` does NOT traverse into it. The hook must also format + // that sub-package explicitly — otherwise AC3 (commit with bad + // formatting fails) silently misses the sub-package. + assert!( + cfg.contains("--manifest-path tests/downstream_consumer/Cargo.toml --check"), + "cargo-fmt-check hook must also run `cargo fmt --manifest-path \ + tests/downstream_consumer/Cargo.toml --check` — the nested downstream_consumer \ + package is not a workspace member, so the root invocation does not cover it." + ); // The four contract attributes for a `cargo fmt`-style local hook. // We assert their presence somewhere in the file rather than by From 88fabe5a6d3534a67895acf87262f9ee04a6f08f Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:04:24 +0000 Subject: [PATCH 06/28] test(infra): ENG-4687 add pre-commit e2e tests (AC2-AC4) Shells out to the real `pre-commit` binary and asserts the runtime acceptance criteria that the dev-phase string-assertion test in `tests/pre_commit_config.rs` cannot cover: - AC2 `pre-commit run --all-files` exits 0 on the worktree - AC3 staging a fmt-bad `.rs` file fails `cargo-fmt-check` with a rustfmt diff - AC4 `pre-commit run --hook-stage manual cargo-clippy` invokes clippy - AC4 corollary: clippy is filtered out on the default stage (proves `stages: [manual]` is doing real work) Tests are skip-clean when pre-commit/git are absent (Rust-only contributors), so `cargo test` stays runnable without `pip install pre-commit`. CI exercises the suite for real because pre-commit is installed in the workflow. AC3/AC4 isolate against a throwaway git repo built in `tempfile::TempDir` with the real `.pre-commit-config.yaml` copied in; AC2 runs against the actual tree and restores any auto-fixer modifications it triggers. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 429 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 429 insertions(+) create mode 100644 tests/pre_commit_e2e.rs diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs new file mode 100644 index 0000000..491aa8d --- /dev/null +++ b/tests/pre_commit_e2e.rs @@ -0,0 +1,429 @@ +//! ENG-4687 end-to-end tests: shell out to the real `pre-commit` tool +//! and assert against the user-observable behaviour of the hooks. +//! +//! Scope vs. `tests/pre_commit_config.rs` +//! -------------------------------------- +//! `pre_commit_config.rs` locks the YAML's *invariants* via string +//! assertions (cheap, no external dependencies, runs on every `cargo +//! test`). That file proves AC5 ("invariants locked against silent +//! drift"). +//! +//! The tests in this file cover the *runtime* acceptance criteria that +//! require actually executing the hooks: +//! +//! - AC2: `pre-commit run --all-files` passes on the current tree. +//! - AC3: Committing a fmt-bad `.rs` file fails the +//! `cargo-fmt-check` hook with a non-zero exit and a diff. +//! - AC4: `pre-commit run --hook-stage manual cargo-clippy` runs +//! clippy. +//! +//! Skip semantics +//! -------------- +//! These tests shell out to the system `pre-commit` binary. If it is +//! not on `PATH` (e.g. a Rust-only contributor without Python), the +//! test prints a skip message and returns `Ok(())` rather than failing +//! — matches the spec's "AC1 manual" framing where pre-commit install +//! is a per-clone setup, not a cargo-test prereq. CI (which installs +//! pre-commit explicitly) will exercise these tests for real. +//! +//! Isolation +//! --------- +//! AC3 mutates a worktree and creates a commit. To avoid touching the +//! checked-out rustyroute repo, the test materialises a throwaway git +//! repo in `std::env::temp_dir()` with the *real* +//! `.pre-commit-config.yaml` symlinked/copied in, plus the minimum +//! Cargo scaffolding so `cargo fmt --check` has something to check. +//! The test cleans up its tempdir on success and on panic via +//! `tempfile::TempDir`. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/// Returns the repo root (the directory containing this crate's +/// `Cargo.toml`). +fn repo_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +/// Returns `Some(path)` if `pre-commit` is on PATH, else `None`. +fn pre_commit_on_path() -> Option { + // Use `which`-equivalent logic by trying `pre-commit --version`. + let out = Command::new("pre-commit").arg("--version").output().ok()?; + if out.status.success() { + // The exact path doesn't matter; just signal "available". + Some(PathBuf::from("pre-commit")) + } else { + None + } +} + +fn git_on_path() -> bool { + Command::new("git") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +fn cargo_bin() -> String { + std::env::var("CARGO").unwrap_or_else(|_| "cargo".into()) +} + +/// If pre-commit or git is missing, print why and return false. +/// Callers should early-return when this is false. +fn prereqs_available(test_name: &str) -> bool { + if pre_commit_on_path().is_none() { + eprintln!( + "skipping {test_name}: pre-commit not found on PATH \ + (install with `pip install pre-commit`)" + ); + return false; + } + if !git_on_path() { + eprintln!("skipping {test_name}: git not found on PATH"); + return false; + } + true +} + +/// Run a command, capture stdout+stderr, return (status, combined output). +fn run(cmd: &mut Command) -> (std::process::ExitStatus, String) { + let out = cmd.output().expect("spawn command"); + let mut combined = String::new(); + combined.push_str(&String::from_utf8_lossy(&out.stdout)); + combined.push_str(&String::from_utf8_lossy(&out.stderr)); + (out.status, combined) +} + +/// Build a throwaway git repo in `dir` with: +/// - the real `.pre-commit-config.yaml` from this repo +/// - a minimal `Cargo.toml` so `cargo fmt --check` has something to do +/// - the `tests/downstream_consumer/` sub-package layout (because the +/// hook entry runs `cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check` +/// — if the manifest is missing the hook fails for an unrelated reason) +/// - an initial commit so subsequent `git commit` invocations work +/// +/// Returns the path to the seeded repo root. +fn seed_tmp_repo(dir: &Path) -> PathBuf { + let repo = dir.to_path_buf(); + + // git init + identity (pre-commit requires a git repo; commit needs author). + let (st, out) = run(Command::new("git").arg("init").arg("-q").arg(&repo)); + assert!(st.success(), "git init failed: {out}"); + for (k, v) in [ + ("user.email", "e2e@rustyroute.test"), + ("user.name", "E2E Test"), + ("commit.gpgsign", "false"), + ] { + let (st, out) = run(Command::new("git") + .current_dir(&repo) + .args(["config", k, v])); + assert!(st.success(), "git config {k} failed: {out}"); + } + + // Copy the real .pre-commit-config.yaml. + let cfg_src = repo_root().join(".pre-commit-config.yaml"); + let cfg_dst = repo.join(".pre-commit-config.yaml"); + fs::copy(&cfg_src, &cfg_dst).expect("copy .pre-commit-config.yaml"); + + // Minimal root Cargo.toml + src/lib.rs (formatted). + fs::write( + repo.join("Cargo.toml"), + "[package]\n\ + name = \"e2e_root\"\n\ + version = \"0.0.0\"\n\ + edition = \"2024\"\n", + ) + .unwrap(); + fs::create_dir_all(repo.join("src")).unwrap(); + fs::write( + repo.join("src/lib.rs"), + "pub fn hello() -> u32 {\n 42\n}\n", + ) + .unwrap(); + + // Stub the downstream_consumer sub-package because the hook entry + // references `tests/downstream_consumer/Cargo.toml` explicitly. + let sub = repo.join("tests/downstream_consumer"); + fs::create_dir_all(sub.join("src")).unwrap(); + fs::write( + sub.join("Cargo.toml"), + "[package]\n\ + name = \"e2e_sub\"\n\ + version = \"0.0.0\"\n\ + edition = \"2024\"\n", + ) + .unwrap(); + fs::write(sub.join("src/lib.rs"), "pub fn ping() -> u32 {\n 1\n}\n").unwrap(); + + // Initial commit so future commits have a parent. + let (st, out) = run(Command::new("git").current_dir(&repo).args(["add", "-A"])); + assert!(st.success(), "git add failed: {out}"); + let (st, out) = run(Command::new("git").current_dir(&repo).args([ + "commit", + "-q", + "--no-verify", + "-m", + "seed", + ])); + assert!(st.success(), "git commit (seed) failed: {out}"); + + repo +} + +// --------------------------------------------------------------------------- +// AC2: pre-commit run --all-files passes on a clean rustyroute tree +// --------------------------------------------------------------------------- +// +// This runs against the ACTUAL repo root (not a tempdir) — that is the +// point of AC2: the rustyroute working tree should be hook-clean. +// +// If the suite fails, the test prints pre-commit's stdout/stderr so the +// failing hook id is visible in CI logs. We do NOT mutate the working +// tree — pre-commit's auto-fixers (e.g. end-of-file-fixer) can write +// files, but we leave restoration to the caller / git. The test +// re-snapshots the working tree before and after via `git diff +// --quiet`; if pre-commit modified files, that itself counts as a fail. + +#[test] +fn ac2_pre_commit_run_all_files_passes_on_current_tree() { + if !prereqs_available("ac2_pre_commit_run_all_files_passes_on_current_tree") { + return; + } + let root = repo_root(); + + // Snapshot working-tree state before running. + let (pre_st, _) = run(Command::new("git") + .current_dir(&root) + .args(["diff", "--quiet"])); + let pre_clean = pre_st.success(); + + let (st, output) = run(Command::new("pre-commit").current_dir(&root).args([ + "run", + "--all-files", + "--color", + "never", + ])); + + // If a hook is an auto-fixer (end-of-file-fixer, trailing-whitespace, + // mixed-line-ending) it will both modify files AND return non-zero. + // Restore any modifications so we don't leave the worktree dirty — + // but only if it was clean BEFORE we ran. Otherwise we'd clobber + // user changes. + if pre_clean { + let (post_st, _) = run(Command::new("git") + .current_dir(&root) + .args(["diff", "--quiet"])); + if !post_st.success() { + let _ = run(Command::new("git") + .current_dir(&root) + .args(["checkout", "--", "."])); + } + } + + assert!( + st.success(), + "AC2 violated: `pre-commit run --all-files` failed on the current tree.\n\ + A failing hook means a contributor running `git commit` will be blocked\n\ + until the offending file is fixed. The acceptance criterion requires the\n\ + main-equivalent tree to be hook-clean.\n\n\ + pre-commit output:\n{output}" + ); +} + +// --------------------------------------------------------------------------- +// AC3: cargo-fmt-check fails on staged-but-misformatted Rust +// --------------------------------------------------------------------------- +// +// Materialise a throwaway repo, introduce a mis-indented `.rs` file, +// stage it, and run `pre-commit run cargo-fmt-check`. We expect a +// non-zero exit AND output that contains the substring `Diff` (rustfmt +// prints `Diff in ` when `--check` finds drift). We do NOT +// require any specific diff text — only that the hook flags the drift. + +#[test] +fn ac3_cargo_fmt_check_fails_on_misformatted_rust() { + if !prereqs_available("ac3_cargo_fmt_check_fails_on_misformatted_rust") { + return; + } + + let tmp = tempfile::tempdir().expect("create tempdir"); + let repo = seed_tmp_repo(tmp.path()); + + // Write a misformatted Rust file (double space, weird indentation) + // — rustfmt will refuse this with `cargo fmt --check`. + fs::write(repo.join("src/lib.rs"), "pub fn bad( ) ->u32{42 }\n").unwrap(); + let (st, out) = run(Command::new("git") + .current_dir(&repo) + .args(["add", "src/lib.rs"])); + assert!(st.success(), "stage bad file failed: {out}"); + + // Pre-commit needs CARGO/rustfmt on PATH. The hook uses + // `language: system`, so we just need the real cargo. + let cargo = cargo_bin(); + let path = std::env::var("PATH").unwrap_or_default(); + let cargo_dir = Path::new(&cargo) + .parent() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_default(); + let augmented_path = if cargo_dir.is_empty() { + path.clone() + } else { + format!("{cargo_dir}:{path}") + }; + + let (st, output) = run(Command::new("pre-commit") + .current_dir(&repo) + .env("PATH", &augmented_path) + .args(["run", "cargo-fmt-check", "--color", "never"])); + + assert!( + !st.success(), + "AC3 violated: cargo-fmt-check passed on misformatted code.\n\ + The hook should reject `pub fn bad( ) ->u32{{42 }}` because rustfmt's \n\ + `--check` mode exits non-zero when reformatting would change the file.\n\n\ + pre-commit output:\n{output}" + ); + + // Rustfmt's --check output contains `Diff in` lines. Verify the + // contributor actually sees a diff (not a generic error), so the + // failure is self-explanatory. + assert!( + output.contains("Diff in") || output.contains("cargo fmt"), + "AC3 partial: cargo-fmt-check failed but output does not look like rustfmt's \ + diff. Expected `Diff in` (rustfmt --check signature) or at least `cargo fmt` \ + in the output so contributors know which hook fired.\n\n\ + actual output:\n{output}" + ); +} + +// --------------------------------------------------------------------------- +// AC4: pre-commit run --hook-stage manual cargo-clippy invokes clippy +// --------------------------------------------------------------------------- +// +// Manual-stage hooks only run when invoked explicitly. We assert that +// `pre-commit run --hook-stage manual cargo-clippy` reaches the +// clippy binary at all — its pass/fail is not the point here (clippy +// on a 5-line tempdir crate is uninteresting). We confirm "clippy was +// invoked" by looking for clippy's banner in the output, which always +// includes `Checking e2e_root v0.0.0` (cargo) and either `clippy` in +// the entry name or clippy's own diagnostics. + +#[test] +fn ac4_manual_stage_invokes_clippy() { + if !prereqs_available("ac4_manual_stage_invokes_clippy") { + return; + } + + let tmp = tempfile::tempdir().expect("create tempdir"); + let repo = seed_tmp_repo(tmp.path()); + + // We need cargo + clippy-driver on PATH inside the pre-commit + // subprocess. + let cargo = cargo_bin(); + let path = std::env::var("PATH").unwrap_or_default(); + let cargo_dir = Path::new(&cargo) + .parent() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_default(); + let augmented_path = if cargo_dir.is_empty() { + path.clone() + } else { + format!("{cargo_dir}:{path}") + }; + + let (st, output) = run(Command::new("pre-commit") + .current_dir(&repo) + .env("PATH", &augmented_path) + .args([ + "run", + "--hook-stage", + "manual", + "cargo-clippy", + "--color", + "never", + ])); + + // The hook should be RECOGNISED — pre-commit's "no hook with this + // id" message exits non-zero with a clear diagnostic; we don't + // want that. + assert!( + !output.contains("No hook with id `cargo-clippy`"), + "AC4 violated: pre-commit did not recognise the cargo-clippy hook id at the \ + manual stage. The hook config may be missing `stages: [manual]` or have a \ + typo in the id.\n\n\ + pre-commit output:\n{output}" + ); + + // The hook should actually invoke cargo+clippy. cargo prints + // `Checking v` when it starts work, and clippy + // prepends `clippy` in its diagnostics or banner. Either is + // sufficient evidence of invocation. + let invoked = output.contains("Checking ") + || output.contains("clippy") + || output.contains("cargo clippy --no-deps"); + assert!( + invoked, + "AC4 partial: cargo-clippy hook ran at manual stage but output does not \ + show clippy actually executing. Expected `Checking ...` (cargo banner) or \ + `clippy` in the output.\n\n\ + hook exit: {st:?}\n\ + pre-commit output:\n{output}" + ); +} + +// --------------------------------------------------------------------------- +// AC4 corollary: cargo-clippy does NOT run on the default pre-commit stage +// --------------------------------------------------------------------------- +// +// The spec is explicit: clippy is opt-in. If it crept onto the default +// stage, `pre-commit run --all-files` would invoke it (slow). Verify +// the negative. + +#[test] +fn ac4_clippy_does_not_run_on_default_stage() { + if !prereqs_available("ac4_clippy_does_not_run_on_default_stage") { + return; + } + + let tmp = tempfile::tempdir().expect("create tempdir"); + let repo = seed_tmp_repo(tmp.path()); + + let cargo = cargo_bin(); + let path = std::env::var("PATH").unwrap_or_default(); + let cargo_dir = Path::new(&cargo) + .parent() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_default(); + let augmented_path = if cargo_dir.is_empty() { + path.clone() + } else { + format!("{cargo_dir}:{path}") + }; + + // Ask pre-commit to run *just* cargo-clippy on the default stage. + // It should refuse (the hook is configured for manual only). + let (_st, output) = run(Command::new("pre-commit") + .current_dir(&repo) + .env("PATH", &augmented_path) + .args(["run", "cargo-clippy", "--all-files", "--color", "never"])); + + // pre-commit's exact message when a hook is filtered out by stage + // is "No hook with id `cargo-clippy` in stage `pre-commit`" (or + // similar wording across versions). Either that phrase or the + // absence of the cargo-clippy invocation banner is acceptable. + let filtered_out = output.contains("No hook with id `cargo-clippy`") + || !(output.contains("Checking ") || output.contains("cargo clippy --no-deps")); + assert!( + filtered_out, + "AC4 corollary violated: cargo-clippy executed on the DEFAULT pre-commit stage. \ + It must be opt-in via `--hook-stage manual` (clippy is too slow for every \ + commit). Check that the hook keeps `stages: [manual]`.\n\n\ + pre-commit output:\n{output}" + ); +} From d154555adca5239cd4a2acbb7f83b36068905ab9 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:09:18 +0000 Subject: [PATCH 07/28] fix(docs): ENG-4687 strip extra trailing newline from CODE_OF_CONDUCT.md end-of-file-fixer requires a single trailing LF; the file shipped with two from the initial repo skeleton, causing `pre-commit run --all-files` to fail (ac2 e2e test). Signed-off-by: Jimbo Freedman --- CODE_OF_CONDUCT.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md index ddf22ed..31961c0 100644 --- a/CODE_OF_CONDUCT.md +++ b/CODE_OF_CONDUCT.md @@ -81,4 +81,3 @@ For answers to common questions about this code of conduct, see the FAQ at [http [Mozilla CoC]: https://github.com/mozilla/diversity [FAQ]: https://www.contributor-covenant.org/faq [translations]: https://www.contributor-covenant.org/translations - From 45a81e1d191a006775970392f2c2fa5f79e58b01 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:28:53 +0000 Subject: [PATCH 08/28] test(infra): ENG-4687 tighten AC3 fallback to match `rustfmt` not `cargo fmt` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AC3 fallback assertion in `tests/pre_commit_e2e.rs` accepted either `Diff in` (the rustfmt --check signature) OR `cargo fmt` in pre-commit output. The second alternative was too broad: pre-commit's own hook banner prints "cargo fmt --check", so the assertion would pass even if rustfmt stopped emitting diff output and the hook failed for some other reason — degrading the contributor signal to "the hook failed but I don't see why". Narrow the fallback to `rustfmt` so the assertion only succeeds when the output genuinely looks like rustfmt diagnostics, not just the hook chrome. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 491aa8d..5976b56 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -294,9 +294,9 @@ fn ac3_cargo_fmt_check_fails_on_misformatted_rust() { // contributor actually sees a diff (not a generic error), so the // failure is self-explanatory. assert!( - output.contains("Diff in") || output.contains("cargo fmt"), + output.contains("Diff in") || output.contains("rustfmt"), "AC3 partial: cargo-fmt-check failed but output does not look like rustfmt's \ - diff. Expected `Diff in` (rustfmt --check signature) or at least `cargo fmt` \ + diff. Expected `Diff in` (rustfmt --check signature) or at least `rustfmt` \ in the output so contributors know which hook fired.\n\n\ actual output:\n{output}" ); From e7235af981b850eb7d9e4bfee21567701fee6f36 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:29:03 +0000 Subject: [PATCH 09/28] docs: ENG-4687 reposition "Before opening a PR, run:" in CONTRIBUTING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Before opening a pull request, run:" sentence used to lead directly into the `cargo fmt --check / cargo check / ...` block. After the pre-commit stanza was inserted above that block, the sentence read as a lead-in to an install instruction, not a command list — disjointed UX. Drop the stranded sentence from the section opener and move it down to immediately precede the actual command block. No content changes, just reordering so the sentence introduces the commands it's describing. Signed-off-by: Jimbo Freedman --- CONTRIBUTING.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 35f68e2..447f4b5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,8 +69,6 @@ See for the full text of the DCO. ## Local validation -Before opening a pull request, run: - First-time setup — install the [pre-commit](https://pre-commit.com) hook so format and file-hygiene checks run on every `git commit`: @@ -88,7 +86,8 @@ pre-commit run --hook-stage manual # runs cargo clippy --no-deps The pre-commit gate runs `cargo fmt --check` plus lightweight file hygiene (trailing whitespace, EOF newlines, YAML/TOML/JSON syntax, merge-conflict markers, private keys). Clippy and the rest of the -full local validation suite remain manual: +full local validation suite remain manual. Before opening a pull +request, run: ```sh cargo fmt --check From 2ab9146166c44026c9841a7387811e8d95fef18f Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:29:22 +0000 Subject: [PATCH 10/28] ci: ENG-4687 add pre-commit job so AC2/AC3/AC4 are machine-verified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e2e tests in `tests/pre_commit_e2e.rs` shell out to the system `pre-commit` binary and silently skip when it is not on PATH. The existing CI workflow did not install pre-commit anywhere, so every CI run was returning success without exercising a single hook — AC2 ("pre-commit run --all-files passes"), AC3 (fmt-bad commit fails the hook), and AC4 (manual-stage clippy) were effectively self-attested. Add a dedicated `pre-commit` job that: - installs the stable Rust toolchain with rustfmt + clippy components - sets up Python 3.x and `pip install pre-commit` - runs `pre-commit run --all-files --show-diff-on-failure` (AC2 direct) - runs `cargo test --test pre_commit_e2e` (AC2/AC3/AC4 via harness) Also update the module doc-comment in `tests/pre_commit_e2e.rs` to match reality — the previous wording falsely claimed CI exercised the tests; it now points at the new pre-commit job and is explicit that other CI jobs intentionally skip. Signed-off-by: Jimbo Freedman --- .github/workflows/ci.yaml | 23 +++++++++++++++++++++++ tests/pre_commit_e2e.rs | 8 ++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b880725..b2a1acd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -125,3 +125,26 @@ jobs: with: shared-key: docs - run: cargo doc --no-deps --all-features + + pre-commit: + name: pre-commit + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + - uses: actions/setup-python@v5 + with: + python-version: '3.x' + - run: pip install pre-commit + - uses: Swatinem/rust-cache@v2 + with: + shared-key: pre-commit + # Exercise the hooks directly (AC2) plus the e2e harness that + # also covers AC3 (fmt-bad commit) and AC4 (manual-stage clippy). + # Without pre-commit on PATH, tests/pre_commit_e2e.rs silently + # skips — installing it here is what makes AC2/AC3/AC4 actually + # machine-verified rather than self-attested. + - run: pre-commit run --all-files --show-diff-on-failure + - run: cargo test --test pre_commit_e2e diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 5976b56..033187a 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -23,8 +23,12 @@ //! not on `PATH` (e.g. a Rust-only contributor without Python), the //! test prints a skip message and returns `Ok(())` rather than failing //! — matches the spec's "AC1 manual" framing where pre-commit install -//! is a per-clone setup, not a cargo-test prereq. CI (which installs -//! pre-commit explicitly) will exercise these tests for real. +//! is a per-clone setup, not a cargo-test prereq. The CI workflow's +//! dedicated `pre-commit` job (`.github/workflows/ci.yaml`) installs +//! `pre-commit` via `pip` and then runs `cargo test --test +//! pre_commit_e2e` so AC2/AC3/AC4 are exercised for real on every PR; +//! the other CI jobs (fmt, clippy, test-matrix, ...) do NOT install +//! pre-commit, so this file silently skips there by design. //! //! Isolation //! --------- From 6534022127c47ff606b470a87ba46530c461e263 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:41:27 +0000 Subject: [PATCH 11/28] test(infra): ENG-4687 tighten AC4 clippy-invoked probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the bare `output.contains("clippy")` alternative from the ac4_manual_stage_invokes_clippy probe. That substring matches pre-commit's hook-name banner, which is printed regardless of whether the hook entry actually executed — making the probe firable on the banner alone. Keep `Checking ` (cargo's compile banner) and the literal entry `cargo clippy --no-deps` as the two valid evidence substrings. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 033187a..d3d7ce9 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -365,12 +365,13 @@ fn ac4_manual_stage_invokes_clippy() { ); // The hook should actually invoke cargo+clippy. cargo prints - // `Checking v` when it starts work, and clippy - // prepends `clippy` in its diagnostics or banner. Either is - // sufficient evidence of invocation. - let invoked = output.contains("Checking ") - || output.contains("clippy") - || output.contains("cargo clippy --no-deps"); + // `Checking v` when it starts work, and the + // literal entry `cargo clippy --no-deps` appears in pre-commit's + // command echo. Either is sufficient evidence of invocation. + // (We deliberately don't match the bare substring `clippy` — that + // also appears in pre-commit's hook-name banner, which is printed + // regardless of whether the entry actually executed.) + let invoked = output.contains("Checking ") || output.contains("cargo clippy --no-deps"); assert!( invoked, "AC4 partial: cargo-clippy hook ran at manual stage but output does not \ From 099b8808bfacc4a892c8e6b80d6ca8638d60bb4c Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:41:46 +0000 Subject: [PATCH 12/28] test(infra): ENG-4687 anchor AC4 negative check on seeded crate banner Replace the substring `cargo clippy --no-deps` in the ac4_clippy_does_not_run_on_default_stage filter with `Checking e2e_root`. The previous substring overlaps the hook NAME in .pre-commit-config.yaml, which can appear in pre-commit's output regardless of whether clippy actually launched. `Checking e2e_root` is cargo's compile banner for the seeded test crate and only appears when clippy genuinely runs against it, making the negative assertion robust against pre-commit upgrades. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index d3d7ce9..b838b99 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -422,8 +422,15 @@ fn ac4_clippy_does_not_run_on_default_stage() { // is "No hook with id `cargo-clippy` in stage `pre-commit`" (or // similar wording across versions). Either that phrase or the // absence of the cargo-clippy invocation banner is acceptable. + // Anchor the negative check on cargo's compile banner for the + // seeded crate (`Checking e2e_root`). The hook NAME `cargo clippy + // --no-deps` from .pre-commit-config.yaml can appear in pre-commit's + // banner output regardless of whether the entry actually launched, + // so matching on it here would risk a future-pre-commit-version + // flip. The `Checking e2e_root` banner only appears if clippy + // actually ran against the seeded crate. let filtered_out = output.contains("No hook with id `cargo-clippy`") - || !(output.contains("Checking ") || output.contains("cargo clippy --no-deps")); + || !output.contains("Checking e2e_root"); assert!( filtered_out, "AC4 corollary violated: cargo-clippy executed on the DEFAULT pre-commit stage. \ From 95404cb12d8d9f012d8087ad537e9076c773cf20 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:41:54 +0000 Subject: [PATCH 13/28] test(infra): ENG-4687 make python-hook absence check case-insensitive Lowercase the .pre-commit-config.yaml contents before substring matching in python_specific_backend_hooks_are_absent. The previous test forbade the exact substring `djLint` (upstream repo casing), which a future contributor using the lowercase hook id `djlint` would silently bypass. Lowercasing the haystack and forbidding `djlint` catches both casings with no enumeration. Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index b62171e..facc6de 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -232,7 +232,11 @@ fn cargo_clippy_hook_is_manual_stage() { #[test] fn python_specific_backend_hooks_are_absent() { - let cfg = read_config(); + // Lowercase the config once so the forbidden-substring check + // is case-insensitive. This catches both `djLint` (the upstream + // repo casing) and `djlint` (the lowercase hook id) without + // needing to enumerate every casing variant. + let cfg = read_config().to_lowercase(); for forbidden in [ // Python-runtime hooks from pre-commit-hooks v5 "id: debug-statements", @@ -241,13 +245,13 @@ fn python_specific_backend_hooks_are_absent() { // Python tooling repos copied wholesale from backend "ruff-pre-commit", "django-upgrade", - "djLint", + "djlint", ] { assert!( !cfg.contains(forbidden), - "`.pre-commit-config.yaml` must NOT include `{forbidden}` — this is a \ - Python-specific hook/repo from backend's config and does not apply to \ - a Rust crate. Catches accidental wholesale copy of backend config." + "`.pre-commit-config.yaml` must NOT include `{forbidden}` (case-insensitive) \ + — this is a Python-specific hook/repo from backend's config and does not \ + apply to a Rust crate. Catches accidental wholesale copy of backend config." ); } } From 477f44bfeca2f69734663fb200373f3e432dcd2e Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:44:22 +0000 Subject: [PATCH 14/28] style: ENG-4687 rustfmt pre_commit_e2e (collapse multi-line let binding) Pre-existing drift introduced when tightening the AC3 fallback assertion; cargo fmt --check (the form pre-commit runs) flagged it. Without this fix, the new AC2 e2e test fails against the worktree itself. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index b838b99..316179e 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -429,8 +429,8 @@ fn ac4_clippy_does_not_run_on_default_stage() { // so matching on it here would risk a future-pre-commit-version // flip. The `Checking e2e_root` banner only appears if clippy // actually ran against the seeded crate. - let filtered_out = output.contains("No hook with id `cargo-clippy`") - || !output.contains("Checking e2e_root"); + let filtered_out = + output.contains("No hook with id `cargo-clippy`") || !output.contains("Checking e2e_root"); assert!( filtered_out, "AC4 corollary violated: cargo-clippy executed on the DEFAULT pre-commit stage. \ From 9de6ae10c7382215954a16769f85a49a2512eb52 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:58:58 +0000 Subject: [PATCH 15/28] fix(infra): ENG-4687 split cargo-fmt-check hook for cross-platform support The chained `bash -c '... && ...'` entry required bash on PATH, breaking the Windows runners in the CI test matrix. Splitting into two separate local hooks (`cargo-fmt-check` for the root crate, `cargo-fmt-check-downstream` for the nested sub-package) keeps coverage identical without needing a shell. Invariant test updated to assert both hooks exist and neither uses a shell entry. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- .pre-commit-config.yaml | 23 +++++++++++------ tests/pre_commit_config.rs | 51 +++++++++++++++++++++++++------------- 2 files changed, 50 insertions(+), 24 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2e6be94..577c252 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -44,13 +44,22 @@ repos: - repo: local hooks: - id: cargo-fmt-check - # Runs `cargo fmt --check` for the root crate AND for the nested - # downstream_consumer sub-package (its own [package], not a - # workspace member — so the root `cargo fmt --check` does not - # traverse into it). Both manifests are checked together so a - # contributor cannot silently land fmt drift in either crate. - name: cargo fmt --check (root + downstream_consumer) - entry: bash -c 'cargo fmt --check && cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check' + # Checks the root crate. A second hook + # (`cargo-fmt-check-downstream`) covers the nested + # `tests/downstream_consumer/` sub-package, which is its own + # `[package]` (not a workspace member) — so the root + # `cargo fmt --check` does NOT traverse into it. The two + # invocations live in separate hooks rather than chaining a + # single shell command, so neither entry depends on a shell + # being available — relevant on Windows CI runners. + name: cargo fmt --check (root) + entry: cargo fmt --check + language: system + pass_filenames: false + types: [rust] + - id: cargo-fmt-check-downstream + name: cargo fmt --check (downstream_consumer) + entry: cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check language: system pass_filenames: false types: [rust] diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index facc6de..2b1578a 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -159,32 +159,49 @@ fn baseline_hooks_present() { fn cargo_fmt_check_hook_contract() { let cfg = read_config(); - // Hook id and entry must appear. + // Two hooks: the root crate and the nested downstream_consumer + // sub-package (its own [package], not a workspace member — so + // the root `cargo fmt --check` does NOT traverse into it). Split + // into two hooks (rather than chained via `bash -c`) so neither + // entry depends on bash being on PATH — relevant on Windows + // runners in the CI matrix. assert!( cfg.contains("id: cargo-fmt-check"), - "must define a local hook with `id: cargo-fmt-check`" + "must define a local hook with `id: cargo-fmt-check` covering the root crate" ); + assert!( + cfg.contains("id: cargo-fmt-check-downstream"), + "must define a second local hook with `id: cargo-fmt-check-downstream` for the \ + nested tests/downstream_consumer sub-package — the root `cargo fmt --check` \ + does not traverse into a non-workspace-member [package], so AC3 would silently \ + miss fmt drift in that sub-crate without an explicit hook." + ); + + // Neither entry may rely on a shell. `bash -c '...'` would fail + // on Windows pre-commit runs that don't have bash on PATH. + assert!( + !cfg.contains("bash -c"), + "cargo-fmt-check hooks must not use `bash -c` — bash is not guaranteed on \ + Windows runners. Split chained commands into separate hooks instead." + ); + // The entry must run `cargo fmt --check` (with `--check`, not a // bare `cargo fmt` that would silently reformat files instead of - // failing on drift). We assert the substring rather than the - // literal full entry because the hook also runs `cargo fmt - // --manifest-path tests/downstream_consumer/Cargo.toml --check` - // (a non-workspace sub-package — see below). + // failing on drift). assert!( - cfg.contains("cargo fmt --check"), - "cargo-fmt-check hook entry must include `cargo fmt --check` — without `--check`, \ - the hook would silently reformat files instead of failing on drift." + cfg.contains("entry: cargo fmt --check"), + "cargo-fmt-check hook entry must be exactly `cargo fmt --check` — without \ + `--check`, the hook would silently reformat files instead of failing on drift." ); - // The nested `tests/downstream_consumer/` package is its own - // `[package]` (not a workspace member), so the root `cargo fmt - // --check` does NOT traverse into it. The hook must also format - // that sub-package explicitly — otherwise AC3 (commit with bad - // formatting fails) silently misses the sub-package. + + // The downstream hook must explicitly target the sub-package's + // manifest. Without `--manifest-path`, cargo finds the root + // manifest and re-checks the root crate, missing the sub-package. assert!( cfg.contains("--manifest-path tests/downstream_consumer/Cargo.toml --check"), - "cargo-fmt-check hook must also run `cargo fmt --manifest-path \ - tests/downstream_consumer/Cargo.toml --check` — the nested downstream_consumer \ - package is not a workspace member, so the root invocation does not cover it." + "cargo-fmt-check-downstream hook entry must include `--manifest-path \ + tests/downstream_consumer/Cargo.toml --check` — without it, cargo would \ + find the root manifest and skip the sub-package." ); // The four contract attributes for a `cargo fmt`-style local hook. From 2848e7424473eaa69e620934e638040699359e8f Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:59:11 +0000 Subject: [PATCH 16/28] test(infra): ENG-4687 harden e2e tests per Copilot review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AC2: assert the worktree stays hook-clean after `pre-commit run` even if pre-commit exited 0. A silent auto-fixer rewriting files violates the "main-equivalent tree is hook-clean" guarantee just as much as a non-zero exit would. Capture the post-run diff for the error message before restoring the tree. - AC3: drop the `output.contains("rustfmt")` fallback in the diff-visible assertion. The bare "rustfmt" substring would match unrelated error messages (e.g. "rustfmt component not installed"), letting AC3 pass for the wrong reason. Require `Diff in` only. - AC4 corollary: tighten the negative check to require pre-commit's explicit "No hook with id" diagnostic and a non-zero exit. The prior absence-of-banner fallback could be true if clippy errored before printing — false positive risk. - Extract `cargo_augmented_path()` helper using `std::env::join_paths` so PATH manipulation uses the platform separator (Windows uses `;`, Unix `:`). The old `format!("{cargo_dir}:{path}")` only worked on Unix. Removes ~30 lines of duplication across 3 call sites. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 167 +++++++++++++++++++++++----------------- 1 file changed, 96 insertions(+), 71 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 316179e..b8d99a8 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -40,6 +40,7 @@ //! The test cleans up its tempdir on success and on panic via //! `tempfile::TempDir`. +use std::ffi::OsString; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; @@ -78,6 +79,29 @@ fn cargo_bin() -> String { std::env::var("CARGO").unwrap_or_else(|_| "cargo".into()) } +/// Build a PATH value with `cargo`'s parent directory prepended to +/// the current `PATH`, using the platform's native separator (`:` on +/// Unix, `;` on Windows). Pre-commit hooks shell out to `cargo` and +/// `rustfmt`/`clippy-driver`; on Windows runners those binaries live +/// next to the `cargo` we were launched with (typically +/// `~/.rustup/toolchains/.../bin`), not on the inherited PATH. +fn cargo_augmented_path() -> OsString { + let cargo = cargo_bin(); + let cargo_dir = Path::new(&cargo).parent().map(Path::to_path_buf); + + let mut entries: Vec = Vec::new(); + if let Some(dir) = cargo_dir + && !dir.as_os_str().is_empty() + { + entries.push(dir); + } + if let Some(path) = std::env::var_os("PATH") { + entries.extend(std::env::split_paths(&path)); + } + + std::env::join_paths(&entries).expect("PATH entries must not contain the platform separator") +} + /// If pre-commit or git is missing, print why and return false. /// Callers should early-return when this is false. fn prereqs_available(test_name: &str) -> bool { @@ -188,11 +212,13 @@ fn seed_tmp_repo(dir: &Path) -> PathBuf { // point of AC2: the rustyroute working tree should be hook-clean. // // If the suite fails, the test prints pre-commit's stdout/stderr so the -// failing hook id is visible in CI logs. We do NOT mutate the working -// tree — pre-commit's auto-fixers (e.g. end-of-file-fixer) can write -// files, but we leave restoration to the caller / git. The test -// re-snapshots the working tree before and after via `git diff -// --quiet`; if pre-commit modified files, that itself counts as a fail. +// failing hook id is visible in CI logs. The test snapshots `git diff +// --quiet` before and after the run; if the tree was clean going in +// and pre-commit modified any files, that counts as a hook-modified-tree +// failure even if pre-commit somehow exited 0 — an auto-fixer that +// "succeeds" by silently rewriting files would still break AC2's +// "main-equivalent tree is hook-clean" contract. We capture the diff +// (for the error message), restore the tree, then assert. #[test] fn ac2_pre_commit_run_all_files_passes_on_current_tree() { @@ -216,19 +242,28 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { // If a hook is an auto-fixer (end-of-file-fixer, trailing-whitespace, // mixed-line-ending) it will both modify files AND return non-zero. - // Restore any modifications so we don't leave the worktree dirty — - // but only if it was clean BEFORE we ran. Otherwise we'd clobber - // user changes. - if pre_clean { + // Capture the post-run drift (if we started clean) so we can both + // restore the worktree and fail the assertion with a useful diff. + let post_drift: Option = if pre_clean { let (post_st, _) = run(Command::new("git") .current_dir(&root) .args(["diff", "--quiet"])); - if !post_st.success() { + if post_st.success() { + None + } else { + let (_diff_st, diff) = run(Command::new("git").current_dir(&root).args(["diff"])); let _ = run(Command::new("git") .current_dir(&root) .args(["checkout", "--", "."])); + Some(diff) } - } + } else { + // We started dirty (contributor's local edits in progress). + // Don't touch the worktree, don't assert on drift — the + // contributor's own changes would dominate the diff. AC2's + // exit-status check below is still meaningful. + None + }; assert!( st.success(), @@ -238,6 +273,19 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { main-equivalent tree to be hook-clean.\n\n\ pre-commit output:\n{output}" ); + + // Even if pre-commit exited 0, a hook may have silently rewritten + // a file. That violates AC2's "the main-equivalent tree is + // hook-clean" guarantee just as much as a non-zero exit would. + assert!( + post_drift.is_none(), + "AC2 violated: a pre-commit hook modified files on the current tree even \ + though pre-commit exited 0. AC2 requires the tree to be hook-clean — an \ + auto-fixer rewriting files is a fail even without a non-zero exit.\n\n\ + tree diff after pre-commit run:\n{}\n\n\ + pre-commit output:\n{output}", + post_drift.as_deref().unwrap_or("") + ); } // --------------------------------------------------------------------------- @@ -269,17 +317,7 @@ fn ac3_cargo_fmt_check_fails_on_misformatted_rust() { // Pre-commit needs CARGO/rustfmt on PATH. The hook uses // `language: system`, so we just need the real cargo. - let cargo = cargo_bin(); - let path = std::env::var("PATH").unwrap_or_default(); - let cargo_dir = Path::new(&cargo) - .parent() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default(); - let augmented_path = if cargo_dir.is_empty() { - path.clone() - } else { - format!("{cargo_dir}:{path}") - }; + let augmented_path = cargo_augmented_path(); let (st, output) = run(Command::new("pre-commit") .current_dir(&repo) @@ -294,15 +332,18 @@ fn ac3_cargo_fmt_check_fails_on_misformatted_rust() { pre-commit output:\n{output}" ); - // Rustfmt's --check output contains `Diff in` lines. Verify the - // contributor actually sees a diff (not a generic error), so the - // failure is self-explanatory. + // Rustfmt's --check output emits `Diff in ` for each file + // that would be reformatted. Require that exact signature — a + // broader match (e.g. on the bare substring `rustfmt`) would let + // an unrelated error like "rustfmt component not installed" pass + // AC3 without the contributor seeing a meaningful diff. assert!( - output.contains("Diff in") || output.contains("rustfmt"), - "AC3 partial: cargo-fmt-check failed but output does not look like rustfmt's \ - diff. Expected `Diff in` (rustfmt --check signature) or at least `rustfmt` \ - in the output so contributors know which hook fired.\n\n\ - actual output:\n{output}" + output.contains("Diff in"), + "AC3 partial: cargo-fmt-check failed but output does not contain rustfmt's \ + `Diff in` signature. Without that, the hook reported a failure but the \ + contributor cannot tell which file needs reformatting. Likely cause: rustfmt \ + exited non-zero for a non-drift reason (e.g. component missing, parse error).\ + \n\nactual output:\n{output}" ); } @@ -329,17 +370,7 @@ fn ac4_manual_stage_invokes_clippy() { // We need cargo + clippy-driver on PATH inside the pre-commit // subprocess. - let cargo = cargo_bin(); - let path = std::env::var("PATH").unwrap_or_default(); - let cargo_dir = Path::new(&cargo) - .parent() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default(); - let augmented_path = if cargo_dir.is_empty() { - path.clone() - } else { - format!("{cargo_dir}:{path}") - }; + let augmented_path = cargo_augmented_path(); let (st, output) = run(Command::new("pre-commit") .current_dir(&repo) @@ -399,43 +430,37 @@ fn ac4_clippy_does_not_run_on_default_stage() { let tmp = tempfile::tempdir().expect("create tempdir"); let repo = seed_tmp_repo(tmp.path()); - let cargo = cargo_bin(); - let path = std::env::var("PATH").unwrap_or_default(); - let cargo_dir = Path::new(&cargo) - .parent() - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default(); - let augmented_path = if cargo_dir.is_empty() { - path.clone() - } else { - format!("{cargo_dir}:{path}") - }; + let augmented_path = cargo_augmented_path(); // Ask pre-commit to run *just* cargo-clippy on the default stage. // It should refuse (the hook is configured for manual only). - let (_st, output) = run(Command::new("pre-commit") + let (st, output) = run(Command::new("pre-commit") .current_dir(&repo) .env("PATH", &augmented_path) .args(["run", "cargo-clippy", "--all-files", "--color", "never"])); - // pre-commit's exact message when a hook is filtered out by stage - // is "No hook with id `cargo-clippy` in stage `pre-commit`" (or - // similar wording across versions). Either that phrase or the - // absence of the cargo-clippy invocation banner is acceptable. - // Anchor the negative check on cargo's compile banner for the - // seeded crate (`Checking e2e_root`). The hook NAME `cargo clippy - // --no-deps` from .pre-commit-config.yaml can appear in pre-commit's - // banner output regardless of whether the entry actually launched, - // so matching on it here would risk a future-pre-commit-version - // flip. The `Checking e2e_root` banner only appears if clippy - // actually ran against the seeded crate. - let filtered_out = - output.contains("No hook with id `cargo-clippy`") || !output.contains("Checking e2e_root"); + // Pre-commit emits a stage-mismatch diagnostic when asked to run + // a hook that is not active in the current stage. The wording is + // "No hook with id `cargo-clippy` in stage `pre-commit`" on + // current versions; we match the stable prefix + // `No hook with id `cargo-clippy`` to ride out trailing-wording + // tweaks across versions. Pre-commit exits non-zero in that case + // — assert both the diagnostic and the non-zero exit so a future + // version that filters the hook silently (exit 0, no banner) + // cannot pass the test by absence-of-evidence alone. assert!( - filtered_out, - "AC4 corollary violated: cargo-clippy executed on the DEFAULT pre-commit stage. \ - It must be opt-in via `--hook-stage manual` (clippy is too slow for every \ - commit). Check that the hook keeps `stages: [manual]`.\n\n\ - pre-commit output:\n{output}" + !st.success(), + "AC4 corollary partial: pre-commit exited 0 when asked to run `cargo-clippy` on \ + the default stage. The expected outcome is a non-zero exit with a \"No hook with \ + id\" diagnostic — silent success means clippy may actually have run on the \ + default stage.\n\nfull output:\n{output}" + ); + assert!( + output.contains("No hook with id `cargo-clippy`"), + "AC4 corollary violated: pre-commit did not emit the expected stage-mismatch \ + diagnostic (`No hook with id \\`cargo-clippy\\``) when asked to run the hook \ + on the default stage. The hook must be opt-in via `--hook-stage manual` — \ + clippy is too slow for every commit. Check that the hook keeps \ + `stages: [manual]`.\n\nfull output:\n{output}" ); } From 718252eb7791f1d8c6f789a7658c269892ab4611 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 18:59:18 +0000 Subject: [PATCH 17/28] docs: ENG-4687 add pre-commit install command to CONTRIBUTING.md The previous stanza jumped straight to `pre-commit install` without explaining how to obtain the `pre-commit` tool itself. Add pipx/pip/brew install options before the install line so the setup steps stand alone. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- CONTRIBUTING.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 447f4b5..2c9c72e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,10 +69,12 @@ See for the full text of the DCO. ## Local validation -First-time setup — install the [pre-commit](https://pre-commit.com) -hook so format and file-hygiene checks run on every `git commit`: +First-time setup — install [pre-commit](https://pre-commit.com) itself +(via pipx, pip, or Homebrew), then enable the git hook so format and +file-hygiene checks run on every `git commit`: ```sh +pipx install pre-commit # or: pip install pre-commit / brew install pre-commit pre-commit install ``` From a7f53e1164ae51e931312f386931818c067e227d Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:08:16 +0000 Subject: [PATCH 18/28] fix: ENG-4687 address second Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ci: pin `pre-commit==4.6.0` in the CI job. The e2e tests assert on specific pre-commit diagnostic wording ("No hook with id …"); an unpinned upstream release could change that wording and cause nondeterministic CI failures. - docs: extend the CONTRIBUTING.md manual-stage comment to include `-- -D warnings` so the example matches the actual hook entry. - test(infra): tighten AC2's worktree-clean check to use `git status --porcelain` (empty == truly clean: no unstaged, no staged, no untracked) instead of `git diff --quiet`. The previous check only saw unstaged drift, so a contributor with staged work or untracked files could trigger the drift-recovery `git checkout -- .` and lose their in-progress changes. - test(infra): collapse the duplicate `pre_commit_on_path` / `git_on_path` helpers into a single `bin_on_path(&str)`. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- .github/workflows/ci.yaml | 6 ++++- CONTRIBUTING.md | 2 +- tests/pre_commit_e2e.rs | 56 +++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b2a1acd..37fd7d6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -137,7 +137,11 @@ jobs: - uses: actions/setup-python@v5 with: python-version: '3.x' - - run: pip install pre-commit + # Pin `pre-commit` so CI stays reproducible. The e2e tests assert + # on specific pre-commit diagnostic wording (e.g. "No hook with + # id ..."); an unpinned upstream release could change that + # wording and cause nondeterministic CI failures. + - run: pip install 'pre-commit==4.6.0' - uses: Swatinem/rust-cache@v2 with: shared-key: pre-commit diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2c9c72e..37af38e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,7 +82,7 @@ To run the managed hooks explicitly (e.g. before pushing): ```sh pre-commit run --all-files -pre-commit run --hook-stage manual # runs cargo clippy --no-deps +pre-commit run --hook-stage manual # runs cargo clippy --no-deps -- -D warnings ``` The pre-commit gate runs `cargo fmt --check` plus lightweight file diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index b8d99a8..c98fee0 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -55,20 +55,11 @@ fn repo_root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } -/// Returns `Some(path)` if `pre-commit` is on PATH, else `None`. -fn pre_commit_on_path() -> Option { - // Use `which`-equivalent logic by trying `pre-commit --version`. - let out = Command::new("pre-commit").arg("--version").output().ok()?; - if out.status.success() { - // The exact path doesn't matter; just signal "available". - Some(PathBuf::from("pre-commit")) - } else { - None - } -} - -fn git_on_path() -> bool { - Command::new("git") +/// Returns true if a `--version` probe of `bin` succeeds. Used to test +/// for `pre-commit` and `git` on PATH without taking a dependency on +/// the `which` crate. +fn bin_on_path(bin: &str) -> bool { + Command::new(bin) .arg("--version") .output() .map(|o| o.status.success()) @@ -105,14 +96,14 @@ fn cargo_augmented_path() -> OsString { /// If pre-commit or git is missing, print why and return false. /// Callers should early-return when this is false. fn prereqs_available(test_name: &str) -> bool { - if pre_commit_on_path().is_none() { + if !bin_on_path("pre-commit") { eprintln!( "skipping {test_name}: pre-commit not found on PATH \ (install with `pip install pre-commit`)" ); return false; } - if !git_on_path() { + if !bin_on_path("git") { eprintln!("skipping {test_name}: git not found on PATH"); return false; } @@ -227,11 +218,16 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { } let root = repo_root(); - // Snapshot working-tree state before running. - let (pre_st, _) = run(Command::new("git") + // Snapshot working-tree state before running. `git status + // --porcelain` is empty only when the tree has no unstaged + // changes, no staged changes, AND no untracked files — `git diff + // --quiet` alone would treat a tree with staged or untracked + // work as "clean" and the drift-recovery path below would then + // `git checkout -- .` over a contributor's in-progress work. + let (pre_status_st, pre_status_out) = run(Command::new("git") .current_dir(&root) - .args(["diff", "--quiet"])); - let pre_clean = pre_st.success(); + .args(["status", "--porcelain"])); + let pre_clean = pre_status_st.success() && pre_status_out.is_empty(); let (st, output) = run(Command::new("pre-commit").current_dir(&root).args([ "run", @@ -245,23 +241,27 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { // Capture the post-run drift (if we started clean) so we can both // restore the worktree and fail the assertion with a useful diff. let post_drift: Option = if pre_clean { - let (post_st, _) = run(Command::new("git") + let (post_status_st, post_status_out) = run(Command::new("git") .current_dir(&root) - .args(["diff", "--quiet"])); - if post_st.success() { + .args(["status", "--porcelain"])); + if post_status_st.success() && post_status_out.is_empty() { None } else { let (_diff_st, diff) = run(Command::new("git").current_dir(&root).args(["diff"])); let _ = run(Command::new("git") .current_dir(&root) .args(["checkout", "--", "."])); - Some(diff) + Some(if diff.is_empty() { + post_status_out + } else { + diff + }) } } else { - // We started dirty (contributor's local edits in progress). - // Don't touch the worktree, don't assert on drift — the - // contributor's own changes would dominate the diff. AC2's - // exit-status check below is still meaningful. + // We started with local changes (unstaged edits, staged work, + // or untracked files). Don't touch the worktree, don't assert + // on drift — the contributor's own changes would dominate the + // diff. AC2's exit-status check below is still meaningful. None }; From 9bc9a7bfd58d7702f30f5dddc4eccab5941ad637 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:15:08 +0000 Subject: [PATCH 19/28] fix: ENG-4687 address third Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - security(infra): drop `args: ['--unsafe']` from check-yaml. Verified via `yaml.safe_load(...)` that every YAML in the repo (workflows, config, .pre-commit-config.yaml itself) loads safely — there's no custom-tag YAML that needs the unsafe loader, so the flag was pure attack surface against running pre-commit on untrusted PR branches. - test(infra): AC2 drift-recovery now also runs `git clean -fd` after `git checkout -- .` so an untracked artifact written by a hook doesn't leave the worktree dirty. Safe because the recovery path only fires when the pre-check confirmed the tree was truly clean. - test(infra): tighten the cargo-fmt-check invariant test to match the full `entry:` line (post-trim) instead of a substring. The prior `cfg.contains("entry: cargo fmt --check")` would have happily passed `entry: cargo fmt --check --all` and silently expanded coverage. Now compares against the exact expected line for both hooks. - docs(test): update the e2e module-level doc-comment from `std::env::temp_dir()` to `tempfile::tempdir()` (auto-cleans on drop) to match the implementation. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- .pre-commit-config.yaml | 1 - tests/pre_commit_config.rs | 43 ++++++++++++++++++++++++++------------ tests/pre_commit_e2e.rs | 18 +++++++++++----- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 577c252..a174b03 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -30,7 +30,6 @@ repos: - id: check-json - id: check-toml - id: check-yaml - args: ['--unsafe'] - id: check-merge-conflict - id: check-case-conflict - id: check-added-large-files diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 2b1578a..8819751 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -185,23 +185,40 @@ fn cargo_fmt_check_hook_contract() { Windows runners. Split chained commands into separate hooks instead." ); - // The entry must run `cargo fmt --check` (with `--check`, not a - // bare `cargo fmt` that would silently reformat files instead of - // failing on drift). + // Find every `entry:` line and check the cargo-fmt-check entries + // verbatim (modulo leading whitespace). A `cfg.contains(...)` + // substring check would let `entry: cargo fmt --check --all` pass + // even though `--all` would silently expand coverage to workspace + // members the hook does not intend to format here. + let entries: Vec<&str> = cfg + .lines() + .map(str::trim) + .filter(|line| line.starts_with("entry:")) + .collect(); + + // The root hook must be `entry: cargo fmt --check` — exactly. No + // trailing args (no `--all`, no extra manifest, etc.) — the + // sub-package gets its own dedicated hook below. assert!( - cfg.contains("entry: cargo fmt --check"), - "cargo-fmt-check hook entry must be exactly `cargo fmt --check` — without \ - `--check`, the hook would silently reformat files instead of failing on drift." + entries + .iter() + .any(|line| *line == "entry: cargo fmt --check"), + "cargo-fmt-check (root) hook entry must be exactly `cargo fmt --check` — \ + without `--check`, the hook would silently reformat files instead of \ + failing on drift; with extra args (e.g. `--all`) the coverage would \ + silently shift. Found entries: {entries:?}" ); - // The downstream hook must explicitly target the sub-package's - // manifest. Without `--manifest-path`, cargo finds the root - // manifest and re-checks the root crate, missing the sub-package. + // The downstream hook must target the sub-package's manifest + // explicitly and use `--check`. assert!( - cfg.contains("--manifest-path tests/downstream_consumer/Cargo.toml --check"), - "cargo-fmt-check-downstream hook entry must include `--manifest-path \ - tests/downstream_consumer/Cargo.toml --check` — without it, cargo would \ - find the root manifest and skip the sub-package." + entries.iter().any(|line| { + *line == "entry: cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check" + }), + "cargo-fmt-check-downstream hook entry must be exactly `cargo fmt \ + --manifest-path tests/downstream_consumer/Cargo.toml --check` — without \ + `--manifest-path`, cargo would find the root manifest and skip the \ + sub-package. Found entries: {entries:?}" ); // The four contract attributes for a `cargo fmt`-style local hook. diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index c98fee0..c80ca27 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -34,11 +34,10 @@ //! --------- //! AC3 mutates a worktree and creates a commit. To avoid touching the //! checked-out rustyroute repo, the test materialises a throwaway git -//! repo in `std::env::temp_dir()` with the *real* -//! `.pre-commit-config.yaml` symlinked/copied in, plus the minimum -//! Cargo scaffolding so `cargo fmt --check` has something to check. -//! The test cleans up its tempdir on success and on panic via -//! `tempfile::TempDir`. +//! repo via `tempfile::tempdir()` (which auto-cleans on drop, even on +//! panic) with the *real* `.pre-commit-config.yaml` copied in, plus +//! the minimum Cargo scaffolding so `cargo fmt --check` has something +//! to check. use std::ffi::OsString; use std::fs; @@ -248,9 +247,18 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { None } else { let (_diff_st, diff) = run(Command::new("git").current_dir(&root).args(["diff"])); + // Safe to fully reset here: the pre-check used + // `git status --porcelain` to confirm the tree was truly + // clean (no unstaged, no staged, no untracked), so anything + // we see now must have come from pre-commit. Restore + // tracked-file edits AND remove any untracked artifacts + // (`git checkout -- .` alone leaves untracked files behind). let _ = run(Command::new("git") .current_dir(&root) .args(["checkout", "--", "."])); + let _ = run(Command::new("git") + .current_dir(&root) + .args(["clean", "-fd"])); Some(if diff.is_empty() { post_status_out } else { From 3f430a76a7470cadc5bd32a2f9cc809094e1fd33 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:22:15 +0000 Subject: [PATCH 20/28] test(infra): ENG-4687 address fourth Copilot review on PR #8 Three nits flagged on commit fd46d11: - AC4 `invoked` probe: drop the `cargo clippy --no-deps` substring alternative. It also matches pre-commit's `name:` banner, which is printed regardless of whether the entry actually executed (e.g. when the clippy toolchain component is missing). Anchor solely on cargo's own `Checking ...` output. - Skip-semantics module doc: the wording "returns Ok(())" was wrong; these are `#[test] fn ... ()` tests that `return;` early. Updated the doc-comment to describe the actual mechanism. - `cargo_fmt_check_hook_contract`: previously asserted that the four contract attributes appeared *somewhere* in the file. That would let the downstream fmt hook silently lose `pass_filenames: false` or `types: [rust]` without detection. Switched to a count-based assertion (`>= 2 occurrences` per attribute), so dropping one from either hook now trips the invariant. Tests: 11/11 (config) + 4/4 (e2e) pass. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 19 ++++++++++++------- tests/pre_commit_e2e.rs | 38 ++++++++++++++++++++++---------------- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 8819751..8d55644 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -222,15 +222,20 @@ fn cargo_fmt_check_hook_contract() { ); // The four contract attributes for a `cargo fmt`-style local hook. - // We assert their presence somewhere in the file rather than by - // YAML structure to keep the test parser-free, matching - // `tests/ci_workflow.rs`'s style. + // Both fmt hooks (root + downstream) must carry these, so each + // attribute should appear AT LEAST twice. A bare `cfg.contains(...)` + // would pass even if the downstream hook silently lost its + // `pass_filenames: false` or `types: [rust]` — at which point that + // hook would either fail (filenames passed as positional args) or + // fire on non-Rust commits. for needle in ["language: system", "pass_filenames: false", "types: [rust]"] { + let count = cfg.matches(needle).count(); assert!( - cfg.contains(needle), - "cargo-fmt-check hook must include `{needle}` — without it the hook \ - would either need a managed Rust toolchain (`language: rust` isn't \ - supported for `cargo fmt`), pass filenames as positional args \ + count >= 2, + "both cargo-fmt-check hooks must include `{needle}` — found {count} \ + occurrence(s), expected at least 2 (one per fmt hook). Without it the \ + hook would either need a managed Rust toolchain (`language: rust` \ + isn't supported for `cargo fmt`), pass filenames as positional args \ (which `cargo fmt` rejects), or fire on every commit even when no \ Rust file is staged." ); diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index c80ca27..e917594 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -21,14 +21,17 @@ //! -------------- //! These tests shell out to the system `pre-commit` binary. If it is //! not on `PATH` (e.g. a Rust-only contributor without Python), the -//! test prints a skip message and returns `Ok(())` rather than failing -//! — matches the spec's "AC1 manual" framing where pre-commit install -//! is a per-clone setup, not a cargo-test prereq. The CI workflow's -//! dedicated `pre-commit` job (`.github/workflows/ci.yaml`) installs -//! `pre-commit` via `pip` and then runs `cargo test --test -//! pre_commit_e2e` so AC2/AC3/AC4 are exercised for real on every PR; -//! the other CI jobs (fmt, clippy, test-matrix, ...) do NOT install -//! pre-commit, so this file silently skips there by design. +//! test prints a skip message to stderr and `return`s early without +//! asserting — matches the spec's "AC1 manual" framing where +//! pre-commit install is a per-clone setup, not a cargo-test prereq. +//! Cargo still reports these as passing (no panic == pass for a +//! `#[test] fn` returning `()`), so the skip is invisible to the +//! suite's pass/fail count. The CI workflow's dedicated `pre-commit` +//! job (`.github/workflows/ci.yaml`) installs `pre-commit` via `pip` +//! and then runs `cargo test --test pre_commit_e2e` so AC2/AC3/AC4 +//! are exercised for real on every PR; the other CI jobs (fmt, +//! clippy, test-matrix, ...) do NOT install pre-commit, so this file +//! silently skips there by design. //! //! Isolation //! --------- @@ -407,15 +410,18 @@ fn ac4_manual_stage_invokes_clippy() { // `Checking v` when it starts work, and the // literal entry `cargo clippy --no-deps` appears in pre-commit's // command echo. Either is sufficient evidence of invocation. - // (We deliberately don't match the bare substring `clippy` — that - // also appears in pre-commit's hook-name banner, which is printed - // regardless of whether the entry actually executed.) - let invoked = output.contains("Checking ") || output.contains("cargo clippy --no-deps"); + // We anchor the positive check on cargo's own output (`Checking + // `). The `cargo clippy --no-deps` substring is NOT a + // sufficient signal — it's also the hook's `name:` field in + // `.pre-commit-config.yaml`, which pre-commit prints regardless + // of whether the entry actually executed (e.g. if the clippy + // toolchain component is missing). assert!( - invoked, - "AC4 partial: cargo-clippy hook ran at manual stage but output does not \ - show clippy actually executing. Expected `Checking ...` (cargo banner) or \ - `clippy` in the output.\n\n\ + output.contains("Checking "), + "AC4 partial: cargo-clippy hook ran at manual stage but pre-commit output \ + does not contain cargo's `Checking ` banner, so clippy did NOT \ + actually execute against the seeded crate. Likely cause: missing clippy \ + toolchain component, or the entry failed before reaching cargo.\n\n\ hook exit: {st:?}\n\ pre-commit output:\n{output}" ); From bb66ded9d0d0d75819e8b869dd449d2b9e4d79f1 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:22:39 +0000 Subject: [PATCH 21/28] fix: ENG-4687 address fourth Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - test(infra): AC4 manual-stage check now uses `pre-commit run --verbose` and asserts only on cargo's own `Checking ` banner. Pre-commit swallows command stdout by default when a hook exits 0, so the prior version had to fall back to matching `cargo clippy --no-deps` — which is also the hook's `name:` in `.pre-commit-config.yaml` and would have appeared even if clippy never ran (missing toolchain component, etc.). With --verbose, cargo's banner is reliably visible whenever clippy actually executes; the substring fallback is removed. - test(infra): tighten `cargo_fmt_check_hook_contract` to require `language: system`, `pass_filenames: false`, and `types: [rust]` appear AT LEAST twice (once per fmt hook). A bare `contains` would silently accept a downstream hook that lost one of those keys, which would break that hook in ways the existing e2e (which only exercises `cargo-fmt-check`) wouldn't catch. - docs(test): clarify the skip-semantics doc-comment — the tests are plain `#[test] fn ... -> ()` that `return;` early when pre-commit is missing, not Result-returning functions. Cargo treats no-panic as pass, so the skip is invisible to the suite count. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index e917594..98fd3c3 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -383,6 +383,11 @@ fn ac4_manual_stage_invokes_clippy() { // subprocess. let augmented_path = cargo_augmented_path(); + // `--verbose` is required so pre-commit forwards the underlying + // command's stdout — by default the framework swallows output when + // a hook exits 0, leaving only the "Passed" banner and no evidence + // that cargo+clippy actually ran. With --verbose we see cargo's + // own `Checking ` banner. let (st, output) = run(Command::new("pre-commit") .current_dir(&repo) .env("PATH", &augmented_path) @@ -393,6 +398,7 @@ fn ac4_manual_stage_invokes_clippy() { "cargo-clippy", "--color", "never", + "--verbose", ])); // The hook should be RECOGNISED — pre-commit's "no hook with this From 8e8e78f6d5b7388c6e21b2ccb322fe23327f1a9c Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:28:16 +0000 Subject: [PATCH 22/28] fix(test): ENG-4687 AC4 strip cargo ANSI colors so `Checking` matches The CI run of `cargo test --test pre_commit_e2e` on Linux failed AC4 because cargo's `Checking` banner came back with ANSI escape codes glued to it (`Checking\x1b[0m e2e_root`), so the substring assertion `output.contains("Checking ")` (with a trailing space) did not match. `pre-commit run --color never` only controls pre-commit's own colors, not what the underlying cargo subprocess emits. Set `CARGO_TERM_COLOR=never` on the pre-commit env so cargo strips its ANSI output, and tighten the substring to `Checking e2e_root` (the seeded crate name) so the assertion fails clearly if anything other than that specific compile happened. Local cargo test --test pre_commit_e2e: 4/4 PASS. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 98fd3c3..6d1ba00 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -388,9 +388,16 @@ fn ac4_manual_stage_invokes_clippy() { // a hook exits 0, leaving only the "Passed" banner and no evidence // that cargo+clippy actually ran. With --verbose we see cargo's // own `Checking ` banner. + // + // `CARGO_TERM_COLOR=never` strips cargo's ANSI escape codes so the + // substring match below sees `Checking ` (with a real space) + // instead of `Checking\x1b[0m ` — `--color never` only controls + // pre-commit's own colors, not what cargo emits when it inherits a + // TTY env from the CI runner. let (st, output) = run(Command::new("pre-commit") .current_dir(&repo) .env("PATH", &augmented_path) + .env("CARGO_TERM_COLOR", "never") .args([ "run", "--hook-stage", @@ -412,18 +419,14 @@ fn ac4_manual_stage_invokes_clippy() { pre-commit output:\n{output}" ); - // The hook should actually invoke cargo+clippy. cargo prints - // `Checking v` when it starts work, and the - // literal entry `cargo clippy --no-deps` appears in pre-commit's - // command echo. Either is sufficient evidence of invocation. - // We anchor the positive check on cargo's own output (`Checking - // `). The `cargo clippy --no-deps` substring is NOT a - // sufficient signal — it's also the hook's `name:` field in - // `.pre-commit-config.yaml`, which pre-commit prints regardless - // of whether the entry actually executed (e.g. if the clippy - // toolchain component is missing). + // The hook should actually invoke cargo+clippy. We anchor the + // positive check on cargo's own output (`Checking `). The + // `cargo clippy --no-deps` substring is NOT a sufficient signal — + // it's also the hook's `name:` field in `.pre-commit-config.yaml`, + // which pre-commit prints regardless of whether the entry actually + // executed (e.g. if the clippy toolchain component is missing). assert!( - output.contains("Checking "), + output.contains("Checking e2e_root"), "AC4 partial: cargo-clippy hook ran at manual stage but pre-commit output \ does not contain cargo's `Checking ` banner, so clippy did NOT \ actually execute against the seeded crate. Likely cause: missing clippy \ From 5bb28286288130ed1ae81ad7f2a676b6ede4cb12 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:30:29 +0000 Subject: [PATCH 23/28] test(infra): ENG-4687 address fifth Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two AC2 nits flagged on commit 8d19074: - AC2 dirty-tree behaviour: previously, the test ran `pre-commit run --all-files` even when `pre_clean` was false, and intentionally disabled the drift-recovery path in that case so a contributor's in-progress work wouldn't be `git checkout`-ed over. The side effect was that auto-fixer hooks (end-of-file-fixer, trailing-whitespace, mixed-line-ending) could still silently rewrite those in-progress changes — worse than failing. Skip with a clear stderr message instead; AC2 is about a main-equivalent CLEAN tree, not whatever transient state is in someone's worktree mid-edit. The post-run drift logic is now unconditional (no `if pre_clean { ... } else { None }`) since we early-return on dirty trees. - AC2 module doc-comment: said the test snapshots `git diff --quiet` before/after, but the implementation uses `git status --porcelain` + `git diff`. Update the wording to match the actual mechanism so future readers don't chase the wrong command when debugging drift. Tests: 11/11 (config) + 4/4 (e2e) pass; rustfmt clean. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 57 +++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 6d1ba00..14d5ede 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -205,13 +205,24 @@ fn seed_tmp_repo(dir: &Path) -> PathBuf { // point of AC2: the rustyroute working tree should be hook-clean. // // If the suite fails, the test prints pre-commit's stdout/stderr so the -// failing hook id is visible in CI logs. The test snapshots `git diff -// --quiet` before and after the run; if the tree was clean going in -// and pre-commit modified any files, that counts as a hook-modified-tree -// failure even if pre-commit somehow exited 0 — an auto-fixer that -// "succeeds" by silently rewriting files would still break AC2's -// "main-equivalent tree is hook-clean" contract. We capture the diff -// (for the error message), restore the tree, then assert. +// failing hook id is visible in CI logs. The test snapshots +// `git status --porcelain` before and after the run; if the tree was +// clean going in and pre-commit modified any files, that counts as a +// hook-modified-tree failure even if pre-commit somehow exited 0 — an +// auto-fixer that "succeeds" by silently rewriting files would still +// break AC2's "main-equivalent tree is hook-clean" contract. We +// capture the diff (for the error message), restore the tree, then +// assert. +// +// If the worktree is NOT clean going in (the contributor has local +// unstaged/staged/untracked work), the test skips early instead of +// running the suite — several configured hooks are auto-fixers +// (end-of-file-fixer, trailing-whitespace, mixed-line-ending) and the +// drift-recovery path is intentionally disabled when there's +// pre-existing local work, so running here would silently rewrite the +// contributor's in-progress changes. AC2 is specifically about a +// main-equivalent clean tree; running on a dirty tree would be testing +// a different scenario anyway. #[test] fn ac2_pre_commit_run_all_files_passes_on_current_tree() { @@ -231,6 +242,25 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { .args(["status", "--porcelain"])); let pre_clean = pre_status_st.success() && pre_status_out.is_empty(); + // If we're not on a clean tree, skip rather than run. Several + // configured hooks are auto-fixers (end-of-file-fixer, + // trailing-whitespace, mixed-line-ending) and the drift-recovery + // path below is disabled when `pre_clean` is false (we don't want + // to `git checkout -- .` over a contributor's in-progress work). + // Running the hook suite here would therefore silently rewrite + // their changes — worse than failing — and AC2 is about a + // main-equivalent clean tree anyway, not whatever transient state + // is sitting in someone's worktree mid-edit. + if !pre_clean { + eprintln!( + "skipping ac2_pre_commit_run_all_files_passes_on_current_tree: \ + working tree has local changes (status --porcelain non-empty). \ + AC2 is verified against a clean tree; rerun after committing \ + or stashing." + ); + return; + } + let (st, output) = run(Command::new("pre-commit").current_dir(&root).args([ "run", "--all-files", @@ -240,9 +270,10 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { // If a hook is an auto-fixer (end-of-file-fixer, trailing-whitespace, // mixed-line-ending) it will both modify files AND return non-zero. - // Capture the post-run drift (if we started clean) so we can both - // restore the worktree and fail the assertion with a useful diff. - let post_drift: Option = if pre_clean { + // We early-returned above when the tree wasn't clean, so any drift + // observed here came from pre-commit. Capture the post-run diff, + // then restore the worktree, then assert. + let post_drift: Option = { let (post_status_st, post_status_out) = run(Command::new("git") .current_dir(&root) .args(["status", "--porcelain"])); @@ -268,12 +299,6 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { diff }) } - } else { - // We started with local changes (unstaged edits, staged work, - // or untracked files). Don't touch the worktree, don't assert - // on drift — the contributor's own changes would dominate the - // diff. AC2's exit-status check below is still meaningful. - None }; assert!( From 8a8b3e69ac79ba74ac86475306c4c4fbb7092d7e Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:37:07 +0000 Subject: [PATCH 24/28] test+ci: ENG-4687 address sixth Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two nits flagged on commit bfb40f6: - `cargo_fmt_check_hook_contract` previously used global occurrence counts (`>= 2`) for `language: system` / `pass_filenames: false` / `types: [rust]`. That could be satisfied by the cargo-clippy hook (which also has `language: system` and `pass_filenames: false`), masking a silently dropped attribute on cargo-fmt-check-downstream. Now scopes the check per-hook: locate each `- id: cargo-fmt-check*` header, read up to the next `- id:`, assert each attribute appears inside the block. Drops the cargo-clippy false positive entirely. - CI workflow was running `pre-commit run --all-files --show-diff-on-failure` and then `cargo test --test pre_commit_e2e` immediately after — AC2 in that test calls `pre-commit run --all-files` on the same root, so the hook suite ran twice. AC2 also asserts the tree stays drift-free, which the standalone step did not, so AC2 strictly supersedes it. Removed the standalone step; updated the comment to explain the dedup. Tests: 11/11 (config) + 4/4 (e2e) pass; rustfmt clean. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- .github/workflows/ci.yaml | 18 +++++++++----- tests/pre_commit_config.rs | 50 ++++++++++++++++++++++++-------------- 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 37fd7d6..31341bb 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -145,10 +145,16 @@ jobs: - uses: Swatinem/rust-cache@v2 with: shared-key: pre-commit - # Exercise the hooks directly (AC2) plus the e2e harness that - # also covers AC3 (fmt-bad commit) and AC4 (manual-stage clippy). - # Without pre-commit on PATH, tests/pre_commit_e2e.rs silently - # skips — installing it here is what makes AC2/AC3/AC4 actually - # machine-verified rather than self-attested. - - run: pre-commit run --all-files --show-diff-on-failure + # `cargo test --test pre_commit_e2e` exercises pre-commit + # end-to-end: AC2 (`ac2_pre_commit_run_all_files_passes_on_current_tree`) + # runs `pre-commit run --all-files` on the repo root and also + # asserts the tree stays drift-free; AC3 covers the fmt-bad + # commit path; AC4 covers the manual-stage clippy hook and its + # default-stage absence. AC2 supersedes a standalone + # `pre-commit run --all-files` step here — it does the same hook + # invocation plus the drift check, so running both would + # duplicate the hook suite and burn CI time. Without pre-commit + # on PATH, the e2e tests silently skip — installing it above is + # what makes AC2/AC3/AC4 actually machine-verified rather than + # self-attested. - run: cargo test --test pre_commit_e2e diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 8d55644..7978961 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -221,24 +221,38 @@ fn cargo_fmt_check_hook_contract() { sub-package. Found entries: {entries:?}" ); - // The four contract attributes for a `cargo fmt`-style local hook. - // Both fmt hooks (root + downstream) must carry these, so each - // attribute should appear AT LEAST twice. A bare `cfg.contains(...)` - // would pass even if the downstream hook silently lost its - // `pass_filenames: false` or `types: [rust]` — at which point that - // hook would either fail (filenames passed as positional args) or - // fire on non-Rust commits. - for needle in ["language: system", "pass_filenames: false", "types: [rust]"] { - let count = cfg.matches(needle).count(); - assert!( - count >= 2, - "both cargo-fmt-check hooks must include `{needle}` — found {count} \ - occurrence(s), expected at least 2 (one per fmt hook). Without it the \ - hook would either need a managed Rust toolchain (`language: rust` \ - isn't supported for `cargo fmt`), pass filenames as positional args \ - (which `cargo fmt` rejects), or fire on every commit even when no \ - Rust file is staged." - ); + // The three contract attributes for a `cargo fmt`-style local hook. + // Both fmt hooks (root + downstream) must carry these. We assert + // per-hook (by locating each `- id:` line and reading up to the + // next `- id:`) rather than counting occurrences across the whole + // file — a global count of `>= 2` could be satisfied by other + // hooks that also use `language: system` and `pass_filenames: + // false` (e.g. cargo-clippy), masking a silently dropped attribute + // on one of the fmt hooks. + let lines: Vec<&str> = cfg.lines().collect(); + for hook_id in ["cargo-fmt-check", "cargo-fmt-check-downstream"] { + let header = format!("- id: {hook_id}"); + let start = lines + .iter() + .position(|line| line.trim_start() == header) + .unwrap_or_else(|| panic!("hook `{hook_id}` not found in config")); + let end = lines[start + 1..] + .iter() + .position(|line| line.trim_start().starts_with("- id:")) + .map(|offset| start + 1 + offset) + .unwrap_or(lines.len()); + let block = &lines[start..end]; + + for needle in ["language: system", "pass_filenames: false", "types: [rust]"] { + assert!( + block.iter().any(|line| line.contains(needle)), + "hook `{hook_id}` must include `{needle}` in its own block — without \ + it the hook would either need a managed Rust toolchain (`language: \ + rust` isn't supported for `cargo fmt`), pass filenames as positional \ + args (which `cargo fmt` rejects), or fire on every commit even when \ + no Rust file is staged. Block scanned:\n{block:#?}" + ); + } } } From 100d67588a062f14607e5bdda62eee36b618ac38 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:45:04 +0000 Subject: [PATCH 25/28] test(infra): ENG-4687 address seventh Copilot review on PR #8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of three nits flagged on commit 5a4e172: - AC3/AC4 toolchain-component probes: `prereqs_available()` only checked for `pre-commit` and `git`, but AC3 needs rustfmt (via `cargo fmt`) and AC4 needs clippy-driver (via `cargo clippy`). A contributor with `pre-commit` installed but missing rustfmt or clippy would hit a misleading hook failure. Added a `cargo_subcommand_available(name)` helper that probes `cargo --version` with the cargo-augmented PATH, and gates AC3 / AC4 on it so they skip cleanly with a `rustup component add ...` hint. - `pre_commit_config.rs` module-doc invariant list: said `cargo fmt --check` had four contract attributes, but the test now checks three per-block (`language: system`, `pass_filenames: false`, `types: [rust]`) — `entry:` is the fourth attribute, asserted separately as an exact match. Reworded the bullet to call out both fmt hooks and to distinguish the per-block attributes from the separately-asserted entry line. The third nit (Copilot suggested making the e2e tests opt-in via `#[ignore]` or an env var to avoid pre-commit auto-downloading hook envs on first cargo-test run) is a design call. The reply on PR #8 explains why current skip-on-PATH-missing semantics is the right tradeoff for this project. Tests: 11/11 (config) + 4/4 (e2e) pass; rustfmt clean. Reported by Copilot on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 5 ++++- tests/pre_commit_e2e.rs | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 7978961..3ab1c6f 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -22,7 +22,10 @@ //! - `pre-commit-hooks` is pinned to v5.0.0 (matches backend's //! pinning convention). //! - Required baseline hook ids are present. -//! - `cargo fmt --check` hook has the four contract attributes. +//! - Both `cargo fmt --check` hooks (root + downstream) carry the +//! three per-block contract attributes (`language: system`, +//! `pass_filenames: false`, `types: [rust]`) — plus the `entry:` +//! line, asserted separately above as an exact match. //! - `cargo clippy` hook is on `stages: [manual]`. //! - Python-specific backend hooks/repos are ABSENT. diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 14d5ede..477c7cb 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -112,6 +112,21 @@ fn prereqs_available(test_name: &str) -> bool { true } +/// Returns true if `cargo --version` succeeds with the +/// cargo-augmented PATH (so rustfmt / clippy-driver discovery uses the +/// rustup toolchain dir alongside the active `cargo`). Used by AC3 and +/// AC4 to skip with a clear message when the rustfmt or clippy +/// toolchain component isn't installed, rather than failing the test +/// with a misleading pre-commit error. +fn cargo_subcommand_available(subcommand: &str) -> bool { + Command::new(cargo_bin()) + .env("PATH", cargo_augmented_path()) + .args([subcommand, "--version"]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + /// Run a command, capture stdout+stderr, return (status, combined output). fn run(cmd: &mut Command) -> (std::process::ExitStatus, String) { let out = cmd.output().expect("spawn command"); @@ -339,6 +354,20 @@ fn ac3_cargo_fmt_check_fails_on_misformatted_rust() { if !prereqs_available("ac3_cargo_fmt_check_fails_on_misformatted_rust") { return; } + // AC3 invokes `cargo fmt --check` via pre-commit. If the rustfmt + // toolchain component is missing, the hook would fail for an + // unrelated reason (cargo can't find rustfmt) and the test would + // surface as a misleading AC3 failure. Skip with a clear message + // instead — the dedicated `pre-commit` CI job installs the rustfmt + // component so it always runs there. + if !cargo_subcommand_available("fmt") { + eprintln!( + "skipping ac3_cargo_fmt_check_fails_on_misformatted_rust: \ + `cargo fmt` not available (rustfmt toolchain component missing). \ + Install with `rustup component add rustfmt`." + ); + return; + } let tmp = tempfile::tempdir().expect("create tempdir"); let repo = seed_tmp_repo(tmp.path()); @@ -400,6 +429,19 @@ fn ac4_manual_stage_invokes_clippy() { if !prereqs_available("ac4_manual_stage_invokes_clippy") { return; } + // AC4 invokes `cargo clippy` via pre-commit. If clippy-driver isn't + // installed, the hook fails for an unrelated reason and the test + // surfaces as a misleading AC4 failure. Skip cleanly instead — the + // dedicated CI job installs the clippy toolchain component so this + // test runs there. + if !cargo_subcommand_available("clippy") { + eprintln!( + "skipping ac4_manual_stage_invokes_clippy: \ + `cargo clippy` not available (clippy toolchain component missing). \ + Install with `rustup component add clippy`." + ); + return; + } let tmp = tempfile::tempdir().expect("create tempdir"); let repo = seed_tmp_repo(tmp.path()); From cfe89458281f5868d4120dd17f3670fdc8d396b6 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 20 May 2026 19:49:00 +0000 Subject: [PATCH 26/28] test(infra): ENG-4687 use Vec::contains over manual any() pattern Clippy 1.93 added the `manual_contains` lint, which fires on the `entries.iter().any(|line| *line == "...")` pattern in `cargo_fmt_check_hook_contract`. Both `entries.contains(&"...")` and the `iter().any(|line| *line == ...)` form compile to the same loop, but `contains` is more idiomatic. No behaviour change; cargo test --test pre_commit_config still 11/11 green. Reported by clippy CI on PR #8. Signed-off-by: Jimbo Freedman --- tests/pre_commit_config.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/pre_commit_config.rs b/tests/pre_commit_config.rs index 3ab1c6f..285e80f 100644 --- a/tests/pre_commit_config.rs +++ b/tests/pre_commit_config.rs @@ -203,9 +203,7 @@ fn cargo_fmt_check_hook_contract() { // trailing args (no `--all`, no extra manifest, etc.) — the // sub-package gets its own dedicated hook below. assert!( - entries - .iter() - .any(|line| *line == "entry: cargo fmt --check"), + entries.contains(&"entry: cargo fmt --check"), "cargo-fmt-check (root) hook entry must be exactly `cargo fmt --check` — \ without `--check`, the hook would silently reformat files instead of \ failing on drift; with extra args (e.g. `--all`) the coverage would \ @@ -215,9 +213,9 @@ fn cargo_fmt_check_hook_contract() { // The downstream hook must target the sub-package's manifest // explicitly and use `--check`. assert!( - entries.iter().any(|line| { - *line == "entry: cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check" - }), + entries.contains( + &"entry: cargo fmt --manifest-path tests/downstream_consumer/Cargo.toml --check" + ), "cargo-fmt-check-downstream hook entry must be exactly `cargo fmt \ --manifest-path tests/downstream_consumer/Cargo.toml --check` — without \ `--manifest-path`, cargo would find the root manifest and skip the \ From cd0770e03506b685c0910b38103d3d3720272643 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 22 Jul 2026 09:03:28 +0000 Subject: [PATCH 27/28] test(infra): ENG-4687 pass cargo-augmented PATH to AC2 pre-commit run AC2 invoked `pre-commit run --all-files` with the inherited PATH, while AC3/AC4 pass the cargo-augmented PATH so the fmt hooks can locate cargo/rustfmt on Windows (where they live next to the launching cargo, not on PATH). Align AC2 with AC3/AC4 for cross-platform reliability. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jimbo Freedman --- tests/pre_commit_e2e.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/pre_commit_e2e.rs b/tests/pre_commit_e2e.rs index 477c7cb..407eae6 100644 --- a/tests/pre_commit_e2e.rs +++ b/tests/pre_commit_e2e.rs @@ -276,12 +276,16 @@ fn ac2_pre_commit_run_all_files_passes_on_current_tree() { return; } - let (st, output) = run(Command::new("pre-commit").current_dir(&root).args([ - "run", - "--all-files", - "--color", - "never", - ])); + // Pre-commit's fmt hooks shell out to `cargo`/`rustfmt`; on Windows + // those live next to the launching `cargo`, not on the inherited + // PATH. Pass the cargo-augmented PATH (matching AC3/AC4) so AC2 is + // cross-platform. + let augmented_path = cargo_augmented_path(); + + let (st, output) = run(Command::new("pre-commit") + .current_dir(&root) + .env("PATH", &augmented_path) + .args(["run", "--all-files", "--color", "never"])); // If a hook is an auto-fixer (end-of-file-fixer, trailing-whitespace, // mixed-line-ending) it will both modify files AND return non-zero. From ae6c6adbda509104292e2d4aa4d9cf144440b682 Mon Sep 17 00:00:00 2001 From: Jimbo Freedman Date: Wed, 22 Jul 2026 09:12:11 +0000 Subject: [PATCH 28/28] ci: ENG-4687 cache pre-commit hook envs in pre-commit job Cache ~/.cache/pre-commit keyed on the .pre-commit-config.yaml hash so the pre-commit-hooks v5 repo clone and its venv are reused across runs instead of rebuilt every time, cutting CI time and reducing flakiness from transient network errors during hook environment setup. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jimbo Freedman --- .github/workflows/ci.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 31341bb..6e69be3 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -142,6 +142,15 @@ jobs: # id ..."); an unpinned upstream release could change that # wording and cause nondeterministic CI failures. - run: pip install 'pre-commit==4.6.0' + # Cache the hook environments pre-commit builds under + # ~/.cache/pre-commit (the pre-commit-hooks v5 repo clone + its + # venv). Keyed on the config hash so a hook/rev change busts the + # cache; avoids re-cloning on every run and reduces flakiness from + # transient network errors. + - uses: actions/cache@v4 + with: + path: ~/.cache/pre-commit + key: pre-commit-${{ runner.os }}-${{ hashFiles('.pre-commit-config.yaml') }} - uses: Swatinem/rust-cache@v2 with: shared-key: pre-commit