From 161f0ddcd76d42f638e88d8bf75425871a27ed27 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 03:49:29 +0200 Subject: [PATCH 01/21] Use mockable environment access for CLI configuration (#483) Replace the bespoke config-selector environment trait with `mockable::Env` so discovery and merging use the established injectable seam. Keep automatic discovery from re-reading `NETSUKE_CONFIG` after that injected lookup, and adapt deterministic unit, integration, and BDD coverage to `MockEnv`. --- Cargo.toml | 1 + src/cli/config_path_precedence_tests.rs | 9 ++--- src/cli/discovery_layer_tests.rs | 7 ++-- src/cli/discovery_tracing_tests.rs | 18 ++++----- src/cli/mod.rs | 1 - src/cli/test_support.rs | 43 +++++++++++---------- tests/bdd/helpers/config_environment.rs | 50 +++++++++++-------------- tests/cli_tests/merge_diag.rs | 25 +++---------- 8 files changed, 64 insertions(+), 90 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2e39d0774..ab615e549 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,6 +137,7 @@ sys-locale = "0.3.2" cap-std = "3.4.4" clap = { version = "4.5.0", features = ["derive"] } clap_mangen = "0.3.0" +mockable = "3.0" ortho_config = { version = "0.9.0", features = ["serde_json"] } serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["preserve_order"] } diff --git a/src/cli/config_path_precedence_tests.rs b/src/cli/config_path_precedence_tests.rs index 38ddae444..e37120e1a 100644 --- a/src/cli/config_path_precedence_tests.rs +++ b/src/cli/config_path_precedence_tests.rs @@ -6,7 +6,7 @@ //! same invariant over generated path values. use super::*; -use crate::cli::test_support::TestEnv; +use crate::cli::test_support::{empty_mock_env, mock_env_with}; use proptest::prelude::*; use rstest::rstest; use std::path::PathBuf; @@ -22,10 +22,9 @@ fn resolve_config_path_with_selectors( cli_config: Option, env_config: Option<&PathBuf>, ) -> Option { - let mut env = TestEnv::default(); - if let Some(value) = env_config { - env = env.with_var(CONFIG_ENV_VAR, value.as_os_str()); - } + let env = env_config.map_or_else(empty_mock_env, |value| { + mock_env_with([(CONFIG_ENV_VAR, value.as_os_str().to_owned())]) + }); let cli = Cli { config: cli_config, ..Cli::default() diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 936c777fb..17eb493ba 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -4,15 +4,14 @@ //! versus automatic discovery — and the project-scope second pass in //! [`collect_file_layers`]. Selector precedence and event-schema snapshots live //! in the tracing test module. - -use super::*; -use crate::cli::test_support::TestEnv; use anyhow::{Context, Result, ensure}; use googletest::prelude::*; use pretty_assertions::assert_eq; use rstest::rstest; +use super::*; use tempfile::{TempDir, tempdir}; +use crate::cli::test_support::empty_mock_env; use super::event_assertions::{EventAssertion, capture_events, find_event}; use super::layers::{collect_file_layers, collect_file_layers_with_normalizer}; use super::paths::FailingPathNormalizer; @@ -55,7 +54,7 @@ fn collect_diag_file_layers_logs_selected_branch( ) -> Result<()> { let temp = tempdir().context("create temp dir")?; let cli = scenario_cli(scenario, &temp)?; - let env = TestEnv::default(); + let env = empty_mock_env(); let (layers, events) = capture_events(|| collect_diag_file_layers_with_env(&cli, &env))?; let branch_event = find_event(&events, expected_event)?; diff --git a/src/cli/discovery_tracing_tests.rs b/src/cli/discovery_tracing_tests.rs index 5100955b8..19dce6e7a 100644 --- a/src/cli/discovery_tracing_tests.rs +++ b/src/cli/discovery_tracing_tests.rs @@ -1,13 +1,14 @@ //! Tests for configuration discovery tracing. //! -//! Selection is exercised through the injected [`EnvProvider`] double, so these +//! Selection is exercised through an injected [`mockable::MockEnv`], so these //! tests never mutate the process environment and need no lock. use super::*; -use crate::cli::test_support::TestEnv; +use crate::cli::test_support::{empty_mock_env, mock_env_with}; use crate::snapshot_test_support::snapshot_settings; use anyhow::{Context, Result, ensure}; use insta::assert_snapshot; +use mockable::MockEnv; use rstest::rstest; use tempfile::tempdir; @@ -28,7 +29,7 @@ fn snapshot_failure_event(assertion: &EventAssertion<'_>, snapshot_name: &str) - /// Resolve `cli_config`/`env_config` and trace the result, returning both. fn resolve_and_trace( cli_config: Option, - env: &TestEnv, + env: &MockEnv, ) -> Result<(ConfigPathResolution, Vec)> { capture_events(|| { let resolution = resolve_config_selector(cli_config, env); @@ -86,10 +87,9 @@ struct ConfigPathScenario { expected_env_trace: Some((CONFIG_ENV_VAR, false)), })] fn explicit_config_path_logs_selected_selector(#[case] scenario: ConfigPathScenario) -> Result<()> { - let mut env = TestEnv::default(); - if let Some(value) = scenario.config_env { - env = env.with_var(CONFIG_ENV_VAR, value); - } + let env = scenario.config_env.map_or_else(empty_mock_env, |value| { + mock_env_with([(CONFIG_ENV_VAR, value)]) + }); let (resolution, events) = resolve_and_trace(scenario.cli_config.map(PathBuf::from), &env)?; let selector_event = find_event(&events, "resolved config path")?; @@ -150,7 +150,7 @@ fn explicit_config_path_logs_selected_selector(#[case] scenario: ConfigPathScena /// only `NETSUKE_CONFIG` is ever looked up. #[test] fn legacy_config_path_variable_is_not_a_selector() -> Result<()> { - let env = TestEnv::default().with_var("NETSUKE_CONFIG_PATH", "legacy-should-be-ignored.toml"); + let env = mock_env_with([("NETSUKE_CONFIG_PATH", "legacy-should-be-ignored.toml")]); let (resolution, events) = resolve_and_trace(None, &env)?; @@ -176,7 +176,7 @@ fn legacy_config_path_variable_is_not_a_selector() -> Result<()> { fn selector_resolution_event_schema_snapshot() -> Result<()> { let temp = tempdir().context("create temp dir")?; let config_path = temp.path().join("selector.toml"); - let env = TestEnv::default().with_var(CONFIG_ENV_VAR, config_path.as_os_str()); + let env = mock_env_with([(CONFIG_ENV_VAR, config_path.as_os_str().to_owned())]); let (resolution, events) = resolve_and_trace(None, &env)?; ensure!( diff --git a/src/cli/mod.rs b/src/cli/mod.rs index d8df8d584..2f6d622dd 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -22,7 +22,6 @@ pub(crate) mod test_support; pub use config::{AccessibilityPolicy, CliConfig, ColourPolicy, EmojiPolicy, ProgressPolicy}; pub use diag::{resolve_merged_json, resolve_merged_json_with_env}; -pub use discovery::{EnvProvider as ConfigEnvProvider, StdEnvProvider as ConfigStdEnvProvider}; pub use merge::{merge_with_config, merge_with_config_and_env}; pub use parser::{ BuildArgs, Cli, Commands, GraphArgs, json_hint_from_args, locale_hint_from_args, diff --git a/src/cli/test_support.rs b/src/cli/test_support.rs index 10381c003..d5f1f9d9c 100644 --- a/src/cli/test_support.rs +++ b/src/cli/test_support.rs @@ -1,29 +1,28 @@ -//! Shared test doubles for CLI configuration tests. +//! Mockable environment builders for CLI configuration tests. //! -//! `TestEnv` provides deterministic, in-memory environment values for unit -//! tests that exercise [`super::discovery::EnvProvider`] without mutating the -//! process environment. +//! The helpers return [`mockable::MockEnv`] instances, so tests inject each +//! environment read rather than mutating the process environment. +use mockable::MockEnv; use std::{collections::HashMap, ffi::OsString}; -use super::discovery::EnvProvider; - -/// In-memory environment values for deterministic CLI configuration tests. -#[derive(Default)] -pub(crate) struct TestEnv { - values: HashMap<&'static str, OsString>, -} - -impl TestEnv { - /// Add one environment value to this test double. - pub(crate) fn with_var(mut self, name: &'static str, value: impl Into) -> Self { - self.values.insert(name, value.into()); - self - } +/// Build a `MockEnv` that returns `entries` for `os_string` lookups. +pub(crate) fn mock_env_with(entries: impl IntoIterator) -> MockEnv +where + K: AsRef, + V: Into, +{ + let values = entries + .into_iter() + .map(|(key, value)| (key.as_ref().to_owned(), value.into())) + .collect::>(); + let mut env = MockEnv::new(); + env.expect_os_string() + .returning(move |key| values.get(key).cloned()); + env } -impl EnvProvider for TestEnv { - fn get(&self, key: &str) -> Option { - self.values.get(key).cloned() - } +/// Build a `MockEnv` with no configured `os_string` values. +pub(crate) fn empty_mock_env() -> MockEnv { + mock_env_with(std::iter::empty::<(&str, OsString)>()) } diff --git a/tests/bdd/helpers/config_environment.rs b/tests/bdd/helpers/config_environment.rs index 89b6b731b..65b301b99 100644 --- a/tests/bdd/helpers/config_environment.rs +++ b/tests/bdd/helpers/config_environment.rs @@ -1,40 +1,32 @@ //! Injected configuration environment for in-process BDD CLI merges. +use mockable::MockEnv; +use std::collections::HashMap; use std::ffi::OsString; use clap::ArgMatches; -use netsuke::cli::{Cli, ConfigEnvProvider}; +use netsuke::cli::Cli; use ortho_config::OrthoResult; use crate::bdd::fixtures::TestWorld; -struct ScenarioEnvironment { - entries: Vec<(OsString, OsString)>, -} - -impl ScenarioEnvironment { - fn from_world(world: &TestWorld) -> Self { - let entries = world - .env_vars_forward - .borrow() - .iter() - .map(|(key, value)| (OsString::from(key), value.clone())) - .collect(); - Self { entries } - } -} - -impl ConfigEnvProvider for ScenarioEnvironment { - fn get(&self, key: &str) -> Option { - self.entries - .iter() - .find(|(candidate, _)| candidate.eq_ignore_ascii_case(key)) - .map(|(_, value)| value.clone()) - } - - fn entries(&self) -> Vec<(OsString, OsString)> { - self.entries.clone() - } +fn environment_from_world(world: &TestWorld) -> MockEnv { + let values = world + .env_vars_forward + .borrow() + .iter() + .filter_map(|(key, raw_value)| { + raw_value + .to_str() + .map(|text| (key.clone(), text.to_owned())) + }) + .collect::>(); + let selector_values = values.clone(); + let mut env = MockEnv::new(); + env.expect_os_string() + .returning(move |key| selector_values.get(key).map(OsString::from)); + env.expect_all().return_const(values); + env } /// Merge CLI configuration using only the environment recorded in `world`. @@ -43,5 +35,5 @@ pub fn merge_with_world_env( cli: &Cli, matches: &ArgMatches, ) -> OrthoResult { - netsuke::cli::merge_with_config_and_env(cli, matches, &ScenarioEnvironment::from_world(world)) + netsuke::cli::merge_with_config_and_env(cli, matches, &environment_from_world(world)) } diff --git a/tests/cli_tests/merge_diag.rs b/tests/cli_tests/merge_diag.rs index 000351d29..ce2170696 100644 --- a/tests/cli_tests/merge_diag.rs +++ b/tests/cli_tests/merge_diag.rs @@ -2,28 +2,11 @@ use anyhow::{Context, Result, ensure}; use cap_std::{ambient_authority, fs::Dir}; +use mockable::MockEnv; use netsuke::cli_localization; -use std::{collections::HashMap, ffi::OsString, sync::Arc}; +use std::{ffi::OsString, sync::Arc}; use tempfile::tempdir; -#[derive(Default)] -struct TestEnv { - values: HashMap<&'static str, OsString>, -} - -impl TestEnv { - fn with_var(mut self, name: &'static str, value: impl Into) -> Self { - self.values.insert(name, value.into()); - self - } -} - -impl netsuke::cli::ConfigEnvProvider for TestEnv { - fn get(&self, key: &str) -> Option { - self.values.get(key).cloned() - } -} - #[test] fn resolve_merged_json_honours_injected_env() -> Result<()> { let temp_dir = tempdir().context("create temporary config directory")?; @@ -39,7 +22,9 @@ fn resolve_merged_json_honours_injected_env() -> Result<()> { let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke", "--config", &config_arg], &localizer) .context("parse CLI args for injected JSON env")?; - let env = TestEnv::default().with_var("NETSUKE_JSON", "1"); + let mut env = MockEnv::new(); + env.expect_os_string() + .returning(|key| (key == "NETSUKE_JSON").then(|| OsString::from("1"))); ensure!( netsuke::cli::resolve_merged_json_with_env(&cli, &matches, &env)?, From 38b53aa3c89950f25540dbe89289088f1cc310d8 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 04:13:28 +0200 Subject: [PATCH 02/21] Cache config file layer discovery (#319) Discover file-backed configuration layers once during diagnostic resolution and pass that result into the full merge. This preserves existing standalone merge behaviour while removing repeated startup file loading. Keep verbose selector tracing by replaying the cached decision after the diagnostic output mode enables the tracing filter. Cover the shared flow with a mock environment that permits one config-selector lookup only. --- src/cli/diag.rs | 73 ++++++----- src/cli/discovery.rs | 215 +++++++++++++------------------ src/cli/discovery_layer_tests.rs | 92 ++++++++++++- src/cli/merge.rs | 41 +++--- src/cli/mod.rs | 7 +- src/main.rs | 26 ++-- tests/cli_tests/merge_diag.rs | 43 ++++++- 7 files changed, 294 insertions(+), 203 deletions(-) diff --git a/src/cli/diag.rs b/src/cli/diag.rs index fff7c9288..bffa5ef8a 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -7,14 +7,12 @@ use clap::ArgMatches; use clap::parser::ValueSource; +use mockable::{DefaultEnv, Env}; use ortho_config::{OrthoError, OrthoResult}; use serde_json::Value; use std::sync::Arc; -use super::discovery::{ - DiscoverySources, EnvProvider, StdEnvProvider, collect_diag_file_layers_with_sources, - discovery_env_source, -}; +use super::discovery::{DiscoveredLayers, collect_diag_file_layers_with_env}; use super::parser::Cli; const JSON_ENV_VAR: &str = "NETSUKE_JSON"; @@ -29,12 +27,7 @@ const JSON_ENV_VAR: &str = "NETSUKE_JSON"; /// Returns an [`ortho_config::OrthoError`] when a selected config file cannot /// be loaded, or when `NETSUKE_JSON` contains an invalid boolean. pub fn resolve_merged_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - resolve_merged_json_with_sources( - cli, - matches, - &StdEnvProvider, - Arc::new(ortho_config::ProcessEnv), - ) + resolve_merged_json_with_env(cli, matches, &DefaultEnv) } /// Resolve the JSON preference using an injected environment provider. @@ -49,26 +42,37 @@ pub fn resolve_merged_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult pub fn resolve_merged_json_with_env( cli: &Cli, matches: &ArgMatches, - env: &impl EnvProvider, + env: &impl Env, ) -> OrthoResult { - resolve_merged_json_with_sources(cli, matches, env, discovery_env_source(env)) + let (json, _) = resolve_json_and_layers_with_env(cli, matches, env)?; + Ok(json) } -/// Resolve JSON using the same selector and discovery adapters as merging. -fn resolve_merged_json_with_sources( +/// Resolve diagnostic JSON mode and retain the discovered file layers. +/// +/// The returned layers belong to this exact resolution pass and must be passed +/// to [`super::merge::merge_with_layers`] for the subsequent full merge. +/// +/// # Errors +/// +/// Returns the first discovery error immediately, or a validation error when +/// `NETSUKE_JSON` contains an invalid boolean. +pub fn resolve_json_and_layers_with_env( cli: &Cli, matches: &ArgMatches, - env: &impl EnvProvider, - discovery_env: ortho_config::SharedEnvSource, -) -> OrthoResult { - let sources = DiscoverySources::new(env, discovery_env); - let mut json = json_from_file_layers(cli, &sources)?; + env: &impl Env, +) -> OrthoResult<(bool, DiscoveredLayers)> { + let layers = collect_diag_file_layers_with_env(cli, env); + if let Some(error) = layers.first_error() { + return Err(Arc::clone(error)); + } + let mut json = json_from_layers(layers.layers()); if !has_cli_json_override(matches) && let Some(env_json) = json_from_env(env)? { json = env_json; } - Ok(json_from_matches(cli, matches, json)) + Ok((json_from_matches(cli, matches, json), layers)) } fn json_from_layer(value: &Value) -> Option { @@ -92,28 +96,23 @@ fn has_cli_json_override(matches: &ArgMatches) -> bool { matches.value_source("json") == Some(ValueSource::CommandLine) } -/// Resolve the last valid JSON preference from the selected config layers. -fn json_from_file_layers( - cli: &Cli, - sources: &DiscoverySources<'_, impl EnvProvider>, -) -> OrthoResult { - let default = Cli::default().json; - let layers = collect_diag_file_layers_with_sources(cli, sources)?; - let mut json = default; +/// Resolve the last valid JSON preference from discovered config layers. +fn json_from_layers(layers: &[ortho_config::MergeLayer<'static>]) -> bool { + let mut json = Cli::default().json; for layer in layers { - if let Some(layer_json) = json_from_layer(&layer.into_value()) { + if let Some(layer_json) = json_from_layer(&layer.clone().into_value()) { json = layer_json; } } - Ok(json) + json } /// Parse the optional `NETSUKE_JSON` value supplied by `env`. /// /// Invalid or non-Unicode values are validation errors rather than silently /// falling back, so users receive actionable configuration feedback. -fn json_from_env(env: &impl EnvProvider) -> OrthoResult> { - let Some(value) = env.get(JSON_ENV_VAR) else { +fn json_from_env(env: &impl Env) -> OrthoResult> { + let Some(value) = env.os_string(JSON_ENV_VAR) else { return Ok(None); }; let raw = value.into_string().map_err(|invalid_value| { @@ -140,7 +139,7 @@ mod tests { //! Unit tests for early JSON preference resolution. use super::*; - use crate::cli::test_support::TestEnv; + use crate::cli::test_support::mock_env_with; use anyhow::ensure; use cap_std::{ambient_authority, fs::Dir}; use clap::CommandFactory; @@ -170,7 +169,7 @@ mod tests { config: Some(config_path), ..Cli::default() }; - let env = TestEnv::default().with_var(JSON_ENV_VAR, "true"); + let env = mock_env_with([(JSON_ENV_VAR, "true")]); ensure!( resolve_merged_json_with_env(&cli, &matches, &env)?, @@ -192,7 +191,7 @@ mod tests { config: Some(config_path), ..Cli::default() }; - let env = TestEnv::default().with_var(JSON_ENV_VAR, "yes"); + let env = mock_env_with([(JSON_ENV_VAR, "yes")]); let error = resolve_merged_json_with_env(&cli, &matches, &env) .expect_err("invalid JSON env value should fail"); @@ -207,7 +206,7 @@ mod tests { let dir = tempdir()?; let missing_config_path = dir.path().join("missing-netsuke.toml"); let matches = Cli::command().get_matches_from(["netsuke"]); - let env = TestEnv::default().with_var("NETSUKE_CONFIG", &missing_config_path); + let env = mock_env_with([("NETSUKE_CONFIG", missing_config_path.as_os_str().to_owned())]); let error = resolve_merged_json_with_env(&Cli::default(), &matches, &env) .expect_err("missing injected explicit config should fail"); @@ -231,7 +230,7 @@ mod tests { let args = ["netsuke", "--config", config_path_string, "--json"]; let cli = Cli::parse_from(args); let matches = Cli::command().get_matches_from(args); - let env = TestEnv::default().with_var(JSON_ENV_VAR, "yes"); + let env = mock_env_with([(JSON_ENV_VAR, "yes")]); ensure!( resolve_merged_json_with_env(&cli, &matches, &env)?, diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index 0bc3ed9aa..e0fe50619 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -4,11 +4,9 @@ //! through [`ConfigDiscovery`], handling explicit paths from CLI flags and //! environment variables, and loading TOML chains into [`MergeLayer`] values. -use ortho_config::{ - MapEnv, MergeComposer, MergeLayer, OrthoResult, SharedEnvSource, load_config_file_as_chain, -}; +use mockable::Env; +use ortho_config::{MergeComposer, MergeLayer, OrthoResult, load_config_file_as_chain}; use std::borrow::Cow; -use std::ffi::OsString; use std::io; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -28,87 +26,79 @@ use diagnostics::{ ConfigLoadFailureKind, debug_config_path, path_hash, trace_config_path_variable, warn_explicit_config_load_failed, }; -use layers::collect_file_layers_with_env_source; +use layers::collect_file_layers; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; -const DISCOVERY_ENV_KEYS: [&str; 7] = [ - CONFIG_ENV_VAR, - "HOME", - "USERPROFILE", - "XDG_CONFIG_HOME", - "XDG_CONFIG_DIRS", - "APPDATA", - "LOCALAPPDATA", -]; - -/// Provides access to environment variables used during config discovery. + +/// File layers and loading errors produced by one discovery pass. /// -/// Production code uses [`StdEnvProvider`]. Tests can provide an in-memory -/// implementation so config-selection logic does not mutate process-global -/// environment state. -pub trait EnvProvider { - /// Return the value of `key`, or `None` when the key is unset. - fn get(&self, key: &str) -> Option; - - /// Return all values available to the configuration environment layer. - /// - /// Providers concerned only with selector lookup may retain the empty - /// default. Full merge providers override this method. - fn entries(&self) -> Vec<(OsString, OsString)> { - Vec::new() - } +/// The diagnostic pre-pass borrows the layers to resolve JSON output, then the +/// full merge consumes the same result. Keeping errors beside the layers lets +/// those phases retain their distinct error policies without rediscovery. +pub struct DiscoveredLayers { + layers: Vec>, + errors: Vec>, + resolution: ConfigPathResolution, } -/// Environment provider backed by [`std::env::var_os`]. -#[derive(Debug, Default, Clone, Copy)] -pub struct StdEnvProvider; - -#[expect( - clippy::disallowed_methods, - reason = "composition root: StdEnvProvider is the process-backed adapter behind the EnvProvider seam" -)] -impl EnvProvider for StdEnvProvider { - fn get(&self, key: &str) -> Option { - std::env::var_os(key) +impl DiscoveredLayers { + /// Borrow the file layers in discovery order. + pub(crate) fn layers(&self) -> &[MergeLayer<'static>] { + &self.layers } - fn entries(&self) -> Vec<(OsString, OsString)> { - std::env::vars_os().collect() + /// Borrow the first discovery error, if loading failed. + pub(crate) fn first_error(&self) -> Option<&Arc> { + self.errors.first() } -} -/// Environment adapters consumed by configuration file discovery. -/// -/// The value adapter remains the Netsuke-owned [`EnvProvider`] port, whereas -/// `discovery_env` is the deliberately narrow `OrthoConfig` adapter. Keeping -/// them together makes every composition root choose both dependencies -/// explicitly and prevents a test-only environment from leaking ambient reads. -pub(crate) struct DiscoverySources<'a, E: EnvProvider + ?Sized> { - env: &'a E, - discovery_env: SharedEnvSource, + /// Consume the result into its reusable layers and deferred errors. + pub(crate) fn into_parts( + self, + ) -> (Vec>, Vec>) { + (self.layers, self.errors) + } + + /// Re-emit the bounded selector diagnostic without querying the environment. + /// + /// Startup enables verbose tracing only after resolving its diagnostic mode. + /// Replaying the cached decision at that point preserves the existing trace + /// event without repeating configuration discovery or file loading. + pub fn replay_config_path_trace(&self) { + trace_config_path_resolution(&self.resolution); + } } -impl<'a, E: EnvProvider + ?Sized> DiscoverySources<'a, E> { - /// Construct the discovery adapters selected by one composition root. - pub(crate) fn new(env: &'a E, discovery_env: SharedEnvSource) -> Self { - Self { env, discovery_env } +/// Discover configuration layers once through the injected environment. +pub(crate) fn discover_file_layers(cli: &Cli, env: &impl Env) -> DiscoveredLayers { + let (resolution, outcome) = collect_file_layers_with_env(cli, env); + match outcome { + Ok(layers) => DiscoveredLayers { + layers, + errors: Vec::new(), + resolution, + }, + Err(error) => DiscoveredLayers { + layers: Vec::new(), + errors: vec![error], + resolution, + }, } } -/// Load configuration layers with explicit selector and discovery adapters. -pub(crate) fn push_file_layers_with_sources( - cli: &Cli, +/// Add a discovered file layer result to a merge composition. +/// +/// Discovery errors join the merge error collection, retaining the normal +/// merge path's accumulated-error behaviour. +pub(crate) fn push_discovered_file_layers( composer: &mut MergeComposer, errors: &mut Vec>, - sources: &DiscoverySources<'_, impl EnvProvider>, + discovered: DiscoveredLayers, ) { - match collect_file_layers_with_env(cli, sources) { - Ok(layers) => { - for layer in layers { - composer.push_layer(layer); - } - } - Err(err) => errors.push(err), + let (layers, discovery_errors) = discovered.into_parts(); + errors.extend(discovery_errors); + for layer in layers { + composer.push_layer(layer); } } @@ -118,38 +108,21 @@ pub(crate) fn push_file_layers_with_sources( /// select the same file layers while retaining their own error handling. fn collect_file_layers_with_env( cli: &Cli, - sources: &DiscoverySources<'_, impl EnvProvider>, -) -> OrthoResult>> { - let resolution = resolve_config_selector(cli.config.clone(), sources.env); + env: &impl Env, +) -> (ConfigPathResolution, OrthoResult>>) { + let resolution = resolve_config_selector(cli.config.clone(), env); trace_config_path_resolution(&resolution); - resolution.path.map_or_else( + let outcome = resolution.path.as_deref().map_or_else( || { debug!("using config discovery"); - collect_file_layers_with_env_source( - cli.directory.as_deref(), - Arc::clone(&sources.discovery_env), - ) + collect_file_layers(cli.directory.as_deref()) }, |path| { - debug_config_path("using explicit config path", &path); - load_layers_from_path(&path) + debug_config_path("using explicit config path", path); + load_layers_from_path(path) }, - ) -} - -/// Project the fixed discovery inputs from Netsuke's environment port. -/// -/// This adapter is private to CLI configuration composition. It intentionally -/// exposes only discovery's documented lookup keys: `EnvironmentLayer` remains -/// the sole owner of complete `NETSUKE_*` enumeration for value merging. -pub(crate) fn discovery_env_source(env: &impl EnvProvider) -> SharedEnvSource { - let mut source = MapEnv::new(); - for key in DISCOVERY_ENV_KEYS { - if let Some(value) = env.get(key) { - source.insert(key, value); - } - } - Arc::new(source) + ); + (resolution, outcome) } /// Select an explicit config path, giving `--config` precedence over `env`. @@ -161,7 +134,7 @@ pub(crate) fn discovery_env_source(env: &impl EnvProvider) -> SharedEnvSource { /// Production code takes the richer [`ConfigPathResolution`] so it can trace the /// environment lookups, leaving this as a convenience for precedence tests. #[cfg(test)] -pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl EnvProvider) -> Option { +pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl Env) -> Option { resolve_config_selector(cli.config.clone(), env).path } @@ -170,7 +143,7 @@ pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl EnvProvider) - /// Records the winning selector, its optional path, and every environment /// lookup evaluated to reach the decision, so a caller can emit diagnostics /// afterwards without giving the query tracing side effects. -#[derive(Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] struct ConfigPathResolution { selector: &'static str, path: Option, @@ -181,10 +154,7 @@ struct ConfigPathResolution { /// /// `cli_config` wins when present, in which case no environment lookup is /// recorded because none is performed. This query emits no tracing. -fn resolve_config_selector( - cli_config: Option, - env: &impl EnvProvider, -) -> ConfigPathResolution { +fn resolve_config_selector(cli_config: Option, env: &impl Env) -> ConfigPathResolution { if let Some(path) = cli_config { return ConfigPathResolution { selector: "cli_flag", @@ -222,8 +192,8 @@ fn trace_config_path_resolution(resolution: &ConfigPathResolution) { /// /// Returns `None` when the variable is unset or empty, so discovery still runs. /// This query emits no tracing. -fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option { - env.get(var_name) +fn env_config_path(env: &impl Env, var_name: &str) -> Option { + env.os_string(var_name) .filter(|value| !value.is_empty()) .map(PathBuf::from) } @@ -259,22 +229,12 @@ pub(crate) fn load_layers_from_path( } } -/// Load diagnostic file layers with the same discovery adapter as merging. -pub(crate) fn collect_diag_file_layers_with_sources( - cli: &Cli, - sources: &DiscoverySources<'_, impl EnvProvider>, -) -> OrthoResult>> { +/// Load file layers for early JSON resolution using injected environment access. +/// +/// This delegates to the same precedence boundary as the normal merge path. +pub(crate) fn collect_diag_file_layers_with_env(cli: &Cli, env: &impl Env) -> DiscoveredLayers { let _span = debug_span!("collect_diag_file_layers").entered(); - collect_file_layers_with_env(cli, sources) -} - -#[cfg(test)] -fn collect_diag_file_layers_with_env( - cli: &Cli, - env: &impl EnvProvider, -) -> OrthoResult>> { - let sources = DiscoverySources::new(env, discovery_env_source(env)); - collect_diag_file_layers_with_sources(cli, &sources) + discover_file_layers(cli, env) } #[cfg(test)] @@ -298,7 +258,7 @@ mod tests { //! Unit tests for config discovery through injected environment access. use super::*; - use crate::cli::test_support::TestEnv; + use crate::cli::test_support::{empty_mock_env, mock_env_with}; use anyhow::ensure; use cap_std::{ambient_authority, fs::Dir}; use rstest::rstest; @@ -306,19 +266,19 @@ mod tests { #[test] fn env_config_path_returns_none_when_var_unset() { - let env = TestEnv::default(); + let env = empty_mock_env(); assert!(env_config_path(&env, "__NETSUKE_TEST_VAR").is_none()); } #[test] fn env_config_path_returns_none_when_var_empty() { - let env = TestEnv::default().with_var("__NETSUKE_TEST_VAR", ""); + let env = mock_env_with([("__NETSUKE_TEST_VAR", "")]); assert!(env_config_path(&env, "__NETSUKE_TEST_VAR").is_none()); } #[test] fn env_config_path_returns_path_when_var_set() { - let env = TestEnv::default().with_var("__NETSUKE_TEST_VAR", "/tmp/foo.toml"); + let env = mock_env_with([("__NETSUKE_TEST_VAR", "/tmp/foo.toml")]); let result = env_config_path(&env, "__NETSUKE_TEST_VAR"); assert_eq!(result, Some(PathBuf::from("/tmp/foo.toml"))); } @@ -336,10 +296,9 @@ mod tests { #[case] cli_path: Option<&'static str>, #[case] expected: Option<&'static str>, ) { - let mut env = TestEnv::default(); - if let Some(path) = env_path { - env = env.with_var(CONFIG_ENV_VAR, path); - } + let env = env_path.map_or_else(empty_mock_env, |path| { + mock_env_with([(CONFIG_ENV_VAR, path)]) + }); let cli = Cli { config: cli_path.map(PathBuf::from), ..Cli::default() @@ -358,12 +317,12 @@ mod tests { let config_dir = Dir::open_ambient_dir(dir.path(), ambient_authority())?; config_dir.write("netsuke.toml", b"json = true\n")?; - let env = TestEnv::default().with_var(CONFIG_ENV_VAR, config_path.as_os_str()); - let layers = collect_diag_file_layers_with_env(&Cli::default(), &env)?; + let env = mock_env_with([(CONFIG_ENV_VAR, config_path.as_os_str().to_owned())]); + let layers = collect_diag_file_layers_with_env(&Cli::default(), &env); let expected_path = config_path.to_string_lossy().into_owned(); ensure!( - layers.iter().any(|layer| layer + layers.layers().iter().any(|layer| layer .path() .is_some_and(|path| path.as_str() == expected_path)), "should include the injected explicit config layer at {expected_path}" diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 17eb493ba..4d86b81b7 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -56,8 +56,10 @@ fn collect_diag_file_layers_logs_selected_branch( let cli = scenario_cli(scenario, &temp)?; let env = empty_mock_env(); - let (layers, events) = capture_events(|| collect_diag_file_layers_with_env(&cli, &env))?; + let (discovered, events) = + capture_events(|| Ok::<_, anyhow::Error>(collect_diag_file_layers_with_env(&cli, &env)))?; let branch_event = find_event(&events, expected_event)?; + let layers = discovered.layers(); ensure!( layers.is_empty() == should_be_empty, @@ -235,3 +237,91 @@ fn non_utf8_directory_does_not_duplicate_project_layer() -> Result<()> { ensure!(layers.len() == 1, "expected one project layer: {layers:?}"); Ok(()) } + +#[test] +fn discover_file_layers_loads_an_explicit_config() -> Result<()> { + let dir = tempdir().context("create temporary config directory")?; + let config_path = dir.path().join("netsuke.toml"); + test_support::fs::write(&config_path, "json = true\n").context("write config")?; + let cli = Cli { + config: Some(config_path), + ..Cli::default() + }; + + let discovered = discover_file_layers(&cli, &empty_mock_env()); + + ensure!( + discovered.layers().len() == 1, + "the explicit config should produce one layer" + ); + ensure!( + discovered.errors.is_empty(), + "the explicit config should not produce discovery errors" + ); + Ok(()) +} + +#[test] +fn discover_file_layers_records_an_explicit_load_error() -> Result<()> { + let dir = tempdir().context("create temporary config directory")?; + let cli = Cli { + config: Some(dir.path().join("missing.toml")), + ..Cli::default() + }; + + let discovered = discover_file_layers(&cli, &empty_mock_env()); + + ensure!( + discovered.layers().is_empty(), + "a missing explicit config should not produce layers" + ); + ensure!( + discovered.errors.len() == 1, + "a missing explicit config should record one error" + ); + Ok(()) +} + +#[test] +fn discover_file_layers_supports_discovery_without_a_selector() -> Result<()> { + let dir = tempdir().context("create temporary project directory")?; + let cli = Cli { + directory: Some(dir.path().to_path_buf()), + ..Cli::default() + }; + + let discovered = discover_file_layers(&cli, &empty_mock_env()); + + ensure!( + discovered.layers().is_empty(), + "an empty directory should not produce config layers" + ); + ensure!( + discovered.errors.is_empty(), + "an empty directory should not produce discovery errors" + ); + Ok(()) +} + +#[test] +fn discover_file_layers_performs_the_project_scope_second_pass() -> Result<()> { + let dir = tempdir().context("create temporary project directory")?; + test_support::fs::write(dir.path().join(".netsuke.toml"), "jobs = 7\n") + .context("write project config")?; + let cli = Cli { + directory: Some(dir.path().to_path_buf()), + ..Cli::default() + }; + + let discovered = discover_file_layers(&cli, &empty_mock_env()); + + ensure!( + discovered.layers().len() == 1, + "the project pass should discover its config layer" + ); + ensure!( + discovered.errors.is_empty(), + "the project pass should not produce discovery errors" + ); + Ok(()) +} diff --git a/src/cli/merge.rs b/src/cli/merge.rs index 6af1a3413..d73ec2eff 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -22,19 +22,16 @@ use clap::ArgMatches; use clap::parser::ValueSource; +use mockable::{DefaultEnv, Env}; use ortho_config::declarative::LayerComposition; use ortho_config::figment::Figment; use ortho_config::{MergeComposer, OrthoMergeExt, OrthoResult, sanitize_value}; use serde::Serialize; -use std::sync::Arc; use serde_json::{Map, Value, json}; use super::config::{BuildConfig, CliConfig}; -use super::discovery::{ - DiscoverySources, EnvProvider, StdEnvProvider, discovery_env_source, - push_file_layers_with_sources, -}; +use super::discovery::{DiscoveredLayers, discover_file_layers, push_discovered_file_layers}; use super::environment::EnvironmentLayer; use super::parser::{BuildArgs, Cli, Commands}; use super::validation_error; @@ -46,12 +43,7 @@ use super::validation_error; /// Returns an [`ortho_config::OrthoError`] if layer composition or merging /// fails. pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - merge_with_config_sources( - cli, - matches, - &StdEnvProvider, - Arc::new(ortho_config::ProcessEnv), - ) + merge_with_config_and_env(cli, matches, &DefaultEnv) } /// Merge configuration layers using an explicit environment provider. @@ -67,17 +59,22 @@ pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { pub fn merge_with_config_and_env( cli: &Cli, matches: &ArgMatches, - env: &impl EnvProvider, + env: &impl Env, ) -> OrthoResult { - merge_with_config_sources(cli, matches, env, discovery_env_source(env)) + merge_with_layers(cli, matches, env, discover_file_layers(cli, env)) } -/// Merge configuration using distinct value and discovery adapters. -fn merge_with_config_sources( +/// Merge configuration using file layers discovered by an earlier phase. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] if layer composition or merging +/// fails. +pub fn merge_with_layers( cli: &Cli, matches: &ArgMatches, - env: &impl EnvProvider, - discovery_env: ortho_config::SharedEnvSource, + env: &impl Env, + layers: DiscoveredLayers, ) -> OrthoResult { let mut errors = Vec::new(); let mut composer = MergeComposer::with_capacity(4); @@ -87,10 +84,14 @@ fn merge_with_config_sources( Err(err) => errors.push(err), } - let discovery_sources = DiscoverySources::new(env, discovery_env); - push_file_layers_with_sources(cli, &mut composer, &mut errors, &discovery_sources); + push_discovered_file_layers(&mut composer, &mut errors, layers); - match Figment::from(EnvironmentLayer::new(env.entries())) + let environment_entries = env + .all() + .into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect(); + match Figment::from(EnvironmentLayer::new(environment_entries)) .extract::() .into_ortho_merge() { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 2f6d622dd..b3c3f9f89 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -21,8 +21,11 @@ mod parsing; pub(crate) mod test_support; pub use config::{AccessibilityPolicy, CliConfig, ColourPolicy, EmojiPolicy, ProgressPolicy}; -pub use diag::{resolve_merged_json, resolve_merged_json_with_env}; -pub use merge::{merge_with_config, merge_with_config_and_env}; +pub use diag::{ + resolve_json_and_layers_with_env, resolve_merged_json, resolve_merged_json_with_env, +}; +pub use discovery::DiscoveredLayers; +pub use merge::{merge_with_config, merge_with_config_and_env, merge_with_layers}; pub use parser::{ BuildArgs, Cli, Commands, GraphArgs, json_hint_from_args, locale_hint_from_args, parse_with_localizer_from, diff --git a/src/main.rs b/src/main.rs index 9287dc428..4df21a3d9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,6 +5,7 @@ use clap::ArgMatches; use clap::error::ErrorKind; use miette::Report; +use mockable::DefaultEnv; use netsuke::theme::ThemeContext; use netsuke::{ cli, cli_localization, diagnostic_json, locale_resolution, localization, manifest, output_mode, @@ -85,8 +86,8 @@ fn run_with_args( Err(code) => return code, }; - let mode = match resolve_json_mode_or_exit(&parsed_cli, &matches, startup_mode) { - Ok(mode) => mode, + let (mode, layers) = match resolve_diag_mode_or_exit(&parsed_cli, &matches, startup_mode) { + Ok(resolved) => resolved, Err(code) => { settle_startup_diagnostics(&startup_writer, startup_mode); return code; @@ -95,7 +96,7 @@ fn run_with_args( // The effective mode is known here, before configuration is merged, so the // startup warning reaches the user ahead of any configuration processing. settle_startup_diagnostics(&startup_writer, mode); - let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode) { + let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode, layers) { Ok(merged) => merged, Err(code) => return code, }; @@ -219,25 +220,21 @@ fn config_err_to_exit(err: &(dyn std::error::Error + 'static), mode: DiagMode) - } } -fn resolve_json_mode_or_exit( +fn resolve_diag_mode_or_exit( parsed_cli: &cli::Cli, matches: &ArgMatches, fallback_mode: DiagMode, -) -> Result { - match cli::resolve_merged_json(parsed_cli, matches) { - Ok(is_json_enabled) => { +) -> Result<(DiagMode, cli::DiscoveredLayers), ExitCode> { + match cli::resolve_json_and_layers_with_env(parsed_cli, matches, &DefaultEnv) { + Ok((is_json_enabled, layers)) => { let mode = DiagMode::from_json_enabled(is_json_enabled); set_tracing_filter(startup_filter(mode, parsed_cli.verbose)); - Ok(mode) + layers.replay_config_path_trace(); + Ok((mode, layers)) } Err(err) => { let fallback_filter = startup_filter(fallback_mode, parsed_cli.verbose); set_tracing_filter(fallback_filter); - // Resolution failed before its diagnostics could be emitted. Replay - // only for human output after enabling its filter; JSON remains OFF. - if fallback_filter != LevelFilter::OFF { - drop(cli::resolve_merged_json(parsed_cli, matches)); - } Err(config_err_to_exit(err.as_ref(), fallback_mode)) } } @@ -247,8 +244,9 @@ fn merge_cli_or_exit( parsed_cli: &cli::Cli, matches: &ArgMatches, mode: DiagMode, + layers: cli::DiscoveredLayers, ) -> Result { - cli::merge_with_config(parsed_cli, matches) + cli::merge_with_layers(parsed_cli, matches, &DefaultEnv, layers) .map(cli::Cli::with_default_command) .map_err(|err| config_err_to_exit(err.as_ref(), mode)) } diff --git a/tests/cli_tests/merge_diag.rs b/tests/cli_tests/merge_diag.rs index ce2170696..6a87a2043 100644 --- a/tests/cli_tests/merge_diag.rs +++ b/tests/cli_tests/merge_diag.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result, ensure}; use cap_std::{ambient_authority, fs::Dir}; use mockable::MockEnv; use netsuke::cli_localization; -use std::{ffi::OsString, sync::Arc}; +use std::{collections::HashMap, ffi::OsString, sync::Arc}; use tempfile::tempdir; #[test] @@ -33,3 +33,44 @@ fn resolve_merged_json_honours_injected_env() -> Result<()> { Ok(()) } + +#[test] +fn diag_and_merge_reuse_one_discovery_result() -> Result<()> { + let temp_dir = tempdir().context("create temporary config directory")?; + let config_path = temp_dir.path().join("netsuke.toml"); + let config_dir = Dir::open_ambient_dir(temp_dir.path(), ambient_authority()) + .context("open temporary config directory")?; + config_dir + .write("netsuke.toml", b"json = true\njobs = 13\n") + .context("write netsuke.toml")?; + + let localizer = Arc::from(cli_localization::build_localizer(None)); + let (cli, matches) = + netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer).context("parse CLI")?; + let config_selector = config_path.as_os_str().to_owned(); + let mut env = MockEnv::new(); + env.expect_os_string() + .withf(|key| key == "NETSUKE_CONFIG") + .once() + .return_once(move |_| Some(config_selector)); + env.expect_os_string() + .withf(|key| key == "NETSUKE_JSON") + .once() + .return_const(None::); + env.expect_all().once().return_const(HashMap::new()); + + let (is_json, layers) = netsuke::cli::resolve_json_and_layers_with_env(&cli, &matches, &env) + .context("resolve diagnostic mode and discovered layers")?; + let merged = netsuke::cli::merge_with_layers(&cli, &matches, &env, layers) + .context("merge the cached layers")?; + + ensure!( + is_json, + "the cached file layer should enable JSON diagnostics" + ); + ensure!( + merged.jobs == Some(13), + "the merge should consume the discovered config layer" + ); + Ok(()) +} From 242f27dda33256d3c50d4d88ca653c5858614324 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 9 Aug 2026 17:06:32 +0200 Subject: [PATCH 03/21] Preserve raw configuration environment entries (#319) Keep non-Unicode configuration selectors and process environment entries in their raw form so selection remains correct and environment-layer validation can apply its documented policy without a startup panic. Retain cached selector diagnostics through failed startup resolution, so verbose users receive the same bounded context for configuration errors. --- src/cli/diag.rs | 38 ++++++++++++----- src/cli/merge.rs | 54 ++++++++++++++++++++++--- src/cli/mod.rs | 8 +++- src/main.rs | 9 +++-- tests/bdd/helpers/config_environment.rs | 29 +++++++++++-- tests/logging_stderr/config_tracing.rs | 4 ++ 6 files changed, 117 insertions(+), 25 deletions(-) diff --git a/src/cli/diag.rs b/src/cli/diag.rs index bffa5ef8a..dba5f2ace 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -62,17 +62,35 @@ pub fn resolve_json_and_layers_with_env( matches: &ArgMatches, env: &impl Env, ) -> OrthoResult<(bool, DiscoveredLayers)> { + let (result, layers) = resolve_json_and_layers_outcome_with_env(cli, matches, env); + result.map(|json| (json, layers)) +} + +/// Resolve diagnostic JSON mode while retaining layers on both outcomes. +/// +/// Startup uses this form to replay the cached selector diagnostic after it +/// enables its output filter, including when discovery or JSON validation +/// fails. Standalone callers should usually prefer +/// [`resolve_json_and_layers_with_env`]. +pub fn resolve_json_and_layers_outcome_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl Env, +) -> (OrthoResult, DiscoveredLayers) { let layers = collect_diag_file_layers_with_env(cli, env); - if let Some(error) = layers.first_error() { - return Err(Arc::clone(error)); - } - let mut json = json_from_layers(layers.layers()); - if !has_cli_json_override(matches) - && let Some(env_json) = json_from_env(env)? - { - json = env_json; - } - Ok((json_from_matches(cli, matches, json), layers)) + let result = (|| { + if let Some(error) = layers.first_error() { + return Err(Arc::clone(error)); + } + let mut json = json_from_layers(layers.layers()); + if !has_cli_json_override(matches) + && let Some(env_json) = json_from_env(env)? + { + json = env_json; + } + Ok(json_from_matches(cli, matches, json)) + })(); + (result, layers) } fn json_from_layer(value: &Value) -> Option { diff --git a/src/cli/merge.rs b/src/cli/merge.rs index d73ec2eff..405acb607 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -29,6 +29,7 @@ use ortho_config::{MergeComposer, OrthoMergeExt, OrthoResult, sanitize_value}; use serde::Serialize; use serde_json::{Map, Value, json}; +use std::ffi::OsString; use super::config::{BuildConfig, CliConfig}; use super::discovery::{DiscoveredLayers, discover_file_layers, push_discovered_file_layers}; @@ -43,7 +44,7 @@ use super::validation_error; /// Returns an [`ortho_config::OrthoError`] if layer composition or merging /// fails. pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - merge_with_config_and_env(cli, matches, &DefaultEnv) + merge_with_process_environment_layers(cli, matches, discover_file_layers(cli, &DefaultEnv)) } /// Merge configuration layers using an explicit environment provider. @@ -64,6 +65,25 @@ pub fn merge_with_config_and_env( merge_with_layers(cli, matches, env, discover_file_layers(cli, env)) } +/// Merge cached file layers with a raw snapshot of the process environment. +/// +/// This is the production composition boundary. It retains non-Unicode +/// entries so `EnvironmentLayer` can reject Netsuke-prefixed invalid entries +/// and ignore unrelated ones, rather than letting [`std::env::vars`] panic. +/// Selector and JSON reads still use [`DefaultEnv`] through the injected seam. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] if layer composition or merging +/// fails. +pub fn merge_with_process_environment_layers( + cli: &Cli, + matches: &ArgMatches, + layers: DiscoveredLayers, +) -> OrthoResult { + merge_with_layers_and_entries(cli, matches, layers, process_environment_entries()) +} + /// Merge configuration using file layers discovered by an earlier phase. /// /// # Errors @@ -75,6 +95,20 @@ pub fn merge_with_layers( matches: &ArgMatches, env: &impl Env, layers: DiscoveredLayers, +) -> OrthoResult { + let environment_entries = env + .all() + .into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect(); + merge_with_layers_and_entries(cli, matches, layers, environment_entries) +} + +fn merge_with_layers_and_entries( + cli: &Cli, + matches: &ArgMatches, + layers: DiscoveredLayers, + environment_entries: Vec<(OsString, OsString)>, ) -> OrthoResult { let mut errors = Vec::new(); let mut composer = MergeComposer::with_capacity(4); @@ -86,11 +120,6 @@ pub fn merge_with_layers( push_discovered_file_layers(&mut composer, &mut errors, layers); - let environment_entries = env - .all() - .into_iter() - .map(|(key, value)| (key.into(), value.into())) - .collect(); match Figment::from(EnvironmentLayer::new(environment_entries)) .extract::() .into_ortho_merge() @@ -110,6 +139,19 @@ pub fn merge_with_layers( Ok(apply_config(cli, merged)) } +/// Snapshot raw process environment entries at the composition boundary. +/// +/// `mockable::Env::all` models only Unicode variables. Configuration merging +/// needs the raw form because [`EnvironmentLayer`] owns the policy for +/// non-Unicode keys and values. +#[expect( + clippy::disallowed_methods, + reason = "composition root: raw entries preserve EnvironmentLayer's non-Unicode policy" +)] +fn process_environment_entries() -> Vec<(OsString, OsString)> { + std::env::vars_os().collect() +} + fn is_empty_value(value: &Value) -> bool { matches!(value, Value::Object(map) if map.is_empty()) } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index b3c3f9f89..bb52d2d0b 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -22,10 +22,14 @@ pub(crate) mod test_support; pub use config::{AccessibilityPolicy, CliConfig, ColourPolicy, EmojiPolicy, ProgressPolicy}; pub use diag::{ - resolve_json_and_layers_with_env, resolve_merged_json, resolve_merged_json_with_env, + resolve_json_and_layers_outcome_with_env, resolve_json_and_layers_with_env, + resolve_merged_json, resolve_merged_json_with_env, }; pub use discovery::DiscoveredLayers; -pub use merge::{merge_with_config, merge_with_config_and_env, merge_with_layers}; +pub use merge::{ + merge_with_config, merge_with_config_and_env, merge_with_layers, + merge_with_process_environment_layers, +}; pub use parser::{ BuildArgs, Cli, Commands, GraphArgs, json_hint_from_args, locale_hint_from_args, parse_with_localizer_from, diff --git a/src/main.rs b/src/main.rs index 4df21a3d9..9999aa67a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -225,8 +225,10 @@ fn resolve_diag_mode_or_exit( matches: &ArgMatches, fallback_mode: DiagMode, ) -> Result<(DiagMode, cli::DiscoveredLayers), ExitCode> { - match cli::resolve_json_and_layers_with_env(parsed_cli, matches, &DefaultEnv) { - Ok((is_json_enabled, layers)) => { + let (result, layers) = + cli::resolve_json_and_layers_outcome_with_env(parsed_cli, matches, &DefaultEnv); + match result { + Ok(is_json_enabled) => { let mode = DiagMode::from_json_enabled(is_json_enabled); set_tracing_filter(startup_filter(mode, parsed_cli.verbose)); layers.replay_config_path_trace(); @@ -235,6 +237,7 @@ fn resolve_diag_mode_or_exit( Err(err) => { let fallback_filter = startup_filter(fallback_mode, parsed_cli.verbose); set_tracing_filter(fallback_filter); + layers.replay_config_path_trace(); Err(config_err_to_exit(err.as_ref(), fallback_mode)) } } @@ -246,7 +249,7 @@ fn merge_cli_or_exit( mode: DiagMode, layers: cli::DiscoveredLayers, ) -> Result { - cli::merge_with_layers(parsed_cli, matches, &DefaultEnv, layers) + cli::merge_with_process_environment_layers(parsed_cli, matches, layers) .map(cli::Cli::with_default_command) .map_err(|err| config_err_to_exit(err.as_ref(), mode)) } diff --git a/tests/bdd/helpers/config_environment.rs b/tests/bdd/helpers/config_environment.rs index 65b301b99..13fa94b4d 100644 --- a/tests/bdd/helpers/config_environment.rs +++ b/tests/bdd/helpers/config_environment.rs @@ -11,9 +11,8 @@ use ortho_config::OrthoResult; use crate::bdd::fixtures::TestWorld; fn environment_from_world(world: &TestWorld) -> MockEnv { - let values = world - .env_vars_forward - .borrow() + let selector_values = world.env_vars_forward.borrow().clone(); + let values = selector_values .iter() .filter_map(|(key, raw_value)| { raw_value @@ -21,7 +20,6 @@ fn environment_from_world(world: &TestWorld) -> MockEnv { .map(|text| (key.clone(), text.to_owned())) }) .collect::>(); - let selector_values = values.clone(); let mut env = MockEnv::new(); env.expect_os_string() .returning(move |key| selector_values.get(key).map(OsString::from)); @@ -37,3 +35,26 @@ pub fn merge_with_world_env( ) -> OrthoResult { netsuke::cli::merge_with_config_and_env(cli, matches, &environment_from_world(world)) } + +#[cfg(test)] +mod tests { + //! Covers preservation of raw non-UTF-8 configuration selectors. + + use super::*; + + #[cfg(unix)] + #[test] + fn environment_from_world_preserves_a_non_utf8_config_selector() { + use mockable::Env; + use std::os::unix::ffi::OsStringExt; + + let world = TestWorld::default(); + let selector = OsString::from_vec(b"/tmp/config-\xff.toml".to_vec()); + world.track_env_var("NETSUKE_CONFIG".to_owned(), Some(selector.clone())); + + let env = environment_from_world(&world); + + assert_eq!(env.os_string("NETSUKE_CONFIG"), Some(selector)); + assert!(env.all().is_empty()); + } +} diff --git a/tests/logging_stderr/config_tracing.rs b/tests/logging_stderr/config_tracing.rs index 3a66a80d1..91e1e1bcf 100644 --- a/tests/logging_stderr/config_tracing.rs +++ b/tests/logging_stderr/config_tracing.rs @@ -133,6 +133,10 @@ fn invalid_config_traces_without_parser_text() -> Result<()> { joined.contains("explicit config load failed") && joined.contains("failure_kind=LoadError"), "stderr should classify the parse failure: {joined}" ); + ensure!( + joined.contains("resolved config path") && joined.contains("selector=\"cli_flag\""), + "verbose stderr should replay the cached selector decision: {joined}" + ); ensure!( !joined.contains("invalid parser secret"), "diagnostics must not echo the parser input: {joined}" From 0d20452f7225a8c46ed1100ca3325fa7b6958c61 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 14:29:21 +0200 Subject: [PATCH 04/21] Update environment seam documentation Document the current mockable environment APIs and cached discovery handoff while removing the retired provider interfaces. --- docs/developers-guide.md | 249 ++++++++++++++++++++++++++------------- 1 file changed, 169 insertions(+), 80 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4ff6789f6..2cea633af 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -434,34 +434,117 @@ NEXTEST_VERSION="$(sed -n "s/.*NEXTEST_VERSION: '\(.*\)'.*/\1/p" \ .github/workflows/ci.yml)" cargo install cargo-nextest --locked --version "$NEXTEST_VERSION" # or, for a prebuilt binary: -cargo binstall --no-confirm "cargo-nextest@$NEXTEST_VERSION" +cargo binstall --no-confirm --locked \ + "whitaker-installer@$WHITAKER_INSTALLER_VERSION" ``` -See [Test execution](#test-execution) for what the checked-in nextest -configuration does and does not cover. - -`make lint` starts with workspace-wide rustdoc through -`RUSTDOCFLAGS="$(RUSTDOC_FLAGS)"` and -`RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)"`. This -denies warnings, enables Polonius, and preserves any `RUSTFLAGS` supplied by -the caller. It then runs workspace-wide -`cargo clippy --workspace --all-targets --all-features` and the -[Whitaker](whitaker-users-guide.md) Dylint suite -(`whitaker --all -- --all-targets --all-features`). Install Whitaker through -the standalone installer described in the -[Whitaker user's guide](whitaker-users-guide.md) so local linting matches -continuous integration (CI); `make lint-clippy` runs the Clippy-only subset. CI -pins the installer version in `WHITAKER_INSTALLER_VERSION` in -`.github/workflows/ci.yml`. Install that same version locally so local runs -match CI; read the pin from the workflow rather than copying the number, so the -two cannot drift: +`whitaker-installer` and the lint libraries are separate artefacts with +separate versions. `WHITAKER_INSTALLER_VERSION` pins the installer — the tool +that stages libraries — and nothing else. The installer keeps its own checkout +of the Whitaker repository under `~/.local/share/whitaker`, updates it with +`git pull`, and stages the libraries from its default branch. Lint behaviour +therefore tracks Whitaker HEAD. + +**Running the lint libraries at HEAD is deliberate.** Netsuke follows the suite +as it develops, so new lints and fixes arrive without a version bump here. Do +not add a `[workspace.metadata.dylint]` block pinning `whitaker_suite` to a +`tag` or `rev`. The [Whitaker user's guide](whitaker-users-guide.md) documents +that form, and it is the right answer for a project wanting reproducible lint +results, but adopting it here would reverse a standing decision rather than fix +a defect. + +The cost is worth stating plainly: a change upstream can alter lint results +between two runs with no change in this repository, and a local checkout that +has not been restaged will disagree with CI, which stages fresh on every job. +Restaging is what reconciles them. + +What the module-scoped exemptions in `dylint.toml` actually depend on is +[Whitaker PR #315][whitaker-pr-315], which added the `excluded_paths` option, +so the staged libraries must be recent enough to include it. Libraries staged +from an older checkout ignore `excluded_paths` silently — the exemptions stop +applying with no error, and the lint reports the modules they covered. Re-run +`whitaker-installer` to restage from HEAD. If that checkout has been left on a +detached HEAD, the install fails at its `git pull`; put it back on the default +branch and re-run. + +[whitaker-pr-315]: https://github.com/leynos/whitaker/pull/315 + +Whitaker is configured by `dylint.toml` at the repository root, where each +sanctioned ambient-filesystem scope for `no_std_fs_operations` carries a +documented rationale. `docs/whitaker-users-guide.md` is a near-verbatim import +of the [upstream Whitaker user's guide][whitaker-upstream-guide]; refresh it +from that URL rather than editing it in place, preserving the "Netsuke +deviation from upstream" callout, and record Netsuke-specific policy here and in +`dylint.toml`. + +[whitaker-upstream-guide]: https://raw.githubusercontent.com/leynos/whitaker/refs/heads/main/docs/users-guide.md + +Prefer `excluded_paths` over `excluded_crates`: a path entry exempts one module +and its descendants, whereas a crate entry exempts a whole compilation unit. +The application crate's module-scoped exemptions include +`netsuke::stdlib::which::lookup` (executable discovery through `PATH` and +cross-directory symlink canonicalization, which `cap_std` cannot express) and +`netsuke::runner::process::file_io::ambient_sync` (temporary-file +synchronization, scoped to the submodule holding only that `sync_all` so the +rest of `file_io` keeps writing through `cap_std` handles). Configuration +discovery otherwise uses capability-scoped canonicalization. Its small, +dedicated path-normalization module, `netsuke::cli::discovery::paths`, remains +narrowly excluded because `std::fs::canonicalize` preserves the absolute +comparison keys and cross-directory symlink behaviour that `cap_std` rejects. +For man-page generation, the build script compiles the `cli::build_support` +parser subset and deliberately omits runtime discovery. The broader +`netsuke::cli::discovery` module remains under the capability policy; no +`build_script_build` exception is required. The behavioural step definitions, +CLI integration tests, and shared workflow-reading helper that stage fixtures +ambiently are scoped the same way. A crate-level entry is justified only when +the ambient access lives in the crate root itself, where a path entry would be +no narrower — that covers the enumerated integration-test crates. The +`test_support` crate uses capability-backed fixture helpers and remains linted +by Whitaker under its own narrow policy. + +`test_support` is a workspace member, but the root Whitaker invocation selects +only the `netsuke-build` package (the Cargo package name behind the `netsuke` +targets; see ADR-007) and disables Dylint dependency checks. It therefore +compiles `test_support` as a dependency without applying the root +`dylint.toml`. Its one sanctioned ambient boundary is configured per crate. +Workspace membership makes Dylint discover the root configuration even when +launched from `test_support/`, so the scoped recipe supplies the contents of +`test_support/dylint.toml` explicitly through `DYLINT_TOML`. The second pass +also uses `--package test_support` and `--no-deps`, because running from a +member directory alone would otherwise check the parent workspace. That +configuration names only `test_support::fs` in `excluded_paths`. The root +`excluded_crates` must not contain `test_support`: every other module in the +crate remains subject to the filesystem policy. + +Permanent exceptions belong in `dylint.toml`, scoped as narrowly as the lint +allows. Do not use Rust `#[allow]` or `#[expect]` for `no_std_fs_operations`: +this Dylint lint is not known to `rustc`, so its exclusions must be configured +there. Prefer migrating to `cap_std` over any of these; reach for an exclusion +only when the operation is irreducibly ambient. + +To confirm the exclusions have not silently widened, add a temporary +`std::fs::metadata` call to an unexcluded module — for example +`src/stdlib/which/cache.rs`, a sibling of the excluded `lookup` module, or the +body of `src/runner/process/file_io.rs` outside `ambient_sync` — then run +`make lint-whitaker`. Both sites must still be reported; revert the probe +afterwards. The same check applies to `test_support`: a `std::fs` call in, say, +`test_support/src/exec.rs` must be reported even though `test_support::fs` is +exempt. + +When command output is long, preserve exit codes and logs: ```bash -WHITAKER_INSTALLER_VERSION="$(sed -n \ - "s/.*WHITAKER_INSTALLER_VERSION: '\(.*\)'.*/\1/p" \ - .github/workflows/ci.yml)" -cargo install --locked whitaker-installer \ - --version "$WHITAKER_INSTALLER_VERSION" +set -o pipefail +make test 2>&1 | tee /tmp/netsuke-make-test.log +``` + +These gates always use the repository toolchain and the default codegen +backend. For a faster inner loop between gate runs, see +[local build acceleration](#local-build-acceleration). + +For documentation changes, also run `make fmt`, `make markdownlint`, and +`make nixie`. + # or, for a prebuilt binary: cargo binstall --no-confirm --locked \ "whitaker-installer@$WHITAKER_INSTALLER_VERSION" @@ -2441,35 +2524,64 @@ Configuration merge helpers: ### Environment lookup seams -`cli::discovery::EnvProvider` is the port for raw environment access during -early CLI configuration resolution; `src/cli/mod.rs` re-exports it as -`ConfigEnvProvider` (and `StdEnvProvider` as `ConfigStdEnvProvider`), so -external callers see only the `Config*` names below. The production -`StdEnvProvider` adapter delegates to the process environment; tests can inject -map-backed providers without mutating process-global state. +CLI configuration uses the `mockable::Env` seam for environment access. +Production wrappers bind `mockable::DefaultEnv`; tests inject +`mockable::MockEnv` and do not mutate process-global environment variables. +The public entry points have these signatures: ```rust -pub trait ConfigEnvProvider { - fn get(&self, key: &str) -> Option; - fn entries(&self) -> Vec<(std::ffi::OsString, std::ffi::OsString)>; -} +pub fn resolve_merged_json( + cli: &Cli, + matches: &ArgMatches, +) -> OrthoResult; +pub fn resolve_merged_json_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl mockable::Env, +) -> OrthoResult; +pub fn resolve_json_and_layers_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl mockable::Env, +) -> OrthoResult<(bool, DiscoveredLayers)>; +pub fn resolve_json_and_layers_outcome_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl mockable::Env, +) -> (OrthoResult, DiscoveredLayers); +pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult; +pub fn merge_with_config_and_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl mockable::Env, +) -> OrthoResult; +pub fn merge_with_layers( + cli: &Cli, + matches: &ArgMatches, + env: &impl mockable::Env, + layers: DiscoveredLayers, +) -> OrthoResult; +pub fn merge_with_process_environment_layers( + cli: &Cli, + matches: &ArgMatches, + layers: DiscoveredLayers, +) -> OrthoResult; ``` -`get` owns selector lookup, while `entries` supplies the complete snapshot for -the layered `NETSUKE_*` merge. A selector-only provider may retain the empty -default for `entries`. Full-merge adapters must return a stable owned snapshot -so discovery and value merging observe one environment. Keep this port scoped -to CLI configuration; runner, manifest, locale, and stdlib environment seams -remain separate because their input and lifetime contracts differ. - -`DiscoverySources` is a crate-private composition input owned by -`src/cli/discovery.rs`. Only full merge and early JSON resolution may construct -it. Ambient entry points pair `ConfigStdEnvProvider` with OrthoConfig -`ProcessEnv`; injected entry points project the same `ConfigEnvProvider` into a -closed `MapEnv` containing only `NETSUKE_CONFIG`, `HOME`, `USERPROFILE`, -`XDG_CONFIG_HOME`, `XDG_CONFIG_DIRS`, `APPDATA`, and `LOCALAPPDATA`. Do not -reuse this fixed-key projection as a general environment-copy helper; -`EnvironmentLayer` alone enumerates the full `NETSUKE_*` value environment. +`resolve_json_and_layers_with_env` returns the resolved JSON preference and the +`DiscoveredLayers` from that same discovery pass. The caller must pass those +layers to `merge_with_layers` for the subsequent full merge; this handoff +prevents configuration files from being discovered and loaded again. +`resolve_json_and_layers_outcome_with_env` retains the layers when diagnostic +resolution fails, allowing startup to replay the cached selector trace after +enabling its tracing filter. + +`merge_with_process_environment_layers` accepts pre-discovered layers and +reads raw process environment entries only at the composition boundary. This +preserves non-Unicode entries for the environment-layer policy while keeping +discovery and diagnostic lookups behind the injected seam. Standalone callers +can use `merge_with_config_and_env` to discover and merge with an injected +environment; callers with cached layers should pass them to `merge_with_layers`. `explicit_config_path_with_env` is the crate-internal seam for explicit config-file selection. It evaluates the precedence chain in this order: @@ -2481,34 +2593,9 @@ config-file selection. It evaluates the precedence chain in this order: environment value into `PathBuf`. Both full merging and early JSON resolution use the same injected selector and file-layer implementation. -The ambient public APIs `merge_with_config` and `resolve_merged_json` each -accept two arguments. Their injected counterparts accept a `ConfigEnvProvider`: - -```rust -pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult; -pub fn merge_with_config_and_env( - cli: &Cli, - matches: &ArgMatches, - env: &impl ConfigEnvProvider, -) -> OrthoResult; -pub fn resolve_merged_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult; -pub fn resolve_merged_json_with_env( - cli: &Cli, - matches: &ArgMatches, - env: &impl ConfigEnvProvider, -) -> OrthoResult; -``` - -The `cli` module re-exports this trait publicly as `ConfigEnvProvider` (and -`StdEnvProvider` as `ConfigStdEnvProvider`) to keep the CLI seam distinct from -the unrelated `LocaleEnvProvider` in `locale_resolution`; crate-internal code -uses the bare `EnvProvider` name. - -Tests for injected configuration discovery should provide a map-backed -`ConfigEnvProvider`. End-to-end tests of the ambient `ProcessEnv` adapter must -run in an isolated child configured with `env_clear()` followed by -`Command::env`. `EnvLock` is reserved for tests that change the process working -directory alongside `CwdGuard`; it does not justify environment mutation. +Tests for Netsuke's environment seam should inject `mockable::MockEnv` +directly. End-to-end tests may configure a child process with `env_clear()` and +`Command::env`, but in-process tests must not mutate the process environment. Unit tests that only need to verify explicit config path precedence should test `explicit_config_path_with_env` with an injected provider instead of mutating @@ -2516,9 +2603,11 @@ the process environment. Config selector resolution remains a pure query: `resolve_config_selector` records the winning selector, its optional path, and every environment lookup -evaluated, and emits no tracing itself. Structured diagnostics are emitted only -at the file-layer boundary, where `collect_file_layers_with_env` calls -`trace_config_path_resolution` after resolution completes. +evaluated. `DiscoveredLayers` also retains bounded branch metadata from that +pass. `replay_config_path_trace` replays the environment lookups, resolved +selector, file-layer branch, and applicable project-scope outcome without +performing another environment lookup, filesystem scan, path normalization, or +configuration-file load. Tracing never logs full paths or formatted parser errors. Path values are bounded to a `path_hash` correlation identifier plus `path_file_name`, and load From c8a6ec03d1e509b031b88a3a3e678c38bb3ace84 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 15:42:29 +0200 Subject: [PATCH 05/21] Replay cached configuration discovery traces (#319) Retain bounded selector, branch, and project-scope metadata while loading configuration layers. Replay the original diagnostics after startup enables verbose output without rereading the environment or filesystem. Cover explicit and automatic replay paths, project-scope outcomes, and verbose stderr output while preserving the existing bounded trace schema. --- src/cli/discovery.rs | 43 ++++++---- src/cli/discovery_diagnostics.rs | 57 +++++++++++-- src/cli/discovery_layer_tests.rs | 77 +++++++++++++++++ src/cli/discovery_layers.rs | 113 ++++++++++++++----------- src/cli/discovery_trace.rs | 101 ++++++++++++++++++++++ tests/logging_stderr/config_tracing.rs | 4 + 6 files changed, 323 insertions(+), 72 deletions(-) create mode 100644 src/cli/discovery_trace.rs diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index e0fe50619..789bfe5fb 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -22,11 +22,15 @@ mod paths; #[path = "discovery_layers.rs"] mod layers; + +#[path = "discovery_trace.rs"] +mod trace; use diagnostics::{ - ConfigLoadFailureKind, debug_config_path, path_hash, trace_config_path_variable, - warn_explicit_config_load_failed, + BoundedConfigPath, ConfigLoadFailureKind, debug_config_path, path_hash, + trace_config_path_variable, warn_explicit_config_load_failed, }; -use layers::collect_file_layers; +use layers::collect_file_layers_with_trace; +use trace::{DiscoveryTrace, FileLayerTrace}; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; @@ -38,7 +42,7 @@ const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; pub struct DiscoveredLayers { layers: Vec>, errors: Vec>, - resolution: ConfigPathResolution, + trace: DiscoveryTrace, } impl DiscoveredLayers { @@ -59,29 +63,30 @@ impl DiscoveredLayers { (self.layers, self.errors) } - /// Re-emit the bounded selector diagnostic without querying the environment. + /// Re-emit bounded discovery diagnostics without repeating discovery. /// /// Startup enables verbose tracing only after resolving its diagnostic mode. /// Replaying the cached decision at that point preserves the existing trace - /// event without repeating configuration discovery or file loading. + /// events without repeating environment reads, filesystem discovery, path + /// normalization, or configuration-file loading. pub fn replay_config_path_trace(&self) { - trace_config_path_resolution(&self.resolution); + self.trace.replay(); } } /// Discover configuration layers once through the injected environment. pub(crate) fn discover_file_layers(cli: &Cli, env: &impl Env) -> DiscoveredLayers { - let (resolution, outcome) = collect_file_layers_with_env(cli, env); + let (trace, outcome) = collect_file_layers_with_env(cli, env); match outcome { Ok(layers) => DiscoveredLayers { layers, errors: Vec::new(), - resolution, + trace, }, Err(error) => DiscoveredLayers { layers: Vec::new(), errors: vec![error], - resolution, + trace, }, } } @@ -109,20 +114,26 @@ pub(crate) fn push_discovered_file_layers( fn collect_file_layers_with_env( cli: &Cli, env: &impl Env, -) -> (ConfigPathResolution, OrthoResult>>) { +) -> (DiscoveryTrace, OrthoResult>>) { let resolution = resolve_config_selector(cli.config.clone(), env); trace_config_path_resolution(&resolution); - let outcome = resolution.path.as_deref().map_or_else( + let (file_layers, outcome) = resolution.path.as_deref().map_or_else( || { debug!("using config discovery"); - collect_file_layers(cli.directory.as_deref()) + let (project_scope, outcome) = collect_file_layers_with_trace(cli.directory.as_deref()); + (FileLayerTrace::Automatic { project_scope }, outcome) }, |path| { debug_config_path("using explicit config path", path); - load_layers_from_path(path) + ( + FileLayerTrace::Explicit { + path: BoundedConfigPath::from_path(Some(path)), + }, + load_layers_from_path(path), + ) }, ); - (resolution, outcome) + (DiscoveryTrace::new(&resolution, file_layers), outcome) } /// Select an explicit config path, giving `--config` precedence over `env`. @@ -143,7 +154,7 @@ pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl Env) -> Option /// Records the winning selector, its optional path, and every environment /// lookup evaluated to reach the decision, so a caller can emit diagnostics /// afterwards without giving the query tracing side effects. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq)] struct ConfigPathResolution { selector: &'static str, path: Option, diff --git a/src/cli/discovery_diagnostics.rs b/src/cli/discovery_diagnostics.rs index 03466d2bc..1e68954bb 100644 --- a/src/cli/discovery_diagnostics.rs +++ b/src/cli/discovery_diagnostics.rs @@ -6,6 +6,7 @@ //! text. use std::collections::hash_map::DefaultHasher; +use std::ffi::OsString; use std::hash::{Hash, Hasher}; use std::path::Path; use tracing::{debug, trace, warn}; @@ -23,13 +24,40 @@ pub(super) enum ConfigLoadFailureKind { LoadError, } +/// Bounded path fields retained for deferred discovery diagnostics. +/// +/// This stores only the correlation hash, file name, and presence bit needed +/// to replay a diagnostic event. It deliberately excludes the full path. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct BoundedConfigPath { + pub(super) hash: Option, + pub(super) file_name: Option, + pub(super) is_present: bool, +} + +impl BoundedConfigPath { + /// Capture bounded fields from an optional path without retaining it. + pub(super) fn from_path(path: Option<&Path>) -> Self { + Self { + hash: path.map(path_hash), + file_name: path.and_then(Path::file_name).map(OsString::from), + is_present: path.is_some(), + } + } +} + /// Trace one environment lookup using bounded path fields. pub(super) fn trace_config_path_variable(var_name: &str, path: Option<&Path>) { + trace_config_path_variable_from_fields(var_name, &BoundedConfigPath::from_path(path)); +} + +/// Replay one environment lookup from retained bounded fields. +pub(super) fn trace_config_path_variable_from_fields(var_name: &str, path: &BoundedConfigPath) { trace!( var_name, - found = path.is_some(), - path_hash = path.map(path_hash).as_deref(), - path_file_name = ?path.and_then(Path::file_name), + found = path.is_present, + path_hash = path.hash.as_deref(), + path_file_name = ?path.file_name, "read config path variable" ); } @@ -56,12 +84,25 @@ pub(super) fn debug_config_path(message: &'static str, path: &Path) { ); } -/// Emit `message` with presence and bounded fields for an optional path string. -pub(super) fn debug_optional_config_path(message: &'static str, path: Option<&str>) { +/// Replay an explicit path diagnostic from retained bounded fields. +pub(super) fn debug_config_path_from_fields(message: &'static str, path: &BoundedConfigPath) { + let path_hash = path.hash.as_deref().unwrap_or_default(); + debug!( + path_hash = %path_hash, + path_file_name = ?path.file_name, + message + ); +} + +/// Replay an optional project-scope path diagnostic from bounded fields. +pub(super) fn debug_optional_config_path_from_fields( + message: &'static str, + path: &BoundedConfigPath, +) { debug!( - path_hash = path.map(|value| short_hash(value.as_bytes())).as_deref(), - path_file_name = ?path.and_then(|value| Path::new(value).file_name()), - path_present = path.is_some(), + path_hash = path.hash.as_deref(), + path_file_name = ?path.file_name, + path_present = path.is_present, message ); } diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 4d86b81b7..c4f63a760 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -12,6 +12,8 @@ use super::*; use tempfile::{TempDir, tempdir}; use crate::cli::test_support::empty_mock_env; +use mockable::MockEnv; +use std::ffi::OsString; use super::event_assertions::{EventAssertion, capture_events, find_event}; use super::layers::{collect_file_layers, collect_file_layers_with_normalizer}; use super::paths::FailingPathNormalizer; @@ -85,6 +87,81 @@ fn collect_diag_file_layers_logs_selected_branch( Ok(()) } +/// Cached replay emits the explicit selection branch without another env read. +#[test] +fn replay_logs_the_explicit_config_branch_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; + let mut env = MockEnv::new(); + env.expect_os_string().never(); + + let discovered = discover_file_layers(&cli, &env); + let ((), events) = capture_events(|| { + discovered.replay_config_path_trace(); + Ok::<_, anyhow::Error>(()) + })?; + find_event(&events, "resolved config path")?; + let event = find_event(&events, "using explicit config path")?; + EventAssertion::new(event, cli.config.as_deref().context("explicit config")?) + .ensure_bounded_path_fields()?; + + Ok(()) +} + +/// Cached selector-free replay retains automatic and appended project outcomes. +#[test] +fn replay_logs_discovery_and_appended_project_scope_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli = Cli { + directory: Some(temp.path().to_path_buf()), + ..Cli::default() + }; + let mut env = MockEnv::new(); + env.expect_os_string() + .withf(|key| key == CONFIG_ENV_VAR) + .once() + .return_const(None::); + + let discovered = discover_file_layers(&cli, &env); + let ((), events) = capture_events(|| { + discovered.replay_config_path_trace(); + Ok::<_, anyhow::Error>(()) + })?; + find_event(&events, "read config path variable")?; + find_event(&events, "resolved config path")?; + find_event(&events, "using config discovery")?; + find_event(&events, "appending project-scope layers")?; + + Ok(()) +} + +/// Cached selector-free replay retains an included project-scope outcome. +#[test] +fn replay_logs_included_project_scope_without_environment_access() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + test_support::fs::write(temp.path().join(".netsuke.toml"), "jobs = 7\n") + .context("write project config")?; + let cli = Cli { + directory: Some(temp.path().to_path_buf()), + ..Cli::default() + }; + let mut env = MockEnv::new(); + env.expect_os_string() + .withf(|key| key == CONFIG_ENV_VAR) + .once() + .return_const(None::); + + let discovered = discover_file_layers(&cli, &env); + let ((), events) = capture_events(|| { + discovered.replay_config_path_trace(); + Ok::<_, anyhow::Error>(()) + })?; + find_event(&events, "using config discovery")?; + find_event(&events, "discovery included project-scope layers")?; + + Ok(()) +} + /// Automatic discovery must use the injected XDG directory, not the host. #[test] fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> { diff --git a/src/cli/discovery_layers.rs b/src/cli/discovery_layers.rs index 6b4cb8f56..0899d66c7 100644 --- a/src/cli/discovery_layers.rs +++ b/src/cli/discovery_layers.rs @@ -4,26 +4,45 @@ //! project `.netsuke.toml` outranks user-scope files. Path comparison and its //! fallback policy live here because that policy is a discovery decision. -use ortho_config::{ - ConfigDiscovery, MergeLayer, OrthoResult, SharedEnvSource, load_config_file_as_chain, -}; +use ortho_config::{ConfigDiscovery, MergeLayer, OrthoResult, load_config_file_as_chain}; use std::borrow::Cow; use std::path::{Path, PathBuf}; -#[cfg(test)] -use std::sync::Arc; -use super::CONFIG_ENV_VAR; -use super::diagnostics::debug_optional_config_path; +use super::diagnostics::{BoundedConfigPath, debug_optional_config_path_from_fields}; use super::paths::{FsPathNormalizer, PathNormalizer, normalized_path_key}; +/// Project-scope outcome retained for a later trace replay. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) enum ProjectScopeTrace { + /// The primary discovery scan already yielded the project configuration. + Included(BoundedConfigPath), + /// The project configuration was loaded by the second pass. + Appended(BoundedConfigPath), +} + +impl ProjectScopeTrace { + /// Replay the original project-scope diagnostic without filesystem access. + pub(super) fn replay(&self) { + match self { + Self::Included(path) => { + debug_optional_config_path_from_fields( + "discovery included project-scope layers", + path, + ); + } + Self::Appended(path) => { + debug_optional_config_path_from_fields("appending project-scope layers", path); + } + } + } +} + /// Build the single-pass `OrthoConfig` discovery scanner. /// /// Anchors the project root to `directory` when supplied; otherwise the default -/// project roots apply. `NETSUKE_CONFIG` is registered as the discovery env var. -fn config_discovery(directory: Option<&PathBuf>, env_source: SharedEnvSource) -> ConfigDiscovery { - let mut builder = ConfigDiscovery::builder("netsuke") - .env_var(CONFIG_ENV_VAR) - .env_source(env_source); +/// project roots apply. The caller resolves `NETSUKE_CONFIG` before discovery. +fn config_discovery(directory: Option<&PathBuf>) -> ConfigDiscovery { + let mut builder = ConfigDiscovery::builder("netsuke"); if let Some(dir) = directory { builder = builder.clear_project_roots().add_project_root(dir); } @@ -34,15 +53,7 @@ fn config_discovery(directory: Option<&PathBuf>, env_source: SharedEnvSource) -> pub(crate) fn collect_file_layers( directory: Option<&Path>, ) -> OrthoResult>> { - collect_file_layers_with_env_source(directory, Arc::new(ortho_config::ProcessEnv)) -} - -/// Collect layers with the environment source chosen at the composition root. -pub(super) fn collect_file_layers_with_env_source( - directory: Option<&Path>, - env_source: SharedEnvSource, -) -> OrthoResult>> { - collect_file_layers_with_normalizer_and_env_source(directory, &FsPathNormalizer, env_source) + collect_file_layers_with_trace(directory).1 } /// Return the key used to compare `path` against the expected project file. @@ -63,27 +74,35 @@ pub(super) fn collect_file_layers_with_normalizer( directory: Option<&Path>, normalizer: &impl PathNormalizer, ) -> OrthoResult>> { - collect_file_layers_with_normalizer_and_env_source( - directory, - normalizer, - Arc::new(ortho_config::ProcessEnv), - ) + collect_file_layers_with_normalizer_and_trace(directory, normalizer).1 +} + +/// Run discovery once and retain the project-scope outcome for later replay. +pub(super) fn collect_file_layers_with_trace( + directory: Option<&Path>, +) -> ( + Option, + OrthoResult>>, +) { + collect_file_layers_with_normalizer_and_trace(directory, &FsPathNormalizer) } -/// Build the discovery layer chain with an injected environment source. -fn collect_file_layers_with_normalizer_and_env_source( +/// Build the discovery chain and its bounded project-scope trace metadata. +fn collect_file_layers_with_normalizer_and_trace( directory: Option<&Path>, normalizer: &impl PathNormalizer, - env_source: SharedEnvSource, -) -> OrthoResult>> { - let discovery = config_discovery(directory.map(PathBuf::from).as_ref(), env_source); +) -> ( + Option, + OrthoResult>>, +) { + let discovery = config_discovery(directory.map(PathBuf::from).as_ref()); let mut file_layers = discovery.compose_layers(); let mut errors = file_layers.required_errors; if file_layers.value.is_empty() { errors.append(&mut file_layers.optional_errors); } if let Some(err) = errors.into_iter().next() { - return Err(err); + return (None, Err(err)); } let project_file = project_scope_file(directory); @@ -97,25 +116,23 @@ fn collect_file_layers_with_normalizer_and_env_source( .is_some_and(|key| key.to_string_lossy() == path.as_str()) }) }); - let project_file_display = project_file.as_deref().map(Path::to_string_lossy); + let project_trace_path = BoundedConfigPath::from_path(project_file.as_deref()); if has_project_layer { - debug_optional_config_path( - "discovery included project-scope layers", - project_file_display.as_deref(), - ); - return Ok(file_layers.value); + let trace = ProjectScopeTrace::Included(project_trace_path); + trace.replay(); + return (Some(trace), Ok(file_layers.value)); } - debug_optional_config_path( - "appending project-scope layers", - project_file_display.as_deref(), - ); - let project_layers = project_scope_layers(project_file.as_deref())?; - Ok(file_layers - .value - .into_iter() - .chain(project_layers) - .collect()) + let trace = ProjectScopeTrace::Appended(project_trace_path); + trace.replay(); + let result = project_scope_layers(project_file.as_deref()).map(|project_layers| { + file_layers + .value + .into_iter() + .chain(project_layers) + .collect() + }); + (Some(trace), result) } fn project_scope_file(directory: Option<&Path>) -> Option { diff --git a/src/cli/discovery_trace.rs b/src/cli/discovery_trace.rs new file mode 100644 index 000000000..653d40e45 --- /dev/null +++ b/src/cli/discovery_trace.rs @@ -0,0 +1,101 @@ +//! Bounded metadata for replaying configuration discovery diagnostics. +//! +//! Discovery captures this data while it still owns the selected paths and +//! project-scope result. Startup later replays the original trace events after +//! it enables verbose output, without accessing the environment or filesystem. + +use tracing::debug; + +use super::ConfigPathResolution; +use super::diagnostics::{ + BoundedConfigPath, debug_config_path_from_fields, trace_config_path_variable_from_fields, +}; +use super::layers::ProjectScopeTrace; + +/// Bounded selector and layer-branch diagnostics retained after discovery. +#[derive(Clone, Debug)] +pub(super) struct DiscoveryTrace { + resolution: ConfigPathTrace, + file_layers: FileLayerTrace, +} + +impl DiscoveryTrace { + /// Combine a resolved selector and layer branch without retaining raw paths. + pub(super) fn new(resolution: &ConfigPathResolution, file_layers: FileLayerTrace) -> Self { + Self { + resolution: ConfigPathTrace::from_resolution(resolution), + file_layers, + } + } + + /// Replay all discovery diagnostics from bounded metadata only. + pub(super) fn replay(&self) { + self.resolution.replay(); + self.file_layers.replay(); + } +} + +/// Bounded diagnostics for selector resolution and its environment reads. +#[derive(Clone, Debug)] +struct ConfigPathTrace { + selector: &'static str, + path: BoundedConfigPath, + environment_lookups: Vec<(&'static str, BoundedConfigPath)>, +} + +impl ConfigPathTrace { + /// Build a trace-only view without retaining raw paths. + fn from_resolution(resolution: &ConfigPathResolution) -> Self { + Self { + selector: resolution.selector, + path: BoundedConfigPath::from_path(resolution.path.as_deref()), + environment_lookups: resolution + .environment_lookups + .iter() + .map(|(var_name, path)| (*var_name, BoundedConfigPath::from_path(path.as_deref()))) + .collect(), + } + } + + /// Replay selector diagnostics from their bounded representation. + fn replay(&self) { + for (var_name, path) in &self.environment_lookups { + trace_config_path_variable_from_fields(var_name, path); + } + debug!( + selector = self.selector, + path_hash = self.path.hash.as_deref(), + path_file_name = ?self.path.file_name, + path_present = self.path.is_present, + "resolved config path" + ); + } +} + +/// File-layer branch diagnostics retained after the first discovery pass. +#[derive(Clone, Debug)] +pub(super) enum FileLayerTrace { + /// An explicit CLI or environment selector chose a configuration path. + Explicit { path: BoundedConfigPath }, + /// Selector-free discovery, with any project-scope second-pass outcome. + Automatic { + project_scope: Option, + }, +} + +impl FileLayerTrace { + /// Replay the selected layer-collection branch without filesystem access. + fn replay(&self) { + match self { + Self::Explicit { path } => { + debug_config_path_from_fields("using explicit config path", path); + } + Self::Automatic { project_scope } => { + debug!("using config discovery"); + if let Some(trace) = project_scope { + trace.replay(); + } + } + } + } +} diff --git a/tests/logging_stderr/config_tracing.rs b/tests/logging_stderr/config_tracing.rs index 91e1e1bcf..c8beb9dfb 100644 --- a/tests/logging_stderr/config_tracing.rs +++ b/tests/logging_stderr/config_tracing.rs @@ -71,6 +71,10 @@ fn explicit_selection_traces_bounded_fields() -> Result<()> { joined.contains("selected-secret-name.toml"), "the bounded file name should be present: {joined}" ); + ensure!( + joined.contains("using explicit config path"), + "verbose stderr should replay the cached explicit branch: {joined}" + ); ensure!( !joined.contains(raw_path.as_str()), "diagnostics must not log the raw config path: {joined}" From 61eaa76665ce3d00cbece205202f47e1e93ad22d Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 15:46:27 +0200 Subject: [PATCH 06/21] Refresh configuration architecture documentation Replace retired environment-provider references with the current `mockable::Env` boundary and describe the cached `DiscoveredLayers` handoff used by startup. --- docs/netsuke-design.md | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index d3be16862..34594b7f2 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2887,24 +2887,21 @@ manual flag repetition. project, and user file layers, merges them with defaults, adds environment variables via Figment, and finally applies CLI overrides extracted from `ArgMatches`. -- The `config_discovery()` function uses OrthoConfig's builder API with the - application name, environment selector, and an environment source selected at - the CLI composition root. Ambient runs use `ProcessEnv`; injected runs use a - closed `MapEnv` projected from Netsuke's environment port, preventing tests - from falling through to host directories. -- A missing optional candidate means no configuration layer and therefore - built-in defaults. A candidate that exists but cannot load is retained as an - error when no candidate succeeds, so malformed configuration and a missing - `extends` parent are never mistaken for absence. +- The `config_discovery()` function uses OrthoConfig's builder API without + further customization beyond the application name and environment variable + override, relying on OrthoConfig's platform-specific defaults for standard + directory resolution. - Netsuke-owned environment reads for explicit config selection and early JSON - resolution go through the `EnvProvider` port in `src/cli/discovery.rs`. - Production code uses `StdEnvProvider`; tests can inject a map-backed provider - instead of mutating the process environment. The v0.9.0 adapter projects only - the documented discovery keys into OrthoConfig, while `EnvironmentLayer` - retains the complete `NETSUKE_*` value merge boundary. -- Configuration files use TOML. OrthoConfig's optional YAML provider remains - disabled; Netsukefile YAML continues to be parsed by the separate - `serde-saphyr` manifest boundary. + resolution take an injected `&impl mockable::Env` in the public + `*_with_env` entry points. Production wrappers supply + `mockable::DefaultEnv`; tests supply `mockable::MockEnv` without mutating the + process environment. Startup passes the `DiscoveredLayers` returned by + `resolve_json_and_layers_with_env` to `merge_with_layers`, so file discovery + and loading happen once. OrthoConfig discovery remains an external boundary + and may still read platform environment variables directly. +- Configuration files use TOML format by default. JSON5 (`.json`, `.json5`) and + YAML (`.yaml`, `.yml`) formats are supported when the corresponding Cargo + features are enabled. - Explicit config selection is handled outside OrthoConfig's built-in discovery override surface so Netsuke keeps its custom two-pass project-over-user merge behaviour for automatic discovery. If an explicit selector is set, the From 88169e6cc268268bc6dbf9c1d7bfb4cbb4f5602e Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 15:53:43 +0200 Subject: [PATCH 07/21] Document deferred discovery diagnostics Update the CLI architecture references for `DiscoveryOutcome`, its `emit_diagnostics()` and `into_layers()` composition boundary, and the production raw-environment merge handoff. --- docs/developers-guide.md | 18 ++++++++++-------- docs/netsuke-design.md | 9 +++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 2cea633af..829de61eb 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2548,7 +2548,7 @@ pub fn resolve_json_and_layers_outcome_with_env( cli: &Cli, matches: &ArgMatches, env: &impl mockable::Env, -) -> (OrthoResult, DiscoveredLayers); +) -> (OrthoResult, DiscoveryOutcome); pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult; pub fn merge_with_config_and_env( cli: &Cli, @@ -2572,9 +2572,11 @@ pub fn merge_with_process_environment_layers( `DiscoveredLayers` from that same discovery pass. The caller must pass those layers to `merge_with_layers` for the subsequent full merge; this handoff prevents configuration files from being discovered and loaded again. -`resolve_json_and_layers_outcome_with_env` retains the layers when diagnostic -resolution fails, allowing startup to replay the cached selector trace after -enabling its tracing filter. +`resolve_json_and_layers_outcome_with_env` returns a `DiscoveryOutcome` that +retains the layers and bounded diagnostics when diagnostic resolution fails. +The startup boundary calls `emit_diagnostics()` after enabling its tracing +filter, then calls `into_layers()` and passes the layers to +`merge_with_process_environment_layers`. `merge_with_process_environment_layers` accepts pre-discovered layers and reads raw process environment entries only at the composition boundary. This @@ -2603,10 +2605,10 @@ the process environment. Config selector resolution remains a pure query: `resolve_config_selector` records the winning selector, its optional path, and every environment lookup -evaluated. `DiscoveredLayers` also retains bounded branch metadata from that -pass. `replay_config_path_trace` replays the environment lookups, resolved -selector, file-layer branch, and applicable project-scope outcome without -performing another environment lookup, filesystem scan, path normalization, or +evaluated. `DiscoveryOutcome` also retains bounded branch metadata from that +pass. `emit_diagnostics()` replays the environment lookups, resolved selector, +file-layer branch, and applicable project-scope outcome without performing +another environment lookup, filesystem scan, path normalization, or configuration-file load. Tracing never logs full paths or formatted parser errors. Path values are diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 34594b7f2..66997c567 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2895,10 +2895,11 @@ manual flag repetition. resolution take an injected `&impl mockable::Env` in the public `*_with_env` entry points. Production wrappers supply `mockable::DefaultEnv`; tests supply `mockable::MockEnv` without mutating the - process environment. Startup passes the `DiscoveredLayers` returned by - `resolve_json_and_layers_with_env` to `merge_with_layers`, so file discovery - and loading happen once. OrthoConfig discovery remains an external boundary - and may still read platform environment variables directly. + process environment. Startup obtains a `DiscoveryOutcome` from + `resolve_json_and_layers_outcome_with_env`, emits its deferred diagnostics, + then passes `into_layers()` to `merge_with_process_environment_layers`, so + file discovery and loading happen once. OrthoConfig discovery remains an + external boundary and may still read platform environment variables directly. - Configuration files use TOML format by default. JSON5 (`.json`, `.json5`) and YAML (`.yaml`, `.yml`) formats are supported when the corresponding Cargo features are enabled. From 7e15a4690f268863b0b018a936535119d5a96b2d Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 16:00:40 +0200 Subject: [PATCH 08/21] Refresh configuration helper documentation Describe the current cached discovery and deferred-diagnostics helpers, and remove references to the retired direct file-layer push flow. --- docs/developers-guide.md | 28 +++++++++++++++------------- docs/netsuke-design.md | 8 ++++---- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 829de61eb..b273c7f87 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2488,10 +2488,10 @@ Private helper functions for config discovery and JSON-output resolution. Configuration merge helpers: -- `config_discovery(directory, env_source) -> ConfigDiscovery` builds the - single-pass OrthoConfig discovery scanner with an optional project-root - anchor and the environment adapter selected at the composition root. -- `project_scope_file(directory: Option<&Path>) -> Option` +- `config_discovery(directory: Option<&PathBuf>) -> ConfigDiscovery` builds + the single-pass OrthoConfig discovery scanner with an optional project-root + anchor. +- `project_scope_file_str(directory: Option<&Path>) -> Option` resolves the expected project `.netsuke.toml` path for project-layer detection. - `project_scope_layers(directory)` loads the project-scope config directly, @@ -2502,15 +2502,17 @@ Configuration merge helpers: `PathBuf`. - `explicit_config_path_with_env(cli, env) -> Option` resolves explicit config selection from `--config` and `NETSUKE_CONFIG`. -- `push_file_layers_with_sources(cli, composer, errors, sources) -> ()` pushes - explicit or discovered file layers onto a `MergeComposer`. Explicit load - errors are pushed into `errors`, and automatic discovery is not attempted - after an explicit selector fails. -- `collect_diag_file_layers_with_sources(cli, sources)` reuses the same - file-layer precedence for early JSON resolution. -- `collect_file_layers_with_env_source(directory, env_source)` builds the - fallback discovery layer chain, applies the project-layer second pass, and - returns `OrthoResult>>`. +- `discover_file_layers(cli, env) -> DiscoveryOutcome` performs the single + explicit-or-automatic discovery and load pass, retaining its layers, errors, + and deferred bounded diagnostics for the composition boundary. +- `push_discovered_file_layers(composer, errors, layers) -> ()` consumes the + cached layers and discovery errors while composing the full configuration. +- `collect_diag_file_layers_with_env(cli, env) -> DiscoveryOutcome` preserves + the diagnostic collection span while routing discovery through the shared + cached outcome. +- `collect_file_layers(directory)` builds the fallback discovery layer chain, + applies the project-layer second pass, and returns + `OrthoResult>>`. - `is_empty_value(value: &serde_json::Value) -> bool` detects an empty CLI override object. - `json_from_layer(value: &serde_json::Value) -> Option` extracts `json` diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 66997c567..3aa93bae0 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2883,10 +2883,10 @@ manual flag repetition. selectors before automatic discovery so missing or invalid explicit files remain hard errors. - The `merge_with_config()` function in `src/cli/merge.rs` orchestrates the - full layer composition: it calls `push_file_layers(...)` to load explicit, - project, and user file layers, merges them with defaults, adds environment - variables via Figment, and finally applies CLI overrides extracted from - `ArgMatches`. + full layer composition: it performs one `discover_file_layers(...)` pass, + emits its deferred diagnostics, consumes the resulting layers, merges them + with defaults, adds environment variables via Figment, and finally applies + CLI overrides extracted from `ArgMatches`. - The `config_discovery()` function uses OrthoConfig's builder API without further customization beyond the application name and environment variable override, relying on OrthoConfig's platform-specific defaults for standard From b7cccdb828ce06a8215858373f5b15ddc4a323cd Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 16:29:10 +0200 Subject: [PATCH 09/21] Defer configuration discovery diagnostics (#319) Keep discovery side-effect free by returning cached layers and bounded diagnostics in `DiscoveryOutcome`. Emit the retained events only at the startup or standalone merge composition boundary, so verbose output remains complete without a second discovery pass. Compile an external Cargo fixture against the public cached configuration API and cover deferred tracing, load warnings, and selector-free branches. --- src/cli/diag.rs | 21 +-- src/cli/discovery.rs | 150 +++++++++++--------- src/cli/discovery_diagnostics.rs | 46 +++--- src/cli/discovery_layer_tests.rs | 53 +++++-- src/cli/discovery_layers.rs | 9 +- src/cli/discovery_trace.rs | 49 +++++-- src/cli/discovery_tracing_tests.rs | 38 +++-- src/cli/merge.rs | 8 +- src/cli/mod.rs | 2 +- src/main.rs | 8 +- tests/command_env_ui_tests.rs | 41 +++++- tests/ui/cli_configuration_pass/Cargo.toml | 11 ++ tests/ui/cli_configuration_pass/src/main.rs | 31 ++++ 13 files changed, 326 insertions(+), 141 deletions(-) create mode 100644 tests/ui/cli_configuration_pass/Cargo.toml create mode 100644 tests/ui/cli_configuration_pass/src/main.rs diff --git a/src/cli/diag.rs b/src/cli/diag.rs index dba5f2ace..fdbbc80da 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -12,7 +12,7 @@ use ortho_config::{OrthoError, OrthoResult}; use serde_json::Value; use std::sync::Arc; -use super::discovery::{DiscoveredLayers, collect_diag_file_layers_with_env}; +use super::discovery::{DiscoveredLayers, DiscoveryOutcome, collect_diag_file_layers_with_env}; use super::parser::Cli; const JSON_ENV_VAR: &str = "NETSUKE_JSON"; @@ -62,27 +62,28 @@ pub fn resolve_json_and_layers_with_env( matches: &ArgMatches, env: &impl Env, ) -> OrthoResult<(bool, DiscoveredLayers)> { - let (result, layers) = resolve_json_and_layers_outcome_with_env(cli, matches, env); - result.map(|json| (json, layers)) + let (result, outcome) = resolve_json_and_layers_outcome_with_env(cli, matches, env); + result.map(|json| (json, outcome.into_layers())) } -/// Resolve diagnostic JSON mode while retaining layers on both outcomes. +/// Resolve diagnostic JSON mode while retaining a discovery outcome. /// /// Startup uses this form to replay the cached selector diagnostic after it /// enables its output filter, including when discovery or JSON validation -/// fails. Standalone callers should usually prefer +/// fails. The outcome owns the discovered layers and deferred diagnostics. +/// Standalone callers should usually prefer /// [`resolve_json_and_layers_with_env`]. pub fn resolve_json_and_layers_outcome_with_env( cli: &Cli, matches: &ArgMatches, env: &impl Env, -) -> (OrthoResult, DiscoveredLayers) { - let layers = collect_diag_file_layers_with_env(cli, env); +) -> (OrthoResult, DiscoveryOutcome) { + let outcome = collect_diag_file_layers_with_env(cli, env); let result = (|| { - if let Some(error) = layers.first_error() { + if let Some(error) = outcome.first_error() { return Err(Arc::clone(error)); } - let mut json = json_from_layers(layers.layers()); + let mut json = json_from_layers(outcome.layers()); if !has_cli_json_override(matches) && let Some(env_json) = json_from_env(env)? { @@ -90,7 +91,7 @@ pub fn resolve_json_and_layers_outcome_with_env( } Ok(json_from_matches(cli, matches, json)) })(); - (result, layers) + (result, outcome) } fn json_from_layer(value: &Value) -> Option { diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index 789bfe5fb..45b768adc 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -10,7 +10,6 @@ use std::borrow::Cow; use std::io; use std::path::{Path, PathBuf}; use std::sync::Arc; -use tracing::{debug, debug_span}; use super::parser::Cli; @@ -25,12 +24,9 @@ mod layers; #[path = "discovery_trace.rs"] mod trace; -use diagnostics::{ - BoundedConfigPath, ConfigLoadFailureKind, debug_config_path, path_hash, - trace_config_path_variable, warn_explicit_config_load_failed, -}; +use diagnostics::{BoundedConfigPath, ConfigLoadFailureKind, ConfigLoadWarning}; use layers::collect_file_layers_with_trace; -use trace::{DiscoveryTrace, FileLayerTrace}; +use trace::{DiscoveryDiagnostics, DiscoveryTrace, FileLayerTrace}; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; @@ -42,7 +38,6 @@ const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; pub struct DiscoveredLayers { layers: Vec>, errors: Vec>, - trace: DiscoveryTrace, } impl DiscoveredLayers { @@ -62,32 +57,56 @@ impl DiscoveredLayers { ) -> (Vec>, Vec>) { (self.layers, self.errors) } +} + +/// Layers and diagnostics returned by a side-effect-free discovery pass. +/// +/// The diagnostic pre-pass reads the layers while retaining the bounded events +/// for a composition boundary to emit after it installs the tracing filter. +pub struct DiscoveryOutcome { + layers: DiscoveredLayers, + diagnostics: DiscoveryDiagnostics, +} + +impl DiscoveryOutcome { + /// Borrow file layers in discovery order. + pub(crate) fn layers(&self) -> &[MergeLayer<'static>] { + self.layers.layers() + } + + /// Borrow the first discovery error, if loading failed. + pub(crate) fn first_error(&self) -> Option<&Arc> { + self.layers.first_error() + } + + /// Consume the outcome into the reusable file layers. + #[must_use] + pub fn into_layers(self) -> DiscoveredLayers { + self.layers + } - /// Re-emit bounded discovery diagnostics without repeating discovery. - /// - /// Startup enables verbose tracing only after resolving its diagnostic mode. - /// Replaying the cached decision at that point preserves the existing trace - /// events without repeating environment reads, filesystem discovery, path - /// normalization, or configuration-file loading. - pub fn replay_config_path_trace(&self) { - self.trace.replay(); + /// Emit deferred diagnostics without repeating discovery. + pub fn emit_diagnostics(&self) { + self.diagnostics.emit(); } } /// Discover configuration layers once through the injected environment. -pub(crate) fn discover_file_layers(cli: &Cli, env: &impl Env) -> DiscoveredLayers { - let (trace, outcome) = collect_file_layers_with_env(cli, env); - match outcome { +pub(crate) fn discover_file_layers(cli: &Cli, env: &impl Env) -> DiscoveryOutcome { + let (trace, load_warning, outcome) = collect_file_layers_with_env(cli, env); + let layers = match outcome { Ok(layers) => DiscoveredLayers { layers, errors: Vec::new(), - trace, }, Err(error) => DiscoveredLayers { layers: Vec::new(), errors: vec![error], - trace, }, + }; + DiscoveryOutcome { + layers, + diagnostics: DiscoveryDiagnostics::new(trace, load_warning), } } @@ -114,33 +133,40 @@ pub(crate) fn push_discovered_file_layers( fn collect_file_layers_with_env( cli: &Cli, env: &impl Env, -) -> (DiscoveryTrace, OrthoResult>>) { +) -> ( + DiscoveryTrace, + Option, + OrthoResult>>, +) { let resolution = resolve_config_selector(cli.config.clone(), env); - trace_config_path_resolution(&resolution); - let (file_layers, outcome) = resolution.path.as_deref().map_or_else( + let (file_layers, load_warning, outcome) = resolution.path.as_deref().map_or_else( || { - debug!("using config discovery"); let (project_scope, outcome) = collect_file_layers_with_trace(cli.directory.as_deref()); - (FileLayerTrace::Automatic { project_scope }, outcome) + (FileLayerTrace::Automatic { project_scope }, None, outcome) }, |path| { - debug_config_path("using explicit config path", path); + let (load_warning, outcome) = load_layers_from_path_with_warning(path); ( FileLayerTrace::Explicit { path: BoundedConfigPath::from_path(Some(path)), }, - load_layers_from_path(path), + load_warning, + outcome, ) }, ); - (DiscoveryTrace::new(&resolution, file_layers), outcome) + ( + DiscoveryTrace::new(&resolution, file_layers), + load_warning, + outcome, + ) } /// Select an explicit config path, giving `--config` precedence over `env`. /// /// A thin wrapper over [`resolve_config_selector`] for callers that need only -/// the winning path. Like that query it performs no tracing; orchestration -/// boundaries call [`trace_config_path_resolution`] to emit diagnostics. +/// the winning path. Like that query it performs no tracing; discovery returns +/// bounded diagnostics for composition boundaries to emit later. /// /// Production code takes the richer [`ConfigPathResolution`] so it can trace the /// environment lookups, leaving this as a convenience for precedence tests. @@ -182,23 +208,6 @@ fn resolve_config_selector(cli_config: Option, env: &impl Env) -> Confi } } -/// Emit bounded diagnostics for a completed path `resolution`. -/// -/// Environment lookups are traced before the selector event. A selected path -/// contributes only a correlation hash and file name, never its full value. -fn trace_config_path_resolution(resolution: &ConfigPathResolution) { - for (var_name, path) in &resolution.environment_lookups { - trace_config_path_variable(var_name, path.as_deref()); - } - debug!( - selector = resolution.selector, - path_hash = resolution.path.as_deref().map(path_hash).as_deref(), - path_file_name = ?resolution.path.as_deref().and_then(Path::file_name), - path_present = resolution.path.is_some(), - "resolved config path" - ); -} - /// Read a non-empty config path from `var_name` through `env`. /// /// Returns `None` when the variable is unset or empty, so discovery still runs. @@ -209,19 +218,22 @@ fn env_config_path(env: &impl Env, var_name: &str) -> Option { .map(PathBuf::from) } -/// Load the configuration chain rooted at an explicit file path. -/// -/// Unlike discovery, a missing explicit file is an error because the caller -/// selected it deliberately. -pub(crate) fn load_layers_from_path( - path: &std::path::Path, -) -> OrthoResult>> { +/// Load explicit layers while retaining a warning for the composition boundary. +fn load_layers_from_path_with_warning( + path: &Path, +) -> ( + Option, + OrthoResult>>, +) { match load_config_file_as_chain(path) { - Ok(Some(chain)) => Ok(chain - .values - .into_iter() - .map(|(value, layer_path)| MergeLayer::file(Cow::Owned(value), Some(layer_path))) - .collect()), + Ok(Some(chain)) => ( + None, + Ok(chain + .values + .into_iter() + .map(|(value, layer_path)| MergeLayer::file(Cow::Owned(value), Some(layer_path))) + .collect()), + ), Ok(None) => { let error = Arc::new(ortho_config::OrthoError::File { path: path.to_path_buf(), @@ -230,21 +242,25 @@ pub(crate) fn load_layers_from_path( "explicit configuration file not found", )), }); - warn_explicit_config_load_failed(path, ConfigLoadFailureKind::Missing); - Err(error) - } - Err(error) => { - warn_explicit_config_load_failed(path, ConfigLoadFailureKind::LoadError); - Err(error) + ( + Some(ConfigLoadWarning::new(path, ConfigLoadFailureKind::Missing)), + Err(error), + ) } + Err(error) => ( + Some(ConfigLoadWarning::new( + path, + ConfigLoadFailureKind::LoadError, + )), + Err(error), + ), } } /// Load file layers for early JSON resolution using injected environment access. /// /// This delegates to the same precedence boundary as the normal merge path. -pub(crate) fn collect_diag_file_layers_with_env(cli: &Cli, env: &impl Env) -> DiscoveredLayers { - let _span = debug_span!("collect_diag_file_layers").entered(); +pub(crate) fn collect_diag_file_layers_with_env(cli: &Cli, env: &impl Env) -> DiscoveryOutcome { discover_file_layers(cli, env) } diff --git a/src/cli/discovery_diagnostics.rs b/src/cli/discovery_diagnostics.rs index 1e68954bb..67d09e6d4 100644 --- a/src/cli/discovery_diagnostics.rs +++ b/src/cli/discovery_diagnostics.rs @@ -24,6 +24,28 @@ pub(super) enum ConfigLoadFailureKind { LoadError, } +/// Bounded warning metadata retained when an explicit config load fails. +#[derive(Clone, Debug)] +pub(super) struct ConfigLoadWarning { + path: BoundedConfigPath, + failure_kind: ConfigLoadFailureKind, +} + +impl ConfigLoadWarning { + /// Capture a load failure without retaining its raw path or error text. + pub(super) fn new(path: &Path, failure_kind: ConfigLoadFailureKind) -> Self { + Self { + path: BoundedConfigPath::from_path(Some(path)), + failure_kind, + } + } + + /// Emit the fixed explicit-load warning from bounded metadata. + pub(super) fn emit(&self) { + warn_explicit_config_load_failed_from_fields(&self.path, self.failure_kind); + } +} + /// Bounded path fields retained for deferred discovery diagnostics. /// /// This stores only the correlation hash, file name, and presence bit needed @@ -46,11 +68,6 @@ impl BoundedConfigPath { } } -/// Trace one environment lookup using bounded path fields. -pub(super) fn trace_config_path_variable(var_name: &str, path: Option<&Path>) { - trace_config_path_variable_from_fields(var_name, &BoundedConfigPath::from_path(path)); -} - /// Replay one environment lookup from retained bounded fields. pub(super) fn trace_config_path_variable_from_fields(var_name: &str, path: &BoundedConfigPath) { trace!( @@ -66,24 +83,19 @@ pub(super) fn trace_config_path_variable_from_fields(var_name: &str, path: &Boun /// /// The event exposes the failure class, file name, and correlation hash, but /// neither the full path nor the formatted parser or I/O error. -pub(super) fn warn_explicit_config_load_failed(path: &Path, failure_kind: ConfigLoadFailureKind) { +pub(super) fn warn_explicit_config_load_failed_from_fields( + path: &BoundedConfigPath, + failure_kind: ConfigLoadFailureKind, +) { + let path_hash = path.hash.as_deref().unwrap_or_default(); warn!( - path_hash = %path_hash(path), - path_file_name = ?path.file_name(), + path_hash = %path_hash, + path_file_name = ?path.file_name, failure_kind = ?failure_kind, "explicit config load failed" ); } -/// Emit `message` with bounded fields identifying `path`. -pub(super) fn debug_config_path(message: &'static str, path: &Path) { - debug!( - path_hash = %path_hash(path), - path_file_name = ?path.file_name(), - message - ); -} - /// Replay an explicit path diagnostic from retained bounded fields. pub(super) fn debug_config_path_from_fields(message: &'static str, path: &BoundedConfigPath) { let path_hash = path.hash.as_deref().unwrap_or_default(); diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index c4f63a760..39240f43b 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -58,8 +58,11 @@ fn collect_diag_file_layers_logs_selected_branch( let cli = scenario_cli(scenario, &temp)?; let env = empty_mock_env(); - let (discovered, events) = - capture_events(|| Ok::<_, anyhow::Error>(collect_diag_file_layers_with_env(&cli, &env)))?; + let discovered = collect_diag_file_layers_with_env(&cli, &env); + let ((), events) = capture_events(|| { + discovered.emit_diagnostics(); + Ok::<_, anyhow::Error>(()) + })?; let branch_event = find_event(&events, expected_event)?; let layers = discovered.layers(); @@ -87,6 +90,27 @@ fn collect_diag_file_layers_logs_selected_branch( Ok(()) } +/// Discovery returns its diagnostics without emitting them. +#[test] +fn discovery_defers_diagnostics_to_the_composition_boundary() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; + let env = empty_mock_env(); + + let (discovered, discovery_events) = + capture_events(|| Ok::<_, anyhow::Error>(discover_file_layers(&cli, &env)))?; + ensure!( + discovery_events.is_empty(), + "discovery must not emit tracing events: {discovery_events:?}" + ); + + let ((), emitted_events) = capture_events(|| { + discovered.emit_diagnostics(); + Ok::<_, anyhow::Error>(()) + })?; + find_event(&emitted_events, "using explicit config path")?; + Ok(()) +} /// Cached replay emits the explicit selection branch without another env read. #[test] fn replay_logs_the_explicit_config_branch_without_environment_access() -> Result<()> { @@ -97,7 +121,7 @@ fn replay_logs_the_explicit_config_branch_without_environment_access() -> Result let discovered = discover_file_layers(&cli, &env); let ((), events) = capture_events(|| { - discovered.replay_config_path_trace(); + discovered.emit_diagnostics(); Ok::<_, anyhow::Error>(()) })?; find_event(&events, "resolved config path")?; @@ -124,7 +148,7 @@ fn replay_logs_discovery_and_appended_project_scope_without_environment_access() let discovered = discover_file_layers(&cli, &env); let ((), events) = capture_events(|| { - discovered.replay_config_path_trace(); + discovered.emit_diagnostics(); Ok::<_, anyhow::Error>(()) })?; find_event(&events, "read config path variable")?; @@ -153,7 +177,7 @@ fn replay_logs_included_project_scope_without_environment_access() -> Result<()> let discovered = discover_file_layers(&cli, &env); let ((), events) = capture_events(|| { - discovered.replay_config_path_trace(); + discovered.emit_diagnostics(); Ok::<_, anyhow::Error>(()) })?; find_event(&events, "using config discovery")?; @@ -244,7 +268,12 @@ fn existing_project_scope_layer_is_not_appended_twice() -> Result<()> { // Equivalent to `project_dir`, but not in canonical form. let non_canonical = project_dir.join("."); - let (layers, events) = capture_events(|| collect_file_layers(Some(non_canonical.as_path())))?; + let cli = Cli { + directory: Some(non_canonical), + ..Cli::default() + }; + let discovered = discover_file_layers(&cli, &empty_mock_env()); + let layers = discovered.layers(); let project_layers = layers .iter() @@ -258,6 +287,10 @@ fn existing_project_scope_layer_is_not_appended_twice() -> Result<()> { project_layers == 1, "project-scope layer should appear exactly once, found {project_layers}: {layers:?}" ); + let ((), events) = capture_events(|| { + discovered.emit_diagnostics(); + Ok::<_, anyhow::Error>(()) + })?; find_event(&events, "discovery included project-scope layers")?; Ok(()) } @@ -332,7 +365,7 @@ fn discover_file_layers_loads_an_explicit_config() -> Result<()> { "the explicit config should produce one layer" ); ensure!( - discovered.errors.is_empty(), + discovered.first_error().is_none(), "the explicit config should not produce discovery errors" ); Ok(()) @@ -353,7 +386,7 @@ fn discover_file_layers_records_an_explicit_load_error() -> Result<()> { "a missing explicit config should not produce layers" ); ensure!( - discovered.errors.len() == 1, + discovered.first_error().is_some(), "a missing explicit config should record one error" ); Ok(()) @@ -374,7 +407,7 @@ fn discover_file_layers_supports_discovery_without_a_selector() -> Result<()> { "an empty directory should not produce config layers" ); ensure!( - discovered.errors.is_empty(), + discovered.first_error().is_none(), "an empty directory should not produce discovery errors" ); Ok(()) @@ -397,7 +430,7 @@ fn discover_file_layers_performs_the_project_scope_second_pass() -> Result<()> { "the project pass should discover its config layer" ); ensure!( - discovered.errors.is_empty(), + discovered.first_error().is_none(), "the project pass should not produce discovery errors" ); Ok(()) diff --git a/src/cli/discovery_layers.rs b/src/cli/discovery_layers.rs index 0899d66c7..1864e71cc 100644 --- a/src/cli/discovery_layers.rs +++ b/src/cli/discovery_layers.rs @@ -21,8 +21,8 @@ pub(super) enum ProjectScopeTrace { } impl ProjectScopeTrace { - /// Replay the original project-scope diagnostic without filesystem access. - pub(super) fn replay(&self) { + /// Emit the original project-scope diagnostic from bounded metadata. + pub(super) fn emit(&self) { match self { Self::Included(path) => { debug_optional_config_path_from_fields( @@ -63,7 +63,8 @@ pub(crate) fn collect_file_layers( /// frequently does not exist, or a directory the process cannot read — is /// compared literally with `OrthoConfig`'s already-canonicalized layer path /// rather than failing discovery. An exact textual match still identifies the layer; -/// otherwise the project-scope pass appends it and emits its normal debug event. +/// otherwise the project-scope pass appends it and retains its normal debug +/// event for the composition boundary. fn comparison_key(normalizer: &impl PathNormalizer, path: &str) -> PathBuf { normalized_path_key(normalizer, path).unwrap_or_else(|_| PathBuf::from(path)) } @@ -119,12 +120,10 @@ fn collect_file_layers_with_normalizer_and_trace( let project_trace_path = BoundedConfigPath::from_path(project_file.as_deref()); if has_project_layer { let trace = ProjectScopeTrace::Included(project_trace_path); - trace.replay(); return (Some(trace), Ok(file_layers.value)); } let trace = ProjectScopeTrace::Appended(project_trace_path); - trace.replay(); let result = project_scope_layers(project_file.as_deref()).map(|project_layers| { file_layers .value diff --git a/src/cli/discovery_trace.rs b/src/cli/discovery_trace.rs index 653d40e45..1f3d185d4 100644 --- a/src/cli/discovery_trace.rs +++ b/src/cli/discovery_trace.rs @@ -8,7 +8,8 @@ use tracing::debug; use super::ConfigPathResolution; use super::diagnostics::{ - BoundedConfigPath, debug_config_path_from_fields, trace_config_path_variable_from_fields, + BoundedConfigPath, ConfigLoadWarning, debug_config_path_from_fields, + trace_config_path_variable_from_fields, }; use super::layers::ProjectScopeTrace; @@ -28,10 +29,10 @@ impl DiscoveryTrace { } } - /// Replay all discovery diagnostics from bounded metadata only. - pub(super) fn replay(&self) { - self.resolution.replay(); - self.file_layers.replay(); + /// Emit all discovery diagnostics from bounded metadata only. + pub(super) fn emit(&self) { + self.resolution.emit(); + self.file_layers.emit(); } } @@ -57,8 +58,8 @@ impl ConfigPathTrace { } } - /// Replay selector diagnostics from their bounded representation. - fn replay(&self) { + /// Emit selector diagnostics from their bounded representation. + fn emit(&self) { for (var_name, path) in &self.environment_lookups { trace_config_path_variable_from_fields(var_name, path); } @@ -84,8 +85,8 @@ pub(super) enum FileLayerTrace { } impl FileLayerTrace { - /// Replay the selected layer-collection branch without filesystem access. - fn replay(&self) { + /// Emit the selected layer-collection branch without filesystem access. + fn emit(&self) { match self { Self::Explicit { path } => { debug_config_path_from_fields("using explicit config path", path); @@ -93,9 +94,37 @@ impl FileLayerTrace { Self::Automatic { project_scope } => { debug!("using config discovery"); if let Some(trace) = project_scope { - trace.replay(); + trace.emit(); } } } } } + +/// Diagnostics deferred until a composition boundary enables its filter. +#[derive(Clone, Debug)] +pub(super) struct DiscoveryDiagnostics { + trace: DiscoveryTrace, + load_warning: Option, +} + +impl DiscoveryDiagnostics { + /// Combine bounded discovery events and any explicit-load warning. + pub(super) const fn new( + trace: DiscoveryTrace, + load_warning: Option, + ) -> Self { + Self { + trace, + load_warning, + } + } + + /// Emit deferred diagnostics without querying the environment or filesystem. + pub(super) fn emit(&self) { + self.trace.emit(); + if let Some(warning) = &self.load_warning { + warning.emit(); + } + } +} diff --git a/src/cli/discovery_tracing_tests.rs b/src/cli/discovery_tracing_tests.rs index 19dce6e7a..42b72646c 100644 --- a/src/cli/discovery_tracing_tests.rs +++ b/src/cli/discovery_tracing_tests.rs @@ -33,7 +33,13 @@ fn resolve_and_trace( ) -> Result<(ConfigPathResolution, Vec)> { capture_events(|| { let resolution = resolve_config_selector(cli_config, env); - trace_config_path_resolution(&resolution); + DiscoveryTrace::new( + &resolution, + FileLayerTrace::Automatic { + project_scope: None, + }, + ) + .emit(); Ok::<_, anyhow::Error>(resolution) }) } @@ -205,17 +211,19 @@ fn load_layers_from_path_logs_bounded_failure_fields() -> Result<()> { let temp = tempdir().context("create temp dir")?; let missing_path = temp.path().join("missing-secret-name.toml"); - let (error, events) = capture_events(|| { - Ok::<_, anyhow::Error>( - load_layers_from_path(&missing_path) - .expect_err("missing explicit config file should fail"), - ) + let (warning, load_result) = load_layers_from_path_with_warning(&missing_path); + let (load_error, events) = capture_events(|| { + warning + .as_ref() + .expect("missing explicit config should retain a warning") + .emit(); + Ok::<_, anyhow::Error>(load_result.expect_err("missing explicit config file should fail")) })?; let warn_event = find_event(&events, "explicit config load failed")?; let assertion = EventAssertion::new(warn_event, &missing_path); ensure!( - error.to_string().contains("missing-secret-name.toml"), + load_error.to_string().contains("missing-secret-name.toml"), "returned error should retain the diagnostic path" ); ensure!( @@ -227,7 +235,7 @@ fn load_layers_from_path_logs_bounded_failure_fields() -> Result<()> { !warn_event.contains("error="), "warn event should not include full formatted error text: {warn_event}" ); - assertion.ensure_private_event_fields(&error.to_string())?; + assertion.ensure_private_event_fields(&load_error.to_string())?; snapshot_failure_event(&assertion, "explicit_load_missing_event_schema")?; Ok(()) } @@ -239,14 +247,16 @@ fn load_layers_from_path_logs_invalid_toml_failure() -> Result<()> { test_support::fs::write(&config_path, "theme = [invalid parser secret\n") .with_context(|| format!("write {}", config_path.display()))?; - let (error, events) = capture_events(|| { - Ok::<_, anyhow::Error>( - load_layers_from_path(&config_path) - .expect_err("invalid explicit config file should fail"), - ) + let (warning, load_result) = load_layers_from_path_with_warning(&config_path); + let (load_error, events) = capture_events(|| { + warning + .as_ref() + .expect("invalid explicit config should retain a warning") + .emit(); + Ok::<_, anyhow::Error>(load_result.expect_err("invalid explicit config file should fail")) })?; let warn_event = find_event(&events, "explicit config load failed")?; - let formatted_error = error.to_string(); + let formatted_error = load_error.to_string(); ensure!( warn_event.contains("failure_kind=LoadError"), diff --git a/src/cli/merge.rs b/src/cli/merge.rs index 405acb607..aa190d1b9 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -44,7 +44,9 @@ use super::validation_error; /// Returns an [`ortho_config::OrthoError`] if layer composition or merging /// fails. pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - merge_with_process_environment_layers(cli, matches, discover_file_layers(cli, &DefaultEnv)) + let outcome = discover_file_layers(cli, &DefaultEnv); + outcome.emit_diagnostics(); + merge_with_process_environment_layers(cli, matches, outcome.into_layers()) } /// Merge configuration layers using an explicit environment provider. @@ -62,7 +64,9 @@ pub fn merge_with_config_and_env( matches: &ArgMatches, env: &impl Env, ) -> OrthoResult { - merge_with_layers(cli, matches, env, discover_file_layers(cli, env)) + let outcome = discover_file_layers(cli, env); + outcome.emit_diagnostics(); + merge_with_layers(cli, matches, env, outcome.into_layers()) } /// Merge cached file layers with a raw snapshot of the process environment. diff --git a/src/cli/mod.rs b/src/cli/mod.rs index bb52d2d0b..c1fa8c166 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -25,7 +25,7 @@ pub use diag::{ resolve_json_and_layers_outcome_with_env, resolve_json_and_layers_with_env, resolve_merged_json, resolve_merged_json_with_env, }; -pub use discovery::DiscoveredLayers; +pub use discovery::{DiscoveredLayers, DiscoveryOutcome}; pub use merge::{ merge_with_config, merge_with_config_and_env, merge_with_layers, merge_with_process_environment_layers, diff --git a/src/main.rs b/src/main.rs index 9999aa67a..97a3cc176 100644 --- a/src/main.rs +++ b/src/main.rs @@ -225,19 +225,19 @@ fn resolve_diag_mode_or_exit( matches: &ArgMatches, fallback_mode: DiagMode, ) -> Result<(DiagMode, cli::DiscoveredLayers), ExitCode> { - let (result, layers) = + let (result, outcome) = cli::resolve_json_and_layers_outcome_with_env(parsed_cli, matches, &DefaultEnv); match result { Ok(is_json_enabled) => { let mode = DiagMode::from_json_enabled(is_json_enabled); set_tracing_filter(startup_filter(mode, parsed_cli.verbose)); - layers.replay_config_path_trace(); - Ok((mode, layers)) + outcome.emit_diagnostics(); + Ok((mode, outcome.into_layers())) } Err(err) => { let fallback_filter = startup_filter(fallback_mode, parsed_cli.verbose); set_tracing_filter(fallback_filter); - layers.replay_config_path_trace(); + outcome.emit_diagnostics(); Err(config_err_to_exit(err.as_ref(), fallback_mode)) } } diff --git a/tests/command_env_ui_tests.rs b/tests/command_env_ui_tests.rs index 0dbe45517..f480bd307 100644 --- a/tests/command_env_ui_tests.rs +++ b/tests/command_env_ui_tests.rs @@ -1,10 +1,13 @@ -//! Compile-time tests for the explicit Ninja environment API. +//! Compile-time tests for public environment-injection APIs. //! //! The fixture in `tests/ui/command_env_embedder_pass.rs` imports and //! constructs `CommandEnv`, `NinjaBuildRequest`, and `NinjaToolRequest`, and //! references `run_ninja_with`/`run_ninja_tool_with`, exactly as an external //! embedder would, so a visibility or signature regression fails this suite //! rather than only the crate's own tests. +//! The cached CLI configuration fixture exercises the equivalent public +//! boundary for `mockable::Env` and `DiscoveredLayers` through Cargo, which +//! resolves the identical `mockable` crate instance expected by Netsuke. //! //! There is deliberately no compile-fail case for the removed APIs //! (`EnvMut`, `PathGuard`, `prepend_dir_to_path`, `override_ninja_env`): the @@ -24,6 +27,7 @@ use std::{ path::{Path, PathBuf}, process::{Command, Output}, }; +use test_support::fs as test_fs; /// The embedder fixture type-checks against the public API. #[test] @@ -34,6 +38,41 @@ fn command_env_embedder_fixture_compiles() -> io::Result<()> { ) } +/// The CLI configuration fixture type-checks against the public cache API. +#[test] +fn cli_configuration_fixture_compiles() -> io::Result<()> { + let temporary_root = tempfile::tempdir_in(manifest_dir().join("target"))?; + let fixture_dir = temporary_root.path().join("cli_configuration_pass"); + test_fs::create_dir_all(fixture_dir.join("src"))?; + test_fs::copy( + manifest_dir().join("tests/ui/cli_configuration_pass/Cargo.toml"), + fixture_dir.join("Cargo.toml"), + )?; + test_fs::copy( + manifest_dir().join("tests/ui/cli_configuration_pass/src/main.rs"), + fixture_dir.join("src/main.rs"), + )?; + + let manifest = fixture_dir.join("Cargo.toml"); + let output = Command::new(cargo()) + .arg("check") + .arg("--manifest-path") + .arg(manifest) + .env( + "CARGO_TARGET_DIR", + manifest_dir().join("target/cli-configuration-ui"), + ) + .output()?; + + if !output.status.success() { + return Err(io::Error::other(format!( + "the CLI configuration fixture should compile against the public API:\n{}", + stderr(&output), + ))); + } + Ok(()) +} + /// The public command-list constructors compile for an external embedder. #[test] fn command_list_public_api_fixture_compiles() -> io::Result<()> { diff --git a/tests/ui/cli_configuration_pass/Cargo.toml b/tests/ui/cli_configuration_pass/Cargo.toml new file mode 100644 index 000000000..820e13f4e --- /dev/null +++ b/tests/ui/cli_configuration_pass/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "netsuke-cli-configuration-ui" +version = "0.0.0" +edition = "2024" +publish = false + +[workspace] + +[dependencies] +mockable = "3.0" +netsuke = { package = "netsuke-build", path = "../../.." } diff --git a/tests/ui/cli_configuration_pass/src/main.rs b/tests/ui/cli_configuration_pass/src/main.rs new file mode 100644 index 000000000..9092cc27d --- /dev/null +++ b/tests/ui/cli_configuration_pass/src/main.rs @@ -0,0 +1,31 @@ +//! Compile-pass fixture for Netsuke's public cached configuration API. +//! +//! Cargo resolves `mockable` alongside `netsuke-build`, so this verifies the +//! public `mockable::Env` boundary exactly as an external embedder uses it. + +use mockable::DefaultEnv; +use netsuke::{cli, cli_localization}; +use std::sync::Arc; + +fn compose_cached_configuration_flow() { + let localizer = Arc::from(cli_localization::build_localizer(None)); + let (parsed, matches) = + match cli::parse_with_localizer_from(["netsuke", "generate"], &localizer) { + Ok(parsed) => parsed, + Err(_) => return, + }; + let env = DefaultEnv; + + let _ = cli::resolve_merged_json_with_env(&parsed, &matches, &env); + let _ = cli::resolve_json_and_layers_with_env(&parsed, &matches, &env); + let (result, outcome) = cli::resolve_json_and_layers_outcome_with_env(&parsed, &matches, &env); + outcome.emit_diagnostics(); + let layers = outcome.into_layers(); + let _ = result; + let _ = cli::merge_with_layers(&parsed, &matches, &env, layers); + let _ = cli::merge_with_config_and_env(&parsed, &matches, &env); +} + +fn main() { + let _ = compose_cached_configuration_flow; +} From 70e9d99c2e39dae0bbea0f26a2953c1cadd88ad6 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 21:21:00 +0200 Subject: [PATCH 10/21] Track the CLI API fixture with Dependabot (#319) Register the compile-pass fixture's Cargo manifest so dependency updates continue to cover every checked-in Rust package and the manifest inventory gate remains accurate. --- .github/dependabot.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 02ba966c7..2d531f7db 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -43,6 +43,7 @@ updates: directories: - "/" - "/test_support" + - "/tests/ui/cli_configuration_pass" open-pull-requests-limit: 5 labels: - "dependencies" From 960abfeaf259912773214b5911b030e3b9305fc5 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 14 Aug 2026 21:39:40 +0200 Subject: [PATCH 11/21] Measure cached configuration discovery reuse (#319) Record a bounded counter at each full-merge boundary to distinguish reuse of pre-discovered layers from standalone discovery. Keep paths, selectors, errors, and configuration values out of metric labels. Cover both outcomes with local recorders and document the telemetry contract for future startup observability work. --- Cargo.toml | 1 + docs/developers-guide.md | 20 +++++++++ src/cli/merge.rs | 47 ++++++++++++++++++-- tests/cli_tests/merge_diag.rs | 81 ++++++++++++++++++++++++++++++++++- 4 files changed, 144 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ab615e549..ca3bed727 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,6 +137,7 @@ sys-locale = "0.3.2" cap-std = "3.4.4" clap = { version = "4.5.0", features = ["derive"] } clap_mangen = "0.3.0" +metrics = "0.24.6" mockable = "3.0" ortho_config = { version = "0.9.0", features = ["serde_json"] } serde = { version = "1", features = ["derive"] } diff --git a/docs/developers-guide.md b/docs/developers-guide.md index b273c7f87..f5dc258d5 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2613,6 +2613,26 @@ file-layer branch, and applicable project-scope outcome without performing another environment lookup, filesystem scan, path normalization, or configuration-file load. + +#### Cached discovery telemetry + +The full-merge boundary increments +`netsuke_cli_config_discovery_cache_total` exactly once per merge. Its sole +`outcome` label belongs to a closed two-value set: + +- `reused` — `merge_with_layers` or + `merge_with_process_environment_layers` consumed layers discovered by an + earlier phase. +- `bypass` — `merge_with_config` or `merge_with_config_and_env` performed + standalone discovery immediately before merging. + +This counter measures the layer handoff only. Configuration-load success, +failure, and duration belong to the startup observability boundary, so this +metric does not duplicate them. It carries no selectors, paths, error text, or +configuration values. Recorder-backed tests pin both outcomes through local +`metrics_util::DebuggingRecorder` instances without installing process-global +state. + Tracing never logs full paths or formatted parser errors. Path values are bounded to a `path_hash` correlation identifier plus `path_file_name`, and load failures are classified with the `ConfigLoadFailureKind` enum instead of the diff --git a/src/cli/merge.rs b/src/cli/merge.rs index aa190d1b9..befd936b3 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -22,6 +22,7 @@ use clap::ArgMatches; use clap::parser::ValueSource; +use metrics::{counter, describe_counter}; use mockable::{DefaultEnv, Env}; use ortho_config::declarative::LayerComposition; use ortho_config::figment::Figment; @@ -29,7 +30,7 @@ use ortho_config::{MergeComposer, OrthoMergeExt, OrthoResult, sanitize_value}; use serde::Serialize; use serde_json::{Map, Value, json}; -use std::ffi::OsString; +use std::{ffi::OsString, sync::Once}; use super::config::{BuildConfig, CliConfig}; use super::discovery::{DiscoveredLayers, discover_file_layers, push_discovered_file_layers}; @@ -37,6 +38,8 @@ use super::environment::EnvironmentLayer; use super::parser::{BuildArgs, Cli, Commands}; use super::validation_error; +const CONFIG_DISCOVERY_CACHE_TOTAL: &str = "netsuke_cli_config_discovery_cache_total"; + /// Merge discovered configuration layers over parsed CLI input. /// /// # Errors @@ -46,7 +49,13 @@ use super::validation_error; pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { let outcome = discover_file_layers(cli, &DefaultEnv); outcome.emit_diagnostics(); - merge_with_process_environment_layers(cli, matches, outcome.into_layers()) + record_config_discovery_cache("bypass"); + merge_with_layers_and_entries( + cli, + matches, + outcome.into_layers(), + process_environment_entries(), + ) } /// Merge configuration layers using an explicit environment provider. @@ -66,7 +75,8 @@ pub fn merge_with_config_and_env( ) -> OrthoResult { let outcome = discover_file_layers(cli, env); outcome.emit_diagnostics(); - merge_with_layers(cli, matches, env, outcome.into_layers()) + record_config_discovery_cache("bypass"); + merge_with_injected_environment(cli, matches, env, outcome.into_layers()) } /// Merge cached file layers with a raw snapshot of the process environment. @@ -85,6 +95,7 @@ pub fn merge_with_process_environment_layers( matches: &ArgMatches, layers: DiscoveredLayers, ) -> OrthoResult { + record_config_discovery_cache("reused"); merge_with_layers_and_entries(cli, matches, layers, process_environment_entries()) } @@ -99,6 +110,21 @@ pub fn merge_with_layers( matches: &ArgMatches, env: &impl Env, layers: DiscoveredLayers, +) -> OrthoResult { + record_config_discovery_cache("reused"); + merge_with_injected_environment(cli, matches, env, layers) +} + +/// Merge layers with the Unicode environment snapshot supplied by `env`. +/// +/// Cache-aware entry points record their bounded outcome before calling this +/// helper; keeping recording outside prevents standalone discovery from being +/// misclassified as reuse. +fn merge_with_injected_environment( + cli: &Cli, + matches: &ArgMatches, + env: &impl Env, + layers: DiscoveredLayers, ) -> OrthoResult { let environment_entries = env .all() @@ -108,6 +134,21 @@ pub fn merge_with_layers( merge_with_layers_and_entries(cli, matches, layers, environment_entries) } +/// Record whether the full merge reused pre-discovered file layers. +/// +/// `outcome` is confined to the closed set `reused` and `bypass`; selectors, +/// paths, failures, and configuration payloads never become metric labels. +fn record_config_discovery_cache(outcome: &'static str) { + static DESCRIBE: Once = Once::new(); + DESCRIBE.call_once(|| { + describe_counter!( + CONFIG_DISCOVERY_CACHE_TOTAL, + "Counts full merges by cached discovery reuse or standalone bypass." + ); + }); + counter!(CONFIG_DISCOVERY_CACHE_TOTAL, "outcome" => outcome).increment(1); +} + fn merge_with_layers_and_entries( cli: &Cli, matches: &ArgMatches, diff --git a/tests/cli_tests/merge_diag.rs b/tests/cli_tests/merge_diag.rs index 6a87a2043..2f1fb237f 100644 --- a/tests/cli_tests/merge_diag.rs +++ b/tests/cli_tests/merge_diag.rs @@ -2,11 +2,49 @@ use anyhow::{Context, Result, ensure}; use cap_std::{ambient_authority, fs::Dir}; +use metrics::{SharedString, Unit}; +use metrics_util::{ + CompositeKey, MetricKind, + debugging::{DebugValue, DebuggingRecorder}, +}; use mockable::MockEnv; use netsuke::cli_localization; use std::{collections::HashMap, ffi::OsString, sync::Arc}; use tempfile::tempdir; +const CONFIG_DISCOVERY_CACHE_TOTAL: &str = "netsuke_cli_config_discovery_cache_total"; + +type SnapshotEntry = (CompositeKey, Option, Option, DebugValue); + +fn recorded(operation: impl FnOnce() -> T) -> (T, Vec) { + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let result = metrics::with_local_recorder(&recorder, operation); + (result, snapshotter.snapshot().into_vec()) +} + +fn cache_outcomes(snapshot: &[SnapshotEntry]) -> Vec<(String, u64)> { + snapshot + .iter() + .filter_map(|(key, _unit, _description, value)| { + if key.kind() != MetricKind::Counter || key.key().name() != CONFIG_DISCOVERY_CACHE_TOTAL + { + return None; + } + let outcome = key + .key() + .labels() + .find(|label| label.key() == "outcome")? + .value() + .to_owned(); + match value { + DebugValue::Counter(count) => Some((outcome, *count)), + _ => None, + } + }) + .collect() +} + #[test] fn resolve_merged_json_honours_injected_env() -> Result<()> { let temp_dir = tempdir().context("create temporary config directory")?; @@ -61,8 +99,11 @@ fn diag_and_merge_reuse_one_discovery_result() -> Result<()> { let (is_json, layers) = netsuke::cli::resolve_json_and_layers_with_env(&cli, &matches, &env) .context("resolve diagnostic mode and discovered layers")?; - let merged = netsuke::cli::merge_with_layers(&cli, &matches, &env, layers) - .context("merge the cached layers")?; + let (merge_result, snapshot) = recorded(|| { + netsuke::cli::merge_with_layers(&cli, &matches, &env, layers) + .context("merge the cached layers") + }); + let merged = merge_result?; ensure!( is_json, @@ -72,5 +113,41 @@ fn diag_and_merge_reuse_one_discovery_result() -> Result<()> { merged.jobs == Some(13), "the merge should consume the discovered config layer" ); + ensure!( + cache_outcomes(&snapshot) == vec![("reused".to_owned(), 1)], + "the cached handoff should record exactly one reuse" + ); + Ok(()) +} + +#[test] +fn standalone_merge_records_discovery_cache_bypass() -> Result<()> { + let temp_dir = tempdir().context("create temporary config directory")?; + let config_path = temp_dir.path().join("netsuke.toml"); + let config_dir = Dir::open_ambient_dir(temp_dir.path(), ambient_authority()) + .context("open temporary config directory")?; + config_dir + .write("netsuke.toml", b"jobs = 7\n") + .context("write netsuke.toml")?; + + let localizer = Arc::from(cli_localization::build_localizer(None)); + let config_arg = config_path.to_string_lossy().into_owned(); + let (cli, matches) = + netsuke::cli::parse_with_localizer_from(["netsuke", "--config", &config_arg], &localizer) + .context("parse CLI")?; + let mut env = MockEnv::new(); + env.expect_os_string().never(); + env.expect_all().once().return_const(HashMap::new()); + + let (merged, snapshot) = recorded(|| { + netsuke::cli::merge_with_config_and_env(&cli, &matches, &env) + .context("merge with standalone discovery") + }); + + ensure!(merged?.jobs == Some(7), "the selected config should merge"); + ensure!( + cache_outcomes(&snapshot) == vec![("bypass".to_owned(), 1)], + "standalone discovery should record exactly one cache bypass" + ); Ok(()) } From f7342a6e4d63e4b00ebe7b5ebfa77bf94aaf9682 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 15 Aug 2026 20:36:44 +0200 Subject: [PATCH 12/21] Document cached configuration API (#319) Explain the unstable cached-discovery hand-offs, the replacement environment seam, and standalone composition alternatives for API callers. --- docs/users-guide.md | 42 ++++++++++++++++++++++++++++++--- docs/v0-1-0-migration-guide.md | 43 ++++++++++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index d24d61b36..ea100d73c 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -865,14 +865,50 @@ Events then identify whether Netsuke uses an explicit file or discovered layers. If an explicit file cannot be loaded, the warning records `failure_kind` as `Missing` or `LoadError`. Path fields are bounded to `path_hash` and -`path_file_name`; full paths and formatted parser errors are not tracing -fields. The file name is visible, and the unkeyed hash is only a correlation -identifier: it does not confidentially conceal a guessable path. +`path_present`; full paths, file names, and formatted parser errors are not +tracing fields. The unkeyed hash is only a correlation identifier: it does not +confidentially conceal a guessable path. Configuration tracing is disabled in JSON mode, including when `json = true` comes from a configuration file. This keeps stderr empty for successful JSON commands and reserves it for the single diagnostic document on failure. + +### Use cached configuration discovery + +Ordinary CLI users need no action. Netsuke performs configuration discovery and +merging internally. The following Rust API is unstable during the v0.1.0 beta +series and may change without a compatibility commitment. + +The cached configuration discovery API consists of +`resolve_json_and_layers_with_env`, `resolve_json_and_layers_outcome_with_env`, +`DiscoveredLayers`, `DiscoveryOutcome`, `merge_with_layers`, and +`merge_with_process_environment_layers`. A normal merge hands off the result as +follows: + +1. Call `resolve_json_and_layers_with_env`. +2. Retain its returned JSON decision and `DiscoveredLayers`. +3. Pass those exact discovered layers to `merge_with_layers`, along with the + injected environment. + +For startup diagnostics, the hand-off is: + +1. Call `resolve_json_and_layers_outcome_with_env`. +2. After the tracing filter is configured, call `emit_diagnostics()` on the + returned `DiscoveryOutcome`. +3. Call `into_layers()`. +4. Pass the resulting `DiscoveredLayers` to + `merge_with_process_environment_layers`. + +Both flows reuse the discovered layers, avoiding a second configuration-file +discovery and loading pass. The injected environment entry points use +`&impl mockable::Env`; production callers use `mockable::DefaultEnv`, while +deterministic tests use `mockable::MockEnv`. + +`ConfigEnvProvider` and `ConfigStdEnvProvider` are removed. The +[v0.1.0 migration guide](v0-1-0-migration-guide.md) describes the replacement +flow for callers of this unstable Rust API. + The annotated [sample configuration](sample-netsuke.toml) lists every key. A small project configuration looks like this: diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index e4675700e..db0c1b036 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -1,9 +1,11 @@ # Migrating to v0.1.0 -This guide signposts the child-environment additions arriving in the v0.1.0 -beta series: the injectable child environment (`CommandEnv`) and the named -Ninja request types. Existing callers compile unchanged; every addition is -opt-in. +This guide signposts child-environment additions, cached configuration +discovery, glob expansion, and ordered command lists arriving in the v0.1.0 +beta series. The child-environment additions are the injectable child +environment (`CommandEnv`) and the named Ninja request types. The cached +configuration discovery API is a breaking change for callers of the unstable +Rust API. The manifest additions are opt-in. Ordinary CLI users need no action. ## Netsuke is a build tool, not a library @@ -16,13 +18,14 @@ on it is conditional on tracking those changes. ## At-a-glance changes -Table: v0.1.0 child-environment API additions and their impact +Table: v0.1.0 migration changes and their impact | Area | Impact | Where to read more | | --- | --- | --- | | Convenience wrappers | Unchanged. `run_ninja` and `run_ninja_tool` behave exactly as before, inheriting the process environment. | [Users' guide](users-guide.md) | | Child environment | New opt-in `netsuke::runner::CommandEnv` carries additive variable overrides and an injected `PATH` for Ninja child processes. | [Users' guide](users-guide.md) | | Request types | New `netsuke::runner::NinjaBuildRequest` and `netsuke::runner::NinjaToolRequest` name the program, build file, and targets or tool for the `*_with` run functions. | [Users' guide](users-guide.md) | +| Cached CLI configuration API | Breaking for callers of the unstable Rust API. `ConfigEnvProvider` and `ConfigStdEnvProvider` are removed; use `mockable::Env` and the cached configuration discovery flow described below. | [Users' guide](users-guide.md) | | Glob expansion | Parent-relative patterns such as `glob('../shared/*.h')` now expand. Metadata checks use a capability rooted at the pattern's longest literal directory prefix; missing or non-directory prefixes return no matches, and unresolvable symlink matches are skipped. | [Users' guide](users-guide.md) and [ADR-010](adr-010-scope-glob-capability-to-literal-prefix.md) | | Command recipes | Existing scalar `command` recipes are unchanged. New YAML command lists are opt-in and run in declaration order with fail-fast semantics. | [Rules and recipes](users-guide.md#rules-and-recipes) | @@ -59,6 +62,36 @@ Both request types borrow their fields, so one `CommandEnv` and one `Cli` can serve several invocations. Worked examples live in the users' guide's "Drive Ninja with an explicit environment" section. + +## Cached CLI configuration API + +Callers of the unstable Rust configuration API must update to the cached +configuration discovery flow. `ConfigEnvProvider` and `ConfigStdEnvProvider` +are removed; custom providers should be replaced with `mockable::Env` at the +injected boundary (`&impl mockable::Env`). Production callers use +`mockable::DefaultEnv`, and deterministic tests use `mockable::MockEnv`. This +is a breaking change without a deprecation period or stable compatibility +guarantee. + +For the normal flow: + +1. Call `resolve_json_and_layers_with_env` and retain its returned + `DiscoveredLayers`. +2. Call `merge_with_layers` with the same discovered layers and the injected + environment. + +For startup diagnostics, call +`resolve_json_and_layers_outcome_with_env`, then call `emit_diagnostics()` +after the tracing filter is configured. Call `into_layers()` and pass the +resulting `DiscoveredLayers` to `merge_with_process_environment_layers`. +`DiscoveryOutcome` owns the deferred diagnostics until `emit_diagnostics()` +and the discovered layers until `into_layers()`. + +Reusing the same discovered layers avoids a second configuration-file discovery +and loading pass. `merge_with_config` and `merge_with_config_and_env` remain +standalone alternatives: each discovers and merges configuration in one call, +so neither reuses an earlier discovery. + ## Diagnostics Ninja subprocess spans and warn events carry two bounded fields, From 886e4de33b59cd23a0a80267a1de3ab03191dd0f Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 15 Aug 2026 20:47:50 +0200 Subject: [PATCH 13/21] Remove file names from config discovery traces (#319) Keep deferred configuration diagnostics to their correlation hash, presence, selector, and failure-class fields so verbose output cannot disclose a raw configuration file name. --- src/cli/discovery_diagnostics.rs | 17 ++++------- src/cli/discovery_event_assertions.rs | 29 ++++++++++--------- src/cli/discovery_trace.rs | 1 - src/cli/discovery_tracing_tests.rs | 5 +--- ...sts__explicit_load_error_event_schema.snap | 2 +- ...s__explicit_load_missing_event_schema.snap | 2 +- ...sts__selector_resolution_event_schema.snap | 4 +-- tests/logging_stderr/config_tracing.rs | 18 +++++++----- 8 files changed, 37 insertions(+), 41 deletions(-) diff --git a/src/cli/discovery_diagnostics.rs b/src/cli/discovery_diagnostics.rs index 67d09e6d4..026109fb0 100644 --- a/src/cli/discovery_diagnostics.rs +++ b/src/cli/discovery_diagnostics.rs @@ -1,12 +1,11 @@ //! Bounded diagnostics for configuration discovery. //! //! These helpers keep tracing output free of full paths and formatted parser -//! errors: a path contributes only a correlation hash and its file name, and a +//! errors: a path contributes only a correlation hash, and a //! load failure contributes a [`ConfigLoadFailureKind`] rather than the error //! text. use std::collections::hash_map::DefaultHasher; -use std::ffi::OsString; use std::hash::{Hash, Hasher}; use std::path::Path; use tracing::{debug, trace, warn}; @@ -48,12 +47,11 @@ impl ConfigLoadWarning { /// Bounded path fields retained for deferred discovery diagnostics. /// -/// This stores only the correlation hash, file name, and presence bit needed -/// to replay a diagnostic event. It deliberately excludes the full path. +/// This stores only the correlation hash and presence bit needed to replay a +/// diagnostic event. It deliberately excludes every raw path component. #[derive(Clone, Debug, PartialEq, Eq)] pub(super) struct BoundedConfigPath { pub(super) hash: Option, - pub(super) file_name: Option, pub(super) is_present: bool, } @@ -62,7 +60,6 @@ impl BoundedConfigPath { pub(super) fn from_path(path: Option<&Path>) -> Self { Self { hash: path.map(path_hash), - file_name: path.and_then(Path::file_name).map(OsString::from), is_present: path.is_some(), } } @@ -74,15 +71,14 @@ pub(super) fn trace_config_path_variable_from_fields(var_name: &str, path: &Boun var_name, found = path.is_present, path_hash = path.hash.as_deref(), - path_file_name = ?path.file_name, "read config path variable" ); } /// Warn that an explicit `path` failed with `failure_kind`. /// -/// The event exposes the failure class, file name, and correlation hash, but -/// neither the full path nor the formatted parser or I/O error. +/// The event exposes the failure class and correlation hash, but neither a raw +/// path component nor the formatted parser or I/O error. pub(super) fn warn_explicit_config_load_failed_from_fields( path: &BoundedConfigPath, failure_kind: ConfigLoadFailureKind, @@ -90,7 +86,6 @@ pub(super) fn warn_explicit_config_load_failed_from_fields( let path_hash = path.hash.as_deref().unwrap_or_default(); warn!( path_hash = %path_hash, - path_file_name = ?path.file_name, failure_kind = ?failure_kind, "explicit config load failed" ); @@ -101,7 +96,6 @@ pub(super) fn debug_config_path_from_fields(message: &'static str, path: &Bounde let path_hash = path.hash.as_deref().unwrap_or_default(); debug!( path_hash = %path_hash, - path_file_name = ?path.file_name, message ); } @@ -113,7 +107,6 @@ pub(super) fn debug_optional_config_path_from_fields( ) { debug!( path_hash = path.hash.as_deref(), - path_file_name = ?path.file_name, path_present = path.is_present, message ); diff --git a/src/cli/discovery_event_assertions.rs b/src/cli/discovery_event_assertions.rs index 4d82c2af6..9d98c1776 100644 --- a/src/cli/discovery_event_assertions.rs +++ b/src/cli/discovery_event_assertions.rs @@ -54,14 +54,10 @@ impl<'a> EventAssertion<'a> { Self { event, path } } - /// Assert the event carries the bounded `path_hash` and `path_file_name` - /// fields for the path in any accepted rendering form. + /// Assert the event carries the bounded `path_hash` field while omitting + /// every raw path component. pub(super) fn ensure_bounded_path_fields(&self) -> Result<()> { let hash = path_hash(self.path); - let file_name = self - .path - .file_name() - .with_context(|| format!("expected file name for {}", self.path.display()))?; ensure!( self.event.contains(&format!("path_hash=\"{hash}\"")) || self.event.contains(&format!("path_hash=Some(\"{hash}\")")) @@ -70,13 +66,20 @@ impl<'a> EventAssertion<'a> { self.path.display(), self.event ); - ensure!( - self.event - .contains(&format!("path_file_name=Some({file_name:?})")), - "event should include path file name for {}: {}", - self.path.display(), - self.event - ); + self.ensure_raw_file_name_absent()?; + Ok(()) + } + + /// Assert the event never leaks the selected path's file name. + fn ensure_raw_file_name_absent(&self) -> Result<()> { + if let Some(file_name) = self.path.file_name() { + ensure!( + !self.event.contains(file_name.to_string_lossy().as_ref()), + "event should not include file name for {}: {}", + self.path.display(), + self.event + ); + } Ok(()) } diff --git a/src/cli/discovery_trace.rs b/src/cli/discovery_trace.rs index 1f3d185d4..9aa4ac519 100644 --- a/src/cli/discovery_trace.rs +++ b/src/cli/discovery_trace.rs @@ -66,7 +66,6 @@ impl ConfigPathTrace { debug!( selector = self.selector, path_hash = self.path.hash.as_deref(), - path_file_name = ?self.path.file_name, path_present = self.path.is_present, "resolved config path" ); diff --git a/src/cli/discovery_tracing_tests.rs b/src/cli/discovery_tracing_tests.rs index 42b72646c..dda396ebd 100644 --- a/src/cli/discovery_tracing_tests.rs +++ b/src/cli/discovery_tracing_tests.rs @@ -115,11 +115,8 @@ fn explicit_config_path_logs_selected_selector(#[case] scenario: ConfigPathScena ); match resolved.as_deref() { Some(path) => EventAssertion::new(selector_event, path).ensure_bounded_path_fields()?, - // `Option: Value::record` omits `None`, while `record_debug` renders - // it as `path_file_name=None`; that asymmetry explains these checks. None => ensure!( - !selector_event.contains("path_hash=") - && selector_event.contains("path_file_name=None"), + !selector_event.contains("path_hash="), "empty selection should not include path details: {selector_event}" ), } diff --git a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_error_event_schema.snap b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_error_event_schema.snap index ac1b4e263..8bd5be2a8 100644 --- a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_error_event_schema.snap +++ b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_error_event_schema.snap @@ -2,4 +2,4 @@ source: src/cli/discovery_tests.rs expression: normalized --- -message=explicit config load failed path_hash=[path_hash] path_file_name=Some("invalid-secret-config.toml") failure_kind=LoadError +message=explicit config load failed path_hash=[path_hash] failure_kind=LoadError diff --git a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_missing_event_schema.snap b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_missing_event_schema.snap index c9110568a..9240b5ba9 100644 --- a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_missing_event_schema.snap +++ b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__explicit_load_missing_event_schema.snap @@ -2,4 +2,4 @@ source: src/cli/discovery_tests.rs expression: normalized --- -message=explicit config load failed path_hash=[path_hash] path_file_name=Some("missing-secret-name.toml") failure_kind=Missing +message=explicit config load failed path_hash=[path_hash] failure_kind=Missing diff --git a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__selector_resolution_event_schema.snap b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__selector_resolution_event_schema.snap index 60e267b2b..75a0ef60d 100644 --- a/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__selector_resolution_event_schema.snap +++ b/src/snapshots/discovery/netsuke__cli__discovery__tracing_tests__selector_resolution_event_schema.snap @@ -2,5 +2,5 @@ source: src/cli/discovery_tests.rs expression: normalized --- -message=read config path variable var_name="NETSUKE_CONFIG" found=true path_hash="[path_hash]" path_file_name=Some("selector.toml") -message=resolved config path selector="NETSUKE_CONFIG" path_hash="[path_hash]" path_file_name=Some("selector.toml") path_present=true +message=read config path variable var_name="NETSUKE_CONFIG" found=true path_hash="[path_hash]" +message=resolved config path selector="NETSUKE_CONFIG" path_hash="[path_hash]" path_present=true diff --git a/tests/logging_stderr/config_tracing.rs b/tests/logging_stderr/config_tracing.rs index c8beb9dfb..72eafb09c 100644 --- a/tests/logging_stderr/config_tracing.rs +++ b/tests/logging_stderr/config_tracing.rs @@ -48,7 +48,7 @@ fn workspace() -> Result { #[test] fn explicit_selection_traces_bounded_fields() -> Result<()> { let temp = workspace()?; - let config = temp.path().join("selected-secret-name.toml"); + let config = temp.path().join("customer@example.com.toml"); test_support::fs::write(&config, "emoji = \"always\"\n").context("write config")?; let raw_path = config.to_string_lossy().into_owned(); @@ -64,12 +64,12 @@ fn explicit_selection_traces_bounded_fields() -> Result<()> { "stderr should name the winning selector: {joined}" ); ensure!( - joined.contains("path_hash=") && joined.contains("path_file_name="), - "stderr should carry the bounded path fields: {joined}" + joined.contains("path_hash="), + "stderr should carry the bounded path hash: {joined}" ); ensure!( - joined.contains("selected-secret-name.toml"), - "the bounded file name should be present: {joined}" + !joined.contains("customer@example.com.toml"), + "diagnostics must not log the raw config file name: {joined}" ); ensure!( joined.contains("using explicit config path"), @@ -105,8 +105,12 @@ fn explicit_load_failure_traces_failure_kind() -> Result<()> { "stderr should classify the failure: {joined}" ); ensure!( - joined.contains("path_hash=") && joined.contains("missing-secret-name.toml"), - "stderr should carry the bounded path fields: {joined}" + joined.contains("path_hash="), + "stderr should carry the bounded path hash: {joined}" + ); + ensure!( + !joined.contains("missing-secret-name.toml"), + "diagnostics must not log the raw config file name: {joined}" ); ensure!( !joined.contains(raw_path.as_str()), From 84a64d894edefd36f822982acdcf13f3fbb6d76f Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 15 Aug 2026 20:54:51 +0200 Subject: [PATCH 14/21] Drop UI fixture configuration results (#319) Make the compile-pass fixture explicitly discard its `OrthoResult` values without changing its cached-configuration flow or function-item binding. --- tests/ui/cli_configuration_pass/src/main.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/ui/cli_configuration_pass/src/main.rs b/tests/ui/cli_configuration_pass/src/main.rs index 9092cc27d..5e3f2e607 100644 --- a/tests/ui/cli_configuration_pass/src/main.rs +++ b/tests/ui/cli_configuration_pass/src/main.rs @@ -16,14 +16,14 @@ fn compose_cached_configuration_flow() { }; let env = DefaultEnv; - let _ = cli::resolve_merged_json_with_env(&parsed, &matches, &env); - let _ = cli::resolve_json_and_layers_with_env(&parsed, &matches, &env); + drop(cli::resolve_merged_json_with_env(&parsed, &matches, &env)); + drop(cli::resolve_json_and_layers_with_env(&parsed, &matches, &env)); let (result, outcome) = cli::resolve_json_and_layers_outcome_with_env(&parsed, &matches, &env); outcome.emit_diagnostics(); let layers = outcome.into_layers(); - let _ = result; - let _ = cli::merge_with_layers(&parsed, &matches, &env, layers); - let _ = cli::merge_with_config_and_env(&parsed, &matches, &env); + drop(result); + drop(cli::merge_with_layers(&parsed, &matches, &env, layers)); + drop(cli::merge_with_config_and_env(&parsed, &matches, &env)); } fn main() { From 99b59b6eadb824bb57c78ab7f1b7b1275ee0d781 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 15 Aug 2026 21:13:33 +0200 Subject: [PATCH 15/21] Normalize rebased configuration guide spacing (#319) Remove duplicate blank lines introduced while reconciling the migration and configuration documentation during the rebase. --- docs/users-guide.md | 1 - docs/v0-1-0-migration-guide.md | 1 - 2 files changed, 2 deletions(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index ea100d73c..cb42565c2 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -873,7 +873,6 @@ Configuration tracing is disabled in JSON mode, including when `json = true` comes from a configuration file. This keeps stderr empty for successful JSON commands and reserves it for the single diagnostic document on failure. - ### Use cached configuration discovery Ordinary CLI users need no action. Netsuke performs configuration discovery and diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index db0c1b036..347dc4aac 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -62,7 +62,6 @@ Both request types borrow their fields, so one `CommandEnv` and one `Cli` can serve several invocations. Worked examples live in the users' guide's "Drive Ninja with an explicit environment" section. - ## Cached CLI configuration API Callers of the unstable Rust configuration API must update to the cached From 08c8c09917dcbf7bc1ea9aa18cd712664eef7466 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 04:45:54 +0200 Subject: [PATCH 16/21] Remove stale discovery seam tests (#319) Drop tests carried from the superseded discovery-source adapter. The cached discovery boundary now injects only selector access through `mockable::Env`, so those tests cannot exercise a supported contract. --- src/cli/discovery_layer_tests.rs | 77 ++++---------------------------- 1 file changed, 8 insertions(+), 69 deletions(-) diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 39240f43b..71b14f904 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -4,19 +4,21 @@ //! versus automatic discovery — and the project-scope second pass in //! [`collect_file_layers`]. Selector precedence and event-schema snapshots live //! in the tracing test module. +use super::*; + use anyhow::{Context, Result, ensure}; -use googletest::prelude::*; -use pretty_assertions::assert_eq; use rstest::rstest; -use super::*; use tempfile::{TempDir, tempdir}; use crate::cli::test_support::empty_mock_env; use mockable::MockEnv; use std::ffi::OsString; -use super::event_assertions::{EventAssertion, capture_events, find_event}; -use super::layers::{collect_file_layers, collect_file_layers_with_normalizer}; -use super::paths::FailingPathNormalizer; + +use super::{ + event_assertions::{EventAssertion, capture_events, find_event}, + layers::{collect_file_layers, collect_file_layers_with_normalizer}, + paths::FailingPathNormalizer, +}; #[derive(Debug, Clone, Copy)] enum LayerScenario { @@ -186,69 +188,6 @@ fn replay_logs_included_project_scope_without_environment_access() -> Result<()> Ok(()) } -/// Automatic discovery must use the injected XDG directory, not the host. -#[test] -fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> { - let temp = tempdir().context("create temp dir")?; - let xdg_config_home = temp.path().join("xdg-config"); - let config_path = xdg_config_home.join("netsuke/config.toml"); - test_support::fs::create_dir(&xdg_config_home).context("create injected XDG directory")?; - test_support::fs::create_dir(config_path.parent().context("config parent")?) - .context("create injected config directory")?; - test_support::fs::write(&config_path, "json = true\n").context("write injected config")?; - - let env = TestEnv::default().with_var("XDG_CONFIG_HOME", xdg_config_home.as_os_str()); - let sources = DiscoverySources::new(&env, discovery_env_source(&env)); - let layers = collect_file_layers_with_env(&Cli::default(), &sources)?; - let paths = layers - .iter() - .filter_map(|layer| layer.path().map(|path| path.as_str().to_owned())) - .collect::>(); - - assert_eq!(paths, vec![config_path.to_string_lossy().into_owned()]); - Ok(()) -} - -/// Discovered configuration candidates retain the outcome that their content -/// warrants; an unreadable candidate is never mistaken for an absent one. -#[rstest] -#[case::no_candidate(None, None, 0)] -#[case::valid_candidate(Some("emoji = \"always\"\n"), None, 1)] -#[case::malformed_candidate(Some("emoji = \"always\n"), Some(".netsuke.toml"), 0)] -#[case::missing_parent( - Some("extends = \"missing-parent.toml\"\n"), - Some("missing-parent.toml"), - 0 -)] -fn discovered_project_config_retains_load_outcome( - #[case] contents: Option<&str>, - #[case] expected_error_fragment: Option<&str>, - #[case] expected_layer_count: usize, -) -> Result<()> { - let temp = tempdir().context("create temp dir")?; - if let Some(config_contents) = contents { - test_support::fs::write(temp.path().join(".netsuke.toml"), config_contents) - .context("write project config")?; - } - - let cli = Cli { - directory: Some(temp.path().to_path_buf()), - ..Cli::default() - }; - let env = TestEnv::default(); - let sources = DiscoverySources::new(&env, discovery_env_source(&env)); - let result = collect_file_layers_with_env(&cli, &sources); - - if let Some(fragment) = expected_error_fragment { - let error = result.expect_err("invalid discovered config must fail"); - assert_that!(error.to_string(), contains_substring(fragment)); - } else { - let layers = result.context("valid discovered config must load")?; - assert_eq!(layers.len(), expected_layer_count); - } - Ok(()) -} - /// A project-scope layer already found by discovery is not appended again. /// /// `OrthoConfig` records canonicalised layer paths, so a non-canonical From a956a609d8243e2588031fb2ba28bd1c26f382be Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 04:50:55 +0200 Subject: [PATCH 17/21] Allow linker and environment identifiers Keep the mold linker name and its uppercase configuration identifiers from being corrected as prose. Ignore the tokenized NETSUKE_COLOR environment variable as one technical identifier, and regenerate the tracked spelling policy from the local overlay. --- typos.local.toml | 7 +++++++ typos.toml | 3 +++ 2 files changed, 10 insertions(+) diff --git a/typos.local.toml b/typos.local.toml index fea64c0be..38a57c7a8 100644 --- a/typos.local.toml +++ b/typos.local.toml @@ -30,13 +30,20 @@ accepted = [ ] [words.corrections] +# `mold` names the linker binary (https://github.com/rui314/mold), and `MOLD` +# appears in the associated environment-variable names; neither is prose. +mold = "mold" +MOLD = "MOLD" [patterns] # The tokenizer splits the valid hyphenated prefix in `mis-grouping`. # Inline code spans quote identifiers verbatim (e.g. tokio's `flavor` # attribute argument), so they are not en-GB prose and must stay exempt. +# The scanner tokenizes the `COLOR` suffix in this environment variable +# separately, so ignore the complete identifier rather than accepting `COLOR`. ignore = ["mis-grouping", "`[^`\\n]+`", + "NETSUKE_COLOR", "(?m)^ dist/\\$\\{\\{ inputs\\['bin-name'\\] \\}\\}-\\$\\{\\{ inputs\\.version \\}\\}-\\$\\{\\{ inputs\\['artifact-suffix'\\] \\}\\}\\.pkg$", "(?m)^ ARCHIVE_SUFFIX: \\$\\{\\{ inputs\\['artifact-suffix'\\] \\}\\}$", "(?m)^ name: \\$\\{\\{ inputs\\['artifact-name'\\] \\}\\}$", diff --git a/typos.toml b/typos.toml index 8f776003d..c57280ed5 100644 --- a/typos.toml +++ b/typos.toml @@ -49,6 +49,7 @@ extend-ignore-re = [ "(?m)^ should_upload_workflow_artifacts: \\$\\{\\{ steps\\.release_modes\\.outputs\\['should-upload-workflow-artifacts'\\] \\}\\}$", "(?m)^ should_upload_workflow_artifacts:$", "(?s)```.*?```", + "NETSUKE_COLOR", "\\bartifact-dir\\b", "\\bartifact-name\\b", "\\bartifact-suffix\\b", @@ -59,6 +60,7 @@ extend-ignore-re = [ [default.extend-words] "ASO" = "ASO" +"MOLD" = "MOLD" "absolutisable" = "absolutizable" "absolutisably" = "absolutizably" "absolutisation" = "absolutization" @@ -1502,6 +1504,7 @@ extend-ignore-re = [ "modularizers" = "modularizers" "modularizes" = "modularizes" "modularizing" = "modularizing" +"mold" = "mold" "monetisable" = "monetizable" "monetisably" = "monetizably" "monetisation" = "monetization" From c15ea4e272f8c119bb8b838a899a6e37984ec3e3 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 04:58:29 +0200 Subject: [PATCH 18/21] Repair rebased developer guide (#319) Restore the upstream Quality gates section that the rebase split and duplicated. Keep cached-discovery diagnostics documentation aligned with the bounded fields emitted by the current implementation. --- docs/developers-guide.md | 140 ++++++++------------------------------- 1 file changed, 28 insertions(+), 112 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index f5dc258d5..0f8d71712 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -434,117 +434,34 @@ NEXTEST_VERSION="$(sed -n "s/.*NEXTEST_VERSION: '\(.*\)'.*/\1/p" \ .github/workflows/ci.yml)" cargo install cargo-nextest --locked --version "$NEXTEST_VERSION" # or, for a prebuilt binary: -cargo binstall --no-confirm --locked \ - "whitaker-installer@$WHITAKER_INSTALLER_VERSION" +cargo binstall --no-confirm "cargo-nextest@$NEXTEST_VERSION" ``` -`whitaker-installer` and the lint libraries are separate artefacts with -separate versions. `WHITAKER_INSTALLER_VERSION` pins the installer — the tool -that stages libraries — and nothing else. The installer keeps its own checkout -of the Whitaker repository under `~/.local/share/whitaker`, updates it with -`git pull`, and stages the libraries from its default branch. Lint behaviour -therefore tracks Whitaker HEAD. - -**Running the lint libraries at HEAD is deliberate.** Netsuke follows the suite -as it develops, so new lints and fixes arrive without a version bump here. Do -not add a `[workspace.metadata.dylint]` block pinning `whitaker_suite` to a -`tag` or `rev`. The [Whitaker user's guide](whitaker-users-guide.md) documents -that form, and it is the right answer for a project wanting reproducible lint -results, but adopting it here would reverse a standing decision rather than fix -a defect. - -The cost is worth stating plainly: a change upstream can alter lint results -between two runs with no change in this repository, and a local checkout that -has not been restaged will disagree with CI, which stages fresh on every job. -Restaging is what reconciles them. - -What the module-scoped exemptions in `dylint.toml` actually depend on is -[Whitaker PR #315][whitaker-pr-315], which added the `excluded_paths` option, -so the staged libraries must be recent enough to include it. Libraries staged -from an older checkout ignore `excluded_paths` silently — the exemptions stop -applying with no error, and the lint reports the modules they covered. Re-run -`whitaker-installer` to restage from HEAD. If that checkout has been left on a -detached HEAD, the install fails at its `git pull`; put it back on the default -branch and re-run. - -[whitaker-pr-315]: https://github.com/leynos/whitaker/pull/315 - -Whitaker is configured by `dylint.toml` at the repository root, where each -sanctioned ambient-filesystem scope for `no_std_fs_operations` carries a -documented rationale. `docs/whitaker-users-guide.md` is a near-verbatim import -of the [upstream Whitaker user's guide][whitaker-upstream-guide]; refresh it -from that URL rather than editing it in place, preserving the "Netsuke -deviation from upstream" callout, and record Netsuke-specific policy here and in -`dylint.toml`. - -[whitaker-upstream-guide]: https://raw.githubusercontent.com/leynos/whitaker/refs/heads/main/docs/users-guide.md - -Prefer `excluded_paths` over `excluded_crates`: a path entry exempts one module -and its descendants, whereas a crate entry exempts a whole compilation unit. -The application crate's module-scoped exemptions include -`netsuke::stdlib::which::lookup` (executable discovery through `PATH` and -cross-directory symlink canonicalization, which `cap_std` cannot express) and -`netsuke::runner::process::file_io::ambient_sync` (temporary-file -synchronization, scoped to the submodule holding only that `sync_all` so the -rest of `file_io` keeps writing through `cap_std` handles). Configuration -discovery otherwise uses capability-scoped canonicalization. Its small, -dedicated path-normalization module, `netsuke::cli::discovery::paths`, remains -narrowly excluded because `std::fs::canonicalize` preserves the absolute -comparison keys and cross-directory symlink behaviour that `cap_std` rejects. -For man-page generation, the build script compiles the `cli::build_support` -parser subset and deliberately omits runtime discovery. The broader -`netsuke::cli::discovery` module remains under the capability policy; no -`build_script_build` exception is required. The behavioural step definitions, -CLI integration tests, and shared workflow-reading helper that stage fixtures -ambiently are scoped the same way. A crate-level entry is justified only when -the ambient access lives in the crate root itself, where a path entry would be -no narrower — that covers the enumerated integration-test crates. The -`test_support` crate uses capability-backed fixture helpers and remains linted -by Whitaker under its own narrow policy. - -`test_support` is a workspace member, but the root Whitaker invocation selects -only the `netsuke-build` package (the Cargo package name behind the `netsuke` -targets; see ADR-007) and disables Dylint dependency checks. It therefore -compiles `test_support` as a dependency without applying the root -`dylint.toml`. Its one sanctioned ambient boundary is configured per crate. -Workspace membership makes Dylint discover the root configuration even when -launched from `test_support/`, so the scoped recipe supplies the contents of -`test_support/dylint.toml` explicitly through `DYLINT_TOML`. The second pass -also uses `--package test_support` and `--no-deps`, because running from a -member directory alone would otherwise check the parent workspace. That -configuration names only `test_support::fs` in `excluded_paths`. The root -`excluded_crates` must not contain `test_support`: every other module in the -crate remains subject to the filesystem policy. - -Permanent exceptions belong in `dylint.toml`, scoped as narrowly as the lint -allows. Do not use Rust `#[allow]` or `#[expect]` for `no_std_fs_operations`: -this Dylint lint is not known to `rustc`, so its exclusions must be configured -there. Prefer migrating to `cap_std` over any of these; reach for an exclusion -only when the operation is irreducibly ambient. - -To confirm the exclusions have not silently widened, add a temporary -`std::fs::metadata` call to an unexcluded module — for example -`src/stdlib/which/cache.rs`, a sibling of the excluded `lookup` module, or the -body of `src/runner/process/file_io.rs` outside `ambient_sync` — then run -`make lint-whitaker`. Both sites must still be reported; revert the probe -afterwards. The same check applies to `test_support`: a `std::fs` call in, say, -`test_support/src/exec.rs` must be reported even though `test_support::fs` is -exempt. - -When command output is long, preserve exit codes and logs: +See [Test execution](#test-execution) for what the checked-in nextest +configuration does and does not cover. + +`make lint` starts with workspace-wide rustdoc through +`RUSTDOCFLAGS="$(RUSTDOC_FLAGS)"` and +`RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)"`. This +denies warnings, enables Polonius, and preserves any `RUSTFLAGS` supplied by +the caller. It then runs workspace-wide +`cargo clippy --workspace --all-targets --all-features` and the +[Whitaker](whitaker-users-guide.md) Dylint suite +(`whitaker --all -- --all-targets --all-features`). Install Whitaker through +the standalone installer described in the +[Whitaker user's guide](whitaker-users-guide.md) so local linting matches +continuous integration (CI); `make lint-clippy` runs the Clippy-only subset. CI +pins the installer version in `WHITAKER_INSTALLER_VERSION` in +`.github/workflows/ci.yml`. Install that same version locally so local runs +match CI; read the pin from the workflow rather than copying the number, so the +two cannot drift: ```bash -set -o pipefail -make test 2>&1 | tee /tmp/netsuke-make-test.log -``` - -These gates always use the repository toolchain and the default codegen -backend. For a faster inner loop between gate runs, see -[local build acceleration](#local-build-acceleration). - -For documentation changes, also run `make fmt`, `make markdownlint`, and -`make nixie`. - +WHITAKER_INSTALLER_VERSION="$(sed -n \ + "s/.*WHITAKER_INSTALLER_VERSION: '\(.*\)'.*/\1/p" \ + .github/workflows/ci.yml)" +cargo install --locked whitaker-installer \ + --version "$WHITAKER_INSTALLER_VERSION" # or, for a prebuilt binary: cargo binstall --no-confirm --locked \ "whitaker-installer@$WHITAKER_INSTALLER_VERSION" @@ -2613,7 +2530,6 @@ file-layer branch, and applicable project-scope outcome without performing another environment lookup, filesystem scan, path normalization, or configuration-file load. - #### Cached discovery telemetry The full-merge boundary increments @@ -2634,9 +2550,9 @@ configuration values. Recorder-backed tests pin both outcomes through local state. Tracing never logs full paths or formatted parser errors. Path values are -bounded to a `path_hash` correlation identifier plus `path_file_name`, and load -failures are classified with the `ConfigLoadFailureKind` enum instead of the -formatted error text. `path_hash` is a bounded identifier for correlating +bounded to a `path_hash` correlation identifier plus a presence indicator, and +load failures are classified with the `ConfigLoadFailureKind` enum instead of +the formatted error text. `path_hash` is a bounded identifier for correlating events, not a cryptographic guarantee. #### `json` contract @@ -2874,7 +2790,7 @@ split diagnostics, path comparison, and tests out of the main discovery flow: - `discovery_event_assertions.rs` — shared test-only helpers: `capture_events` runs a closure under a TRACE capturing subscriber, `find_event` locates one emitted event by substring, and `EventAssertion` - bundles an event with its path to assert bounded `path_hash`/`path_file_name` + bundles an event with its path to assert bounded `path_hash` and presence fields, the absence of the raw path or formatted error text, and to normalize the hash before an `insta` snapshot. - `discovery_tracing_tests.rs` — tests selector precedence From 087be7bfdb816bf1182f4972bf3d1bf31d1eea9f Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 13:00:17 +0200 Subject: [PATCH 19/21] Update cached discovery documentation Align the developer and design guides with the current discovery outcome handoff, document that tracing omits raw filenames, and fix the nested eval sentence punctuation. --- docs/developers-guide.md | 15 ++++++++------- docs/netsuke-design.md | 8 ++++++-- docs/users-guide.md | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 0f8d71712..f34aa791a 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2386,8 +2386,9 @@ a two-pass approach when no explicit config path is provided: Because `MergeComposer` uses last-wins semantics, pushing the project layers after user layers gives them higher precedence. -Early JSON resolution reuses this logic through -`collect_diag_file_layers_with_sources`, before full configuration merging. +Early JSON resolution calls `collect_diag_file_layers_with_env`, which +delegates to `discover_file_layers` and returns a `DiscoveryOutcome` before +full configuration merging. ### Layer precedence @@ -2549,11 +2550,11 @@ configuration values. Recorder-backed tests pin both outcomes through local `metrics_util::DebuggingRecorder` instances without installing process-global state. -Tracing never logs full paths or formatted parser errors. Path values are -bounded to a `path_hash` correlation identifier plus a presence indicator, and -load failures are classified with the `ConfigLoadFailureKind` enum instead of -the formatted error text. `path_hash` is a bounded identifier for correlating -events, not a cryptographic guarantee. +Tracing never logs full paths, file names, or formatted parser errors. Path +values are bounded to a `path_hash` correlation identifier plus a presence +indicator. Load failures are classified with the `ConfigLoadFailureKind` enum +instead of the formatted error text. `path_hash` is a bounded identifier for +correlating events, not a cryptographic guarantee. #### `json` contract diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 3aa93bae0..a3c0dfb70 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2776,8 +2776,12 @@ flowchart LR Netsuke configuration discovery is implemented in `src/cli/discovery.rs`. Explicit file selection is handled by `resolve_config_selector(...)`, which applies the precedence `--config` > `NETSUKE_CONFIG`. Layer loading and -automatic discovery are handled by `push_file_layers_with_sources(...)`, which -also applies the `-C/--directory` flag as the project-discovery root. +automatic discovery are handled by `discover_file_layers(...)`, which honours +the `-C/--directory` project-discovery root and returns a `DiscoveryOutcome` +containing the loaded layers and deferred bounded diagnostics. Startup emits +those diagnostics after configuring its tracing filter, consumes +`into_layers()`, and passes the resulting layers to the full merge so discovery +and loading happen only once. **Figure: Explicit Config Selector Resolution** — This diagram shows how Netsuke chooses the configuration file before automatic discovery. Netsuke diff --git a/docs/users-guide.md b/docs/users-guide.md index cb42565c2..a78a75fc7 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1167,7 +1167,7 @@ Netsuke reduces some common quoting mistakes, but it is not a sandbox: structure. An entry may start at most one background job; Netsuke waits for that job before moving to a later entry, and rejects an entry that starts more than one background job during Ninja generation. It also rejects an - entry whose nested `eval` payload makes the background-job count dynamic, + entry whose nested `eval` payload makes the background-job count dynamic because the wrapper cannot safely determine which jobs to wait for. A direct simple `exec`, optionally prefixed by shell assignments, is supervised so its success or failure retains the list's status semantics: a successful From 654ec94e870e3f3ca8c34a9b7c5d1635f535d166 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 13:08:35 +0200 Subject: [PATCH 20/21] Refresh discovery helper documentation Describe the direct discovery outcome handoff and cached JSON preference after removing the diagnostic wrapper. --- docs/developers-guide.md | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index f34aa791a..0ed1c78a4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2386,9 +2386,9 @@ a two-pass approach when no explicit config path is provided: Because `MergeComposer` uses last-wins semantics, pushing the project layers after user layers gives them higher precedence. -Early JSON resolution calls `collect_diag_file_layers_with_env`, which -delegates to `discover_file_layers` and returns a `DiscoveryOutcome` before -full configuration merging. +Early JSON resolution calls `discover_file_layers`, which returns a +`DiscoveryOutcome` containing the cached layers, resolved JSON preference, and +deferred bounded diagnostics before full configuration merging. ### Layer precedence @@ -2422,19 +2422,17 @@ Configuration merge helpers: config selection from `--config` and `NETSUKE_CONFIG`. - `discover_file_layers(cli, env) -> DiscoveryOutcome` performs the single explicit-or-automatic discovery and load pass, retaining its layers, errors, - and deferred bounded diagnostics for the composition boundary. + resolved JSON preference, and deferred bounded diagnostics for the + composition boundary. - `push_discovered_file_layers(composer, errors, layers) -> ()` consumes the cached layers and discovery errors while composing the full configuration. -- `collect_diag_file_layers_with_env(cli, env) -> DiscoveryOutcome` preserves - the diagnostic collection span while routing discovery through the shared - cached outcome. - `collect_file_layers(directory)` builds the fallback discovery layer chain, applies the project-layer second pass, and returns `OrthoResult>>`. - `is_empty_value(value: &serde_json::Value) -> bool` detects an empty CLI override object. -- `json_from_layer(value: &serde_json::Value) -> Option` extracts `json` - from a configuration value. +- `json_from_value(value: &serde_json::Value) -> Option` extracts `json` + from a configuration value while discovery retains the resulting preference. - `json_from_matches(cli, matches, discovered) -> bool` applies an explicit root `--json` override to the discovered value. - `cli_overrides_from_matches(matches: &ArgMatches) -> OrthoValue` extracts @@ -2798,9 +2796,9 @@ split diagnostics, path comparison, and tests out of the main discovery flow: (`--config` versus `NETSUKE_CONFIG`), the removed legacy `NETSUKE_CONFIG_PATH` alias, and event-schema snapshots for both selection and explicit load failures. -- `discovery_layer_tests.rs` — tests the test-only - `collect_diag_file_layers_with_env` wrapper (explicit path versus automatic - discovery) and the project-scope second pass in `collect_file_layers`. +- `discovery_layer_tests.rs` — tests `discover_file_layers` directly for + explicit-path and automatic-discovery branches, plus the project-scope second + pass in `collect_file_layers`. Both test modules import `capture_events`, `find_event`, and `EventAssertion` from `discovery_event_assertions` rather than duplicating them. The `insta` From 733523da6dc45c6161e841d0db3aa829df88a3dc Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 16 Aug 2026 13:28:57 +0200 Subject: [PATCH 21/21] Tighten cached discovery reuse (#319) Resolve JSON while discovery transfers each loaded layer once, and prove the merge retains that cache after the selected file disappears. Remove obsolete build dependencies and harden isolated discovery test setup. --- Cargo.toml | 2 - src/cli/diag.rs | 37 ++------------- src/cli/discovery.rs | 77 +++++++++++++++++++++++--------- src/cli/discovery_json.rs | 35 +++++++++++++++ src/cli/discovery_layer_tests.rs | 30 ++++++++++--- tests/cli_tests/merge_diag.rs | 19 +++++--- tests/command_env_ui_tests.rs | 4 +- 7 files changed, 132 insertions(+), 72 deletions(-) create mode 100644 src/cli/discovery_json.rs diff --git a/Cargo.toml b/Cargo.toml index ca3bed727..2e39d0774 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -137,8 +137,6 @@ sys-locale = "0.3.2" cap-std = "3.4.4" clap = { version = "4.5.0", features = ["derive"] } clap_mangen = "0.3.0" -metrics = "0.24.6" -mockable = "3.0" ortho_config = { version = "0.9.0", features = ["serde_json"] } serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["preserve_order"] } diff --git a/src/cli/diag.rs b/src/cli/diag.rs index fdbbc80da..bfef47dd3 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -9,10 +9,9 @@ use clap::ArgMatches; use clap::parser::ValueSource; use mockable::{DefaultEnv, Env}; use ortho_config::{OrthoError, OrthoResult}; -use serde_json::Value; use std::sync::Arc; -use super::discovery::{DiscoveredLayers, DiscoveryOutcome, collect_diag_file_layers_with_env}; +use super::discovery::{DiscoveredLayers, DiscoveryOutcome, discover_file_layers}; use super::parser::Cli; const JSON_ENV_VAR: &str = "NETSUKE_JSON"; @@ -78,12 +77,12 @@ pub fn resolve_json_and_layers_outcome_with_env( matches: &ArgMatches, env: &impl Env, ) -> (OrthoResult, DiscoveryOutcome) { - let outcome = collect_diag_file_layers_with_env(cli, env); + let outcome = discover_file_layers(cli, env); let result = (|| { if let Some(error) = outcome.first_error() { return Err(Arc::clone(error)); } - let mut json = json_from_layers(outcome.layers()); + let mut json = outcome.json(); if !has_cli_json_override(matches) && let Some(env_json) = json_from_env(env)? { @@ -94,13 +93,6 @@ pub fn resolve_json_and_layers_outcome_with_env( (result, outcome) } -fn json_from_layer(value: &Value) -> Option { - value - .as_object() - .and_then(|map| map.get("json")) - .and_then(Value::as_bool) -} - /// Apply the command-line JSON override to a discovered preference. fn json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> bool { if has_cli_json_override(matches) { @@ -115,17 +107,6 @@ fn has_cli_json_override(matches: &ArgMatches) -> bool { matches.value_source("json") == Some(ValueSource::CommandLine) } -/// Resolve the last valid JSON preference from discovered config layers. -fn json_from_layers(layers: &[ortho_config::MergeLayer<'static>]) -> bool { - let mut json = Cli::default().json; - for layer in layers { - if let Some(layer_json) = json_from_layer(&layer.clone().into_value()) { - json = layer_json; - } - } - json -} - /// Parse the optional `NETSUKE_JSON` value supplied by `env`. /// /// Invalid or non-Unicode values are validation errors rather than silently @@ -163,20 +144,8 @@ mod tests { use cap_std::{ambient_authority, fs::Dir}; use clap::CommandFactory; use clap::Parser; - use serde_json::json; use tempfile::tempdir; - #[test] - fn json_from_layer_reads_json_bool() { - assert_eq!(json_from_layer(&json!({ "json": true })), Some(true)); - assert_eq!(json_from_layer(&json!({ "json": false })), Some(false)); - } - - #[test] - fn json_from_layer_ignores_non_bool_json() { - assert_eq!(json_from_layer(&json!({ "json": "yes" })), None); - } - #[test] fn resolve_merged_json_reads_injected_env() -> anyhow::Result<()> { let dir = tempdir()?; diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index 45b768adc..3053e4347 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -22,9 +22,13 @@ mod paths; #[path = "discovery_layers.rs"] mod layers; +#[path = "discovery_json.rs"] +mod json; + #[path = "discovery_trace.rs"] mod trace; use diagnostics::{BoundedConfigPath, ConfigLoadFailureKind, ConfigLoadWarning}; +use json::json_from_value; use layers::collect_file_layers_with_trace; use trace::{DiscoveryDiagnostics, DiscoveryTrace, FileLayerTrace}; @@ -32,16 +36,48 @@ const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; /// File layers and loading errors produced by one discovery pass. /// -/// The diagnostic pre-pass borrows the layers to resolve JSON output, then the -/// full merge consumes the same result. Keeping errors beside the layers lets -/// those phases retain their distinct error policies without rediscovery. +/// Discovery resolves JSON output while retaining the layers for full merging. +/// Keeping errors beside the layers lets those phases retain their distinct +/// error policies without rediscovery. pub struct DiscoveredLayers { layers: Vec>, errors: Vec>, + json: bool, } impl DiscoveredLayers { + /// Retain file layers and their resolved JSON preference without cloning values. + fn from_file_layers(file_layers: Vec>) -> Self { + let mut json = Cli::default().json; + let layers = file_layers + .into_iter() + .map(|layer| { + let path = layer.path().map(ToOwned::to_owned); + let value = layer.into_value(); + if let Some(layer_json) = json_from_value(&value) { + json = layer_json; + } + MergeLayer::file(Cow::Owned(value), path) + }) + .collect(); + Self { + layers, + errors: Vec::new(), + json, + } + } + + /// Retain a discovery error when loading yields no reusable layers. + fn from_error(error: Arc) -> Self { + Self { + layers: Vec::new(), + errors: vec![error], + json: Cli::default().json, + } + } + /// Borrow the file layers in discovery order. + #[cfg(test)] pub(crate) fn layers(&self) -> &[MergeLayer<'static>] { &self.layers } @@ -51,6 +87,11 @@ impl DiscoveredLayers { self.errors.first() } + /// Return the JSON preference resolved while retaining the loaded layers. + pub(crate) const fn json(&self) -> bool { + self.json + } + /// Consume the result into its reusable layers and deferred errors. pub(crate) fn into_parts( self, @@ -61,8 +102,7 @@ impl DiscoveredLayers { /// Layers and diagnostics returned by a side-effect-free discovery pass. /// -/// The diagnostic pre-pass reads the layers while retaining the bounded events -/// for a composition boundary to emit after it installs the tracing filter. +/// It retains bounded events so startup can emit them after installing its tracing filter. pub struct DiscoveryOutcome { layers: DiscoveredLayers, diagnostics: DiscoveryDiagnostics, @@ -70,10 +110,16 @@ pub struct DiscoveryOutcome { impl DiscoveryOutcome { /// Borrow file layers in discovery order. + #[cfg(test)] pub(crate) fn layers(&self) -> &[MergeLayer<'static>] { self.layers.layers() } + /// Return the JSON preference cached with the discovered file layers. + pub(crate) const fn json(&self) -> bool { + self.layers.json() + } + /// Borrow the first discovery error, if loading failed. pub(crate) fn first_error(&self) -> Option<&Arc> { self.layers.first_error() @@ -95,14 +141,8 @@ impl DiscoveryOutcome { pub(crate) fn discover_file_layers(cli: &Cli, env: &impl Env) -> DiscoveryOutcome { let (trace, load_warning, outcome) = collect_file_layers_with_env(cli, env); let layers = match outcome { - Ok(layers) => DiscoveredLayers { - layers, - errors: Vec::new(), - }, - Err(error) => DiscoveredLayers { - layers: Vec::new(), - errors: vec![error], - }, + Ok(layers) => DiscoveredLayers::from_file_layers(layers), + Err(error) => DiscoveredLayers::from_error(error), }; DiscoveryOutcome { layers, @@ -257,13 +297,6 @@ fn load_layers_from_path_with_warning( } } -/// Load file layers for early JSON resolution using injected environment access. -/// -/// This delegates to the same precedence boundary as the normal merge path. -pub(crate) fn collect_diag_file_layers_with_env(cli: &Cli, env: &impl Env) -> DiscoveryOutcome { - discover_file_layers(cli, env) -} - #[cfg(test)] #[path = "discovery_event_assertions.rs"] mod event_assertions; @@ -338,14 +371,14 @@ mod tests { } #[test] - fn collect_diag_file_layers_uses_injected_explicit_config() -> anyhow::Result<()> { + fn discover_file_layers_uses_injected_explicit_config() -> anyhow::Result<()> { let dir = tempdir()?; let config_path = dir.path().join("netsuke.toml"); let config_dir = Dir::open_ambient_dir(dir.path(), ambient_authority())?; config_dir.write("netsuke.toml", b"json = true\n")?; let env = mock_env_with([(CONFIG_ENV_VAR, config_path.as_os_str().to_owned())]); - let layers = collect_diag_file_layers_with_env(&Cli::default(), &env); + let layers = discover_file_layers(&Cli::default(), &env); let expected_path = config_path.to_string_lossy().into_owned(); ensure!( diff --git a/src/cli/discovery_json.rs b/src/cli/discovery_json.rs new file mode 100644 index 000000000..3f42185ad --- /dev/null +++ b/src/cli/discovery_json.rs @@ -0,0 +1,35 @@ +//! JSON-preference extraction from discovered configuration values. + +use serde_json::Value; + +/// Read an optional JSON preference from one configuration value. +pub(super) fn json_from_value(value: &Value) -> Option { + value + .as_object() + .and_then(|map| map.get("json")) + .and_then(Value::as_bool) +} + +#[cfg(test)] +mod tests { + //! Tests for JSON-preference extraction from configuration values. + + use super::*; + + #[test] + fn reads_json_bool() { + assert_eq!( + json_from_value(&serde_json::json!({ "json": true })), + Some(true) + ); + assert_eq!( + json_from_value(&serde_json::json!({ "json": false })), + Some(false) + ); + } + + #[test] + fn ignores_non_bool_json() { + assert_eq!(json_from_value(&serde_json::json!({ "json": "yes" })), None); + } +} diff --git a/src/cli/discovery_layer_tests.rs b/src/cli/discovery_layer_tests.rs index 71b14f904..1987e2636 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -51,7 +51,7 @@ fn scenario_cli(scenario: LayerScenario, temp: &TempDir) -> Result { #[rstest] #[case::explicit_config_path(LayerScenario::ExplicitConfig, false, "using explicit config path")] #[case::isolated_directory_discovery(LayerScenario::Discovery, true, "using config discovery")] -fn collect_diag_file_layers_logs_selected_branch( +fn discover_file_layers_logs_selected_branch( #[case] scenario: LayerScenario, #[case] should_be_empty: bool, #[case] expected_event: &str, @@ -60,7 +60,7 @@ fn collect_diag_file_layers_logs_selected_branch( let cli = scenario_cli(scenario, &temp)?; let env = empty_mock_env(); - let discovered = collect_diag_file_layers_with_env(&cli, &env); + let discovered = discover_file_layers(&cli, &env); let ((), events) = capture_events(|| { discovered.emit_diagnostics(); Ok::<_, anyhow::Error>(()) @@ -340,10 +340,19 @@ fn discover_file_layers_supports_discovery_without_a_selector() -> Result<()> { }; let discovered = discover_file_layers(&cli, &empty_mock_env()); + let project_layers = discovered + .layers() + .iter() + .filter(|layer| { + layer + .path() + .is_some_and(|path| path.as_str().ends_with(".netsuke.toml")) + }) + .collect::>(); ensure!( - discovered.layers().is_empty(), - "an empty directory should not produce config layers" + project_layers.is_empty(), + "an empty directory should not produce project config layers: {project_layers:?}" ); ensure!( discovered.first_error().is_none(), @@ -363,10 +372,19 @@ fn discover_file_layers_performs_the_project_scope_second_pass() -> Result<()> { }; let discovered = discover_file_layers(&cli, &empty_mock_env()); + let project_layers = discovered + .layers() + .iter() + .filter(|layer| { + layer + .path() + .is_some_and(|path| path.as_str().ends_with(".netsuke.toml")) + }) + .collect::>(); ensure!( - discovered.layers().len() == 1, - "the project pass should discover its config layer" + project_layers.len() == 1, + "the project pass should discover one config layer: {project_layers:?}" ); ensure!( discovered.first_error().is_none(), diff --git a/tests/cli_tests/merge_diag.rs b/tests/cli_tests/merge_diag.rs index 2f1fb237f..3c2036c62 100644 --- a/tests/cli_tests/merge_diag.rs +++ b/tests/cli_tests/merge_diag.rs @@ -97,13 +97,18 @@ fn diag_and_merge_reuse_one_discovery_result() -> Result<()> { .return_const(None::); env.expect_all().once().return_const(HashMap::new()); - let (is_json, layers) = netsuke::cli::resolve_json_and_layers_with_env(&cli, &matches, &env) - .context("resolve diagnostic mode and discovered layers")?; - let (merge_result, snapshot) = recorded(|| { - netsuke::cli::merge_with_layers(&cli, &matches, &env, layers) - .context("merge the cached layers") + let (result, snapshot) = recorded(|| { + let (is_json, layers) = + netsuke::cli::resolve_json_and_layers_with_env(&cli, &matches, &env) + .context("resolve diagnostic mode and discovered layers")?; + config_dir + .remove_file("netsuke.toml") + .context("remove config after discovery")?; + let merged = netsuke::cli::merge_with_layers(&cli, &matches, &env, layers) + .context("merge the cached layers")?; + Ok::<_, anyhow::Error>((is_json, merged)) }); - let merged = merge_result?; + let (is_json, merged) = result?; ensure!( is_json, @@ -111,7 +116,7 @@ fn diag_and_merge_reuse_one_discovery_result() -> Result<()> { ); ensure!( merged.jobs == Some(13), - "the merge should consume the discovered config layer" + "the merge should consume the cached config layer after its file is removed" ); ensure!( cache_outcomes(&snapshot) == vec![("reused".to_owned(), 1)], diff --git a/tests/command_env_ui_tests.rs b/tests/command_env_ui_tests.rs index f480bd307..3b6315236 100644 --- a/tests/command_env_ui_tests.rs +++ b/tests/command_env_ui_tests.rs @@ -41,7 +41,9 @@ fn command_env_embedder_fixture_compiles() -> io::Result<()> { /// The CLI configuration fixture type-checks against the public cache API. #[test] fn cli_configuration_fixture_compiles() -> io::Result<()> { - let temporary_root = tempfile::tempdir_in(manifest_dir().join("target"))?; + let target_dir = manifest_dir().join("target"); + test_fs::create_dir_all(&target_dir)?; + let temporary_root = tempfile::tempdir_in(target_dir)?; let fixture_dir = temporary_root.path().join("cli_configuration_pass"); test_fs::create_dir_all(fixture_dir.join("src"))?; test_fs::copy(