From 786e01c7a744e78d9836bfab31099ddf88406c3e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Apr 2026 12:26:39 -0400 Subject: [PATCH] refactor(manifest): type bundle paths end-to-end Introduce ManifestPath as the canonical in-memory key for run manifests so CLI-produced bundle keys and workflow/server consumers share the same normalization rules. Validate wire keys at the server boundary and add a CLI-to-server round-trip test for user-global @path references. --- lib/crates/fabro-cli/src/lib.rs | 2 + lib/crates/fabro-cli/src/main.rs | 4 + lib/crates/fabro-cli/src/manifest_builder.rs | 117 ++---- .../tests/manifest_path_round_trip.rs | 57 +++ lib/crates/fabro-server/src/lib.rs | 1 + lib/crates/fabro-server/src/run_manifest.rs | 230 ++++++++--- .../fabro-workflow/src/file_resolver.rs | 95 +---- .../src/handler/manager_loop.rs | 22 +- lib/crates/fabro-workflow/src/lib.rs | 2 + .../fabro-workflow/src/manifest_path.rs | 365 ++++++++++++++++++ .../fabro-workflow/src/operations/create.rs | 14 +- .../fabro-workflow/src/operations/source.rs | 4 +- .../fabro-workflow/src/operations/start.rs | 40 +- .../fabro-workflow/src/pipeline/types.rs | 3 +- lib/crates/fabro-workflow/src/services.rs | 6 +- .../fabro-workflow/src/transforms/import.rs | 83 ++-- .../fabro-workflow/src/workflow_bundle.rs | 40 +- 17 files changed, 774 insertions(+), 311 deletions(-) create mode 100644 lib/crates/fabro-cli/tests/manifest_path_round_trip.rs create mode 100644 lib/crates/fabro-workflow/src/manifest_path.rs diff --git a/lib/crates/fabro-cli/src/lib.rs b/lib/crates/fabro-cli/src/lib.rs index efeecfd0a..568dceb3c 100644 --- a/lib/crates/fabro-cli/src/lib.rs +++ b/lib/crates/fabro-cli/src/lib.rs @@ -4,8 +4,10 @@ )] mod args; +mod manifest_builder; use clap::{Command, CommandFactory}; +pub use manifest_builder::{BuiltManifest, ManifestBuildInput, build_run_manifest}; pub fn command_for_reference() -> Command { args::Cli::command() diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 949ab7b90..21df76901 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -10,6 +10,10 @@ mod gh; mod landing; mod local_server; mod logging; +#[allow( + unreachable_pub, + reason = "The library exports manifest builder helpers for tests; the binary includes the same module privately." +)] mod manifest_builder; mod server_client; mod server_runs; diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 0545bef71..015038e19 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -16,12 +16,13 @@ use fabro_graphviz::parser; use fabro_sandbox::daytona::detect_repo_info; use fabro_types::settings::run::{ResolvedGoalSource, ResolvedRunGoal}; use fabro_types::{RunId, WorkflowSettings}; +use fabro_workflow::ManifestPath; use fabro_workflow::git::{GitSyncStatus, head_sha, sync_status}; use crate::args::{PreflightArgs, RunArgs}; #[derive(Debug)] -pub(crate) struct ManifestBuildInput { +pub struct ManifestBuildInput { pub workflow: PathBuf, pub cwd: PathBuf, pub run_overrides: Option, @@ -34,7 +35,7 @@ pub(crate) struct ManifestBuildInput { } #[derive(Debug)] -pub(crate) struct BuiltManifest { +pub struct BuiltManifest { pub manifest: types::RunManifest, pub target_path: PathBuf, } @@ -48,11 +49,11 @@ struct CollectContext<'a> { #[derive(Clone)] struct WorkflowScanInput { absolute_dot_path: PathBuf, - logical_dot_path: PathBuf, + dot_path: ManifestPath, source: String, } -pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result { +pub fn build_run_manifest(input: ManifestBuildInput) -> Result { let root_resolution = resolve_workflow_path(&input.workflow, &input.cwd)?; if root_resolution.workflow_toml_path.is_none() && !root_resolution.resolved_workflow_path.is_file() @@ -91,8 +92,8 @@ pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result Result Result, + from: Option, ) -> Result { let absolute_path = normalize_absolute_path(base_dir, reference) .ok_or_else(|| anyhow!("unsupported manifest reference: {reference}"))?; - let logical_path = to_logical_path(&absolute_path, cwd)?; - let key = logical_path_string(&logical_path); + let path = manifest_path_from_absolute(&absolute_path, cwd)?; + let key = path.to_string(); if !files.contains_key(&key) { let content = std::fs::read_to_string(&absolute_path) .with_context(|| format!("Failed to read {}", absolute_path.display()))?; files.insert(key.clone(), types::ManifestFileEntry { content, ref_: types::ManifestFileRef { - from: from.map(|value| logical_path_string(&value)), + from: from.map(|value| value.to_string()), original: reference.to_string(), type_: ref_type, }, @@ -414,7 +417,7 @@ fn collect_bundled_file( Ok(BundledFile { absolute_path, - logical_path, + path, }) } @@ -528,49 +531,9 @@ fn normalize_absolute_path(base_dir: &Path, reference: &str) -> Option Some(normalized) } -fn to_logical_path(path: &Path, cwd: &Path) -> Result { - if let Ok(stripped) = path.strip_prefix(cwd) { - return Ok(stripped.to_path_buf()); - } - - relative_path_from(path, cwd) - .ok_or_else(|| anyhow!("Failed to compute logical path for {}", path.display())) -} - -fn relative_path_from(path: &Path, base: &Path) -> Option { - let path_components = path.components().collect::>(); - let base_components = base.components().collect::>(); - if path_components.is_empty() || base_components.is_empty() { - return None; - } - - let mut common = 0; - while common < path_components.len() - && common < base_components.len() - && path_components[common] == base_components[common] - { - common += 1; - } - - let mut relative = PathBuf::new(); - for component in &base_components[common..] { - if matches!(component, Component::Normal(_)) { - relative.push(".."); - } - } - for component in &path_components[common..] { - match component { - Component::Normal(part) => relative.push(part), - Component::CurDir => {} - Component::ParentDir => relative.push(".."), - Component::RootDir | Component::Prefix(_) => return None, - } - } - Some(relative) -} - -fn logical_path_string(path: &Path) -> String { - path.to_string_lossy().to_string() +fn manifest_path_from_absolute(path: &Path, cwd: &Path) -> Result { + ManifestPath::from_absolute(path, cwd) + .ok_or_else(|| anyhow!("Failed to compute manifest path for {}", path.display())) } fn manifest_args_is_empty(args: &types::ManifestArgs) -> bool { diff --git a/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs new file mode 100644 index 000000000..12f725e4d --- /dev/null +++ b/lib/crates/fabro-cli/tests/manifest_path_round_trip.rs @@ -0,0 +1,57 @@ +#![expect( + clippy::disallowed_methods, + reason = "Sync temp fixture writes keep this manifest round-trip test simple and isolated." +)] + +use std::path::PathBuf; + +use fabro_cli::{ManifestBuildInput, build_run_manifest}; +use fabro_workflow::ManifestPath; + +#[test] +fn cli_built_manifest_resolves_user_global_at_path() { + let temp = tempfile::tempdir().unwrap(); + let workflow_dir = temp.path().join(".fabro/workflows/demo"); + let project = temp.path().join("project"); + std::fs::create_dir_all(workflow_dir.join("prompts")).unwrap(); + std::fs::create_dir_all(&project).unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r#"digraph Demo { + graph [goal="Demo"] + start [shape=Mdiamond] + prompt [prompt="@prompts/hello.md"] + exit [shape=Msquare] + start -> prompt -> exit + }"#, + ) + .unwrap(); + std::fs::write(workflow_dir.join("prompts/hello.md"), "hello from bundle").unwrap(); + + let built = build_run_manifest(ManifestBuildInput { + workflow: workflow_dir.join("workflow.fabro"), + cwd: project, + run_overrides: None, + cli_overrides: None, + args: None, + run_id: None, + user_settings_path: None, + }) + .unwrap(); + + let bundle = fabro_server::workflow_bundle_from_manifest(&built.manifest.workflows).unwrap(); + let target_path = ManifestPath::from_wire(&built.manifest.target.path).unwrap(); + let workflow = bundle + .workflow(&target_path) + .expect("root workflow should be present"); + let resolved = workflow + .file_resolver() + .resolve(&workflow.current_dir(), "prompts/hello.md") + .expect("prompt should resolve from bundle"); + + assert_eq!(resolved.content, "hello from bundle"); + assert_eq!( + resolved.path, + PathBuf::from("../.fabro/workflows/demo/prompts/hello.md") + ); +} diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index e43bab386..9031427e8 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -38,5 +38,6 @@ pub mod web_auth; mod worker_token; pub use error::{ApiError, Error, Result}; +pub use run_manifest::workflow_bundle_from_manifest; pub use server_secrets::process_env_snapshot; pub use startup::validate_startup; diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index ca2647798..54f8eeeb0 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::{Result, anyhow, bail}; @@ -29,11 +29,11 @@ use fabro_types::settings::run::{ use fabro_types::{RunId, WorkflowSettings}; use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus}; use fabro_validate::Severity; -use fabro_workflow::Error as WorkflowError; use fabro_workflow::operations::{CreateRunInput, ValidateInput, WorkflowInput, validate}; use fabro_workflow::pipeline::Validated; use fabro_workflow::run_materialization::materialize_run; -use fabro_workflow::workflow_bundle::{BundledWorkflow, WorkflowBundle}; +use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; +use fabro_workflow::{Error as WorkflowError, ManifestPath}; use crate::server::AppState; @@ -44,7 +44,7 @@ pub(crate) struct PreparedManifest { pub root_source: String, pub run_id: Option, pub settings: WorkflowSettings, - pub target_path: PathBuf, + pub target_path: ManifestPath, pub workflow_bundle: WorkflowBundle, pub workflow_input: BundledWorkflow, pub working_directory: PathBuf, @@ -70,7 +70,8 @@ pub(crate) fn prepare_manifest( } let cwd = PathBuf::from(&manifest.cwd); - let target_path = PathBuf::from(&manifest.target.path); + let target_path = ManifestPath::from_wire(&manifest.target.path) + .ok_or_else(|| anyhow!("invalid manifest target path: {}", manifest.target.path))?; let workflow_bundle = workflow_bundle_from_manifest(&manifest.workflows)?; let workflow_input = workflow_bundle .workflow(&target_path) @@ -79,7 +80,7 @@ pub(crate) fn prepare_manifest( let root_source = workflow_input.source.clone(); let args_overrides = manifest_args_overrides(manifest.args.as_ref()); - let workflow_run_layer = root_workflow_run_layer(manifest, &workflow_input)?; + let workflow_run_layer = root_workflow_run_layer(&workflow_input)?; let mut workflow_settings_builder = WorkflowSettingsBuilder::new().server_run_defaults(manifest_run_defaults.clone()); if let Some(run) = args_overrides.run { @@ -178,7 +179,12 @@ pub(crate) async fn run_preflight( let (report, checks_ok) = build_preflight_report(state, prepared, validated).await?; let preflight_ok = !validated.has_errors() && checks_ok; Ok(( - preflight_response(validated, &prepared.target_path, &report, preflight_ok), + preflight_response( + validated, + prepared.target_path.as_path(), + &report, + preflight_ok, + ), preflight_ok, )) } @@ -190,35 +196,77 @@ pub(crate) fn graph_source(prepared: &PreparedManifest, direction: Option<&str>) ) } -fn workflow_bundle_from_manifest( +pub fn workflow_bundle_from_manifest( workflows: &HashMap, ) -> Result { - let workflows = workflows - .iter() - .map(|(path, workflow)| { - let files = workflow - .files - .iter() - .map(|(key, entry)| (PathBuf::from(key), entry.content.clone())) - .collect::>(); - Ok::<_, anyhow::Error>((PathBuf::from(path), BundledWorkflow { - logical_path: PathBuf::from(path), - source: workflow.source.clone(), - files, - })) - }) - .collect::>>()?; - Ok(WorkflowBundle::new(workflows)) + let mut bundled = HashMap::new(); + let mut workflow_wire_keys = HashMap::new(); + + for (wire_key, workflow) in workflows { + let path = ManifestPath::from_wire(wire_key) + .ok_or_else(|| anyhow!("invalid manifest workflow key: {wire_key}"))?; + if let Some(previous) = workflow_wire_keys.get(&path) { + bail!( + "duplicate canonical workflow key: {path} (from wire keys {previous:?} and \ + {wire_key:?})" + ); + } + workflow_wire_keys.insert(path.clone(), wire_key.clone()); + + let files = workflow_files_from_manifest(&workflow.files)?; + let config = workflow + .config + .as_ref() + .map(|config| { + let path = ManifestPath::from_wire(&config.path).ok_or_else(|| { + anyhow!("invalid manifest workflow config path: {}", config.path) + })?; + Ok::<_, anyhow::Error>(ParsedWorkflowConfig { + path, + source: config.source.clone(), + }) + }) + .transpose()?; + + bundled.insert(path.clone(), BundledWorkflow { + path, + source: workflow.source.clone(), + config, + files, + }); + } + + Ok(WorkflowBundle::new(bundled)) } -fn root_workflow_run_layer( - manifest: &types::RunManifest, - workflow: &BundledWorkflow, -) -> Result { - let Some(root) = manifest.workflows.get(&manifest.target.path) else { - bail!("manifest target path is missing from workflows map"); - }; - let Some(config) = root.config.as_ref() else { +fn workflow_files_from_manifest( + files: &HashMap, +) -> Result> { + let mut bundled = HashMap::new(); + let mut file_wire_keys = HashMap::new(); + + for (wire_key, entry) in files { + let path = ManifestPath::from_wire(wire_key) + .ok_or_else(|| anyhow!("invalid manifest file key: {wire_key}"))?; + if let Some(previous) = file_wire_keys.get(&path) { + bail!( + "duplicate canonical file key: {path} (from wire keys {previous:?} and \ + {wire_key:?})" + ); + } + if let Some(from) = entry.ref_.from.as_deref() { + ManifestPath::from_wire(from) + .ok_or_else(|| anyhow!("invalid manifest file ref from: {from}"))?; + } + file_wire_keys.insert(path.clone(), wire_key.clone()); + bundled.insert(path, entry.content.clone()); + } + + Ok(bundled) +} + +fn root_workflow_run_layer(workflow: &BundledWorkflow) -> Result { + let Some(config) = workflow.config.as_ref() else { return Ok(RunLayer::default()); }; @@ -232,7 +280,7 @@ fn root_workflow_run_layer( .transpose() .map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))? .unwrap_or_default(); - resolve_manifest_dockerfile(&mut run, Path::new(&config.path), &workflow.files)?; + resolve_manifest_dockerfile(&mut run, &config.path, &workflow.files)?; Ok(run) } @@ -321,8 +369,8 @@ fn resolve_working_directory(settings: &WorkflowSettings, caller_cwd: &Path) -> fn resolve_manifest_dockerfile( run: &mut RunLayer, - config_path: &Path, - files: &HashMap, + config_path: &ManifestPath, + files: &HashMap, ) -> Result<()> { let source = run .sandbox @@ -337,43 +385,19 @@ fn resolve_manifest_dockerfile( return Ok(()); }; let path_owned = path.clone(); - let logical_path = normalize_logical_path( + let manifest_path = ManifestPath::from_reference( config_path.parent().unwrap_or_else(|| Path::new(".")), &path_owned, ) .ok_or_else(|| anyhow!("unsupported dockerfile reference: {path_owned}"))?; let content = files - .get(&logical_path) + .get(&manifest_path) .cloned() - .ok_or_else(|| anyhow!("missing bundled dockerfile: {}", logical_path.display()))?; + .ok_or_else(|| anyhow!("missing bundled dockerfile: {manifest_path}"))?; *source = DaytonaDockerfileLayer::Inline(content); Ok(()) } -fn normalize_logical_path(current_dir: &Path, reference: &str) -> Option { - let path = Path::new(reference); - if path.is_absolute() || reference.starts_with('~') { - return None; - } - - let mut normalized = PathBuf::new(); - for component in current_dir.join(path).components() { - match component { - Component::CurDir => {} - Component::Normal(part) => normalized.push(part), - Component::ParentDir => { - if normalized.file_name().is_some() { - normalized.pop(); - } else { - normalized.push(".."); - } - } - Component::RootDir | Component::Prefix(_) => return None, - } - } - Some(normalized) -} - async fn build_preflight_report( state: &AppState, prepared: &PreparedManifest, @@ -991,6 +1015,88 @@ mod tests { RunLayer::default() } + fn manifest_workflow() -> types::ManifestWorkflow { + types::ManifestWorkflow { + config: None, + files: HashMap::new(), + source: "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" + .to_string(), + } + } + + fn manifest_file(content: &str) -> types::ManifestFileEntry { + types::ManifestFileEntry { + content: content.to_string(), + ref_: types::ManifestFileRef { + from: Some("workflow.fabro".to_string()), + original: "prompt.md".to_string(), + type_: types::ManifestFileRefType::FileInline, + }, + } + } + + #[test] + fn workflow_bundle_rejects_duplicate_canonical_workflow_keys() { + let workflows = HashMap::from([ + ("bar.fabro".to_string(), manifest_workflow()), + ("./foo/../bar.fabro".to_string(), manifest_workflow()), + ]); + + let error = workflow_bundle_from_manifest(&workflows).unwrap_err(); + + assert!( + error + .to_string() + .contains("duplicate canonical workflow key: bar.fabro") + ); + } + + #[test] + fn workflow_bundle_rejects_duplicate_canonical_file_keys() { + let mut workflow = manifest_workflow(); + workflow.files = HashMap::from([ + ("prompts/hello.md".to_string(), manifest_file("first")), + ("./prompts/./hello.md".to_string(), manifest_file("second")), + ]); + let workflows = HashMap::from([("workflow.fabro".to_string(), workflow)]); + + let error = workflow_bundle_from_manifest(&workflows).unwrap_err(); + + assert!( + error + .to_string() + .contains("duplicate canonical file key: prompts/hello.md") + ); + } + + #[test] + fn workflow_bundle_rejects_invalid_workflow_key() { + let workflows = HashMap::from([("/abs/path.fabro".to_string(), manifest_workflow())]); + + let error = workflow_bundle_from_manifest(&workflows).unwrap_err(); + + assert!( + error + .to_string() + .contains("invalid manifest workflow key: /abs/path.fabro") + ); + } + + #[test] + fn workflow_bundle_rejects_invalid_file_key() { + let mut workflow = manifest_workflow(); + workflow.files = HashMap::from([("~/foo.md".to_string(), manifest_file("content"))]); + let workflows = HashMap::from([("workflow.fabro".to_string(), workflow)]); + + let error = workflow_bundle_from_manifest(&workflows).unwrap_err(); + + assert!( + error + .to_string() + .contains("invalid manifest file key: ~/foo.md") + ); + } + #[test] fn prepare_manifest_preserves_explicit_manifest_dry_run() { let server_settings = manifest_run_defaults(Some(&server_settings_fixture( diff --git a/lib/crates/fabro-workflow/src/file_resolver.rs b/lib/crates/fabro-workflow/src/file_resolver.rs index aa8e44fc7..62bcbb831 100644 --- a/lib/crates/fabro-workflow/src/file_resolver.rs +++ b/lib/crates/fabro-workflow/src/file_resolver.rs @@ -4,7 +4,9 @@ )] use std::collections::HashMap; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; + +use crate::ManifestPath; pub trait FileResolver: Send + Sync { fn resolve(&self, current_dir: &Path, reference: &str) -> Option; @@ -12,61 +14,32 @@ pub trait FileResolver: Send + Sync { #[derive(Clone, Debug, PartialEq, Eq)] pub struct ResolvedFile { - pub logical_path: PathBuf, - pub content: String, + pub path: PathBuf, + pub content: String, } #[derive(Clone, Debug, Default)] pub struct BundleFileResolver { - files: HashMap, + files: HashMap, } impl BundleFileResolver { #[must_use] - pub fn new(files: HashMap) -> Self { + pub fn new(files: HashMap) -> Self { Self { files } } } impl FileResolver for BundleFileResolver { fn resolve(&self, current_dir: &Path, reference: &str) -> Option { - let logical_path = normalize_logical_path(current_dir, reference)?; - self.files.get(&logical_path).map(|content| ResolvedFile { - logical_path, + let path = ManifestPath::from_reference(current_dir, reference)?; + self.files.get(&path).map(|content| ResolvedFile { + path: path.as_path().to_path_buf(), content: content.clone(), }) } } -pub(crate) fn normalize_logical_path(current_dir: &Path, reference: &str) -> Option { - let path = Path::new(reference); - if path.is_absolute() || reference.starts_with('~') { - return None; - } - - let mut normalized = PathBuf::new(); - for component in current_dir.join(path).components() { - match component { - Component::CurDir => {} - Component::Normal(part) => normalized.push(part), - Component::ParentDir => { - // If the last component is a normal name, collapse it. - // Otherwise (empty path or trailing `..`), preserve the - // `..` so that leading parent-dir segments survive — these - // occur when a bundled workflow lives outside the CWD - // (e.g. user-global workflows under ~/.fabro/). - if normalized.file_name().is_some() { - normalized.pop(); - } else { - normalized.push(".."); - } - } - Component::RootDir | Component::Prefix(_) => return None, - } - } - Some(normalized) -} - #[derive(Clone, Debug, Default)] pub struct FilesystemFileResolver { fallback_dir: Option, @@ -106,7 +79,7 @@ impl FileResolver for FilesystemFileResolver { match std::fs::read_to_string(&resolved_path) { Ok(content) => Some(ResolvedFile { - logical_path: resolved_path, + path: resolved_path, content, }), Err(error) => { @@ -125,10 +98,14 @@ impl FileResolver for FilesystemFileResolver { mod tests { use super::*; + fn manifest_path(value: &str) -> ManifestPath { + ManifestPath::from_wire(value).expect("path should parse") + } + #[test] fn bundle_resolver_returns_exact_match() { let resolver = BundleFileResolver::new(HashMap::from([( - PathBuf::from("prompts/review.md"), + manifest_path("prompts/review.md"), "check it".to_string(), )])); @@ -136,14 +113,14 @@ mod tests { .resolve(Path::new("."), "prompts/review.md") .expect("file should resolve"); - assert_eq!(resolved.logical_path, PathBuf::from("prompts/review.md")); + assert_eq!(resolved.path, PathBuf::from("prompts/review.md")); assert_eq!(resolved.content, "check it"); } #[test] fn bundle_resolver_normalizes_relative_segments() { let resolver = BundleFileResolver::new(HashMap::from([( - PathBuf::from("prompts/review.md"), + manifest_path("prompts/review.md"), "check it".to_string(), )])); @@ -151,7 +128,7 @@ mod tests { .resolve(Path::new("subflows"), "../prompts/review.md") .expect("file should resolve"); - assert_eq!(resolved.logical_path, PathBuf::from("prompts/review.md")); + assert_eq!(resolved.path, PathBuf::from("prompts/review.md")); } #[test] @@ -160,42 +137,10 @@ mod tests { assert!(resolver.resolve(Path::new("."), "missing.md").is_none()); } - #[test] - fn normalize_preserves_leading_parent_dir() { - // When a bundled workflow lives outside the CWD (e.g. ~/.fabro/), - // the manifest builder produces logical paths with leading `..` - // segments. The normalizer must preserve these so that the lookup - // key matches the stored key. - assert_eq!( - normalize_logical_path(Path::new("../.fabro/workflows/demo"), "prompts/hello.md"), - Some(PathBuf::from("../.fabro/workflows/demo/prompts/hello.md")) - ); - } - - #[test] - fn normalize_preserves_multiple_leading_parent_dirs() { - assert_eq!( - normalize_logical_path(Path::new("../../shared/workflows"), "file.md"), - Some(PathBuf::from("../../shared/workflows/file.md")) - ); - } - - #[test] - fn normalize_collapses_mid_path_parent_dir() { - // ../foo/../bar should normalize to ../bar - assert_eq!( - normalize_logical_path(Path::new("../foo"), "../bar/file.md"), - Some(PathBuf::from("../bar/file.md")) - ); - } - #[test] fn bundle_resolver_resolves_outside_cwd_paths() { - // Reproduces the user-global workflow scenario from issue #175: - // files are keyed with leading `..` because the workflow lives - // outside the CWD. let resolver = BundleFileResolver::new(HashMap::from([( - PathBuf::from("../.fabro/workflows/demo/prompts/hello.md"), + manifest_path("../.fabro/workflows/demo/prompts/hello.md"), "prompt content".to_string(), )])); diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index 95463fb34..cc471f77a 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -19,10 +19,10 @@ use crate::context::{Context, WorkflowContext, keys}; use crate::error::Error; use crate::operations::{ValidateInput, WorkflowInput, validate}; use crate::outcome::{Outcome, OutcomeExt, StageStatus}; -use crate::pipeline; use crate::pipeline::types::Initialized; use crate::run_dir::visit_from_context; use crate::run_options::RunOptions; +use crate::{ManifestPath, pipeline}; /// Orchestrates a child workflow engine, polling for completion or stop /// conditions. @@ -30,7 +30,7 @@ pub struct SubWorkflowHandler; struct ParsedChildWorkflow { graph: Graph, - workflow_path: Option, + workflow_path: Option, } /// Parse a duration string like "45s", "200ms", "5m" into a Duration. @@ -109,9 +109,8 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result WorkflowInput::Path(PathBuf::from(path)), }; let workflow_path = match &workflow { - WorkflowInput::Bundled(workflow) => Some(workflow.logical_path.clone()), - WorkflowInput::Path(path) => Some(path.clone()), - WorkflowInput::DotSource { .. } => None, + WorkflowInput::Bundled(workflow) => Some(workflow.path.clone()), + WorkflowInput::Path(_) | WorkflowInput::DotSource { .. } => None, }; let validated = validate(ValidateInput { workflow, @@ -591,13 +590,14 @@ mod tests { ); let mut services = make_services(); - services.workflow_path = Some(PathBuf::from("workflow.fabro")); + services.workflow_path = Some(ManifestPath::from_wire("workflow.fabro").unwrap()); services.workflow_bundle = Some(Arc::new(WorkflowBundle::new(HashMap::from([( - PathBuf::from("children/review.fabro"), + ManifestPath::from_wire("children/review.fabro").unwrap(), BundledWorkflow { - logical_path: PathBuf::from("children/review.fabro"), - source: child_dot_succeeds().to_string(), - files: HashMap::new(), + path: ManifestPath::from_wire("children/review.fabro").unwrap(), + source: child_dot_succeeds().to_string(), + config: None, + files: HashMap::new(), }, )])))); @@ -632,7 +632,7 @@ mod tests { ); let mut services = make_services(); - services.workflow_path = Some(PathBuf::from("workflow.fabro")); + services.workflow_path = Some(ManifestPath::from_wire("workflow.fabro").unwrap()); services.workflow_bundle = Some(Arc::new(WorkflowBundle::new(HashMap::new()))); let context = Context::new(); diff --git a/lib/crates/fabro-workflow/src/lib.rs b/lib/crates/fabro-workflow/src/lib.rs index 77483e746..2347c1a49 100644 --- a/lib/crates/fabro-workflow/src/lib.rs +++ b/lib/crates/fabro-workflow/src/lib.rs @@ -132,6 +132,7 @@ mod hook_context; reason = "The lifecycle module remains crate-visible for tests and pending integrations." )] pub(crate) mod lifecycle; +mod manifest_path; pub(crate) mod node_handler; pub mod operations; pub mod outcome; @@ -145,6 +146,7 @@ pub mod run_dump; pub mod run_lookup; pub use error::{Error, FailureCategory, FailureSignature, FailureSignatureExt, Result}; +pub use manifest_path::ManifestPath; pub mod run_materialization; pub mod run_options; pub mod run_status; diff --git a/lib/crates/fabro-workflow/src/manifest_path.rs b/lib/crates/fabro-workflow/src/manifest_path.rs new file mode 100644 index 000000000..61108cf0b --- /dev/null +++ b/lib/crates/fabro-workflow/src/manifest_path.rs @@ -0,0 +1,365 @@ +use std::fmt; +use std::path::{Component, Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +/// A path used as a key inside a run manifest. It is anchored at the run's +/// cwd, and may contain leading `..` segments for files outside that cwd. +#[derive(Clone, Debug, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(into = "String", try_from = "String")] +pub struct ManifestPath(PathBuf); + +impl ManifestPath { + #[must_use] + pub fn from_reference(current_dir: &Path, reference: &str) -> Option { + let path = Path::new(reference); + if path.is_absolute() || reference.starts_with('~') { + return None; + } + + normalize_components(current_dir.join(path)).map(Self) + } + + #[must_use] + pub fn from_absolute(absolute: &Path, cwd: &Path) -> Option { + if let Ok(stripped) = absolute.strip_prefix(cwd) { + return normalize_components(stripped).map(Self); + } + + relative_path_from(absolute, cwd).and_then(|path| normalize_components(path).map(Self)) + } + + #[must_use] + pub fn from_wire(value: &str) -> Option { + Self::from_reference(Path::new("."), value) + } + + #[must_use] + pub fn as_path(&self) -> &Path { + &self.0 + } + + #[must_use] + pub fn parent(&self) -> Option<&Path> { + self.0.parent() + } +} + +impl TryFrom for ManifestPath { + type Error = ManifestPathParseError; + + fn try_from(value: String) -> Result { + Self::from_wire(&value).ok_or(ManifestPathParseError(value)) + } +} + +impl From for String { + fn from(value: ManifestPath) -> Self { + value.to_string() + } +} + +impl fmt::Display for ManifestPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0.display()) + } +} + +#[derive(Debug)] +pub struct ManifestPathParseError(String); + +impl fmt::Display for ManifestPathParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "invalid ManifestPath: {}", self.0) + } +} + +impl std::error::Error for ManifestPathParseError {} + +fn normalize_components(path: impl AsRef) -> Option { + let mut normalized = PathBuf::new(); + for component in path.as_ref().components() { + match component { + Component::CurDir => {} + Component::Normal(part) => normalized.push(part), + Component::ParentDir => { + if normalized.file_name().is_some() { + normalized.pop(); + } else { + normalized.push(".."); + } + } + Component::RootDir | Component::Prefix(_) => return None, + } + } + Some(normalized) +} + +fn relative_path_from(path: &Path, base: &Path) -> Option { + let path_components = path.components().collect::>(); + let base_components = base.components().collect::>(); + if path_components.is_empty() || base_components.is_empty() { + return None; + } + + let mut common = 0; + while common < path_components.len() + && common < base_components.len() + && path_components[common] == base_components[common] + { + common += 1; + } + + let mut relative = PathBuf::new(); + for component in &base_components[common..] { + if matches!(component, Component::Normal(_)) { + relative.push(".."); + } + } + for component in &path_components[common..] { + match component { + Component::Normal(part) => relative.push(part), + Component::CurDir => {} + Component::ParentDir => relative.push(".."), + Component::RootDir | Component::Prefix(_) => return None, + } + } + Some(relative) +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::*; + + fn manifest_path(value: &str) -> ManifestPath { + ManifestPath::from_wire(value).expect("path should parse") + } + + #[test] + fn from_reference_rejects_absolute_path() { + assert!(ManifestPath::from_reference(Path::new("."), "/tmp/workflow.fabro").is_none()); + } + + #[test] + fn from_reference_rejects_tilde_reference() { + assert!(ManifestPath::from_reference(Path::new("."), "~/.fabro/workflow.fabro").is_none()); + } + + #[test] + fn from_reference_simple_relative() { + let path = ManifestPath::from_reference(Path::new("flows"), "workflow.fabro").unwrap(); + + assert_eq!(path.as_path(), Path::new("flows/workflow.fabro")); + } + + #[test] + fn from_reference_collapses_curdir_segments() { + let path = ManifestPath::from_reference(Path::new("./flows"), "./workflow.fabro").unwrap(); + + assert_eq!(path.as_path(), Path::new("flows/workflow.fabro")); + } + + #[test] + fn from_reference_collapses_mid_path_parent_dir() { + let path = ManifestPath::from_reference(Path::new("../foo"), "../bar/file.md").unwrap(); + + assert_eq!(path.as_path(), Path::new("../bar/file.md")); + } + + #[test] + fn from_reference_preserves_single_leading_parent_dir() { + let path = + ManifestPath::from_reference(Path::new("../.fabro/workflows/demo"), "prompts/hello.md") + .unwrap(); + + assert_eq!( + path.as_path(), + Path::new("../.fabro/workflows/demo/prompts/hello.md") + ); + } + + #[test] + fn from_reference_preserves_multiple_leading_parent_dirs() { + let path = + ManifestPath::from_reference(Path::new("../../shared/workflows"), "file.md").unwrap(); + + assert_eq!(path.as_path(), Path::new("../../shared/workflows/file.md")); + } + + #[test] + fn from_reference_collapses_then_escapes() { + let path = ManifestPath::from_reference(Path::new("."), "foo/../..").unwrap(); + + assert_eq!(path.as_path(), Path::new("..")); + } + + #[test] + fn from_absolute_file_inside_cwd_strips_prefix() { + let path = ManifestPath::from_absolute( + Path::new("/repo/project/workflow.fabro"), + Path::new("/repo/project"), + ) + .unwrap(); + + assert_eq!(path.as_path(), Path::new("workflow.fabro")); + } + + #[test] + fn from_absolute_sibling_directory_uses_single_parent_dir() { + let path = ManifestPath::from_absolute( + Path::new("/repo/shared/workflow.fabro"), + Path::new("/repo/project"), + ) + .unwrap(); + + assert_eq!(path.as_path(), Path::new("../shared/workflow.fabro")); + } + + #[test] + fn from_absolute_grandparent_uses_two_parent_dirs() { + let path = ManifestPath::from_absolute( + Path::new("/repo/shared/workflow.fabro"), + Path::new("/repo/project/nested"), + ) + .unwrap(); + + assert_eq!(path.as_path(), Path::new("../../shared/workflow.fabro")); + } + + #[test] + fn from_absolute_user_global_workflow_from_unrelated_cwd() { + let path = ManifestPath::from_absolute( + Path::new("/tmp/.fabro/workflows/demo/workflow.fabro"), + Path::new("/tmp/project"), + ) + .unwrap(); + + assert_eq!( + path.as_path(), + Path::new("../.fabro/workflows/demo/workflow.fabro") + ); + } + + #[test] + fn from_wire_accepts_canonical_relative() { + let path = ManifestPath::from_wire("workflow.fabro").unwrap(); + + assert_eq!(path.as_path(), Path::new("workflow.fabro")); + } + + #[test] + fn from_wire_rejects_absolute_path() { + assert!(ManifestPath::from_wire("/tmp/workflow.fabro").is_none()); + } + + #[test] + fn from_wire_rejects_tilde_path() { + assert!(ManifestPath::from_wire("~/.fabro/workflow.fabro").is_none()); + } + + #[test] + fn from_wire_renormalizes_uncollapsed_curdir() { + let path = ManifestPath::from_wire("./foo/./bar").unwrap(); + + assert_eq!(path.as_path(), Path::new("foo/bar")); + } + + #[test] + fn from_wire_renormalizes_mid_path_parent_dir() { + let path = ManifestPath::from_wire("foo/../bar").unwrap(); + + assert_eq!(path.as_path(), Path::new("bar")); + } + + #[test] + fn from_wire_preserves_leading_parent_dir() { + let path = ManifestPath::from_wire("../.fabro/workflows/demo/workflow.fabro").unwrap(); + + assert_eq!( + path.as_path(), + Path::new("../.fabro/workflows/demo/workflow.fabro") + ); + } + + #[test] + fn deserialize_rejects_absolute_path() { + assert!(serde_json::from_str::("\"/abs\"").is_err()); + } + + #[test] + fn deserialize_rejects_tilde_path() { + assert!(serde_json::from_str::("\"~/.fabro/x\"").is_err()); + } + + #[test] + fn deserialize_renormalizes_non_canonical() { + let path: ManifestPath = serde_json::from_str("\"foo/../bar\"").unwrap(); + + assert_eq!(path, manifest_path("bar")); + } + + #[test] + fn round_trip_file_inside_cwd() { + let produced = ManifestPath::from_absolute( + Path::new("/repo/project/prompts/hello.md"), + Path::new("/repo/project"), + ) + .unwrap(); + let consumed = ManifestPath::from_reference(Path::new("prompts"), "hello.md").unwrap(); + + assert_eq!(produced, consumed); + } + + #[test] + fn round_trip_user_global_workflow() { + let produced = ManifestPath::from_absolute( + Path::new("/tmp/.fabro/workflows/demo/prompts/hello.md"), + Path::new("/tmp/project"), + ) + .unwrap(); + let workflow = ManifestPath::from_absolute( + Path::new("/tmp/.fabro/workflows/demo/workflow.fabro"), + Path::new("/tmp/project"), + ) + .unwrap(); + let consumed = + ManifestPath::from_reference(workflow.parent().unwrap(), "prompts/hello.md").unwrap(); + + assert_eq!(produced, consumed); + } + + #[test] + fn round_trip_subworkflow_relative_reference() { + let produced = ManifestPath::from_absolute( + Path::new("/repo/.fabro/workflows/child/workflow.fabro"), + Path::new("/repo"), + ) + .unwrap(); + let root = ManifestPath::from_absolute( + Path::new("/repo/.fabro/workflows/demo/workflow.fabro"), + Path::new("/repo"), + ) + .unwrap(); + let consumed = + ManifestPath::from_reference(root.parent().unwrap(), "../child/workflow.fabro") + .unwrap(); + + assert_eq!(produced, consumed); + } + + #[test] + fn serializes_as_plain_string() { + let serialized = serde_json::to_string(&manifest_path("../workflow.fabro")).unwrap(); + + assert_eq!(serialized, "\"../workflow.fabro\""); + } + + #[test] + fn deserializes_from_plain_string() { + let path: ManifestPath = serde_json::from_str("\"workflow.fabro\"").unwrap(); + + assert_eq!(path.as_path(), Path::new("workflow.fabro")); + } +} diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 4d75e0961..53b1663cd 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -21,6 +21,7 @@ use fabro_util::json::normalize_json_value; use tokio::task::spawn_blocking; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; +use crate::ManifestPath; use crate::error::Error; use crate::event::{Event, append_event, to_run_event_at}; use crate::file_resolver::FileResolver; @@ -38,7 +39,7 @@ pub struct CreateRunInput { pub settings: WorkflowSettings, pub cwd: PathBuf, pub workflow_slug: Option, - pub workflow_path: Option, + pub workflow_path: Option, pub workflow_bundle: Option, pub submitted_manifest_bytes: Option>, pub run_id: Option, @@ -657,8 +658,8 @@ mod tests { fn validate_from_bundle_resolves_nested_import_files_relative_to_imported_graph() { let validated = validate(ValidateInput { workflow: WorkflowInput::Bundled(BundledWorkflow { - logical_path: PathBuf::from("workflow.fabro"), - source: r#"digraph Test { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Test { graph [goal="Ship"] start [shape=Mdiamond] validate [import="./child/validate.fabro"] @@ -666,9 +667,10 @@ mod tests { start -> validate -> exit }"# .to_string(), - files: HashMap::from([ + config: None, + files: HashMap::from([ ( - PathBuf::from("child/validate.fabro"), + ManifestPath::from_wire("child/validate.fabro").unwrap(), r#"digraph Validate { start [shape=Mdiamond] lint [prompt="@../prompts/lint.md"] @@ -678,7 +680,7 @@ mod tests { .to_string(), ), ( - PathBuf::from("prompts/lint.md"), + ManifestPath::from_wire("prompts/lint.md").unwrap(), "Lint {{ goal }}".to_string(), ), ]), diff --git a/lib/crates/fabro-workflow/src/operations/source.rs b/lib/crates/fabro-workflow/src/operations/source.rs index 051818597..a0fd3b055 100644 --- a/lib/crates/fabro-workflow/src/operations/source.rs +++ b/lib/crates/fabro-workflow/src/operations/source.rs @@ -120,9 +120,9 @@ pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result< Ok(ResolvedWorkflow { raw_source: workflow.source.clone(), settings, - workflow_slug: workflow_slug_from_path(&workflow.logical_path), + workflow_slug: workflow_slug_from_path(workflow.path.as_path()), workflow_toml_path: None, - dot_path: Some(workflow.logical_path.clone()), + dot_path: Some(workflow.path.as_path().to_path_buf()), current_dir: Some(workflow.current_dir()), file_resolver: Some(workflow.file_resolver()), goal_override, diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 02f3f0947..c266f7fb6 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -31,6 +31,7 @@ use fabro_vault::Vault; use tokio::runtime::Handle; use tokio::sync::RwLock as AsyncRwLock; +use crate::ManifestPath; use crate::artifact_upload::ArtifactSink; use crate::context::Context; use crate::error::Error; @@ -77,7 +78,7 @@ struct RunSession { pr_github_app: Option, pr_origin_url: Option, pr_model: String, - workflow_path: Option, + workflow_path: Option, workflow_bundle: Option>, run_control: Option>, vault: Option>>, @@ -981,6 +982,7 @@ mod tests { use object_store::memory::InMemory; use super::*; + use crate::ManifestPath; use crate::context::Context; use crate::event::{Emitter, EventBody}; use crate::handler::HandlerRegistry; @@ -1175,9 +1177,11 @@ mod tests { let registry = Arc::new(test_registry()); let store = memory_store(); let workflow_bundle = WorkflowBundle::new(HashMap::from([ - (PathBuf::from("workflow.fabro"), BundledWorkflow { - logical_path: PathBuf::from("workflow.fabro"), - source: r#"digraph Root { + ( + ManifestPath::from_wire("workflow.fabro").unwrap(), + BundledWorkflow { + path: ManifestPath::from_wire("workflow.fabro").unwrap(), + source: r#"digraph Root { graph [goal="Bundle child"] start [shape=Mdiamond] manager [ @@ -1189,19 +1193,25 @@ mod tests { exit [shape=Msquare] start -> manager -> exit }"# - .to_string(), - files: HashMap::new(), - }), - (PathBuf::from("children/review.fabro"), BundledWorkflow { - logical_path: PathBuf::from("children/review.fabro"), - source: r"digraph Review { + .to_string(), + config: None, + files: HashMap::new(), + }, + ), + ( + ManifestPath::from_wire("children/review.fabro").unwrap(), + BundledWorkflow { + path: ManifestPath::from_wire("children/review.fabro").unwrap(), + source: r"digraph Review { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - .to_string(), - files: HashMap::new(), - }), + .to_string(), + config: None, + files: HashMap::new(), + }, + ), ])); crate::operations::create( @@ -1209,7 +1219,7 @@ mod tests { crate::operations::CreateRunInput { workflow: crate::operations::WorkflowInput::Bundled( workflow_bundle - .workflow(Path::new("workflow.fabro")) + .workflow(&ManifestPath::from_wire("workflow.fabro").unwrap()) .unwrap() .clone(), ), @@ -1222,7 +1232,7 @@ mod tests { }), cwd: temp.path().to_path_buf(), workflow_slug: Some("bundle-child".to_string()), - workflow_path: Some(PathBuf::from("workflow.fabro")), + workflow_path: Some(ManifestPath::from_wire("workflow.fabro").unwrap()), workflow_bundle: Some(workflow_bundle), submitted_manifest_bytes: None, run_id: Some(fixtures::RUN_1), diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index defa987f9..ca5da4f79 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -16,6 +16,7 @@ use fabro_validate::{Diagnostic, Severity}; use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; +use crate::ManifestPath; use crate::artifact_upload::ArtifactSink; use crate::context::Context; use crate::error::Error; @@ -241,7 +242,7 @@ pub struct InitOptions { pub interviewer: Arc, pub lifecycle: LifecycleOptions, pub run_options: RunOptions, - pub workflow_path: Option, + pub workflow_path: Option, pub workflow_bundle: Option>, pub hooks: fabro_hooks::HookSettings, pub sandbox_env: SandboxEnvSpec, diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index 90c968935..de720d6b2 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +#[cfg(test)] use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -13,6 +14,7 @@ use fabro_model::Provider; use tokio::time; use tokio_util::sync::CancellationToken; +use crate::ManifestPath; use crate::event::Emitter; use crate::handler::HandlerRegistry; use crate::runtime_store::RunStoreHandle; @@ -121,8 +123,8 @@ pub struct EngineServices { /// When true, handlers should skip real execution and return simulated /// results. pub dry_run: bool, - /// Logical path of the current workflow when running from a bundle. - pub workflow_path: Option, + /// Manifest path of the current workflow when running from a bundle. + pub workflow_path: Option, /// Bundled workflows available for child-workflow resolution. pub workflow_bundle: Option>, } diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 274062c32..2958d67d9 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -110,10 +110,10 @@ impl ImportTransform { return Ok(()); }; - if import_stack.contains(&resolved_file.logical_path) { + if import_stack.contains(&resolved_file.path) { let cycle = import_stack .iter() - .chain(std::iter::once(&resolved_file.logical_path)) + .chain(std::iter::once(&resolved_file.path)) .map(|path| path.display().to_string()) .collect::>() .join(" -> "); @@ -137,7 +137,7 @@ impl ImportTransform { if let Err(message) = Self::splice_import( graph, placeholder_id, - &resolved_file.logical_path, + &resolved_file.path, &placeholder, prepared, ) { @@ -152,52 +152,47 @@ impl ImportTransform { resolved_file: &ResolvedFile, import_stack: &mut Vec, ) -> Result { - Self::with_import_stack( - import_stack, - resolved_file.logical_path.clone(), - |import_stack| { - let rendered_source = render_template( - &resolved_file.content, - &TemplateContext::new() - .with_goal("{{ goal }}") - .with_inputs(self.inputs.clone()), - ) - .map_err(|error| ImportPrepareError::Hard(Error::Validation(error.to_string())))?; + Self::with_import_stack(import_stack, resolved_file.path.clone(), |import_stack| { + let rendered_source = render_template( + &resolved_file.content, + &TemplateContext::new() + .with_goal("{{ goal }}") + .with_inputs(self.inputs.clone()), + ) + .map_err(|error| ImportPrepareError::Hard(Error::Validation(error.to_string())))?; - let mut graph = parser::parse(&rendered_source).map_err(|error| { - ImportPrepareError::Soft(format!( - "failed to parse {}: {error}", - resolved_file.logical_path.display() - )) - })?; + let mut graph = parser::parse(&rendered_source).map_err(|error| { + ImportPrepareError::Soft(format!( + "failed to parse {}: {error}", + resolved_file.path.display() + )) + })?; - let import_base_dir = resolved_file - .logical_path - .parent() - .map_or_else(|| PathBuf::from("."), Path::to_path_buf); - graph = - FileInliningTransform::new(import_base_dir.clone(), Arc::clone(&self.resolver)) - .apply(graph) - .map_err(ImportPrepareError::Hard)?; + let import_base_dir = resolved_file + .path + .parent() + .map_or_else(|| PathBuf::from("."), Path::to_path_buf); + graph = FileInliningTransform::new(import_base_dir.clone(), Arc::clone(&self.resolver)) + .apply(graph) + .map_err(ImportPrepareError::Hard)?; - if let Some(message) = Self::unresolved_imported_prompt_error(&graph) { - return Err(ImportPrepareError::Soft(message)); - } + if let Some(message) = Self::unresolved_imported_prompt_error(&graph) { + return Err(ImportPrepareError::Soft(message)); + } - let nested_imports = Self::collect_import_nodes(&graph); - for (placeholder_id, import_path) in nested_imports { - self.expand_import( - &mut graph, - &placeholder_id, - &import_path, - &import_base_dir, - import_stack, - )?; - } + let nested_imports = Self::collect_import_nodes(&graph); + for (placeholder_id, import_path) in nested_imports { + self.expand_import( + &mut graph, + &placeholder_id, + &import_path, + &import_base_dir, + import_stack, + )?; + } - Self::validate_imported_graph(graph).map_err(ImportPrepareError::Soft) - }, - ) + Self::validate_imported_graph(graph).map_err(ImportPrepareError::Soft) + }) } fn splice_import( diff --git a/lib/crates/fabro-workflow/src/workflow_bundle.rs b/lib/crates/fabro-workflow/src/workflow_bundle.rs index ba5cfb643..bd3624b50 100644 --- a/lib/crates/fabro-workflow/src/workflow_bundle.rs +++ b/lib/crates/fabro-workflow/src/workflow_bundle.rs @@ -4,13 +4,21 @@ use std::sync::Arc; use serde::{Deserialize, Serialize}; -use crate::file_resolver::{BundleFileResolver, FileResolver, normalize_logical_path}; +use crate::ManifestPath; +use crate::file_resolver::{BundleFileResolver, FileResolver}; + +#[derive(Clone, Debug, Serialize, Deserialize)] +pub struct ParsedWorkflowConfig { + pub path: ManifestPath, + pub source: String, +} #[derive(Clone, Debug, Default, Serialize, Deserialize)] pub struct BundledWorkflow { - pub logical_path: PathBuf, - pub source: String, - pub files: HashMap, + pub path: ManifestPath, + pub source: String, + pub config: Option, + pub files: HashMap, } impl BundledWorkflow { @@ -21,7 +29,7 @@ impl BundledWorkflow { #[must_use] pub fn current_dir(&self) -> PathBuf { - self.logical_path + self.path .parent() .map_or_else(|| PathBuf::from("."), Path::to_path_buf) } @@ -29,46 +37,46 @@ impl BundledWorkflow { #[derive(Clone, Debug, Default, Serialize, Deserialize)] pub struct WorkflowBundle { - workflows: HashMap, + workflows: HashMap, } impl WorkflowBundle { #[must_use] - pub fn new(workflows: HashMap) -> Self { + pub fn new(workflows: HashMap) -> Self { Self { workflows } } - pub fn workflow(&self, logical_path: &Path) -> Option<&BundledWorkflow> { - self.workflows.get(logical_path) + pub fn workflow(&self, path: &ManifestPath) -> Option<&BundledWorkflow> { + self.workflows.get(path) } pub fn resolve_child( &self, - current_workflow_path: &Path, + current_workflow_path: &ManifestPath, reference: &str, ) -> Option<&BundledWorkflow> { let current_dir = current_workflow_path .parent() .unwrap_or_else(|| Path::new(".")); - let logical_path = normalize_logical_path(current_dir, reference)?; - self.workflows.get(&logical_path) + let path = ManifestPath::from_reference(current_dir, reference)?; + self.workflows.get(&path) } #[must_use] - pub fn workflows(&self) -> &HashMap { + pub fn workflows(&self) -> &HashMap { &self.workflows } } #[derive(Clone, Debug, Serialize, Deserialize)] pub struct RunDefinition { - pub workflow_path: PathBuf, - pub workflows: HashMap, + pub workflow_path: ManifestPath, + pub workflows: HashMap, } impl RunDefinition { #[must_use] - pub fn new(workflow_path: PathBuf, bundle: WorkflowBundle) -> Self { + pub fn new(workflow_path: ManifestPath, bundle: WorkflowBundle) -> Self { Self { workflow_path, workflows: bundle.workflows,