From 0b46e1d7355eafb32ec8e12446dff808deacd71d Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sun, 30 Aug 2026 13:00:32 -0400 Subject: [PATCH] Simplify local RunIntent producer support Share canonicalize_location and resolve_existing_workflow_location between the local package resolver and the version collector, drop the redundant package-root pre-check and the PackageFileReadError enum in favor of anyhow context, and read HEAD's SHA from git2 instead of a separate rev-parse subprocess. Make GitRunTargetObservation a plain struct, replace the repo-info tuple with a named struct, tighten the closure view trait, avoid deep-cloning the root workflow during collected validation, remove the unused into_closure accessor, and dedupe test helpers. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 1 + .../fabro-server/src/manifest_validation.rs | 12 +- lib/apps/fabro-server/src/run_intent.rs | 32 ++--- lib/components/fabro-manifest/Cargo.toml | 1 + lib/components/fabro-manifest/src/lib.rs | 108 +++++++-------- .../src/local_workflow_package.rs | 124 +++++------------- .../fabro-manifest/src/workflow_bundler.rs | 57 +++----- .../src/workflow_version_collector.rs | 59 ++++----- .../fabro-workflow/src/workflow_bundle.rs | 5 + 9 files changed, 162 insertions(+), 237 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b92ef9ae1..92e98adb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2826,6 +2826,7 @@ dependencies = [ "fabro-template", "fabro-test", "fabro-types", + "fabro-util", "fabro-workflow", "fabro-workflow-version", "git2", diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 28e9928ae..648180db4 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -46,8 +46,8 @@ pub fn validate_collected_workflow( let lowered = run_intent::lower_collected_workflow_closure(closure)?; let workflow = lowered .workflow_bundle - .workflow(&lowered.entrypoint) - .cloned() + .into_workflows() + .remove(&lowered.entrypoint) .ok_or_else(|| anyhow!("lowered root workflow is missing from its bundle"))?; let mut builder = WorkflowSettingsBuilder::new() .server_manifest_defaults( @@ -160,6 +160,10 @@ dockerfile = { path = "Dockerfile" } root.join("root/workflow.toml") } + fn owner_input() -> HashMap { + HashMap::from([("owner".to_string(), toml::Value::String("Ada".to_string()))]) + } + fn run_overrides(goal: &str) -> RunLayer { RunLayer { goal: Some(RunGoalLayer::Inline(InterpString::parse(goal))), @@ -172,7 +176,7 @@ dockerfile = { path = "Dockerfile" } let temp = tempfile::tempdir().unwrap(); let workflow = write_complete_fixture(temp.path()); let run = run_overrides("inline goal"); - let inputs = HashMap::from([("owner".to_string(), toml::Value::String("Ada".to_string()))]); + let inputs = owner_input(); let package = fabro_manifest::resolve_local_workflow_package( &workflow, temp.path(), @@ -226,7 +230,7 @@ dockerfile = { path = "Dockerfile" } let present = validate_collected_workflow( package.closure(), Some(&run_overrides("resolved inline goal")), - &HashMap::from([("owner".to_string(), toml::Value::String("Ada".to_string()))]), + &owner_input(), ) .unwrap(); assert!( diff --git a/lib/apps/fabro-server/src/run_intent.rs b/lib/apps/fabro-server/src/run_intent.rs index a872019b5..489306828 100644 --- a/lib/apps/fabro-server/src/run_intent.rs +++ b/lib/apps/fabro-server/src/run_intent.rs @@ -184,11 +184,16 @@ pub(crate) enum WorkflowClosureLoweringError { }, } +/// Read access to a workflow-version closure, whether loaded from the store +/// or collected from a local checkout, so both lower through one path. trait WorkflowClosureView { fn root_id(&self) -> WorkflowVersionId; - fn root(&self) -> &WorkflowVersion; fn validated_root(&self) -> &ValidatedWorkflowVersion; fn get(&self, id: &WorkflowVersionId) -> Option<&WorkflowVersion>; + + fn root(&self) -> &WorkflowVersion { + self.validated_root().version() + } } impl WorkflowClosureView for LoadedWorkflowVersionClosure { @@ -196,10 +201,6 @@ impl WorkflowClosureView for LoadedWorkflowVersionClosure { self.root_id() } - fn root(&self) -> &WorkflowVersion { - self.root() - } - fn validated_root(&self) -> &ValidatedWorkflowVersion { self.validated_root() } @@ -211,6 +212,7 @@ impl WorkflowClosureView for LoadedWorkflowVersionClosure { struct CollectedWorkflowClosureView<'a> { root_id: WorkflowVersionId, + root: &'a ValidatedWorkflowVersion, versions: HashMap, } @@ -218,10 +220,14 @@ impl<'a> CollectedWorkflowClosureView<'a> { fn new(closure: &'a CollectedWorkflowClosure) -> Result { let root_id = closure.root_id(); let versions = closure.versions().collect::>(); - if !versions.contains_key(&root_id) { - return Err(WorkflowClosureLoweringError::MissingVersion { id: root_id }); - } - Ok(Self { root_id, versions }) + let root = *versions + .get(&root_id) + .ok_or(WorkflowClosureLoweringError::MissingVersion { id: root_id })?; + Ok(Self { + root_id, + root, + versions, + }) } } @@ -230,14 +236,8 @@ impl WorkflowClosureView for CollectedWorkflowClosureView<'_> { self.root_id } - fn root(&self) -> &WorkflowVersion { - self.validated_root().version() - } - fn validated_root(&self) -> &ValidatedWorkflowVersion { - self.versions - .get(&self.root_id) - .expect("collected closure view construction verifies its root") + self.root } fn get(&self, id: &WorkflowVersionId) -> Option<&WorkflowVersion> { diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index ab27375c4..1ac7e1eca 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -28,6 +28,7 @@ toml.workspace = true [dev-dependencies] fabro-test.workspace = true +fabro-util = { path = "../../foundation/fabro-util" } insta.workspace = true serde_json.workspace = true tempfile = "3" diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 3352a91cf..dcf049f63 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -29,7 +29,7 @@ use fabro_types::{ DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, WorkflowSettings, }; use fabro_workflow::git::{ - GitSyncStatus, branch_needs_push, head_sha, push_branch_noninteractive, sync_status, + GitSyncStatus, branch_needs_push, push_branch_noninteractive, sync_status, }; pub use crate::local_workflow_package::{ @@ -217,7 +217,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let configured_repo_origin_url = configured_repo_origin_url(&workflow_settings); let git = observe_git_run_target(&working_directory, configured_repo_origin_url.as_deref()) - .map(GitRunTargetObservation::into_legacy_git_context); + .map(|observation| observation.legacy_git_context); let args = input.args.filter(|args| !manifest_args_is_empty(args)); Ok(BuiltManifest { @@ -316,35 +316,8 @@ fn resolved_goal_to_manifest(resolved: ResolvedRunGoal) -> types::ManifestGoal { /// SHA and normalized-origin behavior independently. #[derive(Clone, Debug)] pub struct GitRunTargetObservation { - run_target: Option, - legacy_git_context: GitContext, -} - -impl GitRunTargetObservation { - #[must_use] - pub fn run_target(&self) -> Option<&GitRunTarget> { - self.run_target.as_ref() - } - - #[must_use] - pub fn into_run_target(self) -> Option { - self.run_target - } - - #[must_use] - pub fn dirty(&self) -> DirtyStatus { - self.legacy_git_context.dirty - } - - #[must_use] - pub fn legacy_git_context(&self) -> &GitContext { - &self.legacy_git_context - } - - #[must_use] - pub fn into_legacy_git_context(self) -> GitContext { - self.legacy_git_context - } + pub run_target: Option, + pub legacy_git_context: GitContext, } /// Observe Git facts without choosing an environment or a non-Git target. @@ -357,8 +330,12 @@ pub fn observe_git_run_target( repo_path: &Path, configured_repo_origin_url: Option<&str>, ) -> Option { - let (origin_url, push_origin_url, branch) = detect_manifest_repo_info(repo_path)?; - let sha = head_sha(repo_path).ok(); + let ManifestRepoInfo { + origin_url, + push_origin_url, + branch, + sha, + } = detect_manifest_repo_info(repo_path)?; let dirty = match sync_status(repo_path, "origin", Some(&branch)) { GitSyncStatus::Dirty => DirtyStatus::Dirty, GitSyncStatus::Synced | GitSyncStatus::Unsynced => DirtyStatus::Clean, @@ -382,7 +359,7 @@ pub fn observe_git_run_target( let run_target = github_run_target( &repo_origin_url, &branch, - remotely_available.then(|| sha.clone()).flatten(), + sha.clone().filter(|_| remotely_available), ); Some(GitRunTargetObservation { run_target, @@ -425,7 +402,17 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { (!normalized.is_empty()).then_some(normalized) } -fn detect_manifest_repo_info(repo_path: &Path) -> Option<(Option, Option, String)> { +struct ManifestRepoInfo { + /// The `origin` URL as libgit2 reports it (after `insteadOf` rewrites). + origin_url: Option, + /// The raw configured `remote.origin.url`, used to compare repository + /// identity before pushing. + push_origin_url: Option, + branch: String, + sha: Option, +} + +fn detect_manifest_repo_info(repo_path: &Path) -> Option { let repo = git2::Repository::discover(repo_path).ok()?; if repo.is_bare() { return None; @@ -435,6 +422,7 @@ fn detect_manifest_repo_info(repo_path: &Path) -> Option<(Option, Option return None; } let branch = head.shorthand().map(ToOwned::to_owned)?; + let sha = head.target().map(|oid| oid.to_string()); let origin_url = repo .find_remote("origin") .ok() @@ -448,7 +436,12 @@ fn detect_manifest_repo_info(repo_path: &Path) -> Option<(Option, Option .ok() .and_then(|config| config.get_string("remote.origin.url").ok()) .or_else(|| origin_url.clone()); - Some((origin_url, push_origin_url, branch)) + Some(ManifestRepoInfo { + origin_url, + push_origin_url, + branch, + sha, + }) } /// Best-effort push of the local branch so clone-based execution can see @@ -485,12 +478,19 @@ fn push_manifest_branch_best_effort( /// Resolve a workflow reference and reject it when neither its config nor /// its graph exists on disk. /// A missing workflow surfaces as `fabro_config::Error::WorkflowNotFound`. -fn resolve_existing_workflow_location(workflow: &Path, cwd: &Path) -> Result { +#[expect( + clippy::result_large_err, + reason = "callers match on the concrete config error to classify missing workflows" +)] +fn resolve_existing_workflow_location( + workflow: &Path, + cwd: &Path, +) -> Result { let location = WorkflowLocation::resolve(workflow, cwd)?; if location.toml.is_none() && !location.graph.is_file() { - return Err( - fabro_config::Error::WorkflowNotFound(location.graph.display().to_string()).into(), - ); + return Err(fabro_config::Error::WorkflowNotFound( + location.graph.display().to_string(), + )); } Ok(location) } @@ -547,6 +547,8 @@ pub(crate) mod test_fixtures { #[cfg(test)] mod tests { + use fabro_workflow::git::head_sha; + use super::*; fn test_environment_defaults() -> MergeMap { @@ -1724,14 +1726,14 @@ exit 1 mark_origin_branch_synced(&workspace, "feature"); let observation = observe_git_run_target(&workspace, None).unwrap(); - let target = observation.run_target().unwrap(); - let legacy = observation.legacy_git_context(); + let target = observation.run_target.as_ref().unwrap(); + let legacy = &observation.legacy_git_context; assert_eq!(target.repo, "acme/widgets"); assert_eq!(target.branch, "feature"); assert_eq!(target.tag, None); assert_eq!(target.sha, legacy.sha); - assert_eq!(observation.dirty(), DirtyStatus::Clean); + assert_eq!(legacy.dirty, DirtyStatus::Clean); assert_eq!(legacy.origin_url, "https://github.com/acme/widgets"); } @@ -1751,10 +1753,10 @@ exit 1 let observation = observe_git_run_target(&workspace, Some("https://github.com/acme/widgets")).unwrap(); - let target = observation.run_target().unwrap(); + let target = observation.run_target.as_ref().unwrap(); - assert_eq!(target.sha, observation.legacy_git_context().sha); - assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), target.sha,); + assert_eq!(target.sha, observation.legacy_git_context.sha); + assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), target.sha); } #[test] @@ -1777,9 +1779,9 @@ exit 1 let failed = observe_git_run_target(&failed_workspace, Some("https://github.com/acme/widgets")) .unwrap(); - assert_eq!(failed.run_target().unwrap().sha, None); - assert!(failed.legacy_git_context().sha.is_some()); - assert_eq!(failed.dirty(), DirtyStatus::Clean); + assert_eq!(failed.run_target.as_ref().unwrap().sha, None); + assert!(failed.legacy_git_context.sha.is_some()); + assert_eq!(failed.legacy_git_context.dirty, DirtyStatus::Clean); let mismatched_workspace = temp.path().join("mismatched"); std::fs::create_dir_all(&mismatched_workspace).unwrap(); @@ -1795,7 +1797,7 @@ exit 1 Some("https://github.com/acme/configured"), ) .unwrap(); - let target = mismatched.run_target().unwrap(); + let target = mismatched.run_target.as_ref().unwrap(); assert_eq!(target.repo, "acme/configured"); assert_eq!(target.sha, None); assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), None); @@ -1811,10 +1813,10 @@ exit 1 std::fs::write(workspace.join("dirty.txt"), "dirty").unwrap(); let unsupported = observe_git_run_target(&workspace, None).unwrap(); - assert!(unsupported.run_target().is_none()); - assert_eq!(unsupported.dirty(), DirtyStatus::Dirty); + assert!(unsupported.run_target.is_none()); + assert_eq!(unsupported.legacy_git_context.dirty, DirtyStatus::Dirty); assert_eq!( - unsupported.legacy_git_context().origin_url, + unsupported.legacy_git_context.origin_url, fabro_github::normalize_repo_origin_url(&bare_origin.to_string_lossy()), ); diff --git a/lib/components/fabro-manifest/src/local_workflow_package.rs b/lib/components/fabro-manifest/src/local_workflow_package.rs index 36d47c095..65cf5167e 100644 --- a/lib/components/fabro-manifest/src/local_workflow_package.rs +++ b/lib/components/fabro-manifest/src/local_workflow_package.rs @@ -8,7 +8,9 @@ use std::path::{Path, PathBuf}; use fabro_config::project::{WorkflowLocation, discover_project_config}; use thiserror::Error; -use crate::workflow_version_collector::collect_workflow_versions_at_location; +use crate::workflow_version_collector::{ + canonicalize_location, collect_workflow_versions_at_location, +}; use crate::{CollectedWorkflowClosure, WorkflowVersionCollectError}; /// One local workflow resolved to a canonical package root and collected @@ -41,11 +43,6 @@ pub enum LocalWorkflowPackageError { #[source] source: std::io::Error, }, - #[error("local workflow package path `{path}` escapes source root `{source_root}`")] - EscapesSourceRoot { - path: PathBuf, - source_root: PathBuf, - }, #[error("failed to collect local workflow package `{workflow}`")] Collect { workflow: PathBuf, @@ -69,11 +66,6 @@ impl ResolvedLocalWorkflowPackage { pub fn closure(&self) -> &CollectedWorkflowClosure { &self.closure } - - #[must_use] - pub fn into_closure(self) -> CollectedWorkflowClosure { - self.closure - } } /// Resolve producer-readable workflow bytes under one stable local source @@ -95,8 +87,12 @@ pub fn resolve_local_workflow_package( resolve_explicit_workflow(workflow, cwd, user_workflows_root)? }; let source_root = canonicalize(&source_root)?; - let workflow_location = canonicalize_location(location)?; - ensure_location_is_within_root(&workflow_location, &source_root)?; + let workflow_location = canonicalize_location(location, |path, source| { + LocalWorkflowPackageError::Canonicalize { + path: path.to_path_buf(), + source, + } + })?; let closure = collect_workflow_versions_at_location(&workflow_location, &source_root, workflow) .map_err(|source| LocalWorkflowPackageError::Collect { workflow: workflow.to_path_buf(), @@ -227,19 +223,12 @@ fn resolve_location( selected: &Path, cwd: &Path, ) -> Result { - let location = WorkflowLocation::resolve(selected, cwd).map_err(|source| { + crate::resolve_existing_workflow_location(selected, cwd).map_err(|source| { LocalWorkflowPackageError::Resolve { workflow: workflow.to_path_buf(), source, } - })?; - if location.toml.is_none() && !location.graph.is_file() { - return Err(LocalWorkflowPackageError::Resolve { - workflow: workflow.to_path_buf(), - source: fabro_config::Error::WorkflowNotFound(location.graph.display().to_string()), - }); - } - Ok(location) + }) } fn worktree_root( @@ -275,38 +264,6 @@ fn canonicalize(path: &Path) -> Result { }) } -fn canonicalize_location( - location: WorkflowLocation, -) -> Result { - let graph = canonicalize(&location.graph)?; - let toml = location.toml.as_deref().map(canonicalize).transpose()?; - let dir = graph - .parent() - .expect("a canonical workflow graph has a parent") - .to_path_buf(); - Ok(WorkflowLocation { - dir, - graph, - toml, - slug: location.slug, - }) -} - -fn ensure_location_is_within_root( - location: &WorkflowLocation, - source_root: &Path, -) -> Result<(), LocalWorkflowPackageError> { - for path in std::iter::once(&location.graph).chain(location.toml.iter()) { - if !path.starts_with(source_root) { - return Err(LocalWorkflowPackageError::EscapesSourceRoot { - path: path.clone(), - source_root: source_root.to_path_buf(), - }); - } - } - Ok(()) -} - #[cfg(test)] mod tests { #![expect( @@ -317,6 +274,9 @@ mod tests { use std::fs; use std::path::{Path, PathBuf}; + use fabro_types::WorkflowVersion; + use fabro_util::error::collect_chain; + use crate::{LocalWorkflowPackageError, resolve_local_workflow_package}; fn write(root: &Path, path: &str, content: &str) { @@ -349,14 +309,12 @@ mod tests { .collect() } - fn error_chain(error: &dyn std::error::Error) -> String { - let mut messages = vec![error.to_string()]; - let mut source = error.source(); - while let Some(error) = source { - messages.push(error.to_string()); - source = error.source(); - } - messages.join(": ") + fn error_chain(error: &(dyn std::error::Error + 'static)) -> String { + collect_chain(error).join(": ") + } + + fn root_version(package: &crate::ResolvedLocalWorkflowPackage) -> &WorkflowVersion { + package.closure().versions().last().unwrap().1.version() } #[test] @@ -381,15 +339,7 @@ mod tests { .unwrap(), ); assert_eq!( - package - .closure() - .versions() - .last() - .unwrap() - .1 - .version() - .entrypoint() - .as_str(), + root_version(&package).entrypoint().as_str(), ".fabro/workflows/hello/workflow.fabro", ); } @@ -406,15 +356,7 @@ mod tests { resolve_local_workflow_package(Path::new("hello"), &cwd, Some(&user)).unwrap(); assert_eq!(package.source_root(), user.canonicalize().unwrap()); assert_eq!( - package - .closure() - .versions() - .last() - .unwrap() - .1 - .version() - .entrypoint() - .as_str(), + root_version(&package).entrypoint().as_str(), "hello/workflow.fabro", ); @@ -474,13 +416,7 @@ mod tests { assert_eq!(package.source_root(), package_root.canonicalize().unwrap()); assert!( - package - .closure() - .versions() - .last() - .unwrap() - .1 - .version() + root_version(&package) .files() .keys() .any(|path| path.as_str() == "shared.md"), @@ -503,8 +439,12 @@ mod tests { package.source_root(), temp.path().join("loose").canonicalize().unwrap(), ); - let root = package.closure().versions().last().unwrap().1.version(); - assert!(root.files().keys().any(|path| path.as_str() == "prompt.md")); + assert!( + root_version(&package) + .files() + .keys() + .any(|path| path.as_str() == "prompt.md") + ); } #[cfg(unix)] @@ -581,10 +521,8 @@ mod tests { let error = resolve_local_workflow_package(Path::new("hello"), &project, None).unwrap_err(); - assert!(matches!( - error, - LocalWorkflowPackageError::EscapesSourceRoot { .. } - )); + assert!(matches!(error, LocalWorkflowPackageError::Collect { .. })); + assert!(error_chain(&error).contains("escapes source root")); } #[test] diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 2bdd4ac97..e01eb629d 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeSet, HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; -use anyhow::{Context as _, Result, anyhow}; +use anyhow::{Context as _, Result, anyhow, bail}; use fabro_api::types; use fabro_config::project::WorkflowLocation; use fabro_config::{ @@ -16,7 +16,6 @@ use fabro_template::{ }; use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; -use thiserror::Error; use crate::{manifest_path_from_absolute, normalize_absolute_path}; @@ -481,50 +480,28 @@ impl<'a> WorkflowBundler<'a> { return std::fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display())); } - let canonical = - path.canonicalize() - .map_err(|source| PackageFileReadError::Canonicalize { - path: path.to_path_buf(), - source, - })?; + let canonical = path.canonicalize().with_context(|| { + format!( + "failed to canonicalize workflow package file `{}`", + path.display() + ) + })?; if !canonical.starts_with(self.package_root) { - return Err(PackageFileReadError::EscapesSourceRoot { - path: path.to_path_buf(), - source_root: self.package_root.to_path_buf(), - } - .into()); + bail!( + "workflow package file `{}` escapes source root `{}`", + path.display(), + self.package_root.display() + ); } - std::fs::read_to_string(&canonical).map_err(|source| { - PackageFileReadError::Read { - path: canonical, - source, - } - .into() + std::fs::read_to_string(&canonical).with_context(|| { + format!( + "failed to read workflow package file `{}`", + canonical.display() + ) }) } } -#[derive(Debug, Error)] -enum PackageFileReadError { - #[error("failed to canonicalize workflow package file `{path}`")] - Canonicalize { - path: PathBuf, - #[source] - source: std::io::Error, - }, - #[error("workflow package file `{path}` escapes source root `{source_root}`")] - EscapesSourceRoot { - path: PathBuf, - source_root: PathBuf, - }, - #[error("failed to read workflow package file `{path}`")] - Read { - path: PathBuf, - #[source] - source: std::io::Error, - }, -} - struct WorkflowScanInput { absolute_dot_path: PathBuf, dot_path: ManifestPath, diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index bb8535a3f..24a0e5420 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -84,7 +84,19 @@ pub fn collect_workflow_versions( ) -> Result { let repository_workflow = repository_workflow_path(workflow); let location = crate::resolve_existing_workflow_location(&repository_workflow, checkout_root) - .map_err(|source| location_error(workflow, source))?; + .map_err(|source| { + match source { + fabro_config::Error::WorkflowNotFound(_) => { + WorkflowVersionCollectError::WorkflowNotFound { + path: workflow.to_path_buf(), + } + } + source => WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: source.into(), + }, + } + })?; let package_root = checkout_root @@ -96,24 +108,25 @@ pub fn collect_workflow_versions( checkout_root.display() )), })?; - let location = canonicalize_location(location, workflow)?; + let location = canonicalize_location(location, |path, source| { + WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::Error::new(source).context(format!( + "failed to canonicalize workflow path {}", + path.display() + )), + } + })?; collect_workflow_versions_at_location(&location, &package_root, workflow) } -fn canonicalize_location( +/// Canonicalize a resolved workflow location so its paths compare against a +/// canonical package root. `map_err` receives the path that failed. +pub(super) fn canonicalize_location( location: WorkflowLocation, - workflow: &Path, -) -> Result { - let canonicalize = |path: &Path| { - path.canonicalize() - .map_err(|source| WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source: anyhow::Error::new(source).context(format!( - "failed to canonicalize workflow path {}", - path.display() - )), - }) - }; + map_err: impl Fn(&Path, std::io::Error) -> E, +) -> Result { + let canonicalize = |path: &Path| path.canonicalize().map_err(|source| map_err(path, source)); let graph = canonicalize(&location.graph)?; let toml = location.toml.as_deref().map(canonicalize).transpose()?; let dir = graph @@ -153,22 +166,6 @@ fn repository_workflow_path(workflow: &Path) -> PathBuf { } } -fn location_error(workflow: &Path, source: anyhow::Error) -> WorkflowVersionCollectError { - if matches!( - source.downcast_ref::(), - Some(fabro_config::Error::WorkflowNotFound(_)) - ) { - WorkflowVersionCollectError::WorkflowNotFound { - path: workflow.to_path_buf(), - } - } else { - WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source, - } - } -} - struct VersionAssembler { root_key: String, /// Sources still waiting to be assembled; each is removed once visited. diff --git a/lib/components/fabro-workflow/src/workflow_bundle.rs b/lib/components/fabro-workflow/src/workflow_bundle.rs index dddcda492..c6d635af9 100644 --- a/lib/components/fabro-workflow/src/workflow_bundle.rs +++ b/lib/components/fabro-workflow/src/workflow_bundle.rs @@ -61,6 +61,11 @@ impl WorkflowBundle { pub fn workflows(&self) -> &HashMap { &self.workflows } + + #[must_use] + pub fn into_workflows(self) -> HashMap { + self.workflows + } } #[derive(Clone, Debug, Serialize, Deserialize)]