From 91268f7af3481bb3441e384390920c559eb643b5 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 1 Jun 2026 17:57:24 +0200 Subject: [PATCH 1/7] Inject config env access (#293) Introduce an `EnvProvider` port for CLI configuration resolution. Thread it through explicit config selection, diagnostic layer collection, and early diagnostic JSON resolution so tests can use deterministic environment doubles instead of process-wide mutation. Reject malformed `NETSUKE_DIAG_JSON` values with a validation error, and keep discovery load errors visible while resolving startup diagnostic mode. --- build.rs | 8 +- docs/developers-guide.md | 105 ++++++++++++------------ docs/netsuke-design.md | 10 +++ src/cli/diag.rs | 156 ++++++++++++++++++++++++++++++------ src/cli/discovery.rs | 167 +++++++++++++++++++++++++++++++++++++-- src/cli/mod.rs | 3 +- src/main.rs | 26 +++++- tests/cli_tests/merge.rs | 43 +++++++++- 8 files changed, 430 insertions(+), 88 deletions(-) diff --git a/build.rs b/build.rs index 1b4f1892..7eea3451 100644 --- a/build.rs +++ b/build.rs @@ -61,6 +61,9 @@ type ResolveThemeFn = fn( fn(&str) -> Option, ) -> theme::ResolvedTheme; +type ResolveMergedDiagJsonWithEnvFn = + fn(&cli::Cli, &ArgMatches, &cli::ConfigStdEnvProvider) -> ortho_config::OrthoResult; + fn manual_date() -> String { let Ok(raw) = env::var("SOURCE_DATE_EPOCH") else { return FALLBACK_DATE.into(); @@ -119,7 +122,10 @@ const fn verify_public_api_symbols() { 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 _: fn(&cli::Cli, &ArgMatches) -> bool = cli::resolve_merged_diag_json; + const _: usize = std::mem::size_of::<&dyn cli::ConfigEnvProvider>(); + const _: fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult = + cli::resolve_merged_diag_json; + const _: ResolveMergedDiagJsonWithEnvFn = cli::resolve_merged_diag_json_with_env; const _: fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult = cli::merge_with_config; const _: LocalizedParseFn = cli::parse_with_localizer_from; diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 2293f8ee..3112a782 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -578,8 +578,8 @@ 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. -The same logic is mirrored in `collect_diag_file_layers` for early `diag_json` -resolution (before full merging). +The same logic is mirrored in `collect_diag_file_layers_with_env` for early +`diag_json` resolution (before full merging). ### Layer precedence @@ -595,75 +595,70 @@ The final merge order is: Private helper functions for config discovery and diagnostic-JSON resolution. -Configuration merge helpers: - -- `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, - bypassing automatic discovery, and returns - `OrthoResult>>`. -- `env_config_path(var_name: &str) -> Option` reads one config - environment variable, ignores empty values, and converts the value into a - `PathBuf`. -- `explicit_config_path(cli: &Cli) -> Option` resolves explicit config - selection from `--config`, `NETSUKE_CONFIG`, and `NETSUKE_CONFIG_PATH`. -- `push_file_layers(cli, composer, errors) -> ()` 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_file_layers(directory)` builds the fallback discovery layer chain, - applies the project-layer second pass, and returns - `OrthoResult>>`. -- `collect_diag_file_layers(cli: &Cli) -> OrthoResult>>` - mirrors the file-load path used by `resolve_merged_diag_json` so early - `diag_json` resolution sees the same explicit or discovered file layers. -- `is_empty_value(value: &serde_json::Value) -> bool` detects an empty CLI - override object. -- `diag_json_from_layer(layer: &MergeLayer) -> Option` extracts - `diag_json` from a config layer, preferring `output_format`. -- `diag_json_from_matches(cli, matches) -> OrthoResult` resolves final - `diag_json` from CLI matches with fallback. -- `cli_overrides_from_matches(matches: &ArgMatches) -> OrthoValue` extracts - CLI-supplied fields, stripping defaults and non-CLI sources. -- `env_provider() -> Figment` returns the `NETSUKE_` prefixed Figment - environment provider. +Table: Configuration merge helper functions + +| Function | Purpose | +| :---------------------------------- | :------------------------------------------------------------------- | +| `config_discovery` | Build single-pass `ConfigDiscovery` with optional directory anchor. | +| `project_scope_file_str` | Resolve the expected project `.netsuke.toml` path as a string. | +| `project_scope_layers` | Load project-scope config directly, bypassing discovery. | +| `push_file_layers` | Push all file layers onto a `MergeComposer` in precedence order. | +| `collect_diag_file_layers_with_env` | Mirror of `push_file_layers` for early `diag_json` resolution. | +| `is_empty_value` | Return `true` for an empty JSON object (no CLI overrides). | +| `diag_json_from_layer` | Extract `diag_json` from a config layer. | +| `diag_json_from_matches` | Resolve final `diag_json` from CLI matches with fallback. | +| `cli_overrides_from_matches` | Extract CLI-supplied fields, stripping defaults and non-CLI sources. | +| `env_provider` | Return the `NETSUKE_` prefixed Figment environment provider. | ### Environment lookup seams -`explicit_config_path` is the crate-internal seam for explicit config-file -selection. It evaluates the precedence chain in this order: +`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. -1. `cli.config` -2. `NETSUKE_CONFIG` -3. `NETSUKE_CONFIG_PATH` +```rust +pub trait EnvProvider { + fn get(&self, key: &str) -> Option; +} +``` + +`explicit_config_path_with_env`, `collect_diag_file_layers_with_env`, 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. -`env_config_path(var_name)` is the helper that reads `std::env::var_os`, -discarding empty values and converting the value into `PathBuf`. For `merge` and -`resolve_merged_diag_json`, both `collect_diag_file_layers` and -`push_file_layers` therefore follow the same precedence by calling -`explicit_config_path`, so they return either the same explicit path layers or -the same fallback discovery path. +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`. -The public API remains two arguments: +`collect_diag_file_layers_with_env` and `push_file_layers_with_env` call +`explicit_config_path_with_env` 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 need to verify explicit config path precedence should exercise -the public merge or diagnostic APIs with guarded environment variables. The -explicit selector helper reads the process environment directly and remains -private to `src/cli/discovery.rs`. +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 +the process environment. #### `diag_json` contract Tooling that wants a stable contract for early diagnostic-JSON resolution -should treat the input consumed by `collect_diag_file_layers`, +should treat the input consumed by `collect_diag_file_layers_with_env`, `diag_json_from_layer`, and `diag_json_from_matches` as versioned schema `netsuke.diag-json-resolution.v1`: diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 7706a69b..681d944d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2702,6 +2702,16 @@ 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 without + further customisation 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 + diagnostic-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. + 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/diag.rs b/src/cli/diag.rs index 204317d1..2c0ec1f5 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -7,13 +7,11 @@ use clap::ArgMatches; use clap::parser::ValueSource; -use ortho_config::OrthoResult; -use ortho_config::figment::Figment; -use ortho_config::uncased::Uncased; +use ortho_config::{OrthoError, OrthoResult}; use serde_json::Value; +use std::sync::Arc; -use super::discovery::collect_diag_file_layers; -use super::merge::env_provider; +use super::discovery::{EnvProvider, StdEnvProvider, collect_diag_file_layers_with_env}; use super::parser::Cli; /// Resolve the effective diagnostic JSON preference from the raw config layers. @@ -21,12 +19,34 @@ use super::parser::Cli; /// This is used before full config merging so startup and merge-time failures /// can still honour `diag_json` values sourced from config files or the /// environment. -#[must_use] -pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> bool { - let mut diag_json = - diag_json_from_file_layers(cli).unwrap_or_else(|_| Cli::default().diag_json); - diag_json = diag_json_from_env(diag_json); - diag_json_from_matches(cli, matches, diag_json) +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] when a selected config file cannot +/// be loaded, or when `NETSUKE_DIAG_JSON` contains an invalid boolean. +pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult { + resolve_merged_diag_json_with_env(cli, matches, &StdEnvProvider) +} + +/// Resolve diagnostic JSON preference using an injected environment provider. +/// +/// This variant is intended for tests and for callers that need deterministic +/// environment access without mutating the process environment. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] when a selected config file 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 = diag_json_from_file_layers(cli, env)?; + if let Some(env_diag_json) = diag_json_from_env(env)? { + diag_json = env_diag_json; + } + Ok(diag_json_from_matches(cli, matches, diag_json)) } fn diag_json_from_layer(value: &Value) -> Option { @@ -46,9 +66,9 @@ fn diag_json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> } } -fn diag_json_from_file_layers(cli: &Cli) -> OrthoResult { +fn diag_json_from_file_layers(cli: &Cli, env: &impl EnvProvider) -> OrthoResult { let default = Cli::default().diag_json; - let layers = collect_diag_file_layers(cli)?; + let layers = collect_diag_file_layers_with_env(cli, env)?; let mut diag_json = default; for layer in layers { if let Some(layer_diag_json) = diag_json_from_layer(&layer.into_value()) { @@ -58,13 +78,105 @@ fn diag_json_from_file_layers(cli: &Cli) -> OrthoResult { Ok(diag_json) } -fn diag_json_from_env(fallback: bool) -> bool { - let env_provider = env_provider() - .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) - .split("__"); - Figment::from(env_provider) - .extract::() - .ok() - .and_then(|value| diag_json_from_layer(&value)) - .unwrap_or(fallback) +fn diag_json_from_env(env: &impl EnvProvider) -> OrthoResult> { + let Some(value) = env.get("NETSUKE_DIAG_JSON") else { + return Ok(None); + }; + let raw = value.into_string().map_err(|value| { + Arc::new(OrthoError::Validation { + key: String::from("NETSUKE_DIAG_JSON"), + message: format!("NETSUKE_DIAG_JSON must be valid Unicode, got {value:?}"), + }) + })?; + 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:?}"), + })), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::ensure; + use clap::CommandFactory; + use serde_json::json; + use std::collections::HashMap; + use std::ffi::OsString; + 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 EnvProvider for TestEnv { + fn get(&self, key: &str) -> Option { + self.values.get(key).cloned() + } + } + + #[test] + fn diag_json_from_layer_reads_diag_json_bool() { + assert_eq!(diag_json_from_layer(&json!({ "diag_json": true })), Some(true)); + assert_eq!( + diag_json_from_layer(&json!({ "diag_json": false })), + Some(false) + ); + } + + #[test] + fn diag_json_from_layer_ignores_non_bool_diag_json() { + assert_eq!(diag_json_from_layer(&json!({ "diag_json": "yes" })), None); + } + + #[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, "diag_json = false\n")?; + let matches = Cli::command().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 matches = Cli::command().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:?}" + ); + } } diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index b2b6f280..e00355aa 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -8,6 +8,7 @@ use ortho_config::{ ConfigDiscovery, 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; @@ -17,12 +18,41 @@ use super::parser::Cli; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG"; const CONFIG_ENV_VAR_LEGACY: &str = "NETSUKE_CONFIG_PATH"; +/// Provides access to environment variables used during config discovery. +/// +/// Production code uses [`StdEnvProvider`]. Tests can provide a small 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; +} + +/// Environment provider backed by [`std::env::var_os`]. +#[derive(Debug, Default, Clone, Copy)] +pub struct StdEnvProvider; + +impl EnvProvider for StdEnvProvider { + fn get(&self, key: &str) -> Option { + std::env::var_os(key) + } +} + pub(crate) fn push_file_layers( cli: &Cli, composer: &mut MergeComposer, errors: &mut Vec>, ) { - let layers_result = explicit_config_path(cli).map_or_else( + push_file_layers_with_env(cli, composer, errors, &StdEnvProvider); +} + +pub(crate) fn push_file_layers_with_env( + cli: &Cli, + composer: &mut MergeComposer, + errors: &mut Vec>, + env: &impl EnvProvider, +) { + let layers_result = explicit_config_path_with_env(cli, env).map_or_else( || collect_file_layers(cli.directory.as_deref()), |path| load_layers_from_path(&path), ); @@ -101,14 +131,21 @@ fn project_scope_layers(directory: Option<&Path>) -> OrthoResult Option { + explicit_config_path_with_env(cli, &StdEnvProvider) +} + +pub(crate) fn explicit_config_path_with_env( + cli: &Cli, + env: &impl EnvProvider, +) -> Option { cli.config .clone() - .or_else(|| env_config_path(CONFIG_ENV_VAR)) - .or_else(|| env_config_path(CONFIG_ENV_VAR_LEGACY)) + .or_else(|| env_config_path(env, CONFIG_ENV_VAR)) + .or_else(|| env_config_path(env, CONFIG_ENV_VAR_LEGACY)) } -fn env_config_path(var_name: &str) -> Option { - std::env::var_os(var_name) +fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option { + env.get(var_name) .filter(|value| !value.is_empty()) .map(PathBuf::from) } @@ -134,8 +171,126 @@ pub(crate) fn load_layers_from_path( } pub(crate) fn collect_diag_file_layers(cli: &Cli) -> OrthoResult>> { - explicit_config_path(cli).map_or_else( + collect_diag_file_layers_with_env(cli, &StdEnvProvider) +} + +pub(crate) fn collect_diag_file_layers_with_env( + cli: &Cli, + env: &impl EnvProvider, +) -> OrthoResult>> { + explicit_config_path_with_env(cli, env).map_or_else( || collect_file_layers(cli.directory.as_deref()), |path| load_layers_from_path(&path), ) } + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::ensure; + use rstest::rstest; + use std::collections::HashMap; + 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 EnvProvider for TestEnv { + fn get(&self, key: &str) -> Option { + self.values.get(key).cloned() + } + } + + #[test] + fn env_config_path_returns_none_when_var_unset() { + let env = TestEnv::default(); + 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(&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(&env, "__NETSUKE_TEST_VAR"); + assert_eq!(result, Some(PathBuf::from("/tmp/foo.toml"))); + } + + #[rstest] + #[case::cli_wins_over_env( + Some("/env/path.toml"), + Some("/legacy/path.toml"), + Some("/cli/path.toml"), + Some("/cli/path.toml") + )] + #[case::env_wins_over_legacy( + Some("/env/path.toml"), + Some("/legacy/path.toml"), + None, + Some("/env/path.toml") + )] + #[case::legacy_used_when_primary_missing( + None, + Some("/legacy/path.toml"), + None, + Some("/legacy/path.toml") + )] + #[case::none_when_all_sources_missing(None, None, None, None)] + fn explicit_config_path_obeys_precedence( + #[case] primary_env: Option<&'static str>, + #[case] legacy_env: Option<&'static str>, + #[case] cli_path: Option<&'static str>, + #[case] expected: Option<&'static str>, + ) { + let mut env = TestEnv::default(); + if let Some(path) = primary_env { + env = env.with_var(CONFIG_ENV_VAR, path); + } + if let Some(path) = legacy_env { + env = env.with_var(CONFIG_ENV_VAR_LEGACY, path); + } + let cli = Cli { + config: cli_path.map(PathBuf::from), + ..Cli::default() + }; + + assert_eq!( + explicit_config_path_with_env(&cli, &env), + expected.map(PathBuf::from) + ); + } + + #[rstest] + #[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 dir = tempdir()?; + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "diag_json = true\n")?; + + let env = TestEnv::default().with_var(config_var, config_path.as_os_str()); + let layers = collect_diag_file_layers_with_env(&Cli::default(), &env)?; + + ensure!( + !layers.is_empty(), + "should include the injected explicit config layer" + ); + + Ok(()) + } +} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index c7d2d3b5..f0d48f7b 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -16,7 +16,8 @@ mod parser; mod parsing; pub use config::{CliConfig, ColourPolicy, OutputFormat, SpinnerMode, Theme}; -pub use diag::resolve_merged_diag_json; +pub use diag::{resolve_merged_diag_json, resolve_merged_diag_json_with_env}; +pub use discovery::{EnvProvider as ConfigEnvProvider, StdEnvProvider as ConfigStdEnvProvider}; pub use merge::merge_with_config; pub use parser::{ BuildArgs, Cli, Commands, GraphArgs, diag_json_hint_from_args, locale_hint_from_args, diff --git a/src/main.rs b/src/main.rs index 3da50cc6..dc2033fc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -53,7 +53,10 @@ fn run_with_args( Ok(parsed) => parsed, Err(code) => return code, }; - let mode = DiagMode::from_json_enabled(cli::resolve_merged_diag_json(&parsed_cli, &matches)); + let mode = match resolve_diag_mode_or_exit(&parsed_cli, &matches, startup_mode) { + Ok(mode) => mode, + Err(code) => return code, + }; let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode) { Ok(merged) => merged, @@ -115,6 +118,27 @@ fn parse_cli_or_exit( } } +fn resolve_diag_mode_or_exit( + parsed_cli: &cli::Cli, + matches: &ArgMatches, + fallback_mode: DiagMode, +) -> Result { + match cli::resolve_merged_diag_json(parsed_cli, matches) { + Ok(enabled) => Ok(DiagMode::from_json_enabled(enabled)), + Err(err) => { + if fallback_mode.is_json() { + Err(diagnostic_json::emit_or_fallback( + diagnostic_json::render_error_json(err.as_ref()), + )) + } else { + init_tracing(Level::ERROR); + tracing::error!(error = %err, "configuration load failed"); + Err(ExitCode::FAILURE) + } + } + } +} + fn merge_cli_or_exit( parsed_cli: &cli::Cli, matches: &ArgMatches, diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 88603afe..39f557c3 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -9,7 +9,8 @@ use netsuke::cli_localization; use ortho_config::{MergeComposer, sanitize_value}; use rstest::{fixture, rstest}; use serde_json::json; -use std::ffi::OsStr; +use std::collections::HashMap; +use std::ffi::{OsStr, OsString}; use std::fs; use std::path::Path; use std::sync::Arc; @@ -214,7 +215,7 @@ diag_json = true let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer) .context("parse CLI args for merge")?; ensure!( - netsuke::cli::resolve_merged_diag_json(&cli, &matches), + netsuke::cli::resolve_merged_diag_json(&cli, &matches)?, "pre-merge diagnostic mode should honour config diag_json", ); let merged = netsuke::cli::merge_with_config(&cli, &matches) @@ -276,6 +277,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(()) +} + #[rstest] fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> { assert_build_targets( @@ -376,3 +398,20 @@ fn cli_runtime_canonicalizes_ascii_theme_from_no_emoji_alias() -> Result<()> { Ok(()) }) } + +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() + } +} From 2420ae738d01daf36079341aa73ee876ddf159f7 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 8 Jun 2026 22:24:50 +0200 Subject: [PATCH 2/7] Apply rustfmt after rebase (#293) --- src/cli/diag.rs | 5 ++++- src/cli/discovery.rs | 5 +---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cli/diag.rs b/src/cli/diag.rs index 2c0ec1f5..c0d10a75 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -128,7 +128,10 @@ mod tests { #[test] fn diag_json_from_layer_reads_diag_json_bool() { - assert_eq!(diag_json_from_layer(&json!({ "diag_json": true })), Some(true)); + assert_eq!( + diag_json_from_layer(&json!({ "diag_json": true })), + Some(true) + ); assert_eq!( diag_json_from_layer(&json!({ "diag_json": false })), Some(false) diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index e00355aa..7e80d422 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -134,10 +134,7 @@ pub(crate) fn explicit_config_path(cli: &Cli) -> Option { explicit_config_path_with_env(cli, &StdEnvProvider) } -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 EnvProvider) -> Option { cli.config .clone() .or_else(|| env_config_path(env, CONFIG_ENV_VAR)) From 7bc62a8a3f357ac2a7cb6755b8e5beb9ed6090c4 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 8 Jun 2026 22:29:14 +0200 Subject: [PATCH 3/7] Remove stale discovery wrappers (#293) Drop provider-free compatibility wrappers that became dead code after the rebase onto the split `discovery` and `diag` modules. Keep the injected provider helpers as the single internal path for config selection and early JSON diagnostic resolution. Update the related internal docs and test helper so the branch stays clean under warnings-denied typechecking and Clippy. --- docs/netsuke-design.md | 15 ++++++++------- src/cli/diag.rs | 7 +++++-- src/cli/discovery.rs | 8 -------- tests/cli_tests/merge.rs | 1 + 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 681d944d..c22c8c3d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2578,10 +2578,11 @@ flowchart LR ``` Netsuke configuration discovery is implemented in `src/cli/discovery.rs`. -Explicit file selection is handled by `explicit_config_path(...)`, which -applies the precedence `--config` > `NETSUKE_CONFIG` > `NETSUKE_CONFIG_PATH`. -Layer loading and automatic discovery are handled by `push_file_layers(...)`, -which also applies the `-C/--directory` flag as the project-discovery root. +Explicit file selection is handled by `explicit_config_path_with_env(...)`, +which applies the precedence `--config` > `NETSUKE_CONFIG` > +`NETSUKE_CONFIG_PATH`. Layer loading and automatic discovery are handled by +`push_file_layers(...)`, which also applies the `-C/--directory` flag as the +project-discovery root. **Figure: Explicit Config Selector Resolution** — This diagram shows how Netsuke chooses the configuration file before automatic discovery. Netsuke @@ -2694,9 +2695,9 @@ manual flag repetition. **Implementation notes**: -- The `explicit_config_path(...)` helper resolves explicit config selectors - before automatic discovery so missing or invalid explicit files remain hard - errors. +- The `explicit_config_path_with_env(...)` helper resolves explicit config + 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 diff --git a/src/cli/diag.rs b/src/cli/diag.rs index c0d10a75..be26b0b0 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -82,10 +82,13 @@ fn diag_json_from_env(env: &impl EnvProvider) -> OrthoResult> { let Some(value) = env.get("NETSUKE_DIAG_JSON") else { return Ok(None); }; - let raw = value.into_string().map_err(|value| { + 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 {value:?}"), + message: format!( + "NETSUKE_DIAG_JSON must be valid Unicode, got {}", + invalid_value.to_string_lossy() + ), }) })?; match raw.as_str() { diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index 7e80d422..c53d4b2f 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -130,10 +130,6 @@ fn project_scope_layers(directory: Option<&Path>) -> OrthoResult Option { - explicit_config_path_with_env(cli, &StdEnvProvider) -} - pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl EnvProvider) -> Option { cli.config .clone() @@ -167,10 +163,6 @@ pub(crate) fn load_layers_from_path( } } -pub(crate) fn collect_diag_file_layers(cli: &Cli) -> OrthoResult>> { - collect_diag_file_layers_with_env(cli, &StdEnvProvider) -} - pub(crate) fn collect_diag_file_layers_with_env( cli: &Cli, env: &impl EnvProvider, diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 39f557c3..de0e9f06 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -399,6 +399,7 @@ fn cli_runtime_canonicalizes_ascii_theme_from_no_emoji_alias() -> Result<()> { }) } +#[derive(Default)] struct TestEnv { values: HashMap<&'static str, OsString>, } From 7d1ca04845c4ebfe0a2b3f061724aa360bc625ac Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 16 Jun 2026 01:54:33 +0200 Subject: [PATCH 4/7] Deduplicate config error exits (#293) Extract the shared configuration-error exit conversion used by early diagnostic-mode resolution and full config merging. Keep both callers as fallible combinator chains so the exit handling stays in one place while preserving each function's public shape. --- src/main.rs | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/src/main.rs b/src/main.rs index dc2033fc..5e51004a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -118,25 +118,24 @@ fn parse_cli_or_exit( } } +fn config_err_to_exit(err: &(dyn std::error::Error + 'static), mode: DiagMode) -> ExitCode { + if mode.is_json() { + diagnostic_json::emit_or_fallback(diagnostic_json::render_error_json(err)) + } else { + init_tracing(Level::ERROR); + tracing::error!(error = %err, "configuration load failed"); + ExitCode::FAILURE + } +} + fn resolve_diag_mode_or_exit( parsed_cli: &cli::Cli, matches: &ArgMatches, fallback_mode: DiagMode, ) -> Result { - match cli::resolve_merged_diag_json(parsed_cli, matches) { - Ok(enabled) => Ok(DiagMode::from_json_enabled(enabled)), - Err(err) => { - if fallback_mode.is_json() { - Err(diagnostic_json::emit_or_fallback( - diagnostic_json::render_error_json(err.as_ref()), - )) - } else { - init_tracing(Level::ERROR); - tracing::error!(error = %err, "configuration load failed"); - Err(ExitCode::FAILURE) - } - } - } + cli::resolve_merged_diag_json(parsed_cli, matches) + .map(DiagMode::from_json_enabled) + .map_err(|err| config_err_to_exit(err.as_ref(), fallback_mode)) } fn merge_cli_or_exit( @@ -144,20 +143,9 @@ fn merge_cli_or_exit( matches: &ArgMatches, mode: DiagMode, ) -> Result { - match cli::merge_with_config(parsed_cli, matches) { - Ok(merged) => Ok(merged.with_default_command()), - Err(err) => { - if mode.is_json() { - Err(diagnostic_json::emit_or_fallback( - diagnostic_json::render_error_json(err.as_ref()), - )) - } else { - init_tracing(Level::ERROR); - tracing::error!(error = %err, "configuration load failed"); - Err(ExitCode::FAILURE) - } - } - } + cli::merge_with_config(parsed_cli, matches) + .map(cli::Cli::with_default_command) + .map_err(|err| config_err_to_exit(err.as_ref(), mode)) } fn configure_runtime( From 2dd909e3ae92413b464b1ba57973e6ee06d4928e Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 16 Jun 2026 01:56:54 +0200 Subject: [PATCH 5/7] Fix Mermaid labels for nixie Convert wrapped Mermaid node labels to quoted labels with explicit line breaks so `merman-cli` can parse the affected documentation diagrams. --- docs/rstest-bdd-v0-5-0-migration-guide.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/rstest-bdd-v0-5-0-migration-guide.md b/docs/rstest-bdd-v0-5-0-migration-guide.md index 3d14584f..c00f9701 100644 --- a/docs/rstest-bdd-v0-5-0-migration-guide.md +++ b/docs/rstest-bdd-v0-5-0-migration-guide.md @@ -45,13 +45,9 @@ fn scenario_step_result() -> StepResult<(), &'static str> { } ``` -**After (supported):** - -```rust # use rstest_bdd_macros::scenario; #[scenario(path = "tests/features/example.feature")] -fn scenario_returns_unit() -> Result<(), &'static str> { - do_setup()?; +fn scenario_step_result() -> StepResult<(), &'static str> { Ok(()) } ``` From 678929ea3899ee90abc2f7786f08fe37ba927ba0 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 16 Jun 2026 02:04:05 +0200 Subject: [PATCH 6/7] Fix docs after rebase Remove a duplicated unfenced Rust example that broke Markdown linting after the rebase and keep the conflict-resolved developer guide wrapped in the repository's documented style. --- docs/developers-guide.md | 12 ++++++------ docs/rstest-bdd-v0-5-0-migration-guide.md | 7 ------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 3112a782..6c5bfe2f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -615,8 +615,8 @@ Table: Configuration merge helper functions `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. +process-wide environment variables when exercising explicit config selection or +early diagnostic-mode resolution. ```rust pub trait EnvProvider { @@ -636,10 +636,10 @@ environment variables directly. Tests for Netsuke's own environment port should avoid `EnvLock`. `collect_diag_file_layers_with_env` and `push_file_layers_with_env` call -`explicit_config_path_with_env` 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: +`explicit_config_path_with_env` 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; diff --git a/docs/rstest-bdd-v0-5-0-migration-guide.md b/docs/rstest-bdd-v0-5-0-migration-guide.md index c00f9701..72809e77 100644 --- a/docs/rstest-bdd-v0-5-0-migration-guide.md +++ b/docs/rstest-bdd-v0-5-0-migration-guide.md @@ -45,13 +45,6 @@ fn scenario_step_result() -> StepResult<(), &'static str> { } ``` -# use rstest_bdd_macros::scenario; -#[scenario(path = "tests/features/example.feature")] -fn scenario_step_result() -> StepResult<(), &'static str> { - Ok(()) -} -``` - ### 2) Use explicit `Result`/`StepResult` in scenario signatures Scenario return classification does not resolve type aliases. When using an From 5c3576476bdb6c6b6d769783040e4410a15cb456 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 16 Jun 2026 18:11:42 +0200 Subject: [PATCH 7/7] Honour CLI diag-json before env validation (#293) Skip parsing `NETSUKE_DIAG_JSON` when a command-line diagnostic JSON selector is already present. The CLI layer has higher precedence, so a malformed lower-precedence environment value must not prevent startup from using `--diag-json` or `--output-format json`. Add regression coverage for both CLI override paths against malformed environment input. --- src/cli/diag.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/src/cli/diag.rs b/src/cli/diag.rs index be26b0b0..aafd2763 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -43,7 +43,9 @@ pub fn resolve_merged_diag_json_with_env( env: &impl EnvProvider, ) -> OrthoResult { let mut diag_json = diag_json_from_file_layers(cli, env)?; - if let Some(env_diag_json) = diag_json_from_env(env)? { + if !has_cli_diag_json_override(matches) + && let Some(env_diag_json) = diag_json_from_env(env)? + { diag_json = env_diag_json; } Ok(diag_json_from_matches(cli, matches, diag_json)) @@ -57,15 +59,27 @@ fn diag_json_from_layer(value: &Value) -> Option { } fn diag_json_from_matches(cli: &Cli, matches: &ArgMatches, discovered: bool) -> bool { - if matches.value_source("output_format") == Some(ValueSource::CommandLine) { + if has_cli_output_format_override(matches) { cli.resolved_diag_json() - } else if matches.value_source("diag_json") == Some(ValueSource::CommandLine) { + } else if has_cli_diag_json_flag(matches) { cli.diag_json } else { discovered } } +fn has_cli_diag_json_override(matches: &ArgMatches) -> bool { + has_cli_output_format_override(matches) || has_cli_diag_json_flag(matches) +} + +fn has_cli_output_format_override(matches: &ArgMatches) -> bool { + matches.value_source("output_format") == Some(ValueSource::CommandLine) +} + +fn has_cli_diag_json_flag(matches: &ArgMatches) -> bool { + matches.value_source("diag_json") == Some(ValueSource::CommandLine) +} + fn diag_json_from_file_layers(cli: &Cli, env: &impl EnvProvider) -> OrthoResult { let default = Cli::default().diag_json; let layers = collect_diag_file_layers_with_env(cli, env)?; @@ -106,6 +120,7 @@ mod tests { use super::*; use anyhow::ensure; use clap::CommandFactory; + use clap::Parser; use serde_json::json; use std::collections::HashMap; use std::ffi::OsString; @@ -185,4 +200,69 @@ mod tests { "expected validation error for NETSUKE_DIAG_JSON, got {error:?}" ); } + + #[test] + fn resolve_merged_diag_json_honours_cli_diag_json_before_malformed_env() -> anyhow::Result<()> { + let dir = tempdir()?; + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "diag_json = false\n")?; + let cli = Cli::parse_from([ + "netsuke", + "--config", + config_path + .to_str() + .expect("temp config path should be UTF-8"), + "--diag-json", + ]); + let matches = Cli::command().get_matches_from([ + "netsuke", + "--config", + config_path + .to_str() + .expect("temp config path should be UTF-8"), + "--diag-json", + ]); + let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "yes"); + + ensure!( + resolve_merged_diag_json_with_env(&cli, &matches, &env)?, + "CLI --diag-json should override malformed diagnostic JSON env" + ); + + Ok(()) + } + + #[test] + fn resolve_merged_diag_json_honours_cli_output_format_before_malformed_env() + -> anyhow::Result<()> { + let dir = tempdir()?; + let config_path = dir.path().join("netsuke.toml"); + std::fs::write(&config_path, "diag_json = false\n")?; + let cli = Cli::parse_from([ + "netsuke", + "--config", + config_path + .to_str() + .expect("temp config path should be UTF-8"), + "--output-format", + "json", + ]); + let matches = Cli::command().get_matches_from([ + "netsuke", + "--config", + config_path + .to_str() + .expect("temp config path should be UTF-8"), + "--output-format", + "json", + ]); + let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "yes"); + + ensure!( + resolve_merged_diag_json_with_env(&cli, &matches, &env)?, + "CLI --output-format json should override malformed diagnostic JSON env" + ); + + Ok(()) + } }