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..2d6c5d009 --- /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 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. + +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 a646f5fe7..39f8f9911 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, @@ -2626,6 +2623,79 @@ 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`. 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 +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/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..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, @@ -172,7 +139,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/manifest/jinja_macros/mod.rs b/src/manifest/jinja_macros/mod.rs index 67f17f334..ebf155b4a 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,14 @@ pub(crate) fn render_template( template: &str, context: &impl Serialize, ) -> Result { - 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..a72831246 --- /dev/null +++ b/src/manifest/jinja_macros/telemetry.rs @@ -0,0 +1,126 @@ +//! 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. +/// +/// 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!( + 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 { + describe_render_metrics(); + 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..546fa3292 --- /dev/null +++ b/src/manifest/tests/macro_invocation_telemetry.rs @@ -0,0 +1,274 @@ +//! 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(()) +} + +/// 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. +#[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/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..149add8f7 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -86,11 +86,16 @@ 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_released(message: &str) { @@ -107,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", ); @@ -131,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", ); @@ -238,6 +251,22 @@ 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. + /// + /// 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(|| { @@ -246,15 +275,17 @@ 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"); + let held_while_live = current_thread_lock_is_held(); drop(recovered_guard); - ENV_LOCK.clear_poison(); + 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/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/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..bbf65201c 100644 --- a/test_support/src/http/tests.rs +++ b/test_support/src/http/tests.rs @@ -56,7 +56,17 @@ 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>, + /// 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] @@ -102,19 +112,29 @@ 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, + 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_value: Some("not-a-number"), + 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_value: None, + expected_warning_error: None, + expected_warning_len: None, })] fn duration_from_env_handles_input( empty_duration_warnings: EmptyDurationWarnings, @@ -129,7 +149,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 +157,22 @@ 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}" ); + 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!( + !warning.contains(configured_value), + "warning must redact the offending value, got {warning}" + ); + } } else { assert!( warnings.is_empty(), @@ -148,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/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/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/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..d2ddab565 100644 --- a/tests/novice_flow_smoke_tests.rs +++ b/tests/novice_flow_smoke_tests.rs @@ -1,8 +1,15 @@ //! Smoke tests for newcomer-facing CLI flows. +// `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)] +use std::path::PathBuf; use tempfile::{TempDir, tempdir}; use test_support::check_ninja; use test_support::fluent::normalize_fluent_isolates; @@ -23,8 +30,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)?, }; @@ -53,6 +68,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")?; diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index 02e3e477c..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] = [ @@ -95,21 +95,28 @@ 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. + // `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| { + Utf8Path::new(path) + .components() + .next() + .is_some_and(|component| component.as_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() ); } 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/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..3bb07a873 100644 --- a/tests/workflow_contracts/ci_lint_test.py +++ b/tests/workflow_contracts/ci_lint_test.py @@ -38,9 +38,59 @@ 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.""" - 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. + """ + # `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, " + 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]]: