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" diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4ff6789f6..0ed1c78a4 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 `discover_file_layers`, which returns a +`DiscoveryOutcome` containing the cached layers, resolved JSON preference, and +deferred bounded diagnostics before full configuration merging. ### Layer precedence @@ -2405,10 +2406,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, @@ -2419,19 +2420,19 @@ 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, + 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_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 @@ -2441,35 +2442,66 @@ 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, DiscoveryOutcome); +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` 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 +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 +2513,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,15 +2523,36 @@ 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. - -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 -events, not a cryptographic guarantee. +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. + +#### 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, 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 @@ -2761,16 +2789,16 @@ 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 (`--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` diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index d3be16862..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 @@ -2883,28 +2887,26 @@ 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`. -- 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. + 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 + 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 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. - 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 diff --git a/docs/users-guide.md b/docs/users-guide.md index d24d61b36..a78a75fc7 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -865,14 +865,49 @@ 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: @@ -1132,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 diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index e4675700e..347dc4aac 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,35 @@ 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, 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/diag.rs b/src/cli/diag.rs index fff7c9288..bfef47dd3 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -7,14 +7,11 @@ 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, DiscoveryOutcome, discover_file_layers}; use super::parser::Cli; const JSON_ENV_VAR: &str = "NETSUKE_JSON"; @@ -29,12 +26,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,33 +41,56 @@ 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)?; - if !has_cli_json_override(matches) - && let Some(env_json) = json_from_env(env)? - { - json = env_json; - } - Ok(json_from_matches(cli, matches, json)) + env: &impl Env, +) -> OrthoResult<(bool, DiscoveredLayers)> { + let (result, outcome) = resolve_json_and_layers_outcome_with_env(cli, matches, env); + result.map(|json| (json, outcome.into_layers())) } -fn json_from_layer(value: &Value) -> Option { - value - .as_object() - .and_then(|map| map.get("json")) - .and_then(Value::as_bool) +/// 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. 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, DiscoveryOutcome) { + let outcome = discover_file_layers(cli, env); + let result = (|| { + if let Some(error) = outcome.first_error() { + return Err(Arc::clone(error)); + } + let mut json = outcome.json(); + 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, outcome) } /// Apply the command-line JSON override to a discovered preference. @@ -92,28 +107,12 @@ 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; - for layer in layers { - if let Some(layer_json) = json_from_layer(&layer.into_value()) { - json = layer_json; - } - } - Ok(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,25 +139,13 @@ 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; 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()?; @@ -170,7 +157,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 +179,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 +194,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 +218,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..3053e4347 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -4,15 +4,12 @@ //! 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; -use tracing::{debug, debug_span}; use super::parser::Cli; @@ -24,91 +21,148 @@ mod paths; #[path = "discovery_layers.rs"] mod layers; -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; + +#[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}; 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() - } +/// 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, } -/// 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 { + /// 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 + } + + /// Borrow the first discovery error, if loading failed. + pub(crate) fn first_error(&self) -> Option<&Arc> { + self.errors.first() + } + + /// Return the JSON preference resolved while retaining the loaded layers. + pub(crate) const fn json(&self) -> bool { + self.json } - fn entries(&self) -> Vec<(OsString, OsString)> { - std::env::vars_os().collect() + /// Consume the result into its reusable layers and deferred errors. + pub(crate) fn into_parts( + self, + ) -> (Vec>, Vec>) { + (self.layers, self.errors) } } -/// Environment adapters consumed by configuration file discovery. +/// Layers and diagnostics returned by a side-effect-free discovery pass. /// -/// 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, +/// It retains bounded events so startup can emit them after installing its tracing filter. +pub struct DiscoveryOutcome { + layers: DiscoveredLayers, + diagnostics: DiscoveryDiagnostics, +} + +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() + } + + /// Consume the outcome into the reusable file layers. + #[must_use] + pub fn into_layers(self) -> DiscoveredLayers { + self.layers + } + + /// Emit deferred diagnostics without repeating discovery. + pub fn emit_diagnostics(&self) { + self.diagnostics.emit(); + } } -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) -> DiscoveryOutcome { + let (trace, load_warning, outcome) = collect_file_layers_with_env(cli, env); + let layers = match outcome { + Ok(layers) => DiscoveredLayers::from_file_layers(layers), + Err(error) => DiscoveredLayers::from_error(error), + }; + DiscoveryOutcome { + layers, + diagnostics: DiscoveryDiagnostics::new(trace, load_warning), } } -/// 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,50 +172,46 @@ 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); - trace_config_path_resolution(&resolution); - resolution.path.map_or_else( + env: &impl Env, +) -> ( + DiscoveryTrace, + Option, + OrthoResult>>, +) { + let resolution = resolve_config_selector(cli.config.clone(), env); + let (file_layers, load_warning, 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), - ) + let (project_scope, outcome) = collect_file_layers_with_trace(cli.directory.as_deref()); + (FileLayerTrace::Automatic { project_scope }, None, outcome) }, |path| { - debug_config_path("using explicit config path", &path); - load_layers_from_path(&path) + let (load_warning, outcome) = load_layers_from_path_with_warning(path); + ( + FileLayerTrace::Explicit { + path: BoundedConfigPath::from_path(Some(path)), + }, + load_warning, + outcome, + ) }, + ); + ( + DiscoveryTrace::new(&resolution, file_layers), + load_warning, + outcome, ) } -/// 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) -} - /// 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. #[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 } @@ -181,10 +231,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", @@ -201,46 +248,32 @@ fn resolve_config_selector( } } -/// 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. /// 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) } -/// 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(), @@ -249,34 +282,21 @@ 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 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>> { - 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) -} - #[cfg(test)] #[path = "discovery_event_assertions.rs"] mod event_assertions; @@ -298,7 +318,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 +326,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 +356,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() @@ -352,18 +371,18 @@ 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 = 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 = discover_file_layers(&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_diagnostics.rs b/src/cli/discovery_diagnostics.rs index 03466d2bc..026109fb0 100644 --- a/src/cli/discovery_diagnostics.rs +++ b/src/cli/discovery_diagnostics.rs @@ -1,7 +1,7 @@ //! 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. @@ -23,45 +23,91 @@ pub(super) enum ConfigLoadFailureKind { LoadError, } -/// Trace one environment lookup using bounded path fields. -pub(super) fn trace_config_path_variable(var_name: &str, path: Option<&Path>) { +/// 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 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) 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), + is_present: path.is_some(), + } + } +} + +/// 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(), "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. -pub(super) fn warn_explicit_config_load_failed(path: &Path, failure_kind: ConfigLoadFailureKind) { +/// 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, +) { + 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, 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) { +/// 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), - path_file_name = ?path.file_name(), + path_hash = %path_hash, message ); } -/// 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 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_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_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 936c777fb..1987e2636 100644 --- a/src/cli/discovery_layer_tests.rs +++ b/src/cli/discovery_layer_tests.rs @@ -4,18 +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 crate::cli::test_support::TestEnv; + use anyhow::{Context, Result, ensure}; -use googletest::prelude::*; -use pretty_assertions::assert_eq; use rstest::rstest; use tempfile::{TempDir, tempdir}; -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 crate::cli::test_support::empty_mock_env; +use mockable::MockEnv; +use std::ffi::OsString; + +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 { @@ -48,17 +51,22 @@ 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, ) -> 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 discovered = discover_file_layers(&cli, &env); + let ((), events) = capture_events(|| { + discovered.emit_diagnostics(); + Ok::<_, anyhow::Error>(()) + })?; let branch_event = find_event(&events, expected_event)?; + let layers = discovered.layers(); ensure!( layers.is_empty() == should_be_empty, @@ -84,66 +92,99 @@ fn collect_diag_file_layers_logs_selected_branch( Ok(()) } -/// Automatic discovery must use the injected XDG directory, not the host. +/// Discovery returns its diagnostics without emitting them. #[test] -fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> { +fn discovery_defers_diagnostics_to_the_composition_boundary() -> 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::>(); + let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; + let env = empty_mock_env(); - assert_eq!(paths, vec![config_path.to_string_lossy().into_owned()]); + 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<()> { + let temp = tempdir().context("create temp dir")?; + let cli = scenario_cli(LayerScenario::ExplicitConfig, &temp)?; + let mut env = MockEnv::new(); + env.expect_os_string().never(); -/// 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 discovered = discover_file_layers(&cli, &env); + let ((), events) = capture_events(|| { + discovered.emit_diagnostics(); + 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")?; - 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 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.emit_diagnostics(); + 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 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); - } + 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.emit_diagnostics(); + Ok::<_, anyhow::Error>(()) + })?; + find_event(&events, "using config discovery")?; + find_event(&events, "discovery included project-scope layers")?; + Ok(()) } @@ -166,7 +207,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() @@ -180,6 +226,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(()) } @@ -236,3 +286,109 @@ 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.first_error().is_none(), + "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.first_error().is_some(), + "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()); + let project_layers = discovered + .layers() + .iter() + .filter(|layer| { + layer + .path() + .is_some_and(|path| path.as_str().ends_with(".netsuke.toml")) + }) + .collect::>(); + + ensure!( + project_layers.is_empty(), + "an empty directory should not produce project config layers: {project_layers:?}" + ); + ensure!( + discovered.first_error().is_none(), + "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()); + let project_layers = discovered + .layers() + .iter() + .filter(|layer| { + layer + .path() + .is_some_and(|path| path.as_str().ends_with(".netsuke.toml")) + }) + .collect::>(); + + ensure!( + project_layers.len() == 1, + "the project pass should discover one config layer: {project_layers:?}" + ); + ensure!( + 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 6b4cb8f56..1864e71cc 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 { + /// 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( + "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. @@ -52,7 +63,8 @@ pub(super) fn collect_file_layers_with_env_source( /// 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)) } @@ -63,27 +75,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 +117,21 @@ 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); + 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); + 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..9aa4ac519 --- /dev/null +++ b/src/cli/discovery_trace.rs @@ -0,0 +1,129 @@ +//! 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, ConfigLoadWarning, 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, + } + } + + /// Emit all discovery diagnostics from bounded metadata only. + pub(super) fn emit(&self) { + self.resolution.emit(); + self.file_layers.emit(); + } +} + +/// 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(), + } + } + + /// 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); + } + debug!( + selector = self.selector, + path_hash = self.path.hash.as_deref(), + 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 { + /// 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); + } + Self::Automatic { project_scope } => { + debug!("using config discovery"); + if let Some(trace) = project_scope { + 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 5100955b8..dda396ebd 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,11 +29,17 @@ 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); - trace_config_path_resolution(&resolution); + DiscoveryTrace::new( + &resolution, + FileLayerTrace::Automatic { + project_scope: None, + }, + ) + .emit(); Ok::<_, anyhow::Error>(resolution) }) } @@ -86,10 +93,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")?; @@ -109,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}" ), } @@ -150,7 +153,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 +179,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!( @@ -205,17 +208,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 +232,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 +244,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 6af1a3413..befd936b3 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -22,23 +22,24 @@ 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; use ortho_config::{MergeComposer, OrthoMergeExt, OrthoResult, sanitize_value}; use serde::Serialize; -use std::sync::Arc; use serde_json::{Map, Value, json}; +use std::{ffi::OsString, sync::Once}; 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; +const CONFIG_DISCOVERY_CACHE_TOTAL: &str = "netsuke_cli_config_discovery_cache_total"; + /// Merge discovered configuration layers over parsed CLI input. /// /// # Errors @@ -46,11 +47,14 @@ 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( + let outcome = discover_file_layers(cli, &DefaultEnv); + outcome.emit_diagnostics(); + record_config_discovery_cache("bypass"); + merge_with_layers_and_entries( cli, matches, - &StdEnvProvider, - Arc::new(ortho_config::ProcessEnv), + outcome.into_layers(), + process_environment_entries(), ) } @@ -67,17 +71,89 @@ 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 { + let outcome = discover_file_layers(cli, env); + outcome.emit_diagnostics(); + 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. +/// +/// 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_config_sources(cli, matches, env, discovery_env_source(env)) + record_config_discovery_cache("reused"); + merge_with_layers_and_entries(cli, matches, layers, process_environment_entries()) } -/// 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 { + 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() + .into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect(); + 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, + layers: DiscoveredLayers, + environment_entries: Vec<(OsString, OsString)>, ) -> OrthoResult { let mut errors = Vec::new(); let mut composer = MergeComposer::with_capacity(4); @@ -87,10 +163,9 @@ 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())) + match Figment::from(EnvironmentLayer::new(environment_entries)) .extract::() .into_ortho_merge() { @@ -109,6 +184,19 @@ fn merge_with_config_sources( 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 d8df8d584..c1fa8c166 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -21,9 +21,15 @@ 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 discovery::{EnvProvider as ConfigEnvProvider, StdEnvProvider as ConfigStdEnvProvider}; -pub use merge::{merge_with_config, merge_with_config_and_env}; +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, DiscoveryOutcome}; +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/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/src/main.rs b/src/main.rs index 9287dc428..97a3cc176 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,24 @@ 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) { +) -> Result<(DiagMode, cli::DiscoveredLayers), ExitCode> { + 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)); - Ok(mode) + 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); - // 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)); - } + outcome.emit_diagnostics(); Err(config_err_to_exit(err.as_ref(), fallback_mode)) } } @@ -247,8 +247,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_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/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/bdd/helpers/config_environment.rs b/tests/bdd/helpers/config_environment.rs index 89b6b731b..13fa94b4d 100644 --- a/tests/bdd/helpers/config_environment.rs +++ b/tests/bdd/helpers/config_environment.rs @@ -1,40 +1,30 @@ //! 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 selector_values = world.env_vars_forward.borrow().clone(); + let values = selector_values + .iter() + .filter_map(|(key, raw_value)| { + raw_value + .to_str() + .map(|text| (key.clone(), text.to_owned())) + }) + .collect::>(); + 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 +33,28 @@ 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)) +} + +#[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/cli_tests/merge_diag.rs b/tests/cli_tests/merge_diag.rs index 000351d29..3c2036c62 100644 --- a/tests/cli_tests/merge_diag.rs +++ b/tests/cli_tests/merge_diag.rs @@ -2,26 +2,47 @@ 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; -#[derive(Default)] -struct TestEnv { - values: HashMap<&'static str, OsString>, -} +const CONFIG_DISCOVERY_CACHE_TOTAL: &str = "netsuke_cli_config_discovery_cache_total"; + +type SnapshotEntry = (CompositeKey, Option, Option, DebugValue); -impl TestEnv { - fn with_var(mut self, name: &'static str, value: impl Into) -> Self { - self.values.insert(name, value.into()); - self - } +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()) } -impl netsuke::cli::ConfigEnvProvider for TestEnv { - fn get(&self, key: &str) -> Option { - self.values.get(key).cloned() - } +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] @@ -39,7 +60,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)?, @@ -48,3 +71,88 @@ 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 (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 (is_json, merged) = result?; + + ensure!( + is_json, + "the cached file layer should enable JSON diagnostics" + ); + ensure!( + merged.jobs == Some(13), + "the merge should consume the cached config layer after its file is removed" + ); + 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(()) +} diff --git a/tests/command_env_ui_tests.rs b/tests/command_env_ui_tests.rs index 0dbe45517..3b6315236 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,43 @@ 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 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( + 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/logging_stderr/config_tracing.rs b/tests/logging_stderr/config_tracing.rs index 3a66a80d1..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,16 @@ 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"), + "verbose stderr should replay the cached explicit branch: {joined}" ); ensure!( !joined.contains(raw_path.as_str()), @@ -101,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()), @@ -133,6 +141,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}" 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..5e3f2e607 --- /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; + + 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(); + drop(result); + drop(cli::merge_with_layers(&parsed, &matches, &env, layers)); + drop(cli::merge_with_config_and_env(&parsed, &matches, &env)); +} + +fn main() { + let _ = compose_cached_configuration_flow; +} 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"