diff --git a/lib/crates/fabro-cli/src/commands/exec.rs b/lib/crates/fabro-cli/src/commands/exec.rs index c421632a8..935e81e95 100644 --- a/lib/crates/fabro-cli/src/commands/exec.rs +++ b/lib/crates/fabro-cli/src/commands/exec.rs @@ -345,7 +345,7 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu // against the CLI process env — the mirror of the `fabro run` worker // boundary in `fabro_workflow::operations::start::runtime_mcp_server`. // Both consumers read the same source-form settings; missing env is a hard - // error (D3) and reserved secrets/inputs tokens surface loudly rather than + // error and reserved secrets/inputs tokens surface loudly rather than // leaking. let mcp_servers = mcp_servers .into_iter() diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 6a9623ea4..7452a3ff5 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -154,7 +154,7 @@ fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings { #[expect( clippy::disallowed_methods, reason = "known leak: prepare step templates collapse to raw source unresolved; strict \ - resolution scheduled in the interpolation unification (Phase 2)" + resolution scheduled for follow-up interpolation cleanup" )] fn resolve_prepare( prepare: Option<&RunPrepareLayer>, diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index 9aba7fa7c..e79e5f9c2 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -647,7 +647,7 @@ name = "sonnet" } other => panic!("expected file goal, got {other:?}"), } - // run.working_dir is demoted (D11): the env token stays literal text. + // run.working_dir is demoted: the env token stays literal text. assert_eq!( settings.working_dir.as_deref(), Some("{{ env.FABRO_WORKDIR }}") diff --git a/lib/crates/fabro-server/src/automation_materializer.rs b/lib/crates/fabro-server/src/automation_materializer.rs index 4658543e6..c63c8bb04 100644 --- a/lib/crates/fabro-server/src/automation_materializer.rs +++ b/lib/crates/fabro-server/src/automation_materializer.rs @@ -13,7 +13,7 @@ use tokio::{fs, task}; use crate::git_checkout::{ GitCheckoutError, GitRepoCache, WorktreePrepareInput, github_metadata_url, - parse_github_repository_slug, resolve_git_auth_config, + resolve_git_auth_config, }; #[derive(Debug, Clone, PartialEq, Eq)] @@ -46,7 +46,6 @@ pub(crate) enum RunMaterializeError { impl From for RunMaterializeError { fn from(value: GitCheckoutError) -> Self { match value { - GitCheckoutError::InvalidTarget(message) => Self::InvalidTarget(message), GitCheckoutError::CloneFailed(message) => Self::CloneFailed(message), } } @@ -93,7 +92,7 @@ impl AutomationRunMaterializer for ProductionAutomationRunMaterializer { &self, input: AutomationRunMaterializeInput, ) -> Result { - let repo = parse_github_repository_slug(&input.target.repository)?; + let repo = parse_target_repository(&input.target.repository)?; fs::create_dir_all(&input.temp_root).await.map_err(|err| { RunMaterializeError::CloneFailed(format!( "failed to create temp root {}: {err}", @@ -157,6 +156,11 @@ fn render_error_chain(error: &(dyn std::error::Error + 'static)) -> String { collect_chain(error).join(": ") } +fn parse_target_repository(value: &str) -> Result { + fabro_automation::parse_github_repository_slug(value) + .map_err(|err| RunMaterializeError::InvalidTarget(err.to_string())) +} + #[derive(Debug)] pub(crate) struct ManifestFromCheckoutInput { workflow: String, @@ -329,7 +333,7 @@ mod tests { let user_settings_path = temp.path().join("settings.toml"); fs::write(&user_settings_path, "_version = 1\n").unwrap(); let run_id = RunId::new(); - let repo = parse_github_repository_slug("workspace-org/app").unwrap(); + let repo = parse_target_repository("workspace-org/app").unwrap(); let sha = "0123456789abcdef0123456789abcdef01234567".to_string(); let materialized = build_manifest_from_checkout(ManifestFromCheckoutInput { diff --git a/lib/crates/fabro-server/src/git_checkout.rs b/lib/crates/fabro-server/src/git_checkout.rs index 8abe77a62..7220a829b 100644 --- a/lib/crates/fabro-server/src/git_checkout.rs +++ b/lib/crates/fabro-server/src/git_checkout.rs @@ -17,8 +17,6 @@ const GIT_REV_PARSE_TIMEOUT: Duration = Duration::from_secs(10); /// Error returned while preparing a checkout from a git source. #[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)] pub(crate) enum GitCheckoutError { - #[error("invalid repository target: {0}")] - InvalidTarget(String), #[error("failed to clone repository: {0}")] CloneFailed(String), } @@ -58,9 +56,9 @@ impl GitRepoCache { /// `//.git`. Subsequent calls reuse the bare clone /// and only `git fetch --depth 1` the requested ref. In both cases a /// short-lived worktree is added at `worktree_dir`; the caller owns its - /// lifetime (typically a `TempDir`). Stale worktree admin entries from - /// crashed prior calls are pruned at the start of each call so they do - /// not accumulate. + /// lifetime (typically a `TempDir`). If stale worktree admin entries from + /// crashed prior calls block the add, the cache prunes those entries and + /// retries once. pub(crate) async fn prepare_worktree( &self, args: WorktreePrepareInput<'_>, @@ -87,7 +85,14 @@ impl GitRepoCache { // and network failures don't trip this branch because // `bare_clone_may_be_corrupt` only returns true when the // bare repo's `HEAD` file is missing or empty. - let _ = fs::remove_dir_all(&bare_dir).await; + if let Err(remove_err) = fs::remove_dir_all(&bare_dir).await { + tracing::warn!( + path = %bare_dir.display(), + error = %remove_err, + "failed to remove corrupt cached git repository" + ); + return Err(first_err); + } self.try_prepare_worktree(&bare_dir, &args, clone_url) .await .map_err(|_| first_err) @@ -105,11 +110,7 @@ impl GitRepoCache { let bare_exists = fs::try_exists(&bare_dir.join("HEAD")) .await .unwrap_or(false); - if bare_exists { - // Drop any worktree admin entries whose working trees were - // deleted by previous `TempDir` cleanup. Cheap and idempotent. - let _ = run_git_plan(build_worktree_prune_plan(bare_dir)).await; - } else { + if !bare_exists { if let Some(parent) = bare_dir.parent() { fs::create_dir_all(parent).await.map_err(|err| { GitCheckoutError::CloneFailed(format!( @@ -133,7 +134,7 @@ impl GitRepoCache { .await .map(|stdout| String::from_utf8_lossy(&stdout).trim().to_string())?; - run_git_plan(build_worktree_add_plan(bare_dir, args.worktree_dir)).await?; + add_worktree_with_stale_retry(bare_dir, args.worktree_dir, &checked_out_sha).await?; Ok(checked_out_sha) } @@ -149,17 +150,20 @@ pub(crate) struct WorktreePrepareInput<'a> { async fn bare_clone_may_be_corrupt(bare_dir: &Path) -> bool { match fs::metadata(&bare_dir.join("HEAD")).await { Ok(meta) => meta.len() == 0, - Err(_) => bare_dir.exists(), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + fs::try_exists(bare_dir).await.unwrap_or(false) + } + Err(err) => { + tracing::warn!( + path = %bare_dir.join("HEAD").display(), + %err, + "Failed to inspect cached git repository HEAD" + ); + false + } } } -pub(crate) fn parse_github_repository_slug( - value: &str, -) -> Result { - fabro_automation::parse_github_repository_slug(value) - .map_err(|err| GitCheckoutError::InvalidTarget(err.to_string())) -} - fn github_clone_url(repo: &GitHubRepositorySlug) -> String { format!("https://github.com/{}/{}.git", repo.owner(), repo.repo()) } @@ -326,7 +330,7 @@ fn build_bare_fetch_plan( .with_auth(clone_url, auth) } -fn build_worktree_add_plan(bare_dir: &Path, worktree_dir: &Path) -> GitCommandPlan { +fn build_worktree_add_plan(bare_dir: &Path, worktree_dir: &Path, target: &str) -> GitCommandPlan { GitCommandPlan::new( [ "worktree".to_string(), @@ -334,7 +338,7 @@ fn build_worktree_add_plan(bare_dir: &Path, worktree_dir: &Path) -> GitCommandPl "--detach".to_string(), "--force".to_string(), worktree_dir.display().to_string(), - "FETCH_HEAD".to_string(), + target.to_string(), ], GIT_WORKTREE_ADD_TIMEOUT, ) @@ -349,6 +353,34 @@ fn build_rev_parse_fetch_head_plan(bare_dir: &Path) -> GitCommandPlan { GitCommandPlan::new(["rev-parse", "FETCH_HEAD"], GIT_REV_PARSE_TIMEOUT).current_dir(bare_dir) } +async fn add_worktree_with_stale_retry( + bare_dir: &Path, + worktree_dir: &Path, + target: &str, +) -> Result<(), GitCheckoutError> { + match run_git_plan(build_worktree_add_plan(bare_dir, worktree_dir, target)).await { + Ok(_) => Ok(()), + Err(first_err) => { + tracing::warn!( + %first_err, + bare_dir = %bare_dir.display(), + worktree_dir = %worktree_dir.display(), + "git worktree add failed; pruning stale worktree entries and retrying" + ); + if let Err(prune_err) = run_git_plan(build_worktree_prune_plan(bare_dir)).await { + tracing::warn!( + %prune_err, + bare_dir = %bare_dir.display(), + "failed to prune stale git worktree entries" + ); + } + run_git_plan(build_worktree_add_plan(bare_dir, worktree_dir, target)) + .await + .map(|_| ()) + } + } +} + async fn run_git_plan(plan: GitCommandPlan) -> Result, GitCheckoutError> { let mut command = Command::new(&plan.program); command.args(&plan.args); @@ -432,9 +464,13 @@ mod tests { use super::*; + fn repository_slug(value: &str) -> GitHubRepositorySlug { + fabro_automation::parse_github_repository_slug(value).expect("slug should parse") + } + #[test] fn target_repository_urls_are_github_metadata_urls_without_credentials() { - let repo = parse_github_repository_slug("fabro-sh/fabro").expect("slug should parse"); + let repo = repository_slug("fabro-sh/fabro"); assert_eq!(repo.owner(), "fabro-sh"); assert_eq!(repo.repo(), "fabro"); @@ -449,25 +485,9 @@ mod tests { assert!(!github_clone_url(&repo).contains('@')); } - #[test] - fn target_repository_validation_rejects_non_github_owner_repo_shapes() { - for value in [ - "fabro-sh", - "https://github.com/fabro-sh/fabro", - "fabro-sh/fabro/extra", - "-owner/repo", - ] { - let error = parse_github_repository_slug(value).expect_err("invalid slug should fail"); - assert!( - error.to_string().contains("invalid repository target"), - "unexpected error for {value}: {error}" - ); - } - } - #[test] fn target_repository_validation_matches_automation_validation() { - let repo = parse_github_repository_slug("owner/.github").expect("slug should parse"); + let repo = repository_slug("owner/.github"); assert_eq!(repo.owner(), "owner"); assert_eq!(repo.repo(), ".github"); @@ -479,7 +499,7 @@ mod tests { #[test] fn bare_cache_command_plans_use_argv_prompt_disable_and_timeouts() { - let repo = parse_github_repository_slug("fabro-sh/fabro").unwrap(); + let repo = repository_slug("fabro-sh/fabro"); let clone_url = github_clone_url(&repo); let temp = TempDir::new().unwrap(); let bare_dir = temp.path().join("fabro-sh/fabro.git"); @@ -511,14 +531,18 @@ mod tests { assert_eq!(fetch.timeout, Duration::from_mins(1)); assert_eq!(fetch.env_value("GIT_TERMINAL_PROMPT"), Some("0")); - let worktree = build_worktree_add_plan(&bare_dir, &worktree_dir); + let worktree = build_worktree_add_plan( + &bare_dir, + &worktree_dir, + "0123456789abcdef0123456789abcdef01234567", + ); assert_eq!(worktree.args, vec![ "worktree", "add", "--detach", "--force", worktree_dir.to_str().unwrap(), - "FETCH_HEAD", + "0123456789abcdef0123456789abcdef01234567", ]); assert_eq!(worktree.current_dir.as_deref(), Some(bare_dir.as_path())); assert_eq!(worktree.timeout, Duration::from_secs(30)); @@ -563,7 +587,7 @@ mod tests { #[test] fn credential_config_env_keeps_clone_url_uncredentialed() { - let repo = parse_github_repository_slug("fabro-sh/fabro").unwrap(); + let repo = repository_slug("fabro-sh/fabro"); let clone_url = github_clone_url(&repo); let auth = GitAuthConfig::new( Some("x-access-token".to_string()), @@ -671,7 +695,7 @@ mod tests { let upstream = temp.path().join("upstream.git"); let expected_sha = seed_upstream(&upstream); let cache = GitRepoCache::new(temp.path().join("cache")); - let repo = parse_github_repository_slug("fabro-sh/fabro").unwrap(); + let repo = repository_slug("fabro-sh/fabro"); let upstream_url = upstream.to_str().unwrap().to_string(); let worktree_a = temp.path().join("wt-a"); @@ -723,7 +747,7 @@ mod tests { let upstream = temp.path().join("upstream.git"); let expected_sha = seed_upstream(&upstream); let cache = GitRepoCache::new(temp.path().join("cache")); - let repo = parse_github_repository_slug("fabro-sh/fabro").unwrap(); + let repo = repository_slug("fabro-sh/fabro"); let upstream_url = upstream.to_str().unwrap().to_string(); let worktree_a = temp.path().join("wt-a"); diff --git a/lib/crates/fabro-types/src/settings/interp.rs b/lib/crates/fabro-types/src/settings/interp.rs index 8395b7cc9..ed768d78f 100644 --- a/lib/crates/fabro-types/src/settings/interp.rs +++ b/lib/crates/fabro-types/src/settings/interp.rs @@ -2,7 +2,7 @@ //! //! An [`InterpString`] field may contain narrow `{{ .NAME }}` //! tokens — no template logic. Three [`Namespace`]s resolve here: `env`, -//! `vars`, and `secrets`. `inputs` is **template-only** (D12): it is a +//! `vars`, and `secrets`. `inputs` is **template-only**: it is a //! recognized namespace so an `{{ inputs.* }}` token fails loudly with a clear //! message instead of passing through as literal text, but it never resolves //! in an `InterpString` field — it belongs in prompts and goals. Which of the @@ -149,7 +149,7 @@ impl<'a> ResolveCtx<'a> { Namespace::Env => self.env.as_mut(), Namespace::Vars => self.vars.as_mut(), Namespace::Secrets => self.secrets.as_mut(), - // `inputs` is template-only (D12): an `InterpString` resolve context + // `inputs` is template-only: an `InterpString` resolve context // never provides it, so an `{{ inputs.* }}` token is always // unavailable here. `substitute_with` still preserves the token so a // goal (an `InterpString` that feeds a template) can forward it. @@ -361,7 +361,7 @@ impl InterpString { clippy::disallowed_methods, reason = "intentional raw-source fallback so a missing env var surfaces as a \ recognizable diagnostic; slated for hard-error semantics in the \ - interpolation unification (D3)" + interpolation cleanup" )] #[must_use] pub fn resolve_or_source(&self, lookup: F) -> String @@ -506,7 +506,7 @@ impl fmt::Display for ResolveError { self.name, self.name ), ResolveErrorKind::Unavailable => match namespace { - // `inputs` is template-only (D12): it never resolves in an + // `inputs` is template-only: it never resolves in an // `InterpString` field. Point the user at where it works. Namespace::Inputs => write!( f, @@ -816,8 +816,8 @@ mod tests { #[test] fn resolve_with_rejects_inputs_as_template_only() { - // D12: `inputs` is template-only. An `{{ inputs.* }}` token never - // resolves in an `InterpString` field — it fails loudly, pointing the + // `inputs` is template-only. An `{{ inputs.* }}` token never resolves + // in an `InterpString` field — it fails loudly, pointing the // user at prompts and goals. let s = InterpString::parse("run-{{ inputs.ticket-id }}"); diff --git a/lib/crates/fabro-workflow/src/file_resolver.rs b/lib/crates/fabro-workflow/src/file_resolver.rs index 5389e522a..53485addf 100644 --- a/lib/crates/fabro-workflow/src/file_resolver.rs +++ b/lib/crates/fabro-workflow/src/file_resolver.rs @@ -181,4 +181,109 @@ mod tests { assert_eq!(resolved.content, "prompt content"); } + + #[test] + fn filesystem_resolver_reads_existing_file() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("prompt.md"), "inlined content").unwrap(); + + let resolved = FilesystemFileResolver::new(None) + .resolve(dir.path(), "prompt.md") + .expect("file should resolve"); + + assert_eq!(resolved.content, "inlined content"); + } + + #[test] + fn filesystem_resolver_returns_none_for_missing_file() { + let dir = tempfile::tempdir().unwrap(); + + assert!( + FilesystemFileResolver::new(None) + .resolve(dir.path(), "nonexistent.md") + .is_none() + ); + } + + #[test] + fn filesystem_resolver_expands_tilde() { + let home = dirs::home_dir().expect("home dir must exist"); + let test_file = home.join(".fabro_test_tilde_tmp"); + std::fs::write(&test_file, "tilde content").unwrap(); + let _cleanup = scopeguard::guard((), |()| { + let _ = std::fs::remove_file(&test_file); + }); + + let dir = tempfile::tempdir().unwrap(); + let resolved = FilesystemFileResolver::new(None) + .resolve(dir.path(), "~/.fabro_test_tilde_tmp") + .expect("tilde path should resolve"); + + assert_eq!(resolved.content, "tilde content"); + } + + #[test] + fn filesystem_resolver_resolves_dotdot() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("file.md"), "dotdot content").unwrap(); + std::fs::create_dir(dir.path().join("subdir")).unwrap(); + + let resolved = FilesystemFileResolver::new(None) + .resolve(dir.path(), "subdir/../file.md") + .expect("dotdot path should resolve"); + + assert_eq!(resolved.content, "dotdot content"); + } + + #[test] + fn filesystem_resolver_falls_back_to_fallback_dir() { + let base = tempfile::tempdir().unwrap(); + let fallback = tempfile::tempdir().unwrap(); + std::fs::write(fallback.path().join("shared.md"), "shared content").unwrap(); + + let resolved = FilesystemFileResolver::new(Some(fallback.path().to_path_buf())) + .resolve(base.path(), "shared.md") + .expect("file should resolve from the fallback dir"); + + assert_eq!(resolved.content, "shared content"); + } + + #[test] + fn filesystem_resolver_base_dir_takes_precedence_over_fallback() { + let base = tempfile::tempdir().unwrap(); + let fallback = tempfile::tempdir().unwrap(); + std::fs::write(base.path().join("prompt.md"), "base content").unwrap(); + std::fs::write(fallback.path().join("prompt.md"), "fallback content").unwrap(); + + let resolved = FilesystemFileResolver::new(Some(fallback.path().to_path_buf())) + .resolve(base.path(), "prompt.md") + .expect("file should resolve from the base dir"); + + assert_eq!(resolved.content, "base content"); + } + + #[test] + fn filesystem_resolver_no_fallback_for_tilde_path() { + let base = tempfile::tempdir().unwrap(); + let fallback = tempfile::tempdir().unwrap(); + std::fs::write(fallback.path().join("file.md"), "fallback").unwrap(); + + // A tilde path to a nonexistent file does not fall back to the fallback dir. + assert!( + FilesystemFileResolver::new(Some(fallback.path().to_path_buf())) + .resolve(base.path(), "~/nonexistent_fabro_test.md") + .is_none() + ); + } + + #[test] + fn filesystem_resolver_returns_none_without_fallback() { + let base = tempfile::tempdir().unwrap(); + + assert!( + FilesystemFileResolver::new(None) + .resolve(base.path(), "missing.md") + .is_none() + ); + } } diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 4bbd50a70..02d870379 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -68,7 +68,6 @@ pub struct CreatedRun { struct PersistCreateOptions { settings: WorkflowSettings, - vars: HashMap, run_id: Option, run_dir: Option, workflow_slug: Option, @@ -146,9 +145,9 @@ pub async fn create( let persisted = spawn_blocking(move || { create_from_source( &raw_source, + vars, PersistCreateOptions { settings, - vars, run_id: Some(run_id), run_dir: Some(persisted_run_dir), workflow_slug: workflow_slug.or(resolved_workflow_slug), @@ -288,19 +287,20 @@ fn store_error(err: impl std::fmt::Display) -> Error { fn create_from_source( dot_source: &str, + vars: HashMap, options: PersistCreateOptions, current_dir: Option, file_resolver: Option>, goal_override: Option<&str>, ) -> Result { + let template_context = template_context(Some(&options.settings), vars); let mut validated = preprocess_and_validate( dot_source, options.source_name.clone(), current_dir, file_resolver, Vec::new(), - Some(&options.settings), - options.vars.clone(), + template_context, goal_override, RenderMode::Structural, &options.catalog, @@ -322,16 +322,13 @@ pub(super) fn preprocess_and_validate( current_dir: Option, file_resolver: Option>, custom_transforms: Vec>, - settings: Option<&WorkflowSettings>, - vars: HashMap, + template_context: TemplateContext, goal_override: Option<&str>, render_mode: RenderMode, catalog: &Arc, ) -> Result { - let inputs = run_inputs(settings); let mut parsed = pipeline::parse(dot_source)?; apply_goal_override(&mut parsed.graph, goal_override); - let template_context = TemplateContext::new().with_inputs(inputs).with_vars(vars); let transformed = pipeline::transform(parsed, &TransformOptions { current_dir, @@ -345,6 +342,15 @@ pub(super) fn preprocess_and_validate( Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) } +pub(super) fn template_context( + settings: Option<&WorkflowSettings>, + vars: HashMap, +) -> TemplateContext { + TemplateContext::new() + .with_inputs(run_inputs(settings)) + .with_vars(vars) +} + fn run_inputs(settings: Option<&WorkflowSettings>) -> HashMap { settings .map(|settings| settings.run.inputs.clone()) @@ -366,7 +372,6 @@ fn persist_validated( ) -> Result { let PersistCreateOptions { settings, - vars: _, run_id, run_dir, workflow_slug, @@ -503,8 +508,7 @@ mod tests { Some(PathBuf::from(".")), None, Vec::new(), - Some(&WorkflowSettings::default()), - vars, + template_context(Some(&WorkflowSettings::default()), vars), None, RenderMode::Structural, &test_catalog(), @@ -672,8 +676,7 @@ mod tests { Some(PathBuf::from(".")), None, Vec::new(), - Some(&WorkflowSettings::default()), - HashMap::new(), + template_context(Some(&WorkflowSettings::default()), HashMap::new()), None, RenderMode::Strict, &test_catalog(), @@ -709,8 +712,7 @@ mod tests { None, ))), Vec::new(), - Some(&WorkflowSettings::default()), - HashMap::new(), + template_context(Some(&WorkflowSettings::default()), HashMap::new()), None, RenderMode::Strict, &test_catalog(), diff --git a/lib/crates/fabro-workflow/src/operations/validate.rs b/lib/crates/fabro-workflow/src/operations/validate.rs index 83375f761..c11315d3c 100644 --- a/lib/crates/fabro-workflow/src/operations/validate.rs +++ b/lib/crates/fabro-workflow/src/operations/validate.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use fabro_model::Catalog; use fabro_types::WorkflowSettings; -use super::create::preprocess_and_validate; +use super::create::{preprocess_and_validate, template_context}; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; use crate::operations::RenderMode; @@ -44,8 +44,7 @@ pub fn validate(input: ValidateInput) -> Result { resolved.current_dir, resolved.file_resolver, input.custom_transforms, - Some(&resolved.settings), - input.vars, + template_context(Some(&resolved.settings), input.vars), resolved.goal_override.as_deref(), RenderMode::Structural, &input.catalog, diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index 1fa18d75a..ff599824b 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -8,33 +7,13 @@ use fabro_types::ManifestPath; use fabro_validate::Diagnostic; use super::Transform; +use super::importable_field::ImportableField; use crate::error::Error; use crate::file_resolver::{FileResolver, FileResolverTemplateStore, ResolvedFile}; -use crate::static_reference::{ReferenceKind, validate_static_reference}; use crate::transforms::variable_expansion::{ RenderMode, TemplateRenderStore, TemplateRenderTarget, render_template_for_target, }; -/// Resolve a potential `@path` file reference. -/// -/// If `value` starts with `@` and the referenced file exists locally, the file -/// contents are returned (inlined). Otherwise the original value is returned -/// unchanged. -pub fn resolve_file_ref( - value: &str, - current_dir: &Path, - resolver: &dyn FileResolver, -) -> Result { - let Some(path_str) = value.strip_prefix('@') else { - return Ok(value.to_string()); - }; - validate_static_reference(path_str, ReferenceKind::FileInline) - .map_err(|error| Error::Validation(error.to_string()))?; - Ok(resolver - .resolve(current_dir, path_str) - .map_or_else(|| value.to_string(), |resolved| resolved.content)) -} - fn parent_dir_or_dot(path: &Path) -> PathBuf { path.parent() .filter(|parent| !parent.as_os_str().is_empty()) @@ -168,13 +147,6 @@ impl FileInliningTransform { self } - /// Run-scoped `{{ vars.* }}` available to prompts and the goal. - #[must_use] - pub fn with_vars(mut self, vars: HashMap) -> Self { - self.context = self.context.with_vars(vars); - self - } - pub(crate) fn apply_with_diagnostics( &self, graph: Graph, @@ -222,7 +194,7 @@ impl FileInliningTransform { &mut diagnostics, )?; let value = self - .render_resolved_file_ref(&rendered, &ctx, target, &mut diagnostics)? + .render_import(&rendered, &ctx, target, &mut diagnostics)? .unwrap_or(rendered); node.attrs .insert("prompt".to_string(), AttrValue::String(value)); @@ -265,7 +237,7 @@ impl FileInliningTransform { let rendered = render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics)?; let value = self - .render_resolved_file_ref(&rendered, &ctx, target, diagnostics)? + .render_import(&rendered, &ctx, target, diagnostics)? .unwrap_or(rendered); graph .attrs @@ -273,19 +245,22 @@ impl FileInliningTransform { Ok(()) } - fn render_resolved_file_ref( + /// Resolve a node `prompt` / graph `goal` value to its final text. The + /// `rendered` value is the already-MiniJinja-rendered inline content; when + /// it is an `@path` import, the file is loaded and its contents rendered + /// too. Returns `Ok(None)` for inline content or a missing file, so the + /// caller falls back to the rendered inline value. + fn render_import( &self, - value: &str, + rendered: &str, ctx: &TemplateContext, owner_target: TemplateRenderTarget, diagnostics: &mut Vec, ) -> Result, Error> { - let Some(path_str) = value.strip_prefix('@') else { + let Some(path) = ImportableField::parse(rendered).import_path()? else { return Ok(None); }; - validate_static_reference(path_str, ReferenceKind::FileInline) - .map_err(|error| Error::Validation(error.to_string()))?; - let Some(resolved) = self.resolver.resolve(&self.current_dir, path_str) else { + let Some(resolved) = self.resolver.resolve(&self.current_dir, path) else { return Ok(None); }; let (source, store) = self.template_source_for_resolved_file(&resolved)?; @@ -293,8 +268,8 @@ impl FileInliningTransform { .with_source_name(resolved.path.display().to_string()) .with_source_origin(Some(&resolved.content), &resolved.content) .with_template_store(TemplateRenderStore::new(source, store)); - Ok(Some(render_file_contents( - &resolved, + Ok(Some(render_template_for_target( + &resolved.content, ctx, self.render_mode, &target, @@ -303,16 +278,14 @@ impl FileInliningTransform { } /// Resolve an `output_schema` value. An inline JSON string is returned - /// as-is; an `@file` reference is loaded verbatim. Unlike `prompt`, + /// as-is; an `@file` import is loaded verbatim. Unlike `prompt`, /// `output_schema` is not a template, so neither the value nor the loaded /// file contents are MiniJinja-rendered. fn resolve_output_schema_ref(&self, node_id: &str, value: &str) -> Result { - let Some(path_str) = value.strip_prefix('@') else { + let Some(path) = ImportableField::parse(value).import_path()? else { return Ok(value.to_string()); }; - validate_static_reference(path_str, ReferenceKind::FileInline) - .map_err(|error| Error::Validation(error.to_string()))?; - let Some(resolved) = self.resolver.resolve(&self.current_dir, path_str) else { + let Some(resolved) = self.resolver.resolve(&self.current_dir, path) else { return Err(Error::Validation(format!( "node '{node_id}' output_schema has unresolved file reference: {value}" ))); @@ -377,16 +350,6 @@ impl Transform for FileInliningTransform { } } -pub(crate) fn render_file_contents( - resolved: &ResolvedFile, - ctx: &TemplateContext, - render_mode: RenderMode, - target: &TemplateRenderTarget, - diagnostics: &mut Vec, -) -> Result { - render_template_for_target(&resolved.content, ctx, render_mode, target, diagnostics) -} - #[cfg(test)] mod tests { #![expect( @@ -394,6 +357,7 @@ mod tests { reason = "These unit tests use the real git CLI to build repositories for file-inlining transform coverage." )] + use std::collections::HashMap; use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph, Node}; @@ -407,45 +371,6 @@ mod tests { ManifestPath::from_wire(value).expect("path should parse") } - #[test] - fn resolve_file_ref_passthrough_non_at() { - let dir = tempfile::tempdir().unwrap(); - assert_eq!( - resolve_file_ref( - "hello world", - dir.path(), - &FilesystemFileResolver::new(None), - ) - .unwrap(), - "hello world" - ); - } - - #[test] - fn resolve_file_ref_passthrough_missing_file() { - let dir = tempfile::tempdir().unwrap(); - assert_eq!( - resolve_file_ref( - "@nonexistent.md", - dir.path(), - &FilesystemFileResolver::new(None), - ) - .unwrap(), - "@nonexistent.md" - ); - } - - #[test] - fn resolve_file_ref_inlines_existing_file() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("prompt.md"), "inlined content").unwrap(); - - assert_eq!( - resolve_file_ref("@prompt.md", dir.path(), &FilesystemFileResolver::new(None)).unwrap(), - "inlined content" - ); - } - #[test] fn file_inlining_transform_inlines_prompt_and_goal() { let dir = tempfile::tempdir().unwrap(); @@ -764,127 +689,6 @@ mod tests { assert!(matches!(err, fabro_template::TemplateError::Load { .. })); } - #[test] - fn resolve_file_ref_expands_tilde() { - let home = dirs::home_dir().expect("home dir must exist"); - let test_file = home.join(".fabro_test_tilde_tmp"); - std::fs::write(&test_file, "tilde content").unwrap(); - let _cleanup = scopeguard::guard((), |()| { - let _ = std::fs::remove_file(&test_file); - }); - - let dir = tempfile::tempdir().unwrap(); - - assert_eq!( - resolve_file_ref( - "@~/.fabro_test_tilde_tmp", - dir.path(), - &FilesystemFileResolver::new(None), - ) - .unwrap(), - "tilde content" - ); - } - - #[test] - fn resolve_file_ref_resolves_dotdot() { - let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("file.md"), "dotdot content").unwrap(); - std::fs::create_dir(dir.path().join("subdir")).unwrap(); - - assert_eq!( - resolve_file_ref( - "@subdir/../file.md", - dir.path(), - &FilesystemFileResolver::new(None), - ) - .unwrap(), - "dotdot content" - ); - } - - #[test] - fn resolve_file_ref_falls_back_to_fallback_dir() { - let base = tempfile::tempdir().unwrap(); - let fallback = tempfile::tempdir().unwrap(); - std::fs::write(fallback.path().join("shared.md"), "shared content").unwrap(); - - assert_eq!( - resolve_file_ref( - "@shared.md", - base.path(), - &FilesystemFileResolver::new(Some(fallback.path().to_path_buf())), - ) - .unwrap(), - "shared content" - ); - } - - #[test] - fn resolve_file_ref_base_dir_takes_precedence_over_fallback() { - let base = tempfile::tempdir().unwrap(); - let fallback = tempfile::tempdir().unwrap(); - std::fs::write(base.path().join("prompt.md"), "base content").unwrap(); - std::fs::write(fallback.path().join("prompt.md"), "fallback content").unwrap(); - - assert_eq!( - resolve_file_ref( - "@prompt.md", - base.path(), - &FilesystemFileResolver::new(Some(fallback.path().to_path_buf())), - ) - .unwrap(), - "base content" - ); - } - - #[test] - fn resolve_file_ref_no_fallback_for_tilde_path() { - let base = tempfile::tempdir().unwrap(); - let fallback = tempfile::tempdir().unwrap(); - std::fs::write(fallback.path().join("file.md"), "fallback").unwrap(); - - // Tilde path to nonexistent file should return original value, not try fallback - let result = resolve_file_ref( - "@~/nonexistent_fabro_test.md", - base.path(), - &FilesystemFileResolver::new(Some(fallback.path().to_path_buf())), - ) - .unwrap(); - assert_eq!(result, "@~/nonexistent_fabro_test.md"); - } - - #[test] - fn resolve_file_ref_fallback_none_behaves_as_before() { - let base = tempfile::tempdir().unwrap(); - assert_eq!( - resolve_file_ref( - "@missing.md", - base.path(), - &FilesystemFileResolver::new(None) - ) - .unwrap(), - "@missing.md" - ); - } - - #[test] - fn resolve_file_ref_rejects_template_path() { - let base = tempfile::tempdir().unwrap(); - let err = resolve_file_ref( - "@prompts/{{ inputs.prompt_file }}", - base.path(), - &FilesystemFileResolver::new(None), - ) - .unwrap_err(); - - assert!( - err.to_string() - .contains("templates are not supported in file inline references"), - "unexpected error: {err}" - ); - } - #[test] fn file_inlining_transform_falls_back_to_fallback_dir() { let base = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 546468cf4..b5406ddc6 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -68,14 +68,6 @@ impl ImportTransform { } } - /// Run-scoped `{{ vars.* }}`, propagated into imported subgraphs so their - /// prompts and goals interpolate variables too. - #[must_use] - pub fn with_vars(mut self, vars: HashMap) -> Self { - self.context = self.context.with_vars(vars); - self - } - #[must_use] pub fn with_template_options( mut self, diff --git a/lib/crates/fabro-workflow/src/transforms/importable_field.rs b/lib/crates/fabro-workflow/src/transforms/importable_field.rs new file mode 100644 index 000000000..acfcc52e8 --- /dev/null +++ b/lib/crates/fabro-workflow/src/transforms/importable_field.rs @@ -0,0 +1,129 @@ +//! The `ImportableField` type: a workflow field that is either inline +//! content or an `@path` file import. +//! +//! Three field consumers share this classification: +//! - node `prompt` and the graph `goal` are *templated* importable fields — the +//! inline value (or an imported file's contents) is MiniJinja-rendered; +//! - `output_schema` is a *verbatim* importable field — inline content and +//! imported file contents are used as-is, never rendered. +//! +//! This type owns the `@`-classification and static-reference validation that +//! used to be hand-rolled at each call site. The render-vs-verbatim handling +//! and the file-store plumbing stay with each consumer in +//! [`super::file_inlining`], where the `FileResolver` and current-dir context +//! live. + +use crate::error::Error; +use crate::static_reference::{ReferenceKind, validate_static_reference}; + +/// A field value that is either inline content or an `@path` file import. +/// +/// Borrows the classified string: callers always already hold the inline value +/// (and fall back to it), so the type never needs to own a copy. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ImportableField<'a> { + /// Inline content — the literal value or, for templated fields, the + /// already-rendered text. The caller keeps the value itself; this variant + /// carries no payload. + Inline, + /// An `@path` file import. `path` has the leading `@` stripped. + Import { path: &'a str }, +} + +impl<'a> ImportableField<'a> { + /// Classify a value: a leading `@` marks a file import, everything else is + /// inline. + /// + /// Callers of templated fields (`prompt`/`goal`) classify the + /// *already-rendered* string, because a leading `@` may be produced by + /// rendering (e.g. `{{ inputs.prompt_file }}` expanding to + /// `@prompts/work.md`). + pub(crate) fn parse(value: &'a str) -> Self { + match value.strip_prefix('@') { + Some(path) => Self::Import { path }, + None => Self::Inline, + } + } + + /// The validated import path (leading `@` stripped), or `None` for inline + /// content. Validating here means a caller cannot extract a path without it + /// being checked: an import is a static reference and must not contain + /// template syntax (e.g. `@prompts/{{ inputs.x }}.md`). + pub(crate) fn import_path(&self) -> Result, Error> { + match self { + Self::Import { path } => { + validate_static_reference(path, ReferenceKind::FileInline) + .map_err(|error| Error::Validation(error.to_string()))?; + Ok(Some(path)) + } + Self::Inline => Ok(None), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_classifies_inline_value() { + assert_eq!( + ImportableField::parse("Do the work"), + ImportableField::Inline + ); + } + + #[test] + fn parse_classifies_at_reference_as_import() { + assert_eq!( + ImportableField::parse("@prompts/work.md"), + ImportableField::Import { + path: "prompts/work.md", + } + ); + } + + #[test] + fn parse_strips_only_the_leading_at() { + // A non-leading `@` (e.g. an email address) is inline, not an import. + assert_eq!( + ImportableField::parse("ping me@example.com"), + ImportableField::Inline + ); + } + + #[test] + fn import_path_returns_validated_path_for_imports_only() { + assert_eq!( + ImportableField::parse("@goal.md").import_path().unwrap(), + Some("goal.md") + ); + assert_eq!( + ImportableField::parse("inline").import_path().unwrap(), + None + ); + } + + #[test] + fn import_path_accepts_inline_and_plain_import_paths() { + ImportableField::parse("plain inline text") + .import_path() + .unwrap(); + ImportableField::parse("@prompts/work.md") + .import_path() + .unwrap(); + } + + #[test] + fn import_path_rejects_template_syntax() { + let err = ImportableField::parse("@prompts/{{ inputs.prompt_file }}") + .import_path() + .unwrap_err(); + + assert!( + err.to_string() + .contains("templates are not supported in file inline references"), + "unexpected error: {err}" + ); + } +} diff --git a/lib/crates/fabro-workflow/src/transforms/mod.rs b/lib/crates/fabro-workflow/src/transforms/mod.rs index f812853d3..1f6879cda 100644 --- a/lib/crates/fabro-workflow/src/transforms/mod.rs +++ b/lib/crates/fabro-workflow/src/transforms/mod.rs @@ -10,13 +10,14 @@ pub trait Transform { mod file_inlining; mod import; +mod importable_field; mod model_resolution; mod preamble; pub mod stylesheet; mod stylesheet_application; pub mod variable_expansion; -pub use file_inlining::{FileInliningTransform, resolve_file_ref}; +pub use file_inlining::FileInliningTransform; pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index b822a930b..66434266a 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -309,12 +309,6 @@ impl TemplateTransform { } } - #[must_use] - pub fn with_vars(mut self, vars: HashMap) -> Self { - self.context = self.context.with_vars(vars); - self - } - pub(crate) fn resolved_goal( &self, graph: &Graph,