Separate manifest expansion events from transformation (#344)

src/manifest/expand.rs

@@ -5,7 +5,6 @@ use anyhow::{Context, Result};
 use minijinja::{Environment, context, value::Value};
 use serde_json::{Number as JsonNumber, map::Entry};
 use sha2::{Digest, Sha256};
-use tracing::{debug, dispatcher, subscriber::NoSubscriber};
 
 /// Counts of manifest entries excluded during template expansion.
 ///
@@ -19,6 +18,36 @@ pub(crate) struct FilteringStats {
     pub filtered_actions: usize,
 }
 
+/// A manifest entry removed by a `when` expression during expansion.
+///
+/// Carries only bounded, non-sensitive correlation data: the raw entry name
+/// has unbounded cardinality and may carry personally identifiable
+/// information, so only a short stable hash is recorded, and the raw `when`
+/// expression may contain secret literals, so only its length is exposed.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) struct FilteredEntry {
+    /// Manifest section the entry belonged to (`targets` or `actions`).
+    pub section: String,
+    /// Short stable hash of the entry name for correlation.
+    pub entry_name_hash: String,
+    /// Iteration index when the entry came from a `foreach` expansion.
+    pub iteration_index: Option<usize>,
+    /// Length of the `when` expression that filtered the entry.
+    pub when_expression_len: usize,
+}
+
+/// Outcome of manifest expansion: counts plus per-entry filtering events.
+///
+/// Expansion reports what it filtered through this data structure rather
+/// than emitting telemetry itself; the caller owns the tracing policy.
+#[derive(Debug, Default, Clone, PartialEq, Eq)]
+pub(crate) struct ExpansionReport {
+    /// Counts of filtered entries per section.
+    pub stats: FilteringStats,
+    /// One event per entry removed by a `when` expression.
+    pub filtered_entries: Vec<FilteredEntry>,
+}
+
 /// Context shared by expansion operations.
 ///
 /// `env` is the Jinja environment used to render templates. `section` is the
@@ -35,29 +64,35 @@ struct ExpansionContext<'a> {
 ///
 /// Returns an error when evaluating `foreach` or `when` expressions, when
 /// iteration values fail to serialize, or when target metadata is malformed.
