From 77a12739534137e8581241722df8ba619730b2bb Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 24 Aug 2026 15:37:07 +0200 Subject: [PATCH 1/3] Inject environment readers (#177) (#221) Replace process-global HNSW test mutation with MockEnv-backed configuration injection, so property tests can run concurrently without unsafe access. Route environment-variable reads through mockable::Env across the workspace and deny the direct std::env methods to prevent regression. --- Cargo.toml | 2 + chutoro-benches/Cargo.toml | 2 + chutoro-benches/benches/hnsw.rs | 26 ++++-- chutoro-benches/benches/hnsw_ef_sweep.rs | 37 ++++++-- chutoro-benches/src/criterion_support.rs | 7 +- .../src/neighbour_scoring/benchmark_runner.rs | 8 +- .../src/neighbour_scoring/build_profile.rs | 24 +++-- chutoro-benches/src/source/mnist/mod.rs | 14 +-- chutoro-benches/src/source/mnist/tests.rs | 4 +- chutoro-cli/Cargo.toml | 1 + chutoro-cli/src/logging.rs | 7 +- chutoro-core/Cargo.toml | 1 + .../src/hnsw/tests/property/search_config.rs | 92 +++++++++---------- chutoro-core/src/hnsw/tests/support.rs | 10 +- chutoro-core/src/mst/property/types.rs | 9 +- chutoro-providers/dense/Cargo.toml | 2 + chutoro-providers/dense/build.rs | 10 +- chutoro-test-support/Cargo.toml | 2 + .../src/bin/benchmark_regression_gate.rs | 11 ++- .../src/bin/kani_nightly_gate.rs | 17 ++-- .../src/ci/benchmark_regression_profile.rs | 52 +++++++++-- .../src/ci/property_test_profile.rs | 40 ++++---- chutoro-test-support/src/process.rs | 9 +- clippy.toml | 9 ++ .../feat-align-workspace-lint-policy.md | 7 ++ 25 files changed, 262 insertions(+), 141 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f86d3476..7f056d40 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,9 +22,11 @@ arrow-array = "59.2.0" arrow-schema = "59.2.0" num-traits = "0.2.19" parquet = "59.2.0" +mockable = "3.0.0" [workspace.lints.clippy] pedantic = { level = "warn", priority = -1 } +disallowed_methods = "deny" # 1. hygiene allow_attributes = "deny" diff --git a/chutoro-benches/Cargo.toml b/chutoro-benches/Cargo.toml index 1796eb9f..eb8a0995 100644 --- a/chutoro-benches/Cargo.toml +++ b/chutoro-benches/Cargo.toml @@ -13,6 +13,7 @@ chutoro-core = { path = "../chutoro-core" } chutoro-providers-dense = { path = "../chutoro-providers/dense" } criterion = { version = "0.5.1", features = ["html_reports"] } flate2 = "1.1.9" +mockable = { workspace = true } rand = { version = "0.8.5", features = ["small_rng"] } strsim = "0.11.1" thiserror = "2.0.17" @@ -59,6 +60,7 @@ harness = false # and updated to stay in sync. [lints.clippy] pedantic = { level = "warn", priority = -1 } +disallowed_methods = "deny" # 1. hygiene allow_attributes = "deny" diff --git a/chutoro-benches/benches/hnsw.rs b/chutoro-benches/benches/hnsw.rs index f144e532..2cbd5f77 100644 --- a/chutoro-benches/benches/hnsw.rs +++ b/chutoro-benches/benches/hnsw.rs @@ -9,6 +9,7 @@ use criterion::{ BatchSize, BenchmarkGroup, BenchmarkId, Criterion, black_box, criterion_main, measurement::WallTime, }; +use mockable::{DefaultEnv, Env}; use chutoro_benches::{ criterion_support::{ @@ -257,8 +258,8 @@ fn hnsw_build(c: &mut Criterion) { } } -fn should_collect_memory_profile() -> bool { - if let Ok(value) = std::env::var("CHUTORO_BENCH_HNSW_MEMORY_PROFILE") { +fn should_collect_memory_profile_with_env(env: &dyn Env) -> bool { + if let Some(value) = env.string("CHUTORO_BENCH_HNSW_MEMORY_PROFILE") { let normalized = value.trim().to_ascii_lowercase(); if matches!(normalized.as_str(), "0" | "false" | "off") { return false; @@ -270,17 +271,21 @@ fn should_collect_memory_profile() -> bool { !is_benchmark_discovery() && !is_exact_benchmark_probe() } -fn memory_report_path() -> PathBuf { - std::env::var_os("CHUTORO_BENCH_HNSW_MEMORY_REPORT_PATH") +fn memory_report_path_with_env(env: &dyn Env) -> PathBuf { + env.os_string("CHUTORO_BENCH_HNSW_MEMORY_REPORT_PATH") .map_or_else(|| PathBuf::from(MEMORY_REPORT_PATH), PathBuf::from) } fn profile_hnsw_memory_impl() -> Result, BenchSetupError> { - if !should_collect_memory_profile() { + profile_hnsw_memory_impl_with_env(&DefaultEnv) +} + +fn profile_hnsw_memory_impl_with_env(env: &dyn Env) -> Result, BenchSetupError> { + if !should_collect_memory_profile_with_env(env) { return Ok(None); } - let report_path = memory_report_path(); + let report_path = memory_report_path_with_env(env); let mut records = Vec::new(); for &point_count in POINT_COUNTS { @@ -330,6 +335,13 @@ fn hnsw_build_with_edges(c: &mut Criterion) { } fn hnsw_build_diverse_sources_impl(c: &mut Criterion) -> Result<(), BenchSetupError> { + hnsw_build_diverse_sources_impl_with_env(c, &DefaultEnv) +} + +fn hnsw_build_diverse_sources_impl_with_env( + c: &mut Criterion, + env: &dyn Env, +) -> Result<(), BenchSetupError> { let mut group = c.benchmark_group("hnsw_build_diverse_sources"); configure_hnsw_group(&mut group); @@ -368,7 +380,7 @@ fn hnsw_build_diverse_sources_impl(c: &mut Criterion) -> Result<(), BenchSetupEr ¶ms, ); - if std::env::var("CHUTORO_BENCH_ENABLE_MNIST").as_deref() == Ok("1") { + if env.string("CHUTORO_BENCH_ENABLE_MNIST").as_deref() == Some("1") { let mnist = SyntheticSource::load_mnist(&MnistConfig::default())?; bench_build_source( &mut group, diff --git a/chutoro-benches/benches/hnsw_ef_sweep.rs b/chutoro-benches/benches/hnsw_ef_sweep.rs index 7599d1ad..e1eb884d 100644 --- a/chutoro-benches/benches/hnsw_ef_sweep.rs +++ b/chutoro-benches/benches/hnsw_ef_sweep.rs @@ -6,6 +6,7 @@ use std::{num::NonZeroUsize, path::PathBuf, time::Duration, time::Instant}; use criterion::{BatchSize, BenchmarkId, Criterion, criterion_main}; +use mockable::{DefaultEnv, Env}; use chutoro_benches::{ ef_sweep::{ @@ -82,8 +83,8 @@ fn warn_unrecognised_bool_env(env_var_name: &str, value: &str) { ); } -fn parse_bool_env_var(env_var_name: &str) -> Option { - let value = std::env::var(env_var_name).ok()?; +fn parse_bool_env_var(env: &dyn Env, env_var_name: &str) -> Option { + let value = env.string(env_var_name)?; let normalized = value.trim().to_ascii_lowercase(); if matches!(normalized.as_str(), "0" | "false" | "off") { return Some(false); @@ -122,24 +123,42 @@ fn ef_sweep_point_counts() -> &'static [usize] { } fn should_collect_recall_report() -> bool { - parse_bool_env_var("CHUTORO_BENCH_HNSW_RECALL_REPORT").unwrap_or_else(|| !is_discovery_mode()) + should_collect_recall_report_with_env(&DefaultEnv) +} + +fn should_collect_recall_report_with_env(env: &dyn Env) -> bool { + parse_bool_env_var(env, "CHUTORO_BENCH_HNSW_RECALL_REPORT") + .unwrap_or_else(|| !is_discovery_mode()) } fn recall_report_path() -> PathBuf { - std::env::var_os("CHUTORO_BENCH_HNSW_RECALL_REPORT_PATH") + recall_report_path_with_env(&DefaultEnv) +} + +fn recall_report_path_with_env(env: &dyn Env) -> PathBuf { + env.os_string("CHUTORO_BENCH_HNSW_RECALL_REPORT_PATH") .map_or_else(|| PathBuf::from(RECALL_REPORT_PATH), PathBuf::from) } fn should_collect_cluster_quality_report() -> bool { - parse_bool_env_var("CHUTORO_BENCH_HNSW_CLUSTER_QUALITY_REPORT") + should_collect_cluster_quality_report_with_env(&DefaultEnv) +} + +fn should_collect_cluster_quality_report_with_env(env: &dyn Env) -> bool { + parse_bool_env_var(env, "CHUTORO_BENCH_HNSW_CLUSTER_QUALITY_REPORT") .unwrap_or_else(|| !is_discovery_mode()) } fn cluster_quality_report_path() -> PathBuf { - std::env::var_os("CHUTORO_BENCH_HNSW_CLUSTER_QUALITY_REPORT_PATH").map_or_else( - || PathBuf::from(CLUSTERING_QUALITY_REPORT_PATH), - PathBuf::from, - ) + cluster_quality_report_path_with_env(&DefaultEnv) +} + +fn cluster_quality_report_path_with_env(env: &dyn Env) -> PathBuf { + env.os_string("CHUTORO_BENCH_HNSW_CLUSTER_QUALITY_REPORT_PATH") + .map_or_else( + || PathBuf::from(CLUSTERING_QUALITY_REPORT_PATH), + PathBuf::from, + ) } /// Returns an evenly-spaced query index for deterministic recall sampling. diff --git a/chutoro-benches/src/criterion_support.rs b/chutoro-benches/src/criterion_support.rs index 6d3e4449..34e5aa98 100644 --- a/chutoro-benches/src/criterion_support.rs +++ b/chutoro-benches/src/criterion_support.rs @@ -6,6 +6,7 @@ use std::{fmt::Display, time::Duration}; use criterion::{BenchmarkGroup, BenchmarkId, Criterion, measurement::WallTime}; +use mockable::{DefaultEnv, Env}; /// Returns whether the current command line includes `flag`. /// @@ -111,9 +112,13 @@ pub fn is_exact_benchmark_probe() -> bool { /// ``` #[must_use] pub fn is_nextest_exact_benchmark_probe() -> bool { + is_nextest_exact_benchmark_probe_with_env(&DefaultEnv) +} + +fn is_nextest_exact_benchmark_probe_with_env(env: &dyn Env) -> bool { is_nextest_exact_benchmark_probe_args( std::env::args(), - std::env::var_os("NEXTEST_TEST_NAME").is_some(), + env.os_string("NEXTEST_TEST_NAME").is_some(), ) } diff --git a/chutoro-benches/src/neighbour_scoring/benchmark_runner.rs b/chutoro-benches/src/neighbour_scoring/benchmark_runner.rs index 46d2caa7..0295ba56 100644 --- a/chutoro-benches/src/neighbour_scoring/benchmark_runner.rs +++ b/chutoro-benches/src/neighbour_scoring/benchmark_runner.rs @@ -16,6 +16,7 @@ use chutoro_core::DataSource; use criterion::{ BenchmarkGroup, BenchmarkId, Criterion, Throughput, black_box, measurement::WallTime, }; +use mockable::{DefaultEnv, Env}; use super::{ CandidateBucket, ScoringFixture, benchmark_support::BenchError, benchmark_support::BenchResult, @@ -34,9 +35,12 @@ fn should_use_short_measurement_value(value: Option<&str>) -> bool { } fn should_use_short_measurement() -> bool { - should_use_short_measurement_value(std::env::var(SHORT_MEASUREMENT_ENV).ok().as_deref()) + should_use_short_measurement_with_env(&DefaultEnv) } +fn should_use_short_measurement_with_env(env: &dyn Env) -> bool { + should_use_short_measurement_value(env.string(SHORT_MEASUREMENT_ENV).as_deref()) +} fn score_candidates( scoring_fixture: &ScoringFixture, candidates: &[usize], @@ -109,7 +113,7 @@ fn neighbour_scoring_impl_with( ) -> BenchResult<()> { let report_parent_dir = report_parent_dir(); let build_profile_target = build_profile_report_target_value( - std::env::var(BUILD_PROFILE_ENV).ok().as_deref(), + DefaultEnv.string(BUILD_PROFILE_ENV).as_deref(), &report_parent_dir, ); let build_profile_report_dir = build_profile_target diff --git a/chutoro-benches/src/neighbour_scoring/build_profile.rs b/chutoro-benches/src/neighbour_scoring/build_profile.rs index 9b785548..0e180c56 100644 --- a/chutoro-benches/src/neighbour_scoring/build_profile.rs +++ b/chutoro-benches/src/neighbour_scoring/build_profile.rs @@ -1,6 +1,7 @@ //! Build-profile report path and environment helpers. use camino::{Utf8Path, Utf8PathBuf}; +use mockable::{DefaultEnv, Env}; /// Environment variable that enables HNSW build-profile report generation. pub const BUILD_PROFILE_ENV: &str = "CHUTORO_BENCH_NEIGHBOUR_PROFILE"; @@ -92,7 +93,11 @@ pub fn should_collect_build_profile_value(value: Option<&str>) -> bool { /// ``` #[must_use] pub fn should_collect_build_profile() -> bool { - should_collect_build_profile_value(std::env::var(BUILD_PROFILE_ENV).ok().as_deref()) + should_collect_build_profile_with_env(&DefaultEnv) +} + +fn should_collect_build_profile_with_env(env: &dyn Env) -> bool { + should_collect_build_profile_value(env.string(BUILD_PROFILE_ENV).as_deref()) } /// Returns the parent directory used for benchmark reports. @@ -124,7 +129,11 @@ pub fn report_parent_dir_value(cargo_target_dir: Option<&str>) -> Utf8PathBuf { /// ``` #[must_use] pub fn report_parent_dir() -> Utf8PathBuf { - report_parent_dir_value(std::env::var(CARGO_TARGET_DIR_ENV).ok().as_deref()) + report_parent_dir_with_env(&DefaultEnv) +} + +fn report_parent_dir_with_env(env: &dyn Env) -> Utf8PathBuf { + report_parent_dir_value(env.string(CARGO_TARGET_DIR_ENV).as_deref()) } /// Returns a benchmark report target below the supplied report parent directory. @@ -202,9 +211,10 @@ pub fn build_profile_report_target_value( /// ``` #[must_use] pub fn build_profile_report_target() -> Option { - let report_parent_dir = report_parent_dir(); - build_profile_report_target_value( - std::env::var(BUILD_PROFILE_ENV).ok().as_deref(), - &report_parent_dir, - ) + build_profile_report_target_with_env(&DefaultEnv) +} + +fn build_profile_report_target_with_env(env: &dyn Env) -> Option { + let report_parent_dir = report_parent_dir_with_env(env); + build_profile_report_target_value(env.string(BUILD_PROFILE_ENV).as_deref(), &report_parent_dir) } diff --git a/chutoro-benches/src/source/mnist/mod.rs b/chutoro-benches/src/source/mnist/mod.rs index d1fbdb5a..6e6acba5 100644 --- a/chutoro-benches/src/source/mnist/mod.rs +++ b/chutoro-benches/src/source/mnist/mod.rs @@ -2,7 +2,7 @@ use crate::source::{SyntheticError, numeric::SyntheticSource}; use flate2::read::GzDecoder; -use std::env; +use mockable::{DefaultEnv, Env}; use std::fs; use std::io::Read; use std::path::{Path, PathBuf}; @@ -27,7 +27,7 @@ pub struct MnistConfig { impl Default for MnistConfig { fn default() -> Self { Self { - cache_dir: default_cache_dir(), + cache_dir: default_cache_dir_with_env(&DefaultEnv), base_url: "https://storage.googleapis.com/cvdf-datasets/mnist".to_owned(), } } @@ -147,23 +147,23 @@ fn file_url(config: &MnistConfig, file_name: &str) -> String { format!("{}/{}", config.base_url.trim_end_matches('/'), file_name) } -fn default_cache_dir() -> PathBuf { - if let Some(explicit) = env::var_os("CHUTORO_MNIST_CACHE_DIR") { +fn default_cache_dir_with_env(env: &dyn Env) -> PathBuf { + if let Some(explicit) = env.os_string("CHUTORO_MNIST_CACHE_DIR") { return PathBuf::from(explicit); } - if let Some(xdg_cache) = env::var_os("XDG_CACHE_HOME") { + if let Some(xdg_cache) = env.os_string("XDG_CACHE_HOME") { return PathBuf::from(xdg_cache).join("chutoro").join("mnist"); } - if let Some(home) = env::var_os("HOME") { + if let Some(home) = env.os_string("HOME") { return PathBuf::from(home) .join(".cache") .join("chutoro") .join("mnist"); } - env::temp_dir().join("chutoro").join("mnist") + std::env::temp_dir().join("chutoro").join("mnist") } #[derive(Debug)] diff --git a/chutoro-benches/src/source/mnist/tests.rs b/chutoro-benches/src/source/mnist/tests.rs index ba647c9e..12ac871b 100644 --- a/chutoro-benches/src/source/mnist/tests.rs +++ b/chutoro-benches/src/source/mnist/tests.rs @@ -1,14 +1,14 @@ //! Unit tests for MNIST parsing and cache helpers. - -use super::*; use chutoro_core::DataSource; use flate2::Compression; use flate2::write::GzEncoder; use rstest::rstest; use std::cell::RefCell; use std::collections::HashMap; +use std::env; use std::io::{self, Write}; use std::time::{SystemTime, UNIX_EPOCH}; +use super::*; struct FakeClient { payloads: HashMap>, diff --git a/chutoro-cli/Cargo.toml b/chutoro-cli/Cargo.toml index 4dabef9a..e43d9312 100644 --- a/chutoro-cli/Cargo.toml +++ b/chutoro-cli/Cargo.toml @@ -6,6 +6,7 @@ edition = "2024" [dependencies] anyhow = "1.0.86" clap = { version = "4.5.51", features = ["derive"] } +mockable = { workspace = true } thiserror = "2.0.17" tracing = { version = "0.1.41", features = ["attributes"] } tracing-log = "0.2.0" diff --git a/chutoro-cli/src/logging.rs b/chutoro-cli/src/logging.rs index 90c00735..2ae9c137 100644 --- a/chutoro-cli/src/logging.rs +++ b/chutoro-cli/src/logging.rs @@ -3,6 +3,7 @@ //! Installs a global `tracing` subscriber with optional JSON formatting and //! bridges the `log` facade so crates using either API emit structured events. +use mockable::{DefaultEnv, Env}; use std::{ env, sync::{Mutex, OnceLock}, @@ -97,7 +98,11 @@ fn mark_initialized() { } fn install_subscriber() -> Result<(), LoggingError> { - let use_json = match env::var(LOG_FORMAT_ENV) { + install_subscriber_with_env(&DefaultEnv) +} + +fn install_subscriber_with_env(env: &dyn Env) -> Result<(), LoggingError> { + let use_json = match env.raw(LOG_FORMAT_ENV) { Ok(raw) => parse_log_format(&raw)?, Err(env::VarError::NotPresent) => false, Err(err @ env::VarError::NotUnicode(_)) => { diff --git a/chutoro-core/Cargo.toml b/chutoro-core/Cargo.toml index ab4fb4fb..0611ce6e 100644 --- a/chutoro-core/Cargo.toml +++ b/chutoro-core/Cargo.toml @@ -26,6 +26,7 @@ thiserror = "2.0.17" tracing = { version = "0.1.41", features = ["attributes"] } [dev-dependencies] +mockable = { workspace = true, features = ["mock"] } proptest = "1.8.0" rstest = "0.26" test-strategy = "0.4.3" diff --git a/chutoro-core/src/hnsw/tests/property/search_config.rs b/chutoro-core/src/hnsw/tests/property/search_config.rs index 3c680d55..3b54dab5 100644 --- a/chutoro-core/src/hnsw/tests/property/search_config.rs +++ b/chutoro-core/src/hnsw/tests/property/search_config.rs @@ -3,7 +3,7 @@ //! Reads environment overrides for the minimum recall threshold and maximum //! fixture length used by the property-based search tests. -use std::env; +use mockable::{DefaultEnv, Env}; #[derive(Clone, Copy, Debug, PartialEq)] pub(super) enum RecallThresholdError { @@ -45,20 +45,27 @@ impl SearchPropertyConfig { pub(super) const DEFAULT_MIN_MAX_CONNECTIONS: usize = 12; pub(super) fn load() -> Self { + Self::load_with_env(&DefaultEnv) + } + + fn load_with_env(env: &dyn Env) -> Self { let min_recall = Self::read_env_or_default( Self::ENV_KEY, Self::DEFAULT_MIN_RECALL, Self::parse_min_recall, + env, ); let max_fixture_len = Self::read_env_or_default( Self::MAX_FIXTURE_LEN_ENV_KEY, Self::DEFAULT_MAX_FIXTURE_LEN, Self::parse_max_fixture_len, + env, ); let min_max_connections = Self::read_env_or_default( Self::MIN_MAX_CONNECTIONS_ENV_KEY, Self::DEFAULT_MIN_MAX_CONNECTIONS, Self::parse_min_max_connections, + env, ); Self { @@ -80,23 +87,26 @@ impl SearchPropertyConfig { self.min_max_connections } - fn read_env_or_default(key: EnvKey, default: T, parser: F) -> T + fn read_env_or_default(key: EnvKey, default: T, parser: F, env: &dyn Env) -> T where T: Copy, F: for<'a> Fn(RawConfigValue<'a>) -> Result, { - env::var(key.as_str()).map_or(default, |raw| match parser(RawConfigValue(raw.as_str())) { - Ok(value) => value, - Err(reason) => { - tracing::warn!( - env = key.as_str(), - raw = %raw, - reason = %reason, - "invalid config override, falling back to default", - ); - default - } - }) + match env.string(key.as_str()) { + Some(raw) => match parser(RawConfigValue(raw.as_str())) { + Ok(value) => value, + Err(reason) => { + tracing::warn!( + env = key.as_str(), + raw = %raw, + reason = %reason, + "invalid config override, falling back to default", + ); + default + } + }, + None => default, + } } fn parse_min_recall(raw: RawConfigValue<'_>) -> Result { @@ -139,14 +149,18 @@ mod tests { //! Unit tests for search configuration. use super::*; + use mockable::MockEnv; use rstest::rstest; - use std::{env, sync::Mutex}; - - static ENV_LOCK: Mutex<()> = Mutex::new(()); - fn unset_min_max_connections_env() { - // SAFETY: tests serialize environment access with ENV_LOCK. - unsafe { env::remove_var(SearchPropertyConfig::MIN_MAX_CONNECTIONS_ENV_KEY.as_str()) }; + fn env_with_min_max_connections(value: Option<&str>) -> MockEnv { + let mut env = MockEnv::new(); + let value = value.map(str::to_owned); + env.expect_string().returning(move |key| { + (key == SearchPropertyConfig::MIN_MAX_CONNECTIONS_ENV_KEY.as_str()) + .then(|| value.clone()) + .flatten() + }); + env } #[rstest] @@ -221,10 +235,8 @@ mod tests { #[test] fn load_uses_default_min_max_connections_when_env_unset() { - let _lock = ENV_LOCK.lock().expect("env lock"); - unset_min_max_connections_env(); - - let config = SearchPropertyConfig::load(); + let env = env_with_min_max_connections(None); + let config = SearchPropertyConfig::load_with_env(&env); assert_eq!( config.min_max_connections(), SearchPropertyConfig::DEFAULT_MIN_MAX_CONNECTIONS @@ -233,43 +245,21 @@ mod tests { #[test] fn load_uses_env_min_max_connections_when_valid() { - let _lock = ENV_LOCK.lock().expect("env lock"); - unset_min_max_connections_env(); - let override_val = SearchPropertyConfig::DEFAULT_MIN_MAX_CONNECTIONS + 4; - // SAFETY: tests serialize environment access with ENV_LOCK. - unsafe { - env::set_var( - SearchPropertyConfig::MIN_MAX_CONNECTIONS_ENV_KEY.as_str(), - override_val.to_string(), - ); - }; - - let config = SearchPropertyConfig::load(); + let value = override_val.to_string(); + let env = env_with_min_max_connections(Some(&value)); + let config = SearchPropertyConfig::load_with_env(&env); assert_eq!(config.min_max_connections(), override_val); - - unset_min_max_connections_env(); } #[test] fn load_falls_back_to_default_min_max_connections_on_invalid_env() { - let _lock = ENV_LOCK.lock().expect("env lock"); - unset_min_max_connections_env(); - - // SAFETY: tests serialize environment access with ENV_LOCK. - unsafe { - env::set_var( - SearchPropertyConfig::MIN_MAX_CONNECTIONS_ENV_KEY.as_str(), - "not-a-number", - ); - }; - - let config = SearchPropertyConfig::load(); + let env = env_with_min_max_connections(Some("not-a-number")); + let config = SearchPropertyConfig::load_with_env(&env); assert_eq!( config.min_max_connections(), SearchPropertyConfig::DEFAULT_MIN_MAX_CONNECTIONS ); - unset_min_max_connections_env(); } } diff --git a/chutoro-core/src/hnsw/tests/support.rs b/chutoro-core/src/hnsw/tests/support.rs index 308c95ae..3702efd2 100644 --- a/chutoro-core/src/hnsw/tests/support.rs +++ b/chutoro-core/src/hnsw/tests/support.rs @@ -1,13 +1,19 @@ //! Shared helpers for CPU HNSW tests. +use mockable::{DefaultEnv, Env}; + /// Detects whether the current test run is coverage-instrumented. /// /// Coverage builds can perturb scheduling and substantially increase the cost /// of some property and parallel-construction tests. pub(super) fn is_coverage_job() -> bool { + is_coverage_job_with_env(&DefaultEnv) +} + +fn is_coverage_job_with_env(env: &dyn Env) -> bool { cfg!(coverage) || option_env!("CARGO_LLVM_COV").is_some() || option_env!("LLVM_PROFILE_FILE").is_some() - || std::env::var_os("CARGO_LLVM_COV").is_some() - || std::env::var_os("LLVM_PROFILE_FILE").is_some() + || env.os_string("CARGO_LLVM_COV").is_some() + || env.os_string("LLVM_PROFILE_FILE").is_some() } diff --git a/chutoro-core/src/mst/property/types.rs b/chutoro-core/src/mst/property/types.rs index 5c3959ee..9cd51bbd 100644 --- a/chutoro-core/src/mst/property/types.rs +++ b/chutoro-core/src/mst/property/types.rs @@ -4,6 +4,7 @@ //! by the graph generation strategies and property functions. use crate::CandidateEdge; +use mockable::{DefaultEnv, Env}; /// Weight distribution strategy for generated graphs. /// @@ -60,8 +61,12 @@ impl ConcurrencyConfig { /// [`MIN_CONCURRENCY_REPS`] are clamped upward so the property always /// performs at least one comparison run against the baseline. pub(super) fn load() -> Self { - let repetitions = std::env::var("CHUTORO_MST_PBT_CONCURRENCY_REPS") - .ok() + Self::load_with_env(&DefaultEnv) + } + + fn load_with_env(env: &dyn Env) -> Self { + let repetitions = env + .string("CHUTORO_MST_PBT_CONCURRENCY_REPS") .and_then(|s| s.parse().ok()) .unwrap_or(5) .max(MIN_CONCURRENCY_REPS); diff --git a/chutoro-providers/dense/Cargo.toml b/chutoro-providers/dense/Cargo.toml index d18b54ed..a9f68a88 100644 --- a/chutoro-providers/dense/Cargo.toml +++ b/chutoro-providers/dense/Cargo.toml @@ -21,6 +21,8 @@ path = "../../chutoro-core" default-features = false features = ["skeleton"] +[build-dependencies] +mockable = { workspace = true } [dev-dependencies] bytes = "1.10" proptest = "1.8.0" diff --git a/chutoro-providers/dense/build.rs b/chutoro-providers/dense/build.rs index 7ec24cde..26feb3d4 100644 --- a/chutoro-providers/dense/build.rs +++ b/chutoro-providers/dense/build.rs @@ -1,6 +1,6 @@ //! Detect whether Cargo is compiling this crate with a nightly Rust toolchain. -use std::env; +use mockable::{DefaultEnv, Env}; use std::error::Error; use std::ffi::OsString; use std::io::{self, Write}; @@ -12,7 +12,7 @@ fn main() -> Result<(), Box> { emit_cargo_directive("cargo:rustc-check-cfg=cfg(kani)")?; emit_cargo_directive("cargo:rustc-check-cfg=cfg(nightly)")?; - if is_nightly_compiler() { + if is_nightly_compiler(&DefaultEnv) { emit_cargo_directive("cargo:rustc-cfg=nightly")?; } Ok(()) @@ -22,8 +22,10 @@ fn emit_cargo_directive(directive: &str) -> io::Result<()> { writeln!(io::stdout().lock(), "{directive}") } -fn is_nightly_compiler() -> bool { - let rustc = env::var_os("RUSTC").unwrap_or_else(|| OsString::from("rustc")); +fn is_nightly_compiler(env: &dyn Env) -> bool { + let rustc = env + .os_string("RUSTC") + .unwrap_or_else(|| OsString::from("rustc")); Command::new(rustc) .arg("--version") .output() diff --git a/chutoro-test-support/Cargo.toml b/chutoro-test-support/Cargo.toml index 82991edb..8365d23d 100644 --- a/chutoro-test-support/Cargo.toml +++ b/chutoro-test-support/Cargo.toml @@ -4,10 +4,12 @@ version = "0.1.0" edition = "2024" [dependencies] +mockable = { workspace = true } tracing = { version = "0.1.41", features = ["attributes"] } tracing-subscriber = { version = "0.3.20", features = ["fmt", "registry"] } [dev-dependencies] +mockable = { workspace = true, features = ["mock"] } rstest = "0.26" [lints] diff --git a/chutoro-test-support/src/bin/benchmark_regression_gate.rs b/chutoro-test-support/src/bin/benchmark_regression_gate.rs index a269e6f6..290fb4aa 100644 --- a/chutoro-test-support/src/bin/benchmark_regression_gate.rs +++ b/chutoro-test-support/src/bin/benchmark_regression_gate.rs @@ -1,6 +1,6 @@ //! Emit benchmark regression mode for CI workflows. -use std::env; +use std::env::VarError; use std::error::Error; use std::fs::OpenOptions; use std::io::Write; @@ -8,6 +8,7 @@ use std::io::Write; use chutoro_test_support::ci::benchmark_regression_profile::{ BenchmarkCiPolicy, BenchmarkRegressionProfile, }; +use mockable::{DefaultEnv, Env}; fn main() -> Result<(), Box> { init_tracing(); @@ -48,7 +49,7 @@ fn emit_github_output( profile: BenchmarkRegressionProfile, reason: &str, ) -> Result<(), Box> { - let output_path = read_optional_env("GITHUB_OUTPUT")?.unwrap_or_default(); + let output_path = read_optional_env(&DefaultEnv, "GITHUB_OUTPUT")?.unwrap_or_default(); if output_path.is_empty() { return Ok(()); } @@ -90,10 +91,10 @@ fn write_github_output_value( Ok(()) } -fn read_optional_env(name: &str) -> Result, Box> { - match env::var(name) { +fn read_optional_env(env: &dyn Env, name: &str) -> Result, Box> { + match env.raw(name) { Ok(value) => Ok(Some(value)), - Err(env::VarError::NotPresent) => Ok(None), + Err(VarError::NotPresent) => Ok(None), Err(error) => Err(error.into()), } } diff --git a/chutoro-test-support/src/bin/kani_nightly_gate.rs b/chutoro-test-support/src/bin/kani_nightly_gate.rs index 18e959f0..ddb5c987 100644 --- a/chutoro-test-support/src/bin/kani_nightly_gate.rs +++ b/chutoro-test-support/src/bin/kani_nightly_gate.rs @@ -1,6 +1,6 @@ //! Emit a decision for whether the nightly Kani workflow should run. -use std::env; +use std::env::VarError; use std::error::Error; use std::fs::OpenOptions; use std::io::Write; @@ -8,6 +8,7 @@ use std::process::Command; use std::time::{SystemTime, UNIX_EPOCH}; use chutoro_test_support::ci::nightly_gate::should_run_kani_full; +use mockable::{DefaultEnv, Env}; fn main() -> Result<(), Box> { let force = read_force_flag()?; @@ -28,7 +29,7 @@ fn main() -> Result<(), Box> { } fn read_force_flag() -> Result> { - let raw = read_optional_env("CHUTORO_KANI_FORCE")?.unwrap_or_default(); + let raw = read_optional_env(&DefaultEnv, "CHUTORO_KANI_FORCE")?.unwrap_or_default(); if raw.is_empty() { return Ok(false); } @@ -45,7 +46,7 @@ fn parse_bool(value: &str) -> Result { } fn read_commit_epoch() -> Result> { - if let Some(value) = read_optional_env("CHUTORO_KANI_COMMIT_EPOCH")? { + if let Some(value) = read_optional_env(&DefaultEnv, "CHUTORO_KANI_COMMIT_EPOCH")? { return Ok(value.parse::()?); } @@ -66,7 +67,7 @@ fn read_commit_epoch() -> Result> { } fn read_now_epoch() -> Result> { - if let Some(value) = read_optional_env("CHUTORO_KANI_NOW_EPOCH")? { + if let Some(value) = read_optional_env(&DefaultEnv, "CHUTORO_KANI_NOW_EPOCH")? { return Ok(value.parse::()?); } @@ -75,7 +76,7 @@ fn read_now_epoch() -> Result> { } fn emit_github_output(should_run: bool, reason: &str) -> Result<(), Box> { - let output_path = read_optional_env("GITHUB_OUTPUT")?.unwrap_or_default(); + let output_path = read_optional_env(&DefaultEnv, "GITHUB_OUTPUT")?.unwrap_or_default(); if output_path.is_empty() { return Ok(()); } @@ -113,10 +114,10 @@ fn write_github_output_value( Ok(()) } -fn read_optional_env(name: &str) -> Result, Box> { - match env::var(name) { +fn read_optional_env(env: &dyn Env, name: &str) -> Result, Box> { + match env.raw(name) { Ok(value) => Ok(Some(value)), - Err(env::VarError::NotPresent) => Ok(None), + Err(VarError::NotPresent) => Ok(None), Err(error) => Err(error.into()), } } diff --git a/chutoro-test-support/src/ci/benchmark_regression_profile.rs b/chutoro-test-support/src/ci/benchmark_regression_profile.rs index b89f4d4a..5432bc94 100644 --- a/chutoro-test-support/src/ci/benchmark_regression_profile.rs +++ b/chutoro-test-support/src/ci/benchmark_regression_profile.rs @@ -3,7 +3,7 @@ //! This module centralizes event and policy parsing for benchmark regression //! checks so workflows can share one deterministic decision surface. -use std::env; +use mockable::{DefaultEnv, Env}; /// Environment variable controlling benchmark CI policy. pub const CHUTORO_BENCH_CI_POLICY_ENV_KEY: &str = "CHUTORO_BENCH_CI_POLICY"; @@ -118,12 +118,9 @@ impl BenchmarkRegressionProfile { /// ``` #[must_use] pub fn load(default_policy: BenchmarkCiPolicy) -> Self { - Self::load_with_lookup(default_policy, |key| env::var(key).ok()) + Self::load_with_env(default_policy, &DefaultEnv) } - fn load_with_lookup(default_policy: BenchmarkCiPolicy, lookup: F) -> Self - where - F: Fn(&'static str) -> Option, { let policy = lookup(CHUTORO_BENCH_CI_POLICY_ENV_KEY).map_or( @@ -157,21 +154,58 @@ impl BenchmarkRegressionProfile { /// Returns the parsed benchmark CI policy. #[must_use] + pub const fn policy(self) -> BenchmarkCiPolicy { self.policy } /// Returns the parsed CI event category. #[must_use] + pub const fn event(self) -> BenchmarkCiEvent { self.event } /// Returns the resolved benchmark regression mode. #[must_use] + pub const fn mode(self) -> BenchmarkRegressionMode { self.mode } + + fn load_with_env(default_policy: BenchmarkCiPolicy, env: &dyn Env) -> Self { + let policy = match env.string(CHUTORO_BENCH_CI_POLICY_ENV_KEY) { + Some(raw) => match parse_policy(&raw) { + Ok(policy) => policy, + Err(reason) => { + tracing::warn!( + env = CHUTORO_BENCH_CI_POLICY_ENV_KEY, + raw = %raw, + reason = %reason, + fallback_policy = default_policy.as_str(), + "invalid benchmark CI policy override; using default", + ); + default_policy + } + }, + None => default_policy, + }; + + let event = env + .string(GITHUB_EVENT_NAME_ENV_KEY) + .as_deref() + .map_or(BenchmarkCiEvent::Other, parse_event_name); + let mode = resolve_regression_mode(event, policy); + + Self { + policy, + event, + mode, + } + } + + /// Returns the parsed benchmark CI policy. + #[must_use] } /// Resolves benchmark regression mode for a CI event and policy. @@ -235,6 +269,7 @@ mod tests { //! Unit tests for benchmark regression profile parsing. use super::*; + use mockable::MockEnv; use rstest::rstest; use std::collections::HashMap; @@ -251,9 +286,10 @@ mod tests { env_entries.insert(CHUTORO_BENCH_CI_POLICY_ENV_KEY, raw.to_owned()); } - BenchmarkRegressionProfile::load_with_lookup(default_policy, |key| { - env_entries.get(key).cloned() - }) + let mut env = MockEnv::new(); + env.expect_string() + .returning(move |key| env_entries.get(key).cloned()); + BenchmarkRegressionProfile::load_with_env(default_policy, &env) } #[rstest] diff --git a/chutoro-test-support/src/ci/property_test_profile.rs b/chutoro-test-support/src/ci/property_test_profile.rs index d0f17892..a918e476 100644 --- a/chutoro-test-support/src/ci/property_test_profile.rs +++ b/chutoro-test-support/src/ci/property_test_profile.rs @@ -3,7 +3,7 @@ //! This module centralizes environment-driven proptest tuning so multiple //! suites can share one policy surface. -use std::env; +use mockable::{DefaultEnv, Env}; /// Environment variable controlling proptest case counts. pub const PROPTEST_CASES_ENV_KEY: &str = "PROPTEST_CASES"; @@ -47,15 +47,12 @@ impl ProptestRunProfile { /// ``` #[must_use] pub fn load(default_cases: u32, default_fork: bool) -> Self { - Self::load_with_lookup(default_cases, default_fork, |key| env::var(key).ok()) + Self::load_with_env(default_cases, default_fork, &DefaultEnv) } - fn load_with_lookup(default_cases: u32, default_fork: bool, lookup: F) -> Self - where - F: Fn(&'static str) -> Option, - { - let cases = read_cases_or_default(default_cases, &lookup); - let fork = read_env_or_default(CHUTORO_PBT_FORK_ENV_KEY, default_fork, parse_bool, &lookup); + fn load_with_env(default_cases: u32, default_fork: bool, env: &dyn Env) -> Self { + let cases = read_cases_or_default(default_cases, env); + let fork = read_env_or_default(CHUTORO_PBT_FORK_ENV_KEY, default_fork, parse_bool, env); Self { cases, fork } } @@ -72,31 +69,26 @@ impl ProptestRunProfile { } } -fn read_cases_or_default(default: u32, lookup: &L) -> u32 -where - L: Fn(&'static str) -> Option, -{ - read_env(PROPTEST_CASES_ENV_KEY, parse_cases, lookup).map_or_else( - || read_env_or_default(PROGTEST_CASES_ENV_KEY, default, parse_cases, lookup), +fn read_cases_or_default(default: u32, env: &dyn Env) -> u32 { + read_env(PROPTEST_CASES_ENV_KEY, parse_cases, env).map_or_else( + || read_env_or_default(PROGTEST_CASES_ENV_KEY, default, parse_cases, env), |cases| cases.unwrap_or(default), ) } -fn read_env_or_default(key: &'static str, default: T, parser: F, lookup: &L) -> T +fn read_env_or_default(key: &'static str, default: T, parser: F, env: &dyn Env) -> T where T: Copy, F: Fn(&str) -> Result, - L: Fn(&'static str) -> Option, { - read_env(key, parser, lookup).map_or(default, |value| value.unwrap_or(default)) + read_env(key, parser, env).map_or(default, |value| value.unwrap_or(default)) } -fn read_env(key: &'static str, parser: F, lookup: &L) -> Option> +fn read_env(key: &'static str, parser: F, env: &dyn Env) -> Option> where F: Fn(&str) -> Result, - L: Fn(&'static str) -> Option, { - lookup(key).map(|raw| { + env.string(key).map(|raw| { parser(&raw).map_err(|reason| { tracing::warn!( env = key, @@ -134,6 +126,7 @@ mod tests { //! Unit tests for property-test profile selection. use super::*; + use mockable::MockEnv; use rstest::rstest; use std::collections::HashMap; @@ -160,9 +153,10 @@ mod tests { env_entries.insert(CHUTORO_PBT_FORK_ENV_KEY, raw.to_owned()); } - ProptestRunProfile::load_with_lookup(default_cases, default_fork, |key| { - env_entries.get(key).cloned() - }) + let mut env = MockEnv::new(); + env.expect_string() + .returning(move |key| env_entries.get(key).cloned()); + ProptestRunProfile::load_with_env(default_cases, default_fork, &env) } #[test] diff --git a/chutoro-test-support/src/process.rs b/chutoro-test-support/src/process.rs index 0264feeb..136b5758 100644 --- a/chutoro-test-support/src/process.rs +++ b/chutoro-test-support/src/process.rs @@ -14,6 +14,8 @@ use std::fmt; use std::fs; use std::path::{Path, PathBuf}; +use mockable::{DefaultEnv, Env}; + /// Errors surfaced when a compiled test binary cannot be located. #[derive(Debug, Clone, PartialEq, Eq)] pub enum TestBinaryError { @@ -78,7 +80,11 @@ impl std::error::Error for TestBinaryError {} /// assert!(path.exists()); /// ``` pub fn find_test_binary(name: &str) -> Result { - if let Ok(value) = env::var(format!("CARGO_BIN_EXE_{name}")) { + find_test_binary_with_env(name, &DefaultEnv) +} + +fn find_test_binary_with_env(name: &str, env: &dyn Env) -> Result { + if let Ok(value) = env.raw(&format!("CARGO_BIN_EXE_{name}")) { return Ok(with_exe_suffix(PathBuf::from(value))); } @@ -101,7 +107,6 @@ pub fn find_test_binary(name: &str) -> Result { name: name.to_owned(), }) } - fn find_in_deps(deps_dir: &Path, name: &str) -> Option { fs::read_dir(deps_dir) .ok()? diff --git a/clippy.toml b/clippy.toml index effc6040..876c9e68 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,4 +1,13 @@ # Align with CodeScene’s ceiling +disallowed-methods = [ + { path = "std::env::var", reason = "inject an environment reader" }, + { path = "std::env::var_os", reason = "inject an environment reader" }, + { path = "std::env::vars", reason = "inject an environment reader" }, + { path = "std::env::vars_os", reason = "inject an environment reader" }, + { path = "std::env::set_var", reason = "use a stub environment in tests" }, + { path = "std::env::remove_var", reason = "use a stub environment in tests" }, +] + cognitive-complexity-threshold = 9 # default is 25 too-many-arguments-threshold = 4 # default is 7 too-many-lines-threshold = 70 # default is 100 diff --git a/docs/execplans/feat-align-workspace-lint-policy.md b/docs/execplans/feat-align-workspace-lint-policy.md index 7088fcad..b40c065f 100644 --- a/docs/execplans/feat-align-workspace-lint-policy.md +++ b/docs/execplans/feat-align-workspace-lint-policy.md @@ -236,6 +236,13 @@ keeps local editor feedback aligned with commit gates. by the convenience constructor and must not be reused by new provider APIs; callers with a capability or readable source use `try_from_parquet_reader`. `dylint.toml` excludes only this adapter. +- 2026-08-24: The environment-injection stack reuses the new + `chutoro-test-support::process::find_test_binary` abstraction rather than + retaining per-test binary discovery. Its environment-variable lookup is now + injected through `mockable::Env`, while the public boundary supplies + `DefaultEnv`. Rationale: the shared helper removes duplicated discovery + logic without creating an exception to the workspace ban on ambient + environment reads. ## Stage 2a: Core Mechanical Lint Onboarding From 8745911f4657fff6c1a193eaf4ab7f2cfa0cbded Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 24 Aug 2026 16:46:50 +0200 Subject: [PATCH 2/3] Reconcile the stacked rebase Regenerate the lockfile for the lint-policy branch dependency graph and resolve the rebased test configuration lint findings. Record the isolated property-test timeout experiment and the shared test-binary reader decision for the active lint-policy plan. --- Cargo.lock | 84 +++++++++++++++- chutoro-benches/src/source/mnist/tests.rs | 2 +- .../src/hnsw/tests/property/search_config.rs | 15 ++- ...lan-2026-08-24-hnsw-idempotency-timeout.md | 97 +++++++++++++++++++ .../feat-align-workspace-lint-policy.md | 6 +- 5 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 docs/debugging/debugging-plan-2026-08-24-hnsw-idempotency-timeout.md diff --git a/Cargo.lock b/Cargo.lock index 7857da51..1044e0e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -406,7 +406,7 @@ dependencies = [ "cap-std", "chutoro-bench-datasets", "chutoro-test-support", - "mockall", + "mockall 0.13.1", "proptest", "rstest", "rstest-bdd", @@ -430,6 +430,7 @@ dependencies = [ "chutoro-providers-dense", "criterion", "flate2", + "mockable", "proptest", "rand 0.8.7", "rstest", @@ -451,6 +452,7 @@ dependencies = [ "chutoro-providers-text", "chutoro-test-support", "clap", + "mockable", "parquet", "rstest", "tempfile", @@ -469,6 +471,7 @@ dependencies = [ "lru", "metrics", "metrics-util", + "mockable", "num-traits", "proptest", "rand 0.8.7", @@ -494,6 +497,7 @@ dependencies = [ "bytes", "chutoro-core", "chutoro-test-support", + "mockable", "parquet", "proptest", "rstest", @@ -516,6 +520,7 @@ dependencies = [ name = "chutoro-test-support" version = "0.1.0" dependencies = [ + "mockable", "rstest", "tracing", "tracing-subscriber", @@ -784,6 +789,12 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "difflib" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" + [[package]] name = "digest" version = "0.11.3" @@ -882,6 +893,15 @@ dependencies = [ "zlib-rs", ] +[[package]] +name = "float-cmp" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "98de4bbd547a563b716d8dfa9aad1cb19bfab00f4fa09a6a4ed21dbcf44ce9c4" +dependencies = [ + "num-traits", +] + [[package]] name = "fluent" version = "0.17.0" @@ -1467,6 +1487,31 @@ dependencies = [ "simd-adler32", ] +[[package]] +name = "mockable" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "696033a748f67c97f2169b29b428fbab0d12317707cede1ca62fcb786e80b7fe" +dependencies = [ + "mockall 0.11.4", + "tracing", +] + +[[package]] +name = "mockall" +version = "0.11.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c84490118f2ee2d74570d114f3d0493cbf02790df303d2707606c3e14e07c96" +dependencies = [ + "cfg-if", + "downcast", + "fragile", + "lazy_static", + "mockall_derive 0.11.4", + "predicates 2.1.5", + "predicates-tree", +] + [[package]] name = "mockall" version = "0.13.1" @@ -1476,11 +1521,23 @@ dependencies = [ "cfg-if", "downcast", "fragile", - "mockall_derive", - "predicates", + "mockall_derive 0.13.1", + "predicates 3.1.4", "predicates-tree", ] +[[package]] +name = "mockall_derive" +version = "0.11.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22ce75669015c4f47b289fd4d4f56e894e4c96003ffdf3ac51313126f94c6cbb" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "mockall_derive" version = "0.13.1" @@ -1508,6 +1565,12 @@ dependencies = [ "smallvec", ] +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + [[package]] name = "nu-ansi-term" version = "0.50.3" @@ -1724,6 +1787,20 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "predicates" +version = "2.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59230a63c37f3e18569bdb90e4a89cbf5bf8b06fea0b84e65ea10cc4df47addd" +dependencies = [ + "difflib", + "float-cmp", + "itertools", + "normalize-line-endings", + "predicates-core", + "regex", +] + [[package]] name = "predicates" version = "3.1.4" @@ -2464,6 +2541,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" dependencies = [ "proc-macro2", + "quote", "unicode-ident", ] diff --git a/chutoro-benches/src/source/mnist/tests.rs b/chutoro-benches/src/source/mnist/tests.rs index 12ac871b..09088ce6 100644 --- a/chutoro-benches/src/source/mnist/tests.rs +++ b/chutoro-benches/src/source/mnist/tests.rs @@ -1,4 +1,5 @@ //! Unit tests for MNIST parsing and cache helpers. +use super::*; use chutoro_core::DataSource; use flate2::Compression; use flate2::write::GzEncoder; @@ -8,7 +9,6 @@ use std::collections::HashMap; use std::env; use std::io::{self, Write}; use std::time::{SystemTime, UNIX_EPOCH}; -use super::*; struct FakeClient { payloads: HashMap>, diff --git a/chutoro-core/src/hnsw/tests/property/search_config.rs b/chutoro-core/src/hnsw/tests/property/search_config.rs index 3b54dab5..c5459dd8 100644 --- a/chutoro-core/src/hnsw/tests/property/search_config.rs +++ b/chutoro-core/src/hnsw/tests/property/search_config.rs @@ -92,8 +92,8 @@ impl SearchPropertyConfig { T: Copy, F: for<'a> Fn(RawConfigValue<'a>) -> Result, { - match env.string(key.as_str()) { - Some(raw) => match parser(RawConfigValue(raw.as_str())) { + env.string(key.as_str()) + .map_or(default, |raw| match parser(RawConfigValue(raw.as_str())) { Ok(value) => value, Err(reason) => { tracing::warn!( @@ -104,9 +104,7 @@ impl SearchPropertyConfig { ); default } - }, - None => default, - } + }) } fn parse_min_recall(raw: RawConfigValue<'_>) -> Result { @@ -152,12 +150,12 @@ mod tests { use mockable::MockEnv; use rstest::rstest; - fn env_with_min_max_connections(value: Option<&str>) -> MockEnv { + fn env_with_min_max_connections(raw_value: Option<&str>) -> MockEnv { let mut env = MockEnv::new(); - let value = value.map(str::to_owned); + let configured_value = raw_value.map(str::to_owned); env.expect_string().returning(move |key| { (key == SearchPropertyConfig::MIN_MAX_CONNECTIONS_ENV_KEY.as_str()) - .then(|| value.clone()) + .then(|| configured_value.clone()) .flatten() }); env @@ -260,6 +258,5 @@ mod tests { config.min_max_connections(), SearchPropertyConfig::DEFAULT_MIN_MAX_CONNECTIONS ); - } } diff --git a/docs/debugging/debugging-plan-2026-08-24-hnsw-idempotency-timeout.md b/docs/debugging/debugging-plan-2026-08-24-hnsw-idempotency-timeout.md new file mode 100644 index 00000000..64a224f5 --- /dev/null +++ b/docs/debugging/debugging-plan-2026-08-24-hnsw-idempotency-timeout.md @@ -0,0 +1,97 @@ +# Debugging Plan: HNSW idempotency property timeout after stack rebase + +**Generated**: 2026-08-24 +**Issue ID**: Rebase of #223 onto #228 +**Severity**: High — blocks the required rebase validation gate +**Falsification sub-agent**: alchemist +**Planning agent boundary**: This document was prepared by the planning agent. +Falsification must be executed by the named sub-agent, not by the planning +agent. + +## Problem Statement + +After rebasing the environment-reader branch onto the lint-policy stack, +`make test` passed 1,081 tests but timed out +`hnsw_idempotency_preserved_proptest` at its 600-second nextest limit. The same +branch passed the complete test gate before the stack rebase. The expected +behaviour is that the idempotency property completes within the configured +timeout without weakening its workload or timeout. + +## Context Summary + +| Aspect | Details | +| ------------------- | ----------------------------------------------------------------- | +| First observed | Rebased commit `2342187` on 2026-08-24 | +| Reproduction rate | One complete workspace run; isolated run not yet attempted | +| Affected components | HNSW idempotency property test and nextest scheduling | +| Recent changes | Rebase onto #228; `is_coverage_job` now reads via `mockable::Env` | + +### Error Artefacts + +```plaintext +TIMEOUT [600.004s] chutoro-core +hnsw::tests::property::tests::hnsw_idempotency_preserved_proptest +Summary 1082 tests run: 1081 passed, 1 timed out, 1 skipped +``` + +### Information Gaps + +- The isolated runtime of the affected property test is not yet known. +- A one-run timeout cannot distinguish shared-machine contention from a + regression in the test's effective workload. + +______________________________________________________________________ + +## Hypotheses + +### H1: Workspace concurrency caused the timeout + +**Claim**: The property test remains within its budget when executed alone; the +full workspace run's concurrent workload caused the 600-second timeout. + +**Plausibility**: Medium — the rebased HNSW idempotency runner still caps its +case count at 16 and disables per-case forking, while the failure occurred near +the end of a concurrent workspace run. + +**Prediction**: An isolated nextest invocation of the exact property test +completes successfully within 600 seconds. + +#### H1 Falsification Plan + +| Step | Action | Expected Negative Result | +| ---- | ------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------- | +| 1 | Run `cargo nextest run -p chutoro-core hnsw_idempotency_preserved_proptest` once, without changing environment variables. | A timeout or failure disproves contention as a sufficient explanation. | + +**Tooling**: Cargo Nextest, existing shared Cargo cache, and a `/tmp` log. + +**Confidence on falsification**: High — the command removes other workspace +tests while retaining the same compiled test and nextest timeout policy. + +______________________________________________________________________ + +## Recommended Execution Order + +1. **H1** — It is the smallest decisive experiment and does not alter test + configuration, code, or process-global environment state. + +## Termination Criteria + +- **Root cause identified**: The isolated test either completes, isolating + shared-run contention, or times out, falsifying H1 and requiring a revised + plan for the property workload. +- **Escalation trigger**: If the isolated command times out or fails, stop and + revise the hypothesis plan before altering implementation or test budgets. + +## Notes for Executing Agent + +Run only the exact command in H1 and return a verdict of falsified, +not-falsified, or inconclusive with the log path and elapsed time. Do not edit +tracked files, modify environment variables, run the full workspace gate, or +change nextest timeouts. + +## Recorded Result + +H1 was not falsified on 2026-08-24. The isolated command passed in 0.213 +seconds (13.886 seconds including compilation), so the prior 600-second timeout +is consistent with shared full-suite contention rather than a regression in the +rebased HNSW idempotency property. diff --git a/docs/execplans/feat-align-workspace-lint-policy.md b/docs/execplans/feat-align-workspace-lint-policy.md index b40c065f..99d65e9e 100644 --- a/docs/execplans/feat-align-workspace-lint-policy.md +++ b/docs/execplans/feat-align-workspace-lint-policy.md @@ -240,9 +240,9 @@ keeps local editor feedback aligned with commit gates. `chutoro-test-support::process::find_test_binary` abstraction rather than retaining per-test binary discovery. Its environment-variable lookup is now injected through `mockable::Env`, while the public boundary supplies - `DefaultEnv`. Rationale: the shared helper removes duplicated discovery - logic without creating an exception to the workspace ban on ambient - environment reads. + `DefaultEnv`. Rationale: the shared helper removes duplicated discovery logic + without creating an exception to the workspace ban on ambient environment + reads. ## Stage 2a: Core Mechanical Lint Onboarding From f3c778f9aa13730630e7bd39ae538abecd6be0ac Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 24 Aug 2026 17:41:37 +0200 Subject: [PATCH 3/3] Fix benchmark profile replay merge Keep the parent branch's `map_or` policy parsing while preserving the injected mockable environment reader. The previous rebase replay left both loader bodies in the implementation block, preventing the test-support crate from compiling. --- .../src/ci/benchmark_regression_profile.rs | 76 +++++-------------- 1 file changed, 19 insertions(+), 57 deletions(-) diff --git a/chutoro-test-support/src/ci/benchmark_regression_profile.rs b/chutoro-test-support/src/ci/benchmark_regression_profile.rs index 5432bc94..72c5a306 100644 --- a/chutoro-test-support/src/ci/benchmark_regression_profile.rs +++ b/chutoro-test-support/src/ci/benchmark_regression_profile.rs @@ -121,61 +121,10 @@ impl BenchmarkRegressionProfile { Self::load_with_env(default_policy, &DefaultEnv) } - { - let policy = - lookup(CHUTORO_BENCH_CI_POLICY_ENV_KEY).map_or( - default_policy, - |raw| match parse_policy(&raw) { - Ok(policy) => policy, - Err(reason) => { - tracing::warn!( - env = CHUTORO_BENCH_CI_POLICY_ENV_KEY, - raw = %raw, - reason = %reason, - fallback_policy = default_policy.as_str(), - "invalid benchmark CI policy override; using default", - ); - default_policy - } - }, - ); - - let event = lookup(GITHUB_EVENT_NAME_ENV_KEY) - .as_deref() - .map_or(BenchmarkCiEvent::Other, parse_event_name); - let mode = resolve_regression_mode(event, policy); - - Self { - policy, - event, - mode, - } - } - - /// Returns the parsed benchmark CI policy. - #[must_use] - - pub const fn policy(self) -> BenchmarkCiPolicy { - self.policy - } - - /// Returns the parsed CI event category. - #[must_use] - - pub const fn event(self) -> BenchmarkCiEvent { - self.event - } - - /// Returns the resolved benchmark regression mode. - #[must_use] - - pub const fn mode(self) -> BenchmarkRegressionMode { - self.mode - } - fn load_with_env(default_policy: BenchmarkCiPolicy, env: &dyn Env) -> Self { - let policy = match env.string(CHUTORO_BENCH_CI_POLICY_ENV_KEY) { - Some(raw) => match parse_policy(&raw) { + let policy = env + .string(CHUTORO_BENCH_CI_POLICY_ENV_KEY) + .map_or(default_policy, |raw| match parse_policy(&raw) { Ok(policy) => policy, Err(reason) => { tracing::warn!( @@ -187,9 +136,7 @@ impl BenchmarkRegressionProfile { ); default_policy } - }, - None => default_policy, - }; + }); let event = env .string(GITHUB_EVENT_NAME_ENV_KEY) @@ -206,6 +153,21 @@ impl BenchmarkRegressionProfile { /// Returns the parsed benchmark CI policy. #[must_use] + pub const fn policy(self) -> BenchmarkCiPolicy { + self.policy + } + + /// Returns the parsed CI event category. + #[must_use] + pub const fn event(self) -> BenchmarkCiEvent { + self.event + } + + /// Returns the resolved benchmark regression mode. + #[must_use] + pub const fn mode(self) -> BenchmarkRegressionMode { + self.mode + } } /// Resolves benchmark regression mode for a CI event and policy.