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]