From b6d347b97d75bc1c11b39c733b988a3c2129768b Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 26 Jul 2026 19:12:05 +0200 Subject: [PATCH 1/6] Broaden static-regex lint to cover once_cell Lazy (#410) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `check-static-regexes` guard only rejected `LazyLock::new(... Regex::new(...))`, so a hand-rolled static regex wrapped in `once_cell::sync::Lazy::new(|| Regex::new(...))` slipped past `make lint`. Extract the scan into `scripts/check-static-regexes.sh` as the single source of truth and broaden its pattern to reject both supported lazy-wrapper constructors — `std::sync::LazyLock::new` and `once_cell::sync::Lazy::new` — whether spelled directly or fully qualified. `lazy_regex!` remains the sole sanctioned idiom. Add `tests/static_regex_lint.rs`, which drives the script against fixtures for every supported wrapper form, asserts a clean source passes, and asserts a ripgrep scan failure propagates its exit status. Fixtures live under `tests/data/static_regex/` with a `.rs.txt` extension so the guard does not match them in place. Co-Authored-By: Claude Opus 4.8 (1M context) --- Makefile | 8 +- docs/developers-guide.md | 5 + scripts/check-static-regexes.sh | 37 +++++ tests/data/static_regex/clean.rs.txt | 2 + .../data/static_regex/lazylock_direct.rs.txt | 1 + .../static_regex/lazylock_qualified.rs.txt | 1 + .../static_regex/once_cell_lazy_direct.rs.txt | 1 + .../once_cell_lazy_qualified.rs.txt | 1 + tests/static_regex_lint.rs | 134 ++++++++++++++++++ 9 files changed, 183 insertions(+), 7 deletions(-) create mode 100755 scripts/check-static-regexes.sh create mode 100644 tests/data/static_regex/clean.rs.txt create mode 100644 tests/data/static_regex/lazylock_direct.rs.txt create mode 100644 tests/data/static_regex/lazylock_qualified.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_direct.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_qualified.rs.txt create mode 100644 tests/static_regex_lint.rs diff --git a/Makefile b/Makefile index fe353d71..2737cce9 100644 --- a/Makefile +++ b/Makefile @@ -42,13 +42,7 @@ check-ripgrep: ## Verify ripgrep is available } check-static-regexes: check-ripgrep ## Reject hand-rolled static regular expressions - @status=0; \ - $(RG) -U --glob '*.rs' '\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*LazyLock::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' . || status=$$?; \ - case $$status in \ - 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; \ - 1) ;; \ - *) echo "failed to scan Rust sources (rg exit $$status)" >&2; exit $$status ;; \ - esac + @RG='$(RG)' scripts/check-static-regexes.sh . markdownlint: ## Lint Markdown files $(MDLINT) "**/*.md" diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 10431ae2..06e192ec 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -67,6 +67,11 @@ restores the separator row with widths derived from the final table body. - `check-static-regexes`: Runs before Clippy as part of `make lint` and uses ripgrep (`rg`) to reject hand-rolled static regular expression declarations. + The scan lives in `scripts/check-static-regexes.sh` and rejects any `static` + that wraps `Regex::new` directly in a supported lazy-wrapper constructor — + `std::sync::LazyLock::new` or `once_cell::sync::Lazy::new`, whether spelled + directly or fully qualified — so `lazy_regex!` remains the sole sanctioned + idiom. `tests/static_regex_lint.rs` exercises every supported form. Contributors must install ripgrep locally; Continuous Integration (CI) installs the pinned version before running the lint gate. diff --git a/scripts/check-static-regexes.sh b/scripts/check-static-regexes.sh new file mode 100755 index 00000000..b6f7a349 --- /dev/null +++ b/scripts/check-static-regexes.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Reject hand-rolled static regular expressions that bypass the `lazy_regex!` +# convention. +# +# The guard scans Rust sources for `static` declarations that wrap `Regex::new` +# directly in a supported lazy-wrapper constructor. Two wrapper families are +# supported, each matched whether spelled directly or fully qualified: +# +# * `std::sync::LazyLock::new` +# * `once_cell::sync::Lazy::new` +# +# Usage: check-static-regexes.sh [SCAN_DIR] +# +# SCAN_DIR defaults to the current directory. The RG environment variable +# overrides the ripgrep executable (default: `rg`). +# +# Exit status: +# 0 no prohibited declaration found +# 1 a prohibited declaration was found (diagnostic on stdout) +# * ripgrep failed to scan (diagnostic on stderr; rg's status propagated) +set -euo pipefail + +RG="${RG:-rg}" +scan_dir="${1:-.}" + +# `(?:[[:alnum:]_]+::)*` absorbs any module qualification (for example the +# `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and +# fully qualified spellings of each supported constructor are rejected. +pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' + +status=0 +"$RG" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? +case $status in + 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; + 1) exit 0 ;; + *) echo "failed to scan Rust sources (rg exit $status)" >&2; exit "$status" ;; +esac diff --git a/tests/data/static_regex/clean.rs.txt b/tests/data/static_regex/clean.rs.txt new file mode 100644 index 00000000..783f83b6 --- /dev/null +++ b/tests/data/static_regex/clean.rs.txt @@ -0,0 +1,2 @@ +static RE: LazyLock = lazy_regex!("clean"); +fn build() { let _ = Regex::new("local").unwrap(); } diff --git a/tests/data/static_regex/lazylock_direct.rs.txt b/tests/data/static_regex/lazylock_direct.rs.txt new file mode 100644 index 00000000..3760b95e --- /dev/null +++ b/tests/data/static_regex/lazylock_direct.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = LazyLock::new(|| Regex::new("a").unwrap()); diff --git a/tests/data/static_regex/lazylock_qualified.rs.txt b/tests/data/static_regex/lazylock_qualified.rs.txt new file mode 100644 index 00000000..758fb742 --- /dev/null +++ b/tests/data/static_regex/lazylock_qualified.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = std::sync::LazyLock::new(|| Regex::new("b").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_direct.rs.txt b/tests/data/static_regex/once_cell_lazy_direct.rs.txt new file mode 100644 index 00000000..a601be38 --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_direct.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = Lazy::new(|| Regex::new("c").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_qualified.rs.txt b/tests/data/static_regex/once_cell_lazy_qualified.rs.txt new file mode 100644 index 00000000..9b2879df --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_qualified.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = once_cell::sync::Lazy::new(|| Regex::new("d").unwrap()); diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs new file mode 100644 index 00000000..c1cad98a --- /dev/null +++ b/tests/static_regex_lint.rs @@ -0,0 +1,134 @@ +//! Regression coverage for the `check-static-regexes` lint guard. +//! +//! The guard lives in `scripts/check-static-regexes.sh` (invoked by the +//! Makefile's `check-static-regexes` target). It rejects hand-rolled static +//! regular expressions that bypass the `lazy_regex!` convention by wrapping +//! `Regex::new` directly in a supported lazy-wrapper constructor. +//! +//! These tests exercise the script directly so the Makefile and the tests +//! share a single source of truth for the scan. Every supported wrapper form +//! is asserted to be rejected, a clean fixture is asserted to pass, and a +//! ripgrep scan failure is asserted to propagate. +//! +//! Fixtures live under `tests/data/static_regex/` with a `.rs.txt` extension +//! so the guard (which scans `*.rs`) does not match the fixtures in place; the +//! tests copy each fixture into a temporary directory as a `.rs` file before +//! scanning. + +use std::{ + path::{Path, PathBuf}, + process::Command, +}; + +use rstest::rstest; +use tempfile::TempDir; + +/// The diagnostic emitted when a prohibited declaration is found. +const PROHIBITED_DIAGNOSTIC: &str = "static regular expressions must use lazy_regex!"; + +/// Every lazy-wrapper constructor form the guard must reject when it directly +/// wraps `Regex::new`. Each label maps to a fixture under +/// `tests/data/static_regex/` exercising one supported spelling: +/// +/// * `lazylock_direct` — `LazyLock::new(|| Regex::new(...))` +/// * `lazylock_qualified` — `std::sync::LazyLock::new(|| Regex::new(...))` +/// * `once_cell_lazy_direct` — `Lazy::new(|| Regex::new(...))` +/// * `once_cell_lazy_qualified` — `once_cell::sync::Lazy::new(|| Regex::new(...))` +const PROHIBITED_FORMS: &[&str] = &[ + "lazylock_direct", + "lazylock_qualified", + "once_cell_lazy_direct", + "once_cell_lazy_qualified", +]; + +fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } + +fn script_path() -> PathBuf { manifest_dir().join("scripts/check-static-regexes.sh") } + +fn fixture(label: &str) -> String { + let path = manifest_dir().join(format!("tests/data/static_regex/{label}.rs.txt")); + std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) +} + +/// Materialise `label`'s fixture as a `.rs` file inside a fresh temp directory. +fn scan_dir_with(label: &str) -> TempDir { + let dir = TempDir::new().expect("failed to create temp dir"); + std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) + .expect("failed to write fixture into temp dir"); + dir +} + +/// Run the guard against `scan_dir`, optionally overriding the ripgrep binary +/// via the `RG` environment variable. +fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { + let mut cmd = Command::new(script_path()); + cmd.arg(scan_dir); + if let Some(rg) = rg { + cmd.env("RG", rg); + } + cmd.output() + .expect("failed to execute check-static-regexes.sh") +} + +#[rstest] +fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3)] index: usize) { + let label = PROHIBITED_FORMS[index]; + let dir = scan_dir_with(label); + + let output = run_guard(dir.path(), None); + + assert_eq!( + output.status.code(), + Some(1), + "form `{label}` should be rejected with status 1" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains(PROHIBITED_DIAGNOSTIC), + "form `{label}` should emit the prohibited diagnostic, got: {stdout}" + ); +} + +#[test] +fn accepts_clean_sources() { + // The sanctioned `lazy_regex!` idiom plus an unrelated non-static + // `Regex::new` call that must not trip the guard. + let dir = scan_dir_with("clean"); + + let output = run_guard(dir.path(), None); + + assert_eq!( + output.status.code(), + Some(0), + "clean sources should pass; stdout: {}", + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn propagates_ripgrep_scan_failure() { + let dir = TempDir::new().expect("failed to create temp dir"); + // A stub standing in for ripgrep that fails with a distinctive status. + let stub = dir.path().join("rg-stub.sh"); + std::fs::write(&stub, "#!/bin/sh\nexit 3\n").expect("failed to write stub"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)) + .expect("failed to chmod stub"); + } + + let output = run_guard(dir.path(), Some(&stub)); + + assert_eq!( + output.status.code(), + Some(3), + "a ripgrep scan failure should propagate its exit status" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("failed to scan Rust sources (rg exit 3)"), + "scan failure should emit the scan-failure diagnostic, got: {stderr}" + ); +} From d5f1f26bf304f014b38323de86f3bdf3faaa7696 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 27 Jul 2026 20:04:06 +0200 Subject: [PATCH 2/6] Harden static-regex guard: RG args and move closures (#410) Address review findings on the broadened static-regex lint: - Restore support for `RG` overrides that carry arguments (for example `RG='rg --pcre2'`). Extracting the scan into the shell script replaced the Makefile's word-splitting `$(RG)` expansion with a quoted `"$RG"`, which treated the whole value as one executable name. Split `RG` into an array so arguments are preserved, matching `check-ripgrep`'s `firstword` handling. - Reject `move` closures. The pattern only matched `|| Regex::new(...)`, so a hand-rolled `LazyLock::new(move || Regex::new(...))` (or the `once_cell` `Lazy` equivalent) slipped through. Add `(?:move\s+)?` and fixtures for both wrapper families. - Make the regression tests deterministic: `run_guard` now clears any ambient `RG` on default-path runs so the guard's own `rg` default is exercised. Skipped the suggestion to migrate the test to camino/cap-std path types: it would add two dependencies absent from the tree and diverge from the existing test suite, which uses std::fs and tempfile. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/check-static-regexes.sh | 11 +++++++---- tests/data/static_regex/lazylock_move.rs.txt | 1 + .../data/static_regex/once_cell_lazy_move.rs.txt | 1 + tests/static_regex_lint.rs | 15 +++++++++++---- 4 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 tests/data/static_regex/lazylock_move.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_move.rs.txt diff --git a/scripts/check-static-regexes.sh b/scripts/check-static-regexes.sh index b6f7a349..987311eb 100755 --- a/scripts/check-static-regexes.sh +++ b/scripts/check-static-regexes.sh @@ -12,7 +12,9 @@ # Usage: check-static-regexes.sh [SCAN_DIR] # # SCAN_DIR defaults to the current directory. The RG environment variable -# overrides the ripgrep executable (default: `rg`). +# overrides the ripgrep command (default: `rg`). It is split on whitespace, so +# it may carry arguments — for example `RG='rg --pcre2'` — matching the way the +# Makefile's `$(RG)` expansion behaved before the scan was extracted here. # # Exit status: # 0 no prohibited declaration found @@ -20,16 +22,17 @@ # * ripgrep failed to scan (diagnostic on stderr; rg's status propagated) set -euo pipefail -RG="${RG:-rg}" +read -r -a rg_cmd <<<"${RG:-rg}" scan_dir="${1:-.}" # `(?:[[:alnum:]_]+::)*` absorbs any module qualification (for example the # `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and # fully qualified spellings of each supported constructor are rejected. -pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' +# `(?:move\s+)?` covers `move` closures such as `LazyLock::new(move || ...)`. +pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*(?:move\s+)?\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' status=0 -"$RG" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? +"${rg_cmd[@]}" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? case $status in 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; 1) exit 0 ;; diff --git a/tests/data/static_regex/lazylock_move.rs.txt b/tests/data/static_regex/lazylock_move.rs.txt new file mode 100644 index 00000000..75a1b363 --- /dev/null +++ b/tests/data/static_regex/lazylock_move.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = LazyLock::new(move || Regex::new("e").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_move.rs.txt b/tests/data/static_regex/once_cell_lazy_move.rs.txt new file mode 100644 index 00000000..59ab1dd7 --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_move.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = once_cell::sync::Lazy::new(move || Regex::new("f").unwrap()); diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index c1cad98a..21771321 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -32,13 +32,17 @@ const PROHIBITED_DIAGNOSTIC: &str = "static regular expressions must use lazy_re /// /// * `lazylock_direct` — `LazyLock::new(|| Regex::new(...))` /// * `lazylock_qualified` — `std::sync::LazyLock::new(|| Regex::new(...))` +/// * `lazylock_move` — `LazyLock::new(move || Regex::new(...))` /// * `once_cell_lazy_direct` — `Lazy::new(|| Regex::new(...))` /// * `once_cell_lazy_qualified` — `once_cell::sync::Lazy::new(|| Regex::new(...))` +/// * `once_cell_lazy_move` — `once_cell::sync::Lazy::new(move || Regex::new(...))` const PROHIBITED_FORMS: &[&str] = &[ "lazylock_direct", "lazylock_qualified", + "lazylock_move", "once_cell_lazy_direct", "once_cell_lazy_qualified", + "once_cell_lazy_move", ]; fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } @@ -64,15 +68,18 @@ fn scan_dir_with(label: &str) -> TempDir { fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { let mut cmd = Command::new(script_path()); cmd.arg(scan_dir); - if let Some(rg) = rg { - cmd.env("RG", rg); - } + // Control the ripgrep dependency explicitly: override it for `Some`, and + // clear any ambient `RG` for `None` so default-path runs are deterministic. + match rg { + Some(rg) => cmd.env("RG", rg), + None => cmd.env_remove("RG"), + }; cmd.output() .expect("failed to execute check-static-regexes.sh") } #[rstest] -fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3)] index: usize) { +fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3, 4, 5)] index: usize) { let label = PROHIBITED_FORMS[index]; let dir = scan_dir_with(label); From f89dde6ea6f31ee7ed801c305a190fd1509ec7f7 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 31 Jul 2026 02:16:53 +0200 Subject: [PATCH 3/6] Cover argument-bearing RG overrides (#410) Address review findings on the static-regex lint regression tests: - Widen `run_guard`'s `rg` parameter from `Option<&Path>` to `Option<&str>` so tests can supply an `RG` value that carries arguments, and add `preserves_arguments_supplied_through_rg`. It drives the guard with `RG=' --pcre2'` and asserts, via a stub that records its argv, that the override's arguments are forwarded ahead of the guard's own and that the scan directory remains last. Reverting the array split makes this test fail with exit 127, the exact symptom the fix removed. - Extract the executable-stub setup into a `write_stub` helper shared by the scan-failure and argument-preservation tests. - Spell the `scan_dir_with` doc comment "Materialize", matching the repository's en-GB-oxendict Oxford-spelling convention. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/static_regex_lint.rs | 89 +++++++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 15 deletions(-) diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index 21771321..f91d86a0 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -55,7 +55,7 @@ fn fixture(label: &str) -> String { .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) } -/// Materialise `label`'s fixture as a `.rs` file inside a fresh temp directory. +/// Materialize `label`'s fixture as a `.rs` file inside a fresh temp directory. fn scan_dir_with(label: &str) -> TempDir { let dir = TempDir::new().expect("failed to create temp dir"); std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) @@ -63,13 +63,16 @@ fn scan_dir_with(label: &str) -> TempDir { dir } -/// Run the guard against `scan_dir`, optionally overriding the ripgrep binary -/// via the `RG` environment variable. -fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { +/// Run the guard against `scan_dir`, optionally overriding the `RG` ripgrep +/// command. +/// +/// `rg` is the raw `RG` value, so it may carry arguments (for example +/// `rg --pcre2`); the guard splits it on whitespace. Passing `None` clears any +/// ambient `RG` so default-path runs exercise the guard's own `rg` default +/// deterministically. +fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { let mut cmd = Command::new(script_path()); cmd.arg(scan_dir); - // Control the ripgrep dependency explicitly: override it for `Some`, and - // clear any ambient `RG` for `None` so default-path runs are deterministic. match rg { Some(rg) => cmd.env("RG", rg), None => cmd.env_remove("RG"), @@ -78,6 +81,19 @@ fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { .expect("failed to execute check-static-regexes.sh") } +/// Write `script` to `/` and mark it executable. +fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, script).expect("failed to write stub"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("failed to chmod stub"); + } + path +} + #[rstest] fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3, 4, 5)] index: usize) { let label = PROHIBITED_FORMS[index]; @@ -117,16 +133,9 @@ fn accepts_clean_sources() { fn propagates_ripgrep_scan_failure() { let dir = TempDir::new().expect("failed to create temp dir"); // A stub standing in for ripgrep that fails with a distinctive status. - let stub = dir.path().join("rg-stub.sh"); - std::fs::write(&stub, "#!/bin/sh\nexit 3\n").expect("failed to write stub"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)) - .expect("failed to chmod stub"); - } + let stub = write_stub(dir.path(), "rg-stub.sh", "#!/bin/sh\nexit 3\n"); - let output = run_guard(dir.path(), Some(&stub)); + let output = run_guard(dir.path(), Some(&stub.display().to_string())); assert_eq!( output.status.code(), @@ -139,3 +148,53 @@ fn propagates_ripgrep_scan_failure() { "scan failure should emit the scan-failure diagnostic, got: {stderr}" ); } + +/// An `RG` override may carry arguments — the Makefile's `$(RG)` expansion +/// supported this before the scan moved into the script, and `check-ripgrep` +/// still validates only `$(firstword $(RG))`. The guard must therefore split +/// `RG` on whitespace and forward the extra arguments to ripgrep ahead of its +/// own, rather than treating the whole value as one executable name. +#[test] +fn preserves_arguments_supplied_through_rg() { + let dir = TempDir::new().expect("failed to create temp dir"); + let argv_log = dir.path().join("argv.txt"); + // A stub that records its argv, then reports "no matches" so the guard + // takes its clean-scan path. + let stub = write_stub( + dir.path(), + "rg-stub.sh", + &format!( + "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > '{}'\nexit 1\n", + argv_log.display() + ), + ); + + let output = run_guard(dir.path(), Some(&format!("{} --pcre2", stub.display()))); + + assert_eq!( + output.status.code(), + Some(0), + "an argument-bearing RG override must still run; stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let argv: Vec = std::fs::read_to_string(&argv_log) + .expect("stub should have recorded its argv") + .lines() + .map(str::to_owned) + .collect(); + assert_eq!( + argv.first().map(String::as_str), + Some("--pcre2"), + "the RG override's own arguments must be forwarded first, got: {argv:?}" + ); + assert!( + argv.contains(&"-U".to_owned()) && argv.contains(&"--glob".to_owned()), + "the guard's own ripgrep arguments must follow, got: {argv:?}" + ); + assert_eq!( + argv.last().map(String::as_str), + Some(dir.path().to_str().expect("temp dir path should be UTF-8")), + "the scan directory must remain the final argument, got: {argv:?}" + ); +} From 668322857e80ce479bf14a1d4f6cd479fe76d6b4 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 2 Aug 2026 09:50:12 +0200 Subject: [PATCH 4/6] Harden argv-recording stub against quoted paths (#410) The `preserves_arguments_supplied_through_rg` stub interpolated a Rust-side path into single-quoted shell source, so a `TMPDIR` containing a single quote would terminate the quote early and produce an unparsable script. Derive the log path from `$0` inside the stub instead, writing to `"$0.argv"` so no path is interpolated into the script source at all, and build the matching path in Rust by appending to the stub's `OsString`. Parameterize the test over a plain and a single-quote-bearing scan directory to lock the behaviour in. Reverting the stub to the interpolated form fails the new case with `unexpected EOF while looking for matching '`, the exact symptom the fix removes. The directory name omits whitespace because `RG` is split on it by design. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/static_regex_lint.rs | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index f91d86a0..dfc68fc7 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -154,22 +154,34 @@ fn propagates_ripgrep_scan_failure() { /// still validates only `$(firstword $(RG))`. The guard must therefore split /// `RG` on whitespace and forward the extra arguments to ripgrep ahead of its /// own, rather than treating the whole value as one executable name. -#[test] -fn preserves_arguments_supplied_through_rg() { - let dir = TempDir::new().expect("failed to create temp dir"); - let argv_log = dir.path().join("argv.txt"); +/// +/// The `single_quote` case pins the stub's own quoting: the log path is derived +/// from `$0` inside the script rather than interpolated into its source, so a +/// `TMPDIR` containing a single quote cannot break the recording. (The +/// directory name deliberately omits whitespace, since `RG` is split on it.) +#[rstest] +#[case::plain("scan")] +#[case::single_quote("scan's")] +fn preserves_arguments_supplied_through_rg(#[case] dir_name: &str) { + let root = TempDir::new().expect("failed to create temp dir"); + let dir = root.path().join(dir_name); + std::fs::create_dir(&dir).expect("failed to create scan dir"); + // A stub that records its argv, then reports "no matches" so the guard - // takes its clean-scan path. + // takes its clean-scan path. It writes beside itself via `"$0.argv"`, so no + // path is interpolated into the script source. let stub = write_stub( - dir.path(), + &dir, "rg-stub.sh", - &format!( - "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > '{}'\nexit 1\n", - argv_log.display() - ), + "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > \"$0.argv\"\nexit 1\n", ); + let argv_log = { + let mut path = stub.clone().into_os_string(); + path.push(".argv"); + PathBuf::from(path) + }; - let output = run_guard(dir.path(), Some(&format!("{} --pcre2", stub.display()))); + let output = run_guard(&dir, Some(&format!("{} --pcre2", stub.display()))); assert_eq!( output.status.code(), @@ -194,7 +206,7 @@ fn preserves_arguments_supplied_through_rg() { ); assert_eq!( argv.last().map(String::as_str), - Some(dir.path().to_str().expect("temp dir path should be UTF-8")), + Some(dir.to_str().expect("temp dir path should be UTF-8")), "the scan directory must remain the final argument, got: {argv:?}" ); } From 071b91887f331ee344e90acd17cf0a5f62725240 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 2 Aug 2026 10:01:34 +0200 Subject: [PATCH 5/6] Add property coverage for the static-regex guard (#410) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fixtures pinned the specific spellings named in the issue, but the guard's actual contract is a syntax-matching invariant over a family of declarations: either supported wrapper, any module qualification, any `\s*`-legal whitespace (including newlines, since the scan runs with `-U`), with or without `move`, and with or without a braced closure body. Generate across that space and assert rejection, and generate across clearly sanctioned forms — the `lazy_regex!` macro, non-static bindings, a supported wrapper around a non-regex value, an unsupported wrapper, and a function reference rather than a closure — and assert acceptance. Each case writes several generated declarations into one directory and scans once, then asserts every generated file is named in the guard's output, so process spawns scale with cases rather than declarations while each declaration is still checked individually. Mutation-testing the pattern confirms the properties add reach beyond the fixtures. Of six narrowing mutations, the fixtures caught three; the properties caught all six, including the three the fixtures cannot see: dropping the optional brace group, and dropping `\s*` after `new` or after the opening parenthesis. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/static_regex_lint.rs | 147 +++++++++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index dfc68fc7..08ac575f 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -20,6 +20,7 @@ use std::{ process::Command, }; +use proptest::prelude::*; use rstest::rstest; use tempfile::TempDir; @@ -210,3 +211,149 @@ fn preserves_arguments_supplied_through_rg(#[case] dir_name: &str) { "the scan directory must remain the final argument, got: {argv:?}" ); } + +// --------------------------------------------------------------------------- +// Property-based coverage +// +// The fixtures above pin the specific spellings named in the issue. The guard's +// real contract, though, is a syntax-matching invariant over a whole family of +// declarations: either supported wrapper, any module qualification, any +// `\s*`-legal whitespace (including newlines, since the scan runs with `-U`), +// with or without `move`, and with or without a braced closure body. The +// properties below generate across that space and assert rejection, and +// generate across clearly-sanctioned forms and assert acceptance. +// +// Each case writes several generated declarations into one directory and runs +// the guard once, then asserts every generated file is named in the output. +// That keeps process spawns proportional to cases rather than declarations +// while still checking each declaration individually. +// --------------------------------------------------------------------------- + +/// Whitespace runs for positions where the guard's pattern allows `\s*`. +fn optional_ws() -> impl Strategy { + prop::sample::select(vec!["", " ", " ", "\t", "\n", "\n "]).prop_map(str::to_owned) +} + +/// Whitespace runs for positions requiring at least one space, such as the +/// gap after a `move` keyword. +fn required_ws() -> impl Strategy { + prop::sample::select(vec![" ", " ", "\t", "\n", "\n "]).prop_map(str::to_owned) +} + +/// A possibly-empty module qualification such as `once_cell::sync::`. +fn qualifier() -> impl Strategy { + prop::collection::vec("[a-z][a-z0-9_]{0,6}", 0..3).prop_map(|segments| { + segments.iter().fold(String::new(), |mut acc, segment| { + acc.push_str(segment); + acc.push_str("::"); + acc + }) + }) +} + +prop_compose! { + /// A static declaration that wraps `Regex::new` in a supported lazy + /// wrapper, and so must always be rejected. + fn prohibited_declaration()( + name in "[A-Z][A-Z0-9_]{0,7}", + wrapper in prop::sample::select(vec!["LazyLock", "Lazy"]), + wrapper_qualifier in qualifier(), + regex_qualifier in qualifier(), + pattern in "[a-z0-9]{1,8}", + after_eq in optional_ws(), + after_new in optional_ws(), + after_paren in optional_ws(), + after_closure in optional_ws(), + move_gap in prop::option::of(required_ws()), + braced in any::(), + ) -> String { + let move_kw = move_gap.map_or_else(String::new, |gap| format!("move{gap}")); + let (open, close) = if braced { ("{ ", " }") } else { ("", "") }; + format!( + "static {name}: {wrapper} ={after_eq}{wrapper_qualifier}{wrapper}::new\ + {after_new}({after_paren}{move_kw}||{after_closure}{open}\ + {regex_qualifier}Regex::new(\"{pattern}\").unwrap(){close});\n" + ) + } +} + +prop_compose! { + /// A declaration the guard must leave alone: the sanctioned macro, a + /// non-static binding, a supported wrapper around a non-regex value, an + /// unsupported wrapper, or a function reference rather than a closure. + fn clean_declaration()( + name in "[A-Z][A-Z0-9_]{0,7}", + pattern in "[a-z0-9]{1,8}", + form in 0_usize..5, + ) -> String { + match form { + 0 => format!("static {name}: LazyLock = lazy_regex!(\"{pattern}\");\n"), + 1 => format!("fn build_{name}() {{ let re = Regex::new(\"{pattern}\").unwrap(); }}\n"), + 2 => format!("static {name}: LazyLock = LazyLock::new(|| String::new());\n"), + 3 => format!("static {name}: OnceLock = OnceLock::new();\n"), + _ => format!("static {name}: LazyLock = LazyLock::new(make_{name});\n"), + } + } +} + +/// Write each declaration to its own `.rs` file and scan the directory once. +fn scan_generated(declarations: &[String]) -> (TempDir, std::process::Output) { + let dir = TempDir::new().expect("failed to create temp dir"); + for (index, declaration) in declarations.iter().enumerate() { + std::fs::write(dir.path().join(format!("case{index}.rs")), declaration) + .expect("failed to write generated source"); + } + let output = run_guard(dir.path(), None); + (dir, output) +} + +proptest! { + #![proptest_config(ProptestConfig::with_cases(64))] + + /// Every declaration in the supported wrapper family is rejected, whatever + /// its qualification, whitespace, `move` keyword, or closure body shape. + #[test] + fn rejects_generated_prohibited_declarations( + declarations in prop::collection::vec(prohibited_declaration(), 1..5), + ) { + let (_dir, output) = scan_generated(&declarations); + + prop_assert_eq!( + output.status.code(), + Some(1), + "generated declarations should be rejected: {:?}", + declarations + ); + let stdout = String::from_utf8_lossy(&output.stdout); + prop_assert!(stdout.contains(PROHIBITED_DIAGNOSTIC)); + // Assert per declaration, so one matching file cannot mask the rest. + for (index, declaration) in declarations.iter().enumerate() { + prop_assert!( + stdout.contains(&format!("case{index}.rs")), + "declaration {} went undetected: {:?}\nguard output: {stdout}", + index, + declaration + ); + } + } + + /// Sanctioned and unrelated declarations never trip the guard. + #[test] + fn accepts_generated_clean_declarations( + declarations in prop::collection::vec(clean_declaration(), 1..5), + ) { + let (_dir, output) = scan_generated(&declarations); + + prop_assert_eq!( + output.status.code(), + Some(0), + "clean declarations should pass: {:?}\nguard output: {}", + declarations, + String::from_utf8_lossy(&output.stdout) + ); + } +} + +// Bounded model checking is unsuitable here because the guard is a ripgrep +// pattern over unbounded Rust source text rather than a bounded state machine; +// property testing exercises that input domain directly. From f5eadacab0a82c7bc01245dbce462546841567bd Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 3 Aug 2026 00:57:28 +0200 Subject: [PATCH 6/6] Migrate static-regex lint tests to camino and cap-std (#410) Address the outstanding review finding by moving the guard's regression tests off `std::path` and `std::fs`. Paths are now `camino::Utf8Path`/`Utf8PathBuf`, and every filesystem operation goes through a `cap_std::fs_utf8::Dir` capability scoped to the directory it touches, so fixture reads and temporary writes cannot stray outside the manifest or temp directory they belong to. `cap-std`'s `fs_utf8` module uses camino types natively, so the two libraries compose without an adapter layer. Behaviour is preserved. `std::process::Command` still executes the guard and `tempfile::TempDir` still supplies the isolated directories; the sole remaining `std::path` reference is a documented `utf8()` adapter that converts `TempDir`'s ambient path, failing loudly rather than lossily on non-UTF-8. Executable bits now use `cap_std::fs::PermissionsExt::from_mode`, dropping the `std::os::unix` import. The UTF-8 guarantee also removes two lossy conversions: the scan-directory assertion uses `as_str()` instead of `to_str().expect(...)`, and the argv log is read by relative name through the scoped capability rather than by reconstructing a path through `OsString`. No dependency changes are needed: `main` already carries `camino` and `cap-std` (with `fs_utf8`) as production dependencies for the CLI's capability-scoped file I/O, and integration tests can use them from there. Mutation testing after the migration confirms the suite retains its reach: the fixtures still catch a dropped `|Lazy` alternation, and the properties still catch a dropped `\s*` that the fixtures cannot see. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/static_regex_lint.rs | 104 ++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 35 deletions(-) diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index 08ac575f..5dd92384 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -14,12 +14,18 @@ //! so the guard (which scans `*.rs`) does not match the fixtures in place; the //! tests copy each fixture into a temporary directory as a `.rs` file before //! scanning. +//! +//! Paths are [`camino`] UTF-8 types and every filesystem operation goes +//! through a [`cap_std::fs_utf8::Dir`] capability scoped to the directory it +//! touches, so reads and writes cannot stray outside the manifest or the +//! temporary directory they belong to. [`TempDir`] still provides the isolated +//! directories and [`Command`] still runs the guard; only the path and +//! filesystem layers change. -use std::{ - path::{Path, PathBuf}, - process::Command, -}; +use std::process::Command; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ambient_authority, fs_utf8::Dir}; use proptest::prelude::*; use rstest::rstest; use tempfile::TempDir; @@ -46,20 +52,40 @@ const PROHIBITED_FORMS: &[&str] = &[ "once_cell_lazy_move", ]; -fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } +/// Adapt an ambient [`std::path::Path`] — as produced by [`TempDir::path`] — +/// into a UTF-8 path, failing loudly rather than lossily if it is not UTF-8. +fn utf8(path: &std::path::Path) -> &Utf8Path { + Utf8Path::from_path(path).expect("temporary directory path should be UTF-8") +} + +/// Open a filesystem capability scoped to `dir`. +/// +/// Every subsequent operation names a path relative to this handle, so it +/// cannot reach outside `dir`. +fn open_dir(dir: &Utf8Path) -> Dir { + Dir::open_ambient_dir(dir, ambient_authority()) + .unwrap_or_else(|e| panic!("failed to open directory {dir}: {e}")) +} -fn script_path() -> PathBuf { manifest_dir().join("scripts/check-static-regexes.sh") } +/// The crate root, used as the capability root for reading fixtures. +fn manifest_dir() -> Utf8PathBuf { Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")) } +/// The guard script under test. +fn script_path() -> Utf8PathBuf { manifest_dir().join("scripts/check-static-regexes.sh") } + +/// Read `label`'s fixture through a capability scoped to the crate root. fn fixture(label: &str) -> String { - let path = manifest_dir().join(format!("tests/data/static_regex/{label}.rs.txt")); - std::fs::read_to_string(&path) - .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) + let relative = format!("tests/data/static_regex/{label}.rs.txt"); + open_dir(&manifest_dir()) + .read_to_string(&relative) + .unwrap_or_else(|e| panic!("failed to read fixture {relative}: {e}")) } /// Materialize `label`'s fixture as a `.rs` file inside a fresh temp directory. fn scan_dir_with(label: &str) -> TempDir { let dir = TempDir::new().expect("failed to create temp dir"); - std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) + open_dir(utf8(dir.path())) + .write(format!("{label}.rs"), fixture(label)) .expect("failed to write fixture into temp dir"); dir } @@ -71,7 +97,7 @@ fn scan_dir_with(label: &str) -> TempDir { /// `rg --pcre2`); the guard splits it on whitespace. Passing `None` clears any /// ambient `RG` so default-path runs exercise the guard's own `rg` default /// deterministically. -fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { +fn run_guard(scan_dir: &Utf8Path, rg: Option<&str>) -> std::process::Output { let mut cmd = Command::new(script_path()); cmd.arg(scan_dir); match rg { @@ -82,17 +108,21 @@ fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { .expect("failed to execute check-static-regexes.sh") } -/// Write `script` to `/` and mark it executable. -fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { - let path = dir.join(name); - std::fs::write(&path, script).expect("failed to write stub"); +/// Write `script` to `/`, mark it executable, and return its path. +/// +/// Both operations go through a capability scoped to `dir`, so `name` is +/// resolved relative to that directory rather than against ambient authority. +fn write_stub(dir: &Utf8Path, name: &str, script: &str) -> Utf8PathBuf { + let handle = open_dir(dir); + handle.write(name, script).expect("failed to write stub"); #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + use cap_std::fs::{Permissions, PermissionsExt}; + handle + .set_permissions(name, Permissions::from_mode(0o755)) .expect("failed to chmod stub"); } - path + dir.join(name) } #[rstest] @@ -100,7 +130,7 @@ fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3, 4, 5)] index: usize let label = PROHIBITED_FORMS[index]; let dir = scan_dir_with(label); - let output = run_guard(dir.path(), None); + let output = run_guard(utf8(dir.path()), None); assert_eq!( output.status.code(), @@ -120,7 +150,7 @@ fn accepts_clean_sources() { // `Regex::new` call that must not trip the guard. let dir = scan_dir_with("clean"); - let output = run_guard(dir.path(), None); + let output = run_guard(utf8(dir.path()), None); assert_eq!( output.status.code(), @@ -133,10 +163,11 @@ fn accepts_clean_sources() { #[test] fn propagates_ripgrep_scan_failure() { let dir = TempDir::new().expect("failed to create temp dir"); + let scan_dir = utf8(dir.path()); // A stub standing in for ripgrep that fails with a distinctive status. - let stub = write_stub(dir.path(), "rg-stub.sh", "#!/bin/sh\nexit 3\n"); + let stub = write_stub(scan_dir, "rg-stub.sh", "#!/bin/sh\nexit 3\n"); - let output = run_guard(dir.path(), Some(&stub.display().to_string())); + let output = run_guard(scan_dir, Some(stub.as_str())); assert_eq!( output.status.code(), @@ -164,25 +195,25 @@ fn propagates_ripgrep_scan_failure() { #[case::plain("scan")] #[case::single_quote("scan's")] fn preserves_arguments_supplied_through_rg(#[case] dir_name: &str) { + const STUB_NAME: &str = "rg-stub.sh"; + let root = TempDir::new().expect("failed to create temp dir"); - let dir = root.path().join(dir_name); - std::fs::create_dir(&dir).expect("failed to create scan dir"); + let root_dir = utf8(root.path()); + open_dir(root_dir) + .create_dir(dir_name) + .expect("failed to create scan dir"); + let dir = root_dir.join(dir_name); // A stub that records its argv, then reports "no matches" so the guard // takes its clean-scan path. It writes beside itself via `"$0.argv"`, so no // path is interpolated into the script source. let stub = write_stub( &dir, - "rg-stub.sh", + STUB_NAME, "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > \"$0.argv\"\nexit 1\n", ); - let argv_log = { - let mut path = stub.clone().into_os_string(); - path.push(".argv"); - PathBuf::from(path) - }; - let output = run_guard(&dir, Some(&format!("{} --pcre2", stub.display()))); + let output = run_guard(&dir, Some(&format!("{stub} --pcre2"))); assert_eq!( output.status.code(), @@ -191,7 +222,8 @@ fn preserves_arguments_supplied_through_rg(#[case] dir_name: &str) { String::from_utf8_lossy(&output.stderr) ); - let argv: Vec = std::fs::read_to_string(&argv_log) + let argv: Vec = open_dir(&dir) + .read_to_string(format!("{STUB_NAME}.argv")) .expect("stub should have recorded its argv") .lines() .map(str::to_owned) @@ -207,7 +239,7 @@ fn preserves_arguments_supplied_through_rg(#[case] dir_name: &str) { ); assert_eq!( argv.last().map(String::as_str), - Some(dir.to_str().expect("temp dir path should be UTF-8")), + Some(dir.as_str()), "the scan directory must remain the final argument, got: {argv:?}" ); } @@ -299,11 +331,13 @@ prop_compose! { /// Write each declaration to its own `.rs` file and scan the directory once. fn scan_generated(declarations: &[String]) -> (TempDir, std::process::Output) { let dir = TempDir::new().expect("failed to create temp dir"); + let handle = open_dir(utf8(dir.path())); for (index, declaration) in declarations.iter().enumerate() { - std::fs::write(dir.path().join(format!("case{index}.rs")), declaration) + handle + .write(format!("case{index}.rs"), declaration) .expect("failed to write generated source"); } - let output = run_guard(dir.path(), None); + let output = run_guard(utf8(dir.path()), None); (dir, output) }