From 449c23629b7eee9c1dc328751ce68097b40badb4 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 8 Sep 2026 16:09:12 -0400 Subject: [PATCH] Simplify CLI workflow source and run target selection Remove validation that ran twice on the same inputs: clap already enforces the flag co-occurrence rules, and the native Git layer no longer re-checks selectors, branch names, refs, and commit SHAs that selection parsing already validated. Remote selector shape rules now delegate to the shared WorkflowPath validator. Reuse fabro_proc for the process-group kill and liveness probe instead of calling nix directly, dropping the extra nix features. Fold the duplicated branch/tag candidate derivation into one RefCandidates type, label each Git command explicitly instead of inferring it from argv, hoist the duplicated workflow resolver call in create_run, and merge the two directory target arms now that the default is just the caller path. Share the run-argument parser and workflow/commit fixtures across the unit tests through a test_support module, drop an integration test that duplicated one cell of the cross-product test, and make the malformed slug vectors assert the clap rejection they exercise. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/Cargo.toml | 2 +- lib/apps/fabro-cli/src/args.rs | 34 +-- lib/apps/fabro-cli/src/commands/run/create.rs | 30 +- lib/apps/fabro-cli/src/commands/run/mod.rs | 2 + .../src/commands/run/remote_workflow.rs | 267 +++++++++--------- .../fabro-cli/src/commands/run/resolution.rs | 132 +++------ .../fabro-cli/src/commands/run/selection.rs | 77 ++--- .../src/commands/run/test_support.rs | 62 ++++ lib/apps/fabro-cli/tests/it/cmd/create.rs | 51 ---- 9 files changed, 269 insertions(+), 388 deletions(-) create mode 100644 lib/apps/fabro-cli/src/commands/run/test_support.rs diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index f97e7c963..fa16a7375 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -99,7 +99,7 @@ object_store.workspace = true bytes.workspace = true tokio-util.workspace = true libc = "0.2" -nix = { version = "0.30", features = ["fs", "signal", "process"] } +nix = { version = "0.30", features = ["fs"] } [target.'cfg(target_os = "macos")'.dependencies] core-foundation = { version = "0.9", optional = true } diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index 06d947635..bccd95593 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -1162,6 +1162,8 @@ pub(crate) struct UpgradeArgs { #[derive(Subcommand)] pub(crate) enum RunCommands { + // Boxed so `RunArgs` does not dominate the size of the flattened + // `Commands` enum (clippy `large_enum_variant`). /// Register a workflow version, create a run, and start it Run(Box), /// Register a workflow version and create a submitted run @@ -1883,21 +1885,12 @@ pub(crate) struct CompletionArgs { #[cfg(test)] mod run_selection_grammar_tests { - use clap::Parser as _; - - use super::RunArgs; - - #[derive(clap::Parser)] - struct Command { - #[command(flatten)] - run: RunArgs, - } + use crate::commands::run::test_support::parse_run_args; #[test] fn run_selection_accepts_independent_resource_flags() { for flags in [ vec![ - "fabro", "review", "--workflow-git", "acme/workflows", @@ -1908,28 +1901,21 @@ mod run_selection_grammar_tests { "--target-branch", "release/topic", ], - vec!["fabro", "./review.toml", "--target-path", "../app"], + vec!["./review.toml", "--target-path", "../app"], ] { - assert!(Command::try_parse_from(flags).is_ok()); + assert!(parse_run_args(flags).is_ok()); } } #[test] fn run_selection_requires_modifier_owners_and_exclusive_targets() { for flags in [ - vec!["fabro", "review", "--workflow-ref", "v1"], - vec!["fabro", "review", "--target-branch", "release"], - vec![ - "fabro", - "review", - "--target-path", - ".", - "--target-git", - "acme/app", - ], - vec!["fabro", "--workflow-git", "acme/workflows"], + vec!["review", "--workflow-ref", "v1"], + vec!["review", "--target-branch", "release"], + vec!["review", "--target-path", ".", "--target-git", "acme/app"], + vec!["--workflow-git", "acme/workflows"], ] { - assert!(Command::try_parse_from(flags).is_err()); + assert!(parse_run_args(flags).is_err()); } } } diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index 380a19dea..e3e365e6f 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -36,19 +36,18 @@ pub(crate) async fn create_run( ) })?; let user_workflows_root = fabro_util::Home::from_env().workflows_dir(); + let resolve_workflow = || { + resolution::workflow( + &workflow_selection, + &canonical_cwd, + Some(&user_workflows_root), + ) + }; // Preserve local lookup diagnostics before contacting the server. Remote // acquisition waits until parent, environment, and target are validated. - let local_package = if matches!(workflow_selection, WorkflowSelection::Local(_)) { - Some( - resolution::workflow( - &workflow_selection, - &canonical_cwd, - Some(&user_workflows_root), - ) - .await?, - ) - } else { - None + let local_package = match &workflow_selection { + WorkflowSelection::Local(_) => Some(resolve_workflow().await?), + WorkflowSelection::Git { .. } => None, }; let prepared = prepare_intent_overrides(args, &canonical_cwd).await?; @@ -100,14 +99,7 @@ pub(crate) async fn create_run( } let package = match local_package { Some(package) => package, - None => { - resolution::workflow( - &workflow_selection, - &canonical_cwd, - Some(&user_workflows_root), - ) - .await? - } + None => resolve_workflow().await?, }; let workflow_version_id = package.closure().root_id(); client diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index fa0854a0c..6b9177673 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/lib/apps/fabro-cli/src/commands/run/mod.rs @@ -31,6 +31,8 @@ mod selection; pub(crate) mod ssh; pub(crate) mod start; pub(crate) mod steer; +#[cfg(test)] +pub(crate) mod test_support; pub(crate) mod wait; pub(crate) async fn dispatch( diff --git a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs index fc2760af1..8e9005abc 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -7,15 +7,12 @@ use std::time::Duration; use anyhow::{Context as _, bail}; use fabro_manifest::CollectedWorkflowClosure; use fabro_types::{GitHubRepositorySlug, GitRunTarget, repository}; -use nix::errno::Errno; -use nix::sys::signal::{self, Signal}; -use nix::unistd::Pid; use tokio::io::{AsyncRead, AsyncReadExt as _}; use tokio::process::Command; use tokio::{fs, signal as tokio_signal, task, time}; use tokio_util::sync::CancellationToken; -use super::selection::{self, RemoteWorkflowRevision}; +use super::selection::RemoteWorkflowRevision; const OUTPUT_LIMIT: usize = 64 * 1024; @@ -55,8 +52,10 @@ impl NativeGit { } } + /// Run one Git command; `operation` labels it in failure diagnostics. async fn command( &self, + operation: &'static str, cwd: &Path, args: &[&str], cancel: &CancellationToken, @@ -118,7 +117,7 @@ impl NativeGit { if !status.success() { // Output may contain arbitrary helper/config secrets, even after pattern // redaction. Never retain it in an error/cause chain or tracing event. - return Err(RemoteWorkflowError::Process { operation: operation(args), status }); + return Err(RemoteWorkflowError::Process { operation, status }); } if overflow { return Err(RemoteWorkflowError::OutputLimit); } Ok(stdout) @@ -126,24 +125,14 @@ impl NativeGit { }; // Helpers may inherit pipes or survive their Git parent. Terminate the // owned process group on both completion and interruption, then reap Git. - let mut cleanup_error = None; #[cfg(unix)] if let Some(id) = process_id { - let pid = Pid::from_raw(id.cast_signed()); - match signal::killpg(pid, Signal::SIGKILL) { - Ok(()) | Err(Errno::ESRCH) => {} - Err(source) => cleanup_error = Some(std::io::Error::from(source)), - } + fabro_proc::sigkill_process_group(id); } if result.is_err() { - if let Err(source) = child.kill().await { - cleanup_error = Some(source); - } + child.kill().await?; } child.wait().await?; - if let Some(source) = cleanup_error { - return Err(source.into()); - } result } @@ -156,7 +145,9 @@ impl NativeGit { let url = repository.https_url(); let mut args = vec!["ls-remote", "--symref", &url]; args.extend(patterns.iter().map(String::as_str)); - let bytes = self.command(&self.cwd, &args, cancel).await?; + let bytes = self + .command("metadata lookup", &self.cwd, &args, cancel) + .await?; Ok(std::str::from_utf8(&bytes) .context("local Git returned invalid metadata encoding")? .to_owned()) @@ -169,9 +160,6 @@ impl NativeGit { cancel: &CancellationToken, ) -> anyhow::Result { let (branch, sha) = if let Some(branch) = branch { - if !repository::is_valid_git_branch_name(&branch) { - bail!("invalid target working branch"); - } let reference = format!("refs/heads/{branch}"); let records = self .records(&repository, std::slice::from_ref(&reference), cancel) @@ -198,26 +186,17 @@ impl NativeGit { cancel: &CancellationToken, ) -> anyhow::Result { match revision { - RemoteWorkflowRevision::Commit(sha) => { - repository::normalize_git_commit_sha(sha).context("invalid full source commit SHA") - } + RemoteWorkflowRevision::Commit(sha) => Ok(sha.clone()), RemoteWorkflowRevision::DefaultBranch => { let records = self.records(repository, &["HEAD".into()], cancel).await?; Ok(default_head(&records)?.1) } RemoteWorkflowRevision::Ref(reference) => { - RemoteWorkflowRevision::parse(Some(reference))?; - let patterns = if reference.starts_with("refs/") { - vec![reference.clone(), format!("{reference}^{{}}")] - } else { - vec![ - format!("refs/heads/{reference}"), - format!("refs/tags/{reference}"), - format!("refs/tags/{reference}^{{}}"), - ] - }; - let records = self.records(repository, &patterns, cancel).await?; - resolve_named_ref(&records, reference) + let candidates = RefCandidates::new(reference); + let records = self + .records(repository, &candidates.patterns(), cancel) + .await?; + candidates.resolve(&records) } } } @@ -229,7 +208,6 @@ impl NativeGit { revision: RemoteWorkflowRevision, cancel: CancellationToken, ) -> anyhow::Result { - selection::validate_remote_selector(&selector)?; let sha = self .resolve_revision(&repository, &revision, &cancel) .await?; @@ -277,9 +255,15 @@ impl NativeGit { root: &Path, cancel: &CancellationToken, ) -> anyhow::Result<()> { - self.command(root, &["init", "--quiet", "--template="], cancel) - .await?; self.command( + "repository initialization", + root, + &["init", "--quiet", "--template="], + cancel, + ) + .await?; + self.command( + "fetch", root, &[ "fetch", @@ -293,7 +277,9 @@ impl NativeGit { cancel, ) .await?; - let kind = self.command(root, &["cat-file", "-t", sha], cancel).await?; + let kind = self + .command("object inspection", root, &["cat-file", "-t", sha], cancel) + .await?; if kind != b"commit\n" { bail!("the selected source SHA is not a commit"); } @@ -306,6 +292,7 @@ impl NativeGit { ) .await?; self.command( + "checkout", root, &[ "-c", @@ -320,7 +307,12 @@ impl NativeGit { ) .await?; let head = self - .command(root, &["rev-parse", "--verify", "HEAD"], cancel) + .command( + "checkout verification", + root, + &["rev-parse", "--verify", "HEAD"], + cancel, + ) .await?; if head != format!("{sha}\n").as_bytes() { bail!("workflow checkout did not match the selected commit"); @@ -329,15 +321,6 @@ impl NativeGit { } } -fn operation(args: &[&str]) -> &'static str { - match args.first().copied() { - Some("ls-remote") => "metadata lookup", - Some("fetch") => "fetch", - Some("checkout") => "checkout", - _ => "repository operation", - } -} - async fn capture(reader: &mut (impl AsyncRead + Unpin)) -> std::io::Result<(Vec, bool)> { let mut captured = Vec::new(); let mut overflow = false; @@ -394,48 +377,83 @@ fn default_head(records: &str) -> anyhow::Result<(String, String)> { )) } -fn resolve_named_ref(records: &str, reference: &str) -> anyhow::Result { - let heads = if reference.starts_with("refs/") { - reference.to_owned() - } else { - format!("refs/heads/{reference}") - }; - let tags = if reference.starts_with("refs/") { - reference.to_owned() - } else { - format!("refs/tags/{reference}") - }; - let branch = if heads.starts_with("refs/heads/") { - exact_record(records, &heads)? - } else { - None - }; - let tag = if tags.starts_with("refs/tags/") { - exact_record(records, &tags)? - } else { - None - }; - if branch.is_some() && tag.is_some() { - bail!( - "workflow ref is ambiguous between a branch and tag; use refs/heads/... or refs/tags/..." - ); +/// The fully qualified refs a validated `--workflow-ref` may name. A bare name +/// may be a branch or a tag; a `refs/heads/` or `refs/tags/` name is exactly +/// one. +struct RefCandidates { + branch: Option, + tag: Option, +} + +impl RefCandidates { + fn new(reference: &str) -> Self { + if reference.starts_with("refs/") { + let qualified = Some(reference.to_owned()); + if reference.starts_with("refs/heads/") { + Self { + branch: qualified, + tag: None, + } + } else { + Self { + branch: None, + tag: qualified, + } + } + } else { + Self { + branch: Some(format!("refs/heads/{reference}")), + tag: Some(format!("refs/tags/{reference}")), + } + } } - if let Some(sha) = branch { - return Ok(sha); + + /// `ls-remote` patterns, including the peeled form of any tag candidate. + fn patterns(&self) -> Vec { + self.branch + .iter() + .cloned() + .chain( + self.tag + .iter() + .flat_map(|tag| [tag.clone(), format!("{tag}^{{}}")]), + ) + .collect() } - if let Some(sha) = tag { - return Ok(exact_record(records, &format!("{tags}^{{}}"))?.unwrap_or(sha)); + + fn resolve(&self, records: &str) -> anyhow::Result { + let branch = match &self.branch { + Some(name) => exact_record(records, name)?, + None => None, + }; + let tag = match &self.tag { + Some(name) => exact_record(records, name)?.map(|sha| (name, sha)), + None => None, + }; + match (branch, tag) { + (Some(_), Some(_)) => bail!( + "workflow ref is ambiguous between a branch and tag; use refs/heads/... or refs/tags/..." + ), + (Some(sha), None) => Ok(sha), + // Prefer the peeled commit of an annotated tag over the tag object. + (None, Some((name, sha))) => { + Ok(exact_record(records, &format!("{name}^{{}}"))?.unwrap_or(sha)) + } + (None, None) => { + bail!("workflow ref was not found; no alternative revision was selected") + } + } } - bail!("workflow ref was not found; no alternative revision was selected") } fn check_source_symlinks(root: &Path) -> anyhow::Result<()> { let root = root.canonicalize()?; + let git_dir = root.join(".git"); // Validate links before the collector's initial TOML lookup can read them. for entry in walkdir::WalkDir::new(&root) .follow_links(false) .into_iter() - .filter_entry(|entry| entry.path() != root.join(".git")) + .filter_entry(|entry| entry.path() != git_dir) { let entry = entry?; if entry.file_type().is_symlink() && !entry.path().canonicalize()?.starts_with(&root) { @@ -479,6 +497,7 @@ mod tests { use nix::sys::stat::Mode; use nix::unistd; + use super::super::test_support::{commit_all, write_workflow}; use super::*; struct Fixture { @@ -497,30 +516,8 @@ mod tests { git2::RepositoryInitOptions::new().initial_head("trunk"), ) .unwrap(); - let workflow = repo_dir.join(".fabro/workflows/review"); - std::fs::create_dir_all(&workflow).unwrap(); - std::fs::write( - workflow.join("workflow.toml"), - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", - ) - .unwrap(); - std::fs::write( - workflow.join("workflow.fabro"), - "digraph Review { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", - ) - .unwrap(); - let mut index = repo.index().unwrap(); - index - .add_all(["."], git2::IndexAddOption::DEFAULT, None) - .unwrap(); - let tree_id = index.write_tree().unwrap(); - let sha = { - let tree = repo.find_tree(tree_id).unwrap(); - let signature = git2::Signature::now("Fixture", "fixture@example.test").unwrap(); - repo.commit(Some("HEAD"), &signature, &signature, "workflow", &tree, &[]) - .unwrap() - .to_string() - }; + write_workflow(&repo_dir, ".fabro/workflows/review"); + let sha = commit_all(&repo, "workflow"); let config = root.path().join("gitconfig"); std::fs::write( &config, @@ -549,30 +546,6 @@ mod tests { } } - impl Fixture { - fn commit_changes(&self) -> String { - let mut index = self.repo.index().unwrap(); - index - .add_all(["."], git2::IndexAddOption::DEFAULT, None) - .unwrap(); - let tree_id = index.write_tree().unwrap(); - let tree = self.repo.find_tree(tree_id).unwrap(); - let parent = self.repo.head().unwrap().peel_to_commit().unwrap(); - let signature = git2::Signature::now("Fixture", "fixture@example.test").unwrap(); - self.repo - .commit( - Some("HEAD"), - &signature, - &signature, - "update workflow", - &tree, - &[&parent], - ) - .unwrap() - .to_string() - } - } - #[tokio::test] async fn remote_workflow_cleanup_preserves_sibling_identity_and_disables_filters_hooks() { let fixture = Fixture::new(); @@ -610,7 +583,7 @@ mod tests { &format!("touch '{}'; cat", sentinel.display()), ) .unwrap(); - let sha = fixture.commit_changes(); + let sha = commit_all(&fixture.repo, "update workflow"); let local = fabro_manifest::collect_workflow_versions(Path::new("review"), source).unwrap(); assert_eq!(local.versions().count(), 2); for selector in [ @@ -676,7 +649,7 @@ mod tests { .join(".fabro/workflows/review/workflow.toml"); std::fs::remove_file(&path).unwrap(); std::os::unix::fs::symlink(&fifo, path).unwrap(); - let sha = fixture.commit_changes(); + let sha = commit_all(&fixture.repo, "update workflow"); let error = fixture .git .collect( @@ -712,7 +685,7 @@ mod tests { }) .await .unwrap(); - let pid: i32 = std::fs::read_to_string(path.join("pid")) + let pid: u32 = std::fs::read_to_string(path.join("pid")) .unwrap() .parse() .unwrap(); @@ -725,7 +698,7 @@ mod tests { }) .await .unwrap(); - assert_eq!(signal::kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)); + assert!(!fabro_proc::process_exists(pid)); drop(fake); } @@ -936,7 +909,11 @@ mod tests { #[test] fn remote_workflow_matches_records_exactly_and_rejects_host_symlinks() { let sha = "1234567890123456789012345678901234567890"; - assert!(resolve_named_ref(&format!("{sha}\trefs/heads/nested/main\n"), "main").is_err()); + assert!( + RefCandidates::new("main") + .resolve(&format!("{sha}\trefs/heads/nested/main\n")) + .is_err() + ); let root = tempfile::tempdir().unwrap(); let host = tempfile::tempdir().unwrap(); std::os::unix::fs::symlink(host.path(), root.path().join("outside")).unwrap(); @@ -960,7 +937,7 @@ mod tests { "i=0; while [ $i -lt 9000 ]; do printf 'sentinel-secret-plain-text\\n'; printf 'sentinel-secret-plain-text\\n' >&2; i=$((i+1)); done; exit 42", ); let error = git - .command(root.path(), &["fetch"], &CancellationToken::new()) + .command("fetch", root.path(), &["fetch"], &CancellationToken::new()) .await .unwrap_err(); assert!( @@ -971,9 +948,14 @@ mod tests { "i=0; while [ $i -lt 9000 ]; do printf 'metadata-output\\n'; i=$((i+1)); done", ); assert!(matches!( - git.command(root.path(), &["ls-remote"], &CancellationToken::new()) - .await - .unwrap_err(), + git.command( + "metadata lookup", + root.path(), + &["ls-remote"], + &CancellationToken::new() + ) + .await + .unwrap_err(), RemoteWorkflowError::OutputLimit )); } @@ -996,17 +978,20 @@ mod tests { cancel.cancel(); } }; - let (result, ()) = tokio::join!(git.command(root.path(), &["fetch"], &cancel), trigger); + let (result, ()) = tokio::join!( + git.command("fetch", root.path(), &["fetch"], &cancel), + trigger + ); let error = result.unwrap_err(); assert!(matches!( error, RemoteWorkflowError::Timeout | RemoteWorkflowError::Cancelled )); - let pid: i32 = std::fs::read_to_string(root.path().join("pid")) + let pid: u32 = std::fs::read_to_string(root.path().join("pid")) .unwrap() .parse() .unwrap(); - assert_eq!(signal::kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)); + assert!(!fabro_proc::process_exists(pid)); } } } diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index 57bd458af..c5a034302 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; use anyhow::{Context as _, anyhow, bail}; use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; @@ -71,18 +71,11 @@ pub(super) async fn target( provider: EnvironmentProvider, cwd: &Path, ) -> anyhow::Result<(RunTarget, bool)> { - match selection { - TargetSelection::CurrentDirectory => observe_directory(provider, cwd.to_path_buf()).await, - TargetSelection::Path(path) => { - let path = cwd - .join(path) - .canonicalize() - .context("failed to canonicalize target directory")?; - if !path.is_dir() { - bail!("target path must be a directory"); - } - observe_directory(provider, path).await - } + let path = match selection { + TargetSelection::Path(path) => cwd + .join(path) + .canonicalize() + .context("failed to canonicalize target directory")?, TargetSelection::Git { repository, branch } => { if !provider.is_clone_based() { bail!("Git targets require a clone-enabled Docker or Daytona environment"); @@ -94,15 +87,12 @@ pub(super) async fn target( }) .await?; // Canonical admission retains ownership of provider capabilities. - Ok((RunTarget::Git(target), false)) + return Ok((RunTarget::Git(target), false)); } + }; + if !path.is_dir() { + bail!("target path must be a directory"); } -} - -async fn observe_directory( - provider: EnvironmentProvider, - path: PathBuf, -) -> anyhow::Result<(RunTarget, bool)> { // The existing observer can push/query Git synchronously. Preserve its // behavior without blocking a Tokio worker or promising a new timeout. task::spawn_blocking(move || run_target_for_environment(provider, &path)) @@ -199,67 +189,18 @@ fn none_target_for_unversioned_directory(canonical_cwd: &Path) -> anyhow::Result reason = "resolver tests construct small local workflow fixtures" )] mod tests { - use clap::Parser as _; - - use super::super::selection; + use super::super::test_support::write_workflow; use super::*; - use crate::args::RunArgs; - - fn write_workflow(root: &Path, name: &str) { - let dir = root.join(name); - std::fs::create_dir_all(&dir).unwrap(); - std::fs::write( - dir.join("workflow.toml"), - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", - ) - .unwrap(); - std::fs::write( - dir.join("workflow.fabro"), - "digraph Test { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", - ) - .unwrap(); - } - - #[derive(clap::Parser)] - struct Command { - #[command(flatten)] - args: RunArgs, - } #[tokio::test] - async fn run_selection_direct_and_parsed_resolve_identically() { + async fn run_selection_target_resolution_by_provider() { let caller = tempfile::tempdir().unwrap(); let root = caller.path().canonicalize().unwrap(); write_workflow(&root, ".fabro/workflows/review"); std::fs::create_dir(root.join("target")).unwrap(); - let args = Command::try_parse_from(["cmd", "review", "--target-path", "target"]).unwrap(); - let (parsed_workflow, parsed_target) = selection::parse(&args.args).unwrap(); - let direct_workflow = WorkflowSelection::Local("review".into()); - let direct_target = TargetSelection::Path("target".into()); + let selected = TargetSelection::Path("target".into()); assert_eq!( - workflow(&parsed_workflow, &root, None) - .await - .unwrap() - .closure() - .root_id(), - workflow(&direct_workflow, &root, None) - .await - .unwrap() - .closure() - .root_id() - ); - for provider in [ - EnvironmentProvider::Local, - EnvironmentProvider::Docker, - EnvironmentProvider::Daytona, - ] { - assert_eq!( - target(&parsed_target, provider, &root).await.unwrap(), - target(&direct_target, provider, &root).await.unwrap() - ); - } - assert_eq!( - target(&direct_target, EnvironmentProvider::Local, &root) + target(&selected, EnvironmentProvider::Local, &root) .await .unwrap() .0, @@ -267,16 +208,15 @@ mod tests { path: root.join("target").to_str().unwrap().into(), } ); - assert_eq!( - target(&direct_target, EnvironmentProvider::Docker, &root) - .await - .unwrap() - .0, - RunTarget::None {} - ); + for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { + assert_eq!( + target(&selected, provider, &root).await.unwrap().0, + RunTarget::None {} + ); + } assert_eq!( target( - &TargetSelection::CurrentDirectory, + &TargetSelection::Path(".".into()), EnvironmentProvider::Local, &root ) @@ -301,24 +241,18 @@ mod tests { .to_string() .contains("Docker or Daytona") ); - assert!( - target( - &TargetSelection::Path("missing".into()), - EnvironmentProvider::Local, - &root - ) - .await - .is_err() - ); - assert!( - target( - &TargetSelection::Path(".fabro/workflows/review/workflow.toml".into()), - EnvironmentProvider::Local, - &root - ) - .await - .is_err() - ); + for path in ["missing", ".fabro/workflows/review/workflow.toml"] { + assert!( + target( + &TargetSelection::Path(path.into()), + EnvironmentProvider::Local, + &root + ) + .await + .is_err(), + "{path}" + ); + } } #[tokio::test] diff --git a/lib/apps/fabro-cli/src/commands/run/selection.rs b/lib/apps/fabro-cli/src/commands/run/selection.rs index 038fde822..409936aa9 100644 --- a/lib/apps/fabro-cli/src/commands/run/selection.rs +++ b/lib/apps/fabro-cli/src/commands/run/selection.rs @@ -1,9 +1,9 @@ //! CLI syntax ends here. Resolvers receive selections and explicit caller //! context. -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use anyhow::{Context as _, bail}; -use fabro_types::{GitHubRepositorySlug, repository}; +use fabro_types::{GitHubRepositorySlug, WorkflowPath, repository}; use crate::args::RunArgs; @@ -19,7 +19,8 @@ pub(super) enum WorkflowSelection { #[derive(Debug, Clone, PartialEq, Eq)] pub(super) enum TargetSelection { - CurrentDirectory, + /// Directory relative to the caller; the default is the caller directory + /// itself. Path(PathBuf), Git { repository: GitHubRepositorySlug, @@ -59,43 +60,25 @@ pub(super) fn validate_remote_selector(path: &Path) -> anyhow::Result<()> { let value = path .to_str() .context("remote workflow selector must be valid UTF-8")?; - if value.is_empty() - || value.contains('\\') - || value.chars().any(char::is_control) - || path - .components() - .any(|part| !matches!(part, Component::Normal(_) | Component::CurDir)) - || value.split('/').any(|part| part == "..") - { + let value = value.strip_prefix("./").unwrap_or(value); + if WorkflowPath::new(value).is_err() { bail!( "remote workflow must be a name or repository-relative .fabro/.toml file without traversal" ); } - match path.extension().and_then(|ext| ext.to_str()) { - Some("toml" | "fabro") => {} - None if path - .file_name() - .is_some_and(|name| path.as_os_str() == name) - && value != "." - && !value.starts_with('-') => {} + let is_bare_name = !value.contains('/') && !value.starts_with('-'); + match Path::new(value).extension().and_then(|ext| ext.to_str()) { + Some("toml" | "fabro") => Ok(()), + None if is_bare_name => Ok(()), _ => bail!( "remote workflow must be a name or explicit .fabro/.toml file; directories are ambiguous" ), } - Ok(()) } pub(super) fn parse(args: &RunArgs) -> anyhow::Result<(WorkflowSelection, TargetSelection)> { + // Flag co-occurrence rules (`requires`/`conflicts_with`) are enforced by clap. let workflow = args.workflow.as_ref().context("workflow is required")?; - if args.workflow_ref.is_some() && args.workflow_git.is_none() { - bail!("--workflow-ref requires --workflow-git"); - } - if args.target_branch.is_some() && args.target_git.is_none() { - bail!("--target-branch requires --target-git"); - } - if args.target_path.is_some() && args.target_git.is_some() { - bail!("--target-path conflicts with --target-git"); - } let workflow = match &args.workflow_git { None => WorkflowSelection::Local(workflow.clone()), Some(repository) => { @@ -124,7 +107,7 @@ pub(super) fn parse(args: &RunArgs) -> anyhow::Result<(WorkflowSelection, Target branch: args.target_branch.clone(), } } - _ => TargetSelection::CurrentDirectory, + _ => TargetSelection::Path(PathBuf::from(".")), }; Ok((workflow, target)) } @@ -184,17 +167,10 @@ mod tests { #[cfg(test)] mod adapter_tests { - use clap::Parser as _; - + use super::super::test_support::parse_run_args; use super::*; use crate::args::{Cli, Commands, RunCommands}; - #[derive(clap::Parser)] - struct Command { - #[command(flatten)] - args: RunArgs, - } - #[test] fn run_selection_both_commands_share_the_adapter() { for command in ["run", "create"] { @@ -240,23 +216,21 @@ mod adapter_tests { #[test] fn run_selection_adapter_rejects_invalid_inputs_without_acquisition() { + // Malformed repository slugs never reach the adapter. for flags in [ - vec![ - "cmd", + [ "review", "--workflow-git", "https://github.com/acme/workflows", ], - vec!["cmd", "review", "--target-git", "acme/app/extra"], - vec!["cmd", "../review.toml", "--workflow-git", "acme/workflows"], + ["review", "--target-git", "acme/app/extra"], + ] { + assert!(parse_run_args(flags).is_err()); + } + for flags in [ + vec!["../review.toml", "--workflow-git", "acme/workflows"], + vec!["/tmp/review.toml", "--workflow-git", "acme/workflows"], vec![ - "cmd", - "/tmp/review.toml", - "--workflow-git", - "acme/workflows", - ], - vec![ - "cmd", "review", "--workflow-git", "acme/workflows", @@ -264,7 +238,6 @@ mod adapter_tests { "HEAD~1", ], vec![ - "cmd", "review", "--target-git", "acme/app", @@ -272,7 +245,6 @@ mod adapter_tests { "refs/tags/v1", ], vec![ - "cmd", "review", "--target-git", "acme/app", @@ -280,9 +252,8 @@ mod adapter_tests { "1234567890123456789012345678901234567890", ], ] { - if let Ok(command) = Command::try_parse_from(flags) { - assert!(parse(&command.args).is_err()); - } + let args = parse_run_args(flags.iter().copied()).unwrap(); + assert!(parse(&args).is_err(), "{flags:?}"); } } } diff --git a/lib/apps/fabro-cli/src/commands/run/test_support.rs b/lib/apps/fabro-cli/src/commands/run/test_support.rs new file mode 100644 index 000000000..ee08ba847 --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/test_support.rs @@ -0,0 +1,62 @@ +//! Fixtures shared by the run selection, resolution, and remote workflow +//! unit tests. +#![expect( + clippy::disallowed_methods, + reason = "test fixtures write small files synchronously" +)] +use std::path::Path; + +use clap::Parser as _; + +use crate::args::RunArgs; + +#[derive(clap::Parser)] +struct Command { + #[command(flatten)] + args: RunArgs, +} + +/// Parse `fabro run`/`fabro create` arguments exactly as clap would. +pub(crate) fn parse_run_args<'a>( + args: impl IntoIterator, +) -> Result { + Command::try_parse_from(std::iter::once("cmd").chain(args)).map(|command| command.args) +} + +/// Write a minimal two-file workflow package under `root/name`. +pub(super) fn write_workflow(root: &Path, name: &str) { + let dir = root.join(name); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("workflow.toml"), + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .unwrap(); + std::fs::write( + dir.join("workflow.fabro"), + "digraph Test { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); +} + +/// Stage every file in the worktree and commit it on HEAD, returning the SHA. +pub(super) fn commit_all(repo: &git2::Repository, message: &str) -> String { + let mut index = repo.index().unwrap(); + index + .add_all(["."], git2::IndexAddOption::DEFAULT, None) + .unwrap(); + let tree = repo.find_tree(index.write_tree().unwrap()).unwrap(); + let parent = repo.head().ok().and_then(|head| head.peel_to_commit().ok()); + let parents: Vec<_> = parent.iter().collect(); + let signature = git2::Signature::now("Fixture", "fixture@example.test").unwrap(); + repo.commit( + Some("HEAD"), + &signature, + &signature, + message, + &tree, + &parents, + ) + .unwrap() + .to_string() +} diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 93716c01e..b94df6061 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -1639,57 +1639,6 @@ draft = false assert!(!pull_request.draft); } -#[test] -fn run_selection_target_path_keeps_caller_workflow_and_goal() { - let context = test_context!(); - let server = MockServer::start(); - let environment = mock_environment(&server, "local", "local"); - let versions = mock_workflow_version_registrations(&server); - let requests = Arc::new(Mutex::new(Vec::new())); - let create = mock_intent_create(&server, &unique_run_id(), Arc::clone(&requests)); - let caller = tempfile::tempdir().unwrap(); - let target = caller.path().join("target"); - write_workflow(caller.path(), ".fabro/workflows/review", "Caller"); - write_workflow(&target, ".fabro/workflows/review", "Target"); - std::fs::write(caller.path().join("goal.txt"), "Caller goal").unwrap(); - std::fs::write(target.join("goal.txt"), "Target goal").unwrap(); - let expected = fabro_manifest::resolve_local_workflow_package( - std::path::Path::new("review"), - caller.path(), - None, - ) - .unwrap() - .closure() - .root_id(); - let output = context - .create_cmd() - .current_dir(caller.path()) - .args([ - "review", - "--target-path", - "target", - "--goal-file", - "goal.txt", - "--environment", - "local", - "--server", - &format!("{}/api/v1", server.base_url()), - ]) - .output() - .unwrap(); - assert!(output.status.success(), "{}", output_stderr(&output)); - environment.assert(); - versions.assert(); - create.assert(); - let requests = requests.lock().unwrap(); - assert_eq!(requests[0]["workflow_version_id"], expected.to_string()); - assert_eq!( - requests[0]["target"], - json!({"kind": "folder", "path": target.canonicalize().unwrap()}) - ); - assert_eq!(requests[0]["goal"], "Caller goal"); -} - fn init_remote_fixture(path: &std::path::Path, branch: &str) -> String { let repo = git2::Repository::init_opts( path,