diff --git a/src/manifest/expand.rs b/src/manifest/expand.rs index d4bf2087..0f229fb1 100644 --- a/src/manifest/expand.rs +++ b/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, + /// 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, +} + /// 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 { +pub(crate) fn expand_foreach( + doc: &mut ManifestValue, + env: &Environment, +) -> Result { 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 { +fn expand_section( + doc: &mut ManifestValue, + key: &str, + env: &Environment, +) -> Result> { 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,7 +110,7 @@ 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, ) -> Result> { if let Some(expr_val) = map.get("foreach") { let values = parse_foreach_values(expr_val, context.env)?; @@ -83,8 +118,8 @@ fn expand_target( 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 { +) -> Result> { 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::()) + // 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 { diff --git a/src/manifest/expand_test_cases/tracing_capture.rs b/src/manifest/expand_test_cases/tracing_capture.rs index 3d786c25..a5031b4c 100644 --- a/src/manifest/expand_test_cases/tracing_capture.rs +++ b/src/manifest/expand_test_cases/tracing_capture.rs @@ -79,7 +79,7 @@ fn with_test_subscriber(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 diff --git a/src/manifest/expand_tests.rs b/src/manifest/expand_tests.rs index 1ff974f8..11e2dda1 100644 --- a/src/manifest/expand_tests.rs +++ b/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"); diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index a074e6c4..77141c9a 100644 --- a/src/manifest/mod.rs +++ b/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 { } } +/// 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 =