-pub(crate) fn expand_foreach(doc: &mut ManifestValue, env: &Environment) -> Result<FilteringStats> {
+pub(crate) fn expand_foreach(
+    doc: &mut ManifestValue,
+    env: &Environment,
+) -> Result<ExpansionReport> {
     let filtered_targets = expand_section(doc, "targets", env)?;
     let filtered_actions = expand_section(doc, "actions", env)?;
     let stats = FilteringStats {
-        filtered_targets,
-        filtered_actions,
+        filtered_targets: filtered_targets.len(),
+        filtered_actions: filtered_actions.len(),
     };
-    debug!(
-        filtered_targets = stats.filtered_targets,
-        filtered_actions = stats.filtered_actions,
-        filtered_entry_count = stats.filtered_targets + stats.filtered_actions,
-        "expanded manifest foreach and when directives"
-    );
-    Ok(stats)
+    let mut filtered_entries = filtered_targets;
+    filtered_entries.extend(filtered_actions);
+    Ok(ExpansionReport {
+        stats,
+        filtered_entries,
+    })
 }
 
-fn expand_section(doc: &mut ManifestValue, key: &str, env: &Environment) -> Result<usize> {
+fn expand_section(
+    doc: &mut ManifestValue,
+    key: &str,
+    env: &Environment,
+) -> Result<Vec<FilteredEntry>> {
     let Some(entries) = doc.get_mut(key).and_then(|v| v.as_array_mut()) else {
-        return Ok(0);
+        return Ok(Vec::new());
     };
 
     let mut expanded = Vec::new();
-    let mut filtered = 0;
+    let mut filtered = Vec::new();
     let context = ExpansionContext { env, section: key };
     for entry in std::mem::take(entries) {
         match entry {
@@ -75,16 +110,16 @@ fn expand_section(doc: &mut ManifestValue, key: &str, env: &Environment) -> Resu
 fn expand_target(
     mut map: ManifestMap,
     context: &ExpansionContext<'_>,
-    filtered: &mut usize,
+    filtered: &mut Vec<FilteredEntry>,
 ) -> Result<Vec<ManifestValue>> {
     if let Some(expr_val) = map.get("foreach") {
         let values = parse_foreach_values(expr_val, context.env)?;
         let mut items = Vec::new();
         for (index, item) in values.into_iter().enumerate() {
             let mut clone = map.clone();
             clone.remove("foreach");
-            if !when_allows(&mut clone, context, Some((&item, index)))? {
-                *filtered += 1;
+            if let Some(event) = when_allows(&mut clone, context, Some((&item, index)))? {
+                filtered.push(event);
                 continue;
             }
             inject_iteration_vars(&mut clone, &item, index)?;
@@ -94,8 +129,8 @@ fn expand_target(
     } else {
         // For targets without foreach, still evaluate and remove the `when` clause.
         // Use empty context since there's no iteration variable.
-        if !when_allows(&mut map, context, None)? {
-            *filtered += 1;
+        if let Some(event) = when_allows(&mut map, context, None)? {
+            filtered.push(event);
             return Ok(vec![]);
         }
         Ok(vec![ManifestValue::Object(map)])
@@ -186,35 +221,25 @@ fn when_allows(
     map: &mut ManifestMap,
     context: &ExpansionContext<'_>,
     iteration: Option<(&Value, usize)>,
-) -> Result<bool> {
+) -> Result<Option<FilteredEntry>> {
     let Some(when_val) = map.remove("when") else {
-        return Ok(true);
+        return Ok(None);
     };
     let expr = as_str(&when_val, "when")?;
     let ctx = when_context(map, iteration)?;
     let allowed = eval_when(context.env, expr, ctx)?;
-    if !allowed && has_subscriber() {
-        let entry_name = entry_name(map);
-        let iteration_index = iteration.map(|(_, index)| index);
-        let entry_name_hash = entry_name_hash(entry_name);
-        // Keep filtering logs useful without leaking manifest contents. The raw
-        // entry name has unbounded cardinality and may carry PII, so log only a
-        // short stable hash for correlation. The raw `when` expression may
-        // contain secret literals from the manifest, so expose only its length.
-        debug!(
-            section = context.section,
-            entry_name_hash,
-            iteration_index,
-            when_expression_len = expr.len(),
-            when_result = false,
-            "filtered manifest entry by when expression"
-        );
+    if allowed {
+        return Ok(None);
     }
-    Ok(allowed)
-}
-
-fn has_subscriber() -> bool {
-    dispatcher::get_default(|current| !current.is::<NoSubscriber>())
+    // Report what was filtered through data rather than telemetry; the
+    // caller owns the tracing policy. Only bounded, non-sensitive fields are
+    // captured (see `FilteredEntry`).
+    Ok(Some(FilteredEntry {
+        section: context.section.to_owned(),
+        entry_name_hash: entry_name_hash(entry_name(map)),
+        iteration_index: iteration.map(|(_, index)| index),
+        when_expression_len: expr.len(),
+    }))
 }
 
 fn when_context(map: &ManifestMap, iteration: Option<(&Value, usize)>) -> Result<Value> {

src/manifest/expand_test_cases/tracing_capture.rs

@@ -79,7 +79,7 @@ fn with_test_subscriber<T>(test: impl FnOnce(CapturedEvents) -> T) -> T {
 }
 
 #[test]
-fn expand_foreach_emits_debug_event_for_filtered_entry() -> Result<()> {
+fn trace_expansion_report_emits_debug_event_for_filtered_entry() -> Result<()> {
     with_test_subscriber(|captured| {
         let env = Environment::new();
         let when_expr = "secret_token == 'literal-secret'";
@@ -91,12 +91,20 @@ fn expand_foreach_emits_debug_event_for_filtered_entry() -> Result<()> {
         );
         let mut doc: ManifestValue = serde_saphyr::from_str(&yaml)?;
 
-        let stats = expand_foreach(&mut doc, &env)?;
+        // Expansion itself is telemetry-free; the orchestrator-side helper
+        // owns the tracing policy.
+        let report = expand_foreach(&mut doc, &env)?;
+        let pre_trace_events = captured.snapshot();
+        anyhow::ensure!(
+            pre_trace_events.is_empty(),
+            "expansion must not emit telemetry itself: {pre_trace_events:?}"
+        );
+        crate::manifest::trace_expansion_report(&report);
         let events = captured.snapshot();
 
         anyhow::ensure!(
-            stats.filtered_targets == 1,
-            "expected one filtered target: {stats:?}"
+            report.stats.filtered_targets == 1,
+            "expected one filtered target: {report:?}"
         );
         let expected_hash = entry_name_hash("skipped-target-secret");
         let event = events

src/manifest/expand_tests.rs

@@ -76,15 +76,28 @@ actions:
     when: item != 'skip'";
     let mut doc: ManifestValue = serde_saphyr::from_str(yaml)?;
 
-    let stats = expand_foreach(&mut doc, &env)?;
+    let report = expand_foreach(&mut doc, &env)?;
 
     anyhow::ensure!(
-        stats
+        report.stats
             == FilteringStats {
                 filtered_targets: 1,
                 filtered_actions: 2,
             },
-        "unexpected filtering stats: {stats:?}"
+        "unexpected filtering stats: {report:?}"
+    );
+    anyhow::ensure!(
+        report.filtered_entries.len() == 3,
+        "expected one filtering event per removed entry: {report:?}"
+    );
+    anyhow::ensure!(
+        report
+            .filtered_entries
+            .iter()
+            .filter(|entry| entry.section == "targets")
+            .count()
+            == 1,
+        "expected one filtered target event: {report:?}"
     );
     anyhow::ensure!(targets(&doc)?.len() == 1, "expected one kept target");
     anyhow::ensure!(actions(&doc)?.len() == 1, "expected one kept action");

src/manifest/mod.rs

@@ -47,6 +47,7 @@ pub use diagnostics::{
 };
 pub use glob::glob_paths;
 
+use expand::ExpansionReport;
 pub(crate) use expand::expand_foreach;
 pub use render::render_manifest;
 
@@ -99,6 +100,31 @@ fn env_var(name: &str) -> std::result::Result<String, Error> {
     }
 }
 
+/// Emit tracing events for a manifest expansion report.
+///
+/// Expansion itself is a pure transformation that reports filtering through
+/// [`ExpansionReport`]; this orchestrator-side helper owns the telemetry
+/// policy. The report's fields are already bounded and non-sensitive (name
+/// hash, expression length), so they can be logged verbatim.
+fn trace_expansion_report(report: &ExpansionReport) {
+    for entry in &report.filtered_entries {
+        tracing::debug!(
+            section = entry.section.as_str(),
+            entry_name_hash = entry.entry_name_hash.as_str(),
+            iteration_index = entry.iteration_index,
+            when_expression_len = entry.when_expression_len,
+            when_result = false,
+            "filtered manifest entry by when expression"
+        );
+    }
+    tracing::debug!(
+        filtered_targets = report.stats.filtered_targets,
+        filtered_actions = report.stats.filtered_actions,
+        filtered_entry_count = report.stats.filtered_targets + report.stats.filtered_actions,
+        "expanded manifest foreach and when directives"
+    );
+}
+
 /// Invoke the stage callback when present.
 fn notify_stage(
     on_stage: &mut Option<&mut dyn FnMut(ManifestLoadStage)>,
@@ -161,7 +187,8 @@ fn from_str_named(
     notify_stage(on_stage, ManifestLoadStage::TemplateExpansion);
     register_manifest_macros(&doc, &mut jinja)?;
 
-    expand_foreach(&mut doc, &jinja)?;
+    let expansion_report = expand_foreach(&mut doc, &jinja)?;
+    trace_expansion_report(&expansion_report);
 
     notify_stage(on_stage, ManifestLoadStage::FinalRendering);
     let manifest: NetsukeManifest =