From bd5769cbda2dc3a32fb1299778242d50ce80d840 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 4 Aug 2026 03:21:01 +0200 Subject: [PATCH 01/11] Narrow the build-script module graph (#513) Compile only the schema slice required for `Cli::command()` in `build.rs`. Keep parsing, preferences, validation, host matching, and runtime discovery in sibling modules so build-script dead-code analysis remains meaningful. Preserve the existing `help` command and CLI identity within the schema slice, and document the four schema-only modules that the build script compiles. --- build.rs | 97 +++++----- docs/developers-guide.md | 35 ++++ docs/netsuke-design.md | 4 +- src/cli/command.rs | 228 ++++++++++++++++++++++ src/cli/config.rs | 5 +- src/cli/diag.rs | 2 +- src/cli/discovery.rs | 2 +- src/cli/discovery_helper_proptests.rs | 2 +- src/cli/discovery_layers.rs | 2 +- src/cli/merge.rs | 11 +- src/cli/merge_input.rs | 2 +- src/cli/mod.rs | 29 +-- src/cli/parser.rs | 265 +------------------------- src/cli/parsing.rs | 6 +- src/cli/preferences.rs | 45 +++++ src/cli/validation.rs | 21 ++ src/host_matching.rs | 66 +++++++ src/host_pattern.rs | 57 +----- src/lib.rs | 1 + src/stdlib/network/policy/mod.rs | 3 +- 20 files changed, 492 insertions(+), 391 deletions(-) create mode 100644 src/cli/command.rs create mode 100644 src/cli/preferences.rs create mode 100644 src/cli/validation.rs create mode 100644 src/host_matching.rs diff --git a/build.rs b/build.rs index faa994695..06ed92fb1 100644 --- a/build.rs +++ b/build.rs @@ -9,6 +9,7 @@ //! in `locales/*/messages.ftl`, failing the build if any declared key is missing from a //! locale. use cap_std::{ambient_authority, fs::Dir}; +use clap::CommandFactory; use clap_complete::aot::{Shell, generate_to}; use clap_mangen::Man; use std::{ @@ -23,34 +24,49 @@ use time::{OffsetDateTime, format_description::well_known::Iso8601}; /// does not supply the reproducible-builds epoch. const FALLBACK_DATE: &str = "1970-01-01"; -// The build script recompiles the parser subset needed to construct -// `cli::Cli::command()` for man-page generation. Runtime discovery is excluded: -// the build script does not perform discovery, and compiling it here would pull -// its ambient canonicalization boundary into this separate compilation unit. -// The parser subset exposes more library API than this binary reaches, so the -// compiler reports unused items that the library crate and its tests exercise. -#[expect( - dead_code, - unused_imports, - reason = "shared library source; the unreached API is exercised by the library crate" -)] -#[path = "src/cli/build_support.rs"] -mod cli; +// The build script recompiles a slice of the library as its own crate so that +// `cli::Cli::command()` (used for man-page and completion generation) can be +// constructed, and so that the localization audit can read the declared key +// registry. +// +// The slice is named file by file rather than by pulling in `src/cli/mod.rs`, +// because that would drag the whole `cli` subtree: configuration discovery, +// merging, diagnostics, and localized value parsing, none of which is +// reachable here. Runtime discovery is excluded deliberately: the build script +// does not perform discovery, and compiling it here would pull its ambient +// canonicalization boundary into this separate compilation unit. Recompiling +// only what is reachable keeps rustc's unused-item analysis meaningful here +// instead of requiring module-wide `#[expect(dead_code)]` suppressions that +// would also mask genuinely dead library code. +// +// The library modules below are laid out to keep this slice small: +// `src/cli/command.rs` holds command-schema and default-command behavior, +// including `Cli::with_default_command`, with runtime preferences in +// `src/cli/preferences.rs` and the localization-aware parsing entry point in +// `src/cli/parser.rs`; matching logic is split out of `src/host_pattern.rs` +// into `src/host_matching.rs`. Adding a dependency on anything outside this +// slice will surface here as a compile error, which is the intended signal. +#[path = "src/cli"] +mod cli { + //! The Clap schema slice of `src/cli`, mirroring `src/cli/mod.rs`. -#[path = "src/cli_localization.rs"] -mod cli_localization; + #[path = "config.rs"] + pub mod config; + #[path = "validation.rs"] + mod validation; -#[expect( - dead_code, - reason = "shared library source; the unreached API is exercised by the library crate" -)] -#[path = "src/cli_l10n.rs"] -mod cli_l10n; + #[path = "help.rs"] + mod help; -#[expect( - dead_code, - reason = "shared library source; the unreached API is exercised by the library crate" -)] + #[path = "command.rs"] + mod command; + + pub use command::Cli; + pub use config::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; +} + +#[path = "src/cli_localization.rs"] +mod cli_localization; #[path = "src/host_pattern.rs"] mod host_pattern; @@ -65,6 +81,7 @@ mod host_pattern; #[path = "src/locale_catalogues.rs"] pub mod locale_catalogues; +mod build_l10n_audit; /// Message rendering, shared with the library crate. /// /// Exposed as `crate::localization`, which `cli`, `cli_l10n`, and @@ -74,22 +91,6 @@ pub mod locale_catalogues; #[path = "src/localization/mod.rs"] pub mod localization; -#[expect( - dead_code, - reason = "shared library source; the unreached API is exercised by the library crate" -)] -#[path = "src/output_mode.rs"] -mod output_mode; - -#[expect( - dead_code, - reason = "shared library source; the unreached API is exercised by the library crate" -)] -#[path = "src/theme.rs"] -mod theme; - -mod build_l10n_audit; - /// Compute the manual page date from `SOURCE_DATE_EPOCH`. /// /// Returns the epoch parsed as a Unix timestamp formatted as an ISO 8601 date; @@ -170,11 +171,11 @@ fn write_man_page(data: &[u8], dir: &Path, page_name: &str) -> std::io::Result

Result<(), Box> { // Build artefacts preserve the source en-US wording while still using the // configured parser metadata, so documentation stays deterministic. - let cmd = cli::configured_command(None); + let cmd = cli::Cli::command(); let name = cmd .get_bin_name() .unwrap_or_else(|| cmd.get_name()) @@ -259,7 +260,7 @@ fn generate_completions(out_dir: &Path) -> Result<(), Box let working_dir = Dir::open_ambient_dir(".", ambient_authority())?; working_dir.create_dir_all(out_dir)?; // Keep completion metadata in the same source en-US wording as the manual. - let cli_command = cli::configured_command(None); + let cli_command = cli::Cli::command(); let name = cli_command .get_bin_name() .unwrap_or_else(|| cli_command.get_name()) @@ -272,7 +273,7 @@ fn generate_completions(out_dir: &Path) -> Result<(), Box Shell::PowerShell, Shell::Zsh, ] { - let mut completion_command = cli::configured_command(None); + let mut completion_command = cli::Cli::command(); generate_to(shell, &mut completion_command, &name, out_dir)?; } diff --git a/docs/developers-guide.md b/docs/developers-guide.md index a4677fe7f..16d0f892b 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1001,6 +1001,41 @@ references inside TOML code fences in those files during a version bump. When release-validation requirements or documentation paths change, update `lading.toml` and this section in the same change-set. +## The build script's module slice + +`build.rs` recompiles part of the library as its own crate: it needs +`cli::Cli::command()` for man-page generation and the key registry in +`src/localization/keys.rs` for the Fluent audit. Rather than declaring +`src/cli/mod.rs` and inheriting the whole subtree, it declares an inline `cli` +module naming exactly four files — `src/cli/command.rs`, `src/cli/config.rs`, +`src/cli/help.rs`, and `src/cli/validation.rs`. + +That slice is a maintained boundary, not an accident: + +- `src/cli/command.rs` holds Clap definitions only. Runtime behaviour on `Cli` + belongs in `src/cli/preferences.rs`, and the localization-aware parsing entry + point belongs in `src/cli/parser.rs`. +- `src/cli/validation.rs` holds the shared limits and error constructor that + `src/cli/config.rs` needs, so neither file has to reach up into + `src/cli/mod.rs`. +- `src/cli/help.rs` holds the `help` subcommand's data types, which are part of + the Clap schema but do not need the runtime help renderer. +- `src/host_pattern.rs` covers pattern syntax; matching a concrete hostname + against a parsed pattern lives in `src/host_matching.rs`, which the build + script does not compile. + +Keeping the slice narrow is what lets rustc's unused-item analysis run normally +inside the build-script crate. Widening it — for example by making +`src/cli/command.rs` depend on the merge or discovery layers — reintroduces +unreachable items and, with them, the module-wide `#[expect(dead_code)]` +suppressions that issue #513 removed. Those suppressions also masked genuinely +dead code: an unused `pub` item in `src/cli/config.rs` is reported by the +build-script crate but not by the library, because the library exports that +module publicly. + +A dependency added outside the slice surfaces as a build-script compile error. +Prefer moving the new code into a sibling module over widening the slice. + ## Local build acceleration Debug builds and tests can optionally use the [`mold`] linker and the Cranelift diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index bec6b4418..bb3606ec2 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2734,7 +2734,9 @@ the targets listed in the `defaults` section of the manifest are built. ### 8.4 Design Decisions -The parser-facing `Cli` type is now defined in `src/cli/parser.rs`, while +The parser-facing `Cli` type is now defined in `src/cli/command.rs`, with the +localization-aware parsing entry point in `src/cli/parser.rs` and the runtime +preference accessors in `src/cli/preferences.rs`, while layered configuration lives in a dedicated `CliConfig` struct derived with OrthoConfig in `src/cli/config.rs`. The top-level `src/cli/mod.rs` module re-exports that public CLI surface. This separation keeps parsing, diff --git a/src/cli/command.rs b/src/cli/command.rs new file mode 100644 index 000000000..9d45bd928 --- /dev/null +++ b/src/cli/command.rs @@ -0,0 +1,228 @@ +//! The Clap-derived command tree. +//! +//! This module owns the runtime-visible [`Cli`] struct and every associated +//! Clap definition ([`InteractionArgs`], [`BuildArgs`], [`GraphArgs`], +//! [`Commands`]). It holds definitions only: no parsing entry point, no +//! localisation, and no runtime behaviour. +//! +//! **Pipeline position:** schema layer, below [`super::parser`]. +//! +//! The narrow dependency surface is deliberate. `build.rs` recompiles this +//! module (plus [`super::config`], [`super::help`], and +//! [`super::validation`]) to obtain `Cli::command()` for man-page generation; +//! anything reachable from here is also compiled by the build script, so +//! behaviour that the man page does not need belongs in a sibling module +//! instead. + +use clap::{Args, Parser, Subcommand}; +use serde::{Deserialize, Serialize}; +use std::path::PathBuf; + +use super::config::CliConfig; +use super::help::HelpArgs; +use super::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; +use crate::host_pattern::HostPattern; + +/// A modern, friendly build system that uses YAML and Jinja, powered by Ninja. +#[derive(Debug, Parser, Serialize, Deserialize)] +#[command( + name = "netsuke", + bin_name = "netsuke", + author, + version, + about, + long_about = None, + disable_help_subcommand = true +)] +pub struct Cli { + /// Path to the Netsuke manifest file to use. + #[arg( + short, + long, + value_name = "FILE", + default_value_os_t = CliConfig::default_manifest_path() + )] + pub file: PathBuf, + + /// Run as if started in this directory. + /// + /// This affects manifest lookup, output paths, and config discovery. + #[arg(short = 'C', long, value_name = "DIR")] + pub directory: Option, + + /// Path to a configuration file, bypassing automatic discovery. + #[arg(long, value_name = "FILE")] + #[serde(skip)] + pub config: Option, + + /// Set the number of parallel build jobs. + /// + /// Values must be between 1 and 64. + #[arg(short, long, value_name = "N")] + pub jobs: Option, + + /// Enable verbose diagnostic logging and completion timing summaries. + #[arg(short, long)] + pub verbose: bool, + + /// Locale tag for CLI copy (for example: en-US, es-ES). + #[arg(long, value_name = "LOCALE")] + pub locale: Option, + + /// Additional URL schemes allowed for the `fetch` helper. + #[arg(long = "fetch-allow-scheme", value_name = "SCHEME")] + pub fetch_allow_scheme: Vec, + + /// Hostnames that are permitted when default deny is enabled. + /// + /// Supports wildcards such as `*.example.com`. + #[arg(long = "fetch-allow-host", value_name = "HOST")] + pub fetch_allow_host: Vec, + + /// Hostnames that are always blocked, even when allowed elsewhere. + /// + /// Supports wildcards such as `*.example.com`. + #[arg(long = "fetch-block-host", value_name = "HOST")] + pub fetch_block_host: Vec, + + /// Deny all hosts by default; only allow the declared allowlist. + #[arg(long = "fetch-default-deny")] + pub fetch_default_deny: bool, + + /// Emit machine-readable JSON output. + #[arg(long)] + pub json: bool, + + /// Interaction policy flags. + #[command(flatten)] + pub interaction: InteractionArgs, + + /// Select the colour policy for terminal output. + #[arg(long, value_name = "POLICY", default_value_t)] + pub color: ColourPolicy, + + /// Select the emoji policy for terminal output. + #[arg(long, value_name = "POLICY", default_value_t)] + pub emoji: EmojiPolicy, + + /// Select the progress-rendering policy. + #[arg(long, value_name = "POLICY", default_value_t)] + pub progress: ProgressPolicy, + + /// Select the accessible-output policy. + #[arg(long, value_name = "POLICY", default_value_t)] + pub accessibility: AccessibilityPolicy, + + /// Default build targets used when none are specified on the CLI. + #[arg(long = "default-target", value_name = "TARGET")] + pub default_targets: Vec, + + /// Optional subcommand to execute; defaults to `build` when omitted. + #[serde(skip)] + #[command(subcommand)] + pub command: Option, +} + +impl Cli { + /// Apply the default command if none was specified. + #[must_use] + pub fn with_default_command(mut self) -> Self { + if self.command.is_none() { + self.command = Some(Commands::Build(BuildArgs::default())); + } + self + } +} + +impl Default for Cli { + fn default() -> Self { + Self { + file: CliConfig::default_manifest_path(), + directory: None, + config: None, + jobs: None, + verbose: false, + locale: None, + fetch_allow_scheme: Vec::new(), + fetch_allow_host: Vec::new(), + fetch_block_host: Vec::new(), + fetch_default_deny: false, + json: false, + interaction: InteractionArgs::default(), + color: ColourPolicy::Auto, + emoji: EmojiPolicy::Auto, + progress: ProgressPolicy::Auto, + accessibility: AccessibilityPolicy::Auto, + default_targets: Vec::new(), + command: None, + } + .with_default_command() + } +} + +/// Arguments controlling whether Netsuke may read interactive input. +#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize)] +pub struct InteractionArgs { + /// Never read interactive input. + #[arg(long, default_value_t = true)] + pub no_input: bool, +} + +impl Default for InteractionArgs { + fn default() -> Self { + Self { no_input: true } + } +} + +/// Arguments accepted by the `build` command. +#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] +pub struct BuildArgs { + /// A list of specific targets to build. + #[serde(default)] + pub targets: Vec, +} + +/// Arguments accepted by the `graph` command. +/// +/// `html` and `output` are per-invocation flags and are intentionally excluded +/// from `OrthoConfig` layering (`#[serde(skip)]`); layering them through a +/// configuration file would silently change the artefact destination. +#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] +pub struct GraphArgs { + /// Render the graph as a self-contained HTML page instead of DOT. + #[arg(long)] + #[serde(skip)] + pub html: bool, + + /// Write the graph artefact to FILE. Use `-` for stdout. + #[arg(long, value_name = "FILE")] + #[serde(skip)] + pub output: Option, +} + +/// Available top-level commands for Netsuke. +#[derive(Debug, Subcommand, PartialEq, Eq, Clone, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum Commands { + /// Build specified targets (or default targets if none are given). + Build(BuildArgs), + + /// Remove build artefacts and intermediate files. + Clean, + + /// Display the build dependency graph in DOT format for visualisation. + Graph(GraphArgs), + + /// Generate the Ninja manifest without invoking Ninja. + Generate { + /// Write the generated Ninja manifest to FILE instead of stdout. + #[arg(long, value_name = "FILE")] + output: Option, + }, + + /// Print the top-level help, or the help for a named topic such as `help targets`. + /// + /// With no topic this matches `--help`. `help targets` renders the + /// target and action catalogue for the selected manifest. + Help(HelpArgs), +} diff --git a/src/cli/config.rs b/src/cli/config.rs index 36b61ba0d..56f6ff56a 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -3,14 +3,13 @@ //! [`CliConfig`] is the single typed schema used for configuration discovery //! and merging. It captures global CLI settings plus per-subcommand defaults //! under the `cmds` namespace. - use ortho_config::{OrthoConfig, OrthoResult, PostMergeContext, PostMergeHook}; use serde::{Deserialize, Serialize}; use std::fmt; use std::path::PathBuf; use std::str::FromStr; -use super::validation_error; +use super::validation::validation_error; use crate::host_pattern::HostPattern; #[path = "policy_definitions.rs"] @@ -281,7 +280,7 @@ impl CliConfig { } /// Maximum number of parallel build jobs accepted by the CLI. -const MAX_JOBS: usize = super::MAX_JOBS; +const MAX_JOBS: usize = super::validation::MAX_JOBS; /// Fixed reason reported when merged configuration enables interactive input. pub(crate) const NO_INPUT_VALIDATION_REASON: &str = diff --git a/src/cli/diag.rs b/src/cli/diag.rs index 927cd4a92..aa324545f 100644 --- a/src/cli/diag.rs +++ b/src/cli/diag.rs @@ -10,10 +10,10 @@ use clap::parser::ValueSource; use ortho_config::{OrthoError, OrthoResult}; use std::sync::Arc; +use super::command::Cli; use super::discovery::{ DiscoveryOutcome, EnvProvider, StdEnvProvider, collect_diag_file_layers_with_env, }; -use super::parser::Cli; /// Environment variable carrying an explicit JSON-output preference. const JSON_ENV_VAR: &str = "NETSUKE_JSON"; diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index f315e62a5..c96b650f5 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -10,7 +10,7 @@ use std::io; use std::path::{Path, PathBuf}; use std::sync::Arc; -use super::parser::Cli; +use super::command::Cli; #[path = "discovery_environment.rs"] mod environment; diff --git a/src/cli/discovery_helper_proptests.rs b/src/cli/discovery_helper_proptests.rs index 961be8e3f..0382eae77 100644 --- a/src/cli/discovery_helper_proptests.rs +++ b/src/cli/discovery_helper_proptests.rs @@ -205,7 +205,7 @@ proptest! { .iter() .filter_map(json_from_value) .next_back() - .unwrap_or_else(|| crate::cli::parser::Cli::default().json); + .unwrap_or_else(|| crate::cli::Cli::default().json); prop_assert_eq!(json_preference, expected); prop_assert_eq!(final_layers.len(), source_len); } diff --git a/src/cli/discovery_layers.rs b/src/cli/discovery_layers.rs index aadd055de..17e0b3b55 100644 --- a/src/cli/discovery_layers.rs +++ b/src/cli/discovery_layers.rs @@ -16,7 +16,7 @@ use std::path::{Path, PathBuf}; #[cfg(test)] use std::sync::Arc; -use super::super::parser::Cli; +use super::super::command::Cli; use super::CONFIG_ENV_VAR; use super::diagnostics::{ BoundedConfigPath, ProjectLayerDeduplication, debug_optional_config_path_from_fields, diff --git a/src/cli/merge.rs b/src/cli/merge.rs index 63e162042..ed072fa55 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -1,6 +1,6 @@ //! Layer-composition and conversion helpers for CLI configuration. //! -//! This module bridges the Clap-facing [`Cli`] type from [`super::parser`] +//! This module bridges the Clap-facing [`Cli`] type from [`super::command`] //! and the OrthoConfig-derived [`CliConfig`] schema from [`super::config`]. //! It implements the full four-layer merge pipeline: //! @@ -13,7 +13,8 @@ //! //! **Pipeline position:** merge layer. //! -//! - Consumes `(Cli, ArgMatches)` from [`super::parser`]. +//! - Consumes `(Cli, ArgMatches)` from [`super::parser`], whose schema lives +//! in [`super::command`]. //! - Applies `CliConfig`'s `PostMergeHook` for cross-field validation. //! - Produces a fully resolved `Cli` for the runner. //! @@ -28,6 +29,7 @@ use serde::Serialize; use serde_json::{Map, Value, json}; +use super::command::{BuildArgs, Cli, Commands}; use super::config::{BuildConfig, CliConfig, validation_rejection_reason}; use super::discovery::{ DiscoveredLayers, EnvProvider, StdEnvProvider, discover_file_layers, @@ -38,8 +40,7 @@ use super::merge_input::{CachedMergeInput, MergeComposition}; use super::merge_observability::{ NoopMergeObserver, collect_override_leaf_paths, is_empty_configuration_value, }; -use super::parser::{BuildArgs, Cli, Commands}; -use super::validation_error; +use super::validation::validation_error; use super::{MergeEvent, MergeObserver}; /// Merge discovered configuration layers over parsed CLI input. @@ -354,7 +355,7 @@ fn apply_config(parsed: &Cli, config: CliConfig) -> Cli { fetch_block_host: config.fetch_block_host, fetch_default_deny: config.fetch_default_deny, json: config.json, - interaction: super::parser::InteractionArgs { + interaction: super::command::InteractionArgs { no_input: config.no_input.is_enabled(), }, color: config.color, diff --git a/src/cli/merge_input.rs b/src/cli/merge_input.rs index 4d3f582d9..b5bf52ae0 100644 --- a/src/cli/merge_input.rs +++ b/src/cli/merge_input.rs @@ -9,9 +9,9 @@ use ortho_config::declarative::LayerComposition; use ortho_config::{MergeComposer, OrthoError, OrthoResult}; use std::sync::Arc; +use super::command::Cli; use super::config::CliConfig; use super::discovery::{DiscoveredLayers, EnvProvider}; -use super::parser::Cli; /// Inputs for one cached configuration merge, owned by its application caller. pub struct CachedMergeInput<'a, E: ?Sized> { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 4ff87b59d..4c504e356 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -4,10 +4,13 @@ //! syntax, while [`CliConfig`] is the authoritative OrthoConfig-derived schema //! used to merge defaults, configuration files, environment variables, and CLI //! overrides into the runtime shape consumed by the runner. +//! +//! The module is split so that `build.rs` can compile the Clap schema alone. +//! `command`, [`config`], `help`, and `validation` form that self-contained +//! slice; every other submodule here is library-only. See the note in +//! `build.rs`. -use ortho_config::OrthoError; -use std::sync::Arc; - +mod command; pub mod config; mod constants; mod diag; @@ -20,11 +23,14 @@ mod merge_observability; mod parser; mod parsing; mod policy_values; +mod preferences; mod release_help; #[cfg(test)] pub(crate) mod test_support; +mod validation; mod value_parser; +pub use command::{BuildArgs, Cli, Commands, GraphArgs}; pub use config::{AccessibilityPolicy, CliConfig, ColourPolicy, EmojiPolicy, ProgressPolicy}; pub use diag::{ resolve_json_and_layers_outcome_with_env, resolve_merged_json, resolve_merged_json_with_env, @@ -50,10 +56,7 @@ pub use merge_input::CachedMergeInput; /// Bounded events and the production tracing adapter for observer-enabled merges. pub use merge_observability::{MergeEvent, MergeObserver, TracingMergeObserver}; pub(crate) use parser::configured_command; -pub use parser::{ - BuildArgs, Cli, Commands, GraphArgs, json_hint_from_args, locale_hint_from_args, - parse_with_localizer_from, -}; +pub use parser::{json_hint_from_args, locale_hint_from_args, parse_with_localizer_from}; pub use release_help::ReleaseHelpCli; /// Counter recording configuration discovery passes by bounded outcome. @@ -62,18 +65,6 @@ pub const DISCOVERY_TOTAL: &str = "netsuke_cli_config_discovery_total"; pub const DISCOVERY_DURATION: &str = "netsuke_cli_config_discovery_duration_seconds"; /// Bounded outcome values admitted on the discovery counter series. pub const DISCOVERY_OUTCOME_VALUES: [&str; 2] = ["success", "error"]; - -/// Maximum number of jobs accepted by the CLI. -pub(super) const MAX_JOBS: usize = 64; - -/// Build an `OrthoError::Validation` error for `key` with the given message. -pub(super) fn validation_error(key: &str, message: &str) -> Arc { - Arc::new(OrthoError::Validation { - key: key.to_owned(), - message: message.to_owned(), - }) -} - #[cfg(test)] #[path = "merge_logging_proptests.rs"] mod merge_logging_proptests; diff --git a/src/cli/parser.rs b/src/cli/parser.rs index 4df215390..52d0f001d 100644 --- a/src/cli/parser.rs +++ b/src/cli/parser.rs @@ -1,11 +1,9 @@ -//! Clap-facing parser types and localization helpers. +//! Localisation-aware parsing helpers for the CLI command schema. //! -//! This module owns the runtime-visible [`Cli`] struct and all associated -//! Clap definitions ([`BuildArgs`], [`Commands`]). It also provides -//! [`configured_command`], which optionally localizes the Clap command and installs -//! localization-aware [`LocalizedValueParser`] instances for every typed -//! argument. [`parse_with_localizer_from`] parses that configured command and -//! returns `(Cli, ArgMatches)` for downstream processing. +//! [`super::command`] owns [`Cli`] and its Clap definitions. This module +//! localises that schema, installs [`LocalizedValueParser`] instances for every +//! typed argument, and provides [`parse_with_localizer_from`] for downstream +//! processing. //! //! **Pipeline position:** parsing layer. //! @@ -16,15 +14,12 @@ //! [`LocalizedValueParser`]: super::value_parser::LocalizedValueParser use clap::builder::ValueParser; -use clap::{ArgMatches, Args, CommandFactory, Parser, Subcommand}; +use clap::{ArgMatches, CommandFactory}; use ortho_config::{LocalizationArgs, Localizer, parse_localized_command}; -use serde::{Deserialize, Serialize}; use std::ffi::OsString; -use std::path::PathBuf; use std::sync::Arc; -use super::config::CliConfig; -use super::help::HelpArgs; +use super::command::Cli; use super::parsing::{ parse_accessibility_policy, parse_color_policy, parse_emoji_policy, parse_host_pattern, parse_jobs, parse_locale, parse_progress_policy, parse_scheme, @@ -34,12 +29,9 @@ use super::policy_values::{ emoji_policy_possible_values, progress_policy_possible_values, }; use super::value_parser::LocalizedValueParser; -use super::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; use crate::cli_l10n::localize_command; pub use crate::cli_l10n::{json_hint_from_args, locale_hint_from_args}; use crate::cli_localization::build_localizer; -use crate::host_pattern::HostPattern; -use crate::theme::ThemePreference; /// Return the localized message for `key`, or `fallback` when no translation exists. pub(super) fn validation_message( @@ -51,243 +43,7 @@ pub(super) fn validation_message( localizer.message(key, args, fallback) } -/// A modern, friendly build system that uses YAML and Jinja, powered by Ninja. -#[derive(Debug, Parser, Serialize, Deserialize)] -#[command( - name = "netsuke", - bin_name = "netsuke", - author, - version, - about, - long_about = None, - disable_help_subcommand = true -)] -pub struct Cli { - #[arg( - short, - long, - value_name = "FILE", - default_value_os_t = CliConfig::default_manifest_path() - )] - /// Path to the Netsuke manifest file to use. - pub file: PathBuf, - - /// Run as if started in this directory. - /// - /// This affects manifest lookup, output paths, and config discovery. - #[arg(short = 'C', long, value_name = "DIR")] - pub directory: Option, - - /// Path to a configuration file, bypassing automatic discovery. - #[arg(long, value_name = "FILE")] - #[serde(skip)] - pub config: Option, - - /// Set the number of parallel build jobs. - /// - /// Values must be between 1 and 64. - #[arg(short, long, value_name = "N")] - pub jobs: Option, - - /// Enable verbose diagnostic logging and completion timing summaries. - #[arg(short, long)] - pub verbose: bool, - - /// Locale tag for CLI copy (for example: en-US, es-ES). - #[arg(long, value_name = "LOCALE")] - pub locale: Option, - - /// Additional URL schemes allowed for the `fetch` helper. - #[arg(long = "fetch-allow-scheme", value_name = "SCHEME")] - pub fetch_allow_scheme: Vec, - - /// Hostnames that are permitted when default deny is enabled. - /// - /// Supports wildcards such as `*.example.com`. - #[arg(long = "fetch-allow-host", value_name = "HOST")] - pub fetch_allow_host: Vec, - - /// Hostnames that are always blocked, even when allowed elsewhere. - /// - /// Supports wildcards such as `*.example.com`. - #[arg(long = "fetch-block-host", value_name = "HOST")] - pub fetch_block_host: Vec, - - /// Deny all hosts by default; only allow the declared allowlist. - #[arg(long = "fetch-default-deny")] - pub fetch_default_deny: bool, - - /// Emit machine-readable JSON output. - #[arg(long)] - pub json: bool, - - /// Interaction policy flags. - #[command(flatten)] - pub interaction: InteractionArgs, - - /// Select the colour policy for terminal output. - #[arg(long, value_name = "POLICY", default_value_t)] - pub color: ColourPolicy, - - /// Select the emoji policy for terminal output. - #[arg(long, value_name = "POLICY", default_value_t)] - pub emoji: EmojiPolicy, - - /// Select the progress-rendering policy. - #[arg(long, value_name = "POLICY", default_value_t)] - pub progress: ProgressPolicy, - - /// Select the accessible-output policy. - #[arg(long, value_name = "POLICY", default_value_t)] - pub accessibility: AccessibilityPolicy, - - /// Default build targets used when none are specified on the CLI. - #[arg(long = "default-target", value_name = "TARGET")] - pub default_targets: Vec, - - /// Optional subcommand to execute; defaults to `build` when omitted. - #[serde(skip)] - #[command(subcommand)] - pub command: Option, -} - -impl Cli { - /// Apply the default command if none was specified. - #[must_use] - pub fn with_default_command(mut self) -> Self { - if self.command.is_none() { - self.command = Some(Commands::Build(BuildArgs::default())); - } - self - } - - /// Return the effective theme preference for emoji policy resolution. - #[must_use] - pub const fn theme_preference(&self) -> Option { - match self.emoji { - EmojiPolicy::Auto => None, - EmojiPolicy::Always => Some(ThemePreference::Unicode), - EmojiPolicy::Never => Some(ThemePreference::Ascii), - } - } - - /// Return an explicit accessible-output override, if configured. - #[must_use] - pub const fn accessibility_override(&self) -> Option { - match self.accessibility { - AccessibilityPolicy::Auto => None, - AccessibilityPolicy::On => Some(true), - AccessibilityPolicy::Off => Some(false), - } - } - - /// Return whether interactive input is disabled. - #[must_use] - pub const fn no_input(&self) -> bool { - self.interaction.no_input - } - - /// Return whether progress summaries should be enabled. - #[must_use] - pub const fn progress_enabled(&self) -> bool { - !matches!(self.progress, ProgressPolicy::Never) - } -} - -impl Default for Cli { - fn default() -> Self { - Self { - file: CliConfig::default_manifest_path(), - directory: None, - config: None, - jobs: None, - verbose: false, - locale: None, - fetch_allow_scheme: Vec::new(), - fetch_allow_host: Vec::new(), - fetch_block_host: Vec::new(), - fetch_default_deny: false, - json: false, - interaction: InteractionArgs::default(), - color: ColourPolicy::Auto, - emoji: EmojiPolicy::Auto, - progress: ProgressPolicy::Auto, - accessibility: AccessibilityPolicy::Auto, - default_targets: Vec::new(), - command: None, - } - .with_default_command() - } -} - -/// Arguments controlling whether Netsuke may read interactive input. -#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub struct InteractionArgs { - /// Never read interactive input. - #[arg(long, default_value_t = true)] - pub no_input: bool, -} - -impl Default for InteractionArgs { - fn default() -> Self { - Self { no_input: true } - } -} - -/// Arguments accepted by the `build` command. -#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] -pub struct BuildArgs { - /// A list of specific targets to build. - #[serde(default)] - pub targets: Vec, -} - -/// Arguments accepted by the `graph` command. -/// -/// `html` and `output` are per-invocation flags and are intentionally excluded -/// from `OrthoConfig` layering (`#[serde(skip)]`); layering them through a -/// configuration file would silently change the artefact destination. -#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] -pub struct GraphArgs { - /// Render the graph as a self-contained HTML page instead of DOT. - #[arg(long)] - #[serde(skip)] - pub html: bool, - - /// Write the graph artefact to FILE. Use `-` for stdout. - #[arg(long, value_name = "FILE")] - #[serde(skip)] - pub output: Option, -} - -/// Available top-level commands for Netsuke. -#[derive(Debug, Subcommand, PartialEq, Eq, Clone, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case")] -pub enum Commands { - /// Build specified targets (or default targets if none are given). - Build(BuildArgs), - - /// Remove build artefacts and intermediate files. - Clean, - - /// Display the build dependency graph in DOT format for visualization. - Graph(GraphArgs), - - /// Generate the Ninja manifest without invoking Ninja. - Generate { - /// Write the generated Ninja manifest to FILE instead of stdout. - #[arg(long, value_name = "FILE")] - output: Option, - }, - - /// Print the top-level help, or the help for a named topic such as `help targets`. - /// - /// With no topic this matches `--help`. `help targets` renders the - /// target and action catalogue for the selected manifest. - Help(HelpArgs), -} - -/// Parse CLI arguments with localized clap output. +/// Parse CLI arguments with localized Clap output. /// /// Returns both the parsed CLI struct and the `ArgMatches` required for /// configuration merging. @@ -309,9 +65,8 @@ where /// Construct the command with every typed validation parser installed. /// -/// This is the sole command-composition path for parser, runtime-help, and -/// generated-artefact consumers. A supplied localizer localizes the command; -/// a missing one retains source `en-US` wording for build artefacts. +/// A supplied localizer localizes the command; without one, the helper retains +/// source `en-US` wording for runtime help consumers. pub(crate) fn configured_command(localizer: Option<&Arc>) -> clap::Command { let parser_localizer = localizer .cloned() diff --git a/src/cli/parsing.rs b/src/cli/parsing.rs index 5a0f417a7..386838bac 100644 --- a/src/cli/parsing.rs +++ b/src/cli/parsing.rs @@ -34,17 +34,17 @@ pub(super) fn parse_jobs(localizer: &dyn Localizer, s: &str) -> Result Option { + match self.emoji { + EmojiPolicy::Auto => None, + EmojiPolicy::Always => Some(ThemePreference::Unicode), + EmojiPolicy::Never => Some(ThemePreference::Ascii), + } + } + + /// Return an explicit accessible-output override, if configured. + #[must_use] + pub const fn accessibility_override(&self) -> Option { + match self.accessibility { + AccessibilityPolicy::Auto => None, + AccessibilityPolicy::On => Some(true), + AccessibilityPolicy::Off => Some(false), + } + } + + /// Return whether interactive input is disabled. + #[must_use] + pub const fn no_input(&self) -> bool { + self.interaction.no_input + } + + /// Return whether progress summaries should be enabled. + #[must_use] + pub const fn progress_enabled(&self) -> bool { + !matches!(self.progress, ProgressPolicy::Never) + } +} diff --git a/src/cli/validation.rs b/src/cli/validation.rs new file mode 100644 index 000000000..4e025a51c --- /dev/null +++ b/src/cli/validation.rs @@ -0,0 +1,21 @@ +//! Shared validation limits and error construction for the CLI module tree. +//! +//! These items are needed by both [`super::config`] (layered-configuration +//! validation) and [`super::parsing`] (Clap value validation), so they live in +//! their own leaf module rather than in [`super`]. Keeping them free of +//! dependencies lets the build script compile [`super::config`] without +//! dragging in the rest of the `cli` subtree. + +use ortho_config::OrthoError; +use std::sync::Arc; + +/// Maximum number of jobs accepted by the CLI. +pub(super) const MAX_JOBS: usize = 64; + +/// Build a validation error for `key` with `message`. +pub(super) fn validation_error(key: &str, message: &str) -> Arc { + Arc::new(OrthoError::Validation { + key: key.to_owned(), + message: message.to_owned(), + }) +} diff --git a/src/host_matching.rs b/src/host_matching.rs new file mode 100644 index 000000000..bfa52bb6a --- /dev/null +++ b/src/host_matching.rs @@ -0,0 +1,66 @@ +//! Matching of concrete hostnames against parsed host patterns. +//! +//! Pattern *syntax* lives in [`crate::host_pattern`]; the matching rules that +//! consume a parsed pattern live here. The split keeps +//! [`crate::host_pattern`] free of anything the Clap schema does not need, so +//! `build.rs` can compile it for man-page generation without recompiling +//! runtime policy evaluation. See the note in `build.rs`. + +use crate::host_pattern::HostPattern; + +/// A concrete hostname being tested against a [`HostPattern`]. +#[derive(Copy, Clone)] +pub(crate) struct HostCandidate<'a>(pub(crate) &'a str); + +impl<'a> HostCandidate<'a> { + /// Return the wrapped hostname. + const fn as_str(self) -> &'a str { + self.0 + } +} + +impl HostPattern { + /// Return whether `candidate` is covered by this pattern. + pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool { + let host = candidate.as_str().to_ascii_lowercase(); + if self.wildcard { + // Wildcard patterns match only subdomains, not the apex domain. + // Example: "*.example.com" matches "sub.example.com" but not + // "example.com". + host.strip_suffix(&self.pattern) + .and_then(|prefix| prefix.strip_suffix('.')) + .is_some_and(|prefix| !prefix.is_empty()) + } else { + host == self.pattern + } + } +} + +#[cfg(test)] +mod tests { + //! Unit tests for wildcard and exact host matching. + use super::*; + + use anyhow::{Result, ensure}; + use rstest::rstest; + + #[rstest] + #[case("example.com", "example.com", true)] + #[case("example.com", "sub.example.com", false)] + #[case("*.example.com", "sub.example.com", true)] + #[case("*.example.com", "example.com", false)] + #[case("*.example.com", "deep.sub.example.com", true)] + #[case("*.example.com", "other.com", false)] + fn host_pattern_matches_expected( + #[case] pattern: &str, + #[case] host: &str, + #[case] expected: bool, + ) -> Result<()> { + let parsed = HostPattern::parse(pattern)?; + ensure!( + parsed.matches(HostCandidate(host)) == expected, + "expected match={expected} for {host} against {pattern}", + ); + Ok(()) + } +} diff --git a/src/host_pattern.rs b/src/host_pattern.rs index e02927763..c1073781c 100644 --- a/src/host_pattern.rs +++ b/src/host_pattern.rs @@ -1,7 +1,10 @@ //! Shared host pattern validation helpers. //! -//! The module centralizes normalization and matching logic so CLI parsing and -//! runtime policy evaluation agree on allowable host syntax. +//! The module centralizes host pattern normalization so CLI parsing and +//! runtime policy evaluation agree on allowable host syntax. Matching a +//! concrete hostname against a parsed pattern lives in +//! `crate::host_matching`, which keeps this module's dependency surface +//! narrow enough for `build.rs` to compile it for man-page generation. use crate::localization::{self, LocalizedMessage, keys}; use serde::{Deserialize, Serialize}; @@ -18,18 +21,6 @@ impl<'a> HostPatternInput<'a> { self.0 } } - -/// Host name presented for matching against a `HostPattern`. -#[derive(Copy, Clone)] -pub(crate) struct HostCandidate<'a>(pub(crate) &'a str); - -impl<'a> HostCandidate<'a> { - /// Return the wrapped host name. - const fn as_str(self) -> &'a str { - self.0 - } -} - /// Shared validation state for one host pattern. struct ValidationContext<'a> { /// Original pattern, used in error messages. @@ -242,22 +233,6 @@ impl HostPattern { wildcard, }) } - - /// Return whether a candidate host matches this pattern. Wildcard patterns - /// match subdomains only, never the apex domain itself. - pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool { - let host = candidate.as_str().to_ascii_lowercase(); - if self.wildcard { - // Wildcard patterns match only subdomains, not the apex domain. - // Example: "*.example.com" matches "sub.example.com" but not - // "example.com". - host.strip_suffix(&self.pattern) - .and_then(|prefix| prefix.strip_suffix('.')) - .is_some_and(|prefix| !prefix.is_empty()) - } else { - host == self.pattern - } - } } impl<'a> TryFrom<&'a str> for HostPattern { @@ -310,7 +285,7 @@ impl<'de> Deserialize<'de> for HostPattern { #[cfg(test)] mod tests { - //! Unit tests for host pattern parsing and wildcard matching. + //! Unit tests for host pattern parsing and normalisation. use super::*; use anyhow::{Result, ensure}; @@ -332,26 +307,6 @@ mod tests { Ok(()) } - #[rstest] - #[case("example.com", "example.com", true)] - #[case("example.com", "sub.example.com", false)] - #[case("*.example.com", "sub.example.com", true)] - #[case("*.example.com", "example.com", false)] - #[case("*.example.com", "deep.sub.example.com", true)] - #[case("*.example.com", "other.com", false)] - fn host_pattern_matches_expected( - #[case] pattern: &str, - #[case] host: &str, - #[case] expected: bool, - ) -> Result<()> { - let parsed = HostPattern::parse(pattern)?; - ensure!( - parsed.matches(HostCandidate(host)) == expected, - "expected match={expected} for {host} against {pattern}", - ); - Ok(()) - } - #[rstest] #[case("-example.com")] #[case("example-.com")] diff --git a/src/lib.rs b/src/lib.rs index 35a60787d..ab5591ed8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,7 @@ pub(crate) mod diagnostics; pub mod graph_view; pub mod hasher; pub mod hex; +mod host_matching; pub mod host_pattern; pub mod ir; mod json_envelope; diff --git a/src/stdlib/network/policy/mod.rs b/src/stdlib/network/policy/mod.rs index f68ac965b..5ddf9c3d2 100644 --- a/src/stdlib/network/policy/mod.rs +++ b/src/stdlib/network/policy/mod.rs @@ -9,7 +9,8 @@ use crate::localization::{self, LocalizedMessage, keys}; use thiserror::Error; use url::Url; -use crate::host_pattern::{HostCandidate, HostPattern, HostPatternError}; +use crate::host_matching::HostCandidate; +use crate::host_pattern::{HostPattern, HostPatternError}; /// Declarative allow- and deny-list policy for outbound network requests. /// From 3f2a9bc99e2c60e81355c2e037c918a954e03093 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 28 Aug 2026 16:37:49 +0200 Subject: [PATCH 02/11] Add build-slice validation coverage (#513) Protect the narrowed build-script CLI composition root with focused unit, parser-schema, property, and direct-rustc UI tests. Document the maintained UI boundary so later slice changes update its positive and negative fixtures deliberately. --- docs/developers-guide.md | 5 ++ src/cli/validation.rs | 23 ++++++ src/host_matching.rs | 65 +++++++++++++++ tests/build_module_slice_ui_tests.rs | 81 +++++++++++++++++++ tests/cli_tests/command_schema.rs | 76 +++++++++++++++++ tests/cli_tests/mod.rs | 10 ++- .../build_module_slice_runtime_module_fail.rs | 25 ++++++ tests/ui/build_module_slice_supported.rs | 23 ++++++ 8 files changed, 304 insertions(+), 4 deletions(-) create mode 100644 tests/build_module_slice_ui_tests.rs create mode 100644 tests/cli_tests/command_schema.rs create mode 100644 tests/ui/build_module_slice_runtime_module_fail.rs create mode 100644 tests/ui/build_module_slice_supported.rs diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 16d0f892b..74e4bece2 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1036,6 +1036,11 @@ module publicly. A dependency added outside the slice surfaces as a build-script compile error. Prefer moving the new code into a sibling module over widening the slice. +`tests/build_module_slice_ui_tests.rs` makes that boundary a direct-`rustc` +contract. Its positive fixture mirrors the four declared modules, while its +negative fixture imports `cli::discovery` and must fail with an unresolved +module diagnostic. Update the fixtures whenever the build-script slice changes. + ## Local build acceleration Debug builds and tests can optionally use the [`mold`] linker and the Cranelift diff --git a/src/cli/validation.rs b/src/cli/validation.rs index 4e025a51c..54a12a53f 100644 --- a/src/cli/validation.rs +++ b/src/cli/validation.rs @@ -19,3 +19,26 @@ pub(super) fn validation_error(key: &str, message: &str) -> Arc { message: message.to_owned(), }) } + +#[cfg(test)] +mod tests { + //! Unit tests for CLI validation limits and errors. + + use super::*; + + #[test] + fn max_jobs_matches_the_cli_contract() { + assert_eq!(MAX_JOBS, 64); + } + + #[test] + fn validation_error_preserves_its_key_and_message() { + let error = validation_error("jobs", "message"); + let OrthoError::Validation { key, message } = error.as_ref() else { + panic!("validation_error should construct OrthoError::Validation"); + }; + + assert_eq!(key, "jobs"); + assert_eq!(message, "message"); + } +} diff --git a/src/host_matching.rs b/src/host_matching.rs index bfa52bb6a..a50768c7f 100644 --- a/src/host_matching.rs +++ b/src/host_matching.rs @@ -42,8 +42,19 @@ mod tests { use super::*; use anyhow::{Result, ensure}; + use proptest::{prelude::*, test_runner::TestCaseError}; use rstest::rstest; + /// Generate one ASCII DNS-label-safe hostname component. + fn ascii_dns_label_strategy() -> impl Strategy { + "[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?" + } + + /// Parse a generated valid pattern without losing proptest's shrinking context. + fn parse_generated_pattern(pattern: &str) -> Result { + HostPattern::parse(pattern).map_err(|error| TestCaseError::fail(error.to_string())) + } + #[rstest] #[case("example.com", "example.com", true)] #[case("example.com", "sub.example.com", false)] @@ -51,6 +62,8 @@ mod tests { #[case("*.example.com", "example.com", false)] #[case("*.example.com", "deep.sub.example.com", true)] #[case("*.example.com", "other.com", false)] + #[case("example.com", "", false)] + #[case("example.com", "ÉXAMPLE.COM", false)] fn host_pattern_matches_expected( #[case] pattern: &str, #[case] host: &str, @@ -63,4 +76,56 @@ mod tests { ); Ok(()) } + + #[test] + fn host_matching_normalizes_ascii_candidates_only() -> Result<()> { + let pattern = HostPattern::parse("example.test")?; + + ensure!( + pattern.matches(HostCandidate("EXAMPLE.TEST")), + "ASCII case variants should match" + ); + ensure!( + !pattern.matches(HostCandidate("ÉXAMPLE.TEST")), + "to_ascii_lowercase should leave non-ASCII letters unchanged" + ); + Ok(()) + } + + proptest! { + #[test] + fn exact_patterns_match_ascii_case_insensitively( + label in ascii_dns_label_strategy(), + ) { + let pattern = format!("{label}.example.test"); + let candidate = pattern.to_ascii_uppercase(); + let parsed = parse_generated_pattern(&pattern)?; + + prop_assert!(parsed.matches(HostCandidate(&candidate))); + } + + #[test] + fn wildcard_patterns_match_every_nonempty_ascii_subdomain_prefix( + labels in prop::collection::vec(ascii_dns_label_strategy(), 1..5), + ) { + let prefix = labels.join("."); + let candidate = format!("{prefix}.example.test").to_ascii_uppercase(); + let parsed = parse_generated_pattern("*.example.test")?; + + prop_assert!(parsed.matches(HostCandidate(&candidate))); + prop_assert!(!parsed.matches(HostCandidate("example.test"))); + } + + #[test] + fn exact_patterns_reject_strict_suffixes_and_superdomains( + label in ascii_dns_label_strategy(), + ) { + let pattern = format!("{label}.example.test"); + let superdomain = format!("sub.{pattern}"); + let parsed = parse_generated_pattern(&pattern)?; + + prop_assert!(!parsed.matches(HostCandidate("example.test"))); + prop_assert!(!parsed.matches(HostCandidate(&superdomain))); + } + } } diff --git a/tests/build_module_slice_ui_tests.rs b/tests/build_module_slice_ui_tests.rs new file mode 100644 index 000000000..b62c5d465 --- /dev/null +++ b/tests/build_module_slice_ui_tests.rs @@ -0,0 +1,81 @@ +//! Direct-rustc UI tests for the `build.rs` CLI module slice. +//! +//! The fixtures mirror the inline `cli` composition root without compiling the +//! production modules. This keeps the negative case dependency-free while +//! making the declared-module boundary an explicit compiler contract. + +use std::{ + io, + path::{Path, PathBuf}, + process::{Command, Output}, +}; + +/// Verify the supported build-script module root compiles directly. +#[test] +fn supported_build_module_slice_compiles() -> io::Result<()> { + let output = compile_ui_fixture("tests/ui/build_module_slice_supported.rs")?; + if !output.status.success() { + return Err(io::Error::other(format!( + "the supported build module slice should compile:\n{}", + stderr(&output), + ))); + } + Ok(()) +} + +/// Verify runtime-only CLI modules remain absent from the build-script slice. +#[test] +fn runtime_module_import_is_rejected_by_the_build_module_slice() -> io::Result<()> { + let output = compile_ui_fixture("tests/ui/build_module_slice_runtime_module_fail.rs")?; + let standard_error = stderr(&output); + + if output.status.success() { + return Err(io::Error::other( + "the build module slice should reject a runtime-only module import", + )); + } + if !standard_error.contains("discovery") { + return Err(io::Error::other(format!( + "the compiler diagnostic should identify discovery as missing:\n{standard_error}", + ))); + } + if !standard_error.contains("unresolved import") && !standard_error.contains("could not find") { + return Err(io::Error::other(format!( + "the compiler diagnostic should explain the unresolved module:\n{standard_error}", + ))); + } + Ok(()) +} + +/// Compile one dependency-free module-slice fixture with the workspace rustc. +fn compile_ui_fixture(source: &str) -> io::Result { + let output_dir = tempfile::tempdir_in(manifest_dir().join("target"))?; + + Command::new(rustc()) + .arg("--edition=2024") + .arg("--crate-type=bin") + .arg("--emit=metadata") + .arg(manifest_dir().join(source)) + .arg("-o") + .arg(output_dir.path().join("build-module-slice-ui.rmeta")) + .output() +} + +/// Return the repository root supplied by Cargo for this test target. +fn manifest_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +/// Return the rustc executable selected for the workspace. +#[expect( + clippy::disallowed_methods, + reason = "Cargo supplies the rustc path for direct UI compilation; the test only reads the tool location" +)] +fn rustc() -> PathBuf { + std::env::var_os("RUSTC").map_or_else(|| Path::new("rustc").to_path_buf(), PathBuf::from) +} + +/// Render a compiler invocation's standard error for a test failure. +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} diff --git a/tests/cli_tests/command_schema.rs b/tests/cli_tests/command_schema.rs new file mode 100644 index 000000000..5842ca72a --- /dev/null +++ b/tests/cli_tests/command_schema.rs @@ -0,0 +1,76 @@ +//! Command-schema coverage for the Clap definitions moved into `cli::command`. +//! +//! These assertions stop at the localized parser boundary. They deliberately do +//! not exercise runner dispatch, so a failing case identifies a schema change +//! rather than runtime command behaviour. + +use anyhow::{Context, Result, ensure}; +use netsuke::cli::{BuildArgs, Commands, GraphArgs, HelpArgs, HelpTopic}; +use netsuke::cli_localization; +use std::path::PathBuf; +use std::sync::Arc; + +#[test] +fn omitted_subcommand_selects_the_default_build_command() -> Result<()> { + let localizer = Arc::from(cli_localization::build_localizer(None)); + let (parsed, _) = + netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer).context("parse CLI")?; + let command = parsed + .with_default_command() + .command + .context("default command should be present")?; + + ensure!( + command == Commands::Build(BuildArgs::default()), + "an omitted subcommand should select the default build command" + ); + Ok(()) +} + +#[test] +fn supported_commands_parse_to_their_schema_variants() -> Result<()> { + let localizer = Arc::from(cli_localization::build_localizer(None)); + let cases = [ + ( + vec!["netsuke", "build", "first", "second"], + Commands::Build(BuildArgs { + targets: vec![String::from("first"), String::from("second")], + }), + ), + (vec!["netsuke", "clean"], Commands::Clean), + ( + vec!["netsuke", "graph", "--html", "--output", "graph.html"], + Commands::Graph(GraphArgs { + html: true, + output: Some(PathBuf::from("graph.html")), + }), + ), + ( + vec!["netsuke", "generate", "--output", "generated.ninja"], + Commands::Generate { + output: Some(PathBuf::from("generated.ninja")), + }, + ), + ( + vec!["netsuke", "help", "targets"], + Commands::Help(HelpArgs { + topic: Some(HelpTopic::Targets), + }), + ), + ]; + + for (argv, expected) in cases { + let (parsed, _) = netsuke::cli::parse_with_localizer_from(argv.clone(), &localizer) + .with_context(|| format!("parse command schema for {argv:?}"))?; + let command = parsed + .with_default_command() + .command + .context("parsed command should be present")?; + + ensure!( + command == expected, + "command schema mismatch for {argv:?}: got {command:?}, expected {expected:?}" + ); + } + Ok(()) +} diff --git a/tests/cli_tests/mod.rs b/tests/cli_tests/mod.rs index 05ccbb760..7babfde7e 100644 --- a/tests/cli_tests/mod.rs +++ b/tests/cli_tests/mod.rs @@ -1,7 +1,3 @@ -//! Unit tests for CLI argument parsing and validation. -//! -//! This module exercises the command-line interface defined in `netsuke::cli`. - mod config_discovery; #[cfg(unix)] mod config_precedence_ladder; @@ -19,3 +15,9 @@ mod merge_probe; mod merge_targets_proptests; mod parsing; mod policy; + +//! Unit tests for CLI argument parsing and validation. +//! +//! This module exercises the command-line interface defined in `netsuke::cli`. + +mod command_schema; diff --git a/tests/ui/build_module_slice_runtime_module_fail.rs b/tests/ui/build_module_slice_runtime_module_fail.rs new file mode 100644 index 000000000..e8e1680ba --- /dev/null +++ b/tests/ui/build_module_slice_runtime_module_fail.rs @@ -0,0 +1,25 @@ +//! Compile-fail mirror of an invalid runtime import from the `build.rs` slice. + +mod cli { + //! Minimal declaration-only model of the build-script CLI slice. + + pub mod config { + //! Configuration schema stand-in. + } + + mod validation { + //! Validation helper stand-in. + } + + mod help { + //! Help-schema stand-in. + } + + mod command { + //! Command-schema stand-in. + } +} + +use cli::discovery; + +fn main() {} diff --git a/tests/ui/build_module_slice_supported.rs b/tests/ui/build_module_slice_supported.rs new file mode 100644 index 000000000..8877d0760 --- /dev/null +++ b/tests/ui/build_module_slice_supported.rs @@ -0,0 +1,23 @@ +//! Compile-pass mirror of the `build.rs` CLI module composition root. + +mod cli { + //! Minimal declaration-only model of the build-script CLI slice. + + pub mod config { + //! Configuration schema stand-in. + } + + mod validation { + //! Validation helper stand-in. + } + + mod help { + //! Help-schema stand-in. + } + + mod command { + //! Command-schema stand-in. + } +} + +fn main() {} From 8e4d36ce0fec7ea2880e3da8aa32ab407209f0cc Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 28 Aug 2026 19:44:18 +0200 Subject: [PATCH 03/11] Compile the real build-module slice (#513) Bind the direct-rustc UI fixtures to the production CLI paths and verify their declarations still match `build.rs`. Keep the runtime-module rejection meaningful by compiling the same real support graph in the positive and negative fixtures. --- docs/developers-guide.md | 3 +- tests/build_module_slice_ui_tests.rs | 213 +++++++++++++++--- .../build_module_slice_runtime_module_fail.rs | 38 ++-- tests/ui/build_module_slice_supported.rs | 42 ++-- 4 files changed, 233 insertions(+), 63 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 74e4bece2..3795d5a46 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1037,7 +1037,8 @@ A dependency added outside the slice surfaces as a build-script compile error. Prefer moving the new code into a sibling module over widening the slice. `tests/build_module_slice_ui_tests.rs` makes that boundary a direct-`rustc` -contract. Its positive fixture mirrors the four declared modules, while its +contract. Its fixtures compile the production module paths selected by +`build.rs`; the positive fixture mirrors the four declared modules, while the negative fixture imports `cli::discovery` and must fail with an unresolved module diagnostic. Update the fixtures whenever the build-script slice changes. diff --git a/tests/build_module_slice_ui_tests.rs b/tests/build_module_slice_ui_tests.rs index b62c5d465..c1dfa2d97 100644 --- a/tests/build_module_slice_ui_tests.rs +++ b/tests/build_module_slice_ui_tests.rs @@ -1,8 +1,14 @@ -//! Direct-rustc UI tests for the `build.rs` CLI module slice. +//! Direct-rustc UI tests for the production `build.rs` CLI module slice. //! -//! The fixtures mirror the inline `cli` composition root without compiling the -//! production modules. This keeps the negative case dependency-free while -//! making the declared-module boundary an explicit compiler contract. +//! The fixtures compile the production CLI modules and their direct support +//! graph, rather than declaration-only stand-ins. A small source assertion +//! keeps their composition root aligned with the inline `cli` module in +//! `build.rs`. + +#[path = "support/cargo_artifacts.rs"] +mod cargo_artifacts; +#[path = "support/rustc_response_file.rs"] +mod rustc_response_file; use std::{ io, @@ -10,26 +16,35 @@ use std::{ process::{Command, Output}, }; -/// Verify the supported build-script module root compiles directly. +/// The external crates required by the real build-script CLI slice. +const REQUIRED_EXTERNS: &[&str] = &["clap", "ortho_config", "serde", "thiserror", "tracing"]; + +/// The exact module declarations that the fixture mirrors from `build.rs`. +const BUILD_SLICE_MODULES: &[(&str, &str)] = &[ + ("config.rs", "pub mod config;"), + ("validation.rs", "mod validation;"), + ("help.rs", "mod help;"), + ("command.rs", "mod command;"), +]; + +/// Verify the production build-script module root and its runtime boundary. #[test] -fn supported_build_module_slice_compiles() -> io::Result<()> { - let output = compile_ui_fixture("tests/ui/build_module_slice_supported.rs")?; - if !output.status.success() { +fn production_build_module_slice_has_expected_boundary() -> io::Result<()> { + assert_fixture_matches_build_rs()?; + let dependencies = BuildSliceDependencies::build()?; + let supported = dependencies.compile("tests/ui/build_module_slice_supported.rs")?; + if !supported.status.success() { return Err(io::Error::other(format!( "the supported build module slice should compile:\n{}", - stderr(&output), + stderr(&supported), ))); } - Ok(()) -} -/// Verify runtime-only CLI modules remain absent from the build-script slice. -#[test] -fn runtime_module_import_is_rejected_by_the_build_module_slice() -> io::Result<()> { - let output = compile_ui_fixture("tests/ui/build_module_slice_runtime_module_fail.rs")?; - let standard_error = stderr(&output); + let runtime_import = + dependencies.compile("tests/ui/build_module_slice_runtime_module_fail.rs")?; + let standard_error = stderr(&runtime_import); - if output.status.success() { + if runtime_import.status.success() { return Err(io::Error::other( "the build module slice should reject a runtime-only module import", )); @@ -47,18 +62,149 @@ fn runtime_module_import_is_rejected_by_the_build_module_slice() -> io::Result<( Ok(()) } -/// Compile one dependency-free module-slice fixture with the workspace rustc. -fn compile_ui_fixture(source: &str) -> io::Result { - let output_dir = tempfile::tempdir_in(manifest_dir().join("target"))?; - - Command::new(rustc()) - .arg("--edition=2024") - .arg("--crate-type=bin") - .arg("--emit=metadata") - .arg(manifest_dir().join(source)) - .arg("-o") - .arg(output_dir.path().join("build-module-slice-ui.rmeta")) - .output() +/// The direct-rustc dependencies used by the production module fixtures. +struct BuildSliceDependencies { + /// Extern crates explicitly imported by the production module paths. + externs: Vec<(&'static str, PathBuf)>, + /// Directories containing transitive artefacts rustc resolves from metadata. + dependency_dirs: Vec, +} + +impl BuildSliceDependencies { + /// Build the crate and collect the artefacts required by the UI fixtures. + fn build() -> io::Result { + let output = Command::new(cargo()) + .arg("build") + .arg("--lib") + .arg("--manifest-path") + .arg(manifest_dir().join("Cargo.toml")) + .arg("--message-format=json") + .output()?; + if !output.status.success() { + return Err(io::Error::other(format!( + "building the production module dependencies failed:\n{}", + stderr(&output), + ))); + } + + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let externs = REQUIRED_EXTERNS + .iter() + .map(|name| { + let artefact = stdout + .lines() + .filter_map(|line| cargo_artifacts::library_path_in_message(line, name)) + .next_back() + .ok_or_else(|| { + io::Error::other(format!( + "cargo reported no {name} artefact for the build-slice fixture" + )) + })?; + Ok((*name, artefact)) + }) + .collect::>>()?; + let mut dependency_dirs = Vec::new(); + for parent in stdout + .lines() + .flat_map(cargo_artifacts::dependency_dirs_in_message) + { + if !dependency_dirs.contains(&parent) { + dependency_dirs.push(parent); + } + } + if dependency_dirs.is_empty() { + return Err(io::Error::other( + "cargo reported no dependency artefact directories for the build-slice fixture", + )); + } + + Ok(Self { + externs, + dependency_dirs, + }) + } + + /// Compile one fixture against the production modules with the workspace rustc. + fn compile(&self, source: &str) -> io::Result { + let output_dir = tempfile::tempdir_in(manifest_dir().join("target"))?; + let mut args = vec![ + String::from("--edition=2024"), + String::from("--crate-type=bin"), + String::from("--emit=metadata"), + manifest_dir().join(source).to_string_lossy().into_owned(), + ]; + args.extend(self.externs.iter().flat_map(|(name, path)| { + [ + String::from("--extern"), + format!("{name}={}", path.display()), + ] + })); + args.extend( + self.dependency_dirs + .iter() + .flat_map(|path| [String::from("-L"), format!("dependency={}", path.display())]), + ); + args.extend([ + String::from("-o"), + output_dir + .path() + .join("build-module-slice-ui.rmeta") + .to_string_lossy() + .into_owned(), + ]); + + let response = + rustc_response_file::write(output_dir.path(), "build-module-slice-ui.args", &args)?; + let mut command = Command::new(rustc()); + command.arg(response).envs(package_environment()); + command.output() + } +} + +/// Verify the fixture root still mirrors the module declarations in `build.rs`. +fn assert_fixture_matches_build_rs() -> io::Result<()> { + let build_script = test_support::fs::read_to_string(manifest_dir().join("build.rs"))?; + let slice_start = build_script + .find("#[path = \"src/cli\"]\nmod cli {") + .ok_or_else(|| io::Error::other("build.rs no longer declares its inline cli module"))?; + let declared_slice = build_script + .get(slice_start..) + .and_then(|slice| { + slice + .find("#[path = \"src/cli_localization.rs\"]") + .and_then(|slice_end| slice.get(..slice_end)) + }) + .ok_or_else(|| { + io::Error::other("could not locate the end of build.rs's cli module slice") + })?; + + for (path, declaration) in BUILD_SLICE_MODULES { + let expected = format!("#[path = \"{path}\"]\n {declaration}"); + if !declared_slice.contains(&expected) { + return Err(io::Error::other(format!( + "build.rs's cli slice no longer matches the UI fixture: missing {expected:?}", + ))); + } + } + if declared_slice.matches("#[path = ").count() != BUILD_SLICE_MODULES.len() + 1 { + return Err(io::Error::other( + "build.rs's cli slice contains a different set of path modules than the UI fixture", + )); + } + Ok(()) +} + +/// Supply the Cargo package variables consumed by Clap's command derives. +const fn package_environment() -> [(&'static str, &'static str); 7] { + [ + ("CARGO_PKG_NAME", env!("CARGO_PKG_NAME")), + ("CARGO_PKG_VERSION", env!("CARGO_PKG_VERSION")), + ("CARGO_PKG_AUTHORS", env!("CARGO_PKG_AUTHORS")), + ("CARGO_PKG_DESCRIPTION", env!("CARGO_PKG_DESCRIPTION")), + ("CARGO_PKG_HOMEPAGE", env!("CARGO_PKG_HOMEPAGE")), + ("CARGO_PKG_REPOSITORY", env!("CARGO_PKG_REPOSITORY")), + ("CARGO_PKG_LICENSE", env!("CARGO_PKG_LICENSE")), + ] } /// Return the repository root supplied by Cargo for this test target. @@ -66,6 +212,15 @@ fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } +/// Return the Cargo executable selected for the workspace. +#[expect( + clippy::disallowed_methods, + reason = "Cargo supplies the executable path for direct UI compilation; the test only reads the tool location" +)] +fn cargo() -> PathBuf { + std::env::var_os("CARGO").map_or_else(|| Path::new("cargo").to_path_buf(), PathBuf::from) +} + /// Return the rustc executable selected for the workspace. #[expect( clippy::disallowed_methods, diff --git a/tests/ui/build_module_slice_runtime_module_fail.rs b/tests/ui/build_module_slice_runtime_module_fail.rs index e8e1680ba..9d804d83c 100644 --- a/tests/ui/build_module_slice_runtime_module_fail.rs +++ b/tests/ui/build_module_slice_runtime_module_fail.rs @@ -1,23 +1,29 @@ -//! Compile-fail mirror of an invalid runtime import from the `build.rs` slice. +//! Compile-fail fixture for a runtime import outside the `build.rs` slice. -mod cli { - //! Minimal declaration-only model of the build-script CLI slice. - - pub mod config { - //! Configuration schema stand-in. - } +#[path = "../../src/locale_catalogues.rs"] +pub mod locale_catalogues; +#[path = "../../src/cli_localization.rs"] +mod cli_localization; +#[path = "../../src/localization/mod.rs"] +pub mod localization; +#[path = "../../src/host_pattern.rs"] +mod host_pattern; - mod validation { - //! Validation helper stand-in. - } +#[path = "../../src/cli"] +mod cli { + //! The production CLI modules compiled by `build.rs`. - mod help { - //! Help-schema stand-in. - } + #[path = "config.rs"] + pub mod config; + #[path = "validation.rs"] + mod validation; + #[path = "help.rs"] + mod help; + #[path = "command.rs"] + mod command; - mod command { - //! Command-schema stand-in. - } + pub use command::Cli; + pub use config::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; } use cli::discovery; diff --git a/tests/ui/build_module_slice_supported.rs b/tests/ui/build_module_slice_supported.rs index 8877d0760..9ff7c335c 100644 --- a/tests/ui/build_module_slice_supported.rs +++ b/tests/ui/build_module_slice_supported.rs @@ -1,23 +1,31 @@ -//! Compile-pass mirror of the `build.rs` CLI module composition root. +//! Compile-pass fixture for the production `build.rs` CLI module slice. -mod cli { - //! Minimal declaration-only model of the build-script CLI slice. - - pub mod config { - //! Configuration schema stand-in. - } +#[path = "../../src/locale_catalogues.rs"] +pub mod locale_catalogues; +#[path = "../../src/cli_localization.rs"] +mod cli_localization; +#[path = "../../src/localization/mod.rs"] +pub mod localization; +#[path = "../../src/host_pattern.rs"] +mod host_pattern; - mod validation { - //! Validation helper stand-in. - } +#[path = "../../src/cli"] +mod cli { + //! The production CLI modules compiled by `build.rs`. - mod help { - //! Help-schema stand-in. - } + #[path = "config.rs"] + pub mod config; + #[path = "validation.rs"] + mod validation; + #[path = "help.rs"] + mod help; + #[path = "command.rs"] + mod command; - mod command { - //! Command-schema stand-in. - } + pub use command::Cli; + pub use config::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; } -fn main() {} +fn main() { + let _ = ::command; +} From caaa7ec0d20b8334519590b552c11e6fc17c372b Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 28 Aug 2026 20:10:22 +0200 Subject: [PATCH 04/11] Repair rebased build-slice validation (#513) Restore the runtime CLI imports that the rebase lost while keeping the four-file build-script slice narrow. Track the help schema file, retain the UI boundary contract, and make command-schema coverage independent per command variant. --- build.rs | 1 + src/cli/config.rs | 11 --- src/cli/merge.rs | 3 +- src/cli/merge_observability.rs | 14 ++++ src/host_matching.rs | 5 ++ tests/cli_tests/command_schema.rs | 100 ++++++++++++----------- tests/cli_tests/mod.rs | 11 ++- tests/ui/build_module_slice_supported.rs | 1 + 8 files changed, 81 insertions(+), 65 deletions(-) diff --git a/build.rs b/build.rs index 06ed92fb1..d3f3362b2 100644 --- a/build.rs +++ b/build.rs @@ -174,6 +174,7 @@ fn emit_rerun_directives() { // Only the modules this script actually compiles need to trigger a rerun. println!("cargo:rerun-if-changed=src/cli/command.rs"); println!("cargo:rerun-if-changed=src/cli/config.rs"); + println!("cargo:rerun-if-changed=src/cli/help.rs"); println!("cargo:rerun-if-changed=src/cli/validation.rs"); println!("cargo:rerun-if-changed=src/host_pattern.rs"); println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION"); diff --git a/src/cli/config.rs b/src/cli/config.rs index 56f6ff56a..7de711bbf 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -285,17 +285,6 @@ const MAX_JOBS: usize = super::validation::MAX_JOBS; /// Fixed reason reported when merged configuration enables interactive input. pub(crate) const NO_INPUT_VALIDATION_REASON: &str = "no_input = false is unsupported because Netsuke has no interactive mode"; -/// Fixed reason reported when a merged parallel job count is out of range. -pub(crate) const JOBS_VALIDATION_REASON: &str = "job count is outside the supported range"; - -/// Return the bounded observability reason for a known validation key. -pub(crate) fn validation_rejection_reason(key: &str) -> Option<&'static str> { - match key { - "no_input" => Some(NO_INPUT_VALIDATION_REASON), - "jobs" => Some(JOBS_VALIDATION_REASON), - _ => None, - } -} /// Return whether `jobs` falls outside the accepted range. const fn jobs_out_of_bounds(jobs: usize) -> bool { jobs == 0 || jobs > MAX_JOBS diff --git a/src/cli/merge.rs b/src/cli/merge.rs index ed072fa55..82e5c1f2c 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -30,7 +30,7 @@ use serde::Serialize; use serde_json::{Map, Value, json}; use super::command::{BuildArgs, Cli, Commands}; -use super::config::{BuildConfig, CliConfig, validation_rejection_reason}; +use super::config::{BuildConfig, CliConfig}; use super::discovery::{ DiscoveredLayers, EnvProvider, StdEnvProvider, discover_file_layers, push_discovered_file_layers, @@ -39,6 +39,7 @@ use super::environment::EnvironmentLayer; use super::merge_input::{CachedMergeInput, MergeComposition}; use super::merge_observability::{ NoopMergeObserver, collect_override_leaf_paths, is_empty_configuration_value, + validation_rejection_reason, }; use super::validation::validation_error; use super::{MergeEvent, MergeObserver}; diff --git a/src/cli/merge_observability.rs b/src/cli/merge_observability.rs index cae166c8b..e88f2654a 100644 --- a/src/cli/merge_observability.rs +++ b/src/cli/merge_observability.rs @@ -6,11 +6,25 @@ use serde_json::Value; +use super::config::NO_INPUT_VALIDATION_REASON; + +/// Fixed reason reported when a merged parallel job count is out of range. +const JOBS_VALIDATION_REASON: &str = "job count is outside the supported range"; + /// Return whether a configuration-layer object contains no supplied settings. pub(crate) fn is_empty_configuration_value(value: &Value) -> bool { matches!(value, Value::Object(map) if map.is_empty()) } +/// Return the bounded observability reason for a known validation key. +pub(crate) fn validation_rejection_reason(key: &str) -> Option<&'static str> { + match key { + "no_input" => Some(NO_INPUT_VALIDATION_REASON), + "jobs" => Some(JOBS_VALIDATION_REASON), + _ => None, + } +} + /// A bounded event emitted by an explicitly supplied configuration observer. /// /// The event surface intentionally excludes configuration values and raw diff --git a/src/host_matching.rs b/src/host_matching.rs index a50768c7f..4214ebcc1 100644 --- a/src/host_matching.rs +++ b/src/host_matching.rs @@ -55,6 +55,7 @@ mod tests { HostPattern::parse(pattern).map_err(|error| TestCaseError::fail(error.to_string())) } + /// Verify representative exact and wildcard host matches. #[rstest] #[case("example.com", "example.com", true)] #[case("example.com", "sub.example.com", false)] @@ -77,6 +78,7 @@ mod tests { Ok(()) } + /// Verify candidate normalization changes ASCII letters only. #[test] fn host_matching_normalizes_ascii_candidates_only() -> Result<()> { let pattern = HostPattern::parse("example.test")?; @@ -93,6 +95,7 @@ mod tests { } proptest! { + /// Verify exact patterns match ASCII case variants. #[test] fn exact_patterns_match_ascii_case_insensitively( label in ascii_dns_label_strategy(), @@ -104,6 +107,7 @@ mod tests { prop_assert!(parsed.matches(HostCandidate(&candidate))); } + /// Verify wildcard patterns match every non-empty subdomain prefix. #[test] fn wildcard_patterns_match_every_nonempty_ascii_subdomain_prefix( labels in prop::collection::vec(ascii_dns_label_strategy(), 1..5), @@ -116,6 +120,7 @@ mod tests { prop_assert!(!parsed.matches(HostCandidate("example.test"))); } + /// Verify exact patterns reject strict suffixes and superdomains. #[test] fn exact_patterns_reject_strict_suffixes_and_superdomains( label in ascii_dns_label_strategy(), diff --git a/tests/cli_tests/command_schema.rs b/tests/cli_tests/command_schema.rs index 5842ca72a..ca5e39c0d 100644 --- a/tests/cli_tests/command_schema.rs +++ b/tests/cli_tests/command_schema.rs @@ -7,12 +7,20 @@ use anyhow::{Context, Result, ensure}; use netsuke::cli::{BuildArgs, Commands, GraphArgs, HelpArgs, HelpTopic}; use netsuke::cli_localization; +use ortho_config::Localizer; +use rstest::{fixture, rstest}; use std::path::PathBuf; use std::sync::Arc; -#[test] -fn omitted_subcommand_selects_the_default_build_command() -> Result<()> { - let localizer = Arc::from(cli_localization::build_localizer(None)); +#[fixture] +fn localizer() -> Arc { + Arc::from(cli_localization::build_localizer(None)) +} + +#[rstest] +fn omitted_subcommand_selects_the_default_build_command( + localizer: Arc, +) -> Result<()> { let (parsed, _) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer).context("parse CLI")?; let command = parsed @@ -27,50 +35,48 @@ fn omitted_subcommand_selects_the_default_build_command() -> Result<()> { Ok(()) } -#[test] -fn supported_commands_parse_to_their_schema_variants() -> Result<()> { - let localizer = Arc::from(cli_localization::build_localizer(None)); - let cases = [ - ( - vec!["netsuke", "build", "first", "second"], - Commands::Build(BuildArgs { - targets: vec![String::from("first"), String::from("second")], - }), - ), - (vec!["netsuke", "clean"], Commands::Clean), - ( - vec!["netsuke", "graph", "--html", "--output", "graph.html"], - Commands::Graph(GraphArgs { - html: true, - output: Some(PathBuf::from("graph.html")), - }), - ), - ( - vec!["netsuke", "generate", "--output", "generated.ninja"], - Commands::Generate { - output: Some(PathBuf::from("generated.ninja")), - }, - ), - ( - vec!["netsuke", "help", "targets"], - Commands::Help(HelpArgs { - topic: Some(HelpTopic::Targets), - }), - ), - ]; - - for (argv, expected) in cases { - let (parsed, _) = netsuke::cli::parse_with_localizer_from(argv.clone(), &localizer) - .with_context(|| format!("parse command schema for {argv:?}"))?; - let command = parsed - .with_default_command() - .command - .context("parsed command should be present")?; - - ensure!( - command == expected, - "command schema mismatch for {argv:?}: got {command:?}, expected {expected:?}" - ); +#[rstest] +#[case( + vec!["netsuke", "build", "first", "second"], + Commands::Build(BuildArgs { + targets: vec![String::from("first"), String::from("second")], + }) +)] +#[case(vec!["netsuke", "clean"], Commands::Clean)] +#[case( + vec!["netsuke", "graph", "--html", "--output", "graph.html"], + Commands::Graph(GraphArgs { + html: true, + output: Some(PathBuf::from("graph.html")), + }) +)] +#[case( + vec!["netsuke", "generate", "--output", "generated.ninja"], + Commands::Generate { + output: Some(PathBuf::from("generated.ninja")), } +)] +#[case( + vec!["netsuke", "help", "targets"], + Commands::Help(HelpArgs { + topic: Some(HelpTopic::Targets), + }) +)] +fn supported_commands_parse_to_their_schema_variants( + localizer: Arc, + #[case] argv: Vec<&str>, + #[case] expected: Commands, +) -> Result<()> { + let (parsed, _) = netsuke::cli::parse_with_localizer_from(argv.clone(), &localizer) + .with_context(|| format!("parse command schema for {argv:?}"))?; + let command = parsed + .with_default_command() + .command + .context("parsed command should be present")?; + + ensure!( + command == expected, + "command schema mismatch for {argv:?}: got {command:?}, expected {expected:?}" + ); Ok(()) } diff --git a/tests/cli_tests/mod.rs b/tests/cli_tests/mod.rs index 7babfde7e..3e10a7112 100644 --- a/tests/cli_tests/mod.rs +++ b/tests/cli_tests/mod.rs @@ -1,3 +1,8 @@ +//! Unit tests for CLI argument parsing and validation. +//! +//! This module exercises the command-line interface defined in `netsuke::cli`. + +mod command_schema; mod config_discovery; #[cfg(unix)] mod config_precedence_ladder; @@ -15,9 +20,3 @@ mod merge_probe; mod merge_targets_proptests; mod parsing; mod policy; - -//! Unit tests for CLI argument parsing and validation. -//! -//! This module exercises the command-line interface defined in `netsuke::cli`. - -mod command_schema; diff --git a/tests/ui/build_module_slice_supported.rs b/tests/ui/build_module_slice_supported.rs index 9ff7c335c..057dcf343 100644 --- a/tests/ui/build_module_slice_supported.rs +++ b/tests/ui/build_module_slice_supported.rs @@ -26,6 +26,7 @@ mod cli { pub use config::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy}; } +/// Compile the supported production module slice. fn main() { let _ = ::command; } From 542362ab19b2f43896b20705ab016d11fee16852 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 28 Aug 2026 20:40:23 +0200 Subject: [PATCH 05/11] Harden build-slice review contracts (#513) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the obsolete `build_support` façade and align module and developer documentation with the inline build-script slice. Normalize one terminal DNS dot before policy matching while preserving wildcard apex rejection. --- docs/developers-guide.md | 15 ++++++++----- src/cli/build_support.rs | 30 ------------------------- src/cli/command.rs | 10 +++++++++ src/cli/mod.rs | 9 ++++---- src/cli/preferences.rs | 48 ++++++++++++++++++++++++++++++++++++++++ src/cli/validation.rs | 14 ++++++++++++ src/host_matching.rs | 22 +++++++++++++++++- 7 files changed, 108 insertions(+), 40 deletions(-) delete mode 100644 src/cli/build_support.rs diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 3795d5a46..80375acdd 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -675,8 +675,12 @@ discovery otherwise uses capability-scoped canonicalization. Its small, dedicated path-normalization module, `netsuke::cli::discovery::paths`, remains narrowly excluded because `std::fs::canonicalize` preserves the absolute comparison keys and cross-directory symlink behaviour that `cap_std` rejects. -For man-page generation, the build script compiles the `cli::build_support` -parser subset and deliberately omits runtime discovery. The broader +For ordinary man-page and completion generation, the build script compiles its +inline `cli` facade: the four-file slice containing `src/cli/command.rs`, +`src/cli/config.rs`, `src/cli/help.rs`, and `src/cli/validation.rs`. The +`command.rs` module owns the Clap command schema and default-command behaviour, +including `Cli::with_default_command()`, while runtime discovery remains +deliberately outside the slice. The broader `netsuke::cli::discovery` module remains under the capability policy; no `build_script_build` exception is required. The behavioural step definitions, CLI integration tests, and shared workflow-reading helper that stage fixtures @@ -1012,9 +1016,10 @@ module naming exactly four files — `src/cli/command.rs`, `src/cli/config.rs`, That slice is a maintained boundary, not an accident: -- `src/cli/command.rs` holds Clap definitions only. Runtime behaviour on `Cli` - belongs in `src/cli/preferences.rs`, and the localization-aware parsing entry - point belongs in `src/cli/parser.rs`. +- `src/cli/command.rs` holds the Clap command schema and default-command + behaviour, including `Cli::with_default_command()`. Runtime behaviour on + `Cli` belongs in `src/cli/preferences.rs`, and the localization-aware parsing + entry point belongs in `src/cli/parser.rs`. - `src/cli/validation.rs` holds the shared limits and error constructor that `src/cli/config.rs` needs, so neither file has to reach up into `src/cli/mod.rs`. diff --git a/src/cli/build_support.rs b/src/cli/build_support.rs deleted file mode 100644 index 29adaafe2..000000000 --- a/src/cli/build_support.rs +++ /dev/null @@ -1,30 +0,0 @@ -//! Build-script composition root for the CLI parser. -//! -//! The release manual needs Clap metadata from [`Cli`], but not runtime -//! configuration discovery or command merging. Keeping this subset separate -//! prevents the build script from compiling those runtime-only boundaries. - -use ortho_config::OrthoError; -use std::sync::Arc; - -mod config; -mod help; -mod parser; -mod parsing; -mod policy_values; -mod value_parser; - -pub use config::{AccessibilityPolicy, CliConfig, ColourPolicy, EmojiPolicy, ProgressPolicy}; -pub use parser::Cli; -pub(crate) use parser::configured_command; - -/// Maximum number of jobs accepted by the CLI. -pub(super) const MAX_JOBS: usize = 64; - -/// Build a validation `OrthoError` with the given key and message. -pub(super) fn validation_error(key: &str, message: &str) -> Arc { - Arc::new(OrthoError::Validation { - key: key.to_owned(), - message: message.to_owned(), - }) -} diff --git a/src/cli/command.rs b/src/cli/command.rs index 9d45bd928..e3b5db69d 100644 --- a/src/cli/command.rs +++ b/src/cli/command.rs @@ -125,6 +125,16 @@ pub struct Cli { impl Cli { /// Apply the default command if none was specified. + /// + /// # Examples + /// + /// ``` + /// use netsuke::cli::{BuildArgs, Cli, Commands}; + /// + /// let command = Cli::default().with_default_command().command; + /// + /// assert_eq!(command, Some(Commands::Build(BuildArgs::default()))); + /// ``` #[must_use] pub fn with_default_command(mut self) -> Self { if self.command.is_none() { diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 4c504e356..ae0e19ab3 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -1,9 +1,10 @@ //! Command-line parsing plus layered CLI configuration support. //! -//! The parser-facing [`Cli`] type remains responsible for user-facing command -//! syntax, while [`CliConfig`] is the authoritative OrthoConfig-derived schema -//! used to merge defaults, configuration files, environment variables, and CLI -//! overrides into the runtime shape consumed by the runner. +//! `command` owns the user-facing [`Cli`] schema and default-command behaviour, +//! while `parser` localises that schema before parsing. [`CliConfig`] +//! is the authoritative OrthoConfig-derived schema used to merge defaults, +//! configuration files, environment variables, and CLI overrides into the +//! runtime shape consumed by the runner. //! //! The module is split so that `build.rs` can compile the Clap schema alone. //! `command`, [`config`], `help`, and `validation` form that self-contained diff --git a/src/cli/preferences.rs b/src/cli/preferences.rs index e8d8e0035..76cc7640c 100644 --- a/src/cli/preferences.rs +++ b/src/cli/preferences.rs @@ -12,6 +12,20 @@ use crate::theme::ThemePreference; impl Cli { /// Return the effective theme preference for emoji policy resolution. + /// + /// # Examples + /// + /// ``` + /// use netsuke::cli::{Cli, EmojiPolicy}; + /// use netsuke::theme::ThemePreference; + /// + /// let cli = Cli { + /// emoji: EmojiPolicy::Always, + /// ..Cli::default() + /// }; + /// + /// assert_eq!(cli.theme_preference(), Some(ThemePreference::Unicode)); + /// ``` #[must_use] pub const fn theme_preference(&self) -> Option { match self.emoji { @@ -22,6 +36,19 @@ impl Cli { } /// Return an explicit accessible-output override, if configured. + /// + /// # Examples + /// + /// ``` + /// use netsuke::cli::{AccessibilityPolicy, Cli}; + /// + /// let cli = Cli { + /// accessibility: AccessibilityPolicy::On, + /// ..Cli::default() + /// }; + /// + /// assert_eq!(cli.accessibility_override(), Some(true)); + /// ``` #[must_use] pub const fn accessibility_override(&self) -> Option { match self.accessibility { @@ -32,12 +59,33 @@ impl Cli { } /// Return whether interactive input is disabled. + /// + /// # Examples + /// + /// ``` + /// use netsuke::cli::Cli; + /// + /// assert!(Cli::default().no_input()); + /// ``` #[must_use] pub const fn no_input(&self) -> bool { self.interaction.no_input } /// Return whether progress summaries should be enabled. + /// + /// # Examples + /// + /// ``` + /// use netsuke::cli::{Cli, ProgressPolicy}; + /// + /// let cli = Cli { + /// progress: ProgressPolicy::Never, + /// ..Cli::default() + /// }; + /// + /// assert!(!cli.progress_enabled()); + /// ``` #[must_use] pub const fn progress_enabled(&self) -> bool { !matches!(self.progress, ProgressPolicy::Never) diff --git a/src/cli/validation.rs b/src/cli/validation.rs index 54a12a53f..97bca52fb 100644 --- a/src/cli/validation.rs +++ b/src/cli/validation.rs @@ -13,6 +13,20 @@ use std::sync::Arc; pub(super) const MAX_JOBS: usize = 64; /// Build a validation error for `key` with `message`. +/// +/// Produces [`OrthoError::Validation`] so callers can preserve the rejected +/// field and its diagnostic while propagating a shared error value. +/// +/// # Examples +/// +/// ```ignore +/// let error = validation_error("jobs", "must be positive"); +/// assert!(matches!( +/// error.as_ref(), +/// OrthoError::Validation { key, message } +/// if key == "jobs" && message == "must be positive" +/// )); +/// ``` pub(super) fn validation_error(key: &str, message: &str) -> Arc { Arc::new(OrthoError::Validation { key: key.to_owned(), diff --git a/src/host_matching.rs b/src/host_matching.rs index 4214ebcc1..543a74649 100644 --- a/src/host_matching.rs +++ b/src/host_matching.rs @@ -21,8 +21,26 @@ impl<'a> HostCandidate<'a> { impl HostPattern { /// Return whether `candidate` is covered by this pattern. + /// + /// A terminal DNS dot is ignored before matching. Wildcards still require + /// a non-empty subdomain prefix, so they do not match the apex. + /// + /// # Examples + /// + /// ```ignore + /// let exact = HostPattern::parse("example.test")?; + /// assert!(exact.matches(HostCandidate("example.test."))); + /// + /// let wildcard = HostPattern::parse("*.example.test")?; + /// assert!(wildcard.matches(HostCandidate("sub.example.test."))); + /// assert!(!wildcard.matches(HostCandidate("example.test."))); + /// # Ok::<(), anyhow::Error>(()) + /// ``` pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool { - let host = candidate.as_str().to_ascii_lowercase(); + let lowercased_host = candidate.as_str().to_ascii_lowercase(); + let host = lowercased_host + .strip_suffix('.') + .unwrap_or(&lowercased_host); if self.wildcard { // Wildcard patterns match only subdomains, not the apex domain. // Example: "*.example.com" matches "sub.example.com" but not @@ -63,6 +81,8 @@ mod tests { #[case("*.example.com", "example.com", false)] #[case("*.example.com", "deep.sub.example.com", true)] #[case("*.example.com", "other.com", false)] + #[case("example.com", "example.com.", true)] + #[case("*.example.com", "sub.example.com.", true)] #[case("example.com", "", false)] #[case("example.com", "ÉXAMPLE.COM", false)] fn host_pattern_matches_expected( From 0514e8d77a9bd391958a582266a579e232874d14 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 00:20:10 +0200 Subject: [PATCH 06/11] Normalise build-slice source assertions (#513) Accept CRLF checkouts before parsing the inline `build.rs` facade while retaining the exact module declarations and count. Add a CRLF regression alongside the direct-rustc boundary contract. --- tests/build_module_slice_ui_tests.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/build_module_slice_ui_tests.rs b/tests/build_module_slice_ui_tests.rs index c1dfa2d97..ab1b182e4 100644 --- a/tests/build_module_slice_ui_tests.rs +++ b/tests/build_module_slice_ui_tests.rs @@ -164,10 +164,16 @@ impl BuildSliceDependencies { /// Verify the fixture root still mirrors the module declarations in `build.rs`. fn assert_fixture_matches_build_rs() -> io::Result<()> { let build_script = test_support::fs::read_to_string(manifest_dir().join("build.rs"))?; - let slice_start = build_script + assert_fixture_matches_build_source(&build_script) +} + +/// Verify one build-script source text declares exactly the fixture module slice. +fn assert_fixture_matches_build_source(build_script: &str) -> io::Result<()> { + let normalised_build_script = build_script.replace("\r\n", "\n"); + let slice_start = normalised_build_script .find("#[path = \"src/cli\"]\nmod cli {") .ok_or_else(|| io::Error::other("build.rs no longer declares its inline cli module"))?; - let declared_slice = build_script + let declared_slice = normalised_build_script .get(slice_start..) .and_then(|slice| { slice @@ -194,6 +200,21 @@ fn assert_fixture_matches_build_rs() -> io::Result<()> { Ok(()) } +#[test] +fn fixture_contract_accepts_crlf_build_script_source() -> io::Result<()> { + let mut build_script = String::from("#[path = \"src/cli\"]\r\nmod cli {\r\n"); + for (path, declaration) in BUILD_SLICE_MODULES { + build_script.push_str(" #[path = \""); + build_script.push_str(path); + build_script.push_str("\"]\r\n "); + build_script.push_str(declaration); + build_script.push_str("\r\n"); + } + build_script.push_str("}\r\n#[path = \"src/cli_localization.rs\"]\r\n"); + + assert_fixture_matches_build_source(&build_script) +} + /// Supply the Cargo package variables consumed by Clap's command derives. const fn package_environment() -> [(&'static str, &'static str); 7] { [ From 067bb8b2a3c7fa3384214138e112c134f2744cbf Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 00:43:55 +0200 Subject: [PATCH 07/11] Replay cached merge events at application boundary (#513) Keep cached configuration merging free of observer callbacks by returning bounded events with the merge result and replaying them in `config_load`. Update callers, documentation and the spelling policy to preserve the established event order and required en-GB prose. --- build.rs | 8 +- ...ation-owned-configuration-observability.md | 11 ++- docs/developers-guide.md | 53 ++++++----- docs/netsuke-design.md | 57 ++++++----- docs/users-guide.md | 66 +++++++++++++ src/cli/command.rs | 2 + src/cli/discovery_merge_layers.rs | 16 ++-- src/cli/merge.rs | 95 +++++++++---------- src/cli/merge_observability.rs | 14 +-- src/cli/mod.rs | 4 +- src/cli/validation.rs | 2 + src/config_load.rs | 8 +- tests/cli_tests/merge_logging.rs | 23 +++-- .../config_cached_discovery_embedder_pass.rs | 10 +- typos.local.toml | 3 + typos.toml | 1 + 16 files changed, 230 insertions(+), 143 deletions(-) diff --git a/build.rs b/build.rs index d3f3362b2..3bf517317 100644 --- a/build.rs +++ b/build.rs @@ -26,15 +26,15 @@ const FALLBACK_DATE: &str = "1970-01-01"; // The build script recompiles a slice of the library as its own crate so that // `cli::Cli::command()` (used for man-page and completion generation) can be -// constructed, and so that the localization audit can read the declared key +// constructed, and so that the localisation audit can read the declared key // registry. // // The slice is named file by file rather than by pulling in `src/cli/mod.rs`, // because that would drag the whole `cli` subtree: configuration discovery, -// merging, diagnostics, and localized value parsing, none of which is +// merging, diagnostics, and localised value parsing, none of which is // reachable here. Runtime discovery is excluded deliberately: the build script // does not perform discovery, and compiling it here would pull its ambient -// canonicalization boundary into this separate compilation unit. Recompiling +// canonicalisation boundary into this separate compilation unit. Recompiling // only what is reachable keeps rustc's unused-item analysis meaningful here // instead of requiring module-wide `#[expect(dead_code)]` suppressions that // would also mask genuinely dead library code. @@ -42,7 +42,7 @@ const FALLBACK_DATE: &str = "1970-01-01"; // The library modules below are laid out to keep this slice small: // `src/cli/command.rs` holds command-schema and default-command behavior, // including `Cli::with_default_command`, with runtime preferences in -// `src/cli/preferences.rs` and the localization-aware parsing entry point in +// `src/cli/preferences.rs` and the localisation-aware parsing entry point in // `src/cli/parser.rs`; matching logic is split out of `src/host_pattern.rs` // into `src/host_matching.rs`. Adding a dependency on anything outside this // slice will surface here as a compile error, which is the intended signal. diff --git a/docs/adr-013-application-owned-configuration-observability.md b/docs/adr-013-application-owned-configuration-observability.md index d9e60b083..84b83ff3d 100644 --- a/docs/adr-013-application-owned-configuration-observability.md +++ b/docs/adr-013-application-owned-configuration-observability.md @@ -28,9 +28,11 @@ metric labels. Compose configuration observability at the CLI composition root. `config_load::resolve_configuration` orchestrates `cli::resolve_json_and_layers_outcome_with_env` and -`cli::merge_with_cached_file_layers`; those query functions do not install a -recorder or own configuration-load metrics. `src/observability.rs` owns the -phase-level vocabulary and classification helpers. +`cli::merge_with_cached_file_layers_with_observer`. That query returns bounded +merge events alongside the merge result; the application replays them through +`TracingMergeObserver`. These query functions neither install a recorder nor +invoke observers or own configuration-load metrics. `src/observability.rs` owns +the phase-level vocabulary and classification helpers. Both aggregate and phase-level configuration-load timing receive the same `&impl monotony::MonotonicClock` seam. Production supplies @@ -109,7 +111,8 @@ may include both phase-level and startup-attempt entries. - Configuration-load orchestration: [`src/config_load.rs`](../src/config_load.rs), which composes `cli::resolve_json_and_layers_outcome_with_env` and - `cli::merge_with_cached_file_layers` + `cli::merge_with_cached_file_layers_with_observer`, then replays its bounded + events through `cli::TracingMergeObserver` - Configuration query implementations: [`src/cli/diag.rs`](../src/cli/diag.rs) and [`src/cli/merge.rs`](../src/cli/merge.rs) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 80375acdd..cb66a1b51 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1018,7 +1018,7 @@ That slice is a maintained boundary, not an accident: - `src/cli/command.rs` holds the Clap command schema and default-command behaviour, including `Cli::with_default_command()`. Runtime behaviour on - `Cli` belongs in `src/cli/preferences.rs`, and the localization-aware parsing + `Cli` belongs in `src/cli/preferences.rs`, and the localisation-aware parsing entry point belongs in `src/cli/parser.rs`. - `src/cli/validation.rs` holds the shared limits and error constructor that `src/cli/config.rs` needs, so neither file has to reach up into @@ -3005,10 +3005,11 @@ discovering and loading the same configuration files more than once. At the application composition boundary, call `DiscoveryOutcome::emit_diagnostics()` after tracing is configured, then consume the outcome with `into_layers()` and construct a `CachedMergeInput`. Pass that input to -`merge_with_cached_file_layers_with_observer` with a `MergeObserver`, such as -`TracingMergeObserver`, for the full merge. This preserves diagnostics from the -same discovery pass while avoiding repeated file loading and keeps observation -outside the merge query. +`merge_with_cached_file_layers_with_observer` for the full merge; it returns +bounded events alongside the result. Replay those events through a +`MergeObserver`, such as `TracingMergeObserver`. This preserves diagnostics +from the same discovery pass while avoiding repeated file loading and keeps +observation outside the merge query. #### Cached merge API (unstable) @@ -3016,10 +3017,10 @@ Programs using Netsuke's unstable Rust API can retain the layers from one discovery pass and observe the subsequent merge. Construct `CachedMergeInput::new(cli, matches, env, discovered)` with the parsed CLI values, an injected `ConfigEnvProvider`, and `DiscoveryOutcome::into_layers()`; -then pass it to -`cli::merge_with_cached_file_layers_with_observer(input, &mut observer)`. -The application uses `TracingMergeObserver`, while another caller can provide -its own `MergeObserver` implementation. Observers receive bounded +then pass it to `cli::merge_with_cached_file_layers_with_observer(input)`. The +function returns the merge result alongside bounded events; replay those events +through `MergeObserver`, such as `TracingMergeObserver`. Another caller can +provide its own `MergeObserver` implementation. Observers receive bounded `MergeEvent` values: layer application and failure states, file `path_hash` and layer counts, CLI override leaf keys, and validation `key`/`reason` fields. Configuration values and raw paths are never included. Ordinary @@ -3047,10 +3048,12 @@ with a large nested configuration payload. It protects the ownership transfer that avoids copying complete `MergeLayer` values before the cached merge. The standalone `merge_with_config_and_env` path performs discovery and -delegates to the ordinary, no-op-observer merge query. It does not replay -retained discovery diagnostics or emit merge tracing. `merge_with_config` is +delegates to the ordinary merge query, which discards its collected events. It +does not replay retained discovery diagnostics or emit merge tracing. +`merge_with_config` is the process-environment wrapper around that path. The application startup path -replays discovery diagnostics and injects `TracingMergeObserver` explicitly. +replays discovery diagnostics and returned merge events through +`TracingMergeObserver` explicitly. Deferred bounded discovery diagnostics are replay metadata only. Discovery errors remain owned by `DiscoveredLayers` and are handled by the diagnostic @@ -3090,9 +3093,9 @@ Configuration merge helpers: - `discover_file_layers(cli, env) -> DiscoveryOutcome` performs one discovery pass and retains the discovered layers, discovery errors and bounded deferred diagnostics for the diagnostic and merge callers. -- `push_discovered_file_layers(composer, errors, discovered, observer) -> ()` +- `push_discovered_file_layers(composer, errors, discovered, events) -> ()` transfers the retained layers and discovery errors into the full merge - composition while reporting bounded file-layer events to the observer. + composition while collecting bounded file-layer events for replay. - `collect_file_layers_with_normalizer_and_trace(directory, normalizer, env_source)` runs the one discovery pass with the injected path normalizer and environment source, and retains bounded project-scope trace metadata for deferred @@ -3106,10 +3109,10 @@ Configuration merge helpers: - `merge_with_cached_file_layers(cli, matches, env, discovered)` consumes the discovered layers without rediscovery and uses no-op observation. - `CachedMergeInput::new(cli, matches, env, discovered)` packages parsed input - and cached layers for an observer-enabled merge. -- `merge_with_cached_file_layers_with_observer(input, observer)` consumes the - cached input and reports bounded `MergeEvent` values to the supplied - `MergeObserver`. + and cached layers for the bounded-event merge query. +- `merge_with_cached_file_layers_with_observer(input)` consumes the cached input + and returns the merge result alongside bounded `MergeEvent` values for + application-side replay. - `is_empty_value(value: &serde_json::Value) -> bool` detects an empty CLI override object. - `retain_layers_and_resolve_json(layers)` transfers each owned file-layer @@ -3185,7 +3188,7 @@ These ordinary merge and JSON-resolution queries have no tracing side effects. The startup boundary obtains the cached layers, replays their deferred discovery diagnostics, and uses `CachedMergeInput::new` with `merge_with_cached_file_layers_with_observer` when it needs the bounded merge -events. A caller supplying its own `MergeObserver` can consume those events +events. A caller supplying its own `MergeObserver` can replay those events without installing a global subscriber. The `cli` module re-exports this trait publicly as `ConfigEnvProvider` (and @@ -3532,10 +3535,10 @@ metrics are recorded by `config_load::resolve_configuration`, which receives a `resolve_json_mode_or_exit` and `merge_cli_or_exit`. The diagnostic-mode helper resolves and caches discovered layers with `cli::resolve_json_and_layers_outcome_with_env`; the merge helper passes those -cached layers to `cli::merge_with_cached_file_layers_with_observer` with a -`cli::TracingMergeObserver` for the full merge. The boundary replays deferred -discovery diagnostics before that merge; the ordinary query helpers do not emit -tracing themselves. +cached layers to `cli::merge_with_cached_file_layers_with_observer`, then +replays the returned merge events through `cli::TracingMergeObserver`. The +boundary replays deferred discovery diagnostics before that merge; the +ordinary query helpers do not emit tracing themselves. Phase-level metrics are composed in `src/observability.rs` around those two operations. @@ -4436,8 +4439,8 @@ modules and must not add unbounded configuration detail to these series. The public `cli::MergeObserver` seam carries bounded `cli::MergeEvent` values from `merge_with_cached_file_layers_with_observer`. The application supplies `cli::TracingMergeObserver` from `config_load::resolve_configuration`; direct -callers of the ordinary merge queries use no-op observation and emit no -tracing. Custom observers may consume the bounded events, which exclude raw +callers of the ordinary merge queries discard their collected events and emit +no tracing. Custom observers may consume the bounded events, which exclude raw configuration values and paths. Keep observer ownership at the application boundary rather than installing a subscriber in a query. diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index bb3606ec2..a7123412f 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2735,7 +2735,7 @@ the targets listed in the `defaults` section of the manifest are built. ### 8.4 Design Decisions The parser-facing `Cli` type is now defined in `src/cli/command.rs`, with the -localization-aware parsing entry point in `src/cli/parser.rs` and the runtime +localisation-aware parsing entry point in `src/cli/parser.rs` and the runtime preference accessors in `src/cli/preferences.rs`, while layered configuration lives in a dedicated `CliConfig` struct derived with OrthoConfig in `src/cli/config.rs`. The top-level `src/cli/mod.rs` module @@ -2782,15 +2782,17 @@ presence bits for path metadata so events can be replayed after discovery. Those deferred trace events never retain or emit filenames or raw paths. The application-owned `MergeObserver` seam receives bounded `MergeEvent` values -from `merge_with_cached_file_layers_with_observer`. Production supplies -`TracingMergeObserver` from `config_load::resolve_configuration`; callers that -use the ordinary `merge_with_config*` or `merge_with_cached_file_layers` query -APIs receive no observation effects. Callers that need merge events may provide -their own observer, but the event surface excludes raw configuration values and -paths. CLI events contain override keys only; file-layer events use bounded -path hashes; validation events use fixed `key` and `reason` fields. The -application adjusts its tracing filter before this merge and turns it off for -JSON diagnostics, preserving machine-readable stderr. +returned alongside the merge result by +`merge_with_cached_file_layers_with_observer`. Production replays those events +through `TracingMergeObserver` from `config_load::resolve_configuration`; +callers that use the ordinary `merge_with_config*` or +`merge_with_cached_file_layers` query APIs receive no observation effects. +Callers that need merge events may replay them through their own observer, but +the event surface excludes raw configuration values and paths. CLI events +contain override keys only; file-layer events use bounded path hashes; +validation events use fixed `key` and `reason` fields. The application adjusts +its tracing filter before this merge and turns it off for JSON diagnostics, +preserving machine-readable stderr. CLI help and clap errors are localized via Fluent resources; locale resolution is handled in `src/locale_resolution.rs` in two phases. Before the @@ -2824,9 +2826,10 @@ this decision is recorded in `config_load::resolve_configuration` owns the startup-attempt measurement: it resolves diagnostic mode with `cli::resolve_json_and_layers_outcome_with_env`, then replays the outcome's deferred diagnostics and passes the cached layers to -`cli::merge_with_cached_file_layers_with_observer` with -`cli::TracingMergeObserver` for the full merge. The ordinary query functions do -not install a recorder or emit tracing. `src/observability.rs` owns the phase +`cli::merge_with_cached_file_layers_with_observer`. That query returns bounded +merge events alongside the result, which `config_load::resolve_configuration` +replays through `cli::TracingMergeObserver`. The ordinary query functions do not +install a recorder or emit tracing. `src/observability.rs` owns the phase recorder and bounded phase/outcome vocabulary, while `src/config_load.rs` owns the startup-attempt series. The application installs an in-process `DebuggingRecorder`; it does not open a metrics listener as a side effect of a @@ -3033,12 +3036,14 @@ normalizer and environment source, retaining bounded project-scope trace metadata. The normalizer canonicalizes comparison keys so equivalent project path spellings de-duplicate to one layer. `DiscoveryOutcome::into_layers()` transfers the same discovered layers to -`merge_with_cached_file_layers_with_observer(...)`, which consumes them for the -full merge and prevents a second discovery pass. The standalone +`merge_with_cached_file_layers_with_observer(...)`, which returns bounded merge +events alongside the full merge result and prevents a second discovery pass. +The standalone `merge_with_config_and_env(...)` path performs discovery and delegates to the -ordinary no-op-observer merge query; it does not replay retained diagnostics or -emit merge tracing. The application startup boundary replays the diagnostics -and injects `TracingMergeObserver` explicitly. +ordinary merge query, which discards its collected events; it does not replay +retained diagnostics or emit merge tracing. The application startup boundary +replays the diagnostics +and the returned merge events through `TracingMergeObserver` explicitly. Deferred bounded discovery diagnostics are retained only for replay after the startup tracing boundary is configured. They do not contain raw paths or file @@ -3144,11 +3149,13 @@ manual flag repetition. selectors before automatic discovery so missing or invalid explicit files remain hard errors. - The `merge_with_config_and_env()` function in `src/cli/merge.rs` performs - discovery and delegates to the ordinary no-op-observer merge query. The + discovery and delegates to the ordinary merge query, which discards its + collected events. The application startup boundary replays retained bounded diagnostics and calls - `merge_with_cached_file_layers_with_observer(...)` with - `TracingMergeObserver` to merge defaults, discovered layers, environment - variables via Figment and CLI overrides extracted from `ArgMatches`. + `merge_with_cached_file_layers_with_observer(...)` to merge defaults, + discovered layers, environment variables via Figment and CLI overrides + extracted from `ArgMatches`, then replays its returned events through + `TracingMergeObserver`. - The `config_discovery()` function uses OrthoConfig's builder API with the application name, injected discovery environment, and optional project-root anchor, relying on OrthoConfig's platform-specific defaults for standard @@ -3160,9 +3167,9 @@ manual flag repetition. environment. Startup obtains a `DiscoveryOutcome` from `resolve_json_and_layers_outcome_with_env`, emits its deferred diagnostics, then passes `into_layers()` to - `merge_with_cached_file_layers_with_observer` with - `TracingMergeObserver`, so file discovery and loading happen once while - merge observation stays at the application boundary. OrthoConfig discovery + `merge_with_cached_file_layers_with_observer`, replaying its returned events + through `TracingMergeObserver`, so file discovery and loading happen once + while merge observation stays at the application boundary. 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 diff --git a/docs/users-guide.md b/docs/users-guide.md index c2ffd6e5d..b1170963d 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1119,6 +1119,72 @@ be loaded cannot enable diagnostics because configuration merging has not completed. JSON mode suppresses the tracing and snapshot so stderr remains one machine-readable diagnostic document. + +#### Cached merge API (unstable) + +Programs using Netsuke's unstable Rust API can retain the layers from one +discovery pass and observe the subsequent merge. Construct +`CachedMergeInput::new(cli, matches, env, discovered)` with the parsed CLI +values, an injected `ConfigEnvProvider`, and `DiscoveryOutcome::into_layers()`; +then pass it to `cli::merge_with_cached_file_layers_with_observer(input)`. The +function returns the merge result alongside bounded events; replay those events +through `MergeObserver`, such as `TracingMergeObserver`. Another caller can +provide its own `MergeObserver` implementation. Observers receive bounded +`MergeEvent` values: layer application and failure states, file `path_hash` +and layer counts, CLI override leaf keys, and validation `key`/`reason` fields. +Configuration values and raw paths are never included. Ordinary +`merge_with_config*` and `merge_with_cached_file_layers` calls discard their +collected events and do not emit merge tracing. + +If an explicit file cannot be loaded, the warning records `failure_kind` as +`Missing` or `LoadError`. Verbose tracing uses only `path_hash` and +`path_present`; it never exposes a file name or full path. The unkeyed +`path_hash` is only a correlation identifier: it does not protect a guessable +path from disclosure. + +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. + +For a terminal human-mode failure in either the early diagnostic-mode +preference pass or the full `config_merge` phase, the +`configuration load failed` event includes bounded `operation` and +`error_category` fields. JSON mode instead emits the diagnostic document. +Passing `--verbose` additionally emits one final `metrics snapshot` debug event +before Netsuke exits. The snapshot is an in-process diagnostic record, not a +metrics listener or a Prometheus endpoint. After a successful configuration +merge, verbosity can also come from `NETSUKE_VERBOSE` or `verbose = true` in a +configuration file. A configuration failure before that merge uses only CLI +`--verbose`. + +It includes the bounded configuration-load series: + +- `netsuke_config_load_total`, with `outcome=success` or `outcome=failure`. +- `netsuke_config_load_duration_seconds`, with one sample for the startup + configuration-load attempt. +- The phase-level `config_load_total` and + `config_load_duration_seconds` entries, labelled with `phase=diag_mode` or + `phase=merge`; the counter also carries the bounded outcome value. +- `netsuke_cli_config_discovery_total`, with `outcome=success` or + `outcome=error`, and `netsuke_cli_config_discovery_duration_seconds`, which + records the cached discovery pass duration. + +For example, a missing explicit file reports the actionable error first, then +the bounded tracing fields and the snapshot (timestamps and metric values vary): + + + +```plaintext +Configuration file error in 'missing.toml': explicit configuration file not found +ERROR ... configuration load failed operation="diag_mode_resolution" error_category="io" +DEBUG ... metrics snapshot metrics=[...] +``` + +The snapshot is available for a configuration failure when `--verbose` was +supplied on the command line. A `verbose = true` setting in a file that cannot +be loaded cannot enable diagnostics because configuration merging has not +completed. JSON mode suppresses the tracing and snapshot so stderr remains one +machine-readable diagnostic document. #### Bounded configuration metrics Configuration loading is recorded as two bounded metric series, both emitted in diff --git a/src/cli/command.rs b/src/cli/command.rs index e3b5db69d..2b3f1e455 100644 --- a/src/cli/command.rs +++ b/src/cli/command.rs @@ -145,6 +145,7 @@ impl Cli { } impl Default for Cli { + /// Construct default CLI values with the `build` command selected. fn default() -> Self { Self { file: CliConfig::default_manifest_path(), @@ -179,6 +180,7 @@ pub struct InteractionArgs { } impl Default for InteractionArgs { + /// Construct interaction defaults that reject prompts unless explicitly enabled. fn default() -> Self { Self { no_input: true } } diff --git a/src/cli/discovery_merge_layers.rs b/src/cli/discovery_merge_layers.rs index aa4ed49ce..4ee8fc68b 100644 --- a/src/cli/discovery_merge_layers.rs +++ b/src/cli/discovery_merge_layers.rs @@ -7,34 +7,32 @@ use ortho_config::MergeComposer; use std::sync::Arc; use super::{DiscoveredLayers, diagnostics::short_hash}; -use crate::cli::{MergeEvent, MergeObserver}; +use crate::cli::MergeEvent; /// Add discovered file layers to the supplied merge composition. /// /// This helper belongs only to the cached merge boundary. It appends to the /// caller-owned composer and error collection, and never discovers layers or /// completes a partial merge. -pub(crate) fn push_discovered_file_layers( +pub(crate) fn push_discovered_file_layers( composer: &mut MergeComposer, errors: &mut Vec>, discovered: DiscoveredLayers, - observer: &mut O, -) where - O: MergeObserver + ?Sized, -{ + events: &mut Vec, +) { let (layers, discovery_errors) = discovered.into_parts(); if discovery_errors.is_empty() { - observer.observe(MergeEvent::FileLayersCollected { + events.push(MergeEvent::FileLayersCollected { layer_count: layers.len(), }); } else { - observer.observe(MergeEvent::FileLayerCollectionFailed { + events.push(MergeEvent::FileLayerCollectionFailed { error_count: discovery_errors.len(), }); } errors.extend(discovery_errors); for layer in layers { - observer.observe(MergeEvent::FileLayerApplied { + events.push(MergeEvent::FileLayerApplied { path_hash: layer .path() .map(|path| short_hash(path.as_str().as_bytes())), diff --git a/src/cli/merge.rs b/src/cli/merge.rs index 82e5c1f2c..6e0e78156 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -29,6 +29,7 @@ use serde::Serialize; use serde_json::{Map, Value, json}; +use super::MergeEvent; use super::command::{BuildArgs, Cli, Commands}; use super::config::{BuildConfig, CliConfig}; use super::discovery::{ @@ -38,11 +39,9 @@ use super::discovery::{ use super::environment::EnvironmentLayer; use super::merge_input::{CachedMergeInput, MergeComposition}; use super::merge_observability::{ - NoopMergeObserver, collect_override_leaf_paths, is_empty_configuration_value, - validation_rejection_reason, + collect_override_leaf_paths, is_empty_configuration_value, validation_rejection_reason, }; use super::validation::validation_error; -use super::{MergeEvent, MergeObserver}; /// Merge discovered configuration layers over parsed CLI input. /// @@ -88,28 +87,25 @@ pub fn merge_with_cached_file_layers( env: &impl EnvProvider, discovered: DiscoveredLayers, ) -> OrthoResult { - let mut observer = NoopMergeObserver; let input = CachedMergeInput::new(cli, matches, env, discovered); - merge_with_cached_file_layers_with_observer(input, &mut observer) + let (merged, _) = merge_with_cached_file_layers_with_observer(input); + merged } -/// Merge cached configuration layers and report bounded events to `observer`. +/// Merge cached configuration layers and collect bounded merge events. /// -/// Application adapters opt into observability by supplying an observer. The -/// ordinary merge functions use a no-op observer so they remain side-effect -/// free and reusable in non-CLI contexts. +/// The returned events preserve merge and validation ordering without invoking +/// an observer from the query. Application adapters decide whether and how to +/// replay them after the query completes. /// -/// # Errors -/// -/// Returns an [`ortho_config::OrthoError`] if layer composition or merging -/// fails. -pub fn merge_with_cached_file_layers_with_observer( +/// The first tuple member contains either the merged [`Cli`] or an +/// [`ortho_config::OrthoError`]. The events remain available in either case so +/// an adapter can report a validation rejection before handling the error. +pub fn merge_with_cached_file_layers_with_observer( input: CachedMergeInput<'_, E>, - observer: &mut O, -) -> OrthoResult +) -> (OrthoResult, Vec) where E: EnvProvider + ?Sized, - O: MergeObserver + ?Sized, { let CachedMergeInput { cli, @@ -118,38 +114,40 @@ where discovered, } = input; let mut composition = MergeComposition::new(); + let mut events = Vec::new(); - push_defaults_layer(&mut composition, observer); + push_defaults_layer(&mut composition, &mut events); push_discovered_file_layers( &mut composition.composer, &mut composition.errors, discovered, - observer, + &mut events, ); - push_environment_layer(env, &mut composition, observer); - push_cli_layer(cli, matches, &mut composition, observer); + push_environment_layer(env, &mut composition, &mut events); + push_cli_layer(cli, matches, &mut composition, &mut events); - let merged = composition - .into_merge_result() - .inspect_err(|error| observe_validation_rejection(observer, error.as_ref()))?; - Ok(apply_config(cli, merged)) + let merged = match composition.into_merge_result() { + Ok(config) => Ok(apply_config(cli, config)), + Err(error) => { + collect_validation_rejection(&mut events, error.as_ref()); + Err(error) + } + }; + (merged, events) } /// Push the default configuration layer and retain any serialization failure. /// /// This helper belongs only to the cached merge boundary: it must append to the /// caller's shared composer and error collection rather than finish a merge. -fn push_defaults_layer(composition: &mut MergeComposition, observer: &mut O) -where - O: MergeObserver + ?Sized, -{ +fn push_defaults_layer(composition: &mut MergeComposition, events: &mut Vec) { match sanitize_value(&CliConfig::default()) { Ok(value) => { - observer.observe(MergeEvent::DefaultsApplied); + events.push(MergeEvent::DefaultsApplied); composition.composer.push_defaults(value); } Err(err) => { - observer.observe(MergeEvent::DefaultsFailed); + events.push(MergeEvent::DefaultsFailed); composition.errors.push(err); } } @@ -159,25 +157,23 @@ where /// /// This helper belongs only to the cached merge boundary and never reads the /// process environment: callers supply the environment adapter explicitly. -fn push_environment_layer( +fn push_environment_layer( env: &(impl EnvProvider + ?Sized), composition: &mut MergeComposition, - observer: &mut O, -) where - O: MergeObserver + ?Sized, -{ + events: &mut Vec, +) { match Figment::from(EnvironmentLayer::new(env.entries())) .extract::() .into_ortho_merge() { Ok(value) => { - observer.observe(MergeEvent::EnvironmentApplied { + events.push(MergeEvent::EnvironmentApplied { is_empty: is_empty_configuration_value(&value), }); composition.composer.push_environment(value); } Err(err) => { - observer.observe(MergeEvent::EnvironmentFailed); + events.push(MergeEvent::EnvironmentFailed); composition.errors.push(err); } } @@ -187,39 +183,34 @@ fn push_environment_layer( /// /// This helper is limited to the cached merge boundary because only that /// boundary owns the shared layer order and accumulated error collection. -fn push_cli_layer( +fn push_cli_layer( cli: &Cli, matches: &ArgMatches, composition: &mut MergeComposition, - observer: &mut O, -) where - O: MergeObserver + ?Sized, -{ + events: &mut Vec, +) { match cli_overrides_from_matches(cli, matches) { Ok(value) if !is_empty_configuration_value(&value) => { // Values may echo user-supplied paths or host lists, so records // identify only the keys that were explicitly overridden. - observer.observe(MergeEvent::CliOverridesApplied { + events.push(MergeEvent::CliOverridesApplied { override_keys: collect_override_leaf_paths(&value), }); composition.composer.push_cli(value); } - Ok(_) => observer.observe(MergeEvent::CliOverridesAbsent), + Ok(_) => events.push(MergeEvent::CliOverridesAbsent), Err(err) => { - observer.observe(MergeEvent::CliOverridesFailed); + events.push(MergeEvent::CliOverridesFailed); composition.errors.push(err); } } } -/// Forward known validation rejections to the explicit observer with fixed data. -fn observe_validation_rejection(observer: &mut O, error: &OrthoError) -where - O: MergeObserver + ?Sized, -{ +/// Collect a bounded event for a known validation rejection. +fn collect_validation_rejection(events: &mut Vec, error: &OrthoError) { if let OrthoError::Validation { key, .. } = error && let Some(reason) = validation_rejection_reason(key) { - observer.observe(MergeEvent::ValidationRejected { + events.push(MergeEvent::ValidationRejected { key: key.clone(), reason, }); diff --git a/src/cli/merge_observability.rs b/src/cli/merge_observability.rs index e88f2654a..e504918c8 100644 --- a/src/cli/merge_observability.rs +++ b/src/cli/merge_observability.rs @@ -1,8 +1,8 @@ //! Explicit observability adapter for configuration merging. //! -//! Merge queries accept a [`MergeObserver`] from their application boundary. -//! Production supplies [`TracingMergeObserver`], while direct callers use the -//! no-op implementation and remain free of logging side effects. +//! Merge queries return [`MergeEvent`] values to their application boundary. +//! Production replays them through [`TracingMergeObserver`], while direct +//! callers remain free of logging side effects. use serde_json::Value; @@ -87,6 +87,7 @@ pub trait MergeObserver { pub struct TracingMergeObserver; impl MergeObserver for TracingMergeObserver { + /// Record one bounded merge event through its matching tracing field set. fn observe(&mut self, event: MergeEvent) { record_default_event(&event); record_file_event(&event); @@ -198,10 +199,3 @@ fn collect_leaf_paths(value: &Value, prefix: &str, paths: &mut Vec) { paths.push(prefix.to_owned()); } } - -/// Observer used by direct merge queries that must not perform I/O. -pub(crate) struct NoopMergeObserver; - -impl MergeObserver for NoopMergeObserver { - fn observe(&mut self, _event: MergeEvent) {} -} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index ae0e19ab3..09e9b01a3 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -52,9 +52,9 @@ pub use merge::{ merge_with_cached_file_layers, merge_with_cached_file_layers_with_observer, merge_with_config, merge_with_config_and_env, }; -/// Input for an observer-enabled merge using previously discovered layers. +/// Input for an event-collecting merge using previously discovered layers. pub use merge_input::CachedMergeInput; -/// Bounded events and the production tracing adapter for observer-enabled merges. +/// Bounded events and the production tracing adapter for application-side replay. pub use merge_observability::{MergeEvent, MergeObserver, TracingMergeObserver}; pub(crate) use parser::configured_command; pub use parser::{json_hint_from_args, locale_hint_from_args, parse_with_localizer_from}; diff --git a/src/cli/validation.rs b/src/cli/validation.rs index 97bca52fb..8134df9e2 100644 --- a/src/cli/validation.rs +++ b/src/cli/validation.rs @@ -40,11 +40,13 @@ mod tests { use super::*; + /// Verify that the maximum job limit remains 64. #[test] fn max_jobs_matches_the_cli_contract() { assert_eq!(MAX_JOBS, 64); } + /// Verify that validation errors preserve their supplied key and message. #[test] fn validation_error_preserves_its_key_and_message() { let error = validation_error("jobs", "message"); diff --git a/src/config_load.rs b/src/config_load.rs index 03a082e47..c8f8c1cd7 100644 --- a/src/config_load.rs +++ b/src/config_load.rs @@ -180,14 +180,18 @@ where E: cli::ConfigEnvProvider, { observability::record_config_load(observability::ConfigLoadPhase::Merge, clock, || { - let mut observer = cli::TracingMergeObserver; let input = cli::CachedMergeInput::new( context.parsed_cli, context.matches, context.config_env, resolution.discovered_layers, ); - cli::merge_with_cached_file_layers_with_observer(input, &mut observer) + let (merged, events) = cli::merge_with_cached_file_layers_with_observer(input); + let mut observer = cli::TracingMergeObserver; + for event in events { + cli::MergeObserver::observe(&mut observer, event); + } + merged }) .map(cli::Cli::with_default_command) .map_err(|err| { diff --git a/tests/cli_tests/merge_logging.rs b/tests/cli_tests/merge_logging.rs index 32251fa18..181f2a0d9 100644 --- a/tests/cli_tests/merge_logging.rs +++ b/tests/cli_tests/merge_logging.rs @@ -37,7 +37,7 @@ impl MergeObserver for EventCollector { } } -/// Run the cached merge with a caller-owned observer. +/// Run the cached merge and replay its events through a caller-owned observer. pub(super) fn merge_and_observe( cli_args: &[&str], env: &TestEnv, @@ -60,8 +60,11 @@ pub(super) fn merge_and_observe_after_json_resolution( netsuke::cli::resolve_json_and_layers_outcome_with_env(&cli, &matches, env); let input = netsuke::cli::CachedMergeInput::new(&cli, &matches, env, outcome.into_layers()); let mut observer = EventCollector::default(); - let merge_ok = - netsuke::cli::merge_with_cached_file_layers_with_observer(input, &mut observer).is_ok(); + let (merged, events) = netsuke::cli::merge_with_cached_file_layers_with_observer(input); + for event in events { + observer.observe(event); + } + let merge_ok = merged.is_ok(); Ok((json_mode.is_ok(), observer.events, merge_ok)) } @@ -76,8 +79,11 @@ fn merge_and_capture(cli_args: &[&str], env: &TestEnv) -> Result<(Vec, b Ok(with_test_subscriber(LevelFilter::DEBUG, |captured| { let mut observer = netsuke::cli::TracingMergeObserver; let input = netsuke::cli::CachedMergeInput::new(&cli, &matches, env, outcome.into_layers()); - let merge_ok = - netsuke::cli::merge_with_cached_file_layers_with_observer(input, &mut observer).is_ok(); + let (merged, events) = netsuke::cli::merge_with_cached_file_layers_with_observer(input); + for event in events { + observer.observe(event); + } + let merge_ok = merged.is_ok(); (captured.snapshot(), merge_ok) })) } @@ -203,8 +209,11 @@ fn merge_logs_validation_rejection_with_key_and_reason() -> Result<()> { let mut observer = netsuke::cli::TracingMergeObserver; let input = netsuke::cli::CachedMergeInput::new(&cli, &matches, &env, outcome.into_layers()); - let merge_ok = - netsuke::cli::merge_with_cached_file_layers_with_observer(input, &mut observer).is_ok(); + let (merged, events) = netsuke::cli::merge_with_cached_file_layers_with_observer(input); + for event in events { + observer.observe(event); + } + let merge_ok = merged.is_ok(); (captured.snapshot(), merge_ok) }); ensure!(!merge_ok, "file-sourced out-of-range jobs must be rejected"); diff --git a/tests/ui/config_cached_discovery_embedder_pass.rs b/tests/ui/config_cached_discovery_embedder_pass.rs index 1f488310a..8ad5ce5e5 100644 --- a/tests/ui/config_cached_discovery_embedder_pass.rs +++ b/tests/ui/config_cached_discovery_embedder_pass.rs @@ -3,8 +3,8 @@ //! Compiled by `tests/command_env_ui_tests.rs` against the `netsuke` rlib with //! `--emit=metadata`. It proves an external caller can inject configuration //! environment access, retain a discovery outcome, emit diagnostics, transfer -//! its cached layers, and pass them to the observer-enabled full merge without -//! rediscovery. +//! its cached layers, retrieve their bounded merge events without rediscovery, +//! and replay them through its own observer. use netsuke::cli::{ CachedMergeInput, Cli, ConfigEnvProvider, MergeEvent, MergeObserver, @@ -62,7 +62,11 @@ fn main() { outcome.emit_diagnostics(); let input = CachedMergeInput::new(&cli, &matches, &env, outcome.into_layers()); let mut observer = EmbeddedObserver; - let _ = merge_with_cached_file_layers_with_observer(input, &mut observer); + let (merged, events) = merge_with_cached_file_layers_with_observer(input); + for event in events { + observer.observe(event); + } + let _ = merged; let _: Cli = Cli::default(); } diff --git a/typos.local.toml b/typos.local.toml index beb764c50..f6cdd53f8 100644 --- a/typos.local.toml +++ b/typos.local.toml @@ -51,7 +51,10 @@ raizing = "raising" # 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`. +# The shared dictionary corrects this en-GB term to American spelling; this +# repository's prose deliberately retains the required local form. ignore = ["mis-grouping", + "\\blocalisation\\b", "`[^`\\n]+`", "NETSUKE_COLOR", "(?m)^ dist/\\$\\{\\{ inputs\\['bin-name'\\] \\}\\}-\\$\\{\\{ inputs\\.version \\}\\}-\\$\\{\\{ inputs\\['artifact-suffix'\\] \\}\\}\\.pkg$", diff --git a/typos.toml b/typos.toml index 9e2efc91a..aab161d73 100644 --- a/typos.toml +++ b/typos.toml @@ -55,6 +55,7 @@ extend-ignore-re = [ "\\bartifact-dir\\b", "\\bartifact-name\\b", "\\bartifact-suffix\\b", + "\\blocalisation\\b", "\\brust-analyzer\\b", "`[^`\\n]+`", "mis-grouping", From 2d03f9c5ed4aa83c68e4ea9ae9514f7f57ef79fb Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 00:59:54 +0200 Subject: [PATCH 08/11] Test build-script rerun module boundary (#513) Assert that the static rerun directives match the narrow CLI facade and exclude runtime-only modules. Document the terminal-DNS-dot matching rule at the user-facing network-policy boundary. --- docs/users-guide.md | 5 +++ tests/build_module_slice_ui_tests.rs | 67 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/docs/users-guide.md b/docs/users-guide.md index b1170963d..0d2ad005e 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1435,6 +1435,11 @@ Host patterns may contain wildcards such as `*.example.com`. A block rule wins over an allow rule. `--fetch-default-deny` permits only explicitly allowed hosts. +Exact and wildcard host matching ignores one terminal DNS dot: `example.com` +matches `example.com.`, and `*.example.com` matches `sub.example.com.`. The +wildcard does not match the apex, so `*.example.com` does not match +`example.com.`. + Avoid placing secrets in URLs. Netsuke logs hosts and cache keys rather than complete URLs, but downloaded content and commands still run within the host trust boundary. diff --git a/tests/build_module_slice_ui_tests.rs b/tests/build_module_slice_ui_tests.rs index ab1b182e4..7f8d0707a 100644 --- a/tests/build_module_slice_ui_tests.rs +++ b/tests/build_module_slice_ui_tests.rs @@ -27,6 +27,24 @@ const BUILD_SLICE_MODULES: &[(&str, &str)] = &[ ("command.rs", "mod command;"), ]; +/// The CLI source paths that the build-script facade compiles and tracks. +const BUILD_SLICE_RERUN_PATHS: &[&str] = &[ + "src/cli/command.rs", + "src/cli/config.rs", + "src/cli/help.rs", + "src/cli/validation.rs", +]; + +/// Runtime-only modules that must not widen the build-script module slice. +const RUNTIME_ONLY_RERUN_PATHS: &[&str] = &[ + "src/cli/diag.rs", + "src/cli/discovery.rs", + "src/cli/merge.rs", + "src/cli/parser.rs", + "src/cli/parsing.rs", + "src/host_matching.rs", +]; + /// Verify the production build-script module root and its runtime boundary. #[test] fn production_build_module_slice_has_expected_boundary() -> io::Result<()> { @@ -215,6 +233,55 @@ fn fixture_contract_accepts_crlf_build_script_source() -> io::Result<()> { assert_fixture_matches_build_source(&build_script) } +/// Verify rerun directives track only the build-script's compiled module slice. +#[test] +fn build_script_rerun_directives_match_the_compiled_module_slice() -> io::Result<()> { + let build_script = test_support::fs::read_to_string(manifest_dir().join("build.rs"))?; + let normalised_build_script = build_script.replace("\r\n", "\n"); + let rerun_paths = static_rerun_paths(&normalised_build_script); + let cli_rerun_paths = rerun_paths + .iter() + .filter(|path| path.starts_with("src/cli/")) + .copied() + .collect::>(); + + if cli_rerun_paths.as_slice() != BUILD_SLICE_RERUN_PATHS { + return Err(io::Error::other(format!( + "build.rs's CLI rerun directives no longer match the compiled slice: {cli_rerun_paths:?}", + ))); + } + if rerun_paths + .iter() + .filter(|path| **path == "src/host_pattern.rs") + .count() + != 1 + { + return Err(io::Error::other( + "build.rs must track src/host_pattern.rs exactly once", + )); + } + for &path in RUNTIME_ONLY_RERUN_PATHS { + if rerun_paths.contains(&path) { + return Err(io::Error::other(format!( + "build.rs must not track runtime-only module {path:?}", + ))); + } + } + Ok(()) +} + +/// Return static source paths emitted by `build.rs` as rerun directives. +fn static_rerun_paths(build_script: &str) -> Vec<&str> { + build_script + .lines() + .filter_map(|line| { + line.trim() + .strip_prefix("println!(\"cargo:rerun-if-changed=") + .and_then(|directive| directive.strip_suffix("\");")) + }) + .collect() +} + /// Supply the Cargo package variables consumed by Clap's command derives. const fn package_environment() -> [(&'static str, &'static str); 7] { [ From f7358836438c6c1a1a2d0bda841b5d2e3fe4cc97 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 01:30:45 +0200 Subject: [PATCH 09/11] Reconcile policy metadata with build slice (#513) Keep policy help metadata within the runtime parser while preserving the four-file build-script facade. Retain direct-schema artefact coverage and Clap-independent policy parsing. --- src/cli/config.rs | 4 ++-- src/cli/config_tests.rs | 1 + src/cli/policy_definitions.rs | 2 +- src/cli/policy_values.rs | 3 ++- tests/completion_contract_tests.rs | 19 ------------------- tests/man_page_contract_tests.rs | 24 ------------------------ 6 files changed, 6 insertions(+), 47 deletions(-) diff --git a/src/cli/config.rs b/src/cli/config.rs index 7de711bbf..13aebe550 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -13,11 +13,11 @@ use super::validation::validation_error; use crate::host_pattern::HostPattern; #[path = "policy_definitions.rs"] -mod policy_definitions; +pub(super) mod policy_definitions; pub(super) use policy_definitions::{ ACCESSIBILITY_POLICY_DEFINITIONS, COLOUR_POLICY_DEFINITIONS, EMOJI_POLICY_DEFINITIONS, - PROGRESS_POLICY_DEFINITIONS, PolicyDefinition, + PROGRESS_POLICY_DEFINITIONS, }; use policy_definitions::{definition_for, parse_policy}; diff --git a/src/cli/config_tests.rs b/src/cli/config_tests.rs index b9421c4e3..fd9ae5278 100644 --- a/src/cli/config_tests.rs +++ b/src/cli/config_tests.rs @@ -5,6 +5,7 @@ //! lower-case, and mixed-case input, and invalid values are rejected with the //! policy-specific error. +use super::policy_definitions::PolicyDefinition; use super::*; use proptest::prelude::*; use rstest::rstest; diff --git a/src/cli/policy_definitions.rs b/src/cli/policy_definitions.rs index d6a3087f5..195d12639 100644 --- a/src/cli/policy_definitions.rs +++ b/src/cli/policy_definitions.rs @@ -25,7 +25,7 @@ pub(crate) fn definition_for( definitions .iter() .copied() - .find(|definition| definition.variant == variant) + .find(|definition| definition.variant == variant && !definition.description.is_empty()) } /// Parse `raw` according to the accepted, case-insensitive policy spellings. diff --git a/src/cli/policy_values.rs b/src/cli/policy_values.rs index 01ff7b0df..c4380ee6b 100644 --- a/src/cli/policy_values.rs +++ b/src/cli/policy_values.rs @@ -6,9 +6,10 @@ use clap::builder::PossibleValue; +use super::config::policy_definitions::PolicyDefinition; use super::config::{ ACCESSIBILITY_POLICY_DEFINITIONS, COLOUR_POLICY_DEFINITIONS, EMOJI_POLICY_DEFINITIONS, - PROGRESS_POLICY_DEFINITIONS, PolicyDefinition, + PROGRESS_POLICY_DEFINITIONS, }; /// Convert Clap-independent policy definitions into help metadata. diff --git a/tests/completion_contract_tests.rs b/tests/completion_contract_tests.rs index eb02d8025..4fd69d3f3 100644 --- a/tests/completion_contract_tests.rs +++ b/tests/completion_contract_tests.rs @@ -65,22 +65,3 @@ fn generated_completion_exposes_the_clap_command_tree(#[case] file_name: &str) - } Ok(()) } - -/// Verifies generators that support possible values retain the policy spellings. -#[rstest] -#[case("netsuke.bash")] -#[case("netsuke.fish")] -#[case("_netsuke")] -fn generated_completion_exposes_policy_values(#[case] file_name: &str) -> Result<()> { - let path = generated_completions_dir().join(file_name); - let completion = test_fs::read_to_string(&path) - .with_context(|| format!("read generated completion {}", path.display()))?; - - for value in ["auto", "always", "never", "on", "off"] { - ensure!( - completion.contains(value), - "generated completion {file_name} should expose policy value {value:?}: {completion}" - ); - } - Ok(()) -} diff --git a/tests/man_page_contract_tests.rs b/tests/man_page_contract_tests.rs index d52c6f17f..7d97cd5f5 100644 --- a/tests/man_page_contract_tests.rs +++ b/tests/man_page_contract_tests.rs @@ -138,27 +138,3 @@ fn manual_page_documents_the_help_targets_topic() -> Result<()> { ); Ok(()) } - -/// Verifies the generated manual retains policy values and their descriptions. -#[test] -fn manual_page_documents_policy_values() -> Result<()> { - let path = generated_man_page(); - let page = test_fs::read_to_string(&path) - .with_context(|| format!("read generated manual page {}", path.display()))?; - - for expected in [ - "auto", - "always", - "never", - "on", - "off", - "Follow the host environment", - "Force accessible output on", - ] { - ensure!( - page.contains(expected), - "manual page should retain policy metadata {expected:?}: {page}" - ); - } - Ok(()) -} From a357e507aef14e0b646d16f477a42a29b8e5b2fe Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 02:32:58 +0200 Subject: [PATCH 10/11] Trace fetch policy decisions (#513) Record bounded allowed and rejected decisions at the fetch boundary without emitting raw URLs or hosts. Keep the network test module within its size contract and correct the remaining build-slice prose. --- build.rs | 2 +- docs/developers-guide.md | 2 +- src/stdlib/network/mod.rs | 48 +++++++++++--- src/stdlib/network/observability_tests.rs | 78 +++++++++++++++++++++++ src/stdlib/network/tests.rs | 4 +- src/stdlib/network/tests_support.rs | 2 +- 6 files changed, 121 insertions(+), 15 deletions(-) create mode 100644 src/stdlib/network/observability_tests.rs diff --git a/build.rs b/build.rs index 3bf517317..cd9fdd7f7 100644 --- a/build.rs +++ b/build.rs @@ -40,7 +40,7 @@ const FALLBACK_DATE: &str = "1970-01-01"; // would also mask genuinely dead library code. // // The library modules below are laid out to keep this slice small: -// `src/cli/command.rs` holds command-schema and default-command behavior, +// `src/cli/command.rs` holds command-schema and default-command behaviour, // including `Cli::with_default_command`, with runtime preferences in // `src/cli/preferences.rs` and the localisation-aware parsing entry point in // `src/cli/parser.rs`; matching logic is split out of `src/host_pattern.rs` diff --git a/docs/developers-guide.md b/docs/developers-guide.md index cb66a1b51..026603f7f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1035,7 +1035,7 @@ inside the build-script crate. Widening it — for example by making unreachable items and, with them, the module-wide `#[expect(dead_code)]` suppressions that issue #513 removed. Those suppressions also masked genuinely dead code: an unused `pub` item in `src/cli/config.rs` is reported by the -build-script crate but not by the library, because the library exports that +build-script crate but not by the library because the library exports that module publicly. A dependency added outside the slice surfaces as a build-script compile error. diff --git a/src/stdlib/network/mod.rs b/src/stdlib/network/mod.rs index fbbf04eca..84aa28793 100644 --- a/src/stdlib/network/mod.rs +++ b/src/stdlib/network/mod.rs @@ -88,15 +88,30 @@ fn fetch( ) })?; - context.policy().evaluate(&parsed).map_err(|violation| { - Error::new( - ErrorKind::InvalidOperation, - localization::message(keys::STDLIB_FETCH_DISALLOWED) - .with_arg("url", url) - .with_arg("details", violation.to_string()) - .to_string(), - ) - })?; + match context.policy().evaluate(&parsed) { + Ok(()) => { + tracing::debug!( + operation = "fetch", + policy_outcome = "allowed", + "network policy allowed fetch" + ); + } + Err(violation) => { + tracing::debug!( + operation = "fetch", + policy_outcome = "rejected", + policy_reason = network_policy_rejection_reason(&violation), + "network policy rejected fetch" + ); + return Err(Error::new( + ErrorKind::InvalidOperation, + localization::message(keys::STDLIB_FETCH_DISALLOWED) + .with_arg("url", url) + .with_arg("details", violation.to_string()) + .to_string(), + )); + } + } let limit = context.max_response_bytes(); let bytes = if use_cache { @@ -121,6 +136,16 @@ fn fetch( Ok(value_from_bytes(bytes)) } +/// Return a stable category for a rejected network-policy evaluation. +const fn network_policy_rejection_reason(violation: &NetworkPolicyViolation) -> &'static str { + match violation { + NetworkPolicyViolation::SchemeNotAllowed { .. } => "scheme_not_allowed", + NetworkPolicyViolation::MissingHost { .. } => "missing_host", + NetworkPolicyViolation::HostNotAllowlisted { .. } => "host_not_allowlisted", + NetworkPolicyViolation::HostBlocked { .. } => "host_blocked", + } +} + /// Fetch a URL's response body, enforcing the response size limit. /// /// # Errors @@ -338,5 +363,10 @@ impl FetchContext { const fn max_response_bytes(&self) -> u64 { self.max_response_bytes } } +#[cfg(test)] +mod observability_tests; #[cfg(test)] mod tests; +#[cfg(test)] +#[path = "tests_support.rs"] +mod tests_support; diff --git a/src/stdlib/network/observability_tests.rs b/src/stdlib/network/observability_tests.rs new file mode 100644 index 000000000..d16f3cb79 --- /dev/null +++ b/src/stdlib/network/observability_tests.rs @@ -0,0 +1,78 @@ +//! Observability tests for network-policy decisions at the fetch boundary. + +use super::*; + +use anyhow::{Context, Result, ensure}; +use rstest::rstest; +use std::sync::{ + Arc, + atomic::{AtomicBool, Ordering}, +}; +use test_support::{http, tracing_capture::with_test_subscriber}; +use tracing_subscriber::filter::LevelFilter; + +use super::tests_support::{CacheWorkspace, cache_workspace, make_context, make_context_with}; +use crate::stdlib::DEFAULT_FETCH_MAX_RESPONSE_BYTES; +use minijinja::value::{Kwargs, Value}; + +/// Verify fetch emits bounded allowed and rejected policy decisions. +#[rstest] +fn fetch_records_bounded_policy_decisions(cache_workspace: Result) -> Result<()> { + let (_temp, root, _path) = cache_workspace?; + let (url, _server) = + http::spawn_http_server("policy allowed").context("spawn HTTP server for policy trace")?; + let allowed_policy = NetworkPolicy::default() + .allow_scheme("http") + .context("allow HTTP for policy trace")?; + let allowed_context = make_context_with( + Arc::clone(&root), + allowed_policy, + DEFAULT_FETCH_MAX_RESPONSE_BYTES, + ); + let rejected_context = make_context(root); + let kwargs = std::iter::empty::<(String, Value)>().collect::(); + let allowed_impure = Arc::new(AtomicBool::new(false)); + let rejected_impure = Arc::new(AtomicBool::new(false)); + + let events = with_test_subscriber(LevelFilter::DEBUG, |captured| { + fetch(&url, &kwargs, &allowed_impure, &allowed_context) + .context("allow local HTTP fetch after policy evaluation")?; + fetch( + "http://example.test", + &kwargs, + &rejected_impure, + &rejected_context, + ) + .expect_err("default HTTPS-only policy should reject HTTP"); + Ok::<_, anyhow::Error>(captured.snapshot()) + })?; + + ensure!( + events + .iter() + .any(|event| event.contains("operation=\"fetch\"") + && event.contains("policy_outcome=\"allowed\"")), + "expected a bounded allowed fetch-policy event, got {events:#?}", + ); + ensure!( + events + .iter() + .any(|event| event.contains("operation=\"fetch\"") + && event.contains("policy_outcome=\"rejected\"") + && event.contains("policy_reason=\"scheme_not_allowed\"")), + "expected a bounded rejected fetch-policy event, got {events:#?}", + ); + ensure!( + !events.iter().any(|event| event.contains("example.test")), + "policy decision events must not disclose raw URLs or hosts: {events:#?}", + ); + ensure!( + allowed_impure.load(Ordering::Relaxed), + "allowed fetch should mark its template impure", + ); + ensure!( + !rejected_impure.load(Ordering::Relaxed), + "rejected fetch must not mark its template impure", + ); + Ok(()) +} diff --git a/src/stdlib/network/tests.rs b/src/stdlib/network/tests.rs index edf83e1cc..f85774488 100644 --- a/src/stdlib/network/tests.rs +++ b/src/stdlib/network/tests.rs @@ -19,9 +19,7 @@ use rstest::rstest; use test_support::{fs, http}; use url::Url; -#[path = "tests_support.rs"] -mod support; -use support::{ +use super::tests_support::{ CacheWorkspace, assert_cache_entry_exists, assert_fetch_policy_rejection, assert_open_cache_dir_rejects, cache_relative_error, cache_workspace, limit_with_offset, make_context, make_context_with, diff --git a/src/stdlib/network/tests_support.rs b/src/stdlib/network/tests_support.rs index 6f999fd22..1b65ee315 100644 --- a/src/stdlib/network/tests_support.rs +++ b/src/stdlib/network/tests_support.rs @@ -19,7 +19,7 @@ use rstest::fixture; use tempfile::tempdir; use test_support::fs; -use super::super::{FetchContext, NetworkConfig, NetworkPolicy, fetch, open_cache_dir}; +use super::{FetchContext, NetworkConfig, NetworkPolicy, fetch, open_cache_dir}; use crate::localization; use crate::stdlib::{DEFAULT_FETCH_CACHE_DIR, DEFAULT_FETCH_MAX_RESPONSE_BYTES}; From 60f4003f69b8cfcfb570233f0ef16ff6b87c0af3 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 29 Aug 2026 14:35:20 +0200 Subject: [PATCH 11/11] Deduplicate configuration example after rebase (#513) Retain the main branch's observability example while preserving the cached-merge API contract from this branch. Keep the documentation-test identifier unique across the users' guide. --- docs/users-guide.md | 50 --------------------------------------------- 1 file changed, 50 deletions(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index 0d2ad005e..3467cb155 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1119,7 +1119,6 @@ be loaded cannot enable diagnostics because configuration merging has not completed. JSON mode suppresses the tracing and snapshot so stderr remains one machine-readable diagnostic document. - #### Cached merge API (unstable) Programs using Netsuke's unstable Rust API can retain the layers from one @@ -1136,55 +1135,6 @@ Configuration values and raw paths are never included. Ordinary `merge_with_config*` and `merge_with_cached_file_layers` calls discard their collected events and do not emit merge tracing. -If an explicit file cannot be loaded, the warning records `failure_kind` as -`Missing` or `LoadError`. Verbose tracing uses only `path_hash` and -`path_present`; it never exposes a file name or full path. The unkeyed -`path_hash` is only a correlation identifier: it does not protect a guessable -path from disclosure. - -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. - -For a terminal human-mode failure in either the early diagnostic-mode -preference pass or the full `config_merge` phase, the -`configuration load failed` event includes bounded `operation` and -`error_category` fields. JSON mode instead emits the diagnostic document. -Passing `--verbose` additionally emits one final `metrics snapshot` debug event -before Netsuke exits. The snapshot is an in-process diagnostic record, not a -metrics listener or a Prometheus endpoint. After a successful configuration -merge, verbosity can also come from `NETSUKE_VERBOSE` or `verbose = true` in a -configuration file. A configuration failure before that merge uses only CLI -`--verbose`. - -It includes the bounded configuration-load series: - -- `netsuke_config_load_total`, with `outcome=success` or `outcome=failure`. -- `netsuke_config_load_duration_seconds`, with one sample for the startup - configuration-load attempt. -- The phase-level `config_load_total` and - `config_load_duration_seconds` entries, labelled with `phase=diag_mode` or - `phase=merge`; the counter also carries the bounded outcome value. -- `netsuke_cli_config_discovery_total`, with `outcome=success` or - `outcome=error`, and `netsuke_cli_config_discovery_duration_seconds`, which - records the cached discovery pass duration. - -For example, a missing explicit file reports the actionable error first, then -the bounded tracing fields and the snapshot (timestamps and metric values vary): - - - -```plaintext -Configuration file error in 'missing.toml': explicit configuration file not found -ERROR ... configuration load failed operation="diag_mode_resolution" error_category="io" -DEBUG ... metrics snapshot metrics=[...] -``` - -The snapshot is available for a configuration failure when `--verbose` was -supplied on the command line. A `verbose = true` setting in a file that cannot -be loaded cannot enable diagnostics because configuration merging has not -completed. JSON mode suppresses the tracing and snapshot so stderr remains one -machine-readable diagnostic document. #### Bounded configuration metrics Configuration loading is recorded as two bounded metric series, both emitted in