From 9163f45ec4f69392633f8ea5c45d276f92b500ec Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 12 Jun 2026 20:53:06 +0200 Subject: [PATCH 1/7] Separate Ninja generation steps from runner reporting (#343) Keep generation reusable without a status reporter while retaining runner orchestration for progress updates and bundle telemetry. --- src/runner/generation.rs | 61 +++++++++++++++++++++++++++++++++++++++ src/runner/graph.rs | 6 ++-- src/runner/mod.rs | 62 ++++++++++++++++++++++++---------------- src/runner/tests.rs | 24 ++++++++++++++++ 4 files changed, 125 insertions(+), 28 deletions(-) create mode 100644 src/runner/generation.rs diff --git a/src/runner/generation.rs b/src/runner/generation.rs new file mode 100644 index 000000000..7b1d160fc --- /dev/null +++ b/src/runner/generation.rs @@ -0,0 +1,61 @@ +//! Pure Ninja-generation steps for the runner. +//! +//! Generation decomposes into three query-style steps — load the manifest, +//! build the graph, generate the Ninja text — none of which require a status +//! reporter. Progress reporting stays in the thin orchestration wrappers in +//! [`super`] (`generate_ninja`, +//! `load_manifest_with_stage_reporting`), so generation can be reused as a +//! pure operation (for example for dry runs or background generation). + +use anyhow::{Context, Result}; +use camino::Utf8Path; + +use crate::ast::NetsukeManifest; +use crate::ir::BuildGraph; +use crate::localization::{self, keys}; +use crate::stdlib::NetworkPolicy; +use crate::{manifest, ninja_gen}; + +/// Optional observer for manifest-loading stages. +/// +/// Callers that want progress reporting pass a callback translating +/// [`manifest::ManifestLoadStage`] values into their own reporting; passing +/// `None` keeps the pipeline free of side effects. +pub(super) type StageObserver<'a> = Option<&'a mut dyn FnMut(manifest::ManifestLoadStage)>; + +/// Load and render the Netsuke manifest at `path`. +/// +/// # Errors +/// +/// Returns an error when the manifest cannot be read, parsed, or rendered. +pub(super) fn load_manifest( + path: &Utf8Path, + policy: NetworkPolicy, + on_stage: StageObserver<'_>, +) -> Result { + manifest::from_path_with_policy(path.as_std_path(), policy, on_stage).with_context(|| { + localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST).with_arg("path", path.as_str()) + }) +} + +/// Translate a manifest into the build graph intermediate representation. +/// +/// # Errors +/// +/// Returns an error when graph construction or validation fails (for example +/// on circular dependencies or duplicate outputs). +pub(super) fn build_graph(manifest: &NetsukeManifest) -> Result { + BuildGraph::from_manifest(manifest) + .context(localization::message(keys::RUNNER_CONTEXT_BUILD_GRAPH)) +} + +/// Generate the Ninja bundle for a build graph. +/// +/// # Errors +/// +/// Returns an error when Ninja synthesis fails. +pub(super) fn ninja_text( + graph: &BuildGraph, +) -> Result { + ninja_gen::generate_bundle(graph) +} diff --git a/src/runner/graph.rs b/src/runner/graph.rs index 0f1f7ad8d..1f4513d9f 100644 --- a/src/runner/graph.rs +++ b/src/runner/graph.rs @@ -16,7 +16,6 @@ use crate::graph_view::GraphView; use crate::graph_view::render::GraphRenderer; use crate::graph_view::render_dot::DotRenderer; use crate::graph_view::render_html::HtmlRenderer; -use crate::ir::BuildGraph; use crate::localization::{self, keys}; use crate::result_json; use crate::status::{LocalizationKey, PipelineStage, StatusReporter, report_pipeline_stage}; @@ -24,7 +23,7 @@ use crate::status::{LocalizationKey, PipelineStage, StatusReporter, report_pipel use super::path_helpers::{ ensure_manifest_exists_or_error, resolve_manifest_path, resolve_output_path, }; -use super::{load_manifest_with_stage_reporting, process}; +use super::{generation, load_manifest_with_stage_reporting, process}; /// Render the build graph in-process and write the selected artefact. /// @@ -53,8 +52,7 @@ pub(super) fn handle_graph( .context(localization::message(keys::RUNNER_CONTEXT_NETWORK_POLICY))?; let manifest = load_manifest_with_stage_reporting(&manifest_path, policy, reporter)?; report_pipeline_stage(reporter, PipelineStage::IrGenerationValidation, None); - let graph = BuildGraph::from_manifest(&manifest) - .context(localization::message(keys::RUNNER_CONTEXT_BUILD_GRAPH))?; + let graph = generation::build_graph(&manifest)?; let view = GraphView::from_build_graph(&graph); let status_key: LocalizationKey = if args.html { diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 886416eb6..9db76b665 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -13,7 +13,7 @@ use crate::localization::{self, keys}; use crate::output_mode; use crate::output_prefs::OutputPrefs; use crate::status::{LocalizationKey, PipelineStage, StatusReporter, report_pipeline_stage}; -use crate::{ir::BuildGraph, manifest, ninja_gen}; +use crate::{manifest, ninja_gen}; use anyhow::{Context, Result}; use camino::Utf8PathBuf; pub use error::RunnerError; @@ -24,6 +24,7 @@ use tracing::{debug, info}; /// Default Ninja executable to invoke. pub const NINJA_PROGRAM: &str = "ninja"; +mod generation; /// Environment variable override for the Ninja executable. /// /// # Examples @@ -304,8 +305,7 @@ fn generate_ninja( } report_pipeline_stage(reporter, PipelineStage::IrGenerationValidation, None); - let graph = BuildGraph::from_manifest(&manifest) - .context(localization::message(keys::RUNNER_CONTEXT_BUILD_GRAPH))?; + let graph = generation::build_graph(&manifest)?; report_pipeline_stage( reporter, @@ -313,36 +313,50 @@ fn generate_ninja( tool_key, ); dyndep_generation_telemetry::instrument_bundle_generation(&graph, || { - ninja_gen::generate_bundle(&graph) + generation::ninja_text(&graph) }) .context(localization::message(keys::RUNNER_CONTEXT_GENERATE_NINJA)) } -/// Load the manifest, reporting each loading stage through `reporter`. +/// Map manifest-loading stages onto the status reporter's pipeline stages. +fn stage_reporting_callback( + reporter: &dyn StatusReporter, +) -> impl FnMut(manifest::ManifestLoadStage) + '_ { + move |stage: manifest::ManifestLoadStage| { + let pipeline_stage = match stage { + manifest::ManifestLoadStage::ManifestIngestion => PipelineStage::ManifestIngestion, + manifest::ManifestLoadStage::InitialYamlParsing => PipelineStage::InitialYamlParsing, + manifest::ManifestLoadStage::TemplateExpansion => PipelineStage::TemplateExpansion, + manifest::ManifestLoadStage::FinalRendering => PipelineStage::FinalRendering, + }; + report_pipeline_stage(reporter, pipeline_stage, None); + } +} +/// Map manifest-loading stages onto the status reporter's pipeline stages. +fn stage_reporting_callback( + reporter: &dyn StatusReporter, +) -> impl FnMut(manifest::ManifestLoadStage) + '_ { + move |stage: manifest::ManifestLoadStage| { + let pipeline_stage = match stage { + manifest::ManifestLoadStage::ManifestIngestion => PipelineStage::ManifestIngestion, + manifest::ManifestLoadStage::InitialYamlParsing => PipelineStage::InitialYamlParsing, + manifest::ManifestLoadStage::TemplateExpansion => PipelineStage::TemplateExpansion, + manifest::ManifestLoadStage::FinalRendering => PipelineStage::FinalRendering, + }; + report_pipeline_stage(reporter, pipeline_stage, None); + } +} + +/// Load the manifest, translating loading stages into reporter updates. +/// +/// Thin reporting wrapper over [`generation::load_manifest`]. pub(super) fn load_manifest_with_stage_reporting( manifest_path: &Utf8PathBuf, policy: crate::stdlib::NetworkPolicy, reporter: &dyn StatusReporter, ) -> Result { - let mut on_stage = |stage: manifest::ManifestLoadStage| match stage { - manifest::ManifestLoadStage::ManifestIngestion => { - report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); - } - manifest::ManifestLoadStage::InitialYamlParsing => { - report_pipeline_stage(reporter, PipelineStage::InitialYamlParsing, None); - } - manifest::ManifestLoadStage::TemplateExpansion => { - report_pipeline_stage(reporter, PipelineStage::TemplateExpansion, None); - } - manifest::ManifestLoadStage::FinalRendering => { - report_pipeline_stage(reporter, PipelineStage::FinalRendering, None); - } - }; - manifest::from_path_with_policy(manifest_path.as_std_path(), policy, Some(&mut on_stage)) - .with_context(|| { - localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) - .with_arg("path", manifest_path.as_str()) - }) + let mut on_stage = stage_reporting_callback(reporter); + generation::load_manifest(manifest_path, policy, Some(&mut on_stage)) } #[cfg(test)] diff --git a/src/runner/tests.rs b/src/runner/tests.rs index bb68dad16..b22fb4f2d 100644 --- a/src/runner/tests.rs +++ b/src/runner/tests.rs @@ -26,6 +26,30 @@ fn resolve_output_path_respects_directory( assert_eq!(resolved.as_ref(), Path::new(expected)); } +#[rstest] +fn generation_steps_run_without_reporter() -> anyhow::Result<()> { + let temp = tempfile::tempdir()?; + let manifest_path = temp.path().join("Netsukefile"); + test_support::fs::write( + &manifest_path, + "netsuke_version: \"1.0.0\"\ntargets:\n - name: hello\n command: echo hi\n", + )?; + let utf8_path = camino::Utf8PathBuf::from_path_buf(manifest_path) + .map_err(|path| anyhow::anyhow!("non-UTF-8 temp path: {}", path.display()))?; + + // The pure pipeline composes without a runner status reporter. + let manifest = + generation::load_manifest(&utf8_path, crate::stdlib::NetworkPolicy::default(), None)?; + let graph = generation::build_graph(&manifest)?; + let (ninja_text, _) = generation::ninja_text(&graph)?.into_parts(); + anyhow::ensure!( + ninja_text.contains("build hello:"), + "expected generated Ninja to contain the hello build edge:\n{}", + ninja_text + ); + Ok(()) +} + #[test] fn help_targets_bypasses_ninja_program_resolution() -> Result<()> { let _lock = localizer_test_lock().map_err(|error| anyhow::anyhow!("{error}"))?; From cc6c21ea1507dccae11b82022024420f8a0131d1 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 22 Aug 2026 05:13:08 +0200 Subject: [PATCH 2/7] Document runner generation reuse boundary (#343) Define ownership, permitted callers, and composition rules so dry-run and background work reuse the pure generation pipeline safely. --- docs/developers-guide.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index e83a8f884..0aadd9b5f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -3672,6 +3672,37 @@ context rather than resolving output or process configuration again; tests should inject the program through `run_with_ninja_program` when they need a deterministic child executable. +### Module: `runner::generation` + +`src/runner/generation.rs` owns the runner's reusable, in-memory generation +pipeline. It separates manifest loading, IR construction, and Ninja bundle +synthesis from command reporting and process execution. The pipeline is +`load_manifest` (optionally observing manifest stages), then `build_graph`, +then `ninja_text`. Its final value is `GeneratedNinja`, including any dyndep +sidecars, rather than a materialized file or a running Ninja process. + +#### Generation reuse boundary + +- **Ownership:** `runner::generation` is a private runner submodule. It owns + the three generation steps, their input/output hand-offs, and the manifest + and IR error contexts. It does not own `StatusReporter` updates, command + dispatch, dyndep publication, or Ninja execution. +- **Permitted call-sites:** `runner::generate_ninja` composes the complete + pipeline for build, clean, and generate commands. `runner::graph::handle_graph` + may stop after `build_graph` to render the graph. Runner unit tests may + compose the steps directly. New dry-run or background-generation work may + reuse the steps only within the runner boundary; a public or cross-subsystem + consumer requires an explicit application boundary rather than widening + these internal helpers. +- **Composition rules:** command adapters report stages before or after the + relevant step and wrap `ninja_text` with runner-owned generation telemetry. + Only `load_manifest_with_stage_reporting` translates `StageObserver` events + into status updates. Consumers must not call manifest parsing, IR generation, + or `ninja_gen::generate_bundle` directly in parallel with this pipeline. + Before an adapter writes or executes a returned bundle, it must use the + existing capability-injected dyndep-publication path to materialize its + sidecars; the pure steps never write files or start processes. + ### Module: `runner::reporter` `src/runner/reporter.rs` owns construction of the run's `StatusReporter` from From 736756878e9c07a08487c69a6c88714fe6f05553 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 22 Aug 2026 05:18:17 +0200 Subject: [PATCH 3/7] Document generation-step composition (#343) Show how internal callers compose the pure pipeline without reporter dependencies, including the generated Ninja bundle outcome. --- src/runner/generation.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/runner/generation.rs b/src/runner/generation.rs index 7b1d160fc..b2f909992 100644 --- a/src/runner/generation.rs +++ b/src/runner/generation.rs @@ -25,6 +25,17 @@ pub(super) type StageObserver<'a> = Option<&'a mut dyn FnMut(manifest::ManifestL /// Load and render the Netsuke manifest at `path`. /// +/// # Examples +/// +/// ```rust,ignore +/// let manifest = load_manifest( +/// Utf8Path::new("Netsukefile"), +/// NetworkPolicy::default(), +/// None, +/// )?; +/// // `manifest` is rendered and ready for `build_graph`. +/// ``` +/// /// # Errors /// /// Returns an error when the manifest cannot be read, parsed, or rendered. @@ -40,6 +51,13 @@ pub(super) fn load_manifest( /// Translate a manifest into the build graph intermediate representation. /// +/// # Examples +/// +/// ```rust,ignore +/// let graph = build_graph(&manifest)?; +/// // `graph` contains the validated targets and actions for `ninja_text`. +/// ``` +/// /// # Errors /// /// Returns an error when graph construction or validation fails (for example @@ -51,6 +69,15 @@ pub(super) fn build_graph(manifest: &NetsukeManifest) -> Result { /// Generate the Ninja bundle for a build graph. /// +/// # Examples +/// +/// ```rust,ignore +/// let generated = ninja_text(&graph)?; +/// let (text, sidecars) = generated.into_parts(); +/// assert!(text.contains("build hello:")); +/// assert!(sidecars.is_empty()); +/// ``` +/// /// # Errors /// /// Returns an error when Ninja synthesis fails. From 9c5f93e10b29599396287f6c5f90f6f68a8f880e Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 22 Aug 2026 05:41:17 +0200 Subject: [PATCH 4/7] Enforce pure runner generation boundaries (#343) Separate read-only manifest queries from effectful build evaluation and protect the stage, error, and public progress-output contracts. --- docs/developers-guide.md | 43 +++--- src/runner/generation.rs | 39 ++++- src/runner/help_query.rs | 7 +- src/runner/mod.rs | 2 +- src/runner/tests.rs | 188 +++++++++++++++++++++++-- tests/bdd/steps/manifest_command.rs | 9 ++ tests/features/progress_output.feature | 5 + 7 files changed, 257 insertions(+), 36 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 0aadd9b5f..478bbaade 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -3676,32 +3676,43 @@ deterministic child executable. `src/runner/generation.rs` owns the runner's reusable, in-memory generation pipeline. It separates manifest loading, IR construction, and Ninja bundle -synthesis from command reporting and process execution. The pipeline is -`load_manifest` (optionally observing manifest stages), then `build_graph`, +synthesis from command reporting and process execution. The read-only pipeline +is `load_manifest` (optionally observing manifest stages), then `build_graph`, then `ninja_text`. Its final value is `GeneratedNinja`, including any dyndep sidecars, rather than a materialized file or a running Ninja process. +`load_manifest` uses the manifest-query registration: it permits only its +read-only helpers and rejects template access to the environment, filesystem, +network, clock, and shell. `load_manifest_for_build` is a separate, explicitly +effectful loader for build, clean, generate, and graph commands. It receives a +network policy and enables the full build stdlib; it is not a dry-run or +background-query primitive. + #### Generation reuse boundary - **Ownership:** `runner::generation` is a private runner submodule. It owns - the three generation steps, their input/output hand-offs, and the manifest - and IR error contexts. It does not own `StatusReporter` updates, command - dispatch, dyndep publication, or Ninja execution. + the three read-only generation steps, the explicitly effectful build loader, + their input/output hand-offs, and the manifest and IR error contexts. It + does not own `StatusReporter` updates, command dispatch, dyndep publication, + or Ninja execution. - **Permitted call-sites:** `runner::generate_ninja` composes the complete - pipeline for build, clean, and generate commands. `runner::graph::handle_graph` - may stop after `build_graph` to render the graph. Runner unit tests may - compose the steps directly. New dry-run or background-generation work may - reuse the steps only within the runner boundary; a public or cross-subsystem - consumer requires an explicit application boundary rather than widening - these internal helpers. + build pipeline through `load_manifest_for_build` for build, clean, and + generate commands. `runner::graph::handle_graph` may stop after `build_graph` + to render the graph, and `runner::help_query` uses `load_manifest` for its + read-only target catalogue. Runner unit tests may compose the read-only + steps directly. New dry-run or background-generation work may use + `load_manifest`, `build_graph`, and `ninja_text` only within the runner + boundary; a public or cross-subsystem consumer requires an explicit + application boundary rather than widening these internal helpers. - **Composition rules:** command adapters report stages before or after the relevant step and wrap `ninja_text` with runner-owned generation telemetry. Only `load_manifest_with_stage_reporting` translates `StageObserver` events - into status updates. Consumers must not call manifest parsing, IR generation, - or `ninja_gen::generate_bundle` directly in parallel with this pipeline. - Before an adapter writes or executes a returned bundle, it must use the - existing capability-injected dyndep-publication path to materialize its - sidecars; the pure steps never write files or start processes. + into status updates and selects the effectful build loader. Consumers must + not call manifest parsing, IR generation, or `ninja_gen::generate_bundle` + directly in parallel with this pipeline. Before an adapter writes or + executes a returned bundle, it must use the existing capability-injected + dyndep-publication path to materialize its sidecars; the read-only steps + never write files, start processes, or invoke effectful template helpers. ### Module: `runner::reporter` diff --git a/src/runner/generation.rs b/src/runner/generation.rs index b2f909992..5623e57e0 100644 --- a/src/runner/generation.rs +++ b/src/runner/generation.rs @@ -1,4 +1,4 @@ -//! Pure Ninja-generation steps for the runner. +//! Query-style Ninja-generation steps and the build manifest loader. //! //! Generation decomposes into three query-style steps — load the manifest, //! build the graph, generate the Ninja text — none of which require a status @@ -6,6 +6,11 @@ //! [`super`] (`generate_ninja`, //! `load_manifest_with_stage_reporting`), so generation can be reused as a //! pure operation (for example for dry runs or background generation). +//! +//! [`load_manifest`] is the read-only query step: it rejects template helpers +//! that can access the environment, filesystem, network, clock, or shell. +//! [`load_manifest_for_build`] is deliberately separate because command +//! execution needs the full, effectful manifest stdlib. use anyhow::{Context, Result}; use camino::Utf8Path; @@ -23,23 +28,47 @@ use crate::{manifest, ninja_gen}; /// `None` keeps the pipeline free of side effects. pub(super) type StageObserver<'a> = Option<&'a mut dyn FnMut(manifest::ManifestLoadStage)>; -/// Load and render the Netsuke manifest at `path`. +/// Load and render the Netsuke manifest at `path` without effectful helpers. +/// +/// # Examples +/// +/// ```rust,ignore +/// let manifest = load_manifest(Utf8Path::new("Netsukefile"), None)?; +/// // `manifest` is rendered and ready for `build_graph`. +/// ``` +/// +/// # Errors +/// +/// Returns an error when the manifest cannot be read, parsed, or rendered. +pub(super) fn load_manifest( + path: &Utf8Path, + on_stage: StageObserver<'_>, +) -> Result { + manifest::from_path_for_manifest_query(path.as_std_path(), on_stage).with_context(|| { + localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST).with_arg("path", path.as_str()) + }) +} + +/// Load and render a manifest with the full, effectful build stdlib. +/// +/// This loader is only for command execution. Templates may use configured +/// network, cache, environment, filesystem, clock, and shell helpers. /// /// # Examples /// /// ```rust,ignore -/// let manifest = load_manifest( +/// let manifest = load_manifest_for_build( /// Utf8Path::new("Netsukefile"), /// NetworkPolicy::default(), /// None, /// )?; -/// // `manifest` is rendered and ready for `build_graph`. +/// // `manifest` may use build-time template helpers before `build_graph`. /// ``` /// /// # Errors /// /// Returns an error when the manifest cannot be read, parsed, or rendered. -pub(super) fn load_manifest( +pub(super) fn load_manifest_for_build( path: &Utf8Path, policy: NetworkPolicy, on_stage: StageObserver<'_>, diff --git a/src/runner/help_query.rs b/src/runner/help_query.rs index 797d1e2dc..3a395f95a 100644 --- a/src/runner/help_query.rs +++ b/src/runner/help_query.rs @@ -10,6 +10,7 @@ use crate::localization::{self, keys}; use crate::status::PipelineStage; use super::super::RunnerError; +use super::super::generation; use super::super::path_helpers::{ensure_manifest_exists, resolve_manifest_path}; use super::terminal_safe; @@ -179,11 +180,7 @@ fn load_manifest_for_query( stages: &mut Vec, ) -> Result { let mut on_stage = |stage| stages.push(pipeline_stage(stage)); - crate::manifest::from_path_for_manifest_query(manifest_path.as_std_path(), Some(&mut on_stage)) - .with_context(|| { - localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) - .with_arg("path", manifest_path.as_str()) - }) + generation::load_manifest(manifest_path, Some(&mut on_stage)) } /// Map manifest-loading events to data that the command boundary can report. diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 9db76b665..9a128f32f 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -356,7 +356,7 @@ pub(super) fn load_manifest_with_stage_reporting( reporter: &dyn StatusReporter, ) -> Result { let mut on_stage = stage_reporting_callback(reporter); - generation::load_manifest(manifest_path, policy, Some(&mut on_stage)) + generation::load_manifest_for_build(manifest_path, policy, Some(&mut on_stage)) } #[cfg(test)] diff --git a/src/runner/tests.rs b/src/runner/tests.rs index b22fb4f2d..4689bc607 100644 --- a/src/runner/tests.rs +++ b/src/runner/tests.rs @@ -2,13 +2,62 @@ use super::*; use crate::cli::{HelpArgs, HelpTopic}; +use crate::ir::{BuildEdge, BuildGraph, DependencyOrder}; +use crate::manifest::ManifestLoadStage; +use crate::ninja_gen::NinjaGenError; +use crate::status::{LocalizationKey, StageNumber, StatusReporter}; use anyhow::{Result, ensure}; +use camino::Utf8PathBuf; use rstest::rstest; use std::cell::Cell; use std::path::Path; use std::path::PathBuf; +use std::sync::{Mutex, PoisonError}; use test_support::{localizer_test_lock, set_en_localizer}; +const MINIMAL_MANIFEST: &str = concat!( + "netsuke_version: \"1.0.0\"\n", + "targets:\n", + " - name: hello\n", + " command: echo hi\n", +); + +/// Write a manifest and return a UTF-8 path suitable for runner generation. +fn write_manifest(manifest: &str) -> Result<(tempfile::TempDir, Utf8PathBuf)> { + let temp = tempfile::tempdir()?; + let manifest_path = temp.path().join("Netsukefile"); + test_support::fs::write(&manifest_path, manifest)?; + let utf8_path = Utf8PathBuf::from_path_buf(manifest_path) + .map_err(|path| anyhow::anyhow!("non-UTF-8 temp path: {}", path.display()))?; + Ok((temp, utf8_path)) +} + +/// Record runner pipeline stages without coupling the test to rendered text. +#[derive(Default)] +struct StageRecordingReporter { + stages: Mutex>, +} + +impl StageRecordingReporter { + fn stages(&self) -> Vec { + self.stages + .lock() + .unwrap_or_else(PoisonError::into_inner) + .clone() + } +} + +impl StatusReporter for StageRecordingReporter { + fn report_stage(&self, current: StageNumber, _total: StageNumber, _description: &str) { + self.stages + .lock() + .unwrap_or_else(PoisonError::into_inner) + .push(current.get()); + } + + fn report_complete(&self, _tool_key: LocalizationKey) {} +} + #[rstest] #[case(None, "out.ninja", "out.ninja")] #[case(Some("work"), "out.ninja", "work/out.ninja")] @@ -28,20 +77,24 @@ fn resolve_output_path_respects_directory( #[rstest] fn generation_steps_run_without_reporter() -> anyhow::Result<()> { - let temp = tempfile::tempdir()?; - let manifest_path = temp.path().join("Netsukefile"); - test_support::fs::write( - &manifest_path, - "netsuke_version: \"1.0.0\"\ntargets:\n - name: hello\n command: echo hi\n", - )?; - let utf8_path = camino::Utf8PathBuf::from_path_buf(manifest_path) - .map_err(|path| anyhow::anyhow!("non-UTF-8 temp path: {}", path.display()))?; + let (_temp, manifest_path) = write_manifest(MINIMAL_MANIFEST)?; + let mut stages = Vec::new(); // The pure pipeline composes without a runner status reporter. let manifest = - generation::load_manifest(&utf8_path, crate::stdlib::NetworkPolicy::default(), None)?; + generation::load_manifest(&manifest_path, Some(&mut |stage| stages.push(stage)))?; let graph = generation::build_graph(&manifest)?; let (ninja_text, _) = generation::ninja_text(&graph)?.into_parts(); + ensure!( + stages + == vec![ + ManifestLoadStage::ManifestIngestion, + ManifestLoadStage::InitialYamlParsing, + ManifestLoadStage::TemplateExpansion, + ManifestLoadStage::FinalRendering, + ], + "unexpected query-loader stage sequence: {stages:?}" + ); anyhow::ensure!( ninja_text.contains("build hello:"), "expected generated Ninja to contain the hello build edge:\n{}", @@ -50,6 +103,123 @@ fn generation_steps_run_without_reporter() -> anyhow::Result<()> { Ok(()) } +#[rstest] +#[case::fetch("{{ fetch('https://example.invalid', cache=true) }}", "fetch")] +#[case::shell("{{ 'ignored' | shell('printf side-effect') }}", "shell")] +fn query_loader_rejects_effectful_template_helpers( + #[case] expression: &str, + #[case] helper: &str, +) -> Result<()> { + let manifest = format!( + concat!( + "netsuke_version: \"1.0.0\"\n", + "targets:\n", + " - name: hello\n", + " description: >-\n", + " {}\n", + " command: echo hi\n", + ), + expression + ); + let (temp, manifest_path) = write_manifest(&manifest)?; + + let error = generation::load_manifest(&manifest_path, None) + .expect_err("query loader should reject effectful template helpers"); + ensure!( + error + .chain() + .any(|cause| cause.to_string().contains(helper)), + "query loader should name the rejected helper: {error:?}" + ); + ensure!( + !temp.path().join(".netsuke").exists(), + "query loader must not create an effectful template cache" + ); + Ok(()) +} + +#[test] +fn query_loader_preserves_load_error_context() -> Result<()> { + let temp = tempfile::tempdir()?; + let manifest_path = Utf8PathBuf::from_path_buf(temp.path().join("Netsukefile")) + .map_err(|path| anyhow::anyhow!("non-UTF-8 temp path: {}", path.display()))?; + + let error = generation::load_manifest(&manifest_path, None) + .expect_err("missing manifest should fail to load"); + ensure!( + error.to_string().contains(manifest_path.as_str()), + "load context should name the manifest path: {error:?}" + ); + Ok(()) +} + +#[test] +fn build_graph_preserves_graph_error_context() -> Result<()> { + let _lock = localizer_test_lock().map_err(|error| anyhow::anyhow!("{error}"))?; + let _guard = set_en_localizer(); + let (_temp, manifest_path) = write_manifest(include_str!("../../tests/data/circular.yml"))?; + let manifest = generation::load_manifest(&manifest_path, None)?; + + let error = generation::build_graph(&manifest).expect_err("cycle should fail graph building"); + let expected = localization::message(keys::RUNNER_CONTEXT_BUILD_GRAPH).to_string(); + ensure!( + error.to_string().contains(&expected), + "graph context should be retained: {error:?}" + ); + Ok(()) +} + +#[test] +fn ninja_text_propagates_typed_generation_errors() { + let mut graph = BuildGraph::default(); + graph.targets.insert( + Utf8PathBuf::from("hello"), + BuildEdge { + action_id: "missing".into(), + inputs: Vec::new(), + implicit_deps: Vec::new(), + dependency_order: DependencyOrder::Parallel, + explicit_outputs: vec![Utf8PathBuf::from("hello")], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + }, + ); + + let error = generation::ninja_text(&graph).expect_err("missing action should fail generation"); + assert!(matches!( + error, + NinjaGenError::MissingAction { ref id, .. } if id == "missing" + )); +} + +#[test] +fn runner_reports_the_complete_generation_stage_sequence() -> Result<()> { + let (temp, manifest_path) = write_manifest(MINIMAL_MANIFEST)?; + let cli = Cli { + file: manifest_path.into_std_path_buf(), + directory: Some(temp.path().to_path_buf()), + command: Some(Commands::Generate { output: None }), + ..Cli::default() + }; + let reporter = StageRecordingReporter::default(); + + let generated = generate_ninja(&cli, &reporter, None)?; + let (ninja_text, _) = generated.into_parts(); + + let stages = reporter.stages(); + ensure!( + stages == (1..=6).collect::>(), + "unexpected runner stage sequence: {stages:?}" + ); + ensure!( + ninja_text.contains("build hello:"), + "runner generation should produce the hello build edge: {ninja_text}" + ); + Ok(()) +} + #[test] fn help_targets_bypasses_ninja_program_resolution() -> Result<()> { let _lock = localizer_test_lock().map_err(|error| anyhow::anyhow!("{error}"))?; diff --git a/tests/bdd/steps/manifest_command.rs b/tests/bdd/steps/manifest_command.rs index d490bdb1e..65df8a289 100644 --- a/tests/bdd/steps/manifest_command.rs +++ b/tests/bdd/steps/manifest_command.rs @@ -143,6 +143,15 @@ fn stdout_should_contain_in_order( assert_output_ordering(&world.command_stdout, OutputType::Stdout, &first, &second) } +#[then("stderr should contain {first:string} before {second:string}")] +fn stderr_should_contain_in_order( + world: &TestWorld, + first: OutputFragment, + second: OutputFragment, +) -> Result<()> { + assert_output_ordering(&world.command_stderr, OutputType::Stderr, &first, &second) +} + #[then("the file {name:string} should exist")] fn file_should_exist(world: &TestWorld, name: FileName) -> Result<()> { assert_file_existence(world, &name, true) diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature index 7b1755a60..dfc6ed1d6 100644 --- a/tests/features/progress_output.feature +++ b/tests/features/progress_output.feature @@ -28,6 +28,11 @@ Feature: Progress output When netsuke is run with arguments "--accessibility off --progress always generate" Then the command should succeed And stderr should contain "Stage 1/6" + And stderr should contain "Stage 1/6" before "Stage 2/6" + And stderr should contain "Stage 2/6" before "Stage 3/6" + And stderr should contain "Stage 3/6" before "Stage 4/6" + And stderr should contain "Stage 4/6" before "Stage 5/6" + And stderr should contain "Stage 5/6" before "Stage 6/6" And stderr should contain "Stage 6/6" And stderr should contain "Success:" And stderr should contain "Generate complete." From f6fd127a1253e23ccf588075f1e88fe481029a89 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 23 Aug 2026 20:11:15 +0200 Subject: [PATCH 5/7] Add public generation reporting coverage (#343) Exercise the public CLI with an isolated hello manifest so the generation/reporting boundary remains covered by observable output. --- tests/bdd/steps/manifest_command.rs | 43 +++++++++++++++++++++++--- tests/features/progress_output.feature | 13 ++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/tests/bdd/steps/manifest_command.rs b/tests/bdd/steps/manifest_command.rs index 65df8a289..9a35862e9 100644 --- a/tests/bdd/steps/manifest_command.rs +++ b/tests/bdd/steps/manifest_command.rs @@ -8,6 +8,7 @@ use crate::bdd::types::{ use anyhow::{Context, Result, ensure}; use rstest_bdd::Slot; use rstest_bdd_macros::{given, then, when}; +use std::ffi::OsString; use std::fmt; use std::fs; use std::path::{Path, PathBuf}; @@ -40,6 +41,14 @@ use manifest_command_helpers::{ // Given steps // --------------------------------------------------------------------------- +fn initialise_workspace(world: &TestWorld, temp: tempfile::TempDir) { + *world.temp_dir.borrow_mut() = Some(temp); + world.run_status.clear(); + world.run_error.clear(); + world.command_stdout.clear(); + world.command_stderr.clear(); +} + #[given("a minimal Netsuke workspace")] fn minimal_workspace(world: &TestWorld) -> Result<()> { let temp = tempfile::tempdir().context("create temp dir for manifest workspace")?; @@ -49,11 +58,35 @@ fn minimal_workspace(world: &TestWorld) -> Result<()> { let minimal_yml_path = std::path::Path::new(manifest_dir).join("tests/data/minimal.yml"); fs::copy(&minimal_yml_path, &netsukefile) .with_context(|| format!("copy manifest to {}", netsukefile.display()))?; - *world.temp_dir.borrow_mut() = Some(temp); - world.run_status.clear(); - world.run_error.clear(); - world.command_stdout.clear(); - world.command_stderr.clear(); + initialise_workspace(world, temp); + Ok(()) +} + +#[given("a Netsuke workspace with one hello target")] +fn hello_target_workspace(world: &TestWorld) -> Result<()> { + let temp = tempfile::tempdir().context("create temp dir for hello workspace")?; + let netsukefile = temp.path().join("Netsukefile"); + fs::write( + &netsukefile, + concat!( + "netsuke_version: \"1.0.0\"\n", + "targets:\n", + " - name: hello\n", + " command: \"echo hi\"\n", + ), + ) + .with_context(|| format!("write manifest to {}", netsukefile.display()))?; + initialise_workspace(world, temp); + Ok(()) +} + +#[given("the child PATH is the workspace directory")] +fn child_path_is_workspace_directory(world: &TestWorld) -> Result<()> { + let workspace_path = get_temp_path(world)?; + world.track_env_var( + "PATH".to_owned(), + Some(OsString::from(workspace_path.as_os_str())), + ); Ok(()) } diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature index dfc6ed1d6..19c73641d 100644 --- a/tests/features/progress_output.feature +++ b/tests/features/progress_output.feature @@ -37,6 +37,19 @@ Feature: Progress output And stderr should contain "Success:" And stderr should contain "Generate complete." + Scenario: Generate reports every stage for an isolated hello target + Given a Netsuke workspace with one hello target + And a fake ninja executable that succeeds without output + And the child PATH is the workspace directory + When netsuke is run with arguments "--accessibility off --progress always generate" + Then the command should succeed + And stderr should contain "Stage 1/6: Reading manifest file" before "Stage 2/6: Parsing YAML document" + And stderr should contain "Stage 2/6: Parsing YAML document" before "Stage 3/6: Expanding template directives" + And stderr should contain "Stage 3/6: Expanding template directives" before "Stage 4/6: Deserializing and rendering manifest values" + And stderr should contain "Stage 4/6: Deserializing and rendering manifest values" before "Stage 5/6: Building and validating dependency graph" + And stderr should contain "Stage 5/6: Building and validating dependency graph" before "Stage 6/6: Synthesizing Ninja build plan" + And stdout should contain "build hello:" + Scenario: Verbose mode includes a prefixed completion timing summary Given a minimal Netsuke workspace When netsuke is run with arguments "--accessibility off --progress always --verbose generate" From 0d5111a80a5d25e5a7df3afa57cd04a5b0b2d282 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 25 Aug 2026 20:47:42 +0200 Subject: [PATCH 6/7] Remove duplicate runner stage callback (#343) Keep the reporting adapter defined exactly once after the rebase. The duplicate was an automatic merge artefact and would otherwise prevent the runner from compiling. --- src/runner/mod.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 9a128f32f..4537491ef 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -332,21 +332,6 @@ fn stage_reporting_callback( report_pipeline_stage(reporter, pipeline_stage, None); } } -/// Map manifest-loading stages onto the status reporter's pipeline stages. -fn stage_reporting_callback( - reporter: &dyn StatusReporter, -) -> impl FnMut(manifest::ManifestLoadStage) + '_ { - move |stage: manifest::ManifestLoadStage| { - let pipeline_stage = match stage { - manifest::ManifestLoadStage::ManifestIngestion => PipelineStage::ManifestIngestion, - manifest::ManifestLoadStage::InitialYamlParsing => PipelineStage::InitialYamlParsing, - manifest::ManifestLoadStage::TemplateExpansion => PipelineStage::TemplateExpansion, - manifest::ManifestLoadStage::FinalRendering => PipelineStage::FinalRendering, - }; - report_pipeline_stage(reporter, pipeline_stage, None); - } -} - /// Load the manifest, translating loading stages into reporter updates. /// /// Thin reporting wrapper over [`generation::load_manifest`]. From 8206ea85d5d9564478e88867463397e11d497526 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 25 Aug 2026 20:47:50 +0200 Subject: [PATCH 7/7] Isolate packaging smoke Cargo artefacts (#343) Give every Cargo child in the packaging smoke test a per-test target directory. This prevents Cargo's package verification from racing a parallel Windows test process for the workspace `netsuke.exe` artefact. Retain the package manifest assertions and add a deterministic check of the child-process environment seam. --- tests/packaging_smoke_tests.rs | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/tests/packaging_smoke_tests.rs b/tests/packaging_smoke_tests.rs index 6e19f350f..d808a651a 100644 --- a/tests/packaging_smoke_tests.rs +++ b/tests/packaging_smoke_tests.rs @@ -8,7 +8,9 @@ use camino::Utf8Path; use netsuke::locale_catalogues::SUPPORTED_LOCALES; use std::collections::BTreeSet; use std::env; +use std::ffi::OsStr; use std::process::Command; +use tempfile::TempDir; const REQUIRED_PACKAGED_FILES: [&str; 9] = [ "build_l10n_audit/mod.rs", @@ -30,6 +32,14 @@ fn required_catalogue_paths() -> Vec { .map(|entry| format!("locales/{}/messages.ftl", entry.tag())) .collect() } + +/// Create a Cargo subprocess that writes build artefacts beneath `target_dir`. +fn cargo_subprocess(cargo_binary: &OsStr, target_dir: &TempDir) -> Command { + let mut command = Command::new(cargo_binary); + command.env("CARGO_TARGET_DIR", target_dir.path()); + command +} + #[test] #[expect( clippy::disallowed_methods, @@ -37,7 +47,9 @@ fn required_catalogue_paths() -> Vec { )] fn packaged_manifest_retains_build_script_sources() { let cargo_binary = env::var_os("CARGO").unwrap_or_else(|| "cargo".into()); - let publish_output = Command::new(&cargo_binary) + let cargo_target_dir = tempfile::tempdir() + .unwrap_or_else(|error| panic!("create isolated Cargo target directory: {error}")); + let publish_output = cargo_subprocess(&cargo_binary, &cargo_target_dir) .args([ "publish", "--dry-run", @@ -55,7 +67,7 @@ fn packaged_manifest_retains_build_script_sources() { String::from_utf8_lossy(&publish_output.stderr) ); - let list_output = Command::new(cargo_binary) + let list_output = cargo_subprocess(&cargo_binary, &cargo_target_dir) .args(["package", "--list", "--allow-dirty", "-p", "netsuke-build"]) .current_dir(env!("CARGO_MANIFEST_DIR")) .output() @@ -77,6 +89,18 @@ fn packaged_manifest_retains_build_script_sources() { assert_forbidden_roots_absent(&packaged_paths); } +#[test] +fn cargo_subprocess_uses_the_given_target_directory() { + let target_dir = tempfile::tempdir().expect("create isolated Cargo target directory"); + let command = cargo_subprocess(OsStr::new("cargo"), &target_dir); + let configured_target_dir = command + .get_envs() + .find_map(|(key, value)| (key == OsStr::new("CARGO_TARGET_DIR")).then_some(value)) + .flatten(); + + assert_eq!(configured_target_dir, Some(target_dir.path().as_os_str())); +} + /// Normalize Cargo's platform-native package-list separators for comparison. fn normalize_packaged_path(path: &str) -> String { path.replace('\\', "/")