scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-10 22:43:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Simplify CLI workflow source and run target selection

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Scott Werner 2026-09-08 16:09:12 -04:00
parent dc768888d0
commit 449c23629b
9 changed files with 269 additions and 388 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/apps/fabro-cli/Cargo.toml
View file

@ -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 }

34
lib/apps/fabro-cli/src/args.rs
View file

@ -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<RunArgs>),
/// 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());
}
}
}

30
lib/apps/fabro-cli/src/commands/run/create.rs
View file

@ -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

2
lib/apps/fabro-cli/src/commands/run/mod.rs
View file

@ -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(

267
lib/apps/fabro-cli/src/commands/run/remote_workflow.rs
View file

@ -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<GitRunTarget> {
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<String> {
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<CollectedWorkflowClosure> {
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<u8>, 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<String> {
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<String>,
tag: Option<String>,
}
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<String> {
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<String> {
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));
}
}
}

132
lib/apps/fabro-cli/src/commands/run/resolution.rs
View file

@ -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]

77
lib/apps/fabro-cli/src/commands/run/selection.rs
View file

@ -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:?}");
}
}
}

62
lib/apps/fabro-cli/src/commands/run/test_support.rs Normal file
View file

@ -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<Item = &'a str>,
) -> Result<RunArgs, clap::Error> {
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()
}

51
lib/apps/fabro-cli/tests/it/cmd/create.rs
View file

@ -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,