diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index 62c6b2d0b..dfeba6f01 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -22,6 +22,7 @@ fabro-graphviz = { path = "../fabro-graphviz" } fabro-template = { path = "../../foundation/fabro-template" } fabro-tool = { path = "../fabro-tool" } fabro-types = { path = "../../foundation/fabro-types" } +fabro-util = { path = "../../foundation/fabro-util" } fabro-workflow = { path = "../fabro-workflow" } fabro-workflow-version = { path = "../fabro-workflow-version" } git2.workspace = true @@ -33,7 +34,6 @@ tracing.workspace = true [dev-dependencies] fabro-test.workspace = true -fabro-util = { path = "../../foundation/fabro-util" } insta.workspace = true serde_json.workspace = true temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index 0970bf9ff..e74b533c7 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -4,12 +4,13 @@ use std::collections::BTreeMap; use std::path::Path; -use anyhow::Result; use fabro_config::project::WorkflowLocation; use fabro_types::WorkflowPath; use tempfile::TempDir; -use crate::CollectedWorkflowClosure; +use crate::{CollectedWorkflowClosure, WorkflowVersionCollectError}; + +type Result = std::result::Result; /// Stage `files` in a private temporary directory and collect the workflow /// closure rooted at `entrypoint` with the same collector used for checkouts. @@ -22,34 +23,49 @@ pub fn collect_supplied_workflow_versions( ) -> Result { let staging = tempfile::Builder::new() .prefix("fabro-workflow-version-") - .tempdir()?; + .tempdir() + .map_err(stage_error)?; collect_in_staging(entrypoint, files, &staging) } +fn stage_error(source: std::io::Error) -> WorkflowVersionCollectError { + WorkflowVersionCollectError::Stage { source } +} + fn collect_in_staging( entrypoint: &WorkflowPath, files: &BTreeMap, staging: &TempDir, ) -> Result { - let root = staging.path().canonicalize()?; + let root = staging.path().canonicalize().map_err(stage_error)?; for (path, contents) in files { let destination = root.join(path.as_str()); if let Some(parent) = destination.parent() { - std::fs::create_dir_all(parent)?; + std::fs::create_dir_all(parent).map_err(stage_error)?; } - std::fs::write(destination, contents)?; + std::fs::write(destination, contents).map_err(stage_error)?; } let entrypoint = Path::new(entrypoint.as_str()); - let location = WorkflowLocation::from_exact_path(entrypoint, &root)?; + let location = + WorkflowLocation::from_exact_path(entrypoint, &root).map_err(|source| match source { + fabro_config::Error::WorkflowNotFound(_) => { + WorkflowVersionCollectError::WorkflowNotFound { + path: entrypoint.to_path_buf(), + } + } + source => WorkflowVersionCollectError::Collect { + path: entrypoint.to_path_buf(), + source: source.into(), + }, + })?; let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; // A case-insensitive host must not satisfy a reference that is missing // from the supplied tree under its exact key. for (_, version) in closure.versions() { for path in version.version().files().keys() { - anyhow::ensure!( - files.contains_key(path), - "collected file `{path}` was not supplied" - ); + if !files.contains_key(path) { + return Err(WorkflowVersionCollectError::NotSupplied { path: path.clone() }); + } } } Ok(closure) @@ -59,6 +75,8 @@ fn collect_in_staging( mod tests { use std::path::Path; + use fabro_util::error::collect_chain; + use super::*; struct Supplied { @@ -243,7 +261,7 @@ mod tests { let error = collect_with_staging(&input, staging) .err() .unwrap_or_else(|| panic!("accepted invalid fixture {index}")); - let rendered = format!("{error:#}"); + let rendered = collect_chain(&error).join(": "); // Escaping references must fail before any host file is opened, // so no host diagnostic (parse error, exists-vs-missing) leaks. assert!( @@ -277,9 +295,10 @@ mod tests { ]); let error = collect_supplied_workflow_versions(&renamed.entrypoint, &renamed.files).unwrap_err(); + let rendered = collect_chain(&error).join(": "); assert!( - format!("{error:#}").contains("must be `sub/workflow.toml`"), - "{error:#}" + rendered.contains("must be `sub/workflow.toml`"), + "{rendered}" ); // A config that selects a graph in another directory is not its sibling. let elsewhere = supplied("sub/workflow.toml", &[ diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 98202fbe1..f79a15d20 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -38,6 +38,15 @@ pub(super) struct CollectedWorkflowSource { pub(super) dependency_keys: BTreeSet, } +/// A referenced file that does not exist under the package root, reported +/// with its package-relative path so callers can surface it without the +/// staging directory or any file content. +#[derive(Debug, thiserror::Error)] +#[error("workflow package file `{path}` is missing")] +pub(super) struct MissingPackageFile { + pub(super) path: String, +} + impl<'a> WorkflowBundler<'a> { pub(super) fn new(package_root: &'a Path, inputs: &'a HashMap) -> Self { Self { @@ -512,11 +521,16 @@ impl<'a> WorkflowBundler<'a> { return std::fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display())); } - let canonical = path.canonicalize().with_context(|| { - format!( + let canonical = path.canonicalize().map_err(|source| { + if source.kind() == std::io::ErrorKind::NotFound { + let path = ManifestPath::from_absolute(path, self.package_root) + .map_or_else(|| path.display().to_string(), |path| path.to_string()); + return anyhow::Error::new(MissingPackageFile { path }); + } + anyhow::Error::new(source).context(format!( "failed to canonicalize workflow package file `{}`", path.display() - ) + )) })?; if !canonical.starts_with(self.package_root) { bail!( diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 3c7dd95e4..c6437b170 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -10,7 +10,9 @@ use fabro_types::{ use fabro_workflow_version::{ValidatedWorkflowVersion, WorkflowVersionError}; use thiserror::Error; -use crate::workflow_bundler::{CollectedWorkflowSource, CollectedWorkflowSources, WorkflowBundler}; +use crate::workflow_bundler::{ + CollectedWorkflowSource, CollectedWorkflowSources, MissingPackageFile, WorkflowBundler, +}; /// One locally packaged workflow-version closure in dependency-first order. #[derive(Debug)] @@ -80,6 +82,20 @@ pub enum WorkflowVersionCollectError { DependencyCycle { path: WorkflowPath }, #[error("collected workflow dependency `{path}` is missing")] MissingWorkflow { path: String }, + /// A referenced file is absent from the package root. Surfaced separately + /// from [`Self::Collect`] because the path is the whole message. + #[error("referenced file `{path}` is missing from the workflow source")] + MissingPackageFile { path: String }, + /// Supplied-content packaging: the collector read a file whose exact key + /// the caller did not supply (a case-insensitive host satisfied a + /// reference that differs from the supplied key). + #[error("collected file `{path}` was not supplied")] + NotSupplied { path: WorkflowPath }, + #[error("failed to stage supplied workflow files")] + Stage { + #[source] + source: std::io::Error, + }, } /// Package one workflow and every separately runnable dependency from a local @@ -157,9 +173,19 @@ pub fn collect_workflow_versions_at_location( let inputs = HashMap::new(); let collected = WorkflowBundler::new(package_root, &inputs) .collect_versions(location) - .map_err(|source| WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source, + .map_err(|source| { + let missing = source + .chain() + .find_map(|cause| cause.downcast_ref::()); + match missing { + Some(missing) => WorkflowVersionCollectError::MissingPackageFile { + path: missing.path.clone(), + }, + None => WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source, + }, + } })?; VersionAssembler::new(collected).assemble() } diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index 16a4f51a5..eecacaa2e 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -5,15 +5,17 @@ use async_trait::async_trait; use fabro_tool::{ PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, }; +use fabro_workflow_version::WorkflowVersionError; use tokio::task; use tracing::warn; +use crate::WorkflowVersionCollectError; + /// Packages supplied workflow contents for standalone MCP and capable run /// workers; the backend that owns the API client performs registration. pub struct SuppliedWorkflowVersionPackager; -const PACKAGING_FAILED: &str = "workflow source could not be packaged; check configuration, \ - syntax, local references, and package limits"; +const PACKAGING_HINT: &str = "check configuration, syntax, local references, and package limits"; #[async_trait] impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { @@ -25,11 +27,8 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { let closure = crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) .map_err(|err| { - // Parser diagnostics may quote supplied source, so the - // chain stays in the log and only a generic message - // crosses the tool boundary. warn!(error = %format!("{err:#}"), "workflow version packaging failed"); - ToolError::message(PACKAGING_FAILED) + ToolError::message(render_packaging_error(&err)) })?; Ok(PackagedWorkflowVersions { root_id: closure.root_id(), @@ -41,6 +40,34 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { } } +/// 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 +/// template engine quote the offending source in their diagnostics, so +/// failures that reach them stop at the last path-only level and add a hint. +fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { + let quotes_source = match err { + WorkflowVersionCollectError::Collect { .. } => true, + WorkflowVersionCollectError::InvalidVersion { source, .. } => matches!( + source, + WorkflowVersionError::GraphParse { .. } + | WorkflowVersionError::Template { .. } + | WorkflowVersionError::Config { .. } + ), + _ => false, + }; + if !quotes_source { + return fabro_util::error::collect_chain(err).join(": "); + } + let summary = match err { + // `WorkflowVersionError` names the offending path; only its source + // quotes content. + WorkflowVersionCollectError::InvalidVersion { source, .. } => format!("{err}: {source}"), + _ => err.to_string(), + }; + format!("{summary}; {PACKAGING_HINT}") +} + #[cfg(test)] mod tests { use super::*; @@ -70,6 +97,14 @@ mod tests { ]) } + async fn package_error(input: ValidatedWorkflowVersionCreate) -> String { + let error = SuppliedWorkflowVersionPackager + .package(input) + .await + .unwrap_err(); + format!("{error:#}") + } + #[tokio::test] async fn packager_returns_dependencies_before_root() { let packaged = SuppliedWorkflowVersionPackager @@ -101,27 +136,34 @@ mod tests { "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"missing.md\"" .into(), ); - let mut oversized = fixture(); - oversized.files.insert( - "prompt.md".parse().unwrap(), - "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1), + let mut invalid_config = fixture(); + invalid_config.files.insert( + "workflow.toml".parse().unwrap(), + "_version = 1\nPRIVATE_CONTENT = [unterminated".into(), ); for input in [ invalid_root, - oversized, + invalid_config, source("workflow", &[( "workflow", "PRIVATE_CONTENT invalid source", )]), ] { - let error = SuppliedWorkflowVersionPackager - .package(input) - .await - .unwrap_err(); - let rendered = format!("{error:#}"); + let rendered = package_error(input).await; assert!(!rendered.contains("PRIVATE_CONTENT"), "{rendered}"); - assert_eq!(rendered, PACKAGING_FAILED); } + } + + #[tokio::test] + async fn path_only_failures_tell_the_caller_what_to_fix() { + let mut missing_child = fixture(); + missing_child.files.remove(&"child.fabro".parse().unwrap()); + let rendered = package_error(missing_child).await; + assert!( + rendered.contains("`child.fabro`") && rendered.contains("missing"), + "{rendered}" + ); + let mut wrong_case = fixture(); let prompt = wrong_case .files @@ -130,12 +172,25 @@ mod tests { wrong_case .files .insert("Prompt.md".parse().unwrap(), prompt); - assert!( - SuppliedWorkflowVersionPackager - .package(wrong_case) - .await - .is_err(), - "a case-insensitive host must not satisfy an exact reference" + // A case-insensitive host reads the file and reports the unsupplied + // key; a case-sensitive host reports it missing. Either names the + // path the graph asked for. + let rendered = package_error(wrong_case).await; + assert!(rendered.contains("`prompt.md`"), "{rendered}"); + + let mut oversized = fixture(); + oversized.files.insert( + "prompt.md".parse().unwrap(), + "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1), ); + let rendered = package_error(oversized).await; + assert!(rendered.contains("canonical bytes"), "{rendered}"); + + let rendered = package_error(source("workflow", &[( + "workflow", + "PRIVATE_CONTENT invalid source", + )])) + .await; + assert!(rendered.ends_with(PACKAGING_HINT), "{rendered}"); } }