From 52ff7b2ef2778a6e6b46d2ffc54bc8cda91bbf83 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 29 Aug 2026 09:10:39 -0400 Subject: [PATCH 1/3] Add local RunIntent producer support --- .../fabro-server/src/manifest_validation.rs | 210 +++++- lib/apps/fabro-server/src/run_intent.rs | 135 +++- lib/apps/fabro-server/src/run_manifest.rs | 5 +- lib/components/fabro-manifest/src/lib.rs | 242 ++++++- .../src/local_workflow_package.rs | 609 ++++++++++++++++++ .../fabro-manifest/src/workflow_bundler.rs | 85 ++- .../src/workflow_version_collector.rs | 52 +- 7 files changed, 1288 insertions(+), 50 deletions(-) create mode 100644 lib/components/fabro-manifest/src/local_workflow_package.rs diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index acc5e05e1..28e9928ae 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -1,11 +1,14 @@ use std::collections::HashMap; +use std::path::PathBuf; -use anyhow::Result; +use anyhow::{Result, anyhow}; use fabro_api::types; -use fabro_config::RunLayer; +use fabro_config::{RunLayer, WorkflowSettingsBuilder}; +use fabro_manifest::CollectedWorkflowClosure; +use fabro_workflow::operations::{ValidateInput, WorkflowInput, validate}; use fabro_workflow::pipeline::TEMPLATE_UNDEFINED_VARIABLE_RULE; -use crate::run_manifest; +use crate::{run_intent, run_manifest}; /// Validate a manifest without a model catalog. /// @@ -28,6 +31,54 @@ pub fn validate_manifest( Ok(run_manifest::validate_response(&prepared, &validated)) } +/// Validate an already collected local workflow before any version upload. +/// +/// The supplied run layer is complete, including any already resolved inline +/// goal. Validation uses only seeded environment defaults, immutable workflow +/// settings, and explicit inputs; it performs no store, HTTP, user-settings, +/// project-settings, or model-catalog operation. Undefined template variables +/// are promoted to errors before the response is returned. +pub fn validate_collected_workflow( + closure: &CollectedWorkflowClosure, + run_overrides: Option<&RunLayer>, + input_overrides: &HashMap, +) -> Result { + let lowered = run_intent::lower_collected_workflow_closure(closure)?; + let workflow = lowered + .workflow_bundle + .workflow(&lowered.entrypoint) + .cloned() + .ok_or_else(|| anyhow!("lowered root workflow is missing from its bundle"))?; + let mut builder = WorkflowSettingsBuilder::new() + .server_manifest_defaults( + RunLayer::default(), + fabro_environment::seeded_catalog_layer(), + ) + .server_mcp_catalog(HashMap::new()); + if let Some(run) = run_overrides { + builder = builder.run_overrides(run.clone()); + } + if let Some(layer) = lowered.workflow_layer { + builder = builder.workflow_layer(layer); + } + let mut settings = builder.build().map_err(anyhow::Error::new)?; + settings.run.inputs.extend(input_overrides.clone()); + let validated = validate(ValidateInput { + workflow: WorkflowInput::Bundled(workflow), + settings, + vars: HashMap::new(), + cwd: PathBuf::from("/workspace"), + custom_transforms: Vec::new(), + }) + .map_err(anyhow::Error::new)?; + let mut response = types::ValidateResponse { + ok: !validated.has_errors(), + workflow: run_manifest::workflow_summary(&validated, lowered.entrypoint.as_path()), + }; + promote_template_undefined_variables_to_errors(&mut response); + Ok(response) +} + pub fn promote_template_undefined_variables_to_errors(response: &mut types::ValidateResponse) { let mut promoted = false; for diagnostic in &mut response.workflow.diagnostics { @@ -40,3 +91,156 @@ pub fn promote_template_undefined_variables_to_errors(response: &mut types::Vali response.ok = false; } } + +#[cfg(test)] +mod tests { + #![expect( + clippy::disallowed_methods, + reason = "collected-validation tests write isolated workflow fixtures synchronously" + )] + + use std::fs; + use std::path::{Path, PathBuf}; + + use fabro_config::RunGoalLayer; + use fabro_types::settings::InterpString; + + use super::*; + + fn write(root: &Path, path: &str, content: &str) { + let path = root.join(path); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, content).unwrap(); + } + + fn write_complete_fixture(root: &Path) -> PathBuf { + write( + root, + "root/workflow.toml", + r#"_version = 1 +[workflow] +graph = "workflow.fabro" +[run.goal] +file = "goal.md" +[run.environment.image] +dockerfile = { path = "Dockerfile" } +"#, + ); + write( + root, + "root/workflow.fabro", + r#"digraph Root { + start [shape=Mdiamond] + imported [import="imports/shared.fabro"] + task [prompt="@prompts/task.md", model="future-provider/future-model"] + child [stack.child_workflow="../child/workflow.fabro"] + exit [shape=Msquare] + start -> imported -> task -> child -> exit + }"#, + ); + write( + root, + "root/imports/shared.fabro", + "digraph Shared { start [shape=Mdiamond] shared [prompt=\"shared\"] exit \ + [shape=Msquare] start -> shared -> exit }", + ); + write( + root, + "root/prompts/task.md", + "Hello {{ inputs.owner }}. {% include \"detail.md\" %}", + ); + write(root, "root/prompts/detail.md", "detail"); + write(root, "root/goal.md", "workflow goal"); + write(root, "root/Dockerfile", "FROM alpine\n"); + write( + root, + "child/workflow.fabro", + "digraph Child { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ); + root.join("root/workflow.toml") + } + + fn run_overrides(goal: &str) -> RunLayer { + RunLayer { + goal: Some(RunGoalLayer::Inline(InterpString::parse(goal))), + ..RunLayer::default() + } + } + + #[test] + fn collected_validation_matches_legacy_response_for_equivalent_inputs() { + 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 package = fabro_manifest::resolve_local_workflow_package( + &workflow, + temp.path(), + Some(temp.path()), + ) + .unwrap(); + let manifest = fabro_manifest::build_run_manifest(fabro_manifest::ManifestBuildInput { + workflow, + cwd: temp.path().to_path_buf(), + run_overrides: Some(run.clone()), + input_overrides: inputs.clone(), + args: Some(types::ManifestArgs { + input: vec!["owner=Ada".to_string()], + ..types::ManifestArgs::default() + }), + environment_defaults: fabro_environment::seeded_catalog_layer(), + ..fabro_manifest::ManifestBuildInput::default() + }) + .unwrap(); + + let mut legacy = validate_manifest(&RunLayer::default(), &manifest.manifest).unwrap(); + promote_template_undefined_variables_to_errors(&mut legacy); + let collected = + validate_collected_workflow(package.closure(), Some(&run), &inputs).unwrap(); + + assert_eq!( + serde_json::to_value(collected).unwrap(), + serde_json::to_value(legacy).unwrap(), + ); + } + + #[test] + fn collected_validation_promotes_undefined_inputs_and_accepts_explicit_values() { + let temp = tempfile::tempdir().unwrap(); + let workflow = write_complete_fixture(temp.path()); + let package = fabro_manifest::resolve_local_workflow_package( + &workflow, + temp.path(), + Some(temp.path()), + ) + .unwrap(); + + let missing = + validate_collected_workflow(package.closure(), None, &HashMap::new()).unwrap(); + assert!(!missing.ok); + assert!(missing.workflow.diagnostics.iter().any(|diagnostic| { + diagnostic.rule == TEMPLATE_UNDEFINED_VARIABLE_RULE + && diagnostic.severity == types::WorkflowDiagnosticSeverity::Error + })); + + let present = validate_collected_workflow( + package.closure(), + Some(&run_overrides("resolved inline goal")), + &HashMap::from([("owner".to_string(), toml::Value::String("Ada".to_string()))]), + ) + .unwrap(); + assert!( + present + .workflow + .diagnostics + .iter() + .all(|diagnostic| { diagnostic.rule != TEMPLATE_UNDEFINED_VARIABLE_RULE }) + ); + assert!(present.ok); + assert_eq!(present.workflow.goal, "resolved inline goal"); + assert!(present.workflow.diagnostics.iter().all(|diagnostic| { + !diagnostic.message.contains("future-provider") + && !diagnostic.message.contains("future-model") + })); + } +} diff --git a/lib/apps/fabro-server/src/run_intent.rs b/lib/apps/fabro-server/src/run_intent.rs index 425392159..a872019b5 100644 --- a/lib/apps/fabro-server/src/run_intent.rs +++ b/lib/apps/fabro-server/src/run_intent.rs @@ -4,10 +4,11 @@ use std::path::{Component, Path, PathBuf}; use fabro_config::parse::SettingsSource; use fabro_config::{EnvironmentLayer, RunEnvironmentLayer, RunGoalLayer, SettingsLayer}; use fabro_environment::{EnvironmentId, EnvironmentValidationError}; +use fabro_manifest::CollectedWorkflowClosure; use fabro_types::settings::InterpString; use fabro_types::{ GitContext, ManifestPath, RunTarget, SandboxProviderKind, TargetValidationError, WorkflowPath, - WorkflowVersionId, + WorkflowVersion, WorkflowVersionId, }; use fabro_workflow::git; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; @@ -183,8 +184,69 @@ pub(crate) enum WorkflowClosureLoweringError { }, } -pub(crate) fn lower_workflow_closure( - closure: &LoadedWorkflowVersionClosure, +trait WorkflowClosureView { + fn root_id(&self) -> WorkflowVersionId; + fn root(&self) -> &WorkflowVersion; + fn validated_root(&self) -> &ValidatedWorkflowVersion; + fn get(&self, id: &WorkflowVersionId) -> Option<&WorkflowVersion>; +} + +impl WorkflowClosureView for LoadedWorkflowVersionClosure { + fn root_id(&self) -> WorkflowVersionId { + self.root_id() + } + + fn root(&self) -> &WorkflowVersion { + self.root() + } + + fn validated_root(&self) -> &ValidatedWorkflowVersion { + self.validated_root() + } + + fn get(&self, id: &WorkflowVersionId) -> Option<&WorkflowVersion> { + self.get(id) + } +} + +struct CollectedWorkflowClosureView<'a> { + root_id: WorkflowVersionId, + versions: HashMap, +} + +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 }) + } +} + +impl WorkflowClosureView for CollectedWorkflowClosureView<'_> { + fn root_id(&self) -> WorkflowVersionId { + 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") + } + + fn get(&self, id: &WorkflowVersionId) -> Option<&WorkflowVersion> { + self.versions.get(id).map(|version| version.version()) + } +} + +fn lower_workflow_closure_view( + closure: &impl WorkflowClosureView, ) -> Result { let entrypoint = manifest_path(closure.root().entrypoint(), closure.root().entrypoint())?; let mut mounts = HashMap::new(); @@ -227,6 +289,19 @@ pub(crate) fn lower_workflow_closure( }) } +pub(crate) fn lower_workflow_closure( + closure: &LoadedWorkflowVersionClosure, +) -> Result { + lower_workflow_closure_view(closure) +} + +pub(crate) fn lower_collected_workflow_closure( + closure: &CollectedWorkflowClosure, +) -> Result { + let view = CollectedWorkflowClosureView::new(closure)?; + lower_workflow_closure_view(&view) +} + pub(crate) fn pin_workflow_environment_authority(layer: &mut SettingsLayer, environment_id: &str) { // Both blocks destructure without `..` so adding a field to either layer // type forces a compile-time decision here: server-owned facts are @@ -262,7 +337,7 @@ pub(crate) fn pin_workflow_environment_authority(layer: &mut SettingsLayer, envi } fn mount_version( - closure: &LoadedWorkflowVersionClosure, + closure: &impl WorkflowClosureView, id: WorkflowVersionId, mounted_entrypoint: ManifestPath, mounts: &mut HashMap, @@ -460,6 +535,58 @@ mod tests { .unwrap() } + #[tokio::test] + async fn collected_and_stored_closures_lower_through_the_same_path() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("root"); + let child = temp.path().join("child"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&child).unwrap(); + fs::write( + root.join("workflow.toml"), + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"goal.md\"\n", + ) + .await + .unwrap(); + fs::write( + root.join("workflow.fabro"), + "digraph Root { child [stack.child_workflow=\"../child/workflow.fabro\"] }", + ) + .await + .unwrap(); + fs::write(root.join("goal.md"), "Ship it").await.unwrap(); + fs::write(child.join("workflow.fabro"), "digraph Child {}") + .await + .unwrap(); + + let collected = + fabro_manifest::collect_workflow_versions(&root.join("workflow.toml"), temp.path()) + .unwrap(); + let (database, _) = crate::test_support::test_store_bundle(); + let store = WorkflowVersionStore::new(database.blobs()); + for (_, version) in collected.versions() { + store.put(version).await.unwrap(); + } + let stored = store + .get_closure(&collected.root_id()) + .await + .unwrap() + .unwrap(); + + let from_collected = lower_collected_workflow_closure(&collected).unwrap(); + let from_stored = lower_workflow_closure(&stored).unwrap(); + + assert_eq!(from_collected.entrypoint, from_stored.entrypoint); + assert_eq!( + serde_json::to_value(from_collected.workflow_bundle.workflows()).unwrap(), + serde_json::to_value(from_stored.workflow_bundle.workflows()).unwrap(), + ); + assert_eq!( + format!("{:?}", from_collected.workflow_layer), + format!("{:?}", from_stored.workflow_layer), + ); + } + #[tokio::test] async fn prepares_a_canonical_folder_target_without_git_projection() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 809dd34df..ef7056fb1 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -1563,7 +1563,10 @@ fn preflight_response( } } -fn workflow_summary(validated: &Validated, target_path: &Path) -> types::PreflightWorkflowSummary { +pub(crate) fn workflow_summary( + validated: &Validated, + target_path: &Path, +) -> types::PreflightWorkflowSummary { types::PreflightWorkflowSummary { diagnostics: diagnostics_to_api(validated.diagnostics()), edges: i64::try_from(validated.graph().edges.len()) diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index b4612df28..3352a91cf 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -3,6 +3,7 @@ reason = "CLI manifest builder: sync file I/O building install manifests" )] +mod local_workflow_package; mod workflow_bundler; mod workflow_version_collector; @@ -24,11 +25,16 @@ use fabro_template::validate_static_reference; use fabro_types::graph::ReferenceKind; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; -use fabro_types::{DirtyStatus, GitContext, ManifestPath, WorkflowSettings}; +use fabro_types::{ + DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, WorkflowSettings, +}; use fabro_workflow::git::{ GitSyncStatus, branch_needs_push, head_sha, push_branch_noninteractive, sync_status, }; +pub use crate::local_workflow_package::{ + LocalWorkflowPackageError, ResolvedLocalWorkflowPackage, resolve_local_workflow_package, +}; use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, @@ -210,7 +216,8 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { )?; let configured_repo_origin_url = configured_repo_origin_url(&workflow_settings); - let git = build_git_context(&working_directory, configured_repo_origin_url.as_deref()); + let git = observe_git_run_target(&working_directory, configured_repo_origin_url.as_deref()) + .map(GitRunTargetObservation::into_legacy_git_context); let args = input.args.filter(|args| !manifest_args_is_empty(args)); Ok(BuiltManifest { @@ -300,11 +307,57 @@ fn resolved_goal_to_manifest(resolved: ResolvedRunGoal) -> types::ManifestGoal { } } -fn build_git_context( +/// Facts observed from one usable attached local Git checkout. +/// +/// The optional target is absent when the checkout's effective origin is not +/// a canonical GitHub repository. Its SHA is present only when local tracking +/// state or the existing safe best-effort push proves that exact commit is +/// remotely available. The legacy projection retains the historical local +/// 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 + } +} + +/// Observe Git facts without choosing an environment or a non-Git target. +/// +/// Outer `None` means `repo_path` is not a usable attached checkout. A +/// returned observation with no target means Git facts were available but the +/// effective origin was not a canonical GitHub repository. +#[must_use] +pub fn observe_git_run_target( repo_path: &Path, configured_repo_origin_url: Option<&str>, -) -> Option { - let (origin_url, branch) = detect_manifest_repo_info(repo_path)?; +) -> Option { + let (origin_url, push_origin_url, branch) = detect_manifest_repo_info(repo_path)?; let sha = head_sha(repo_path).ok(); let dirty = match sync_status(repo_path, "origin", Some(&branch)) { GitSyncStatus::Dirty => DirtyStatus::Dirty, @@ -320,17 +373,36 @@ fn build_git_context( .filter(|url| !url.is_empty()) }) .unwrap_or_default(); - push_manifest_branch_best_effort( + let remotely_available = push_manifest_branch_best_effort( repo_path, &branch, - origin_url.as_deref(), + push_origin_url.as_deref(), configured_repo_origin_url, ); - Some(GitContext { - origin_url: repo_origin_url, - branch, + let run_target = github_run_target( + &repo_origin_url, + &branch, + remotely_available.then(|| sha.clone()).flatten(), + ); + Some(GitRunTargetObservation { + run_target, + legacy_git_context: GitContext { + origin_url: repo_origin_url, + branch, + sha, + dirty, + }, + }) +} + +fn github_run_target(origin_url: &str, branch: &str, sha: Option) -> Option { + let (owner, repository) = fabro_github::parse_github_owner_repo(origin_url).ok()?; + let slug = GitHubRepositorySlug::try_new(&format!("{owner}/{repository}"))?; + Some(GitRunTarget { + repo: slug.to_string(), + branch: branch.to_owned(), + tag: None, sha, - dirty, }) } @@ -353,14 +425,30 @@ fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { (!normalized.is_empty()).then_some(normalized) } -fn detect_manifest_repo_info(repo_path: &Path) -> Option<(Option, String)> { +fn detect_manifest_repo_info(repo_path: &Path) -> Option<(Option, Option, String)> { let repo = git2::Repository::discover(repo_path).ok()?; - let branch = repo.head().ok()?.shorthand().map(ToOwned::to_owned)?; + if repo.is_bare() { + return None; + } + let head = repo.head().ok()?; + if !head.is_branch() { + return None; + } + let branch = head.shorthand().map(ToOwned::to_owned)?; let origin_url = repo .find_remote("origin") .ok() .and_then(|remote| remote.url().map(ToOwned::to_owned)); - Some((origin_url, branch)) + // Keep the legacy observed URL above, but compare configured repository + // identity against the remote's raw config bytes. This lets a repository- + // local `url.*.insteadOf` redirect the existing push safely without making + // the rewrite target look like a different repository. + let push_origin_url = repo + .config() + .ok() + .and_then(|config| config.get_string("remote.origin.url").ok()) + .or_else(|| origin_url.clone()); + Some((origin_url, push_origin_url, branch)) } /// Best-effort push of the local branch so clone-based execution can see @@ -372,9 +460,9 @@ fn push_manifest_branch_best_effort( branch: &str, origin_url: Option<&str>, configured_repo_origin_url: Option<&str>, -) { +) -> bool { let Some(origin_url) = origin_url else { - return; + return false; }; if let Some(repo_origin_url) = configured_repo_origin_url @@ -383,15 +471,15 @@ fn push_manifest_branch_best_effort( { let remote = fabro_github::normalize_repo_origin_url(origin_url); if remote != repo_origin_url { - return; + return false; } } if !branch_needs_push(repo_path, "origin", branch) { - return; + return true; } - let _ = push_branch_noninteractive(repo_path, "origin", branch); + push_branch_noninteractive(repo_path, "origin", branch).is_ok() } /// Resolve a workflow reference and reject it when neither its config nor @@ -1627,6 +1715,122 @@ exit 1 assert_eq!(std::fs::read_to_string(helper_log).unwrap(), "0\n"); } + #[test] + fn observes_synced_github_branch_as_an_exact_target() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + std::fs::create_dir_all(&workspace).unwrap(); + init_git_repo(&workspace, "feature", "https://github.com/acme/widgets.git"); + 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(); + + 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.origin_url, "https://github.com/acme/widgets"); + } + + #[test] + fn observes_exact_sha_only_after_a_successful_noninteractive_push() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + std::fs::create_dir_all(&workspace).unwrap(); + let bare_origin = init_bare_origin(temp.path()); + init_git_repo(&workspace, "feature", "https://github.com/acme/widgets"); + let local_url = format!("file://{}", bare_origin.display()); + run_git(&workspace, &[ + "config", + &format!("url.{local_url}.insteadOf"), + "https://github.com/acme/widgets", + ]); + + let observation = + observe_git_run_target(&workspace, Some("https://github.com/acme/widgets")).unwrap(); + let target = observation.run_target().unwrap(); + + assert_eq!(target.sha, observation.legacy_git_context().sha); + assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), target.sha,); + } + + #[test] + fn failed_push_and_origin_mismatch_produce_branch_only_targets() { + let temp = tempfile::tempdir().unwrap(); + let failed_workspace = temp.path().join("failed"); + std::fs::create_dir_all(&failed_workspace).unwrap(); + init_git_repo( + &failed_workspace, + "feature", + "https://github.com/acme/widgets", + ); + let missing_url = format!("file://{}/missing.git", temp.path().display()); + run_git(&failed_workspace, &[ + "config", + &format!("url.{missing_url}.insteadOf"), + "https://github.com/acme/widgets", + ]); + + 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); + + let mismatched_workspace = temp.path().join("mismatched"); + std::fs::create_dir_all(&mismatched_workspace).unwrap(); + let bare_origin = init_bare_origin(&temp.path().join("other")); + init_git_repo( + &mismatched_workspace, + "feature", + bare_origin.to_str().unwrap(), + ); + + let mismatched = observe_git_run_target( + &mismatched_workspace, + Some("https://github.com/acme/configured"), + ) + .unwrap(); + let target = mismatched.run_target().unwrap(); + assert_eq!(target.repo, "acme/configured"); + assert_eq!(target.sha, None); + assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), None); + } + + #[test] + fn git_observation_distinguishes_dirty_unsupported_and_unusable_checkouts() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + std::fs::create_dir_all(&workspace).unwrap(); + let bare_origin = init_bare_origin(temp.path()); + init_git_repo(&workspace, "feature", bare_origin.to_str().unwrap()); + 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_eq!( + unsupported.legacy_git_context().origin_url, + fabro_github::normalize_repo_origin_url(&bare_origin.to_string_lossy()), + ); + + let not_repo = temp.path().join("not-repo"); + std::fs::create_dir_all(¬_repo).unwrap(); + assert!(observe_git_run_target(¬_repo, None).is_none()); + + let unborn = temp.path().join("unborn"); + std::fs::create_dir_all(&unborn).unwrap(); + run_git(&unborn, &["init", "--quiet"]); + assert!(observe_git_run_target(&unborn, None).is_none()); + + run_git(&workspace, &["checkout", "--detach", "--quiet"]); + assert!(observe_git_run_target(&workspace, None).is_none()); + } + fn init_git_repo(path: &Path, branch: &str, origin_url: &str) { run_git(path, &[ "-c", diff --git a/lib/components/fabro-manifest/src/local_workflow_package.rs b/lib/components/fabro-manifest/src/local_workflow_package.rs new file mode 100644 index 000000000..36d47c095 --- /dev/null +++ b/lib/components/fabro-manifest/src/local_workflow_package.rs @@ -0,0 +1,609 @@ +#![expect( + clippy::result_large_err, + reason = "the public error contract preserves concrete config and collection source errors" +)] + +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::{CollectedWorkflowClosure, WorkflowVersionCollectError}; + +/// One local workflow resolved to a canonical package root and collected +/// exactly once. +#[derive(Debug)] +pub struct ResolvedLocalWorkflowPackage { + workflow_location: WorkflowLocation, + source_root: PathBuf, + closure: CollectedWorkflowClosure, +} + +/// Failure while resolving and collecting one local workflow package. +#[derive(Debug, Error)] +pub enum LocalWorkflowPackageError { + #[error("failed to resolve local workflow `{workflow}`")] + Resolve { + workflow: PathBuf, + #[source] + source: fabro_config::Error, + }, + #[error("failed to inspect a Git worktree for local workflow `{workflow}`")] + Repository { + workflow: PathBuf, + #[source] + source: git2::Error, + }, + #[error("failed to canonicalize local workflow package path `{path}`")] + Canonicalize { + path: PathBuf, + #[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, + #[source] + source: WorkflowVersionCollectError, + }, +} + +impl ResolvedLocalWorkflowPackage { + #[must_use] + pub fn workflow_location(&self) -> &WorkflowLocation { + &self.workflow_location + } + + #[must_use] + pub fn source_root(&self) -> &Path { + &self.source_root + } + + #[must_use] + 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 +/// root, then collect one immutable closure. +/// +/// Named workflows prefer the current checkout's `.fabro/workflows` tree, +/// preserve marked-project discovery, and use `user_workflows_root` only when +/// it is explicitly supplied. Explicit paths use their containing Git +/// worktree, the supplied user root, or their own containing directory, in +/// that order. No ambient home or process-current-directory state is read. +pub fn resolve_local_workflow_package( + workflow: &Path, + cwd: &Path, + user_workflows_root: Option<&Path>, +) -> Result { + let (location, source_root) = if is_workflow_name(workflow) { + resolve_named_workflow(workflow, cwd, user_workflows_root)? + } else { + 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 closure = collect_workflow_versions_at_location(&workflow_location, &source_root, workflow) + .map_err(|source| LocalWorkflowPackageError::Collect { + workflow: workflow.to_path_buf(), + source, + })?; + + Ok(ResolvedLocalWorkflowPackage { + workflow_location, + source_root, + closure, + }) +} + +fn is_workflow_name(workflow: &Path) -> bool { + workflow.extension().is_none() && workflow.components().count() == 1 +} + +fn resolve_named_workflow( + workflow: &Path, + cwd: &Path, + user_workflows_root: Option<&Path>, +) -> Result<(WorkflowLocation, PathBuf), LocalWorkflowPackageError> { + let current_root = match worktree_root(cwd, workflow)? { + Some(root) => root, + None => canonicalize(cwd)?, + }; + let project_candidate = current_root + .join(".fabro/workflows") + .join(workflow) + .join("workflow.toml"); + if project_candidate.is_file() { + return resolve_at(workflow, &project_candidate, current_root); + } + + let marked_project = + discover_project_config(cwd).map_err(|source| LocalWorkflowPackageError::Resolve { + workflow: workflow.to_path_buf(), + source, + })?; + if let Some(config) = marked_project { + let fabro_root = config + .parent() + .expect("a discovered project config has a parent"); + let candidate = fabro_root + .join("workflows") + .join(workflow) + .join("workflow.toml"); + if candidate.is_file() { + let project_root = fabro_root + .parent() + .expect("the .fabro directory has a project parent"); + let source_root = match worktree_root(&candidate, workflow)? { + Some(root) => root, + None => canonicalize(project_root)?, + }; + return resolve_at(workflow, &candidate, source_root); + } + } + + if let Some(user_root) = user_workflows_root { + let candidate = user_root.join(workflow).join("workflow.toml"); + if candidate.is_file() { + return resolve_at(workflow, &candidate, canonicalize(user_root)?); + } + } + + Err(LocalWorkflowPackageError::Resolve { + workflow: workflow.to_path_buf(), + source: fabro_config::Error::WorkflowNotFound(workflow.display().to_string()), + }) +} + +fn resolve_explicit_workflow( + workflow: &Path, + cwd: &Path, + user_workflows_root: Option<&Path>, +) -> Result<(WorkflowLocation, PathBuf), LocalWorkflowPackageError> { + let selected = if workflow.extension().is_none() { + let path = if workflow.is_absolute() { + workflow.to_path_buf() + } else { + cwd.join(workflow) + }; + if path.is_dir() { + path.join("workflow.toml") + } else { + return Err(LocalWorkflowPackageError::Resolve { + workflow: workflow.to_path_buf(), + source: fabro_config::Error::WorkflowNotFound(workflow.display().to_string()), + }); + } + } else { + workflow.to_path_buf() + }; + let location = resolve_location(workflow, &selected, cwd)?; + if let Some(root) = worktree_root(&location.graph, workflow)? { + return Ok((location, root)); + } + + if let Some(user_root) = user_workflows_root.filter(|root| root.exists()) { + let canonical_user_root = canonicalize(user_root)?; + let canonical_graph = canonicalize(&location.graph)?; + if canonical_graph.starts_with(&canonical_user_root) { + return Ok((location, canonical_user_root)); + } + } + + let source_root = location.dir.clone(); + Ok((location, source_root)) +} + +fn resolve_at( + workflow: &Path, + selected: &Path, + source_root: PathBuf, +) -> Result<(WorkflowLocation, PathBuf), LocalWorkflowPackageError> { + let selected = if selected.is_absolute() { + selected.to_path_buf() + } else { + canonicalize(selected)? + }; + let resolve_from = selected.parent().unwrap_or_else(|| Path::new(".")); + resolve_location(workflow, &selected, resolve_from).map(|location| (location, source_root)) +} + +fn resolve_location( + workflow: &Path, + selected: &Path, + cwd: &Path, +) -> Result { + let location = WorkflowLocation::resolve(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( + path: &Path, + workflow: &Path, +) -> Result, LocalWorkflowPackageError> { + let discover_from = if path.is_dir() { + path + } else { + path.parent().unwrap_or(path) + }; + let repository = match git2::Repository::discover(discover_from) { + Ok(repository) => repository, + Err(source) if source.code() == git2::ErrorCode::NotFound => return Ok(None), + Err(source) => { + return Err(LocalWorkflowPackageError::Repository { + workflow: workflow.to_path_buf(), + source, + }); + } + }; + if repository.is_bare() { + return Ok(None); + } + Ok(repository.workdir().map(Path::to_path_buf)) +} + +fn canonicalize(path: &Path) -> Result { + path.canonicalize() + .map_err(|source| LocalWorkflowPackageError::Canonicalize { + path: path.to_path_buf(), + source, + }) +} + +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( + clippy::disallowed_methods, + reason = "local-package tests build isolated filesystem and Git fixtures synchronously" + )] + + use std::fs; + use std::path::{Path, PathBuf}; + + use crate::{LocalWorkflowPackageError, resolve_local_workflow_package}; + + fn write(root: &Path, path: &str, content: &str) { + let path = root.join(path); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, content).unwrap(); + } + + fn write_workflow(root: &Path, directory: &str, graph: &str) -> PathBuf { + write( + root, + &format!("{directory}/workflow.toml"), + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ); + write(root, &format!("{directory}/workflow.fabro"), graph); + root.join(directory).join("workflow.toml") + } + + fn init_repo(path: &Path) { + git2::Repository::init(path).unwrap(); + } + + fn canonical_versions( + package: &crate::ResolvedLocalWorkflowPackage, + ) -> Vec<(fabro_types::WorkflowVersionId, Vec)> { + package + .closure() + .versions() + .map(|(id, version)| (id, version.version().canonical_bytes().unwrap())) + .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(": ") + } + + #[test] + fn named_markerless_project_workflow_precedes_explicit_user_root() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path().join("project"); + let user = temp.path().join("user-workflows"); + fs::create_dir_all(&project).unwrap(); + init_repo(&project); + write_workflow(&project, ".fabro/workflows/hello", "digraph Project {}"); + write_workflow(&user, "hello", "digraph User {}"); + + let package = + resolve_local_workflow_package(Path::new("hello"), &project, Some(&user)).unwrap(); + + assert_eq!(package.source_root(), project.canonicalize().unwrap()); + assert_eq!( + package.workflow_location().graph, + project + .join(".fabro/workflows/hello/workflow.fabro") + .canonicalize() + .unwrap(), + ); + assert_eq!( + package + .closure() + .versions() + .last() + .unwrap() + .1 + .version() + .entrypoint() + .as_str(), + ".fabro/workflows/hello/workflow.fabro", + ); + } + + #[test] + fn named_user_workflow_requires_and_uses_the_explicit_root() { + let temp = tempfile::tempdir().unwrap(); + let cwd = temp.path().join("cwd"); + let user = temp.path().join("user-workflows"); + fs::create_dir_all(&cwd).unwrap(); + write_workflow(&user, "hello", "digraph User {}"); + + let package = + 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(), + "hello/workflow.fabro", + ); + + let error = resolve_local_workflow_package(Path::new("hello"), &cwd, None).unwrap_err(); + assert!(matches!(error, LocalWorkflowPackageError::Resolve { .. })); + } + + #[test] + fn explicit_workflow_uses_its_own_checkout_and_has_location_independent_bytes() { + let temp = tempfile::tempdir().unwrap(); + let caller = temp.path().join("caller"); + let first = temp.path().join("first"); + let second = temp.path().join("second"); + for root in [&caller, &first, &second] { + fs::create_dir_all(root).unwrap(); + init_repo(root); + } + let first_workflow = write_workflow( + &first, + "flows/demo", + "digraph Demo { task [prompt=\"@prompt.md\"] }", + ); + write(&first, "flows/demo/prompt.md", "hello"); + let second_workflow = write_workflow( + &second, + "flows/demo", + "digraph Demo { task [prompt=\"@prompt.md\"] }", + ); + write(&second, "flows/demo/prompt.md", "hello"); + + let first_package = resolve_local_workflow_package(&first_workflow, &caller, None).unwrap(); + let second_package = + resolve_local_workflow_package(&second_workflow, &caller, None).unwrap(); + + assert_eq!(first_package.source_root(), first.canonicalize().unwrap()); + assert_eq!(second_package.source_root(), second.canonicalize().unwrap()); + assert_eq!( + canonical_versions(&first_package), + canonical_versions(&second_package) + ); + } + + #[test] + fn workflow_in_a_checkout_allows_parent_segments_that_stay_inside_the_root() { + let temp = tempfile::tempdir().unwrap(); + let package_root = temp.path().join("package"); + fs::create_dir_all(&package_root).unwrap(); + init_repo(&package_root); + let workflow = write_workflow( + &package_root, + "flows", + "digraph Demo { task [prompt=\"@../shared.md\"] }", + ); + write(&package_root, "shared.md", "shared"); + + let package = resolve_local_workflow_package(&workflow, temp.path(), None).unwrap(); + + assert_eq!(package.source_root(), package_root.canonicalize().unwrap()); + assert!( + package + .closure() + .versions() + .last() + .unwrap() + .1 + .version() + .files() + .keys() + .any(|path| path.as_str() == "shared.md"), + ); + } + + #[test] + fn loose_workflow_uses_its_canonical_containing_directory() { + let temp = tempfile::tempdir().unwrap(); + let workflow = write_workflow( + temp.path(), + "loose", + "digraph Demo { task [prompt=\"@prompt.md\"] }", + ); + write(temp.path(), "loose/prompt.md", "hello"); + + let package = resolve_local_workflow_package(&workflow, temp.path(), None).unwrap(); + + assert_eq!( + 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")); + } + + #[cfg(unix)] + #[test] + fn rejects_direct_and_template_symlinks_that_escape_a_loose_package() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let outside = temp.path().join("outside"); + fs::create_dir_all(&outside).unwrap(); + write(&outside, "secret.md", "secret"); + + let direct = write_workflow( + temp.path(), + "direct", + "digraph Demo { task [prompt=\"@secret.md\"] }", + ); + symlink( + outside.join("secret.md"), + temp.path().join("direct/secret.md"), + ) + .unwrap(); + let direct_error = resolve_local_workflow_package(&direct, temp.path(), None).unwrap_err(); + let direct_chain = error_chain(&direct_error); + assert!(direct_chain.contains("secret.md"), "{direct_chain}"); + assert!(direct_chain.contains("direct"), "{direct_chain}"); + + let template = write_workflow( + temp.path(), + "template", + "digraph Demo { task [prompt=\"@prompt.md\"] }", + ); + write( + temp.path(), + "template/prompt.md", + "{% include \"secret.md\" %}", + ); + symlink( + outside.join("secret.md"), + temp.path().join("template/secret.md"), + ) + .unwrap(); + let template_error = + resolve_local_workflow_package(&template, temp.path(), None).unwrap_err(); + let template_chain = error_chain(&template_error); + assert!( + template_chain.contains("escapes template root"), + "{template_chain}" + ); + } + + #[cfg(unix)] + #[test] + fn rejects_a_selected_workflow_symlink_outside_its_checkout() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let project = temp.path().join("project"); + let outside = temp.path().join("outside"); + fs::create_dir_all(project.join(".fabro/workflows/hello")).unwrap(); + fs::create_dir_all(&outside).unwrap(); + init_repo(&project); + write( + &project, + ".fabro/workflows/hello/workflow.toml", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ); + write(&outside, "workflow.fabro", "digraph Outside {}"); + symlink( + outside.join("workflow.fabro"), + project.join(".fabro/workflows/hello/workflow.fabro"), + ) + .unwrap(); + + let error = resolve_local_workflow_package(Path::new("hello"), &project, None).unwrap_err(); + + assert!(matches!( + error, + LocalWorkflowPackageError::EscapesSourceRoot { .. } + )); + } + + #[test] + fn malformed_and_missing_workflows_preserve_config_sources() { + let temp = tempfile::tempdir().unwrap(); + let malformed = temp.path().join("malformed/workflow.toml"); + write(temp.path(), "malformed/workflow.toml", "not valid = ["); + + let malformed_error = + resolve_local_workflow_package(&malformed, temp.path(), None).unwrap_err(); + assert!(matches!( + malformed_error, + LocalWorkflowPackageError::Resolve { .. } + )); + assert!(std::error::Error::source(&malformed_error).is_some()); + + let missing = + resolve_local_workflow_package(Path::new("missing"), temp.path(), None).unwrap_err(); + assert!(matches!(missing, LocalWorkflowPackageError::Resolve { .. })); + assert!(std::error::Error::source(&missing).is_some()); + } +} diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index bbf7454be..2bdd4ac97 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -16,11 +16,12 @@ use fabro_template::{ }; use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; +use thiserror::Error; use crate::{manifest_path_from_absolute, normalize_absolute_path}; pub(super) struct WorkflowBundler<'a> { - cwd: &'a Path, + package_root: &'a Path, inputs: &'a HashMap, template_store: FilesystemTemplateStore, workflows: HashMap, @@ -39,11 +40,11 @@ pub(super) struct CollectedWorkflowSource { } impl<'a> WorkflowBundler<'a> { - pub(super) fn new(cwd: &'a Path, inputs: &'a HashMap) -> Self { + pub(super) fn new(package_root: &'a Path, inputs: &'a HashMap) -> Self { Self { - cwd, + package_root, inputs, - template_store: FilesystemTemplateStore::new(cwd), + template_store: FilesystemTemplateStore::new(package_root), workflows: HashMap::new(), visited_workflows: HashSet::new(), workflow_version_projection: false, @@ -55,7 +56,7 @@ impl<'a> WorkflowBundler<'a> { workflow: &Path, project_config: Option<(&ManifestPath, &str)>, ) -> Result> { - let root_key = self.collect_workflow_entry(workflow, self.cwd)?; + let root_key = self.collect_workflow_entry(workflow, self.package_root)?; if let Some((config_path, source)) = project_config { let mut root = self @@ -89,19 +90,18 @@ impl<'a> WorkflowBundler<'a> { /// Collects the workflow at `location` and returns its manifest key. fn collect_workflow_location(&mut self, location: &WorkflowLocation) -> Result { - let dot_path = manifest_path_from_absolute(&location.graph, self.cwd)?; + let dot_path = manifest_path_from_absolute(&location.graph, self.package_root)?; let dot_key = dot_path.to_string(); if !self.visited_workflows.insert(dot_key.clone()) { return Ok(dot_key); } - let source = std::fs::read_to_string(&location.graph) - .with_context(|| format!("Failed to read {}", location.graph.display()))?; + let source = self.read_package_file(&location.graph)?; let config = if let Some(workflow_toml_path) = location.toml.as_ref() { Some(types::ManifestWorkflowConfig { - path: manifest_path_from_absolute(workflow_toml_path, self.cwd)?.to_string(), - source: std::fs::read_to_string(workflow_toml_path) - .with_context(|| format!("Failed to read {}", workflow_toml_path.display()))?, + path: manifest_path_from_absolute(workflow_toml_path, self.package_root)? + .to_string(), + source: self.read_package_file(workflow_toml_path)?, }) } else { None @@ -253,10 +253,7 @@ impl<'a> WorkflowBundler<'a> { for imported in imports { if visited_imports.insert(imported.path.to_string()) { - let imported_source = std::fs::read_to_string(&imported.absolute_path) - .with_context(|| { - format!("Failed to read {}", imported.absolute_path.display()) - })?; + let imported_source = self.read_package_file(&imported.absolute_path)?; let imported_scan = WorkflowScanInput { absolute_dot_path: imported.absolute_path, dot_path: imported.path, @@ -286,8 +283,7 @@ impl<'a> WorkflowBundler<'a> { bundled: &BundledFile, workflow_template_root: &ManifestPath, ) -> Result<()> { - let source = std::fs::read_to_string(&bundled.absolute_path) - .with_context(|| format!("Failed to read {}", bundled.absolute_path.display()))?; + let source = self.read_package_file(&bundled.absolute_path)?; let template_root = template_root_for_bundled_file(&bundled.path, workflow_template_root)?; self.collect_template_include_files( files, @@ -375,7 +371,7 @@ impl<'a> WorkflowBundler<'a> { let layer = source .parse::() .context("Failed to parse run config TOML")?; - let absolute_config_path = self.cwd.join(config_path.as_path()); + let absolute_config_path = self.package_root.join(config_path.as_path()); let base_dir = absolute_config_path .parent() .unwrap_or_else(|| Path::new(".")); @@ -460,11 +456,10 @@ impl<'a> WorkflowBundler<'a> { let absolute_path = normalize_absolute_path(base_dir, reference) .ok_or_else(|| anyhow!("unsupported manifest reference: {reference}"))?; - let path = manifest_path_from_absolute(&absolute_path, self.cwd)?; + let path = manifest_path_from_absolute(&absolute_path, self.package_root)?; 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()))?; + let content = self.read_package_file(&absolute_path)?; files.insert(key.clone(), types::ManifestFileEntry { content, ref_: types::ManifestFileRef { @@ -480,6 +475,54 @@ impl<'a> WorkflowBundler<'a> { path, }) } + + fn read_package_file(&self, path: &Path) -> Result { + if !self.workflow_version_projection { + 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, + })?; + 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()); + } + std::fs::read_to_string(&canonical).map_err(|source| { + PackageFileReadError::Read { + path: canonical, + source, + } + .into() + }) + } +} + +#[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 { diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 1a01473ce..bb8535a3f 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -2,6 +2,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::{Path, PathBuf}; use fabro_api::types; +use fabro_config::project::WorkflowLocation; use fabro_types::{ WorkflowPath, WorkflowPathParseError, WorkflowVersion, WorkflowVersionId, WorkflowVersionShapeError, @@ -85,9 +86,56 @@ pub fn collect_workflow_versions( let location = crate::resolve_existing_workflow_location(&repository_workflow, checkout_root) .map_err(|source| location_error(workflow, source))?; + let package_root = + checkout_root + .canonicalize() + .map_err(|source| WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::Error::new(source).context(format!( + "failed to canonicalize workflow package root {}", + checkout_root.display() + )), + })?; + let location = canonicalize_location(location, workflow)?; + collect_workflow_versions_at_location(&location, &package_root, workflow) +} + +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() + )), + }) + }; + 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, + }) +} + +pub(super) fn collect_workflow_versions_at_location( + location: &WorkflowLocation, + package_root: &Path, + workflow: &Path, +) -> Result { let inputs = HashMap::new(); - let collected = WorkflowBundler::new(checkout_root, &inputs) - .collect_versions(&location) + let collected = WorkflowBundler::new(package_root, &inputs) + .collect_versions(location) .map_err(|source| WorkflowVersionCollectError::Collect { path: workflow.to_path_buf(), source, From 0b46e1d7355eafb32ec8e12446dff808deacd71d Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sun, 30 Aug 2026 13:00:32 -0400 Subject: [PATCH 2/3] 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)] From 011876edd1013afedde4c74385b3901493e8abda Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 31 Aug 2026 14:08:27 -0400 Subject: [PATCH 3/3] Harden local RunIntent target observation --- lib/components/fabro-manifest/src/lib.rs | 223 +++++++++++++++--- .../src/local_workflow_package.rs | 27 ++- .../src/workflow_version_collector.rs | 22 +- lib/components/fabro-workflow/src/git.rs | 34 +++ .../tests/it/git_integration.rs | 44 +++- 5 files changed, 291 insertions(+), 59 deletions(-) diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index dcf049f63..f1f4bea15 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -26,11 +26,10 @@ use fabro_types::graph::ReferenceKind; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; use fabro_types::{ - DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, WorkflowSettings, -}; -use fabro_workflow::git::{ - GitSyncStatus, branch_needs_push, push_branch_noninteractive, sync_status, + DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, RunTarget, + WorkflowSettings, }; +use fabro_workflow::git::{self, GitSyncStatus}; pub use crate::local_workflow_package::{ LocalWorkflowPackageError, ResolvedLocalWorkflowPackage, resolve_local_workflow_package, @@ -216,8 +215,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(|observation| observation.legacy_git_context); + let git = build_legacy_git_context(&working_directory, configured_repo_origin_url.as_deref()); let args = input.args.filter(|args| !manifest_args_is_empty(args)); Ok(BuiltManifest { @@ -309,11 +307,11 @@ fn resolved_goal_to_manifest(resolved: ResolvedRunGoal) -> types::ManifestGoal { /// Facts observed from one usable attached local Git checkout. /// -/// The optional target is absent when the checkout's effective origin is not -/// a canonical GitHub repository. Its SHA is present only when local tracking -/// state or the existing safe best-effort push proves that exact commit is -/// remotely available. The legacy projection retains the historical local -/// SHA and normalized-origin behavior independently. +/// The optional target is absent when the checkout cannot be represented as a +/// valid GitHub run target. Its SHA is present only when a successful push or +/// a direct query of the remote proves that exact commit is available. The +/// legacy projection retains the historical local SHA and normalized-origin +/// behavior independently. #[derive(Clone, Debug)] pub struct GitRunTargetObservation { pub run_target: Option, @@ -324,19 +322,59 @@ pub struct GitRunTargetObservation { /// /// Outer `None` means `repo_path` is not a usable attached checkout. A /// returned observation with no target means Git facts were available but the -/// effective origin was not a canonical GitHub repository. +/// effective origin or attached branch cannot be represented by a valid +/// GitHub run target. For a valid target, this operation may contact the +/// remote and may make one noninteractive best-effort push of the attached +/// branch so clone-based execution can resolve the observed commit. #[must_use] pub fn observe_git_run_target( repo_path: &Path, configured_repo_origin_url: Option<&str>, ) -> Option { + let local = inspect_local_git(repo_path, configured_repo_origin_url)?; + let legacy_git_context = local.legacy_git_context; + let mut run_target = github_run_target( + &legacy_git_context.origin_url, + &legacy_git_context.branch, + None, + ); + if let Some(target) = run_target.as_mut() { + let publish_status = publish_manifest_branch_best_effort( + repo_path, + &legacy_git_context.branch, + local.push_origin_url.as_deref(), + configured_repo_origin_url, + ); + target.sha = remotely_available_sha( + repo_path, + &legacy_git_context.branch, + legacy_git_context.sha.as_deref(), + publish_status, + ); + } + + Some(GitRunTargetObservation { + run_target, + legacy_git_context, + }) +} + +struct LocalGitObservation { + push_origin_url: Option, + legacy_git_context: GitContext, +} + +fn inspect_local_git( + repo_path: &Path, + configured_repo_origin_url: Option<&str>, +) -> Option { 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)) { + let dirty = match git::sync_status(repo_path, "origin", Some(&branch)) { GitSyncStatus::Dirty => DirtyStatus::Dirty, GitSyncStatus::Synced | GitSyncStatus::Unsynced => DirtyStatus::Clean, }; @@ -350,19 +388,9 @@ pub fn observe_git_run_target( .filter(|url| !url.is_empty()) }) .unwrap_or_default(); - let remotely_available = push_manifest_branch_best_effort( - repo_path, - &branch, - push_origin_url.as_deref(), - configured_repo_origin_url, - ); - let run_target = github_run_target( - &repo_origin_url, - &branch, - sha.clone().filter(|_| remotely_available), - ); - Some(GitRunTargetObservation { - run_target, + + Some(LocalGitObservation { + push_origin_url, legacy_git_context: GitContext { origin_url: repo_origin_url, branch, @@ -372,15 +400,35 @@ pub fn observe_git_run_target( }) } +fn build_legacy_git_context( + repo_path: &Path, + configured_repo_origin_url: Option<&str>, +) -> Option { + let local = inspect_local_git(repo_path, configured_repo_origin_url)?; + publish_manifest_branch_best_effort( + repo_path, + &local.legacy_git_context.branch, + local.push_origin_url.as_deref(), + configured_repo_origin_url, + ); + Some(local.legacy_git_context) +} + fn github_run_target(origin_url: &str, branch: &str, sha: Option) -> Option { let (owner, repository) = fabro_github::parse_github_owner_repo(origin_url).ok()?; let slug = GitHubRepositorySlug::try_new(&format!("{owner}/{repository}"))?; - Some(GitRunTarget { + let validated = RunTarget::Git(GitRunTarget { repo: slug.to_string(), branch: branch.to_owned(), tag: None, sha, }) + .validate() + .ok()?; + let RunTarget::Git(target) = validated.target else { + unreachable!("a validated Git target must remain a Git target") + }; + Some(target) } fn configured_repo_origin_url(settings: &WorkflowSettings) -> Option { @@ -444,18 +492,25 @@ fn detect_manifest_repo_info(repo_path: &Path) -> Option { }) } -/// Best-effort push of the local branch so clone-based execution can see -/// local commits. A failed push must not fail manifest creation, and the +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum BranchPublishStatus { + TrackingRefMatches, + Pushed, + Unavailable, +} + +/// Best-effort publication of the local branch so clone-based execution can +/// see local commits. A failed push must not fail manifest creation, and the /// discarded push error may contain raw Git stderr, so it is deliberately /// neither returned nor logged here. -fn push_manifest_branch_best_effort( +fn publish_manifest_branch_best_effort( repo_path: &Path, branch: &str, origin_url: Option<&str>, configured_repo_origin_url: Option<&str>, -) -> bool { +) -> BranchPublishStatus { let Some(origin_url) = origin_url else { - return false; + return BranchPublishStatus::Unavailable; }; if let Some(repo_origin_url) = configured_repo_origin_url @@ -464,15 +519,39 @@ fn push_manifest_branch_best_effort( { let remote = fabro_github::normalize_repo_origin_url(origin_url); if remote != repo_origin_url { - return false; + return BranchPublishStatus::Unavailable; } } - if !branch_needs_push(repo_path, "origin", branch) { - return true; + if !git::branch_needs_push(repo_path, "origin", branch) { + return BranchPublishStatus::TrackingRefMatches; } - push_branch_noninteractive(repo_path, "origin", branch).is_ok() + if git::push_branch_noninteractive(repo_path, "origin", branch).is_ok() { + BranchPublishStatus::Pushed + } else { + BranchPublishStatus::Unavailable + } +} + +fn remotely_available_sha( + repo_path: &Path, + branch: &str, + local_sha: Option<&str>, + publish_status: BranchPublishStatus, +) -> Option { + let local_sha = local_sha?; + match publish_status { + BranchPublishStatus::Pushed => Some(local_sha.to_owned()), + BranchPublishStatus::TrackingRefMatches => { + git::remote_branch_sha_noninteractive(repo_path, "origin", branch) + .ok() + .flatten() + .filter(|remote_sha| remote_sha == local_sha) + .map(|_| local_sha.to_owned()) + } + BranchPublishStatus::Unavailable => None, + } } /// Resolve a workflow reference and reject it when neither its config nor @@ -1722,10 +1801,18 @@ exit 1 let temp = tempfile::tempdir().unwrap(); let workspace = temp.path().join("workspace"); std::fs::create_dir_all(&workspace).unwrap(); + let bare_origin = init_bare_origin(temp.path()); init_git_repo(&workspace, "feature", "https://github.com/acme/widgets.git"); - mark_origin_branch_synced(&workspace, "feature"); + let local_url = format!("file://{}", bare_origin.display()); + run_git(&workspace, &[ + "config", + &format!("url.{local_url}.insteadOf"), + "https://github.com/acme/widgets.git", + ]); + run_git(&workspace, &["push", "origin", "feature"]); - let observation = observe_git_run_target(&workspace, None).unwrap(); + let observation = + observe_git_run_target(&workspace, Some("https://github.com/acme/widgets")).unwrap(); let target = observation.run_target.as_ref().unwrap(); let legacy = &observation.legacy_git_context; @@ -1759,6 +1846,66 @@ exit 1 assert_eq!(bare_remote_branch_sha(&bare_origin, "feature"), target.sha); } + #[test] + fn stale_matching_tracking_ref_produces_a_branch_only_target() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + std::fs::create_dir_all(&workspace).unwrap(); + let bare_origin = init_bare_origin(temp.path()); + init_git_repo(&workspace, "feature", "https://github.com/acme/widgets"); + let local_url = format!("file://{}", bare_origin.display()); + run_git(&workspace, &[ + "config", + &format!("url.{local_url}.insteadOf"), + "https://github.com/acme/widgets", + ]); + run_git(&workspace, &["push", "origin", "feature"]); + run_git(&workspace, &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "--allow-empty", + "--quiet", + "-m", + "local-only", + ]); + mark_origin_branch_synced(&workspace, "feature"); + + let observation = + observe_git_run_target(&workspace, Some("https://github.com/acme/widgets")).unwrap(); + let target = observation.run_target.as_ref().unwrap(); + + assert_eq!(target.sha, None); + assert!(observation.legacy_git_context.sha.is_some()); + assert_ne!( + bare_remote_branch_sha(&bare_origin, "feature"), + observation.legacy_git_context.sha, + ); + } + + #[test] + fn branches_that_are_invalid_run_selectors_do_not_produce_git_targets() { + let temp = tempfile::tempdir().unwrap(); + let invalid_branches = [ + "heads/topic", + "tags/release", + "0123456789abcdef0123456789abcdef01234567", + ]; + + for (index, branch) in invalid_branches.into_iter().enumerate() { + let workspace = temp.path().join(format!("workspace-{index}")); + std::fs::create_dir_all(&workspace).unwrap(); + init_git_repo(&workspace, branch, "https://github.com/acme/widgets"); + + let observation = observe_git_run_target(&workspace, None).unwrap(); + + assert_eq!(observation.run_target, None, "branch {branch}"); + assert_eq!(observation.legacy_git_context.branch, branch); + } + } + #[test] fn failed_push_and_origin_mismatch_produce_branch_only_targets() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-manifest/src/local_workflow_package.rs b/lib/components/fabro-manifest/src/local_workflow_package.rs index 65cf5167e..5c32a4412 100644 --- a/lib/components/fabro-manifest/src/local_workflow_package.rs +++ b/lib/components/fabro-manifest/src/local_workflow_package.rs @@ -107,7 +107,10 @@ pub fn resolve_local_workflow_package( } fn is_workflow_name(workflow: &Path) -> bool { - workflow.extension().is_none() && workflow.components().count() == 1 + workflow.extension().is_none() + && workflow + .file_name() + .is_some_and(|name| workflow.as_os_str() == name) } fn resolve_named_workflow( @@ -364,6 +367,28 @@ mod tests { assert!(matches!(error, LocalWorkflowPackageError::Resolve { .. })); } + #[test] + fn dot_relative_directory_is_an_explicit_path_even_when_name_exists() { + let temp = tempfile::tempdir().unwrap(); + let project = temp.path().join("project"); + fs::create_dir_all(&project).unwrap(); + init_repo(&project); + write_workflow(&project, ".fabro/workflows/hello", "digraph Named {}"); + write_workflow(&project, "hello", "digraph Explicit {}"); + + let package = resolve_local_workflow_package(Path::new("./hello"), &project, None).unwrap(); + + assert_eq!(package.source_root(), project.canonicalize().unwrap()); + assert_eq!( + package.workflow_location().graph, + project.join("hello/workflow.fabro").canonicalize().unwrap(), + ); + assert_eq!( + root_version(&package).entrypoint().as_str(), + "hello/workflow.fabro", + ); + } + #[test] fn explicit_workflow_uses_its_own_checkout_and_has_location_independent_bytes() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 24a0e5420..a0fb815b6 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -84,19 +84,15 @@ 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| { - match source { - fabro_config::Error::WorkflowNotFound(_) => { - WorkflowVersionCollectError::WorkflowNotFound { - path: workflow.to_path_buf(), - } - } - source => WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source: source.into(), - }, - } - })?; + .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 diff --git a/lib/components/fabro-workflow/src/git.rs b/lib/components/fabro-workflow/src/git.rs index 9b11383cf..91318d6bd 100644 --- a/lib/components/fabro-workflow/src/git.rs +++ b/lib/components/fabro-workflow/src/git.rs @@ -234,6 +234,40 @@ pub fn push_branch_noninteractive(repo: &Path, remote: &str, branch: &str) -> Re ) } +/// Read the exact commit currently advertised for a remote branch without +/// allowing Git to prompt for credentials. +/// +/// This queries the remote itself rather than trusting the checkout's local +/// remote-tracking ref, which may be stale or may have been rewritten locally. +pub fn remote_branch_sha_noninteractive( + repo: &Path, + remote: &str, + branch: &str, +) -> Result> { + let branch_ref = format!("refs/heads/{branch}"); + let output = git_cmd(repo) + .env("GIT_TERMINAL_PROMPT", "0") + .args(["ls-remote", "--refs", remote, &branch_ref]) + .output() + .map_err(|e| Error::engine_with_source("git ls-remote failed", e))?; + if !output.status.success() { + return Err(git_error("git ls-remote failed")); + } + + let stdout = String::from_utf8_lossy(&output.stdout); + for line in stdout.lines() { + let mut fields = line.split_whitespace(); + let (Some(sha), Some(observed_ref), None) = (fields.next(), fields.next(), fields.next()) + else { + continue; + }; + if observed_ref == branch_ref { + return Ok(Some(sha.to_owned())); + } + } + Ok(None) +} + /// Push run and metadata branches to origin if a remote tracking branch exists. /// /// Callers supply pre-built refspecs so they control force-push (`+` prefix). diff --git a/lib/components/fabro-workflow/tests/it/git_integration.rs b/lib/components/fabro-workflow/tests/it/git_integration.rs index 59f9afdb8..a38a248e4 100644 --- a/lib/components/fabro-workflow/tests/it/git_integration.rs +++ b/lib/components/fabro-workflow/tests/it/git_integration.rs @@ -12,7 +12,7 @@ use fabro_agent::Sandbox; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_types::{RunEvent, WorkflowSettings, fixtures}; use fabro_workflow::event::Emitter; -use fabro_workflow::git::{branch_needs_push, push_branch, push_ref}; +use fabro_workflow::git; use fabro_workflow::handler::HandlerRegistry; use fabro_workflow::handler::exit::ExitHandler; use fabro_workflow::handler::start::StartHandler; @@ -178,7 +178,7 @@ fn push_ref_to_bare_remote() { rename_branch(&repo_dir, "test-push"); let url = format!("file://{}", remote_dir.display()); - push_ref(&repo_dir, &url, "refs/heads/test-push").unwrap(); + git::push_ref(&repo_dir, &url, "refs/heads/test-push").unwrap(); assert!(list_branch(&remote_dir, "test-push").contains("test-push")); } @@ -194,7 +194,7 @@ fn push_branch_to_remote() { add_origin(&repo_dir, &remote_dir); rename_branch(&repo_dir, "main"); - push_branch(&repo_dir, "origin", "main").unwrap(); + git::push_branch(&repo_dir, "origin", "main").unwrap(); assert!(list_branch(&remote_dir, "main").contains("main")); } @@ -210,10 +210,10 @@ fn branch_needs_push_when_ahead() { add_origin(&repo_dir, &remote_dir); rename_branch(&repo_dir, "main"); - push_branch(&repo_dir, "origin", "main").unwrap(); + git::push_branch(&repo_dir, "origin", "main").unwrap(); empty_commit(&repo_dir, "second"); - assert!(branch_needs_push(&repo_dir, "origin", "main")); + assert!(git::branch_needs_push(&repo_dir, "origin", "main")); } #[test] @@ -227,9 +227,39 @@ fn branch_needs_push_when_in_sync() { add_origin(&repo_dir, &remote_dir); rename_branch(&repo_dir, "main"); - push_branch(&repo_dir, "origin", "main").unwrap(); + git::push_branch(&repo_dir, "origin", "main").unwrap(); - assert!(!branch_needs_push(&repo_dir, "origin", "main")); + assert!(!git::branch_needs_push(&repo_dir, "origin", "main")); +} + +#[test] +fn remote_branch_sha_ignores_a_locally_rewritten_tracking_ref() { + let dir = tempfile::tempdir().unwrap(); + let repo_dir = dir.path().join("repo"); + let remote_dir = dir.path().join("remote.git"); + + init_bare_remote(&remote_dir); + init_repo(&repo_dir); + add_origin(&repo_dir, &remote_dir); + rename_branch(&repo_dir, "main"); + git::push_branch(&repo_dir, "origin", "main").unwrap(); + let remote_sha = git::head_sha(&repo_dir).unwrap(); + + empty_commit(&repo_dir, "local-only"); + let local_sha = git::head_sha(&repo_dir).unwrap(); + let update_tracking = Command::new("git") + .args(["update-ref", "refs/remotes/origin/main", "HEAD"]) + .current_dir(&repo_dir) + .output() + .expect("git update-ref should run"); + assert_success(&update_tracking, "git update-ref"); + assert!(!git::branch_needs_push(&repo_dir, "origin", "main")); + + assert_eq!( + git::remote_branch_sha_noninteractive(&repo_dir, "origin", "main").unwrap(), + Some(remote_sha.clone()), + ); + assert_ne!(local_sha, remote_sha); } #[tokio::test]