Inject config environment access (#293)

build.rs

@@ -49,6 +49,8 @@ type ThemeContextCtor = fn(
     output_mode::OutputMode,
 ) -> theme::ThemeContext;
 type ResolveMergedDiagJsonFn = fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult<bool>;
+type ResolveMergedDiagJsonWithEnvFn =
+    fn(&cli::Cli, &ArgMatches, &cli::ConfigStdEnvProvider) -> ortho_config::OrthoResult<bool>;
 type MergeWithConfigFn = fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult<cli::Cli>;
 
 /// 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<String> = cli::locale_hint_from_args;
     const _: fn(&[OsString]) -> Option<bool> = cli::diag_json_hint_from_args;
     const _: fn(&str) -> Option<bool> = 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;

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<std::ffi::OsString>
+pub trait EnvProvider {
+    fn get(&self, key: &str) -> Option<std::ffi::OsString>;
+}
 ```
 
-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<Cli>;
 pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> OrthoResult<bool>;
+pub fn resolve_merged_diag_json_with_env(
+    cli: &Cli,
+    matches: &ArgMatches,
+    env: &impl EnvProvider,
+) -> OrthoResult<bool>;
 ```
 
 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

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.

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<F>(var_os: F, var_name: &str) -> Option<PathBuf>
-where
-    F: Fn(&str) -> Option<std::ffi::OsString>,
-{
-    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<OsString>;
+}
+
+/// 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<OsString> {
+        std::env::var_os(key)
+    }
+}
+
+fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option<PathBuf> {
+    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<F>(cli: &Cli, var_os: F) -> Option<PathBuf>
-where
-    F: Fn(&str) -> Option<std::ffi::OsString>,
-{
+fn resolve_config_path(cli: &Cli, env: &impl EnvProvider) -> Option<PathBuf> {
     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<Vec<MergeLayer<'static>>> {
@@ -147,13 +161,23 @@ fn diag_json_from_layer(value: &serde_json::Value) -> Option<bool> {
 /// 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<Vec<MergeLayer<'static>>> {
-    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<Vec<MergeLayer<'static>>> {
+    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<Option<bool>> {
+    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,22 +238,35 @@ 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<bool> {
+    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<bool> {
     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) {
             diag_json = layer_diag_json;
         }
     }
 
-    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::<serde_json::Value>()
-        && 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<Arc<ortho_config::OrthoError>>,
     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<se
 /// Returns an [`ortho_config::OrthoError`] if layer composition or merging
 /// fails.
 pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult<Cli> {
+    merge_with_config_with_env(cli, matches, &StdEnvProvider)
+}
+
+fn merge_with_config_with_env(
+    cli: &Cli,
+    matches: &ArgMatches,
+    env: &impl EnvProvider,
+) -> OrthoResult<Cli> {
     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<Cli> {
         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()))