From 75c109749dcdb8175459dccce9792e546b5a4aee Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 00:39:12 +0200 Subject: [PATCH 1/8] Address PR #515 review follow-ups Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) --- locales/en-GB/messages.ftl | 2 +- locales/en-US/messages.ftl | 2 +- src/manifest/jinja_macros/invocation.rs | 2 +- src/runner/mod.rs | 6 +++++ src/stdlib/config_types.rs | 2 +- src/stdlib/which/env.rs | 30 +++++++++++++++++++--- test_support/src/check_ninja.rs | 13 ++++++++++ test_support/src/command_helper.rs | 11 +++++--- test_support/src/dev_fast/mod.rs | 15 ++++++++++- test_support/src/env_lock.rs | 14 ++++++---- test_support/src/exec.rs | 7 ++++- test_support/src/stdlib_assert.rs | 8 +++++- tests/bdd/steps/ir.rs | 19 ++++++++++---- tests/bdd/steps/manifest/environment.rs | 5 +++- tests/bdd/steps/manifest/mod.rs | 2 +- tests/cli_tests/config_discovery_scopes.rs | 5 +++- tests/cli_tests/config_selection.rs | 6 +++++ tests/cli_tests/merge.rs | 14 ++++++---- tests/cli_tests/merge_probe.rs | 12 ++++++++- tests/manifest_jinja_tests.rs | 14 +++++----- tests/novice_flow_smoke_tests.rs | 12 +++++++-- tests/packaging_smoke_tests.rs | 18 ++++++++----- tests/whitaker_boundary_contract.rs | 28 +++++++++++++++++--- tests/workflow_contracts/ci_lint_test.py | 30 ++++++++++++++++++++-- 24 files changed, 223 insertions(+), 54 deletions(-) diff --git a/locales/en-GB/messages.ftl b/locales/en-GB/messages.ftl index 8c7383422..27ab997e1 100644 --- a/locales/en-GB/messages.ftl +++ b/locales/en-GB/messages.ftl @@ -139,7 +139,7 @@ manifest.macro.register_failed = Failed to register manifest macros. manifest.macro.not_initialised = Macro environment is not initialised. manifest.macro.caller_invalid = Macro caller must be a string. manifest.macro.template_load_failed = Failed to load macro template. -manifest.macro.init_failed = Failed to initialise macro environment. +manifest.macro.init_failed = Failed to initialize macro environment. manifest.macro.missing = Macro { $name } is missing. # Manifest glob errors. diff --git a/locales/en-US/messages.ftl b/locales/en-US/messages.ftl index 0bac0e071..ef6b295aa 100644 --- a/locales/en-US/messages.ftl +++ b/locales/en-US/messages.ftl @@ -139,7 +139,7 @@ manifest.macro.register_failed = Failed to register manifest macros. manifest.macro.not_initialised = Macro environment is not initialised. manifest.macro.caller_invalid = Macro caller must be a string. manifest.macro.template_load_failed = Failed to load macro template. -manifest.macro.init_failed = Failed to initialise macro environment. +manifest.macro.init_failed = Failed to initialize macro environment. manifest.macro.missing = Macro { $name } is missing. # Manifest glob errors. diff --git a/src/manifest/jinja_macros/invocation.rs b/src/manifest/jinja_macros/invocation.rs index c7243b9ee..39068986c 100644 --- a/src/manifest/jinja_macros/invocation.rs +++ b/src/manifest/jinja_macros/invocation.rs @@ -172,7 +172,7 @@ mod tests { let error = validate_macro(&env, "invalid-template", "missing_macro") .expect_err("template initialization should fail validation"); - insta::assert_snapshot!(error.to_string(), @"undefined value: Failed to initialise macro environment."); + insta::assert_snapshot!(error.to_string(), @"undefined value: Failed to initialize macro environment."); } #[rstest] diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 890d5ba9b..c4698ff87 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -54,6 +54,12 @@ use path_helpers::{ensure_manifest_exists_or_error, resolve_manifest_path, resol struct ExecutionContext<'a> { reporter: &'a dyn StatusReporter, progress_enabled: bool, + /// Resolved Ninja executable, passed unchanged to [`std::process::Command::new`]. + /// + /// UTF-8 conversion is confined to `NETSUKE_NINJA` resolution + /// (`process::resolve_ninja_program`); this field must stay a native + /// [`Path`] and must not be converted to a `String`, so that non-UTF-8 + /// executable paths on platforms that allow them remain usable. ninja_program: &'a Path, } diff --git a/src/stdlib/config_types.rs b/src/stdlib/config_types.rs index c9ec5207a..50ad2201d 100644 --- a/src/stdlib/config_types.rs +++ b/src/stdlib/config_types.rs @@ -31,7 +31,7 @@ pub(crate) enum HomeDirectory { Explicit(String), } -/// Internal configuration passed to the network module for fetch cache initialisation. +/// Internal configuration passed to the network module for fetch cache initialization. #[derive(Clone)] pub struct NetworkConfig { /// Capability-scoped workspace root for network caches. diff --git a/src/stdlib/which/env.rs b/src/stdlib/which/env.rs index daf6e27a4..ef937d952 100644 --- a/src/stdlib/which/env.rs +++ b/src/stdlib/which/env.rs @@ -79,9 +79,33 @@ impl EnvSnapshot { path_override: Option<&OsStr>, env: &impl Env, ) -> Result { - #[cfg(windows)] - return Self::capture_impl(cwd_override, path_override, env, None); - #[cfg(not(windows))] + Self::capture_for_platform(cwd_override, path_override, env) + } + + /// Capture a snapshot without a `PATHEXT` override on Windows. + /// + /// Windows threads a `PATHEXT` override that the other platforms have no + /// concept of, so the two `capture_impl` arities diverge. Isolating the + /// divergence in a pair of wrappers keeps `capture_with_env` free of a + /// `cfg`-gated bare `return`, which reads as dead code on either target. + #[cfg(windows)] + fn capture_for_platform( + cwd_override: Option<&Utf8Path>, + path_override: Option<&OsStr>, + env: &impl Env, + ) -> Result { + Self::capture_impl(cwd_override, path_override, env, None) + } + + /// Capture a snapshot on platforms without `PATHEXT` semantics. + /// + /// See the Windows counterpart for why this wrapper exists. + #[cfg(not(windows))] + fn capture_for_platform( + cwd_override: Option<&Utf8Path>, + path_override: Option<&OsStr>, + env: &impl Env, + ) -> Result { Self::capture_impl(cwd_override, path_override, env) } diff --git a/test_support/src/check_ninja.rs b/test_support/src/check_ninja.rs index 2c0d00f46..571a201f1 100644 --- a/test_support/src/check_ninja.rs +++ b/test_support/src/check_ninja.rs @@ -87,6 +87,19 @@ fn write_fake_ninja_script_in_dir( Ok((dir, path)) } +/// Create a fake Ninja that validates the build file path, rooting the +/// temporary directory beneath `parent` instead of at the top level. +/// +/// This exists to support the non-UTF-8 parent-directory contract: the +/// sibling public [`fake_ninja_check_build_file`] instead creates its own +/// top-level [`TempDir`], whereas this variant nests the temporary directory +/// inside `parent` via [`tempfile::Builder::tempdir_in`]. +/// +/// # Errors +/// +/// Returns an error if the temporary directory cannot be created beneath +/// `parent`, or if the fake script cannot be written or marked executable +/// (both delegated to `write_fake_ninja_script_in_dir`). #[cfg(all(test, unix))] pub(crate) fn fake_ninja_check_build_file_in( parent: &std::path::Path, diff --git a/test_support/src/command_helper.rs b/test_support/src/command_helper.rs index 58bd80bd1..906da45b5 100644 --- a/test_support/src/command_helper.rs +++ b/test_support/src/command_helper.rs @@ -211,8 +211,6 @@ mod tests { use mockable::MockEnv; use std::ffi::OsString; #[cfg(unix)] - use std::path::PathBuf; - #[cfg(unix)] use tempfile::tempdir; #[test] @@ -268,8 +266,13 @@ mod tests { ), ) .context("write configured compiler wrapper")?; - let marker = PathBuf::from(format!("{}.invoked", wrapper.display())); - let configured = wrapper.into_os_string(); + // `write_exec_with_content` is a std::path boundary; convert once here so + // the sidecar marker stays in camino's UTF-8 domain like the rest of the + // helper. + let wrapper_path = Utf8PathBuf::from_path_buf(wrapper) + .map_err(|path| anyhow::anyhow!("compiler wrapper path is not UTF-8: {path:?}"))?; + let marker = Utf8PathBuf::from(format!("{wrapper_path}.invoked")); + let configured = OsString::from(wrapper_path.as_str()); let mut env = MockEnv::new(); env.expect_os_string() .withf(|key| key == "RUSTC") diff --git a/test_support/src/dev_fast/mod.rs b/test_support/src/dev_fast/mod.rs index 3181f119c..aa330f1c5 100644 --- a/test_support/src/dev_fast/mod.rs +++ b/test_support/src/dev_fast/mod.rs @@ -38,9 +38,22 @@ pub use bench::{BASELINE_MTIME, BenchFixture, DEFAULT_SLUG, DEV_FAST_SLUG, write pub use cargo_log::{CargoInvocation, RecordingCargo, TargetState}; pub use make::MakeInvocation; pub use release::FakeRelease; +/// Resolve a genuine utility using an injected [`mockable::Env`]. +/// +/// This is the test seam for [`real_utility`]: `PATH` is read through the +/// injected environment rather than the process environment, so `PATH` +/// lookups can be exercised without touching ambient state. `current_dir` +/// resolves relative `PATH` entries and `utility` names the executable to +/// find. +/// +/// # Errors +/// +/// Returns an error if `utility` cannot be resolved from the supplied +/// environment. +pub use sandbox::real_utility_with_env; pub use sandbox::{ DEV_FAST_CONFIG_PATH, PinOverrides, Sandbox, combined, dev_fast_config, pinned_mold_version, - pinned_toolchain, real_utility, real_utility_with_env, + pinned_toolchain, real_utility, }; pub use scenario::{ BuildScenario, InstallerFixture, InstallerScenario, TEST_MOLD_VERSION, WRONG_SHA256, diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index 879d55d37..db142fd7a 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -238,6 +238,12 @@ mod tests { } } + /// Probes `ENV_LOCK` directly, so it requires per-test process isolation + /// (the suite runs under `cargo nextest`, which forks each test). Under a + /// thread-parallel runner it would race any other test touching the global + /// mutex. The poison flag is cleared before the state is asserted so a + /// failing assertion cannot leak process-global poisoning into whatever + /// else shares the process. #[test] fn env_lock_recovers_after_mutex_poisoning() { let poisoner = std::thread::spawn(|| { @@ -246,15 +252,13 @@ mod tests { }); assert!(poisoner.join().is_err(), "poisoning thread should panic"); - assert!( - ENV_LOCK.is_poisoned(), - "the panic should poison the underlying mutex" - ); + let was_poisoned = ENV_LOCK.is_poisoned(); + ENV_LOCK.clear_poison(); + assert!(was_poisoned, "the panic should poison the underlying mutex"); let recovered_guard = EnvLock::acquire(); assert_current_thread_lock_is_held("recovered EnvLock guard should hold ENV_LOCK"); drop(recovered_guard); - ENV_LOCK.clear_poison(); assert_current_thread_lock_is_released("recovered ENV_LOCK should be released normally"); } } diff --git a/test_support/src/exec.rs b/test_support/src/exec.rs index ad34e327e..d10e2a69e 100644 --- a/test_support/src/exec.rs +++ b/test_support/src/exec.rs @@ -70,17 +70,22 @@ fn single_file_name(name: &str) -> Result<()> { } /// Mark an existing file as executable by setting its Unix permission bits. -#[cfg(unix)] /// /// # Errors /// /// Returns an error if executable permissions cannot be applied to the path. +#[cfg(unix)] pub fn make_executable(path: &Path) -> Result<()> { crate::fs::set_mode(path, 0o755).context("chmod exec stub")?; Ok(()) } /// No-op on non-Unix platforms, where executability is not a permission bit. +/// +/// # Errors +/// +/// Never returns an error; the fallible signature matches the Unix variant so +/// callers need no platform-specific handling. #[cfg(not(unix))] pub fn make_executable(_path: &Path) -> Result<()> { Ok(()) diff --git a/test_support/src/stdlib_assert.rs b/test_support/src/stdlib_assert.rs index 3d40da7ad..42adea8ef 100644 --- a/test_support/src/stdlib_assert.rs +++ b/test_support/src/stdlib_assert.rs @@ -5,9 +5,15 @@ use anyhow::{Result, bail}; /// error. This mirrors the behaviour of the Cucumber step assertions so unit /// tests can guard the branching logic. /// +/// # Returns +/// +/// `Ok(output)` when `output` is `Some`; `output` takes precedence when both +/// `output` and `error` are present. +/// /// # Errors /// -/// Returns the reported standard-library error, or an error when neither value is present. +/// Returns an error when `output` is absent. Any `error` text supplied is +/// included in that error. pub fn stdlib_output_or_error<'a>(output: Option<&'a str>, error: Option<&str>) -> Result<&'a str> { match (output, error) { (Some(out), _) => Ok(out), diff --git a/tests/bdd/steps/ir.rs b/tests/bdd/steps/ir.rs index 2fa8b0368..26db08050 100644 --- a/tests/bdd/steps/ir.rs +++ b/tests/bdd/steps/ir.rs @@ -8,9 +8,10 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use crate::bdd::helpers::parse_store::store_parse_outcome; +use crate::bdd::steps::manifest::environment::manifest_env_reader; use anyhow::{Context, Result, anyhow, ensure}; use camino::Utf8PathBuf; -use netsuke::ir::BuildGraph; +use netsuke::{ir::BuildGraph, stdlib::NetworkPolicy}; use rstest_bdd_macros::{given, then, when}; // --------------------------------------------------------------------------- @@ -236,9 +237,17 @@ fn compile_manifest_impl(world: &TestWorld, path: &str) { } else { path.to_owned() }; - let outcome = netsuke::manifest::from_path(&resolved) - .and_then(|m| BuildGraph::from_manifest(&m).context("building IR from manifest")) - .with_context(|| format!("IR generation failed for {path}")) - .map_err(|e| e.to_string()); + // Match the manifest steps' injected-environment flow so IR compilation sees + // scenario values rather than the host environment. + let env_reader = manifest_env_reader(world); + let outcome = netsuke::manifest::from_path_with_policy_and_env( + &resolved, + NetworkPolicy::default(), + &env_reader, + None, + ) + .and_then(|m| BuildGraph::from_manifest(&m).context("building IR from manifest")) + .with_context(|| format!("IR generation failed for {path}")) + .map_err(|e| e.to_string()); store_parse_outcome(&world.build_graph, &world.generation_error, outcome); } diff --git a/tests/bdd/steps/manifest/environment.rs b/tests/bdd/steps/manifest/environment.rs index ca873fbe6..7e8a85a22 100644 --- a/tests/bdd/steps/manifest/environment.rs +++ b/tests/bdd/steps/manifest/environment.rs @@ -6,7 +6,10 @@ use netsuke::manifest; use crate::bdd::fixtures::TestWorld; -pub(super) fn manifest_env_reader(world: &TestWorld) -> manifest::EnvReader { +/// Build a [`manifest::EnvReader`] backed by the scenario's forwarded +/// environment so manifest and IR steps read scenario values rather than the +/// host environment. +pub(crate) fn manifest_env_reader(world: &TestWorld) -> manifest::EnvReader { let values = world.env_vars_forward.borrow().clone(); Arc::new(move |key| { values diff --git a/tests/bdd/steps/manifest/mod.rs b/tests/bdd/steps/manifest/mod.rs index 4196d438c..694ed0a13 100644 --- a/tests/bdd/steps/manifest/mod.rs +++ b/tests/bdd/steps/manifest/mod.rs @@ -5,7 +5,7 @@ //! - `helpers.rs` - Typed assertion utilities and target accessor functions //! - `targets.rs` - Target-specific assertion steps -mod environment; +pub(super) mod environment; mod helpers; mod targets; diff --git a/tests/cli_tests/config_discovery_scopes.rs b/tests/cli_tests/config_discovery_scopes.rs index b936320d6..56a1576ca 100644 --- a/tests/cli_tests/config_discovery_scopes.rs +++ b/tests/cli_tests/config_discovery_scopes.rs @@ -36,7 +36,10 @@ jobs = 8 ) .context("write project .netsuke.toml")?; - let merged = run_scope_scenario(temp_dir.path(), temp_dir.path(), &[])?; + // Keep home separate from the project directory so the assertions below can + // only pass through project-scope discovery. + let home_dir = tempdir().context("create temporary home directory")?; + let merged = run_scope_scenario(temp_dir.path(), home_dir.path(), &[])?; ensure!( merged.emoji == EmojiPolicy::Always, diff --git a/tests/cli_tests/config_selection.rs b/tests/cli_tests/config_selection.rs index ceee2eccd..7abe342b2 100644 --- a/tests/cli_tests/config_selection.rs +++ b/tests/cli_tests/config_selection.rs @@ -242,5 +242,11 @@ fn config_flag_with_nonexistent_file_produces_error( message.contains("missing.toml"), "error should mention the missing explicit config path, got {message}" ); + // Pin the stable diagnostic alongside the path so a generic I/O failure + // cannot satisfy this test. + ensure!( + message.contains("explicit configuration file not found"), + "error should name the explicit-config failure mode, got {message}" + ); Ok(()) } diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 081690f70..eeae5c55d 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -2,7 +2,7 @@ //! //! These tests validate `OrthoConfig` layer precedence (defaults, file, env, //! CLI) and list-value appending. -use super::merge_probe::merge_in_child; +use super::merge_probe::{isolated_environment, merge_in_child}; use anyhow::{Context, Result, ensure}; use netsuke::cli::{CliConfig, ProgressPolicy}; use ortho_config::{MergeComposer, sanitize_value}; @@ -25,14 +25,16 @@ where let temp_dir = tempfile::tempdir().context("create temporary config directory")?; let config_path = temp_dir.path().join("netsuke.toml"); std::fs::write(&config_path, toml_content).context("write netsuke.toml")?; - let merged = merge_in_child( - cli_args, + // Seed the configuration sandbox so host XDG configuration cannot leak into + // the child; `_xdg_config_dirs` must outlive the child process. + let (_xdg_config_dirs, environment) = isolated_environment( temp_dir.path(), &[( OsString::from("NETSUKE_CONFIG"), config_path.into_os_string(), )], )?; + let merged = merge_in_child(cli_args, temp_dir.path(), &environment)?; f(merged) } @@ -175,8 +177,9 @@ json = true "#; fs::write(&config_path, config).context("write netsuke.toml")?; - let merged = merge_in_child( - &["netsuke"], + // As in `with_config_file`, sandbox the child's configuration lookup so the + // precedence assertions cannot be perturbed by host XDG configuration. + let (_xdg_config_dirs, environment) = isolated_environment( temp_dir.path(), &[ ( @@ -186,6 +189,7 @@ json = true (OsString::from("NETSUKE_JOBS"), OsString::from("4")), ], )?; + let merged = merge_in_child(&["netsuke"], temp_dir.path(), &environment)?; ensure!( merged.file.as_path() == Path::new("Configfile"), "config file should override the default manifest path", diff --git a/tests/cli_tests/merge_probe.rs b/tests/cli_tests/merge_probe.rs index b8b261f92..4ac4c28c8 100644 --- a/tests/cli_tests/merge_probe.rs +++ b/tests/cli_tests/merge_probe.rs @@ -87,7 +87,17 @@ pub(super) fn merge_in_child( .current_dir(current_dir) .env_clear(); let process_env = DefaultEnv; - for key in ["SystemRoot", "PATH", "TEMP", "TMP"] { + // `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` are supplied by Cargo so + // the test binary can find its dynamic dependencies; clearing them would + // leave the re-executed worker unable to start on Linux and macOS. + for key in [ + "SystemRoot", + "PATH", + "TEMP", + "TMP", + "LD_LIBRARY_PATH", + "DYLD_FALLBACK_LIBRARY_PATH", + ] { if let Some(value) = process_env.os_string(key) { command.env(key, value); } diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index 0d76dab1a..3d8527194 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -162,17 +162,19 @@ fn renders_env_function_missing_var() -> Result<()> { match manifest::from_str_with_env(&yaml, &reader) { Ok(parsed) => bail!("expected missing env var to error, got manifest {parsed:?}"), Err(err) => { + // Assert the underlying cause first: the outer error type is an + // implementation detail, but a missing variable must always surface + // as a MiniJinja `UndefinedError` somewhere in the chain. + ensure!( + err.chain() + .any(|source| format!("{source:?}").contains("UndefinedError")), + "unexpected error type or message: {err:?}" + ); if let Some(manifest_err) = err.downcast_ref::() { ensure!( matches!(manifest_err, ManifestError::Parse { .. }), "expected ManifestError::Parse, got {manifest_err:?}" ); - } else { - ensure!( - err.chain() - .any(|source| format!("{source:?}").contains("UndefinedError")), - "unexpected error type or message: {err:?}" - ); } // The diagnostic deliberately omits the variable name: environment // variable names routinely identify credentials. diff --git a/tests/novice_flow_smoke_tests.rs b/tests/novice_flow_smoke_tests.rs index 8ee41eead..d278901bd 100644 --- a/tests/novice_flow_smoke_tests.rs +++ b/tests/novice_flow_smoke_tests.rs @@ -23,8 +23,16 @@ fn run_netsuke( args: &[&str], ninja_env: Option<&Path>, ) -> Result { - let ninja = ninja_env.map(|path| path.to_string_lossy()); - let run = match ninja.as_deref() { + // `NETSUKE_NINJA` is set as a string, so a lossy conversion would silently + // point the child at a different executable; fail loudly instead. + let ninja = ninja_env + .map(|path| { + path.to_str().with_context(|| { + format!("ninja override path is not valid UTF-8: {}", path.display()) + }) + }) + .transpose()?; + let run = match ninja { Some(path) => run_netsuke_in_with_env(current_dir, args, &[("NETSUKE_NINJA", path)])?, None => run_netsuke_in(current_dir, args)?, }; diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index 02e3e477c..ab99e1e9c 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -95,14 +95,18 @@ fn assert_required_paths_present(packaged_paths: &BTreeSet<&str>) { fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { for forbidden_root in FORBIDDEN_PACKAGED_ROOTS { + // Name the offending entry: knowing only the forbidden root leaves the + // reader grepping the packaged manifest by hand. + let offender = packaged_paths.iter().find(|path| { + Path::new(path) + .components() + .next() + .is_some_and(|component| component.as_os_str() == forbidden_root) + }); assert!( - packaged_paths.iter().all(|path| { - Path::new(path) - .components() - .next() - .is_none_or(|component| component.as_os_str() != forbidden_root) - }), - "packaged manifest should not contain `{forbidden_root}`" + offender.is_none(), + "packaged manifest should not contain `{forbidden_root}`, found `{}`", + offender.copied().unwrap_or_default() ); } diff --git a/tests/whitaker_boundary_contract.rs b/tests/whitaker_boundary_contract.rs index dd47847ed..197ef53df 100644 --- a/tests/whitaker_boundary_contract.rs +++ b/tests/whitaker_boundary_contract.rs @@ -117,6 +117,26 @@ fn exclusion_list(dylint_toml: &str, key: &str) -> Result> { .collect() } +/// Collect the package names selected by a Makefile recipe line. +/// +/// Substring matching cannot tell `--package test_support` apart from +/// `--package test_support-extra`, nor spot a second selection appended later, +/// so the line is tokenized and whole arguments are compared. Both the +/// separated and `--package=value` spellings are recognized, as is the `-p` +/// short form. +fn package_selections(line: &str) -> Vec<&str> { + let mut tokens = line.split_whitespace(); + let mut packages = Vec::new(); + while let Some(token) = tokens.next() { + if let Some(value) = token.strip_prefix("--package=") { + packages.push(value); + } else if token == "--package" || token == "-p" { + packages.extend(tokens.next()); + } + } + packages +} + #[test] fn lint_whitaker_also_runs_inside_test_support() -> Result<()> { let makefile = read_repo_file(Utf8Path::new("Makefile"))?; @@ -155,12 +175,12 @@ fn lint_whitaker_also_runs_inside_test_support() -> Result<()> { ensure!( scoped.first().is_some_and(|line| { line.contains(r#"DYLINT_TOML="$$(cat dylint.toml)""#) - && line.contains("--package test_support") + && package_selections(line) == ["test_support"] && line.contains("--no-deps") }), concat!( "the scoped Whitaker invocation must explicitly load ", - "test_support/dylint.toml and select only test_support after ", + "test_support/dylint.toml and select exactly test_support after ", "workspace membership changes Cargo's configuration and package ", "roots; found {scoped:?}", ), @@ -177,10 +197,10 @@ fn lint_whitaker_also_runs_inside_test_support() -> Result<()> { ); ensure!( root.first().is_some_and(|line| { - line.contains("--package netsuke-build") && line.contains("--no-deps") + package_selections(line) == ["netsuke-build"] && line.contains("--no-deps") }), concat!( - "the root Whitaker invocation must select only netsuke-build and skip ", + "the root Whitaker invocation must select exactly netsuke-build and skip ", "dependency checks so test_support loads its own dylint.toml; ", "found {root:?}", ), diff --git a/tests/workflow_contracts/ci_lint_test.py b/tests/workflow_contracts/ci_lint_test.py index a51d428c9..49b49fbab 100644 --- a/tests/workflow_contracts/ci_lint_test.py +++ b/tests/workflow_contracts/ci_lint_test.py @@ -39,8 +39,34 @@ def _load() -> dict[str, object]: - """Parse the workflow file.""" - return yaml.safe_load(WORKFLOW_PATH.read_text(encoding="utf-8")) + """Parse the workflow file, rejecting anything but a mapping root. + + ``yaml.safe_load`` happily returns ``None`` for an empty document, or a + scalar or list for a malformed one. Without a runtime check the annotation + is a claim rather than a guarantee, and the failure surfaces later as an + opaque ``AttributeError`` far from the real cause. + + GitHub Actions' ``on:`` trigger key is a YAML 1.1 boolean word, so PyYAML + hands it back as ``True``; it is restored to ``"on"`` so callers see a + uniformly string-keyed mapping. + """ + match yaml.safe_load(WORKFLOW_PATH.read_text(encoding="utf-8")): + case dict() as document: + workflow = { + ("on" if key is True else key): value + for key, value in document.items() + } + case other: + raise AssertionError( + "the workflow must parse to a mapping, " + f"got {type(other).__name__}" + ) + non_string_keys = sorted(repr(key) for key in workflow if not isinstance(key, str)) + if non_string_keys: + raise AssertionError( + f"the workflow mapping must be string-keyed, got {non_string_keys}" + ) + return workflow def _steps(workflow: dict[str, object]) -> list[dict[str, object]]: From d64787da4682c7add3d196a77bbc5bdf2e71aec8 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 01:01:56 +0200 Subject: [PATCH 2/8] Separate manifest telemetry from macro evaluation Address the second review round on #515. Six of the ten findings had already been fixed or never held; the four below were verified against the current tree first. Separate observability from evaluation. `make_macro_fn` created a span, timed with `Instant::now()`, and emitted metrics inside the Jinja callback, mixing instrumentation into what is a query. `render_template` did the same at the template boundary. Both now compose a pure evaluation closure with an instrumentation wrapper in a new `jinja_macros::telemetry` module, which also gives the emitted fields' privacy contract a single place to be reviewed. Cover the macro-invocation boundary with tests. Its counter, histogram, and span appeared only in production code, so deleting them left the suite green. `macro_invocation_telemetry` now pins the success and failure paths and asserts the failure event carries the bounded error category without the macro's name or arguments. Confirmed by deleting the metrics and watching both tests fail. Redact the fixture duration value in `test_support`'s HTTP server. An unparsable override was logged verbatim, putting caller-controlled environment content into the log. The warning now carries the variable name, the bounded parse error, and the value's byte length; the tests assert on that category and additionally assert the value does not appear. An empty-value case joins the table, which the length now distinguishes. Correct two stale developer-guide entries: the executable helpers take `&Path`/`PathBuf` and there is no `exec::utf8_path` conversion point, and `push_file_layers` is really `push_file_layers_with_env`. Not actioned, with reasons: - Imported-macro renders were said to lack instrumentation. They do not: `render_template` has always had its own span and metrics, tested in `macros_telemetry`. A regression test now pins the boundary split. - `env_reader` was said to leak manifest-controlled variable names. It does not; the diagnostics are fixed text and a test asserts the name is absent, which is the documented decision. - The `merge_probe` clone already uses `merged.command.take()`. - `EnvProvider` was said to be renamed to `LocaleEnvProvider`. They are unrelated traits that both still exist, and the distinction is already documented in the developers' guide. - The claimed module cycles do not exist: `ENV_PREFIX` lives in `cli::constants` and `call_macro_value` in the sibling `call` module, which is where the finding asked for them. - Proptests for keyword forwarding and `EnvLock` acquire/drop interleavings already exist. Co-Authored-By: Claude Opus 5 (1M context) --- docs/developers-guide.md | 33 ++- src/manifest/jinja_macros/invocation.rs | 51 +---- src/manifest/jinja_macros/mod.rs | 64 +----- src/manifest/jinja_macros/telemetry.rs | 120 +++++++++++ .../tests/macro_invocation_telemetry.rs | 193 ++++++++++++++++++ src/manifest/tests/mod.rs | 1 + test_support/src/http/mod.rs | 22 +- test_support/src/http/tests.rs | 31 ++- 8 files changed, 388 insertions(+), 127 deletions(-) create mode 100644 src/manifest/jinja_macros/telemetry.rs create mode 100644 src/manifest/tests/macro_invocation_telemetry.rs diff --git a/docs/developers-guide.md b/docs/developers-guide.md index a646f5fe7..35574936d 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1348,25 +1348,20 @@ writes that content and applies executable permissions only on Unix. `write_exec` is the minimal-script convenience wrapper; `write_exec_with_content` is the shared primitive for custom behaviour. -The helpers take `&Utf8Path` and return `Utf8PathBuf`, matching the camino -types used throughout Netsuke. Callers that already hold camino paths pass them -directly. `tempfile::TempDir::path()` still yields an OS-native `&Path`, so -callers convert at that boundary with `exec::utf8_path`, the single conversion -point. `utf8_path` returns a `Result` rather than panicking: it names the -offending path in the error (`path is not valid UTF-8: {path}`), and callers -propagate it with their own context, as `fake_ninja` and -`fake_ninja_check_build_file` do. +The helpers take `&Path` and return `PathBuf`, the OS-native types that +`tempfile::TempDir::path()` already yields. Because the helpers sit at the +`tempfile`/OS boundary, there is no conversion step: callers pass the temporary +directory's path straight through. ```rust let temp = TempDir::new()?; -let root = exec::utf8_path(temp.path()).context("temporary directory")?; -let stub = write_exec(root, "tool")?; +let stub = write_exec(temp.path(), "tool")?; ``` -Because a camino path cannot represent a non-UTF-8 path, the fake-executable -factories now fail on a temporary directory whose path is not valid UTF-8, -rather than succeeding as they previously did. The `test_support` test -`fake_ninja_helpers_reject_non_utf8_temp_directories` pins this behaviour. +Because `write_exec` and `write_exec_with_content` operate on OS-native paths +directly, the fake-executable factories accept a temporary directory whose path +is not valid UTF-8. The `test_support` test +`fake_ninja_helpers_support_non_utf8_temp_directories` pins this behaviour. ### User-facing documentation examples @@ -2185,10 +2180,12 @@ Configuration merge helpers: `PathBuf`. - `explicit_config_path_with_env(cli, env) -> Option` resolves explicit config selection from `--config` and `NETSUKE_CONFIG`. -- `push_file_layers(cli, composer, errors) -> ()` pushes explicit or discovered - file layers onto a `MergeComposer`. Explicit load errors are pushed into - `errors`, and automatic discovery is not attempted after an explicit selector - fails. +- `push_file_layers_with_env(cli, composer, errors, env) -> ()` pushes explicit + or discovered file layers onto a `MergeComposer`. The injected `env` + parameter follows the environment mandate: it supplies environment access + without requiring callers to mutate the process environment. Explicit load + errors are pushed into `errors`, and automatic discovery is not attempted + after an explicit selector fails. - `collect_diag_file_layers_with_env(cli, env)` reuses the same file-layer precedence for early JSON resolution. - `collect_file_layers(directory)` builds the fallback discovery layer chain, diff --git a/src/manifest/jinja_macros/invocation.rs b/src/manifest/jinja_macros/invocation.rs index 39068986c..0ac68a255 100644 --- a/src/manifest/jinja_macros/invocation.rs +++ b/src/manifest/jinja_macros/invocation.rs @@ -1,17 +1,12 @@ //! Safe invocation helpers for manifest-defined Jinja macros. use super::call::call_macro_value; +use super::telemetry; use crate::localization::{self, keys}; -use metrics::{counter, describe_counter, describe_histogram, histogram}; use minijinja::{ AutoEscape, Captured, Environment, Error, ErrorKind, State, value::{Kwargs, Rest, Value}, }; -use std::{sync::Once, time::Instant}; -use tracing::field; - -const MACRO_INVOCATIONS_TOTAL: &str = "netsuke_manifest_macro_invocations_total"; -const MACRO_INVOCATION_DURATION: &str = "netsuke_manifest_macro_invocation_duration_seconds"; #[derive(Clone, Copy)] struct MacroReference<'a> { @@ -25,26 +20,23 @@ struct MacroReference<'a> { /// which is the path that supports Jinja call blocks. Compiled expressions do /// not support imports, so this fallback creates a short-lived captured state /// for each expression call instead of extending its lifetime unsafely. +/// +/// Evaluation stays in [`invoke_macro`]; the callback only composes it with the +/// instrumentation boundary in [`telemetry`], so the query itself carries no +/// timing or metric concerns. pub(super) fn make_macro_fn( template_name: String, macro_name: String, ) -> impl Fn(&State, Rest, Kwargs) -> Result { - describe_metrics(); + telemetry::describe_macro_metrics(); move |state, Rest(args), macro_kwargs| { - let span = tracing::trace_span!( - "manifest.macro.invoke", - outcome = field::Empty, - error_category = field::Empty, - ); - let _guard = span.enter(); - let started = Instant::now(); let reference = MacroReference { template_name: &template_name, macro_name: ¯o_name, }; - let result = invoke_macro(state, args.as_slice(), ¯o_kwargs, reference); - record_invocation(&span, &result, started); - result + telemetry::instrument_macro_invocation(|| { + invoke_macro(state, args.as_slice(), ¯o_kwargs, reference) + }) } } @@ -66,31 +58,6 @@ fn invoke_macro( }) } -fn describe_metrics() { - static DESCRIBE: Once = Once::new(); - DESCRIBE.call_once(|| { - describe_counter!( - MACRO_INVOCATIONS_TOTAL, - "Counts manifest macro invocation outcomes labelled as success or error." - ); - describe_histogram!( - MACRO_INVOCATION_DURATION, - "Measures manifest macro invocation duration in seconds." - ); - }); -} - -fn record_invocation(span: &tracing::Span, result: &Result, started: Instant) { - let outcome = if result.is_ok() { "success" } else { "error" }; - span.record("outcome", outcome); - if let Err(error) = result { - span.record("error_category", format_args!("{:?}", error.kind())); - tracing::debug!(error_category = ?error.kind(), "manifest macro invocation failed"); - } - counter!(MACRO_INVOCATIONS_TOTAL, "outcome" => outcome).increment(1); - histogram!(MACRO_INVOCATION_DURATION).record(started.elapsed()); -} - /// Confirm that a compiled template exports the requested macro. pub(super) fn validate_macro( env: &Environment, diff --git a/src/manifest/jinja_macros/mod.rs b/src/manifest/jinja_macros/mod.rs index 67f17f334..416b9ca05 100644 --- a/src/manifest/jinja_macros/mod.rs +++ b/src/manifest/jinja_macros/mod.rs @@ -9,17 +9,12 @@ use super::ManifestValue; use crate::ast::MacroDefinition; use crate::localization::{self, keys}; use anyhow::{Context, Result}; -use metrics::{counter, describe_counter, describe_histogram, histogram}; use minijinja::{Environment, Error}; use serde::Serialize; -use std::{sync::Once, time::Instant}; -use tracing::field; - -const TEMPLATE_RENDERS_TOTAL: &str = "netsuke_manifest_template_renders_total"; -const TEMPLATE_RENDER_DURATION: &str = "netsuke_manifest_template_render_duration_seconds"; mod call; mod invocation; +mod telemetry; // Only the manifest test suite reaches the helper through the parent path; // `invocation` imports it from the sibling module directly. @@ -148,58 +143,15 @@ pub(crate) fn render_template( template: &str, context: &impl Serialize, ) -> Result { - describe_render_metrics(); + telemetry::describe_render_metrics(); let imports = macro_imports(env); let has_macro_imports = imports.is_some(); - let span = tracing::trace_span!( - "manifest.template.render", - outcome = field::Empty, - has_macro_imports, - error_category = field::Empty, - ); - let _guard = span.enter(); - let started = Instant::now(); - let result = imports.map_or_else( - || env.render_str(template, context), - |import_block| env.render_str(&[import_block.as_str(), template].concat(), context), - ); - record_render(&span, &result, has_macro_imports, started); - result -} - -fn describe_render_metrics() { - static DESCRIBE: Once = Once::new(); - DESCRIBE.call_once(|| { - describe_counter!( - TEMPLATE_RENDERS_TOTAL, - "Counts manifest template renders by bounded outcome and macro-import presence." - ); - describe_histogram!( - TEMPLATE_RENDER_DURATION, - "Measures manifest template rendering duration in seconds." - ); - }); -} - -fn record_render( - span: &tracing::Span, - result: &Result, - has_macro_imports: bool, - started: Instant, -) { - let outcome = if result.is_ok() { "success" } else { "error" }; - span.record("outcome", outcome); - if let Err(error) = result { - span.record("error_category", format_args!("{:?}", error.kind())); - tracing::debug!(error_category = ?error.kind(), "manifest template render failed"); - } - counter!( - TEMPLATE_RENDERS_TOTAL, - "outcome" => outcome, - "has_macro_imports" => if has_macro_imports { "true" } else { "false" }, - ) - .increment(1); - histogram!(TEMPLATE_RENDER_DURATION).record(started.elapsed()); + telemetry::instrument_template_render(has_macro_imports, || { + imports.map_or_else( + || env.render_str(template, context), + |import_block| env.render_str(&[import_block.as_str(), template].concat(), context), + ) + }) } fn register_macro_import(env: &mut Environment<'static>, template_name: &str, macro_name: &str) { diff --git a/src/manifest/jinja_macros/telemetry.rs b/src/manifest/jinja_macros/telemetry.rs new file mode 100644 index 000000000..a0422f17e --- /dev/null +++ b/src/manifest/jinja_macros/telemetry.rs @@ -0,0 +1,120 @@ +//! Bounded observability for manifest template rendering and macro invocation. +//! +//! Rendering a template and invoking a macro are queries: they compute a value +//! and are expected to be free of ambient concerns. Spans, timing, and metric +//! emission therefore live here rather than interleaved with the evaluation +//! logic, so `render_template` and the Jinja callback read as plain evaluation +//! and each instrumented boundary composes explicitly. +//! +//! Collecting the telemetry in one module also gives its privacy contract a +//! single place to be reviewed: every field emitted here is bounded by +//! construction, so manifest-controlled data — template text, macro names, +//! context values, environment variable names — cannot reach a subscriber. + +use metrics::{counter, describe_counter, describe_histogram, histogram}; +use minijinja::Error; +use std::{sync::Once, time::Instant}; +use tracing::field; + +const MACRO_INVOCATIONS_TOTAL: &str = "netsuke_manifest_macro_invocations_total"; +const MACRO_INVOCATION_DURATION: &str = "netsuke_manifest_macro_invocation_duration_seconds"; +const TEMPLATE_RENDERS_TOTAL: &str = "netsuke_manifest_template_renders_total"; +const TEMPLATE_RENDER_DURATION: &str = "netsuke_manifest_template_render_duration_seconds"; + +/// Register the macro-invocation metric descriptions exactly once. +/// +/// Called when a macro is registered rather than per invocation, so the +/// one-time guard never sits on the rendering hot path. +pub(super) fn describe_macro_metrics() { + static DESCRIBE: Once = Once::new(); + DESCRIBE.call_once(|| { + describe_counter!( + MACRO_INVOCATIONS_TOTAL, + "Counts manifest macro invocation outcomes labelled as success or error." + ); + describe_histogram!( + MACRO_INVOCATION_DURATION, + "Measures manifest macro invocation duration in seconds." + ); + }); +} + +/// Register the template-render metric descriptions exactly once. +pub(super) fn describe_render_metrics() { + static DESCRIBE: Once = Once::new(); + DESCRIBE.call_once(|| { + describe_counter!( + TEMPLATE_RENDERS_TOTAL, + "Counts manifest template renders by bounded outcome and macro-import presence." + ); + describe_histogram!( + TEMPLATE_RENDER_DURATION, + "Measures manifest template rendering duration in seconds." + ); + }); +} + +/// Evaluate `invoke` inside a macro-invocation span, recording its outcome. +/// +/// The span and metrics carry only the outcome and, on failure, the `MiniJinja` +/// error kind; the macro's identity and arguments stay out of telemetry. +pub(super) fn instrument_macro_invocation( + invoke: impl FnOnce() -> Result, +) -> Result { + let span = tracing::trace_span!( + "manifest.macro.invoke", + outcome = field::Empty, + error_category = field::Empty, + ); + let _guard = span.enter(); + let started = Instant::now(); + let result = invoke(); + let outcome = outcome_label(&result); + span.record("outcome", outcome); + if let Err(error) = &result { + span.record("error_category", format_args!("{:?}", error.kind())); + tracing::debug!(error_category = ?error.kind(), "manifest macro invocation failed"); + } + counter!(MACRO_INVOCATIONS_TOTAL, "outcome" => outcome).increment(1); + histogram!(MACRO_INVOCATION_DURATION).record(started.elapsed()); + result +} + +/// Evaluate `render` inside a template-render span, recording its outcome. +/// +/// `has_macro_imports` is a bounded shape signal, not content: it distinguishes +/// the import-prefixed render path from the plain one without revealing which +/// macros a manifest defines. +pub(super) fn instrument_template_render( + has_macro_imports: bool, + render: impl FnOnce() -> Result, +) -> Result { + let span = tracing::trace_span!( + "manifest.template.render", + outcome = field::Empty, + has_macro_imports, + error_category = field::Empty, + ); + let _guard = span.enter(); + let started = Instant::now(); + let result = render(); + let outcome = outcome_label(&result); + span.record("outcome", outcome); + if let Err(error) = &result { + span.record("error_category", format_args!("{:?}", error.kind())); + tracing::debug!(error_category = ?error.kind(), "manifest template render failed"); + } + counter!( + TEMPLATE_RENDERS_TOTAL, + "outcome" => outcome, + "has_macro_imports" => if has_macro_imports { "true" } else { "false" }, + ) + .increment(1); + histogram!(TEMPLATE_RENDER_DURATION).record(started.elapsed()); + result +} + +/// Reduce a result to a low-cardinality outcome label. +const fn outcome_label(result: &Result) -> &'static str { + if result.is_ok() { "success" } else { "error" } +} diff --git a/src/manifest/tests/macro_invocation_telemetry.rs b/src/manifest/tests/macro_invocation_telemetry.rs new file mode 100644 index 000000000..0cdcad192 --- /dev/null +++ b/src/manifest/tests/macro_invocation_telemetry.rs @@ -0,0 +1,193 @@ +//! Bounded-telemetry assertions for the macro-invocation boundary. +//! +//! Sibling to `macros_telemetry`, which covers `render_template`. These tests +//! pin the compiled-expression fallback built by `make_macro_fn`: without them +//! the span and metrics could be deleted and the suite would still pass. +//! +//! A local `DebuggingRecorder` captures the counter and histogram without +//! touching the global recorder, and the tracing capture asserts the failure +//! event carries only the bounded error category — never the macro name or its +//! arguments. + +use super::super::jinja_macros::render_template; +use crate::ast::MacroDefinition; +use crate::manifest::jinja_macros::register_macro; +use crate::test_tracing_capture::with_test_subscriber; +use anyhow::{Result as AnyResult, ensure}; +use metrics_util::MetricKind; +use metrics_util::debugging::{DebugValue, DebuggingRecorder}; +use minijinja::{Environment, UndefinedBehavior}; +use rstest::{fixture, rstest}; +use tracing_subscriber::filter::LevelFilter; + +const INVOCATIONS_TOTAL: &str = "netsuke_manifest_macro_invocations_total"; +const INVOCATION_DURATION: &str = "netsuke_manifest_macro_invocation_duration_seconds"; + +/// One drained metrics snapshot; the debugging snapshotter empties +/// histogram samples on read, so it is taken exactly once per test. +type Snapshot = Vec<( + metrics_util::CompositeKey, + Option, + Option, + DebugValue, +)>; + +#[fixture] +fn macro_env() -> Environment<'static> { + let mut env = Environment::new(); + env.set_undefined_behavior(UndefinedBehavior::Strict); + env +} + +/// Run `invoke` under a local recorder and return its result alongside +/// the single drained snapshot. +fn recorded(invoke: impl FnOnce() -> T) -> (T, Snapshot) { + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let value = metrics::with_local_recorder(&recorder, invoke); + (value, snapshotter.snapshot().into_vec()) +} + +fn counter_value(snapshot: &Snapshot, outcome: &str) -> Option { + snapshot + .iter() + .find_map(|(key, _unit, _description, value)| { + if key.kind() != MetricKind::Counter || key.key().name() != INVOCATIONS_TOTAL { + return None; + } + let has_outcome = key + .key() + .labels() + .any(|label| label.key() == "outcome" && label.value() == outcome); + match value { + DebugValue::Counter(count) if has_outcome => Some(*count), + _ => None, + } + }) +} + +fn duration_sample_count(snapshot: &Snapshot) -> usize { + snapshot + .iter() + .find_map(|(key, _unit, _description, value)| { + if key.kind() != MetricKind::Histogram || key.key().name() != INVOCATION_DURATION { + return None; + } + match value { + DebugValue::Histogram(samples) => Some(samples.len()), + _ => None, + } + }) + .unwrap_or_default() +} + +/// Compile `expression` against the registered macro and evaluate it, which +/// routes through the `make_macro_fn` fallback rather than a template import. +fn eval_expression(env: &Environment, expression: &str) -> Result { + env.compile_expression(expression)? + .eval(()) + .map(|value| value.to_string()) +} + +#[rstest] +fn macro_invocation_records_success_telemetry( + mut macro_env: Environment<'static>, +) -> AnyResult<()> { + let definition = MacroDefinition { + signature: "greet(name)".into(), + body: "Hello {{ name }}".into(), + }; + register_macro(&mut macro_env, &definition, 0)?; + + let (rendered, snapshot) = recorded(|| eval_expression(¯o_env, "greet('netsuke')")); + + ensure!(rendered? == "Hello netsuke", "the macro should render"); + ensure!( + counter_value(&snapshot, "success") == Some(1), + "a successful macro invocation should count once" + ); + ensure!( + duration_sample_count(&snapshot) == 1, + "the invocation duration should record one sample" + ); + Ok(()) +} + +#[rstest] +fn failed_macro_invocation_records_error_telemetry_without_macro_details( + mut macro_env: Environment<'static>, +) -> AnyResult<()> { + // The body reads an argument the call site does not supply, so strict + // undefined behaviour fails inside the macro rather than at compile time. + let definition = MacroDefinition { + signature: "reveal(supplied)".into(), + body: "{{ supplied }} {{ s3cr3t_sentinel_variable }}".into(), + }; + register_macro(&mut macro_env, &definition, 0)?; + + let (events, snapshot) = { + let ((result, events), snapshot) = recorded(|| { + with_test_subscriber(LevelFilter::DEBUG, |captured| { + let result = eval_expression(¯o_env, "reveal('visible')"); + (result, captured.snapshot()) + }) + }); + ensure!( + result.is_err(), + "a strict undefined lookup inside the macro should fail the invocation" + ); + (events, snapshot) + }; + + ensure!( + counter_value(&snapshot, "error") == Some(1), + "a failed macro invocation should count once with the error outcome" + ); + ensure!( + duration_sample_count(&snapshot) == 1, + "the failed invocation duration should still record one sample" + ); + ensure!( + events + .iter() + .any(|event| event.contains("manifest macro invocation failed") + && event.contains("error_category=")), + "expected a bounded invocation-failure event in {events:?}" + ); + ensure!( + !events + .iter() + .any(|event| event.contains("s3cr3t_sentinel_variable") + || event.contains("reveal") + || event.contains("visible")), + "macro identity and arguments must not reach telemetry: {events:?}" + ); + Ok(()) +} + +/// The imported-macro path is metered at the render boundary, so a template +/// render must not also emit macro-invocation metrics. This pins the boundary +/// split that keeps the two counters independently meaningful. +#[rstest] +fn imported_macro_render_does_not_emit_invocation_metrics( + mut macro_env: Environment<'static>, +) -> AnyResult<()> { + let definition = MacroDefinition { + signature: "greet(name)".into(), + body: "Hello {{ name }}".into(), + }; + register_macro(&mut macro_env, &definition, 0)?; + + let (rendered, snapshot) = + recorded(|| render_template(¯o_env, "{{ greet('netsuke') }}", &())); + + ensure!( + rendered? == "Hello netsuke", + "the imported macro should render" + ); + ensure!( + counter_value(&snapshot, "success").is_none(), + "the import path renders without the compiled-expression fallback" + ); + Ok(()) +} diff --git a/src/manifest/tests/mod.rs b/src/manifest/tests/mod.rs index 02e1cf66e..b30cf61fc 100644 --- a/src/manifest/tests/mod.rs +++ b/src/manifest/tests/mod.rs @@ -1,5 +1,6 @@ //! Tests for manifest parsing and macro helpers. +mod macro_invocation_telemetry; mod macros; mod macros_telemetry; mod stages; diff --git a/test_support/src/http/mod.rs b/test_support/src/http/mod.rs index 39ddc22fe..a1fbc59b4 100644 --- a/test_support/src/http/mod.rs +++ b/test_support/src/http/mod.rs @@ -290,22 +290,36 @@ fn duration_from_env(env: &impl Env, var: &str, default: Duration) -> Duration { match trimmed.parse::() { Ok(ms) => Duration::from_millis(ms), Err(err) => { - log_duration_parse_error(var, value.as_str(), &err); + log_duration_parse_error(var, trimmed.len(), &err); default } } }) } -fn log_duration_parse_error(var: &str, value: &str, err: &dyn fmt::Display) { +/// Report an unparsable duration override without echoing its value. +/// +/// The value is redacted: an environment variable's contents are outside this +/// crate's control, and logging them verbatim would put whatever the caller +/// exported into the log. `err` already names the bounded parse failure, and +/// `value_len` distinguishes an empty override from a malformed one, which is +/// all the diagnosis this fixture needs. +fn log_duration_parse_error(var: &str, value_len: usize, err: &dyn fmt::Display) { #[cfg(test)] { - record_duration_warning(format!("ignoring invalid {var}='{value}': {err}")); + record_duration_warning(format!( + "ignoring invalid {var}: {err} (value redacted, {value_len} bytes)" + )); } #[cfg(not(test))] { - tracing::warn!(variable = var, value, error = %err, "ignoring invalid fixture duration"); + tracing::warn!( + variable = var, + value_len, + error = %err, + "ignoring invalid fixture duration" + ); } } diff --git a/test_support/src/http/tests.rs b/test_support/src/http/tests.rs index 0280e4a04..4809d3107 100644 --- a/test_support/src/http/tests.rs +++ b/test_support/src/http/tests.rs @@ -56,7 +56,11 @@ struct DurationCase { key: &'static str, value: Option<&'static str>, expected: Duration, - expected_warning_value: Option<&'static str>, + /// Bounded parse-failure text expected in the warning, if it should warn. + /// + /// Deliberately not the offending value: the warning redacts it, so + /// asserting on a category is what keeps that redaction honest. + expected_warning_error: Option<&'static str>, } #[test] @@ -102,19 +106,25 @@ fn from_env_clamps_zero_poll_interval() { key: ENV_HTTP_ACCEPT_TIMEOUT_MS, value: None, expected: Duration::from_secs(3), - expected_warning_value: None, + expected_warning_error: None, })] #[case::invalid(DurationCase { key: ENV_HTTP_ACCEPT_TIMEOUT_MS, value: Some("not-a-number"), expected: Duration::from_secs(3), - expected_warning_value: Some("not-a-number"), + expected_warning_error: Some("invalid digit"), +})] +#[case::empty(DurationCase { + key: ENV_HTTP_ACCEPT_TIMEOUT_MS, + value: Some(""), + expected: Duration::from_secs(3), + expected_warning_error: Some("cannot parse integer from empty string"), })] #[case::whitespace_padded(DurationCase { key: ENV_HTTP_READ_TIMEOUT_MS, value: Some(" 2500 "), expected: Duration::from_millis(2500), - expected_warning_value: None, + expected_warning_error: None, })] fn duration_from_env_handles_input( empty_duration_warnings: EmptyDurationWarnings, @@ -129,7 +139,7 @@ fn duration_from_env_handles_input( assert_eq!(duration, case.expected); let warnings = empty_duration_warnings.take(); - if let Some(expected_value) = case.expected_warning_value { + if let Some(expected_error) = case.expected_warning_error { assert_eq!(warnings.len(), 1); let warning = warnings.first().map_or("", String::as_str); assert!( @@ -137,9 +147,16 @@ fn duration_from_env_handles_input( "warning should mention the variable name" ); assert!( - warning.contains(expected_value), - "warning should include the invalid value" + warning.contains(expected_error), + "warning should name the bounded parse failure, got {warning}" ); + // The value is caller-controlled, so it must never reach the log. + if let Some(configured_value) = case.value.filter(|value| !value.is_empty()) { + assert!( + !warning.contains(configured_value), + "warning must redact the offending value, got {warning}" + ); + } } else { assert!( warnings.is_empty(), From 95fe2e8fbbc92b4d08c9da7840f5b5c4347b7b84 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 03:46:50 +0200 Subject: [PATCH 3/8] Harden telemetry redaction with property coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the third review round on #515. Four findings held; one was half right, and the reasons for each are below. Stop the poisoning-recovery test leaking poison on failure. `env_lock_recovers_after_mutex_poisoning` asserted the held state while `recovered_guard` was still live. Had that assertion failed, unwinding would have dropped the guard mid-panic and poisoned `ENV_LOCK` afresh — the very leak the test's own doc comment claimed to have covered, since clearing the flag earlier only guards the deliberate poisoning. The state is now read through a non-asserting `current_thread_lock_is_held`, the guard dropped, and only then asserted; the doc comment no longer overclaims. Add property coverage for the redaction invariants. Both were pinned only by fixed sentinels, so they held for the sentinel and said nothing about the range: - `macro_telemetry_stays_bounded_for_arbitrary_macros` generates macro names, arguments, and undefined-variable names, then asserts the outcome labels stay within `{success, error}` and that none of the generated identifiers reach a tracing event. Verified by making the failure event log the full `MiniJinja` error instead of its kind: the test fails, naming the leaked macro. - `invalid_duration_warnings_are_composed_only_of_bounded_parts` generates unparsable overrides and asserts the rendered warning equals a message rebuilt from bounded parts alone. Exact match rather than a `!contains` check, which a value of `bytes` would satisfy while sitting in plain sight. Pin the redacted value's byte length in the duration table via a new `expected_warning_len`, measured after trimming to match the call site. It is the one piece of shape the warning still surfaces, so it should not go missing — or revert to the value — unnoticed. Parse the workflow with a YAML 1.2 boolean resolver. Mapping `True` back to `"on"` conflated GitHub Actions' `on:` with a literal `yes:` or `true:` key; under YAML 1.1 those three collapse into a single key, silently dropping entries. Narrowing the resolver to `true`/`false` leaves `on` a string, so the normalization disappears rather than being patched. Use `camino::Utf8Path` in the packaging forbidden-root check, per the project's stated preference over `std::path`. The finding also asked for an exact root match to keep names such as `test_support-extra` allowed; that already held, because comparing whole path components is exact already — only the type changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/macro_invocation_telemetry.rs | 81 +++++++++++++++++++ test_support/src/env_lock.rs | 31 +++++-- test_support/src/http/tests.rs | 58 +++++++++++++ tests/packaging_smoke_tests.rs | 13 +-- tests/workflow_contracts/ci_lint_test.py | 44 +++++++--- 5 files changed, 205 insertions(+), 22 deletions(-) diff --git a/src/manifest/tests/macro_invocation_telemetry.rs b/src/manifest/tests/macro_invocation_telemetry.rs index 0cdcad192..546fa3292 100644 --- a/src/manifest/tests/macro_invocation_telemetry.rs +++ b/src/manifest/tests/macro_invocation_telemetry.rs @@ -165,6 +165,87 @@ fn failed_macro_invocation_records_error_telemetry_without_macro_details( Ok(()) } +/// Collect every `outcome` label recorded against the invocation counter. +/// +/// Used to prove the label set stays bounded rather than echoing manifest +/// content into a metric dimension, which would make the series unbounded. +fn outcome_labels(snapshot: &Snapshot) -> Vec { + snapshot + .iter() + .filter(|(key, _unit, _description, _value)| { + key.kind() == MetricKind::Counter && key.key().name() == INVOCATIONS_TOTAL + }) + .flat_map(|(key, _unit, _description, _value)| { + key.key() + .labels() + .filter(|label| label.key() == "outcome") + .map(|label| label.value().to_owned()) + .collect::>() + }) + .collect() +} + +proptest::proptest! { + /// The bounded-telemetry contract must hold for any macro a manifest can + /// define, not just the fixed sentinels above. + /// + /// Generated identifiers carry a `zq` prefix so an incidental substring + /// match cannot be mistaken for a leak: a bare generated name of `a` would + /// otherwise "appear" in every event that contains the letter. + #[test] + fn macro_telemetry_stays_bounded_for_arbitrary_macros( + name_suffix in "[a-z][a-z0-9_]{0,10}", + arg_suffix in "[a-zA-Z0-9 ._/-]{0,20}", + undefined_suffix in "[a-z][a-z0-9_]{0,10}", + ) { + let macro_name = format!("zqmacro_{name_suffix}"); + let undefined = format!("zqundef_{undefined_suffix}"); + let argument = format!("zqarg_{arg_suffix}"); + + let mut env = Environment::new(); + env.set_undefined_behavior(UndefinedBehavior::Strict); + // The body reads an undefined name, so evaluation fails inside the macro + // and exercises the error path with caller-controlled identifiers. + let definition = MacroDefinition { + signature: format!("{macro_name}(supplied)"), + body: format!("{{{{ supplied }}}} {{{{ {undefined} }}}}"), + }; + register_macro(&mut env, &definition, 0) + .map_err(|error| proptest::test_runner::TestCaseError::fail(error.to_string()))?; + + let ((result, events), snapshot) = recorded(|| { + with_test_subscriber(LevelFilter::DEBUG, |captured| { + let result = eval_expression(&env, &format!("{macro_name}('{argument}')")); + (result, captured.snapshot()) + }) + }); + + proptest::prop_assert!( + result.is_err(), + "an undefined lookup inside the macro should fail the invocation" + ); + proptest::prop_assert_eq!(counter_value(&snapshot, "error"), Some(1)); + proptest::prop_assert_eq!(duration_sample_count(&snapshot), 1); + // Metric dimensions must stay drawn from the fixed outcome vocabulary. + for label in outcome_labels(&snapshot) { + proptest::prop_assert!( + label == "success" || label == "error", + "outcome label must stay bounded, got {}", + label + ); + } + for event in &events { + proptest::prop_assert!( + !event.contains(¯o_name) + && !event.contains(&undefined) + && !event.contains(&argument), + "manifest-controlled data must not reach telemetry: {}", + event + ); + } + } +} + /// The imported-macro path is metered at the render boundary, so a template /// render must not also emit macro-invocation metrics. This pins the boundary /// split that keeps the two counters independently meaningful. diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index db142fd7a..2af6fddd0 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -86,11 +86,20 @@ mod tests { use proptest::prelude::{Just, Strategy, prop_oneof}; use std::{sync::mpsc, time::Duration}; - fn assert_current_thread_lock_is_held(message: &str) { + /// Read the thread-local lock state without asserting. + /// + /// Callers holding a live [`EnvLock`] need this: asserting while the guard + /// is alive would drop it during unwind, and dropping a `MutexGuard` while + /// panicking poisons the mutex. + fn current_thread_lock_is_held() -> bool { ENV_LOCK_STATE.with(|lock_state| { let state = lock_state.borrow(); - assert!(state.depth > 0 && state.guard.is_some(), "{message}"); - }); + state.depth > 0 && state.guard.is_some() + }) + } + + fn assert_current_thread_lock_is_held(message: &str) { + assert!(current_thread_lock_is_held(), "{message}"); } fn assert_current_thread_lock_is_released(message: &str) { @@ -241,9 +250,13 @@ mod tests { /// Probes `ENV_LOCK` directly, so it requires per-test process isolation /// (the suite runs under `cargo nextest`, which forks each test). Under a /// thread-parallel runner it would race any other test touching the global - /// mutex. The poison flag is cleared before the state is asserted so a - /// failing assertion cannot leak process-global poisoning into whatever - /// else shares the process. + /// mutex. + /// + /// Every assertion here is made with no guard live and the poison flag + /// already cleared, so a failure cannot leave process-global poisoning + /// behind: clearing the flag covers the deliberate poisoning, and capturing + /// the held state before dropping the guard covers the unwind path, where a + /// panic would otherwise drop the `MutexGuard` and poison the mutex afresh. #[test] fn env_lock_recovers_after_mutex_poisoning() { let poisoner = std::thread::spawn(|| { @@ -257,8 +270,12 @@ mod tests { assert!(was_poisoned, "the panic should poison the underlying mutex"); let recovered_guard = EnvLock::acquire(); - assert_current_thread_lock_is_held("recovered EnvLock guard should hold ENV_LOCK"); + let held_while_live = current_thread_lock_is_held(); drop(recovered_guard); + assert!( + held_while_live, + "recovered EnvLock guard should hold ENV_LOCK" + ); assert_current_thread_lock_is_released("recovered ENV_LOCK should be released normally"); } } diff --git a/test_support/src/http/tests.rs b/test_support/src/http/tests.rs index 4809d3107..bbf65201c 100644 --- a/test_support/src/http/tests.rs +++ b/test_support/src/http/tests.rs @@ -61,6 +61,12 @@ struct DurationCase { /// Deliberately not the offending value: the warning redacts it, so /// asserting on a category is what keeps that redaction honest. expected_warning_error: Option<&'static str>, + /// Byte length the warning should report for the redacted value. + /// + /// Measured after trimming, matching the call site. This is the one piece of + /// shape the redaction still surfaces, so pinning it stops the length going + /// missing — or turning back into the value — unnoticed. + expected_warning_len: Option, } #[test] @@ -107,24 +113,28 @@ fn from_env_clamps_zero_poll_interval() { value: None, expected: Duration::from_secs(3), expected_warning_error: None, + expected_warning_len: None, })] #[case::invalid(DurationCase { key: ENV_HTTP_ACCEPT_TIMEOUT_MS, value: Some("not-a-number"), expected: Duration::from_secs(3), expected_warning_error: Some("invalid digit"), + expected_warning_len: Some("not-a-number".len()), })] #[case::empty(DurationCase { key: ENV_HTTP_ACCEPT_TIMEOUT_MS, value: Some(""), expected: Duration::from_secs(3), expected_warning_error: Some("cannot parse integer from empty string"), + expected_warning_len: Some(0), })] #[case::whitespace_padded(DurationCase { key: ENV_HTTP_READ_TIMEOUT_MS, value: Some(" 2500 "), expected: Duration::from_millis(2500), expected_warning_error: None, + expected_warning_len: None, })] fn duration_from_env_handles_input( empty_duration_warnings: EmptyDurationWarnings, @@ -150,6 +160,12 @@ fn duration_from_env_handles_input( warning.contains(expected_error), "warning should name the bounded parse failure, got {warning}" ); + if let Some(expected_len) = case.expected_warning_len { + assert!( + warning.contains(&format!("{expected_len} bytes")), + "warning should report the redacted value's byte length, got {warning}" + ); + } // The value is caller-controlled, so it must never reach the log. if let Some(configured_value) = case.value.filter(|value| !value.is_empty()) { assert!( @@ -165,6 +181,48 @@ fn duration_from_env_handles_input( } } +proptest::proptest! { + /// The redaction must hold for any value a caller might export, not just + /// the table's sentinels. + /// + /// Asserting the whole rendered warning against a message rebuilt from + /// bounded parts is stronger than a "does not contain the value" check: it + /// leaves the value nowhere to hide, and it cannot be fooled by a generated + /// value that happens to be a substring of the template itself — `bytes`, + /// for instance, would satisfy a naive `!contains` assertion. + #[test] + fn invalid_duration_warnings_are_composed_only_of_bounded_parts( + raw in r"[^0-9\s][^\s]{0,24}", + ) { + let trimmed = raw.trim(); + // A leading `+` still parses as u64, so filter rather than assume the + // strategy only yields rejects. + proptest::prop_assume!(trimmed.parse::().is_err()); + let parse_error = trimmed + .parse::() + .expect_err("guarded by the assumption above"); + + // Drain any residue so this case observes only the warning it caused. + drop(take_duration_warnings()); + let env = fixture_env(&[(ENV_HTTP_ACCEPT_TIMEOUT_MS, raw.as_str())]); + let default = Duration::from_secs(3); + + let duration = duration_from_env(&env, ENV_HTTP_ACCEPT_TIMEOUT_MS, default); + + proptest::prop_assert_eq!(duration, default); + let warnings = take_duration_warnings(); + proptest::prop_assert_eq!(warnings.len(), 1); + // The variable name is a crate constant, the parse error is one of + // `ParseIntError`'s fixed messages, and the length is a number: an exact + // match therefore proves no caller-supplied byte reached the log. + let expected = format!( + "ignoring invalid {ENV_HTTP_ACCEPT_TIMEOUT_MS}: {parse_error} (value redacted, {} bytes)", + trimmed.len() + ); + proptest::prop_assert_eq!(warnings.first().cloned().unwrap_or_default(), expected); + } +} + #[test] fn accept_connection_respects_accept_timeout() -> anyhow::Result<()> { let listener = TcpListener::bind(("127.0.0.1", 0))?; diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index ab99e1e9c..ee823e51e 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -4,10 +4,10 @@ //! build-script sources remain in its manifest, where an omission would //! otherwise fail only during release. +use camino::Utf8Path; use netsuke::locale_catalogues::SUPPORTED_LOCALES; use std::collections::BTreeSet; use std::env; -use std::path::Path; use std::process::Command; const REQUIRED_PACKAGED_FILES: [&str; 9] = [ @@ -97,11 +97,14 @@ fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { for forbidden_root in FORBIDDEN_PACKAGED_ROOTS { // Name the offending entry: knowing only the forbidden root leaves the // reader grepping the packaged manifest by hand. + // `cargo package --list` emits UTF-8 relative paths, so camino applies + // here as it does elsewhere in the project, and comparing whole + // components keeps neighbours such as `test_support-extra` allowed. let offender = packaged_paths.iter().find(|path| { - Path::new(path) + Utf8Path::new(path) .components() .next() - .is_some_and(|component| component.as_os_str() == forbidden_root) + .is_some_and(|component| component.as_str() == forbidden_root) }); assert!( offender.is_none(), @@ -111,9 +114,9 @@ fn assert_forbidden_roots_absent(packaged_paths: &BTreeSet<&str>) { } assert!( - packaged_paths.iter().all(|path| Path::new(path) + packaged_paths.iter().all(|path| Utf8Path::new(path) .components() - .all(|component| { component.as_os_str() != "ninja_env" })), + .all(|component| { component.as_str() != "ninja_env" })), "packaged manifest should not contain stale `ninja_env` paths" ); } diff --git a/tests/workflow_contracts/ci_lint_test.py b/tests/workflow_contracts/ci_lint_test.py index 49b49fbab..3bb07a873 100644 --- a/tests/workflow_contracts/ci_lint_test.py +++ b/tests/workflow_contracts/ci_lint_test.py @@ -38,6 +38,33 @@ EXPECTED_CLIPPY_FLAGS = "--workspace --all-targets --all-features -- -D warnings" +class _WorkflowLoader(yaml.SafeLoader): + """Loader that resolves booleans the YAML 1.2 way. + + PyYAML implements YAML 1.1, where ``on``, ``yes``, and ``off`` are boolean + words. That silently turns GitHub Actions' ``on:`` trigger key into + ``True``. Mapping ``True`` back to ``"on"`` after the fact would conflate it + with a literal ``yes:`` or ``true:`` key, so the resolver is narrowed to + YAML 1.2's ``true``/``false`` instead and ``on`` simply stays a string. + """ + + +# Drop the inherited YAML 1.1 bool resolver, then reinstate the 1.2 word set. +_WorkflowLoader.yaml_implicit_resolvers = { + initial: [ + (tag, regexp) + for tag, regexp in resolvers + if tag != "tag:yaml.org,2002:bool" + ] + for initial, resolvers in yaml.SafeLoader.yaml_implicit_resolvers.items() +} +_WorkflowLoader.add_implicit_resolver( + "tag:yaml.org,2002:bool", + re.compile(r"^(?:true|True|TRUE|false|False|FALSE)$"), + list("tTfF"), +) + + def _load() -> dict[str, object]: """Parse the workflow file, rejecting anything but a mapping root. @@ -45,17 +72,14 @@ def _load() -> dict[str, object]: scalar or list for a malformed one. Without a runtime check the annotation is a claim rather than a guarantee, and the failure surfaces later as an opaque ``AttributeError`` far from the real cause. - - GitHub Actions' ``on:`` trigger key is a YAML 1.1 boolean word, so PyYAML - hands it back as ``True``; it is restored to ``"on"`` so callers see a - uniformly string-keyed mapping. """ - match yaml.safe_load(WORKFLOW_PATH.read_text(encoding="utf-8")): - case dict() as document: - workflow = { - ("on" if key is True else key): value - for key, value in document.items() - } + # `yaml.load` is safe here: `_WorkflowLoader` derives from `SafeLoader`, so + # it constructs no arbitrary Python objects. + match yaml.load( + WORKFLOW_PATH.read_text(encoding="utf-8"), Loader=_WorkflowLoader + ): + case dict() as workflow: + pass case other: raise AssertionError( "the workflow must parse to a mapping, " From 1eb2aca909f5fcf1a13cf3bd6546b23519d3206b Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 11:56:38 +0200 Subject: [PATCH 4/8] Cover IR environment injection and document telemetry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the fourth review round on #515. All three findings held. Finish the job started last round in `env_lock`. Capturing the lock state before asserting fixed only the poisoning-recovery test; the two reentrant tests still called `assert_current_thread_lock_is_held` with guards live, so a failure there would drop a guard mid-unwind and poison `ENV_LOCK`, burying the real cause. Both now capture their observations, release the guards, then assert, preserving the original messages. The helper had no remaining callers and is gone; `current_thread_lock_is_held` stays. Test the IR environment injection. Compiling a manifest to IR was switched to `from_path_with_policy_and_env`, but no scenario combined a forwarded variable with `compiled to IR`, so reverting to `from_path` left the suite green. A new fixture derives both a target name and an input path from `env(...)`, and two scenarios pin the outcome: one asserts the resulting graph edge, the other that generation fails when the variable is unset. Confirmed by reverting the step to `from_path` — the first scenario fails because the graph never builds. The value had to travel through a target name rather than a command: `BuildEdge` carries no recipe, so `ir.rs` exposes no step that can assert command text. Cover the non-UTF-8 `NETSUKE_NINJA` branch too, Unix-only since building such a path needs POSIX byte semantics. Document the telemetry boundary. A new module arrived with a published metric contract and a redaction policy but no record of either. ADR-008 records the decision to keep observability out of manifest evaluation and telemetry bounded and redacted, with the alternatives; the developers' guide now describes both instrumentation boundaries, the label vocabulary, the redaction rule, and where the contract is tested. The design document and documentation index reference the ADR. Co-Authored-By: Claude Opus 5 (1M context) --- ...009-bounded-redacted-manifest-telemetry.md | 156 ++++++++++++++++++ docs/contents.md | 3 + docs/developers-guide.md | 72 ++++++++ docs/netsuke-design.md | 8 + test_support/src/env_lock.rs | 36 ++-- tests/data/ir_env.yml | 9 + tests/features/ir.feature | 10 ++ tests/novice_flow_smoke_tests.rs | 30 +++- 8 files changed, 307 insertions(+), 17 deletions(-) create mode 100644 docs/adr-009-bounded-redacted-manifest-telemetry.md create mode 100644 tests/data/ir_env.yml diff --git a/docs/adr-009-bounded-redacted-manifest-telemetry.md b/docs/adr-009-bounded-redacted-manifest-telemetry.md new file mode 100644 index 000000000..274dc7bb0 --- /dev/null +++ b/docs/adr-009-bounded-redacted-manifest-telemetry.md @@ -0,0 +1,156 @@ +# Architecture decision record (ADR): Bounded, redacted telemetry for manifest evaluation + +## Status + +Accepted. + +## Date + +2026-08-06. + +## Context and problem statement + +Manifest evaluation renders Jinja templates and invokes manifest-defined +macros. Both are queries: they compute a value and were expected to stay free +of ambient concerns such as timing and metric emission. Once observability was +needed for these paths, two risks had to be weighed against each other: + +- Manifest content — template text, macro names, macro arguments, and context + values — is caller-controlled and unbounded. Recording it directly in a + metric label produces unbounded cardinality in the metric series, and + recording it in a trace risks leaking secrets, because environment variable + names routinely identify credentials (`src/manifest/env_reader.rs` already + applies this rule to `env()` lookup failures). +- Interleaving spans and metric emission with the evaluation logic in + `render_template` and the macro-invocation callback would make those + functions read as instrumentation rather than plain evaluation, and would + scatter the redaction contract across every call site instead of collecting + it in one place a reviewer can audit. + +`AGENTS.md` also constrains the shape of the answer: libraries may emit +`metrics` and `tracing` instrumentation but must not install global recorders +or subscribers; only the application initializes those once, at startup. + +## Decision + +Collect manifest telemetry in `src/manifest/jinja_macros/telemetry.rs`, kept +separate from evaluation, with two distinct instrumentation boundaries: + +- **Template render** — `manifest::jinja_macros::render_template` composes + `telemetry::instrument_template_render`, which wraps the render in the + `manifest.template.render` span, increments the + `netsuke_manifest_template_renders_total` counter, and records the + `netsuke_manifest_template_render_duration_seconds` histogram. +- **Macro invocation** — the compiled-expression fallback built by + `make_macro_fn` (`src/manifest/jinja_macros/invocation.rs`) composes + `telemetry::instrument_macro_invocation`, which wraps the invocation in the + `manifest.macro.invoke` span, increments the + `netsuke_manifest_macro_invocations_total` counter, and records the + `netsuke_manifest_macro_invocation_duration_seconds` histogram. + +Macros reached through a template import are metered only at the render +boundary; the macro-invocation counter covers the compiled-expression fallback +only, because imports evaluate inside the render call and never reach +`make_macro_fn`. `macro_invocation_telemetry.rs`'s +`imported_macro_render_does_not_emit_invocation_metrics` test pins this split. + +The label and field vocabulary is bounded by construction: + +- `outcome` is always `"success"` or `"error"`. +- The render boundary adds `has_macro_imports`, `"true"` or `"false"`. +- On failure, both boundaries add `error_category`, the `Debug` form of + `minijinja::ErrorKind`, never the error's `Display` text, which can embed + manifest content. + +Template text, macro names, macro arguments, context values, and environment +variable names never reach a span field or a metric label. + +## Rationale + +- **Queries stay pure.** Keeping instrumentation in one module lets + `render_template` and the macro-invocation callback compose an + instrumentation boundary explicitly rather than interleave timing and metric + code with evaluation. +- **One place to audit the privacy contract.** Every field emitted by + `telemetry.rs` is enumerated in that module, so confirming that manifest + content cannot reach a subscriber is a single, small review rather than an + audit of every call site. +- **Two boundaries, not one.** Template rendering and macro invocation are + different operations with different failure shapes and different call + patterns (one render can invoke many macros); merging them into a single + counter would hide the render/invocation split that + `imported_macro_render_does_not_emit_invocation_metrics` depends on. +- **Bounded labels prevent cardinality blowups.** `outcome`, + `has_macro_imports`, and `error_category` are drawn from small, fixed + vocabularies, so the metric series stays bounded regardless of how many + distinct manifests, macros, or templates are evaluated. +- **`error_category` uses `Debug`, not `Display`.** `minijinja::ErrorKind`'s + `Debug` form is a fixed enum variant name; the `Display` text of a + `minijinja::Error` can embed manifest content such as variable names. +- **Matches the existing environment-name redaction rule.** `env_var_with` in + `src/manifest/env_reader.rs` already omits the variable name from both + tracing and the returned Jinja error, for the same reason: manifest-supplied + names routinely identify credentials. + +## Consequences + +- New telemetry fields for these two boundaries must be added to + `telemetry.rs` and reviewed against the redaction contract before merging; + they must not be added ad hoc at the render or invocation call sites. +- `src/manifest/tests/macros_telemetry.rs` and + `src/manifest/tests/macro_invocation_telemetry.rs` pin the counter and + histogram names, the bounded label vocabulary, and the render/invocation + boundary split using a local `metrics_util::debugging::DebuggingRecorder` and + the workspace's tracing capture helper, so neither the global recorder nor + the global subscriber is touched by the test suite. + `macro_invocation_telemetry.rs` also runs a proptest, + `macro_telemetry_stays_bounded_for_arbitrary_macros`, asserting the redaction + contract for arbitrary generated macro names, arguments, and + undefined-variable names, not only the fixed sentinel cases. +- The counter and histogram names + (`netsuke_manifest_template_renders_total`, + `netsuke_manifest_template_render_duration_seconds`, + `netsuke_manifest_macro_invocations_total`, + `netsuke_manifest_macro_invocation_duration_seconds`) are a published + contract for anything scraping these metrics; renaming them is a breaking + change. +- Because the module only calls `metrics` and `tracing` macros and never + installs a recorder or subscriber, the application remains free to choose or + omit an exporter at startup without any change to manifest evaluation. + +## Alternatives considered + +- **Instrument inline in the evaluation path.** Rejected. Interleaving spans + and metric emission directly inside `render_template` and the macro-callback + closure would make the redaction contract implicit at each call site instead + of reviewable in one module, and would make `render_template` read as + instrumentation-plus-evaluation rather than plain evaluation composed with an + explicit instrumentation boundary. +- **Inject a telemetry-sink trait.** Rejected. The `metrics` crate already + provides that abstraction: `counter!`/`histogram!`/`describe_*!` route + through whatever recorder the application installs, and `tracing` provides + the equivalent for spans and events through the subscriber. A bespoke sink + trait would duplicate that abstraction while still needing the same redaction + discipline, and `AGENTS.md` already forbids this module from installing a + recorder or subscriber itself. +- **One shared counter and histogram for both boundaries.** Rejected. Merging + render and invocation telemetry would erase the render/invocation split that + distinguishes the import path from the compiled-expression fallback, making + it impossible to answer "did this render's macro imports run, or fall back to + compiled-expression invocation?" from the metrics alone. + +## Implementation references + +- Telemetry module: + [`src/manifest/jinja_macros/telemetry.rs`](../src/manifest/jinja_macros/telemetry.rs) +- Render boundary composition: + [`src/manifest/jinja_macros/mod.rs`](../src/manifest/jinja_macros/mod.rs) +- Macro-invocation boundary composition: + [`src/manifest/jinja_macros/invocation.rs`](../src/manifest/jinja_macros/invocation.rs) +- Matching environment-name redaction rule: + [`src/manifest/env_reader.rs`](../src/manifest/env_reader.rs) +- Tests: + [`src/manifest/tests/macros_telemetry.rs`](../src/manifest/tests/macros_telemetry.rs), + [`src/manifest/tests/macro_invocation_telemetry.rs`](../src/manifest/tests/macro_invocation_telemetry.rs) +- Developer guide: + [`docs/developers-guide.md`](developers-guide.md#manifest-telemetry-template-render-and-macro-invocation) diff --git a/docs/contents.md b/docs/contents.md index 0b15a53ff..1e9461087 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -49,6 +49,9 @@ operator, user, and contributor references are easier to find. - [adr-008-environment-seam-taxonomy.md](adr-008-environment-seam-taxonomy.md): Environment seam taxonomy decision record: three sanctioned shapes for injecting environment-dependent input instead of reading the process directly. +- [adr-009-bounded-redacted-manifest-telemetry.md](adr-009-bounded-redacted-manifest-telemetry.md): + Manifest telemetry decision record, separating observability from evaluation + and bounding and redacting the emitted metrics and spans. ## User and operator guides diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 35574936d..ad60c8c27 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2623,6 +2623,78 @@ MiniJinja state on each invocation, so it must not be treated as a reusable global template cache. Errors remain at the manifest boundary and retain their localized failure category. +### Manifest telemetry: template render and macro invocation + +`src/manifest/jinja_macros/telemetry.rs` instruments the two boundaries above +with `tracing` spans and `metrics` counters/histograms, kept out of the +evaluation code so `render_template` and the macro-invocation callback stay +plain queries. See [ADR-009](adr-009-bounded-redacted-manifest-telemetry.md) +for the decision to separate observability from evaluation this way, and for +the alternatives it rejected. + +There are two independent boundaries, because template rendering and macro +invocation are different operations with different failure shapes: + +- **Template render.** `render_template` composes + `telemetry::instrument_template_render` around the render. It opens the + `manifest.template.render` span, increments the + `netsuke_manifest_template_renders_total` counter, and records the + `netsuke_manifest_template_render_duration_seconds` histogram. +- **Macro invocation.** `make_macro_fn`'s compiled-expression fallback + composes `telemetry::instrument_macro_invocation` around the invocation. It + opens the `manifest.macro.invoke` span, increments the + `netsuke_manifest_macro_invocations_total` counter, and records the + `netsuke_manifest_macro_invocation_duration_seconds` histogram. + +Macros reached through a template import run inside the render call and never +reach `make_macro_fn`, so they are metered at the render boundary only; the +macro-invocation counter covers the compiled-expression fallback. The test +`imported_macro_render_does_not_emit_invocation_metrics` in +`src/manifest/tests/macro_invocation_telemetry.rs` pins this split. + +The label and field vocabulary is bounded by construction, never echoing +manifest content into a span or a metric dimension: + +- `outcome` is always `"success"` or `"error"`. +- The render boundary adds `has_macro_imports`, `"true"` or `"false"`, + distinguishing the import-prefixed render path from the plain one without + revealing which macros a manifest defines. +- On failure, both boundaries add `error_category`, the `Debug` form of + `minijinja::ErrorKind` — never the error's `Display` text, which can embed + manifest content. + +**Redaction rule.** Template text, macro names, macro arguments, context +values, and environment variable names must never reach telemetry. Manifest +content is caller-controlled and unbounded, so recording it in a metric label +would make the metric series unbounded, and recording it in a span or event +risks leaking secrets — environment variable names routinely identify +credentials. This mirrors the redaction rule `env_var_with` already applies to +`env()` lookup failures; see [Manifest `env()` reader](#manifest-env-reader). + +`describe_macro_metrics` and `describe_render_metrics` register each metric's +description exactly once, guarded by `std::sync::Once`. +`describe_macro_metrics` runs when `make_macro_fn` builds a macro's +registration, not on each invocation, so the guard never sits on the invocation +hot path. `describe_render_metrics` runs at the top of every `render_template` +call; the `Once` guard still limits the actual `describe_counter!`/ +`describe_histogram!` calls to the first render. + +Per `AGENTS.md`, this module emits through `metrics` and `tracing` but must not +install a global recorder or subscriber; only the application does that, at +startup. Tests follow the same rule: `src/manifest/tests/macros_telemetry.rs` +(the render boundary) and `src/manifest/tests/macro_invocation_telemetry.rs` +(the macro-invocation boundary) each drive a local +`metrics_util::debugging::DebuggingRecorder` through +`metrics::with_local_recorder`, and capture tracing events with the workspace's +`with_test_subscriber` helper (see [`tracing_capture`](#tracing_capture)), so +neither test touches process-wide state. Extend `macros_telemetry.rs` for +render-boundary coverage and `macro_invocation_telemetry.rs` for +invocation-boundary coverage. The latter also runs a proptest, +`macro_telemetry_stays_bounded_for_arbitrary_macros`, which asserts the +redaction contract holds for arbitrary generated macro names, arguments, and +undefined-variable names, not just the fixed sentinel cases used by the other +tests. + ### Expansion helpers #### expand_foreach diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 6cef64f6a..b6c389ca2 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1020,6 +1020,14 @@ do not depend on the lifetime of the manifest parsing state. This preserves MiniJinja's argument handling, including keyword parameters and `caller` support, while allowing later macros to override earlier ones. +Template rendering and macro invocation are each wrapped in a bounded, redacted +telemetry boundary, kept separate from evaluation so manifest content never +reaches a span or a metric label. +[ADR-009](adr-009-bounded-redacted-manifest-telemetry.md) records this +decision; see the +[developer's guide](developers-guide.md#manifest-telemetry-template-render-and-macro-invocation) +for the metric names, bounded label vocabulary, and redaction rules. + ### 4.4 Essential Custom Functions To transform `minijinja` from a general-purpose templating engine into a diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index 2af6fddd0..4b3f50e66 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -98,10 +98,6 @@ mod tests { }) } - fn assert_current_thread_lock_is_held(message: &str) { - assert!(current_thread_lock_is_held(), "{message}"); - } - fn assert_current_thread_lock_is_released(message: &str) { ENV_LOCK_STATE.with(|lock_state| { let state = lock_state.borrow(); @@ -116,19 +112,25 @@ mod tests { let _inner = EnvLock::acquire(); } + // Observations are captured while the guards live and asserted only once + // they are released: a failing assertion under a live guard would drop it + // mid-unwind and poison `ENV_LOCK`, burying the real cause. let outer = EnvLock::acquire(); - { + let held_with_nested = { let _inner = EnvLock::acquire(); - assert_current_thread_lock_is_held( - "ENV_LOCK should remain locked while nested EnvLock guards are alive", - ); - } - - assert_current_thread_lock_is_held( - "ENV_LOCK should remain locked until the outer EnvLock guard is dropped", - ); + current_thread_lock_is_held() + }; + let held_after_nested_drop = current_thread_lock_is_held(); drop(outer); + assert!( + held_with_nested, + "ENV_LOCK should remain locked while nested EnvLock guards are alive" + ); + assert!( + held_after_nested_drop, + "ENV_LOCK should remain locked until the outer EnvLock guard is dropped" + ); assert_current_thread_lock_is_released( "ENV_LOCK should be unlocked after final EnvLock guard is dropped", ); @@ -140,11 +142,13 @@ mod tests { let inner = EnvLock::acquire(); drop(outer); - assert_current_thread_lock_is_held( - "ENV_LOCK should remain locked while an inner EnvLock guard is alive", - ); + let held_with_inner_alive = current_thread_lock_is_held(); drop(inner); + assert!( + held_with_inner_alive, + "ENV_LOCK should remain locked while an inner EnvLock guard is alive" + ); assert_current_thread_lock_is_released( "ENV_LOCK should be unlocked after the final out-of-order guard drops", ); diff --git a/tests/data/ir_env.yml b/tests/data/ir_env.yml new file mode 100644 index 000000000..8d7befe0f --- /dev/null +++ b/tests/data/ir_env.yml @@ -0,0 +1,9 @@ +netsuke_version: "1.0.0" +# The target name and one input are derived from an environment variable so IR +# compilation can only produce the asserted graph when the scenario environment +# reaches the manifest parser. A host-environment read would leave the variable +# undefined and fail generation instead. +targets: + - name: "out/{{ env('NETSUKE_TEST_IR_ENV') }}" + sources: "src/{{ env('NETSUKE_TEST_IR_ENV') }}.c" + command: "cc -o $out $in" diff --git a/tests/features/ir.feature b/tests/features/ir.feature index 335d7249a..1d6b65266 100644 --- a/tests/features/ir.feature +++ b/tests/features/ir.feature @@ -17,6 +17,16 @@ Feature: BuildGraph Then the graph has 2 actions And the graph has 2 targets + Scenario: IR compilation reads the scenario environment + Given the environment variable "NETSUKE_TEST_IR_ENV" is set to "from-env" + When the manifest file "tests/data/ir_env.yml" is compiled to IR + Then the graph target "out/from-env" has inputs "src/from-env.c" + + Scenario: IR compilation fails when a manifest variable is unset + Given the environment variable "NETSUKE_TEST_IR_ENV" is unset + When the manifest file "tests/data/ir_env.yml" is compiled to IR + Then IR generation fails + Scenario: Rule not found during IR generation When the manifest file "tests/data/missing_rule.yml" is compiled to IR Then IR generation fails diff --git a/tests/novice_flow_smoke_tests.rs b/tests/novice_flow_smoke_tests.rs index d278901bd..1f4c02c52 100644 --- a/tests/novice_flow_smoke_tests.rs +++ b/tests/novice_flow_smoke_tests.rs @@ -1,8 +1,10 @@ //! Smoke tests for newcomer-facing CLI flows. -use anyhow::{Context, Result, ensure}; +use anyhow::{Context, Result, bail, ensure}; use rstest::rstest; use std::path::Path; +#[cfg(unix)] +use std::path::PathBuf; use tempfile::{TempDir, tempdir}; use test_support::check_ninja; use test_support::fluent::normalize_fluent_isolates; @@ -61,6 +63,32 @@ fn assert_contains_all(haystack: &str, fragments: &[&str], label: &str) -> Resul Ok(()) } +/// A non-UTF-8 override must fail loudly rather than being mangled. +/// +/// `NETSUKE_NINJA` is forwarded to the child as a string, so a lossy conversion +/// would silently point it at a different executable. Unix-only: constructing a +/// non-UTF-8 path requires POSIX byte semantics. +#[cfg(unix)] +#[test] +fn non_utf8_ninja_override_is_rejected() -> Result<()> { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let workspace = setup_minimal_workspace("novice smoke non-UTF-8 ninja")?; + let non_utf8 = PathBuf::from(OsString::from_vec(b"ninja-\xff".to_vec())); + + let Err(error) = run_netsuke(workspace.path(), &[], Some(non_utf8.as_path())) else { + bail!("a non-UTF-8 ninja override should not be accepted") + }; + + let message = format!("{error:?}"); + ensure!( + message.contains("not valid UTF-8"), + "error should name the non-UTF-8 override, got {message}" + ); + Ok(()) +} + #[test] fn first_run_without_args_succeeds_in_minimal_workspace() -> Result<()> { let workspace = setup_minimal_workspace("novice smoke first run")?; From 7f35502b608bd747248f6e1dc90730b1fb2fff02 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 14:58:48 +0200 Subject: [PATCH 5/8] Correct the poisoning test's safety note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the fifth review round on #515. Both findings were prose. The comment above `env_lock_recovers_after_mutex_poisoning` claimed every assertion runs with the poison flag already cleared. That is wrong for the first one: the `join` assertion precedes `ENV_LOCK.clear_poison()`, and at that point the mutex is still poisoned — deliberately, since that is the state under test. It is nonetheless safe, because the poisoned guard belonged to the spawned thread and this thread holds none, so a failure has no live guard to drop. The note now separates the two cases and keeps the explanation of why the held state is captured before its guard is dropped. Drop the comma before "because" in the two ADR sentences named by the review. The second needed more than deleting the comma: "covers the compiled-expression fallback only because imports evaluate" reads as "merely because", so "only" moves ahead of the noun phrase to keep the meaning that the counter covers that path alone. Co-Authored-By: Claude Opus 5 (1M context) --- ...dr-009-bounded-redacted-manifest-telemetry.md | 6 +++--- test_support/src/env_lock.rs | 16 +++++++++++----- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/adr-009-bounded-redacted-manifest-telemetry.md b/docs/adr-009-bounded-redacted-manifest-telemetry.md index 274dc7bb0..2d6c5d009 100644 --- a/docs/adr-009-bounded-redacted-manifest-telemetry.md +++ b/docs/adr-009-bounded-redacted-manifest-telemetry.md @@ -18,7 +18,7 @@ needed for these paths, two risks had to be weighed against each other: - Manifest content — template text, macro names, macro arguments, and context values — is caller-controlled and unbounded. Recording it directly in a metric label produces unbounded cardinality in the metric series, and - recording it in a trace risks leaking secrets, because environment variable + recording it in a trace risks leaking secrets because environment variable names routinely identify credentials (`src/manifest/env_reader.rs` already applies this rule to `env()` lookup failures). - Interleaving spans and metric emission with the evaluation logic in @@ -49,8 +49,8 @@ separate from evaluation, with two distinct instrumentation boundaries: `netsuke_manifest_macro_invocation_duration_seconds` histogram. Macros reached through a template import are metered only at the render -boundary; the macro-invocation counter covers the compiled-expression fallback -only, because imports evaluate inside the render call and never reach +boundary; the macro-invocation counter covers only the compiled-expression +fallback because imports evaluate inside the render call and never reach `make_macro_fn`. `macro_invocation_telemetry.rs`'s `imported_macro_render_does_not_emit_invocation_metrics` test pins this split. diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index 4b3f50e66..149add8f7 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -256,11 +256,17 @@ mod tests { /// thread-parallel runner it would race any other test touching the global /// mutex. /// - /// Every assertion here is made with no guard live and the poison flag - /// already cleared, so a failure cannot leave process-global poisoning - /// behind: clearing the flag covers the deliberate poisoning, and capturing - /// the held state before dropping the guard covers the unwind path, where a - /// panic would otherwise drop the `MutexGuard` and poison the mutex afresh. + /// No assertion here can leave process-global poisoning behind, though the + /// reason differs either side of `ENV_LOCK.clear_poison()`: + /// + /// - The `join` assertion runs before the flag is cleared, so `ENV_LOCK` is + /// still poisoned at that point — deliberately, since that is the state + /// under test. It is safe regardless: the poisoned guard belonged to the + /// spawned thread, and this thread holds no `EnvLock`, so a failure has no + /// live guard to drop. + /// - The later assertions run once the flag is cleared, and the held state is + /// captured before its guard is dropped, so an unwinding assertion cannot + /// drop a live `MutexGuard` and poison the mutex afresh. #[test] fn env_lock_recovers_after_mutex_poisoning() { let poisoner = std::thread::spawn(|| { From a41ebed64dc10a32c655fef6c45320a6d549ebb2 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 15:06:18 +0200 Subject: [PATCH 6/8] Keep the metric registry out of the render path `render_template` called `describe_render_metrics()` on every render, so the query function reached for the global metric registry itself. The call moves into `instrument_template_render`, behind the same one-time guard, leaving rendering to name only the instrumentation boundary it composes with. The macro-side registration stays where it is: it runs when a macro is registered, which is setup rather than evaluation. This is a partial response to a review finding asking for a clock and telemetry sink to be injected instead. That part is not actioned; see ADR-008, which records the decision and the reasoning, and the pull request discussion. Co-Authored-By: Claude Opus 5 (1M context) --- src/manifest/jinja_macros/mod.rs | 1 - src/manifest/jinja_macros/telemetry.rs | 8 +++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/manifest/jinja_macros/mod.rs b/src/manifest/jinja_macros/mod.rs index 416b9ca05..ebf155b4a 100644 --- a/src/manifest/jinja_macros/mod.rs +++ b/src/manifest/jinja_macros/mod.rs @@ -143,7 +143,6 @@ pub(crate) fn render_template( template: &str, context: &impl Serialize, ) -> Result { - telemetry::describe_render_metrics(); let imports = macro_imports(env); let has_macro_imports = imports.is_some(); telemetry::instrument_template_render(has_macro_imports, || { diff --git a/src/manifest/jinja_macros/telemetry.rs b/src/manifest/jinja_macros/telemetry.rs index a0422f17e..a72831246 100644 --- a/src/manifest/jinja_macros/telemetry.rs +++ b/src/manifest/jinja_macros/telemetry.rs @@ -40,7 +40,12 @@ pub(super) fn describe_macro_metrics() { } /// Register the template-render metric descriptions exactly once. -pub(super) fn describe_render_metrics() { +/// +/// Called from the instrumentation wrapper rather than from `render_template`, +/// so rendering never reaches for the metric registry itself. The macro +/// counterpart is registered when a macro is registered, which is setup rather +/// than evaluation, so it has a natural home outside the query. +fn describe_render_metrics() { static DESCRIBE: Once = Once::new(); DESCRIBE.call_once(|| { describe_counter!( @@ -89,6 +94,7 @@ pub(super) fn instrument_template_render( has_macro_imports: bool, render: impl FnOnce() -> Result, ) -> Result { + describe_render_metrics(); let span = tracing::trace_span!( "manifest.template.render", outcome = field::Empty, From fa2a18532c33466dcc39aaa6885d5e6a637283b2 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 6 Aug 2026 16:04:57 +0200 Subject: [PATCH 7/8] Realign the telemetry guide with the render path Moving `describe_render_metrics` into `instrument_template_render` left the developers' guide asserting it "runs at the top of every `render_template` call". Describe where the registration actually happens, and why neither description call sits in a query function. Co-Authored-By: Claude Opus 5 (1M context) --- docs/developers-guide.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index ad60c8c27..39f8f9911 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -2672,12 +2672,13 @@ credentials. This mirrors the redaction rule `env_var_with` already applies to `env()` lookup failures; see [Manifest `env()` reader](#manifest-env-reader). `describe_macro_metrics` and `describe_render_metrics` register each metric's -description exactly once, guarded by `std::sync::Once`. -`describe_macro_metrics` runs when `make_macro_fn` builds a macro's -registration, not on each invocation, so the guard never sits on the invocation -hot path. `describe_render_metrics` runs at the top of every `render_template` -call; the `Once` guard still limits the actual `describe_counter!`/ -`describe_histogram!` calls to the first render. +description exactly once, guarded by `std::sync::Once`. Neither is called from a +query function: `describe_macro_metrics` runs when `make_macro_fn` builds a +macro's registration, which is setup rather than evaluation, so the guard never +sits on the invocation hot path; `describe_render_metrics` runs inside +`instrument_template_render`, so `render_template` names only the +instrumentation boundary it composes with and never reaches for the metric +registry itself. Per `AGENTS.md`, this module emits through `metrics` and `tracing` but must not install a global recorder or subscriber; only the application does that, at From df0b242a5d23d6b3f4d58ad9abe9f2b1e1a3704f Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 7 Aug 2026 18:54:57 +0200 Subject: [PATCH 8/8] Gate the Unix-only bail import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bail!` is reached only from `non_utf8_ninja_override_is_rejected`, which is `#[cfg(unix)]` because building a non-UTF-8 path needs POSIX byte semantics. The import was unconditional, so on any other target it became an unused import — and the workspace builds with `-D warnings`, making that a hard error rather than a warning. Gate it alongside the `PathBuf` import that the same test needs. Verified rather than assumed: the pre-fix shape (unconditional import whose sole user is gated out) fails `rustc -D warnings` with `error: unused import`, and the gated shape compiles clean. A direct Windows cross-check was not possible here because a build dependency needs MSVC's `lib.exe`. Co-Authored-By: Claude Opus 5 (1M context) --- tests/novice_flow_smoke_tests.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/novice_flow_smoke_tests.rs b/tests/novice_flow_smoke_tests.rs index 1f4c02c52..d2ddab565 100644 --- a/tests/novice_flow_smoke_tests.rs +++ b/tests/novice_flow_smoke_tests.rs @@ -1,6 +1,11 @@ //! Smoke tests for newcomer-facing CLI flows. -use anyhow::{Context, Result, bail, ensure}; +// `bail!` is reached only from the Unix-only non-UTF-8 test, so the import is +// gated alongside `PathBuf`; left unconditional it would be an unused import on +// other targets, which `-D warnings` turns into a build failure. +#[cfg(unix)] +use anyhow::bail; +use anyhow::{Context, Result, ensure}; use rstest::rstest; use std::path::Path; #[cfg(unix)]