diff --git a/Cargo.lock b/Cargo.lock index 2de971b13..f45bc947f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2775,6 +2775,7 @@ dependencies = [ "tokio", "toml 0.8.23", "tracing", + "tracing-subscriber", ] [[package]] diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index dfeba6f01..edc222c0f 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -34,6 +34,7 @@ tracing.workspace = true [dev-dependencies] fabro-test.workspace = true +tracing-subscriber.workspace = true insta.workspace = true serde_json.workspace = true temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index eecacaa2e..54cb39521 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -1,13 +1,15 @@ //! Application adapter that packages caller-supplied workflow contents for //! the `fabro_workflow_version_create` tool. +use anyhow::Context as _; use async_trait::async_trait; use fabro_tool::{ PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, }; +use fabro_util::error::collect_chain; use fabro_workflow_version::WorkflowVersionError; use tokio::task; -use tracing::warn; +use tracing::debug; use crate::WorkflowVersionCollectError; @@ -23,23 +25,38 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { &self, source: ValidatedWorkflowVersionCreate, ) -> anyhow::Result { - task::spawn_blocking(move || { - let closure = - crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) - .map_err(|err| { - warn!(error = %format!("{err:#}"), "workflow version packaging failed"); - ToolError::message(render_packaging_error(&err)) - })?; - Ok(PackagedWorkflowVersions { - root_id: closure.root_id(), - versions: closure.into_versions(), - }) - }) - .await - .map_err(|err| anyhow::anyhow!("workflow packaging task failed: {err}"))? + let packaged = task::spawn_blocking(move || package_blocking(&source)) + .await + .context("workflow packaging task failed")??; + Ok(packaged) } } +/// Stage, collect, and validate on the calling thread. +/// +/// Packaging failures are expected input errors, so they log at DEBUG. The +/// event carries the collector error's own message only: parser and template +/// diagnostics further down the chain quote supplied file contents, which +/// must not reach the log at any level. +fn package_blocking( + source: &ValidatedWorkflowVersionCreate, +) -> Result { + let closure = crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) + .map_err(|err| { + debug!( + entrypoint = %source.entrypoint, + file_count = source.files.len(), + error = %err, + "workflow version packaging failed" + ); + ToolError::message(render_packaging_error(&err)) + })?; + Ok(PackagedWorkflowVersions { + root_id: closure.root_id(), + versions: closure.into_versions(), + }) +} + /// Render a packaging failure for the tool caller. Every collector variant's /// own message names paths and counts only, so most render their full cause /// chain and the caller can fix the input. The graph parser, TOML parser, and @@ -57,7 +74,7 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { _ => false, }; if !quotes_source { - return fabro_util::error::collect_chain(err).join(": "); + return collect_chain(err).join(": "); } let summary = match err { // `WorkflowVersionError` names the offending path; only its source @@ -70,8 +87,37 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { #[cfg(test)] mod tests { + #[expect( + clippy::disallowed_types, + reason = "test log capture writes synchronously into memory" + )] + use std::io::Write; + use std::sync::{Arc, Mutex}; + + use tracing::{Level, subscriber}; + use super::*; + #[derive(Clone, Default)] + struct CapturedLog(Arc>>); + + impl Write for CapturedLog { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl CapturedLog { + fn text(&self) -> String { + String::from_utf8(self.0.lock().unwrap().clone()).unwrap() + } + } + fn source(entrypoint: &str, files: &[(&str, &str)]) -> ValidatedWorkflowVersionCreate { ValidatedWorkflowVersionCreate { entrypoint: entrypoint.parse().unwrap(), @@ -154,6 +200,41 @@ mod tests { } } + #[test] + fn packaging_failure_log_never_carries_supplied_source() { + let log = CapturedLog::default(); + let writer = log.clone(); + let subscriber = tracing_subscriber::fmt() + .with_max_level(Level::TRACE) + .with_ansi(false) + .with_writer(move || writer.clone()) + .finish(); + let inputs = [ + source("workflow", &[( + "workflow", + "PRIVATE_CONTENT invalid source", + )]), + source("workflow.toml", &[( + "workflow.toml", + "_version = 1\nPRIVATE_CONTENT = [unterminated", + )]), + ]; + // The guard is load-bearing: the full chain does quote the source. + let leaky = + crate::collect_supplied_workflow_versions(&inputs[0].entrypoint, &inputs[0].files) + .unwrap_err(); + assert!(collect_chain(&leaky).join(": ").contains("PRIVATE_CONTENT")); + subscriber::with_default(subscriber, || { + for input in &inputs { + package_blocking(input).unwrap_err(); + } + }); + let text = log.text(); + assert!(text.contains("workflow version packaging failed"), "{text}"); + assert!(!text.contains("PRIVATE_CONTENT"), "{text}"); + assert!(text.contains("DEBUG"), "{text}"); + } + #[tokio::test] async fn path_only_failures_tell_the_caller_what_to_fix() { let mut missing_child = fixture();