diff --git a/build.rs b/build.rs index d47ab917..68e6711c 100644 --- a/build.rs +++ b/build.rs @@ -49,6 +49,8 @@ type ThemeContextCtor = fn( output_mode::OutputMode, ) -> theme::ThemeContext; type ResolveMergedDiagJsonFn = fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult; +type ResolveMergedDiagJsonWithEnvFn = + fn(&cli::Cli, &ArgMatches, &cli::ConfigStdEnvProvider) -> ortho_config::OrthoResult; type MergeWithConfigFn = fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult; /// Anchors all shared-module symbols so they remain linked when the build script is compiled @@ -58,7 +60,9 @@ const fn assert_symbols_linked() { const _: fn(&[OsString]) -> Option = cli::locale_hint_from_args; const _: fn(&[OsString]) -> Option = cli::diag_json_hint_from_args; const _: fn(&str) -> Option = cli_l10n::parse_bool_hint; + const _: usize = std::mem::size_of::<&dyn cli::ConfigEnvProvider>(); const _: ResolveMergedDiagJsonFn = cli::resolve_merged_diag_json; + const _: ResolveMergedDiagJsonWithEnvFn = cli::resolve_merged_diag_json_with_env; const _: MergeWithConfigFn = cli::merge_with_config; const _: LocalizedParseFn = cli::parse_with_localizer_from; const _: fn(&cli::Cli) -> cli::config::CliConfig = cli::Cli::config; diff --git a/docs/developers-guide.md b/docs/developers-guide.md index bf1ec4a3..2fa96fed 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -483,30 +483,47 @@ Table: Configuration merge helper functions ### Environment lookup seams -`resolve_config_path` is the crate-internal seam for explicit config-file -selection. It accepts a `var_os` closure with this shape: +`EnvProvider` is the crate-internal port for raw environment access during CLI +configuration resolution. The production adapter delegates to +`std::env::var_os`; unit tests use map-backed providers so they do not mutate +process-wide environment variables when exercising explicit config selection +or early diagnostic-mode resolution. ```rust -Fn(&str) -> Option +pub trait EnvProvider { + fn get(&self, key: &str) -> Option; +} ``` -Production callers pass `std::env::var_os`, while unit tests pass a -`HashMap`-backed closure. This keeps deterministic tests for `NETSUKE_CONFIG` -and `NETSUKE_CONFIG_PATH` without threading an environment adapter through the -public merge API. +`resolve_config_path`, `env_config_path`, `collect_diag_file_layers`, and the +provider-injected diagnostic resolver all accept this port. This keeps +deterministic tests for `NETSUKE_CONFIG`, `NETSUKE_CONFIG_PATH`, and +`NETSUKE_DIAG_JSON` without coupling those tests to the global process +environment. + +Discovery tests that exercise OrthoConfig's `ConfigDiscovery` may still need +`EnvLock` because the external discovery implementation reads platform +environment variables directly. Tests for Netsuke's own environment port should +avoid `EnvLock`. `collect_diag_file_layers` and `push_file_layers` call `resolve_config_path` -with `std::env::var_os`, so both early diagnostic resolution and the full merge -path use the same explicit config selector precedence. The public API remains -two arguments: +with the same provider, so both early diagnostic resolution and the full merge +path use the same explicit config selector precedence. The production public +API remains two arguments, with an injected variant for tests and composition +code: ```rust pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult; pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult; +pub fn resolve_merged_diag_json_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl EnvProvider, +) -> OrthoResult; ``` Unit tests that only need to verify explicit config path precedence should test -`resolve_config_path` with an injected closure instead of mutating the process +`resolve_config_path` with an injected provider instead of mutating the process environment. #### `diag_json` contract diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 953f17c1..d46a5f4d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2686,6 +2686,11 @@ manual flag repetition. `resolve_config_path()`, calls `push_file_layers()` to load layers, merges them with defaults, adds environment variables via Figment, and finally applies CLI overrides extracted from `ArgMatches`. +- Netsuke-owned environment reads for explicit config selection and early + diagnostic-JSON resolution go through the `EnvProvider` port. Production code + uses `StdEnvProvider`; tests can inject a map-backed provider instead of + mutating the process environment. 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. diff --git a/src/cli/config_merge.rs b/src/cli/config_merge.rs index 37ed776d..de951007 100644 --- a/src/cli/config_merge.rs +++ b/src/cli/config_merge.rs @@ -13,6 +13,7 @@ use ortho_config::{ OrthoResult, load_config_file_as_chain, sanitize_value, }; use std::borrow::Cow; +use std::ffi::OsString; use std::io; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -41,11 +42,27 @@ fn config_discovery(directory: Option<&Path>) -> ConfigDiscovery { builder.build() } -fn env_config_path(var_os: F, var_name: &str) -> Option -where - F: Fn(&str) -> Option, -{ - var_os(var_name) +/// Read-only environment access for CLI configuration resolution. +/// +/// This port keeps process-wide environment reads at the composition boundary +/// so config resolution tests can inject deterministic values. +pub trait EnvProvider { + /// Return the raw environment value for `key`. + fn get(&self, key: &str) -> Option; +} + +/// Environment provider backed by the current process environment. +#[derive(Debug, Default, Copy, Clone)] +pub struct StdEnvProvider; + +impl EnvProvider for StdEnvProvider { + fn get(&self, key: &str) -> Option { + std::env::var_os(key) + } +} + +fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option { + env.get(var_name) .filter(|value| !value.is_empty()) .map(PathBuf::from) } @@ -54,15 +71,12 @@ where clippy::option_as_ref_cloned, reason = "make cloning the selected config path explicit" )] -fn resolve_config_path(cli: &Cli, var_os: F) -> Option -where - F: Fn(&str) -> Option, -{ +fn resolve_config_path(cli: &Cli, env: &impl EnvProvider) -> Option { cli.config .as_ref() .cloned() - .or_else(|| env_config_path(&var_os, CONFIG_ENV_VAR)) - .or_else(|| env_config_path(var_os, CONFIG_ENV_VAR_LEGACY)) + .or_else(|| env_config_path(env, CONFIG_ENV_VAR)) + .or_else(|| env_config_path(env, CONFIG_ENV_VAR_LEGACY)) } fn load_layers_from_path(path: &Path) -> OrthoResult>> { @@ -147,13 +161,23 @@ fn diag_json_from_layer(value: &serde_json::Value) -> Option { /// An explicit config path returns the selected file's layers and propagates /// load failures so startup diagnostic-mode selection reports the same /// configuration errors as the full merge path. -fn collect_diag_file_layers(cli: &Cli) -> OrthoResult>> { - if let Some(path) = resolve_config_path(cli, |name| std::env::var_os(name)) { +fn collect_diag_file_layers( + cli: &Cli, + env: &impl EnvProvider, +) -> OrthoResult>> { + if let Some(path) = resolve_config_path(cli, env) { return load_layers_from_path(&path); } let discovery = config_discovery(cli.directory.as_deref()); - let file_layers = discovery.compose_layers().value; + let mut composition = discovery.compose_layers(); + if !composition.required_errors.is_empty() { + return Err(composition.required_errors.remove(0)); + } + if composition.value.is_empty() && !composition.optional_errors.is_empty() { + return Err(composition.optional_errors.remove(0)); + } + let file_layers = composition.value; let project_file = project_scope_file_str(cli.directory.as_deref()); let first_pass_found_project = file_layers.iter().any(|l| { l.path() @@ -179,6 +203,29 @@ fn diag_json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> } } +fn env_diag_json(env: &impl EnvProvider) -> OrthoResult> { + let Some(value) = env.get("NETSUKE_DIAG_JSON") else { + return Ok(None); + }; + let raw = value.into_string().map_err(|invalid_value| { + Arc::new(OrthoError::Validation { + key: String::from("NETSUKE_DIAG_JSON"), + message: format!( + "NETSUKE_DIAG_JSON must be valid Unicode, got {:?}", + invalid_value.to_string_lossy() + ), + }) + })?; + match raw.as_str() { + "true" | "1" => Ok(Some(true)), + "false" | "0" => Ok(Some(false)), + _ => Err(Arc::new(OrthoError::Validation { + key: String::from("NETSUKE_DIAG_JSON"), + message: format!("NETSUKE_DIAG_JSON must be true, false, 1, or 0; got {raw:?}"), + })), + } +} + /// Resolve the effective diagnostic JSON preference from the raw config layers. /// /// This is used before full config merging so startup and merge-time failures @@ -191,9 +238,27 @@ fn diag_json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> /// configuration layer required for diagnostic-mode resolution cannot be /// loaded. pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult { + resolve_merged_diag_json_with_env(cli, matches, &StdEnvProvider) +} + +/// Resolve the effective diagnostic JSON preference using injected environment +/// access. +/// +/// This variant is intended for tests and composition code that must avoid +/// process-wide environment reads. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] when a required configuration layer +/// cannot be loaded, or when `NETSUKE_DIAG_JSON` contains an invalid boolean. +pub fn resolve_merged_diag_json_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl EnvProvider, +) -> OrthoResult { let mut diag_json = Cli::default().diag_json; - let layers = collect_diag_file_layers(cli)?; + let layers = collect_diag_file_layers(cli, env)?; for layer in layers { let layer_value = layer.into_value(); if let Some(layer_diag_json) = diag_json_from_layer(&layer_value) { @@ -201,12 +266,7 @@ pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult< } } - let env_provider = env_provider() - .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) - .split("__"); - if let Ok(value) = Figment::from(env_provider).extract::() - && let Some(env_diag_json) = diag_json_from_layer(&value) - { + if let Some(env_diag_json) = env_diag_json(env)? { diag_json = env_diag_json; } @@ -241,8 +301,9 @@ fn push_file_layers( composer: &mut MergeComposer, errors: &mut Vec>, cli: &Cli, + env: &impl EnvProvider, ) { - if let Some(path) = resolve_config_path(cli, |name| std::env::var_os(name)) { + if let Some(path) = resolve_config_path(cli, env) { push_layers_result(composer, errors, load_layers_from_path(&path)); return; } @@ -327,6 +388,14 @@ fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult OrthoResult { + merge_with_config_with_env(cli, matches, &StdEnvProvider) +} + +fn merge_with_config_with_env( + cli: &Cli, + matches: &ArgMatches, + env: &impl EnvProvider, +) -> OrthoResult { let command = cli.command.clone(); let mut errors = Vec::new(); let mut composer = MergeComposer::with_capacity(4); @@ -336,7 +405,7 @@ pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { Err(err) => errors.push(err), } - push_file_layers(&mut composer, &mut errors, cli); + push_file_layers(&mut composer, &mut errors, cli, env); let env_provider = env_provider() .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) diff --git a/src/cli/config_merge_tests.rs b/src/cli/config_merge_tests.rs index 6089d485..1daca4fc 100644 --- a/src/cli/config_merge_tests.rs +++ b/src/cli/config_merge_tests.rs @@ -3,7 +3,7 @@ use super::*; use anyhow::ensure; use clap::CommandFactory; -use rstest::{fixture, rstest}; +use rstest::rstest; use serde_json::json; use std::collections::HashMap; use std::ffi::OsString; @@ -27,9 +27,11 @@ impl TestEnv { self.values.insert(name, value.into()); self } +} - fn var_os(&self, name: &str) -> Option { - self.values.get(name).cloned() +impl EnvProvider for TestEnv { + fn get(&self, key: &str) -> Option { + self.values.get(key).cloned() } } @@ -177,75 +179,23 @@ fn project_scope_layers_returns_one_layer_when_file_present() { // collect_diag_file_layers // --------------------------------------------------------------------------- -/// Fixture that sets up isolated config environment for testing config discovery. -/// -/// Returns (`EnvLock`, `project_dir`, `fake_home`, `env_guards`) where `env_guards` -/// isolate `HOME` and platform-specific config paths (`XDG_CONFIG_HOME` on Unix, -/// `APPDATA`/`LOCALAPPDATA` on Windows) and remove explicit config selectors. -#[cfg(test)] -#[fixture] -fn isolated_config_env() -> anyhow::Result<( - test_support::env_lock::EnvLock, - tempfile::TempDir, - tempfile::TempDir, - Vec, -)> { - use test_support::env_lock::EnvLock; - let lock = EnvLock::acquire(); - let dir = tempfile::tempdir()?; - let fake_home = tempfile::tempdir()?; - - #[cfg(unix)] - let guards = vec![ - EnvVarGuard::set("HOME", fake_home.path().as_os_str()), - EnvVarGuard::set("XDG_CONFIG_HOME", fake_home.path().as_os_str()), - EnvVarGuard::remove(CONFIG_ENV_VAR), - EnvVarGuard::remove(CONFIG_ENV_VAR_LEGACY), - ]; - - #[cfg(windows)] - let guards = vec![ - EnvVarGuard::set("HOME", fake_home.path().as_os_str()), - EnvVarGuard::set("APPDATA", fake_home.path().as_os_str()), - EnvVarGuard::set("LOCALAPPDATA", fake_home.path().as_os_str()), - EnvVarGuard::remove(CONFIG_ENV_VAR), - EnvVarGuard::remove(CONFIG_ENV_VAR_LEGACY), - ]; - - Ok((lock, dir, fake_home, guards)) -} - #[rstest] -#[case::no_file(false, true)] -#[case::file_present(true, false)] -fn collect_diag_file_layers_handles_project_file_presence( - isolated_config_env: anyhow::Result<( - test_support::env_lock::EnvLock, - tempfile::TempDir, - tempfile::TempDir, - Vec, - )>, - #[case] create_file: bool, - #[case] expect_empty: bool, +#[case::primary_env(CONFIG_ENV_VAR)] +#[case::legacy_env(CONFIG_ENV_VAR_LEGACY)] +fn collect_diag_file_layers_uses_injected_explicit_config( + #[case] config_var: &'static str, ) -> anyhow::Result<()> { - let (_lock, dir, _fake_home, _guards) = isolated_config_env?; + let dir = tempdir()?; + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, r"diag_json = true")?; - if create_file { - std::fs::write(dir.path().join(".netsuke.toml"), r"diag_json = true") - .expect("write config"); - } + let env = TestEnv::default().with_var(config_var, config_path.as_os_str()); + let layers = collect_diag_file_layers(&Cli::default(), &env)?; - let cli = cli_with_directory(dir.path()); - let layers = collect_diag_file_layers(&cli)?; - - if expect_empty { - ensure!(layers.is_empty(), "expected no layers when file absent"); - } else { - ensure!( - !layers.is_empty(), - "should include the project config layer when file present" - ); - } + ensure!( + !layers.is_empty(), + "should include the injected explicit config layer" + ); Ok(()) } @@ -269,6 +219,48 @@ fn diag_json_from_matches_returns_discovered_when_no_cli_flag_set() { ); } +#[test] +fn resolve_merged_diag_json_reads_injected_env() -> anyhow::Result<()> { + let dir = tempdir()?; + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "")?; + let app = Cli::command(); + let matches = app.get_matches_from(["netsuke"]); + let cli = Cli { + config: Some(config_path), + ..Cli::default() + }; + let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "true"); + + ensure!( + resolve_merged_diag_json_with_env(&cli, &matches, &env)?, + "injected env should enable diagnostic JSON" + ); + + Ok(()) +} + +#[test] +fn resolve_merged_diag_json_rejects_malformed_injected_env() { + let dir = tempdir().expect("tempdir"); + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "").expect("write config"); + let app = Cli::command(); + let matches = app.get_matches_from(["netsuke"]); + let cli = Cli { + config: Some(config_path), + ..Cli::default() + }; + let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "yes"); + + let error = resolve_merged_diag_json_with_env(&cli, &matches, &env) + .expect_err("invalid diagnostic JSON env value should fail"); + assert!( + matches!(error.as_ref(), OrthoError::Validation { key, .. } if key == "NETSUKE_DIAG_JSON"), + "expected validation error for NETSUKE_DIAG_JSON, got {error:?}" + ); +} + // --------------------------------------------------------------------------- // push_file_layers // --------------------------------------------------------------------------- @@ -300,7 +292,7 @@ fn push_file_layers_pushes_expected_layer_count( let _config_guard = EnvVarGuard::remove(CONFIG_ENV_VAR); let _legacy_config_guard = EnvVarGuard::remove(CONFIG_ENV_VAR_LEGACY); let cli = cli_with_directory(dir.path()); - push_file_layers(&mut composer, &mut errors, &cli); + push_file_layers(&mut composer, &mut errors, &cli, &StdEnvProvider); assert!(errors.is_empty(), "no required errors expected"); assert_eq!( composer.layers().len(), @@ -316,19 +308,19 @@ fn push_file_layers_pushes_expected_layer_count( #[test] fn env_config_path_returns_none_when_var_unset() { let env = TestEnv::default(); - assert!(env_config_path(|name| env.var_os(name), "__NETSUKE_TEST_VAR").is_none()); + 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", ""); - assert!(env_config_path(|name| env.var_os(name), "__NETSUKE_TEST_VAR").is_none()); + 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 result = env_config_path(|name| env.var_os(name), "__NETSUKE_TEST_VAR"); + let result = env_config_path(&env, "__NETSUKE_TEST_VAR"); assert_eq!(result, Some(std::path::PathBuf::from("/tmp/foo.toml"))); } @@ -379,7 +371,7 @@ fn resolve_config_path_precedence( ..Cli::default() }; assert_eq!( - resolve_config_path(&cli, |name| env.var_os(name)), + resolve_config_path(&cli, &env), expected.map(std::path::PathBuf::from), "{msg}", ); @@ -389,7 +381,7 @@ fn resolve_config_path_precedence( fn resolve_config_path_returns_none_when_all_absent() { let cli = Cli::default(); assert!( - resolve_config_path(&cli, |name| TestEnv::default().var_os(name)).is_none(), + resolve_config_path(&cli, &TestEnv::default()).is_none(), "should return None when no selector is active" ); } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 7553f245..64a8bf62 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -19,11 +19,15 @@ mod config_merge; mod parsing; mod validation; +/// Read-only environment access for CLI config resolution. +pub use config_merge::EnvProvider as ConfigEnvProvider; +/// Environment provider backed by the process environment for CLI config resolution. +pub use config_merge::StdEnvProvider as ConfigStdEnvProvider; use config_merge::default_manifest_path; /// Merge a parsed CLI struct with layered configuration files. pub use config_merge::merge_with_config; /// Resolve whether JSON diagnostics should be active after config discovery. -pub use config_merge::resolve_merged_diag_json; +pub use config_merge::{resolve_merged_diag_json, resolve_merged_diag_json_with_env}; use validation::configure_validation_parsers; /// Maximum number of jobs accepted by the CLI. diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 1b769a29..4faa9db0 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -16,12 +16,32 @@ use netsuke::theme::ThemePreference; use ortho_config::{MergeComposer, sanitize_value}; use rstest::{fixture, rstest}; use serde_json::json; +use std::collections::HashMap; use std::ffi::OsStr; +use std::ffi::OsString; use std::fs; use std::sync::Arc; use tempfile::tempdir; use test_support::{EnvVarGuard, env_lock::EnvLock}; +#[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() + } +} + #[fixture] fn default_cli_json() -> Result { Ok(sanitize_value(&Cli::default())?) @@ -127,6 +147,27 @@ fn cli_merge_layers_prefers_cli_then_env_then_file_for_locale( Ok(()) } +#[rstest] +fn resolve_merged_diag_json_honours_injected_env() -> Result<()> { + let temp_dir = tempdir().context("create temporary config directory")?; + let config_path = temp_dir.path().join("netsuke.toml"); + fs::write(&config_path, "diag_json = false\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 args for injected diag_json env")?; + let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "1"); + + ensure!( + netsuke::cli::resolve_merged_diag_json_with_env(&cli, &matches, &env)?, + "injected NETSUKE_DIAG_JSON should override file config", + ); + + Ok(()) +} + #[cfg(unix)] #[rstest] fn resolve_merged_diag_json_handles_malformed_project_config(