Keep supplied workflow source out of packaging failure logs

The packager logged the full packaging error chain at WARN. That chain
embeds caller-supplied workflow and prompt source: the graph parser's
diagnostic includes the unparsed remainder and the TOML parser prints
the offending line. The logging strategy prohibits user file contents
in tracing events at every level, and this adapter runs inside
`fabro mcp` and run workers at the default filter.

Log the collector error's own path-only message at DEBUG, since a
malformed request is an expected input error, together with the
entrypoint and file count. Wrap the blocking-task join error with
`context` instead of interpolating it. A test installs a TRACE-level
subscriber around the blocking path and checks that the fixture's
source marker, which the full chain does contain, never reaches the
log.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-09-11 15:38:59 -06:00
parent 97a7bb06e5
commit 709d15f908
3 changed files with 99 additions and 16 deletions

1
Cargo.lock generated
View file

@ -2775,6 +2775,7 @@ dependencies = [
"tokio",
"toml 0.8.23",
"tracing",
"tracing-subscriber",
]
[[package]]

View file

@ -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"

View file

@ -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<PackagedWorkflowVersions> {
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<PackagedWorkflowVersions, ToolError> {
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<Mutex<Vec<u8>>>);
impl Write for CapturedLog {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
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();