From 0e0d9866347b7b1c287babc61e1aa01b73319b67 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 8 Sep 2026 12:05:01 -0400 Subject: [PATCH 01/15] Select CLI workflow sources and run targets independently --- docs/public/core-concepts/workflows.mdx | 68 ++ docs/public/reference/cli.mdx | 26 +- lib/apps/fabro-cli/Cargo.toml | 2 +- lib/apps/fabro-cli/src/args.rs | 92 +- lib/apps/fabro-cli/src/commands/run/create.rs | 63 +- lib/apps/fabro-cli/src/commands/run/mod.rs | 5 +- .../fabro-cli/src/commands/run/overrides.rs | 5 + .../src/commands/run/remote_workflow.rs | 1012 +++++++++++++++++ .../fabro-cli/src/commands/run/resolution.rs | 358 ++++++ .../fabro-cli/src/commands/run/selection.rs | 288 +++++ lib/apps/fabro-cli/tests/it/cmd/create.rs | 363 +++++- lib/apps/fabro-cli/tests/it/cmd/fabro.rs | 4 +- lib/apps/fabro-cli/tests/it/cmd/run.rs | 13 +- 13 files changed, 2249 insertions(+), 50 deletions(-) create mode 100644 lib/apps/fabro-cli/src/commands/run/remote_workflow.rs create mode 100644 lib/apps/fabro-cli/src/commands/run/resolution.rs create mode 100644 lib/apps/fabro-cli/src/commands/run/selection.rs diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 558334219..4119ef9e4 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -140,3 +140,71 @@ In the web UI, the Workflows page lists all available workflows. Click into a wo See the [Quick Start](/getting-started/quick-start) to try it out, or browse the [example workflows](/examples/repl-handoff) for real-world patterns. + +## Select workflow source and run target + +`fabro run` and `fabro create` accept the same source and target options. `create` +registers the workflow and leaves a submitted run for you to start with +`fabro start RUN`. `run` also starts it, then attaches unless you pass `--detach`. + +The required positional argument selects the workflow. With no source flags, +names are found in the current checkout, then a marked project, then installed +user workflows. Explicit local paths retain their usual package roots. + +```sh +fabro run review +fabro create ./review.toml --target-path ../app +fabro run review --workflow-git acme/workflows --workflow-ref v1.2 \ + --target-git acme/app --target-branch release +``` + +In the last example, workflow instructions come from `acme/workflows`, and the +run works on `acme/app`. Neither selection changes the other. Local workflow paths, +`--goal-file`, and other caller inputs still resolve from the invocation context. +Without target flags, Fabro keeps its existing cwd/environment-based target +inference. + +`--workflow-git OWNER/REPO` acquires source using native Git on your machine. A +workflow name selects `.fabro/workflows/NAME/workflow.toml` in that repository; +you can also supply an explicit repository-relative `.toml` or `.fabro` file. +Absolute paths, traversal, and directory selectors are rejected. A missing remote +workflow never falls back to a local or installed workflow. + +`--workflow-ref` requires `--workflow-git`. Omit it, or use `HEAD`, to select the +remote default branch. You can select a branch, tag, or full 40-hex commit SHA. +If a branch and tag share a name, qualify it with `refs/heads/` or `refs/tags/`. +Fabro resolves the revision once, fetches that exact commit into a temporary +checkout, and registers its workflow-version closure. A moved or unavailable +commit never causes a fallback to a newer revision. Checkout hooks, content +filters, and implicit Git LFS expansion are disabled; submodules are not fetched. +Temporary files are removed after collection or failure. Interrupting acquisition +stops owned Git processes before cleanup; collection already in progress must +finish before its files can be removed. + +Target selection depends on the environment: + +| Selection | Local environment | Docker/Daytona environment | +| --- | --- | --- | +| Default cwd or `--target-path PATH` | Uses the live directory, including uncommitted files | Uses the enclosing Git repository and exact available commit; a non-Git directory selects an empty workspace | +| `--target-git OWNER/REPO` | Rejected | Uses the selected repository and exact observed branch commit; cloning must be enabled | + +For clone-based execution, a target path selects a repository, not a subdirectory +working-directory override. Local target files are not uploaded. Existing target +observation may push committed local changes to origin; dirty changes are +excluded from clone targets and produce a warning. Detached or unavailable exact +commits fail. Folder targets require the directory to be accessible to the +server and its Local execution environment; passing a caller-local path does not +transfer it to a remote server. + +`--target-branch` requires `--target-git` and accepts a working branch name, not a +tag or SHA. Without it, Fabro resolves the repository's default branch. The CLI +only looks up target metadata; the execution sandbox clones the target. +`--target-path` and `--target-git` conflict. + +Local Git credential helpers, SSH-agent access through configured URL rewrites, +and user network configuration govern source acquisition and remote target +lookup. Fabro server login does not grant local Git access. The execution +sandbox still needs its own target-clone credentials. + +`--dry-run` simulates execution; it can still fetch and upload workflow source, +and existing local target observation can still publish committed changes. diff --git a/docs/public/reference/cli.mdx b/docs/public/reference/cli.mdx index c5a0ff1d4..8a4822416 100644 --- a/docs/public/reference/cli.mdx +++ b/docs/public/reference/cli.mdx @@ -70,7 +70,7 @@ fabro [OPTIONS] [COMMAND] | `fabro attach` | Attach to a running or finished workflow run | | `fabro auth` | Manage CLI authentication state | | `fabro completion` | Generate shell completions | -| `fabro create` | Register a local workflow version and create a submitted run | +| `fabro create` | Register a workflow version and create a submitted run | | `fabro deny` | Deny pending workflow runs | | `fabro discord` | Open the Discord community in the browser | | `fabro docs` | Open the docs website in the browser | @@ -92,7 +92,7 @@ fabro [OPTIONS] [COMMAND] | `fabro resume` | Resume an interrupted workflow run | | `fabro rewind` | Rewind a workflow run to an earlier checkpoint | | `fabro rm` | Remove one or more workflow runs | -| `fabro run` | Register a local workflow version, create a run, and start it | +| `fabro run` | Register a workflow version, create a run, and start it | | `fabro sandbox` | Sandbox operations (cp, ssh, preview) | | `fabro secret` | Manage server-owned secrets | | `fabro server` | Server operations | @@ -332,7 +332,7 @@ fabro completion [OPTIONS] ### `fabro create` -Register a local workflow version and create a submitted run +Register a workflow version and create a submitted run ```bash fabro create [OPTIONS] @@ -342,7 +342,7 @@ fabro create [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Local workflow name, checkout path, .fabro file, or workflow TOML | +| `WORKFLOW` | Workflow name or path (repository-relative with --workflow-git) | #### Options @@ -350,7 +350,7 @@ fabro create [OPTIONS] | --- | --- | | `--auto-approve` | Auto-approve all human gates | | `-d, --detach` | Run the workflow in the background and print the run ID | -| `--dry-run` | Execute with simulated LLM backend | +| `--dry-run` | Simulate execution; workflow source may still be fetched and uploaded | | `--environment ` | Named environment for agent tools | | `--goal ` | Override the workflow goal (available as {{ goal }} in prompts) | | `--goal-file ` | Read a per-run goal value from a local file | @@ -360,8 +360,13 @@ fabro create [OPTIONS] | `--preserve-sandbox` | Keep the sandbox alive after the run finishes (for debugging) | | `--provider ` | Override default LLM provider | | `--server ` | Fabro server target: http(s) URL or absolute Unix socket path | +| `--target-branch ` | Target working branch (default: remote default branch), pinned to its observed commit | +| `--target-git ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | +| `--target-path ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | | `-I, --input ` | Override a workflow input value (repeatable, format: KEY=VALUE) | | `-v, --verbose` | Enable verbose output | +| `--workflow-git ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | +| `--workflow-ref ` | Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names | ### `fabro deny` @@ -1061,7 +1066,7 @@ fabro rm [OPTIONS] ... ### `fabro run` -Register a local workflow version, create a run, and start it +Register a workflow version, create a run, and start it ```bash fabro run [OPTIONS] @@ -1071,7 +1076,7 @@ fabro run [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Local workflow name, checkout path, .fabro file, or workflow TOML | +| `WORKFLOW` | Workflow name or path (repository-relative with --workflow-git) | #### Options @@ -1079,7 +1084,7 @@ fabro run [OPTIONS] | --- | --- | | `--auto-approve` | Auto-approve all human gates | | `-d, --detach` | Run the workflow in the background and print the run ID | -| `--dry-run` | Execute with simulated LLM backend | +| `--dry-run` | Simulate execution; workflow source may still be fetched and uploaded | | `--environment ` | Named environment for agent tools | | `--goal ` | Override the workflow goal (available as {{ goal }} in prompts) | | `--goal-file ` | Read a per-run goal value from a local file | @@ -1089,8 +1094,13 @@ fabro run [OPTIONS] | `--preserve-sandbox` | Keep the sandbox alive after the run finishes (for debugging) | | `--provider ` | Override default LLM provider | | `--server ` | Fabro server target: http(s) URL or absolute Unix socket path | +| `--target-branch ` | Target working branch (default: remote default branch), pinned to its observed commit | +| `--target-git ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | +| `--target-path ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | | `-I, --input ` | Override a workflow input value (repeatable, format: KEY=VALUE) | | `-v, --verbose` | Enable verbose output | +| `--workflow-git ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | +| `--workflow-ref ` | Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names | ### `fabro sandbox` diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 3d43c52f1..296dd81a0 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -101,7 +101,7 @@ object_store.workspace = true bytes.workspace = true tokio-util.workspace = true libc = "0.2" -nix = { version = "0.30", features = ["fs"] } +nix = { version = "0.30", features = ["fs", "signal", "process"] } [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 16435b955..91048ea6c 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -6,7 +6,7 @@ use clap::{Args, Parser, Subcommand, ValueEnum}; use fabro_config::{CliLayer, CliLoggingLayer, CliOutputLayer, CliUpdatesLayer}; use fabro_server::serve::DEFAULT_TCP_PORT; use fabro_static::EnvVars; -use fabro_types::PermissionLevel; +use fabro_types::{GitHubRepositorySlug, PermissionLevel}; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::run::MergeStrategy; use fabro_util::printer::Printer; @@ -233,11 +233,36 @@ pub(crate) struct RunArgs { #[command(flatten)] pub(crate) inputs: InputOverrideArgs, - /// Local workflow name, checkout path, .fabro file, or workflow TOML + /// Workflow name or path (repository-relative with --workflow-git) #[arg(required = true)] pub(crate) workflow: Option, - /// Execute with simulated LLM backend + /// Acquire workflow source locally from a GitHub OWNER/REPO using native + /// Git credentials + #[arg(long, value_name = "OWNER/REPO")] + pub(crate) workflow_git: Option, + + /// Workflow branch, tag, HEAD (default), or full commit SHA; qualify + /// ambiguous names + #[arg(long, requires = "workflow_git", value_name = "REF")] + pub(crate) workflow_ref: Option, + + /// Observe this target directory instead of cwd; Folder targets require + /// server filesystem access + #[arg(long, conflicts_with = "target_git", value_name = "PATH")] + pub(crate) target_path: Option, + + /// Target GitHub OWNER/REPO; the execution sandbox still needs its own + /// clone credentials + #[arg(long, value_name = "OWNER/REPO")] + pub(crate) target_git: Option, + + /// Target working branch (default: remote default branch), pinned to its + /// observed commit + #[arg(long, requires = "target_git", value_name = "BRANCH")] + pub(crate) target_branch: Option, + + /// Simulate execution; workflow source may still be fetched and uploaded #[arg(long)] pub(crate) dry_run: bool, @@ -1243,10 +1268,10 @@ pub(crate) struct UpgradeArgs { #[derive(Subcommand)] pub(crate) enum RunCommands { - /// Register a local workflow version, create a run, and start it - Run(RunArgs), - /// Register a local workflow version and create a submitted run - Create(RunArgs), + /// Register a workflow version, create a run, and start it + Run(Box), + /// Register a workflow version and create a submitted run + Create(Box), /// Start a created workflow run on the server Start(StartArgs), /// Attach to a running or finished workflow run @@ -1974,3 +1999,56 @@ fn parse_reasoning_effort_arg(value: &str) -> Result { ) }) } + +#[cfg(test)] +mod run_selection_grammar_tests { + use clap::Parser as _; + + use super::RunArgs; + + #[derive(clap::Parser)] + struct Command { + #[command(flatten)] + run: RunArgs, + } + + #[test] + fn run_selection_accepts_independent_resource_flags() { + for flags in [ + vec![ + "fabro", + "review", + "--workflow-git", + "acme/workflows", + "--workflow-ref", + "refs/tags/v1", + "--target-git", + "acme/app", + "--target-branch", + "release/topic", + ], + vec!["fabro", "./review.toml", "--target-path", "../app"], + ] { + assert!(Command::try_parse_from(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"], + ] { + assert!(Command::try_parse_from(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 10365ca4a..380a19dea 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -7,6 +7,9 @@ use fabro_types::{RunId, RunIntent}; use fabro_util::terminal::Styles; use super::overrides::prepare_intent_overrides; +use super::resolution::ResolvedWorkflow; +use super::selection::WorkflowSelection; +use super::{resolution, selection}; use crate::args::RunArgs; use crate::command_context::CommandContext; use crate::commands::resolve_run_id; @@ -16,7 +19,7 @@ pub(crate) struct CreatedRun { pub(crate) run_id: RunId, } -/// Register the local workflow version closure with the server and create a +/// Register the workflow version closure with the server and create a /// run from an immutable workflow intent, leaving it in the submitted state. /// /// This does NOT start the workflow — starting is a separate request. @@ -25,10 +28,7 @@ pub(crate) async fn create_run( args: &RunArgs, styles: &Styles, ) -> anyhow::Result { - let workflow_path = args - .workflow - .as_ref() - .ok_or_else(|| anyhow::anyhow!("--workflow is required"))?; + let (workflow_selection, target_selection) = selection::parse(args)?; let canonical_cwd = ctx.cwd().canonicalize().with_context(|| { format!( "failed to canonicalize caller working directory {}", @@ -36,11 +36,20 @@ pub(crate) async fn create_run( ) })?; let user_workflows_root = fabro_util::Home::from_env().workflows_dir(); - let package = fabro_manifest::resolve_local_workflow_package( - workflow_path, - &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 prepared = prepare_intent_overrides(args, &canonical_cwd).await?; warn_untransmitted_settings( @@ -49,7 +58,12 @@ pub(crate) async fn create_run( ctx.base_config_path(), *ctx.run_settings_key_presence(), ); - let project_config = project::discover_project_config(&package.workflow_location().dir)?; + let project_config = match &local_package { + Some(ResolvedWorkflow::Local(package)) => { + project::discover_project_config(&package.workflow_location().dir)? + } + _ => project::discover_project_config(&canonical_cwd)?, + }; if let Some(path) = project_config.as_deref() { warn_untransmitted_settings( ctx, @@ -71,23 +85,30 @@ pub(crate) async fn create_run( }, resolve_run_environment(client.as_ref(), args.environment.as_deref()), )?; - let configured_repo_origin_url = - fabro_manifest::configured_repo_origin_url_for_location(package.workflow_location())?; - let fabro_manifest::DerivedRunTarget { - target, - dirty_worktree, - } = fabro_manifest::derive_run_target_for_provider( - &environment.settings.provider, + let (target, dirty_worktree) = resolution::target( + &target_selection, + environment.settings.provider, &canonical_cwd, - configured_repo_origin_url.as_deref(), - )?; + ) + .await?; if dirty_worktree { fabro_util::printerr!( ctx.printer(), - "{} the caller Git working tree is dirty; uncommitted changes are not included in the run target.", + "{} the selected target Git working tree is dirty; uncommitted changes are not included in the run target.", styles.yellow.apply_to("Warning:"), ); } + let package = match local_package { + Some(package) => package, + None => { + resolution::workflow( + &workflow_selection, + &canonical_cwd, + Some(&user_workflows_root), + ) + .await? + } + }; let workflow_version_id = package.closure().root_id(); client .register_workflow_versions( diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index 7d0c2e3ed..fa0854a0c 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/lib/apps/fabro-cli/src/commands/run/mod.rs @@ -21,10 +21,13 @@ pub(crate) mod logs; pub(crate) mod output; pub(crate) mod overrides; pub(crate) mod preview; +mod remote_workflow; +mod resolution; pub(crate) mod resume; pub(crate) mod rewind; pub(crate) mod run_progress; pub(crate) mod runner; +mod selection; pub(crate) mod ssh; pub(crate) mod start; pub(crate) mod steer; @@ -38,7 +41,7 @@ pub(crate) async fn dispatch( let printer = base_ctx.printer(); match cmd { - RunCommands::Run(args) => Box::pin(command::execute(args, base_ctx)).await, + RunCommands::Run(args) => Box::pin(command::execute(*args, base_ctx)).await, RunCommands::Create(args) => { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); let ctx = base_ctx.with_target(&args.target)?; diff --git a/lib/apps/fabro-cli/src/commands/run/overrides.rs b/lib/apps/fabro-cli/src/commands/run/overrides.rs index 3c0ffca45..a925b9b01 100644 --- a/lib/apps/fabro-cli/src/commands/run/overrides.rs +++ b/lib/apps/fabro-cli/src/commands/run/overrides.rs @@ -168,6 +168,11 @@ mod tests { target: ServerTargetArgs::default(), inputs: InputOverrideArgs::default(), workflow: Some(PathBuf::from("workflow.fabro")), + workflow_git: None, + workflow_ref: None, + target_path: None, + target_git: None, + target_branch: None, dry_run: false, auto_approve: false, goal: None, diff --git a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs new file mode 100644 index 000000000..fc2760af1 --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -0,0 +1,1012 @@ +//! Native Git acquisition owned by the CLI, with no server credential lookup. +use std::future::Future; +use std::path::{Path, PathBuf}; +use std::process::{ExitStatus, Stdio}; +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}; + +const OUTPUT_LIMIT: usize = 64 * 1024; + +#[derive(Debug, thiserror::Error)] +pub(super) enum RemoteWorkflowError { + #[error( + "local Git {operation} failed ({status}); verify native Git access to the repository using your local credential helper or SSH configuration; Fabro server login does not grant Git access" + )] + Process { + operation: &'static str, + status: ExitStatus, + }, + #[error("local Git command timed out")] + Timeout, + #[error("local Git acquisition cancelled")] + Cancelled, + #[error("local Git metadata exceeds the 64 KiB capture limit")] + OutputLimit, + #[error("local Git I/O failed")] + Io(#[from] std::io::Error), +} + +pub(super) struct NativeGit { + cwd: PathBuf, + timeout: Duration, + #[cfg(test)] + pub(super) environment: Vec<(String, String)>, +} + +impl NativeGit { + pub(super) fn new(cwd: PathBuf) -> Self { + Self { + cwd, + timeout: Duration::from_mins(2), + #[cfg(test)] + environment: Vec::new(), + } + } + + async fn command( + &self, + cwd: &Path, + args: &[&str], + cancel: &CancellationToken, + ) -> Result, RemoteWorkflowError> { + if cancel.is_cancelled() { + return Err(RemoteWorkflowError::Cancelled); + } + let mut command = Command::new("git"); + command + .current_dir(cwd) + .args([ + "-c", + "core.hooksPath=/dev/null", + "-c", + "filter.lfs.smudge=", + "-c", + "filter.lfs.process=", + "-c", + "filter.lfs.required=false", + "-c", + "submodule.recurse=false", + "-c", + "protocol.ext.allow=never", + "-c", + "maintenance.auto=0", + "-c", + "gc.auto=0", + "-c", + "core.autocrlf=false", + ]) + .args(args) + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_LFS_SKIP_SMUDGE", "1") + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .env_remove("GIT_OBJECT_DIRECTORY") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .kill_on_drop(true); + #[cfg(unix)] + command.process_group(0); + #[cfg(test)] + command.envs(self.environment.iter().cloned()); + let mut child = command.spawn()?; + let process_id = child.id(); + let mut stdout = child.stdout.take().expect("piped Git stdout exists"); + let mut stderr = child.stderr.take().expect("piped Git stderr exists"); + let result = tokio::select! { + biased; + () = cancel.cancelled() => Err(RemoteWorkflowError::Cancelled), + () = time::sleep(self.timeout) => Err(RemoteWorkflowError::Timeout), + result = async { + let (status, (stdout, overflow), _) = tokio::try_join!( + child.wait(), capture(&mut stdout), capture(&mut stderr) + )?; + 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 }); + } + if overflow { return Err(RemoteWorkflowError::OutputLimit); } + Ok(stdout) + } => result, + }; + // 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)), + } + } + if result.is_err() { + if let Err(source) = child.kill().await { + cleanup_error = Some(source); + } + } + child.wait().await?; + if let Some(source) = cleanup_error { + return Err(source.into()); + } + result + } + + async fn records( + &self, + repository: &GitHubRepositorySlug, + patterns: &[String], + cancel: &CancellationToken, + ) -> anyhow::Result { + 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?; + Ok(std::str::from_utf8(&bytes) + .context("local Git returned invalid metadata encoding")? + .to_owned()) + } + + pub(super) async fn resolve_target( + &self, + repository: GitHubRepositorySlug, + branch: Option, + 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) + .await?; + let sha = exact_record(&records, &reference)?.context("target branch was not found")?; + (branch, sha) + } else { + let records = self.records(&repository, &["HEAD".into()], cancel).await?; + default_head(&records)? + }; + let target = GitRunTarget { + repo: repository.to_string(), + branch, + tag: None, + sha: Some(sha), + }; + Ok(target.validate()?.into_target()) + } + + pub(super) async fn resolve_revision( + &self, + repository: &GitHubRepositorySlug, + revision: &RemoteWorkflowRevision, + cancel: &CancellationToken, + ) -> anyhow::Result { + match revision { + RemoteWorkflowRevision::Commit(sha) => { + repository::normalize_git_commit_sha(sha).context("invalid full source commit SHA") + } + 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) + } + } + } + + pub(super) async fn collect( + &self, + repository: GitHubRepositorySlug, + selector: PathBuf, + revision: RemoteWorkflowRevision, + cancel: CancellationToken, + ) -> anyhow::Result { + selection::validate_remote_selector(&selector)?; + let sha = self + .resolve_revision(&repository, &revision, &cancel) + .await?; + let checkout = tempfile::Builder::new() + .prefix("fabro-workflow-") + .tempdir()?; + self.collect_checkout(repository, selector, sha, checkout, cancel) + .await + } + + async fn collect_checkout( + &self, + repository: GitHubRepositorySlug, + selector: PathBuf, + sha: String, + checkout: tempfile::TempDir, + cancel: CancellationToken, + ) -> anyhow::Result { + self.checkout(&repository, &sha, checkout.path(), &cancel) + .await?; + // Moving the directory into the blocking task keeps it alive even if + // the outer future is dropped. Collection is not interruptible; finish + // it and clean up before reporting cancellation. + let collection_cancel = cancel.clone(); + let closure = task::spawn_blocking(move || { + check_source_symlinks(checkout.path())?; + if collection_cancel.is_cancelled() { + return Err(RemoteWorkflowError::Cancelled.into()); + } + fabro_manifest::collect_workflow_versions(&selector, checkout.path()) + .map_err(anyhow::Error::new) + }) + .await + .context("workflow collection task failed")??; + if cancel.is_cancelled() { + return Err(RemoteWorkflowError::Cancelled.into()); + } + Ok(closure) + } + + async fn checkout( + &self, + repository: &GitHubRepositorySlug, + sha: &str, + root: &Path, + cancel: &CancellationToken, + ) -> anyhow::Result<()> { + self.command(root, &["init", "--quiet", "--template="], cancel) + .await?; + self.command( + root, + &[ + "fetch", + "--quiet", + "--depth=1", + "--no-tags", + "--no-recurse-submodules", + &repository.https_url(), + sha, + ], + cancel, + ) + .await?; + let kind = self.command(root, &["cat-file", "-t", sha], cancel).await?; + if kind != b"commit\n" { + bail!("the selected source SHA is not a commit"); + } + // The highest-precedence attributes file prevents repository attributes + // from invoking configured filters or rewriting the committed source bytes. + fs::create_dir_all(root.join(".git/info")).await?; + fs::write( + root.join(".git/info/attributes"), + "* -filter -text -ident -working-tree-encoding\n", + ) + .await?; + self.command( + root, + &[ + "-c", + &format!("core.worktree={}", root.display()), + "checkout", + "--quiet", + "--detach", + sha, + "--", + ], + cancel, + ) + .await?; + let head = self + .command(root, &["rev-parse", "--verify", "HEAD"], cancel) + .await?; + if head != format!("{sha}\n").as_bytes() { + bail!("workflow checkout did not match the selected commit"); + } + Ok(()) + } +} + +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; + let mut buffer = vec![0; 8192]; + loop { + let count = reader.read(&mut buffer).await?; + if count == 0 { + return Ok((captured, overflow)); + } + let keep = count.min(OUTPUT_LIMIT - captured.len()); + captured.extend_from_slice(&buffer[..keep]); + overflow |= keep != count; + } +} + +fn exact_record(records: &str, reference: &str) -> anyhow::Result> { + let mut found = None; + for line in records.lines() { + let Some((value, name)) = line.split_once('\t') else { + continue; + }; + if name != reference || value.starts_with("ref: ") { + continue; + } + let sha = repository::normalize_git_commit_sha(value) + .context("invalid Git metadata commit SHA")?; + if found.as_ref().is_some_and(|previous| previous != &sha) { + bail!("conflicting Git metadata records"); + } + found = Some(sha); + } + Ok(found) +} + +fn default_head(records: &str) -> anyhow::Result<(String, String)> { + let mut branch = None; + for line in records.lines() { + if let Some(value) = line + .strip_prefix("ref: refs/heads/") + .and_then(|line| line.strip_suffix("\tHEAD")) + { + if !repository::is_valid_github_ref_selector(value) { + bail!("remote default HEAD does not name a valid branch"); + } + if branch.is_some() { + bail!("ambiguous remote default HEAD"); + } + branch = Some(value.to_owned()); + } + } + Ok(( + branch.context("remote default HEAD must name a branch")?, + exact_record(records, "HEAD")?.context("remote default HEAD has no commit")?, + )) +} + +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/..." + ); + } + if let Some(sha) = branch { + return Ok(sha); + } + if let Some(sha) = tag { + return Ok(exact_record(records, &format!("{tags}^{{}}"))?.unwrap_or(sha)); + } + bail!("workflow ref was not found; no alternative revision was selected") +} + +fn check_source_symlinks(root: &Path) -> anyhow::Result<()> { + let root = root.canonicalize()?; + // 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")) + { + let entry = entry?; + if entry.file_type().is_symlink() && !entry.path().canonicalize()?.starts_with(&root) { + bail!("remote workflow checkout contains a symlink outside its source root"); + } + } + Ok(()) +} + +/// The task owns its child processes and temporary checkout. Dropping the +/// waiter requests cooperative cleanup, not task abortion. Ctrl-C waits for +/// cleanup; an in-progress blocking collection must finish first. +pub(super) async fn owned( + work: impl FnOnce(CancellationToken) -> TFuture + Send + 'static, +) -> anyhow::Result +where + TFuture: Future> + Send + 'static, +{ + let cancel = CancellationToken::new(); + let _cancel_on_drop = cancel.clone().drop_guard(); + let mut task = tokio::spawn(work(cancel.clone())); + tokio::select! { + result = &mut task => result.context("local Git task failed")?, + signal = tokio_signal::ctrl_c() => { + cancel.cancel(); + let _result = task.await.context("local Git cleanup task failed")?; + signal.context("failed to listen for interruption")?; + Err(RemoteWorkflowError::Cancelled.into()) + } + } +} + +#[cfg(test)] +#[expect( + clippy::disallowed_methods, + reason = "hermetic Git fixtures and fake executables use synchronous file setup" +)] +mod tests { + use std::os::unix::fs::PermissionsExt as _; + + use nix::sys::stat::Mode; + use nix::unistd; + + use super::*; + + struct Fixture { + root: tempfile::TempDir, + repo: git2::Repository, + git: NativeGit, + sha: String, + } + + impl Fixture { + fn new() -> Self { + let root = tempfile::tempdir().unwrap(); + let repo_dir = root.path().join("source"); + let repo = git2::Repository::init_opts( + &repo_dir, + 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() + }; + let config = root.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n", + repo_dir.display() + ), + ) + .unwrap(); + let mut git = NativeGit::new(root.path().to_path_buf()); + git.environment = vec![ + ("GIT_CONFIG_GLOBAL".into(), config.to_str().unwrap().into()), + ("GIT_CONFIG_NOSYSTEM".into(), "1".into()), + ("GIT_CONFIG_COUNT".into(), "0".into()), + ]; + Self { + root, + repo, + git, + sha, + } + } + + fn repository() -> GitHubRepositorySlug { + "acme/workflows".parse().unwrap() + } + } + + 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(); + let source = fixture.repo.workdir().unwrap(); + let child = source.join(".fabro/workflows/child"); + std::fs::create_dir_all(&child).unwrap(); + std::fs::write( + child.join("workflow.fabro"), + "digraph Child { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + std::fs::write(source.join(".fabro/workflows/review/workflow.fabro"), "digraph Root { start [shape=Mdiamond] exit [shape=Msquare] child [shape=house, stack.child_workflow=\"../child/workflow.fabro\"] start -> child -> exit }").unwrap(); + std::fs::write(source.join(".gitattributes"), "*.fabro filter=fixture\n").unwrap(); + let config_path = fixture.root.path().join("gitconfig"); + let mut config = git2::Config::open(&config_path).unwrap(); + let sentinel = fixture.root.path().join("executed"); + let hooks = fixture.root.path().join("hooks"); + std::fs::create_dir(&hooks).unwrap(); + std::fs::write( + hooks.join("post-checkout"), + format!("#!/bin/sh\ntouch '{}'\n", sentinel.display()), + ) + .unwrap(); + std::fs::set_permissions( + hooks.join("post-checkout"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + config + .set_str("core.hooksPath", hooks.to_str().unwrap()) + .unwrap(); + config + .set_str( + "filter.fixture.smudge", + &format!("touch '{}'; cat", sentinel.display()), + ) + .unwrap(); + let sha = fixture.commit_changes(); + let local = fabro_manifest::collect_workflow_versions(Path::new("review"), source).unwrap(); + assert_eq!(local.versions().count(), 2); + for selector in [ + "review", + ".fabro/workflows/review/workflow.fabro", + "missing", + ] { + let checkout = tempfile::tempdir().unwrap(); + let checkout_path = checkout.path().to_path_buf(); + let result = fixture + .git + .collect_checkout( + Fixture::repository(), + selector.into(), + sha.clone(), + checkout, + CancellationToken::new(), + ) + .await; + assert!(!checkout_path.exists()); + if selector == "missing" { + assert!(result.is_err()); + } else { + let remote = result.unwrap(); + assert_eq!(local.root_id(), remote.root_id()); + assert_eq!( + local.versions().map(|(id, _)| id).collect::>(), + remote.versions().map(|(id, _)| id).collect::>() + ); + } + assert!(!sentinel.exists()); + } + let checkout = tempfile::tempdir().unwrap(); + let path = checkout.path().to_path_buf(); + assert!( + fixture + .git + .collect_checkout( + Fixture::repository(), + "review".into(), + "1111111111111111111111111111111111111111".into(), + checkout, + CancellationToken::new() + ) + .await + .is_err() + ); + assert!(!path.exists()); + } + + #[tokio::test] + async fn remote_workflow_rejects_toml_symlink_before_reading_host_content() { + let fixture = Fixture::new(); + let host = tempfile::tempdir().unwrap(); + let fifo = host.path().join("host.toml"); + // Any accidental read blocks: there is deliberately no writer. The + // successful containment error proves rejection before TOML loading. + unistd::mkfifo(&fifo, Mode::S_IRUSR).unwrap(); + let path = fixture + .repo + .workdir() + .unwrap() + .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 error = fixture + .git + .collect( + Fixture::repository(), + "review".into(), + RemoteWorkflowRevision::Commit(sha), + CancellationToken::new(), + ) + .await + .unwrap_err(); + assert!(error.to_string().contains("outside its source root")); + } + + #[tokio::test] + async fn remote_workflow_dropped_waiter_cancels_child_before_checkout_cleanup() { + let (fake, git) = fake_git("printf '%s' $$ > pid; exec /bin/sleep 60"); + let checkout = tempfile::tempdir().unwrap(); + let path = checkout.path().to_path_buf(); + let worker = tokio::spawn(owned(move |cancel| async move { + git.collect_checkout( + "acme/workflows".parse().unwrap(), + "review".into(), + "1111111111111111111111111111111111111111".into(), + checkout, + cancel, + ) + .await + })); + time::timeout(Duration::from_secs(5), async { + while !path.join("pid").exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .unwrap(); + let pid: i32 = std::fs::read_to_string(path.join("pid")) + .unwrap() + .parse() + .unwrap(); + worker.abort(); + assert!(worker.await.unwrap_err().is_cancelled()); + time::timeout(Duration::from_secs(5), async { + while path.exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .unwrap(); + assert_eq!(signal::kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)); + drop(fake); + } + + #[tokio::test] + async fn remote_workflow_resolves_exact_default_branches_tags_and_commits() { + let fixture = Fixture::new(); + let object = fixture.repo.revparse_single("HEAD").unwrap(); + fixture + .repo + .branch("topic/slash", object.as_commit().unwrap(), false) + .unwrap(); + fixture + .repo + .tag_lightweight("light", &object, false) + .unwrap(); + let signature = git2::Signature::now("Fixture", "fixture@example.test").unwrap(); + fixture + .repo + .tag("annotated", &object, &signature, "release", false) + .unwrap(); + let cancel = CancellationToken::new(); + for reference in [ + None, + Some("HEAD"), + Some("trunk"), + Some("topic/slash"), + Some("light"), + Some("annotated"), + Some("refs/heads/trunk"), + Some("refs/tags/annotated"), + Some(fixture.sha.as_str()), + ] { + let revision = RemoteWorkflowRevision::parse(reference).unwrap(); + assert_eq!( + fixture + .git + .resolve_revision(&Fixture::repository(), &revision, &cancel) + .await + .unwrap(), + fixture.sha + ); + } + fixture + .repo + .tag_lightweight("trunk", &object, false) + .unwrap(); + assert!( + fixture + .git + .resolve_revision( + &Fixture::repository(), + &RemoteWorkflowRevision::Ref("trunk".into()), + &cancel + ) + .await + .unwrap_err() + .to_string() + .contains("ambiguous") + ); + for reference in ["missing", "refs/tags/missing"] { + assert!( + fixture + .git + .resolve_revision( + &Fixture::repository(), + &RemoteWorkflowRevision::Ref(reference.into()), + &cancel + ) + .await + .is_err() + ); + } + let target = fixture + .git + .resolve_target(Fixture::repository(), None, &cancel) + .await + .unwrap(); + assert_eq!(target.branch, "trunk"); + assert_eq!(target.sha.as_deref(), Some(fixture.sha.as_str())); + assert_eq!( + fixture + .git + .resolve_target(Fixture::repository(), Some("topic/slash".into()), &cancel) + .await + .unwrap() + .branch, + "topic/slash" + ); + for branch in [ + "missing", + "annotated", + "HEAD", + fixture.sha.as_str(), + "refs/heads/trunk", + ] { + assert!( + fixture + .git + .resolve_target(Fixture::repository(), Some(branch.into()), &cancel) + .await + .is_err() + ); + } + // Metadata-only target resolution never creates a checkout directory. + assert_eq!(std::fs::read_dir(fixture.root.path()).unwrap().count(), 2); + } + + #[tokio::test] + async fn remote_workflow_same_bytes_have_same_ids_and_no_lookup_fallback() { + let fixture = Fixture::new(); + let local = fabro_manifest::collect_workflow_versions( + Path::new("review"), + fixture.repo.workdir().unwrap(), + ) + .unwrap(); + let remote = fixture + .git + .collect( + Fixture::repository(), + "review".into(), + RemoteWorkflowRevision::DefaultBranch, + CancellationToken::new(), + ) + .await + .unwrap(); + assert_eq!(local.root_id(), remote.root_id()); + let before: Vec<_> = local.versions().map(|(id, _)| id).collect(); + assert_eq!( + before, + remote.versions().map(|(id, _)| id).collect::>() + ); + assert!( + fixture + .git + .collect( + Fixture::repository(), + "missing".into(), + RemoteWorkflowRevision::DefaultBranch, + CancellationToken::new() + ) + .await + .is_err() + ); + } + + #[tokio::test] + async fn remote_workflow_fetches_observed_commit_after_branch_moves() { + let fixture = Fixture::new(); + let cancel = CancellationToken::new(); + let captured = fixture + .git + .resolve_revision( + &Fixture::repository(), + &RemoteWorkflowRevision::DefaultBranch, + &cancel, + ) + .await + .unwrap(); + let parent = fixture + .repo + .revparse_single("HEAD") + .unwrap() + .peel_to_commit() + .unwrap(); + let signature = git2::Signature::now("Fixture", "fixture@example.test").unwrap(); + let next = fixture + .repo + .commit( + Some("HEAD"), + &signature, + &signature, + "move branch", + &parent.tree().unwrap(), + &[&parent], + ) + .unwrap(); + assert_ne!(next.to_string(), captured); + let checkout = tempfile::tempdir().unwrap(); + fixture + .git + .checkout(&Fixture::repository(), &captured, checkout.path(), &cancel) + .await + .unwrap(); + assert_eq!( + git2::Repository::open(checkout.path()) + .unwrap() + .head() + .unwrap() + .target() + .unwrap() + .to_string(), + captured + ); + assert!( + fixture + .git + .checkout( + &Fixture::repository(), + "1111111111111111111111111111111111111111", + checkout.path(), + &cancel + ) + .await + .is_err() + ); + } + + #[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()); + let root = tempfile::tempdir().unwrap(); + let host = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink(host.path(), root.path().join("outside")).unwrap(); + assert!(check_source_symlinks(root.path()).is_err()); + } + + fn fake_git(script: &str) -> (tempfile::TempDir, NativeGit) { + let root = tempfile::tempdir().unwrap(); + let path = root.path().join("git"); + std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + let mut git = NativeGit::new(root.path().to_path_buf()); + git.environment + .push(("PATH".into(), root.path().to_str().unwrap().into())); + (root, git) + } + + #[tokio::test] + async fn remote_workflow_caps_drains_diagnostics_and_preserves_safe_status() { + let (root, git) = fake_git( + "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()) + .await + .unwrap_err(); + assert!( + matches!(error, RemoteWorkflowError::Process { status, .. } if status.code() == Some(42)) + ); + assert!(!format!("{error:?} {error}").contains("sentinel-secret")); + let (root, git) = fake_git( + "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(), + RemoteWorkflowError::OutputLimit + )); + } + + #[tokio::test] + async fn remote_workflow_timeout_and_cancel_reap_owned_children() { + for timeout in [true, false] { + let (root, mut git) = fake_git("printf '%s' $$ > pid; exec /bin/sleep 60"); + git.timeout = Duration::from_secs(2); + let cancel = CancellationToken::new(); + let trigger = async { + if !timeout { + time::timeout(Duration::from_secs(5), async { + while !root.path().join("pid").exists() { + time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .unwrap(); + cancel.cancel(); + } + }; + let (result, ()) = tokio::join!(git.command(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")) + .unwrap() + .parse() + .unwrap(); + assert_eq!(signal::kill(Pid::from_raw(pid), None), Err(Errno::ESRCH)); + } + } +} diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs new file mode 100644 index 000000000..57bd458af --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -0,0 +1,358 @@ +use std::path::{Path, PathBuf}; + +use anyhow::{Context as _, anyhow, bail}; +use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; +use fabro_types::settings::run::EnvironmentProvider; +use fabro_types::{DirtyStatus, RunTarget}; +use tokio::task; + +use super::remote_workflow::{self, NativeGit}; +use super::selection::{TargetSelection, WorkflowSelection}; + +/// Owns the canonical collector result without copying its contents. Local +/// location metadata remains available solely for existing settings warnings. +pub(super) enum ResolvedWorkflow { + Local(ResolvedLocalWorkflowPackage), + Git(CollectedWorkflowClosure), +} + +impl ResolvedWorkflow { + pub(super) fn closure(&self) -> &CollectedWorkflowClosure { + match self { + Self::Local(package) => package.closure(), + Self::Git(closure) => closure, + } + } +} + +pub(super) async fn workflow( + selection: &WorkflowSelection, + cwd: &Path, + user_workflows: Option<&Path>, +) -> anyhow::Result { + match selection { + WorkflowSelection::Local(path) => { + let (path, cwd, user_workflows) = ( + path.clone(), + cwd.to_path_buf(), + user_workflows.map(Path::to_path_buf), + ); + let package = task::spawn_blocking(move || { + fabro_manifest::resolve_local_workflow_package( + &path, + &cwd, + user_workflows.as_deref(), + ) + .map_err(anyhow::Error::new) + }) + .await + .context("local workflow collection task failed")??; + Ok(ResolvedWorkflow::Local(package)) + } + WorkflowSelection::Git { + repository, + selector, + revision, + } => { + let git = NativeGit::new(cwd.to_path_buf()); + let (repository, selector, revision) = + (repository.clone(), selector.clone(), revision.clone()); + let closure = remote_workflow::owned(move |cancel| async move { + git.collect(repository, selector, revision, cancel).await + }) + .await?; + Ok(ResolvedWorkflow::Git(closure)) + } + } +} + +pub(super) async fn target( + selection: &TargetSelection, + 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 + } + TargetSelection::Git { repository, branch } => { + if !provider.is_clone_based() { + bail!("Git targets require a clone-enabled Docker or Daytona environment"); + } + let git = NativeGit::new(cwd.to_path_buf()); + let (repository, branch) = (repository.clone(), branch.clone()); + let target = remote_workflow::owned(move |cancel| async move { + git.resolve_target(repository, branch, &cancel).await + }) + .await?; + // Canonical admission retains ownership of provider capabilities. + Ok((RunTarget::Git(target), false)) + } + } +} + +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)) + .await + .context("target observation task failed")? +} + +/// Derives the run target from the selected directory for the environment's +/// provider. Returns the target plus whether a clone-based observation found a +/// dirty Git worktree, so the caller can warn about it. +fn run_target_for_environment( + provider: EnvironmentProvider, + canonical_cwd: &Path, +) -> anyhow::Result<(RunTarget, bool)> { + if !provider.is_clone_based() { + let path = canonical_cwd.to_str().ok_or_else(|| { + anyhow!( + "target directory is not valid UTF-8: {}", + canonical_cwd.display() + ) + })?; + return Ok(( + RunTarget::Folder { + path: path.to_string(), + }, + false, + )); + } + let Some(observation) = fabro_manifest::observe_git_run_target(canonical_cwd, None) else { + return Ok((none_target_for_unversioned_directory(canonical_cwd)?, false)); + }; + let dirty = observation.legacy_git_context.dirty == DirtyStatus::Dirty; + let target = observation.run_target.ok_or_else(|| { + anyhow!("the target Git checkout cannot be represented as a canonical GitHub run target") + })?; + if target.sha.is_none() { + bail!( + "the exact local Git commit could not be made available from the canonical GitHub origin; push the commit and try again" + ); + } + Ok((RunTarget::Git(target), dirty)) +} + +fn none_target_for_unversioned_directory(canonical_cwd: &Path) -> anyhow::Result { + let repository = match git2::Repository::discover(canonical_cwd) { + Ok(repository) => repository, + Err(source) if source.code() == git2::ErrorCode::NotFound => return Ok(RunTarget::None {}), + Err(source) => { + return Err(anyhow::Error::new(source)).with_context(|| { + format!( + "failed to inspect target directory {} for Git metadata", + canonical_cwd.display() + ) + }); + } + }; + + if repository.is_bare() { + bail!( + "the target directory resolves to a bare Git repository; clone-based runs require a non-bare checkout with an attached branch" + ); + } + match repository.head() { + Err(source) + if matches!( + source.code(), + git2::ErrorCode::UnbornBranch | git2::ErrorCode::NotFound + ) => + { + bail!( + "the target Git checkout has no commits; create a commit before using a clone-based environment" + ); + } + Err(source) => { + return Err(anyhow::Error::new(source)) + .context("failed to inspect the target Git checkout HEAD"); + } + Ok(head) if !head.is_branch() => { + bail!( + "the target Git checkout has a detached HEAD; check out a branch before using a clone-based environment" + ); + } + Ok(_) => {} + } + + bail!( + "the target Git checkout does not have a usable attached branch for a clone-based run target" + ) +} + +#[cfg(test)] +#[expect( + clippy::disallowed_methods, + reason = "resolver tests construct small local workflow fixtures" +)] +mod tests { + use clap::Parser as _; + + use super::super::selection; + 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() { + 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()); + 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) + .await + .unwrap() + .0, + RunTarget::Folder { + path: root.join("target").to_str().unwrap().into(), + } + ); + assert_eq!( + target(&direct_target, EnvironmentProvider::Docker, &root) + .await + .unwrap() + .0, + RunTarget::None {} + ); + assert_eq!( + target( + &TargetSelection::CurrentDirectory, + EnvironmentProvider::Local, + &root + ) + .await + .unwrap() + .0, + RunTarget::Folder { + path: root.to_str().unwrap().into(), + } + ); + assert!( + target( + &TargetSelection::Git { + repository: "acme/app".parse().unwrap(), + branch: None, + }, + EnvironmentProvider::Local, + &root + ) + .await + .unwrap_err() + .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() + ); + } + + #[tokio::test] + async fn run_selection_local_lookup_preserves_precedence_and_explicit_failure() { + let root = tempfile::tempdir().unwrap(); + let user = root.path().join("user"); + let project = root.path().join("project"); + let checkout = root.path().join("project/checkout"); + write_workflow(&user, "review"); + write_workflow(&project, ".fabro/workflows/review"); + write_workflow(&checkout, ".fabro/workflows/review"); + std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); + git2::Repository::init(&checkout).unwrap(); + let selected = WorkflowSelection::Local("review".into()); + for (cwd, expected_root) in [ + (checkout.as_path(), checkout.as_path()), + (project.as_path(), project.as_path()), + (root.path(), user.as_path()), + ] { + let ResolvedWorkflow::Local(package) = + workflow(&selected, cwd, Some(&user)).await.unwrap() + else { + panic!("local package"); + }; + assert_eq!(package.source_root(), expected_root.canonicalize().unwrap()); + } + assert!( + workflow( + &WorkflowSelection::Local("missing.toml".into()), + &checkout, + Some(&user) + ) + .await + .is_err() + ); + } +} diff --git a/lib/apps/fabro-cli/src/commands/run/selection.rs b/lib/apps/fabro-cli/src/commands/run/selection.rs new file mode 100644 index 000000000..038fde822 --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/selection.rs @@ -0,0 +1,288 @@ +//! CLI syntax ends here. Resolvers receive selections and explicit caller +//! context. +use std::path::{Component, Path, PathBuf}; + +use anyhow::{Context as _, bail}; +use fabro_types::{GitHubRepositorySlug, repository}; + +use crate::args::RunArgs; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum WorkflowSelection { + Local(PathBuf), + Git { + repository: GitHubRepositorySlug, + selector: PathBuf, + revision: RemoteWorkflowRevision, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum TargetSelection { + CurrentDirectory, + Path(PathBuf), + Git { + repository: GitHubRepositorySlug, + branch: Option, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum RemoteWorkflowRevision { + DefaultBranch, + Ref(String), + Commit(String), +} + +impl RemoteWorkflowRevision { + pub(super) fn parse(value: Option<&str>) -> anyhow::Result { + match value { + None | Some("HEAD") => Ok(Self::DefaultBranch), + Some(value) => { + if let Some(sha) = repository::normalize_git_commit_sha(value) { + return Ok(Self::Commit(sha)); + } + if !repository::is_valid_github_ref_selector(value) + || (value.starts_with("refs/") + && !value.starts_with("refs/heads/") + && !value.starts_with("refs/tags/")) + { + bail!("workflow ref must be a branch, tag, HEAD, or full 40-hex commit SHA"); + } + Ok(Self::Ref(value.to_owned())) + } + } + } +} + +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 == "..") + { + 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('-') => {} + _ => 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)> { + 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) => { + validate_remote_selector(workflow)?; + WorkflowSelection::Git { + repository: repository.clone(), + selector: workflow.clone(), + revision: RemoteWorkflowRevision::parse(args.workflow_ref.as_deref())?, + } + } + }; + let target = match (&args.target_path, &args.target_git) { + (Some(path), _) => TargetSelection::Path(path.clone()), + (_, Some(repository)) => { + if args + .target_branch + .as_deref() + .is_some_and(|branch| !repository::is_valid_git_branch_name(branch)) + { + bail!( + "target branch must be a working branch name, not a tag, SHA, or qualified ref" + ); + } + TargetSelection::Git { + repository: repository.clone(), + branch: args.target_branch.clone(), + } + } + _ => TargetSelection::CurrentDirectory, + }; + Ok((workflow, target)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn run_selection_remote_grammar_is_pure_and_rejects_unsafe_selectors() { + for path in [ + "review", + "./review.toml", + ".fabro/workflows/review/workflow.toml", + "dir/graph.fabro", + ] { + validate_remote_selector(Path::new(path)).unwrap(); + } + for path in [ + "", + ".", + "..", + "/tmp/workflow.toml", + "../review.toml", + "a/../review.toml", + "dir/review", + "dir/", + "a\\review.toml", + ] { + assert!(validate_remote_selector(Path::new(path)).is_err(), "{path}"); + } + for value in [ + "topic/slash", + "refs/heads/release", + "refs/tags/v1", + "HEAD", + "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd", + ] { + RemoteWorkflowRevision::parse(Some(value)).unwrap(); + } + for value in [ + "--upload-pack=x", + "topic*", + "HEAD~1", + "main..next", + "refs/pull/1/head", + "a.lock", + "main@{1}", + ] { + assert!( + RemoteWorkflowRevision::parse(Some(value)).is_err(), + "{value}" + ); + } + } +} + +#[cfg(test)] +mod adapter_tests { + use clap::Parser as _; + + 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"] { + let cli = Cli::try_parse_from([ + "fabro", + command, + "review", + "--workflow-git", + "acme/workflows", + "--workflow-ref", + "v1", + "--target-git", + "acme/app", + "--target-branch", + "release", + ]) + .unwrap(); + let Commands::RunCmd(RunCommands::Run(args) | RunCommands::Create(args)) = + *cli.command.unwrap() + else { + panic!("expected shared run arguments"); + }; + assert_eq!( + parse(&args).unwrap(), + ( + WorkflowSelection::Git { + repository: "acme/workflows".parse().unwrap(), + selector: "review".into(), + revision: RemoteWorkflowRevision::Ref("v1".into()), + }, + TargetSelection::Git { + repository: "acme/app".parse().unwrap(), + branch: Some("release".into()), + }, + ) + ); + } + let cli = Cli::try_parse_from(["fabro", "run", "create"]).unwrap(); + assert!( + matches!(*cli.command.unwrap(), Commands::RunCmd(RunCommands::Run(args)) if args.workflow.as_deref() == Some(Path::new("create"))) + ); + } + + #[test] + fn run_selection_adapter_rejects_invalid_inputs_without_acquisition() { + 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"], + vec![ + "cmd", + "/tmp/review.toml", + "--workflow-git", + "acme/workflows", + ], + vec![ + "cmd", + "review", + "--workflow-git", + "acme/workflows", + "--workflow-ref", + "HEAD~1", + ], + vec![ + "cmd", + "review", + "--target-git", + "acme/app", + "--target-branch", + "refs/tags/v1", + ], + vec![ + "cmd", + "review", + "--target-git", + "acme/app", + "--target-branch", + "1234567890123456789012345678901234567890", + ], + ] { + if let Ok(command) = Command::try_parse_from(flags) { + assert!(parse(&command.args).is_err()); + } + } + } +} diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 0818a83b6..93716c01e 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -87,22 +87,27 @@ fn help() { success: true exit_code: 0 ----- stdout ----- - Register a local workflow version and create a submitted run + Register a workflow version and create a submitted run Usage: fabro create [OPTIONS] Arguments: - Local workflow name, checkout path, .fabro file, or workflow TOML + Workflow name or path (repository-relative with --workflow-git) Options: --json Output as JSON [env: FABRO_JSON=] --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) - --dry-run Execute with simulated LLM backend --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --auto-approve Auto-approve all human gates + --workflow-git Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials --quiet Suppress non-essential output [env: FABRO_QUIET=] + --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names + --target-path Observe this target directory instead of cwd; Folder targets require server filesystem access + --target-git Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials + --target-branch Target working branch (default: remote default branch), pinned to its observed commit + --dry-run Simulate execution; workflow source may still be fetched and uploaded + --auto-approve Auto-approve all human gates --goal Override the workflow goal (available as {{ goal }} in prompts) --goal-file Read a per-run goal value from a local file --model Override default LLM model @@ -909,9 +914,9 @@ fn create_rejects_unusable_git_checkouts_instead_of_sending_an_empty_target() { for (working_directory, expected_error) in [ ( detached.path(), - "the caller Git checkout has a detached HEAD", + "the target Git checkout has a detached HEAD", ), - (unborn.path(), "the caller Git checkout has no commits"), + (unborn.path(), "the target Git checkout has no commits"), ] { let output = context .create_cmd() @@ -1633,3 +1638,349 @@ draft = false assert!(pull_request.enabled); 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, + git2::RepositoryInitOptions::new().initial_head(branch), + ) + .expect("fixture repository should initialize"); + let mut index = repo.index().expect("fixture index should open"); + index + .add_all(["."], git2::IndexAddOption::DEFAULT, None) + .expect("fixture files should stage"); + let tree_id = index.write_tree().expect("fixture tree should write"); + let tree = repo.find_tree(tree_id).expect("fixture tree should exist"); + let signature = git2::Signature::now("Fixture", "fixture@example.test") + .expect("fixture signature should be valid"); + repo.commit(Some("HEAD"), &signature, &signature, "fixture", &tree, &[]) + .expect("fixture commit should succeed") + .to_string() +} + +#[test] +fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_independent() { + let context = test_context!(); + let server = MockServer::start(); + let local_env = mock_environment(&server, "local", "local"); + let docker_env = mock_environment(&server, "docker", "docker"); + 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 root = tempfile::tempdir().unwrap(); + let caller = root.path().join("caller"); + let source = root.path().join("source"); + let target = root.path().join("target"); + write_workflow(&caller, ".fabro/workflows/review", "Caller"); + write_workflow(&source, ".fabro/workflows/review", "Remote"); + write_workflow(&target, ".fabro/workflows/review", "Target"); + std::fs::write(caller.join("goal.txt"), "Caller goal").unwrap(); + std::fs::write(target.join("goal.txt"), "Target goal").unwrap(); + init_remote_fixture(&source, "trunk"); + let target_sha = init_remote_fixture(&target, "release"); + let config = root.path().join("gitconfig"); + std::fs::write(&config, format!("[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n[url \"file://{}\"]\n insteadOf = https://github.com/acme/app\n",source.display(),target.display())).unwrap(); + let local_id = fabro_manifest::resolve_local_workflow_package( + std::path::Path::new("review"), + &caller, + None, + ) + .unwrap() + .closure() + .root_id(); + let remote_id = + fabro_manifest::collect_workflow_versions(std::path::Path::new("review"), &source) + .unwrap() + .root_id(); + let file_id = fabro_manifest::resolve_local_workflow_package( + std::path::Path::new(".fabro/workflows/review/workflow.toml"), + &caller, + None, + ) + .unwrap() + .closure() + .root_id(); + assert_ne!(local_id, remote_id); + for source_kind in ["name", "file", "git"] { + for target_kind in ["inferred", "path", "git"] { + let mut command = context.create_cmd(); + command + .current_dir(&caller) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0"); + command.args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--goal-file", + "goal.txt", + ]); + command.arg(if source_kind == "file" { + ".fabro/workflows/review/workflow.toml" + } else { + "review" + }); + if source_kind == "git" { + command.args([ + "--workflow-git", + "acme/workflows", + "--workflow-ref", + "trunk", + ]); + } + match target_kind { + "path" => { + command.args(["--target-path", "../target", "--environment", "local"]); + } + "git" => { + command.args([ + "--target-git", + "acme/app", + "--target-branch", + "release", + "--environment", + "docker", + ]); + } + _ => { + command.args(["--environment", "docker"]); + } + } + let output = command.output().unwrap(); + assert!( + output.status.success(), + "{source_kind}/{target_kind}: {}", + output_stderr(&output) + ); + let requests = requests.lock().unwrap(); + let intent = requests.last().unwrap(); + assert_eq!( + intent["workflow_version_id"], + match source_kind { + "git" => remote_id, + "file" => file_id, + _ => local_id, + } + .to_string() + ); + assert_eq!(intent["goal"], "Caller goal"); + assert_eq!(intent["target"], match target_kind { + "path" => json!({"kind":"folder","path":target.canonicalize().unwrap()}), + "git" => + json!({"kind":"git","repo":"acme/app","branch":"release","sha":target_sha}), + _ => json!({"kind":"none"}), + }); + } + } + local_env.assert_calls(3); + docker_env.assert_calls(6); + versions.assert_calls(9); + create.assert_calls(9); +} + +#[test] +fn remote_workflow_run_starts_once_create_leaves_submitted_and_failures_do_not_refetch() { + let context = test_context!(); + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("source"); + write_workflow(&source, ".fabro/workflows/review", "Remote"); + init_remote_fixture(&source, "trunk"); + let config = root.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n", + source.display() + ), + ) + .unwrap(); + for failure in ["none", "run", "upload", "create", "start"] { + let server = MockServer::start(); + let environment = mock_environment(&server, "default", "docker"); + let version = if failure == "upload" { + server.mock(|when, then| { + when.method("POST").path("/api/v1/workflow-versions"); + then.status(422).body("fixture registration rejection"); + }) + } else { + mock_workflow_version_registrations(&server) + }; + let run_id = unique_run_id(); + let create = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + if failure == "create" { + then.status(422).body("fixture create rejection"); + } else { + then.status(201) + .header("content-type", "application/json") + .body(run_status_response(&run_id, "submitted").to_string()); + } + }); + let start = server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + if failure == "start" { + then.status(422).body("fixture start rejection"); + } else { + then.status(200) + .header("content-type", "application/json") + .body(run_status_response(&run_id, "submitted").to_string()); + } + }); + let trace = root.path().join(format!("trace-{failure}")); + let mut command = if failure == "none" { + context.create_cmd() + } else { + context.run_cmd() + }; + command + .current_dir(root.path()) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .env("GIT_TRACE", &trace) + .args([ + "review", + "--workflow-git", + "acme/workflows", + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + "--detach", + "--json", + ]); + let output = command.output().unwrap(); + assert_eq!( + output.status.success(), + matches!(failure, "none" | "run"), + "{}", + output_stderr(&output) + ); + environment.assert(); + version.assert(); + create.assert_calls(usize::from(failure != "upload")); + start.assert_calls(usize::from(matches!(failure, "run" | "start"))); + if failure == "none" { + assert_eq!( + serde_json::from_slice::(&output.stdout).unwrap(), + json!({"run_id":run_id}) + ); + } + let trace = std::fs::read_to_string(&trace).unwrap(); + assert_eq!( + trace + .lines() + .filter(|line| line.contains("built-in: git fetch ")) + .count(), + 1, + "source fetched more than once" + ); + } +} + +#[test] +fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutations() { + let context = test_context!(); + let server = MockServer::start(); + let environment = mock_environment(&server, "default", "docker"); + let version = mock_workflow_version_registrations(&server); + let create = mock_intent_create(&server, &unique_run_id(), Arc::new(Mutex::new(Vec::new()))); + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("source"); + write_workflow(&source, ".fabro/workflows/other", "Other"); + init_remote_fixture(&source, "trunk"); + write_workflow(root.path(), ".fabro/workflows/review", "Caller"); + write_workflow( + &context.home_dir.join(".fabro/workflows"), + "review", + "Installed", + ); + let config = root.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n", + source.display() + ), + ) + .unwrap(); + for reference in [ + "trunk", + "missing", + "1111111111111111111111111111111111111111", + ] { + let output = context + .create_cmd() + .current_dir(root.path()) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .args([ + "review", + "--workflow-git", + "acme/workflows", + "--workflow-ref", + reference, + "--server", + &format!("{}/api/v1", server.base_url()), + ]) + .output() + .unwrap(); + assert!(!output.status.success()); + assert!(output.stdout.is_empty()); + } + environment.assert_calls(3); + version.assert_calls(0); + create.assert_calls(0); +} diff --git a/lib/apps/fabro-cli/tests/it/cmd/fabro.rs b/lib/apps/fabro-cli/tests/it/cmd/fabro.rs index 1151e285f..2972f10b8 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/fabro.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/fabro.rs @@ -12,8 +12,8 @@ fn help() { Usage: fabro [OPTIONS] [COMMAND] Commands: - run Register a local workflow version, create a run, and start it - create Register a local workflow version and create a submitted run + run Register a workflow version, create a run, and start it + create Register a workflow version and create a submitted run start Start a created workflow run on the server attach Attach to a running or finished workflow run events View the event log of a workflow run diff --git a/lib/apps/fabro-cli/tests/it/cmd/run.rs b/lib/apps/fabro-cli/tests/it/cmd/run.rs index 6f3856a0d..d755cb286 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/run.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/run.rs @@ -119,22 +119,27 @@ fn help() { success: true exit_code: 0 ----- stdout ----- - Register a local workflow version, create a run, and start it + Register a workflow version, create a run, and start it Usage: fabro run [OPTIONS] Arguments: - Local workflow name, checkout path, .fabro file, or workflow TOML + Workflow name or path (repository-relative with --workflow-git) Options: --json Output as JSON [env: FABRO_JSON=] --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) - --dry-run Execute with simulated LLM backend --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --auto-approve Auto-approve all human gates + --workflow-git Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials --quiet Suppress non-essential output [env: FABRO_QUIET=] + --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names + --target-path Observe this target directory instead of cwd; Folder targets require server filesystem access + --target-git Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials + --target-branch Target working branch (default: remote default branch), pinned to its observed commit + --dry-run Simulate execution; workflow source may still be fetched and uploaded + --auto-approve Auto-approve all human gates --goal Override the workflow goal (available as {{ goal }} in prompts) --goal-file Read a per-run goal value from a local file --model Override default LLM model From b0ed5450923cfe965e14b3a18ae4fea344b3158f Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 8 Sep 2026 16:09:12 -0400 Subject: [PATCH 02/15] 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 296dd81a0..3d43c52f1 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -101,7 +101,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 91048ea6c..a7a34873c 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -1268,6 +1268,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 @@ -2002,21 +2004,12 @@ fn parse_reasoning_effort_arg(value: &str) -> Result { #[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", @@ -2027,28 +2020,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, From e99d3cebe89dc284fd2f5caf5ec1ccff0e8bd2c0 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:33:18 -0600 Subject: [PATCH 03/15] Disable fsmonitor during native Git workflow checkout The hardened -c list for CLI-owned Git acquisition disabled hooks, LFS filters, submodules, and maintenance but omitted core.fsmonitor, so a user's global fsmonitor hook (or the builtin daemon) still ran during the temporary checkout. Match the sandbox's hardening and cover it in the hooks/filters test. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/remote_workflow.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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 8e9005abc..64d51d907 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -70,6 +70,8 @@ impl NativeGit { "-c", "core.hooksPath=/dev/null", "-c", + "core.fsmonitor=false", + "-c", "filter.lfs.smudge=", "-c", "filter.lfs.process=", @@ -574,9 +576,26 @@ mod tests { std::fs::Permissions::from_mode(0o755), ) .unwrap(); + // The fsmonitor hook runs during checkout even with hooksPath disabled. + std::fs::write( + hooks.join("fsmonitor"), + format!( + "#!/bin/sh\ntouch '{}'\nprintf 'token\\0/\\0'\n", + sentinel.display() + ), + ) + .unwrap(); + std::fs::set_permissions( + hooks.join("fsmonitor"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); config .set_str("core.hooksPath", hooks.to_str().unwrap()) .unwrap(); + config + .set_str("core.fsmonitor", hooks.join("fsmonitor").to_str().unwrap()) + .unwrap(); config .set_str( "filter.fixture.smudge", From d87f94bead586c806bc9aee9251547badbb6a129 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:33:19 -0600 Subject: [PATCH 04/15] Only kill the Git process group when a command fails The native Git runner SIGKILLed its child's process group after every command, including successful ones. Git spawns credential-cache--daemon into the same group, so each ls-remote or fetch destroyed the cache it had just warmed and every later command re-ran the full helper chain. Kill the group only on timeout, cancellation, or failure, and drop the redundant kill/wait on an already-reaped child. Co-Authored-By: Claude Fable 5.1 --- .../fabro-cli/src/commands/run/remote_workflow.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) 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 64d51d907..ccecc9dba 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -125,16 +125,17 @@ impl NativeGit { Ok(stdout) } => result, }; - // Helpers may inherit pipes or survive their Git parent. Terminate the - // owned process group on both completion and interruption, then reap Git. - #[cfg(unix)] - if let Some(id) = process_id { - fabro_proc::sigkill_process_group(id); - } + // A timed-out, cancelled, or failed Git may leave helpers running in + // its process group; terminate the group, then reap Git. A successful + // Git has closed its pipes, and helpers it deliberately left behind + // (such as `credential-cache--daemon`) keep serving later commands. if result.is_err() { + #[cfg(unix)] + if let Some(id) = process_id { + fabro_proc::sigkill_process_group(id); + } child.kill().await?; } - child.wait().await?; result } From bba5e2b20bc0ee6d9272406c17df8cbd5f49df7a Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:34:52 -0600 Subject: [PATCH 05/15] Run native Git metadata lookups from an owned scratch repository ls-remote ran in the caller's working directory while fetch, cat-file, checkout, and rev-parse ran inside the temporary checkout, so the lookup honored repository-local config (url.*.insteadOf, credential.*, http.*, core.sshCommand) that the fetch never saw, and a broken .git in the caller's directory failed the lookup outright. Initialize the scratch repository first and run every command from it, so all steps see the same configuration. Target resolution uses a short-lived scratch repository of its own and still creates no checkout. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/remote_workflow.rs | 89 ++++++++++++------- .../fabro-cli/src/commands/run/resolution.rs | 4 +- 2 files changed, 60 insertions(+), 33 deletions(-) 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 ccecc9dba..2e1dc417d 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -35,20 +35,22 @@ pub(super) enum RemoteWorkflowError { Io(#[from] std::io::Error), } +/// Every command runs inside a scratch repository Fabro owns, never the +/// caller's working directory, so metadata lookup, fetch, and checkout all see +/// the same Git configuration: the user's global and system config applies, +/// repository-local config from wherever the CLI was invoked does not. pub(super) struct NativeGit { - cwd: PathBuf, timeout: Duration, #[cfg(test)] pub(super) environment: Vec<(String, String)>, } impl NativeGit { - pub(super) fn new(cwd: PathBuf) -> Self { + pub(super) fn new() -> Self { Self { - cwd, - timeout: Duration::from_mins(2), + timeout: Duration::from_mins(2), #[cfg(test)] - environment: Vec::new(), + environment: Vec::new(), } } @@ -139,8 +141,28 @@ impl NativeGit { result } + /// Create and initialize an empty scratch repository. Temporary files are + /// removed when the returned directory drops. + async fn scratch_repository( + &self, + cancel: &CancellationToken, + ) -> anyhow::Result { + let scratch = tempfile::Builder::new() + .prefix("fabro-workflow-") + .tempdir()?; + self.command( + "repository initialization", + scratch.path(), + &["init", "--quiet", "--template="], + cancel, + ) + .await?; + Ok(scratch) + } + async fn records( &self, + root: &Path, repository: &GitHubRepositorySlug, patterns: &[String], cancel: &CancellationToken, @@ -148,9 +170,7 @@ 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("metadata lookup", &self.cwd, &args, cancel) - .await?; + let bytes = self.command("metadata lookup", root, &args, cancel).await?; Ok(std::str::from_utf8(&bytes) .context("local Git returned invalid metadata encoding")? .to_owned()) @@ -162,15 +182,19 @@ impl NativeGit { branch: Option, cancel: &CancellationToken, ) -> anyhow::Result { + let scratch = self.scratch_repository(cancel).await?; + let root = scratch.path(); let (branch, sha) = if let Some(branch) = branch { let reference = format!("refs/heads/{branch}"); let records = self - .records(&repository, std::slice::from_ref(&reference), cancel) + .records(root, &repository, std::slice::from_ref(&reference), cancel) .await?; let sha = exact_record(&records, &reference)?.context("target branch was not found")?; (branch, sha) } else { - let records = self.records(&repository, &["HEAD".into()], cancel).await?; + let records = self + .records(root, &repository, &["HEAD".into()], cancel) + .await?; default_head(&records)? }; let target = GitRunTarget { @@ -182,8 +206,11 @@ impl NativeGit { Ok(target.validate()?.into_target()) } - pub(super) async fn resolve_revision( + /// Resolve `revision` to a commit SHA using metadata lookups run from + /// `root`, an initialized scratch repository. + async fn resolve_revision( &self, + root: &Path, repository: &GitHubRepositorySlug, revision: &RemoteWorkflowRevision, cancel: &CancellationToken, @@ -191,13 +218,15 @@ impl NativeGit { match revision { RemoteWorkflowRevision::Commit(sha) => Ok(sha.clone()), RemoteWorkflowRevision::DefaultBranch => { - let records = self.records(repository, &["HEAD".into()], cancel).await?; + let records = self + .records(root, repository, &["HEAD".into()], cancel) + .await?; Ok(default_head(&records)?.1) } RemoteWorkflowRevision::Ref(reference) => { let candidates = RefCandidates::new(reference); let records = self - .records(repository, &candidates.patterns(), cancel) + .records(root, repository, &candidates.patterns(), cancel) .await?; candidates.resolve(&records) } @@ -211,12 +240,10 @@ impl NativeGit { revision: RemoteWorkflowRevision, cancel: CancellationToken, ) -> anyhow::Result { + let checkout = self.scratch_repository(&cancel).await?; let sha = self - .resolve_revision(&repository, &revision, &cancel) + .resolve_revision(checkout.path(), &repository, &revision, &cancel) .await?; - let checkout = tempfile::Builder::new() - .prefix("fabro-workflow-") - .tempdir()?; self.collect_checkout(repository, selector, sha, checkout, cancel) .await } @@ -251,6 +278,8 @@ impl NativeGit { Ok(closure) } + /// Fetch and check out `sha` into `root`, an initialized scratch + /// repository. async fn checkout( &self, repository: &GitHubRepositorySlug, @@ -258,13 +287,6 @@ impl NativeGit { root: &Path, cancel: &CancellationToken, ) -> anyhow::Result<()> { - self.command( - "repository initialization", - root, - &["init", "--quiet", "--template="], - cancel, - ) - .await?; self.command( "fetch", root, @@ -530,7 +552,7 @@ mod tests { ), ) .unwrap(); - let mut git = NativeGit::new(root.path().to_path_buf()); + let mut git = NativeGit::new(); git.environment = vec![ ("GIT_CONFIG_GLOBAL".into(), config.to_str().unwrap().into()), ("GIT_CONFIG_NOSYSTEM".into(), "1".into()), @@ -606,12 +628,13 @@ mod tests { 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); + let cancel = CancellationToken::new(); for selector in [ "review", ".fabro/workflows/review/workflow.fabro", "missing", ] { - let checkout = tempfile::tempdir().unwrap(); + let checkout = fixture.git.scratch_repository(&cancel).await.unwrap(); let checkout_path = checkout.path().to_path_buf(); let result = fixture .git @@ -636,7 +659,7 @@ mod tests { } assert!(!sentinel.exists()); } - let checkout = tempfile::tempdir().unwrap(); + let checkout = fixture.git.scratch_repository(&cancel).await.unwrap(); let path = checkout.path().to_path_buf(); assert!( fixture @@ -646,7 +669,7 @@ mod tests { "review".into(), "1111111111111111111111111111111111111111".into(), checkout, - CancellationToken::new() + cancel ) .await .is_err() @@ -740,6 +763,7 @@ mod tests { .tag("annotated", &object, &signature, "release", false) .unwrap(); let cancel = CancellationToken::new(); + let scratch = fixture.git.scratch_repository(&cancel).await.unwrap(); for reference in [ None, Some("HEAD"), @@ -755,7 +779,7 @@ mod tests { assert_eq!( fixture .git - .resolve_revision(&Fixture::repository(), &revision, &cancel) + .resolve_revision(scratch.path(), &Fixture::repository(), &revision, &cancel) .await .unwrap(), fixture.sha @@ -769,6 +793,7 @@ mod tests { fixture .git .resolve_revision( + scratch.path(), &Fixture::repository(), &RemoteWorkflowRevision::Ref("trunk".into()), &cancel @@ -783,6 +808,7 @@ mod tests { fixture .git .resolve_revision( + scratch.path(), &Fixture::repository(), &RemoteWorkflowRevision::Ref(reference.into()), &cancel @@ -868,9 +894,11 @@ mod tests { async fn remote_workflow_fetches_observed_commit_after_branch_moves() { let fixture = Fixture::new(); let cancel = CancellationToken::new(); + let checkout = fixture.git.scratch_repository(&cancel).await.unwrap(); let captured = fixture .git .resolve_revision( + checkout.path(), &Fixture::repository(), &RemoteWorkflowRevision::DefaultBranch, &cancel, @@ -896,7 +924,6 @@ mod tests { ) .unwrap(); assert_ne!(next.to_string(), captured); - let checkout = tempfile::tempdir().unwrap(); fixture .git .checkout(&Fixture::repository(), &captured, checkout.path(), &cancel) @@ -945,7 +972,7 @@ mod tests { let path = root.path().join("git"); std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); - let mut git = NativeGit::new(root.path().to_path_buf()); + let mut git = NativeGit::new(); git.environment .push(("PATH".into(), root.path().to_str().unwrap().into())); (root, git) diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index c5a034302..a6fa1daa3 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -54,7 +54,7 @@ pub(super) async fn workflow( selector, revision, } => { - let git = NativeGit::new(cwd.to_path_buf()); + let git = NativeGit::new(); let (repository, selector, revision) = (repository.clone(), selector.clone(), revision.clone()); let closure = remote_workflow::owned(move |cancel| async move { @@ -80,7 +80,7 @@ pub(super) async fn target( if !provider.is_clone_based() { bail!("Git targets require a clone-enabled Docker or Daytona environment"); } - let git = NativeGit::new(cwd.to_path_buf()); + let git = NativeGit::new(); let (repository, branch) = (repository.clone(), branch.clone()); let target = remote_workflow::owned(move |cancel| async move { git.resolve_target(repository, branch, &cancel).await From 3163e71236b7ba5545b839b50435caeddc3f04b5 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:35:41 -0600 Subject: [PATCH 06/15] Reject unusable remote default branches with a --target-branch hint Target resolution accepted any remote default HEAD that passed the ref selector grammar, then failed inside GitRunTarget::validate with a generic branch-grammar error when the default branch was something like heads/main or tags/release. Validate the default branch as a working branch name up front and point the user at --target-branch, since they passed no branch at all. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/remote_workflow.rs | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) 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 2e1dc417d..b0117f14f 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -195,7 +195,7 @@ impl NativeGit { let records = self .records(root, &repository, &["HEAD".into()], cancel) .await?; - default_head(&records)? + default_target_branch(&records)? }; let target = GitRunTarget { repo: repository.to_string(), @@ -402,6 +402,20 @@ fn default_head(records: &str) -> anyhow::Result<(String, String)> { )) } +/// The remote default branch as a run target. `GitRunTarget` requires a bare +/// working branch name, which is stricter than the ref grammar `default_head` +/// accepts for workflow acquisition; report the mismatch with the flag that +/// resolves it instead of a generic branch-grammar error. +fn default_target_branch(records: &str) -> anyhow::Result<(String, String)> { + let (branch, sha) = default_head(records)?; + if !repository::is_valid_git_branch_name(&branch) { + bail!( + "remote default branch `{branch}` cannot name a run target branch; pass --target-branch to select a working branch" + ); + } + Ok((branch, sha)) +} + /// 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. @@ -953,6 +967,26 @@ mod tests { ); } + #[test] + fn remote_workflow_default_target_branch_requires_a_working_branch_name() { + let sha = "1234567890123456789012345678901234567890"; + for (head, valid) in [ + ("trunk", true), + ("topic/slash", true), + ("heads/main", false), + ("tags/release", false), + (sha, false), + ] { + let records = format!("ref: refs/heads/{head}\tHEAD\n{sha}\tHEAD\n"); + assert_eq!(default_head(&records).unwrap().0, head); + let target = default_target_branch(&records); + assert_eq!(target.is_ok(), valid, "{head}"); + if !valid { + assert!(target.unwrap_err().to_string().contains("--target-branch")); + } + } + } + #[test] fn remote_workflow_matches_records_exactly_and_rejects_host_symlinks() { let sha = "1234567890123456789012345678901234567890"; From bf41ad8a24966b5095fa0e9f77ea9cb0acf23141 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:36:05 -0600 Subject: [PATCH 07/15] Classify workflow refs once when parsing --workflow-ref RemoteWorkflowRevision::parse already enforced which ref namespaces a value may name, but RefCandidates re-derived the branch/tag split from the string and treated everything under refs/ that was not refs/heads/ as a tag, relying on an invariant checked in another file. Parse now yields Branch, Tag, or Name variants and resolution matches on them directly, replacing the Option/Option candidate encoding and its impossible (None, None) input state. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/remote_workflow.rs | 127 ++++++++---------- .../fabro-cli/src/commands/run/selection.rs | 61 ++++++--- 2 files changed, 99 insertions(+), 89 deletions(-) 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 b0117f14f..1a77a07c0 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -223,12 +223,25 @@ impl NativeGit { .await?; Ok(default_head(&records)?.1) } - RemoteWorkflowRevision::Ref(reference) => { - let candidates = RefCandidates::new(reference); + RemoteWorkflowRevision::Branch(reference) => { let records = self - .records(root, repository, &candidates.patterns(), cancel) + .records(root, repository, std::slice::from_ref(reference), cancel) .await?; - candidates.resolve(&records) + exact_record(&records, reference)?.context(REF_NOT_FOUND) + } + RemoteWorkflowRevision::Tag(reference) => { + let records = self + .records(root, repository, &tag_patterns(reference), cancel) + .await?; + tag_commit(&records, reference)?.context(REF_NOT_FOUND) + } + RemoteWorkflowRevision::Name(name) => { + let branch = format!("refs/heads/{name}"); + let tag = format!("refs/tags/{name}"); + let mut patterns = vec![branch.clone()]; + patterns.extend(tag_patterns(&tag)); + let records = self.records(root, repository, &patterns, cancel).await?; + resolve_name(&records, &branch, &tag) } } } @@ -416,72 +429,32 @@ fn default_target_branch(records: &str) -> anyhow::Result<(String, String)> { Ok((branch, sha)) } -/// 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, +const REF_NOT_FOUND: &str = "workflow ref was not found; no alternative revision was selected"; + +/// `ls-remote` patterns for a tag: the tag itself and its peeled commit. +fn tag_patterns(tag: &str) -> [String; 2] { + [tag.to_owned(), format!("{tag}^{{}}")] } -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}")), - } - } - } +/// The commit a tag names, preferring the peeled commit of an annotated tag +/// over the tag object. +fn tag_commit(records: &str, tag: &str) -> anyhow::Result> { + let Some(sha) = exact_record(records, tag)? else { + return Ok(None); + }; + Ok(Some( + exact_record(records, &format!("{tag}^{{}}"))?.unwrap_or(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() - } - - 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") - } - } +/// A bare name may be a branch or a tag; it must be exactly one. +fn resolve_name(records: &str, branch: &str, tag: &str) -> anyhow::Result { + match (exact_record(records, branch)?, tag_commit(records, tag)?) { + (Some(_), Some(_)) => bail!( + "workflow ref is ambiguous between a branch and tag; use refs/heads/... or refs/tags/..." + ), + (Some(sha), None) | (None, Some(sha)) => Ok(sha), + (None, None) => bail!(REF_NOT_FOUND), } } @@ -809,7 +782,7 @@ mod tests { .resolve_revision( scratch.path(), &Fixture::repository(), - &RemoteWorkflowRevision::Ref("trunk".into()), + &RemoteWorkflowRevision::Name("trunk".into()), &cancel ) .await @@ -817,18 +790,21 @@ mod tests { .to_string() .contains("ambiguous") ); - for reference in ["missing", "refs/tags/missing"] { + for reference in ["missing", "refs/tags/missing", "refs/heads/missing"] { assert!( fixture .git .resolve_revision( scratch.path(), &Fixture::repository(), - &RemoteWorkflowRevision::Ref(reference.into()), + &RemoteWorkflowRevision::parse(Some(reference)).unwrap(), &cancel ) .await - .is_err() + .unwrap_err() + .to_string() + .contains("was not found"), + "{reference}" ); } let target = fixture @@ -991,9 +967,12 @@ mod tests { fn remote_workflow_matches_records_exactly_and_rejects_host_symlinks() { let sha = "1234567890123456789012345678901234567890"; assert!( - RefCandidates::new("main") - .resolve(&format!("{sha}\trefs/heads/nested/main\n")) - .is_err() + resolve_name( + &format!("{sha}\trefs/heads/nested/main\n"), + "refs/heads/main", + "refs/tags/main" + ) + .is_err() ); let root = tempfile::tempdir().unwrap(); let host = tempfile::tempdir().unwrap(); diff --git a/lib/apps/fabro-cli/src/commands/run/selection.rs b/lib/apps/fabro-cli/src/commands/run/selection.rs index 409936aa9..e560a6b4f 100644 --- a/lib/apps/fabro-cli/src/commands/run/selection.rs +++ b/lib/apps/fabro-cli/src/commands/run/selection.rs @@ -28,10 +28,17 @@ pub(super) enum TargetSelection { }, } +/// A validated `--workflow-ref`, classified once so resolution never re-derives +/// which ref namespaces a value may name. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) enum RemoteWorkflowRevision { DefaultBranch, - Ref(String), + /// A fully qualified `refs/heads/...` branch. + Branch(String), + /// A fully qualified `refs/tags/...` tag. + Tag(String), + /// A bare name that may be a branch or a tag. + Name(String), Commit(String), } @@ -43,14 +50,19 @@ impl RemoteWorkflowRevision { if let Some(sha) = repository::normalize_git_commit_sha(value) { return Ok(Self::Commit(sha)); } - if !repository::is_valid_github_ref_selector(value) - || (value.starts_with("refs/") - && !value.starts_with("refs/heads/") - && !value.starts_with("refs/tags/")) - { + if !repository::is_valid_github_ref_selector(value) { bail!("workflow ref must be a branch, tag, HEAD, or full 40-hex commit SHA"); } - Ok(Self::Ref(value.to_owned())) + let reference = value.to_owned(); + if value.starts_with("refs/heads/") { + Ok(Self::Branch(reference)) + } else if value.starts_with("refs/tags/") { + Ok(Self::Tag(reference)) + } else if value.starts_with("refs/") { + bail!("workflow ref must be a branch, tag, HEAD, or full 40-hex commit SHA"); + } else { + Ok(Self::Name(reference)) + } } } } @@ -139,15 +151,34 @@ mod tests { ] { assert!(validate_remote_selector(Path::new(path)).is_err(), "{path}"); } - for value in [ - "topic/slash", - "refs/heads/release", - "refs/tags/v1", - "HEAD", - "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd", + for (value, expected) in [ + ( + "topic/slash", + RemoteWorkflowRevision::Name("topic/slash".into()), + ), + ( + "refs/heads/release", + RemoteWorkflowRevision::Branch("refs/heads/release".into()), + ), + ( + "refs/tags/v1", + RemoteWorkflowRevision::Tag("refs/tags/v1".into()), + ), + ("HEAD", RemoteWorkflowRevision::DefaultBranch), + ( + "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd", + RemoteWorkflowRevision::Commit("abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd".into()), + ), ] { - RemoteWorkflowRevision::parse(Some(value)).unwrap(); + assert_eq!( + RemoteWorkflowRevision::parse(Some(value)).unwrap(), + expected + ); } + assert_eq!( + RemoteWorkflowRevision::parse(None).unwrap(), + RemoteWorkflowRevision::DefaultBranch + ); for value in [ "--upload-pack=x", "topic*", @@ -199,7 +230,7 @@ mod adapter_tests { WorkflowSelection::Git { repository: "acme/workflows".parse().unwrap(), selector: "review".into(), - revision: RemoteWorkflowRevision::Ref("v1".into()), + revision: RemoteWorkflowRevision::Name("v1".into()), }, TargetSelection::Git { repository: "acme/app".parse().unwrap(), From 0998f60624b76b6cc954af1aeb00d0960251b10d Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:40:46 -0600 Subject: [PATCH 08/15] Contain the selected workflow file instead of pre-walking the checkout Remote acquisition walked the entire depth-1 checkout and failed on any symlink that dangled or resolved outside the root, even when the link was nowhere near the selected workflow. Submodule-style dangling links and links into the host are common in workflow repositories and made --workflow-git fail where the same commit collected fine locally. The bundler already root-checks every file it opens; the only unchecked reads were the selected TOML (or a graph selector's sibling TOML) during location resolution. Check those in collect_workflow_versions and drop the O(repo) walk. walkdir stays a dev-dependency for the dump tests. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/Cargo.toml | 2 +- .../src/commands/run/remote_workflow.rs | 52 ++++---- .../src/workflow_version_collector.rs | 115 ++++++++++++++++-- 3 files changed, 133 insertions(+), 36 deletions(-) diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 3d43c52f1..a91a23eb2 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -96,7 +96,6 @@ serde_yaml = "0.9" tempfile = "3" sha2.workspace = true shlex = "1" -walkdir.workspace = true object_store.workspace = true bytes.workspace = true tokio-util.workspace = true @@ -118,6 +117,7 @@ chrono = { workspace = true } [dev-dependencies] assert_cmd = "2" +walkdir.workspace = true fabro-acp = { path = "../../components/fabro-acp", features = ["test-support"] } fabro-mcp = { path = "../../components/fabro-mcp", features = ["test-support"] } fabro-build-support = { path = "../../foundation/build-support" } 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 1a77a07c0..6a7944c92 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -276,7 +276,6 @@ impl NativeGit { // it and clean up before reporting cancellation. let collection_cancel = cancel.clone(); let closure = task::spawn_blocking(move || { - check_source_symlinks(checkout.path())?; if collection_cancel.is_cancelled() { return Err(RemoteWorkflowError::Cancelled.into()); } @@ -458,23 +457,6 @@ fn resolve_name(records: &str, branch: &str, tag: &str) -> anyhow::Result 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() != git_dir) - { - let entry = entry?; - if entry.file_type().is_symlink() && !entry.path().canonicalize()?.starts_with(&root) { - bail!("remote workflow checkout contains a symlink outside its source root"); - } - } - Ok(()) -} - /// The task owns its child processes and temporary checkout. Dropping the /// waiter requests cooperative cleanup, not task abortion. Ctrl-C waits for /// cleanup; an in-progress blocking collection must finish first. @@ -664,6 +646,29 @@ mod tests { assert!(!path.exists()); } + #[tokio::test] + async fn remote_workflow_tolerates_symlinks_the_selected_workflow_never_reads() { + let fixture = Fixture::new(); + let source = fixture.repo.workdir().unwrap(); + // Submodule-style dangling links and links outside the checkout are + // common in workflow repositories and irrelevant to the selection. + std::os::unix::fs::symlink("../missing-submodule", source.join("vendor")).unwrap(); + std::os::unix::fs::symlink("/usr/local/lib/node_modules", source.join("tools")).unwrap(); + let sha = commit_all(&fixture.repo, "add links"); + let local = fabro_manifest::collect_workflow_versions(Path::new("review"), source).unwrap(); + let remote = fixture + .git + .collect( + Fixture::repository(), + "review".into(), + RemoteWorkflowRevision::Commit(sha), + CancellationToken::new(), + ) + .await + .unwrap(); + assert_eq!(local.root_id(), remote.root_id()); + } + #[tokio::test] async fn remote_workflow_rejects_toml_symlink_before_reading_host_content() { let fixture = Fixture::new(); @@ -690,7 +695,10 @@ mod tests { ) .await .unwrap_err(); - assert!(error.to_string().contains("outside its source root")); + assert!( + format!("{error:?}").contains("outside its source root"), + "{error:?}" + ); } #[tokio::test] @@ -964,7 +972,7 @@ mod tests { } #[test] - fn remote_workflow_matches_records_exactly_and_rejects_host_symlinks() { + fn remote_workflow_matches_records_exactly() { let sha = "1234567890123456789012345678901234567890"; assert!( resolve_name( @@ -974,10 +982,6 @@ mod tests { ) .is_err() ); - let root = tempfile::tempdir().unwrap(); - let host = tempfile::tempdir().unwrap(); - std::os::unix::fs::symlink(host.path(), root.path().join("outside")).unwrap(); - assert!(check_source_symlinks(root.path()).is_err()); } fn fake_git(script: &str) -> (tempfile::TempDir, NativeGit) { diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 131f9990c..6274ad116 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -111,17 +111,6 @@ pub fn collect_workflow_versions( checkout_root: &Path, ) -> 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(), - }, - })?; - let package_root = checkout_root .canonicalize() @@ -132,6 +121,21 @@ pub fn collect_workflow_versions( checkout_root.display() )), })?; + ensure_selection_contained( + &checkout_root.join(&repository_workflow), + &package_root, + 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(), + }, + })?; let location = canonicalize_location(location, |path, source| { WorkflowVersionCollectError::Collect { path: workflow.to_path_buf(), @@ -186,6 +190,51 @@ pub fn collect_workflow_versions_at_location( VersionAssembler::new(collected).assemble() } +/// Location resolution reads the selected TOML, or a selected graph's sibling +/// `workflow.toml`, before the bundler's root-checked reads begin. Refuse a +/// selection whose file resolves outside the package first, so a symlink in an +/// untrusted checkout never reads host content. Missing files are left for +/// resolution to report; symlinks elsewhere in the checkout are irrelevant +/// because every file the bundler reads is checked when it is opened. +fn ensure_selection_contained( + selected: &Path, + package_root: &Path, + workflow: &Path, +) -> Result<(), WorkflowVersionCollectError> { + let mut candidates = vec![selected.to_path_buf()]; + if selected + .extension() + .is_none_or(|extension| extension != "toml") + { + candidates.push(selected.with_file_name("workflow.toml")); + } + for path in candidates { + if path.symlink_metadata().is_err() { + continue; + } + let canonical = + path.canonicalize() + .map_err(|source| WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::Error::new(source).context(format!( + "failed to canonicalize workflow file {}", + path.display() + )), + })?; + if !canonical.starts_with(package_root) { + return Err(WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source: anyhow::anyhow!( + "workflow file `{}` resolves outside its source root `{}`", + path.display(), + package_root.display() + ), + }); + } + } + Ok(()) +} + fn repository_workflow_path(workflow: &Path) -> PathBuf { if workflow.is_relative() && workflow.extension().is_none() { Path::new(".fabro/workflows") @@ -425,6 +474,50 @@ dockerfile = { path = "Dockerfile" } ); } + #[cfg(unix)] + #[test] + fn rejects_selected_files_that_resolve_outside_the_package_root() { + let host = tempfile::tempdir().unwrap(); + write(host.path(), "workflow.toml", "_version = 1\n"); + for selector in ["root", ".fabro/workflows/root/workflow.fabro"] { + let temp = tempfile::tempdir().unwrap(); + write_complete_fixture(temp.path()); + let toml = temp.path().join(".fabro/workflows/root/workflow.toml"); + fs::remove_file(&toml).unwrap(); + std::os::unix::fs::symlink(host.path().join("workflow.toml"), &toml).unwrap(); + let error = collect_workflow_versions(Path::new(selector), temp.path()).unwrap_err(); + assert!( + format!("{error:?}").contains("outside its source root"), + "{selector}: {error:?}" + ); + // A dangling selection is refused rather than reported as missing. + fs::remove_file(&toml).unwrap(); + std::os::unix::fs::symlink(host.path().join("missing.toml"), &toml).unwrap(); + let error = collect_workflow_versions(Path::new(selector), temp.path()).unwrap_err(); + assert!( + format!("{error:?}").contains("failed to canonicalize workflow file"), + "{selector}: {error:?}" + ); + } + } + + #[cfg(unix)] + #[test] + fn ignores_symlinks_the_selected_workflow_never_reads() { + let host = tempfile::tempdir().unwrap(); + let temp = tempfile::tempdir().unwrap(); + write_complete_fixture(temp.path()); + std::os::unix::fs::symlink(host.path(), temp.path().join("tools")).unwrap(); + std::os::unix::fs::symlink("../missing", temp.path().join("vendor")).unwrap(); + std::os::unix::fs::symlink( + "/nonexistent/module", + temp.path().join(".fabro/workflows/root/unrelated"), + ) + .unwrap(); + let closure = collect_workflow_versions(Path::new("root"), temp.path()).unwrap(); + assert_eq!(closure.versions().count(), 2); + } + #[test] fn packages_named_workflow_without_project_config() { let temp = tempfile::tempdir().unwrap(); From 39694ec5e8231d6e9cf9f66768fcfb0143383908 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:42:10 -0600 Subject: [PATCH 09/15] Acquire the remote workflow before observing a local Git target For --workflow-git selections, target resolution ran before the remote workflow ref was verified to exist. On Docker and Daytona environments a path target that is a GitHub checkout is observed via observe_git_run_target, which may silently push the attached branch, so a typo in --workflow-ref produced a remote side effect with no run created. Resolve the remote workflow after parent and environment validation but before target observation, restoring the pre-existing workflow-then-target order, and cover it with a caller checkout whose unpushed branch must stay unpublished. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/src/commands/run/create.rs | 12 +++++---- lib/apps/fabro-cli/tests/it/cmd/create.rs | 27 ++++++++++++++++--- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index e3e365e6f..c7145ef73 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -44,7 +44,7 @@ pub(crate) async fn create_run( ) }; // Preserve local lookup diagnostics before contacting the server. Remote - // acquisition waits until parent, environment, and target are validated. + // acquisition waits until the parent and environment are validated. let local_package = match &workflow_selection { WorkflowSelection::Local(_) => Some(resolve_workflow().await?), WorkflowSelection::Git { .. } => None, @@ -84,6 +84,12 @@ pub(crate) async fn create_run( }, resolve_run_environment(client.as_ref(), args.environment.as_deref()), )?; + // Observing a local Git target may push its branch. Acquire the remote + // workflow first so a bad --workflow-ref never causes that side effect. + let package = match local_package { + Some(package) => package, + None => resolve_workflow().await?, + }; let (target, dirty_worktree) = resolution::target( &target_selection, environment.settings.provider, @@ -97,10 +103,6 @@ pub(crate) async fn create_run( styles.yellow.apply_to("Warning:"), ); } - let package = match local_package { - Some(package) => package, - None => resolve_workflow().await?, - }; let workflow_version_id = package.closure().root_id(); client .register_workflow_versions( diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index b94df6061..a232774e3 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -1889,7 +1889,17 @@ fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutati let source = root.path().join("source"); write_workflow(&source, ".fabro/workflows/other", "Other"); init_remote_fixture(&source, "trunk"); - write_workflow(root.path(), ".fabro/workflows/review", "Caller"); + // The caller is a GitHub-origin checkout with an unpushed branch, which + // Docker target observation would publish if it ran first. + let caller = root.path().join("caller"); + let origin = root.path().join("origin.git"); + git2::Repository::init_bare(&origin).unwrap(); + write_workflow(&caller, ".fabro/workflows/review", "Caller"); + init_remote_fixture(&caller, "main"); + git2::Repository::open(&caller) + .unwrap() + .remote("origin", "https://github.com/acme/app") + .unwrap(); write_workflow( &context.home_dir.join(".fabro/workflows"), "review", @@ -1899,8 +1909,9 @@ fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutati std::fs::write( &config, format!( - "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n", - source.display() + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n[url \"file://{}\"]\n insteadOf = https://github.com/acme/app\n", + source.display(), + origin.display() ), ) .unwrap(); @@ -1911,7 +1922,7 @@ fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutati ] { let output = context .create_cmd() - .current_dir(root.path()) + .current_dir(&caller) .env("GIT_CONFIG_GLOBAL", &config) .env("GIT_CONFIG_NOSYSTEM", "1") .env("GIT_CONFIG_COUNT", "0") @@ -1932,4 +1943,12 @@ fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutati environment.assert_calls(3); version.assert_calls(0); create.assert_calls(0); + // Acquisition failed before target observation, so nothing was pushed. + assert!( + git2::Repository::open_bare(&origin) + .unwrap() + .find_reference("refs/heads/main") + .is_err(), + "a failed remote workflow acquisition must not publish the target branch" + ); } From f15972f8f2198a861499082338dd79f3167b37a9 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:46:30 -0600 Subject: [PATCH 10/15] Keep Ctrl-C owned for the whole native Git command phase owned() polled tokio's ctrl_c() only for the duration of one Git acquisition. On Unix that listener permanently replaces the default SIGINT disposition, so once acquisition finished nothing handled Ctrl-C and fabro create, fabro run --detach, and fabro run before attach silently ignored it while waiting on the server. Introduce an Interruption handle that the command entry points create from the run arguments: it installs a listener only when --workflow-git or --target-git is in play, guards create (and start for fabro run) as one phase, and tracks owned Git tasks so interruption waits for their cleanup before returning. attach keeps installing its own listener. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/Cargo.toml | 2 +- .../fabro-cli/src/commands/run/command.rs | 38 +++-- lib/apps/fabro-cli/src/commands/run/create.rs | 7 + lib/apps/fabro-cli/src/commands/run/mod.rs | 10 +- .../src/commands/run/remote_workflow.rs | 155 ++++++++++++++---- .../fabro-cli/src/commands/run/resolution.rs | 47 ++++-- 6 files changed, 200 insertions(+), 59 deletions(-) diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index a91a23eb2..e953d0a26 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -100,7 +100,7 @@ object_store.workspace = true bytes.workspace = true tokio-util.workspace = true libc = "0.2" -nix = { version = "0.30", features = ["fs"] } +nix = { version = "0.30", features = ["fs", "signal"] } [target.'cfg(target_os = "macos")'.dependencies] core-foundation = { version = "0.9", optional = true } diff --git a/lib/apps/fabro-cli/src/commands/run/command.rs b/lib/apps/fabro-cli/src/commands/run/command.rs index 2ec25f636..2dfdd235a 100644 --- a/lib/apps/fabro-cli/src/commands/run/command.rs +++ b/lib/apps/fabro-cli/src/commands/run/command.rs @@ -1,6 +1,7 @@ use anyhow::Result; use fabro_util::terminal::Styles; +use super::remote_workflow::Interruption; use crate::args::RunArgs; use crate::command_context::CommandContext; use crate::shared::print_json_pretty; @@ -15,16 +16,32 @@ pub(crate) async fn execute(mut args: RunArgs, base_ctx: &CommandContext) -> Res let quiet = args.detach; let prevent_idle_sleep = ctx.user_settings().cli.exec.prevent_idle_sleep; - let created_run = Box::pin(super::create::create_run(&ctx, &args, styles)).await?; + // Ctrl-C stays owned here through start; `attach` installs its own listener. + let interruption = Interruption::for_run_args(&args); + let (created_run, client) = interruption + .guard(async { + let created_run = Box::pin(super::create::create_run( + &ctx, + &args, + styles, + &interruption, + )) + .await?; - if !quiet { - fabro_util::printerr!( - printer, - " {} {}", - styles.dim.apply_to("Run:"), - styles.dim.apply_to(&created_run.run_id), - ); - } + if !quiet { + fabro_util::printerr!( + printer, + " {} {}", + styles.dim.apply_to("Run:"), + styles.dim.apply_to(&created_run.run_id), + ); + } + + let client = ctx.server().await?; + super::start::start_run_with_client(&client, &created_run.run_id, false).await?; + Ok((created_run, client)) + }) + .await?; #[cfg(feature = "sleep_inhibitor")] let _sleep_guard = sleep_inhibitor::guard(prevent_idle_sleep); @@ -32,9 +49,6 @@ pub(crate) async fn execute(mut args: RunArgs, base_ctx: &CommandContext) -> Res #[cfg(not(feature = "sleep_inhibitor"))] let _ = prevent_idle_sleep; - let client = ctx.server().await?; - super::start::start_run_with_client(&client, &created_run.run_id, false).await?; - let json = ctx.json_output(); if args.detach { if json { diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index c7145ef73..ed69e165c 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -7,6 +7,7 @@ use fabro_types::{RunId, RunIntent}; use fabro_util::terminal::Styles; use super::overrides::prepare_intent_overrides; +use super::remote_workflow::Interruption; use super::resolution::ResolvedWorkflow; use super::selection::WorkflowSelection; use super::{resolution, selection}; @@ -23,10 +24,14 @@ pub(crate) struct CreatedRun { /// run from an immutable workflow intent, leaving it in the submitted state. /// /// This does NOT start the workflow — starting is a separate request. +/// +/// Native Git acquisition runs under `interruption`; the caller guards this +/// call (and any later phase before `attach`) with the same handle. pub(crate) async fn create_run( ctx: &CommandContext, args: &RunArgs, styles: &Styles, + interruption: &Interruption, ) -> anyhow::Result { let (workflow_selection, target_selection) = selection::parse(args)?; let canonical_cwd = ctx.cwd().canonicalize().with_context(|| { @@ -41,6 +46,7 @@ pub(crate) async fn create_run( &workflow_selection, &canonical_cwd, Some(&user_workflows_root), + interruption, ) }; // Preserve local lookup diagnostics before contacting the server. Remote @@ -94,6 +100,7 @@ pub(crate) async fn create_run( &target_selection, environment.settings.provider, &canonical_cwd, + interruption, ) .await?; if dirty_worktree { diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index 6b9177673..b904549b4 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/lib/apps/fabro-cli/src/commands/run/mod.rs @@ -47,7 +47,15 @@ pub(crate) async fn dispatch( RunCommands::Create(args) => { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); let ctx = base_ctx.with_target(&args.target)?; - let created_run = Box::pin(create::create_run(&ctx, &args, styles)).await?; + let interruption = remote_workflow::Interruption::for_run_args(&args); + let created_run = interruption + .guard(Box::pin(create::create_run( + &ctx, + &args, + styles, + &interruption, + ))) + .await?; if ctx.json_output() { print_json_pretty(&serde_json::json!({ "run_id": created_run.run_id }))?; } else { 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 6a7944c92..015353490 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -11,8 +11,10 @@ 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 tokio_util::task::TaskTracker; use super::selection::RemoteWorkflowRevision; +use crate::args::RunArgs; const OUTPUT_LIMIT: usize = 64 * 1024; @@ -457,27 +459,76 @@ fn resolve_name(records: &str, branch: &str, tag: &str) -> anyhow::Result( - work: impl FnOnce(CancellationToken) -> TFuture + Send + 'static, -) -> anyhow::Result -where - TFuture: Future> + Send + 'static, -{ - let cancel = CancellationToken::new(); - let _cancel_on_drop = cancel.clone().drop_guard(); - let mut task = tokio::spawn(work(cancel.clone())); - tokio::select! { - result = &mut task => result.context("local Git task failed")?, - signal = tokio_signal::ctrl_c() => { - cancel.cancel(); - let _result = task.await.context("local Git cleanup task failed")?; - signal.context("failed to listen for interruption")?; - Err(RemoteWorkflowError::Cancelled.into()) +/// Cooperative Ctrl-C handling for commands that acquire sources with native +/// Git. +/// +/// Tokio's Ctrl-C listener permanently replaces the default SIGINT disposition +/// for the process, so it is installed only when native Git is in play, and the +/// command keeps it armed for every phase up to the point where `attach` +/// installs its own listener or the process exits. Interruption cancels owned +/// Git tasks and waits for their cleanup; an in-progress blocking collection +/// must finish first. +#[derive(Clone)] +pub(crate) struct Interruption { + cancel: CancellationToken, + tasks: TaskTracker, + listens: bool, +} + +impl Interruption { + /// `native_git` reports whether any selection runs native Git. Without it + /// the default SIGINT disposition is left untouched and `guard` is a + /// pass-through. + pub(crate) fn new(native_git: bool) -> Self { + Self { + cancel: CancellationToken::new(), + tasks: TaskTracker::new(), + listens: native_git, } } + + pub(crate) fn for_run_args(args: &RunArgs) -> Self { + Self::new(args.workflow_git.is_some() || args.target_git.is_some()) + } + + /// Run `work` to completion, or until Ctrl-C cancels it and every owned + /// Git task has cleaned up. + pub(crate) async fn guard( + &self, + work: impl Future>, + ) -> anyhow::Result { + if !self.listens { + return work.await; + } + tokio::select! { + result = work => result, + signal = tokio_signal::ctrl_c() => { + self.cancel.cancel(); + self.tasks.close(); + self.tasks.wait().await; + signal.context("failed to listen for interruption")?; + Err(RemoteWorkflowError::Cancelled.into()) + } + } + } + + /// The task owns its child processes and temporary checkout. Dropping the + /// waiter requests cooperative cleanup, not task abortion; the task keeps + /// running until its Git children are reaped and its files are removed. + pub(super) async fn owned( + &self, + work: impl FnOnce(CancellationToken) -> TFuture + Send + 'static, + ) -> anyhow::Result + where + TFuture: Future> + Send + 'static, + { + let cancel = self.cancel.child_token(); + let _cancel_on_drop = cancel.clone().drop_guard(); + self.tasks + .spawn(work(cancel.clone())) + .await + .context("local Git task failed")? + } } #[cfg(test)] @@ -487,7 +538,10 @@ where )] mod tests { use std::os::unix::fs::PermissionsExt as _; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + use nix::sys::signal::{self, Signal}; use nix::sys::stat::Mode; use nix::unistd; @@ -706,16 +760,20 @@ mod tests { let (fake, git) = fake_git("printf '%s' $$ > pid; exec /bin/sleep 60"); let checkout = tempfile::tempdir().unwrap(); let path = checkout.path().to_path_buf(); - let worker = tokio::spawn(owned(move |cancel| async move { - git.collect_checkout( - "acme/workflows".parse().unwrap(), - "review".into(), - "1111111111111111111111111111111111111111".into(), - checkout, - cancel, - ) - .await - })); + let worker = tokio::spawn(async move { + Interruption::new(true) + .owned(move |cancel| async move { + git.collect_checkout( + "acme/workflows".parse().unwrap(), + "review".into(), + "1111111111111111111111111111111111111111".into(), + checkout, + cancel, + ) + .await + }) + .await + }); time::timeout(Duration::from_secs(5), async { while !path.join("pid").exists() { time::sleep(Duration::from_millis(10)).await; @@ -740,6 +798,45 @@ mod tests { drop(fake); } + /// Each nextest test runs in its own process, so raising SIGINT here only + /// reaches the listener `guard` installed before polling `work`. + #[tokio::test] + async fn remote_workflow_guard_cancels_owned_tasks_and_waits_for_cleanup_on_ctrl_c() { + let interruption = Interruption::new(true); + let cleaned = Arc::new(AtomicBool::new(false)); + let result: anyhow::Result<()> = interruption + .guard({ + let interruption = interruption.clone(); + let cleaned = Arc::clone(&cleaned); + async move { + interruption + .owned(move |cancel| async move { + signal::raise(Signal::SIGINT).unwrap(); + cancel.cancelled().await; + // Cleanup after cancellation must finish before + // `guard` reports the interruption. + time::sleep(Duration::from_millis(200)).await; + cleaned.store(true, Ordering::SeqCst); + Ok(()) + }) + .await + } + }) + .await; + assert!(matches!( + result.unwrap_err().downcast_ref::(), + Some(RemoteWorkflowError::Cancelled) + )); + assert!(cleaned.load(Ordering::SeqCst)); + assert_eq!( + Interruption::new(false) + .guard(async { Ok::<_, anyhow::Error>(7) }) + .await + .unwrap(), + 7 + ); + } + #[tokio::test] async fn remote_workflow_resolves_exact_default_branches_tags_and_commits() { let fixture = Fixture::new(); diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index a6fa1daa3..556e92d1d 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -6,7 +6,7 @@ use fabro_types::settings::run::EnvironmentProvider; use fabro_types::{DirtyStatus, RunTarget}; use tokio::task; -use super::remote_workflow::{self, NativeGit}; +use super::remote_workflow::{Interruption, NativeGit}; use super::selection::{TargetSelection, WorkflowSelection}; /// Owns the canonical collector result without copying its contents. Local @@ -29,6 +29,7 @@ pub(super) async fn workflow( selection: &WorkflowSelection, cwd: &Path, user_workflows: Option<&Path>, + interruption: &Interruption, ) -> anyhow::Result { match selection { WorkflowSelection::Local(path) => { @@ -57,10 +58,11 @@ pub(super) async fn workflow( let git = NativeGit::new(); let (repository, selector, revision) = (repository.clone(), selector.clone(), revision.clone()); - let closure = remote_workflow::owned(move |cancel| async move { - git.collect(repository, selector, revision, cancel).await - }) - .await?; + let closure = interruption + .owned(move |cancel| async move { + git.collect(repository, selector, revision, cancel).await + }) + .await?; Ok(ResolvedWorkflow::Git(closure)) } } @@ -70,6 +72,7 @@ pub(super) async fn target( selection: &TargetSelection, provider: EnvironmentProvider, cwd: &Path, + interruption: &Interruption, ) -> anyhow::Result<(RunTarget, bool)> { let path = match selection { TargetSelection::Path(path) => cwd @@ -82,10 +85,11 @@ pub(super) async fn target( } let git = NativeGit::new(); let (repository, branch) = (repository.clone(), branch.clone()); - let target = remote_workflow::owned(move |cancel| async move { - git.resolve_target(repository, branch, &cancel).await - }) - .await?; + let target = interruption + .owned(move |cancel| async move { + git.resolve_target(repository, branch, &cancel).await + }) + .await?; // Canonical admission retains ownership of provider capabilities. return Ok((RunTarget::Git(target), false)); } @@ -199,8 +203,9 @@ mod tests { write_workflow(&root, ".fabro/workflows/review"); std::fs::create_dir(root.join("target")).unwrap(); let selected = TargetSelection::Path("target".into()); + let interruption = Interruption::new(false); assert_eq!( - target(&selected, EnvironmentProvider::Local, &root) + target(&selected, EnvironmentProvider::Local, &root, &interruption) .await .unwrap() .0, @@ -210,7 +215,10 @@ mod tests { ); for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { assert_eq!( - target(&selected, provider, &root).await.unwrap().0, + target(&selected, provider, &root, &interruption) + .await + .unwrap() + .0, RunTarget::None {} ); } @@ -218,7 +226,8 @@ mod tests { target( &TargetSelection::Path(".".into()), EnvironmentProvider::Local, - &root + &root, + &interruption ) .await .unwrap() @@ -234,7 +243,8 @@ mod tests { branch: None, }, EnvironmentProvider::Local, - &root + &root, + &interruption ) .await .unwrap_err() @@ -246,7 +256,8 @@ mod tests { target( &TargetSelection::Path(path.into()), EnvironmentProvider::Local, - &root + &root, + &interruption ) .await .is_err(), @@ -267,13 +278,16 @@ mod tests { std::fs::write(project.join(".fabro/project.toml"), "_version = 1\n").unwrap(); git2::Repository::init(&checkout).unwrap(); let selected = WorkflowSelection::Local("review".into()); + let interruption = Interruption::new(false); for (cwd, expected_root) in [ (checkout.as_path(), checkout.as_path()), (project.as_path(), project.as_path()), (root.path(), user.as_path()), ] { let ResolvedWorkflow::Local(package) = - workflow(&selected, cwd, Some(&user)).await.unwrap() + workflow(&selected, cwd, Some(&user), &interruption) + .await + .unwrap() else { panic!("local package"); }; @@ -283,7 +297,8 @@ mod tests { workflow( &WorkflowSelection::Local("missing.toml".into()), &checkout, - Some(&user) + Some(&user), + &interruption ) .await .is_err() From 7c97ba1b7082cac79ea737c65c43163fb4408b0f Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:47:25 -0600 Subject: [PATCH 11/15] Move the run-driven remote workflow test to cmd/run.rs remote_workflow_run_starts_once_create_leaves_submitted_and_failures_do_not_refetch lived in cmd/create.rs but drove fabro run in four of its five iterations and asserted the start call, which is fabro run's contract. Split it: cmd/create.rs keeps the single create invocation that must leave the run submitted without starting it, cmd/run.rs owns the run-driven success and failure iterations, and the workflow and remote repository fixtures move to the shared command test support module. Co-Authored-By: Claude Fable 5.1 --- lib/apps/fabro-cli/tests/it/cmd/create.rs | 176 +++++++-------------- lib/apps/fabro-cli/tests/it/cmd/run.rs | 101 +++++++++++- lib/apps/fabro-cli/tests/it/cmd/support.rs | 41 +++++ 3 files changed, 194 insertions(+), 124 deletions(-) diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index a232774e3..bffcb070d 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -12,10 +12,10 @@ use insta::assert_snapshot; use serde_json::json; use super::support::{ - created_run_id, environment_json, fixture, mock_environment, + created_run_id, environment_json, fixture, init_remote_fixture, mock_environment, mock_workflow_version_registrations, mock_workflow_version_registrations_recording, output_stderr, output_stdout, remote_run_summary_json, resolve_run, run_count_for_test_case, - run_git, run_state, + run_git, run_state, write_workflow, }; use crate::support::unique_run_id; @@ -60,24 +60,6 @@ fn mock_intent_create<'a>( }) } -fn write_workflow(root: &std::path::Path, directory: &str, graph_name: &str) -> std::path::PathBuf { - let directory = root.join(directory); - std::fs::create_dir_all(&directory).expect("workflow fixture directory should be created"); - std::fs::write( - directory.join("workflow.toml"), - "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", - ) - .expect("workflow fixture manifest should be written"); - std::fs::write( - directory.join("workflow.fabro"), - format!( - "digraph {graph_name} {{ start [shape=Mdiamond] exit [shape=Msquare] start -> exit }}" - ), - ) - .expect("workflow fixture graph should be written"); - directory.join("workflow.toml") -} - #[test] fn help() { let context = test_context!(); @@ -1639,25 +1621,6 @@ draft = false assert!(!pull_request.draft); } -fn init_remote_fixture(path: &std::path::Path, branch: &str) -> String { - let repo = git2::Repository::init_opts( - path, - git2::RepositoryInitOptions::new().initial_head(branch), - ) - .expect("fixture repository should initialize"); - let mut index = repo.index().expect("fixture index should open"); - index - .add_all(["."], git2::IndexAddOption::DEFAULT, None) - .expect("fixture files should stage"); - let tree_id = index.write_tree().expect("fixture tree should write"); - let tree = repo.find_tree(tree_id).expect("fixture tree should exist"); - let signature = git2::Signature::now("Fixture", "fixture@example.test") - .expect("fixture signature should be valid"); - repo.commit(Some("HEAD"), &signature, &signature, "fixture", &tree, &[]) - .expect("fixture commit should succeed") - .to_string() -} - #[test] fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_independent() { let context = test_context!(); @@ -1779,7 +1742,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde } #[test] -fn remote_workflow_run_starts_once_create_leaves_submitted_and_failures_do_not_refetch() { +fn create_leaves_remote_workflow_run_submitted_without_starting() { let context = test_context!(); let root = tempfile::tempdir().unwrap(); let source = root.path().join("source"); @@ -1794,88 +1757,57 @@ fn remote_workflow_run_starts_once_create_leaves_submitted_and_failures_do_not_r ), ) .unwrap(); - for failure in ["none", "run", "upload", "create", "start"] { - let server = MockServer::start(); - let environment = mock_environment(&server, "default", "docker"); - let version = if failure == "upload" { - server.mock(|when, then| { - when.method("POST").path("/api/v1/workflow-versions"); - then.status(422).body("fixture registration rejection"); - }) - } else { - mock_workflow_version_registrations(&server) - }; - let run_id = unique_run_id(); - let create = server.mock(|when, then| { - when.method("POST").path("/api/v1/runs"); - if failure == "create" { - then.status(422).body("fixture create rejection"); - } else { - then.status(201) - .header("content-type", "application/json") - .body(run_status_response(&run_id, "submitted").to_string()); - } - }); - let start = server.mock(|when, then| { - when.method("POST") - .path(format!("/api/v1/runs/{run_id}/start")); - if failure == "start" { - then.status(422).body("fixture start rejection"); - } else { - then.status(200) - .header("content-type", "application/json") - .body(run_status_response(&run_id, "submitted").to_string()); - } - }); - let trace = root.path().join(format!("trace-{failure}")); - let mut command = if failure == "none" { - context.create_cmd() - } else { - context.run_cmd() - }; - command - .current_dir(root.path()) - .env("GIT_CONFIG_GLOBAL", &config) - .env("GIT_CONFIG_NOSYSTEM", "1") - .env("GIT_CONFIG_COUNT", "0") - .env("GIT_TRACE", &trace) - .args([ - "review", - "--workflow-git", - "acme/workflows", - "--server", - &format!("{}/api/v1", server.base_url()), - "--dry-run", - "--detach", - "--json", - ]); - let output = command.output().unwrap(); - assert_eq!( - output.status.success(), - matches!(failure, "none" | "run"), - "{}", - output_stderr(&output) - ); - environment.assert(); - version.assert(); - create.assert_calls(usize::from(failure != "upload")); - start.assert_calls(usize::from(matches!(failure, "run" | "start"))); - if failure == "none" { - assert_eq!( - serde_json::from_slice::(&output.stdout).unwrap(), - json!({"run_id":run_id}) - ); - } - let trace = std::fs::read_to_string(&trace).unwrap(); - assert_eq!( - trace - .lines() - .filter(|line| line.contains("built-in: git fetch ")) - .count(), - 1, - "source fetched more than once" - ); - } + let server = MockServer::start(); + let environment = mock_environment(&server, "default", "docker"); + let version = mock_workflow_version_registrations(&server); + let run_id = unique_run_id(); + let create = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("content-type", "application/json") + .body(run_status_response(&run_id, "submitted").to_string()); + }); + let start = server.mock(|when, _then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + }); + let trace = root.path().join("trace"); + let output = context + .create_cmd() + .current_dir(root.path()) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .env("GIT_TRACE", &trace) + .args([ + "review", + "--workflow-git", + "acme/workflows", + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + "--json", + ]) + .output() + .unwrap(); + assert!(output.status.success(), "{}", output_stderr(&output)); + environment.assert(); + version.assert(); + create.assert(); + start.assert_calls(0); + assert_eq!( + serde_json::from_slice::(&output.stdout).unwrap(), + json!({"run_id":run_id}) + ); + let trace = std::fs::read_to_string(&trace).unwrap(); + assert_eq!( + trace + .lines() + .filter(|line| line.contains("built-in: git fetch ")) + .count(), + 1, + "source fetched more than once" + ); } #[test] diff --git a/lib/apps/fabro-cli/tests/it/cmd/run.rs b/lib/apps/fabro-cli/tests/it/cmd/run.rs index d755cb286..19e389bfb 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/run.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/run.rs @@ -10,8 +10,8 @@ use httpmock::MockServer; use serde_json::Value; use super::support::{ - created_run_id, mock_environment, mock_workflow_version_registrations, output_stderr, - remote_run_summary_json, run_state, wait_for_event_names, + created_run_id, init_remote_fixture, mock_environment, mock_workflow_version_registrations, + output_stderr, remote_run_summary_json, run_state, wait_for_event_names, write_workflow, }; use crate::support::{LightweightCli, run_output_filters, run_projection_json, unique_run_id}; @@ -1166,3 +1166,100 @@ fn detach_creates_run_dir_with_detach_log() { "# ); } + +#[test] +fn run_starts_remote_workflow_once_and_failures_do_not_refetch() { + let context = test_context!(); + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("source"); + write_workflow(&source, ".fabro/workflows/review", "Remote"); + init_remote_fixture(&source, "trunk"); + let config = root.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/workflows\n", + source.display() + ), + ) + .unwrap(); + for failure in ["none", "upload", "create", "start"] { + let server = MockServer::start(); + let environment = mock_environment(&server, "default", "docker"); + let version = if failure == "upload" { + server.mock(|when, then| { + when.method("POST").path("/api/v1/workflow-versions"); + then.status(422).body("fixture registration rejection"); + }) + } else { + mock_workflow_version_registrations(&server) + }; + let run_id = unique_run_id(); + let create = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + if failure == "create" { + then.status(422).body("fixture create rejection"); + } else { + then.status(201) + .header("content-type", "application/json") + .body(run_status_response(&run_id, "submitted").to_string()); + } + }); + let start = server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + if failure == "start" { + then.status(422).body("fixture start rejection"); + } else { + then.status(200) + .header("content-type", "application/json") + .body(run_status_response(&run_id, "submitted").to_string()); + } + }); + let trace = root.path().join(format!("trace-{failure}")); + let output = context + .run_cmd() + .current_dir(root.path()) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .env("GIT_TRACE", &trace) + .args([ + "review", + "--workflow-git", + "acme/workflows", + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + "--detach", + "--json", + ]) + .output() + .unwrap(); + assert_eq!( + output.status.success(), + failure == "none", + "{failure}: {}", + output_stderr(&output) + ); + environment.assert(); + version.assert(); + create.assert_calls(usize::from(failure != "upload")); + start.assert_calls(usize::from(matches!(failure, "none" | "start"))); + if failure == "none" { + assert_eq!( + serde_json::from_slice::(&output.stdout).unwrap(), + serde_json::json!({"run_id":run_id}) + ); + } + let trace = std::fs::read_to_string(&trace).unwrap(); + assert_eq!( + trace + .lines() + .filter(|line| line.contains("built-in: git fetch ")) + .count(), + 1, + "{failure}: source fetched more than once" + ); + } +} diff --git a/lib/apps/fabro-cli/tests/it/cmd/support.rs b/lib/apps/fabro-cli/tests/it/cmd/support.rs index 3d52f599b..0e1d223ef 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/support.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/support.rs @@ -209,6 +209,47 @@ pub(crate) fn mock_workflow_version_registrations_recording( /// Runs a `git` command in `path` for fixture setup, panicking on failure and /// returning trimmed stdout. +/// Write a minimal `workflow.toml` and `workflow.fabro` pair under +/// `root/directory`; `graph_name` distinguishes fixtures by content. +pub(crate) fn write_workflow(root: &Path, directory: &str, graph_name: &str) -> PathBuf { + let directory = root.join(directory); + std::fs::create_dir_all(&directory).expect("workflow fixture directory should be created"); + std::fs::write( + directory.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .expect("workflow fixture manifest should be written"); + std::fs::write( + directory.join("workflow.fabro"), + format!( + "digraph {graph_name} {{ start [shape=Mdiamond] exit [shape=Msquare] start -> exit }}" + ), + ) + .expect("workflow fixture graph should be written"); + directory.join("workflow.toml") +} + +/// Initialize `path` as a repository on `branch` with one commit of its +/// current contents, returning the commit SHA. +pub(crate) fn init_remote_fixture(path: &Path, branch: &str) -> String { + let repo = git2::Repository::init_opts( + path, + git2::RepositoryInitOptions::new().initial_head(branch), + ) + .expect("fixture repository should initialize"); + let mut index = repo.index().expect("fixture index should open"); + index + .add_all(["."], git2::IndexAddOption::DEFAULT, None) + .expect("fixture files should stage"); + let tree_id = index.write_tree().expect("fixture tree should write"); + let tree = repo.find_tree(tree_id).expect("fixture tree should exist"); + let signature = git2::Signature::now("Fixture", "fixture@example.test") + .expect("fixture signature should be valid"); + repo.commit(Some("HEAD"), &signature, &signature, "fixture", &tree, &[]) + .expect("fixture commit should succeed") + .to_string() +} + pub(crate) fn run_git(path: &Path, args: &[&str]) -> String { let output = std::process::Command::new("git") .args(args) From 321a190315d155d290fd67af2ce5ea0902bd6d71 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 09:51:30 -0600 Subject: [PATCH 12/15] Share one supervised process runner between server and CLI Git The CLI's native Git runner and the server's run_git_plan each hand-rolled the same mechanics: kill-on-drop, a wall-clock timeout, output capture, and (only in the CLI) process-group teardown, bounded capture, and cancellation. Add fabro_proc::SupervisedCommand, which owns stdio, the process group, the timeout, cooperative cancellation, and bounded capture, and put both runners on it. The server gains group teardown on timeout, so helpers a stuck clone or fetch spawned no longer outlive it; the CLI keeps discarding output on failure and gains nothing but less code. The hardened -c overrides become one named list in the CLI. Co-Authored-By: Claude Fable 5.1 --- .../src/commands/run/remote_workflow.rs | 134 ++++++++---------- 1 file changed, 56 insertions(+), 78 deletions(-) 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 015353490..cefe892be 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -6,10 +6,10 @@ use std::time::Duration; use anyhow::{Context as _, bail}; use fabro_manifest::CollectedWorkflowClosure; +use fabro_proc::ProcessError; use fabro_types::{GitHubRepositorySlug, GitRunTarget, repository}; -use tokio::io::{AsyncRead, AsyncReadExt as _}; use tokio::process::Command; -use tokio::{fs, signal as tokio_signal, task, time}; +use tokio::{fs, signal as tokio_signal, task}; use tokio_util::sync::CancellationToken; use tokio_util::task::TaskTracker; @@ -18,6 +18,33 @@ use crate::args::RunArgs; const OUTPUT_LIMIT: usize = 64 * 1024; +/// Configuration overrides that keep an untrusted checkout from running code +/// or rewriting bytes: no hooks or fsmonitor, no LFS smudge, no submodule +/// recursion, no `ext::` transport, no background maintenance, no line-ending +/// conversion. +const HARDENED_GIT_CONFIG: &[&str] = &[ + "-c", + "core.hooksPath=/dev/null", + "-c", + "core.fsmonitor=false", + "-c", + "filter.lfs.smudge=", + "-c", + "filter.lfs.process=", + "-c", + "filter.lfs.required=false", + "-c", + "submodule.recurse=false", + "-c", + "protocol.ext.allow=never", + "-c", + "maintenance.auto=0", + "-c", + "gc.auto=0", + "-c", + "core.autocrlf=false", +]; + #[derive(Debug, thiserror::Error)] pub(super) enum RemoteWorkflowError { #[error( @@ -69,29 +96,9 @@ impl NativeGit { } let mut command = Command::new("git"); command + .stdin(Stdio::null()) .current_dir(cwd) - .args([ - "-c", - "core.hooksPath=/dev/null", - "-c", - "core.fsmonitor=false", - "-c", - "filter.lfs.smudge=", - "-c", - "filter.lfs.process=", - "-c", - "filter.lfs.required=false", - "-c", - "submodule.recurse=false", - "-c", - "protocol.ext.allow=never", - "-c", - "maintenance.auto=0", - "-c", - "gc.auto=0", - "-c", - "core.autocrlf=false", - ]) + .args(HARDENED_GIT_CONFIG) .args(args) .env("GIT_TERMINAL_PROMPT", "0") .env("GIT_LFS_SKIP_SMUDGE", "1") @@ -99,48 +106,33 @@ impl NativeGit { .env_remove("GIT_WORK_TREE") .env_remove("GIT_INDEX_FILE") .env_remove("GIT_OBJECT_DIRECTORY") - .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .kill_on_drop(true); - #[cfg(unix)] - command.process_group(0); + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES"); #[cfg(test)] command.envs(self.environment.iter().cloned()); - let mut child = command.spawn()?; - let process_id = child.id(); - let mut stdout = child.stdout.take().expect("piped Git stdout exists"); - let mut stderr = child.stderr.take().expect("piped Git stderr exists"); - let result = tokio::select! { - biased; - () = cancel.cancelled() => Err(RemoteWorkflowError::Cancelled), - () = time::sleep(self.timeout) => Err(RemoteWorkflowError::Timeout), - result = async { - let (status, (stdout, overflow), _) = tokio::try_join!( - child.wait(), capture(&mut stdout), capture(&mut stderr) - )?; - 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, status }); - } - if overflow { return Err(RemoteWorkflowError::OutputLimit); } - Ok(stdout) - } => result, - }; - // A timed-out, cancelled, or failed Git may leave helpers running in - // its process group; terminate the group, then reap Git. A successful - // Git has closed its pipes, and helpers it deliberately left behind - // (such as `credential-cache--daemon`) keep serving later commands. - if result.is_err() { - #[cfg(unix)] - if let Some(id) = process_id { - fabro_proc::sigkill_process_group(id); - } - child.kill().await?; + let output = fabro_proc::capture( + &mut command, + Some(self.timeout), + cancel, + Some(OUTPUT_LIMIT), + ) + .await + .map_err(|error| match error { + ProcessError::TimedOut => RemoteWorkflowError::Timeout, + ProcessError::Cancelled => RemoteWorkflowError::Cancelled, + ProcessError::Io(source) => RemoteWorkflowError::Io(source), + })?; + if !output.output.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, + status: output.output.status, + }); } - result + if output.stdout_truncated { + return Err(RemoteWorkflowError::OutputLimit); + } + Ok(output.output.stdout) } /// Create and initialize an empty scratch repository. Temporary files are @@ -360,21 +352,6 @@ impl NativeGit { } } -async fn capture(reader: &mut (impl AsyncRead + Unpin)) -> std::io::Result<(Vec, bool)> { - let mut captured = Vec::new(); - let mut overflow = false; - let mut buffer = vec![0; 8192]; - loop { - let count = reader.read(&mut buffer).await?; - if count == 0 { - return Ok((captured, overflow)); - } - let keep = count.min(OUTPUT_LIMIT - captured.len()); - captured.extend_from_slice(&buffer[..keep]); - overflow |= keep != count; - } -} - fn exact_record(records: &str, reference: &str) -> anyhow::Result> { let mut found = None; for line in records.lines() { @@ -544,6 +521,7 @@ mod tests { use nix::sys::signal::{self, Signal}; use nix::sys::stat::Mode; use nix::unistd; + use tokio::time; use super::super::test_support::{commit_all, write_workflow}; use super::*; From a9f28828a671fbb6dedc8c2c0faca27fa21291a9 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 12 Sep 2026 10:49:36 -0600 Subject: [PATCH 13/15] Adapt workflow target selection to sandbox provider kinds --- docs/public/core-concepts/workflows.mdx | 2 +- lib/apps/fabro-cli/src/args.rs | 2 +- lib/apps/fabro-cli/src/commands/run/create.rs | 2 +- .../src/commands/run/remote_workflow.rs | 55 +++++++++++++++---- .../fabro-cli/src/commands/run/resolution.rs | 34 +++++++----- lib/apps/fabro-cli/tests/it/cmd/create.rs | 18 ++++-- 6 files changed, 77 insertions(+), 36 deletions(-) diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 4119ef9e4..334bb642f 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -183,7 +183,7 @@ finish before its files can be removed. Target selection depends on the environment: -| Selection | Local environment | Docker/Daytona environment | +| Selection | Local environment | Clone-based environment (Docker, Daytona, or plugin) | | --- | --- | --- | | Default cwd or `--target-path PATH` | Uses the live directory, including uncommitted files | Uses the enclosing Git repository and exact available commit; a non-Git directory selects an empty workspace | | `--target-git OWNER/REPO` | Rejected | Uses the selected repository and exact observed branch commit; cloning must be enabled | diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index a7a34873c..fab071a5a 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -6,9 +6,9 @@ use clap::{Args, Parser, Subcommand, ValueEnum}; use fabro_config::{CliLayer, CliLoggingLayer, CliOutputLayer, CliUpdatesLayer}; use fabro_server::serve::DEFAULT_TCP_PORT; use fabro_static::EnvVars; -use fabro_types::{GitHubRepositorySlug, PermissionLevel}; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::run::MergeStrategy; +use fabro_types::{GitHubRepositorySlug, PermissionLevel}; use fabro_util::printer::Printer; use lithos_llm::catalog::ProviderId; use lithos_llm::types::ReasoningEffort; diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index ed69e165c..750e175d6 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -98,7 +98,7 @@ pub(crate) async fn create_run( }; let (target, dirty_worktree) = resolution::target( &target_selection, - environment.settings.provider, + &environment.settings.provider, &canonical_cwd, interruption, ) 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 cefe892be..d1244b907 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -109,18 +109,14 @@ impl NativeGit { .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES"); #[cfg(test)] command.envs(self.environment.iter().cloned()); - let output = fabro_proc::capture( - &mut command, - Some(self.timeout), - cancel, - Some(OUTPUT_LIMIT), - ) - .await - .map_err(|error| match error { - ProcessError::TimedOut => RemoteWorkflowError::Timeout, - ProcessError::Cancelled => RemoteWorkflowError::Cancelled, - ProcessError::Io(source) => RemoteWorkflowError::Io(source), - })?; + let output = + fabro_proc::capture(&mut command, Some(self.timeout), cancel, Some(OUTPUT_LIMIT)) + .await + .map_err(|error| match error { + ProcessError::TimedOut => RemoteWorkflowError::Timeout, + ProcessError::Cancelled => RemoteWorkflowError::Cancelled, + ProcessError::Io(source) => RemoteWorkflowError::Io(source), + })?; if !output.output.status.success() { // Output may contain arbitrary helper/config secrets, even after pattern // redaction. Never retain it in an error/cause chain or tracing event. @@ -1099,6 +1095,41 @@ mod tests { )); } + #[tokio::test] + async fn remote_workflow_cancelled_command_never_spawns() { + let (root, git) = fake_git("printf started > spawned"); + let cancel = CancellationToken::new(); + cancel.cancel(); + let error = git + .command("fetch", root.path(), &["fetch"], &cancel) + .await + .unwrap_err(); + assert!(matches!(error, RemoteWorkflowError::Cancelled)); + assert!(!root.path().join("spawned").exists()); + } + + #[tokio::test] + async fn remote_workflow_spawn_failure_preserves_io_source() { + use std::error::Error as _; + + let (root, git) = fake_git("exit 0"); + let error = git + .command( + "fetch", + &root.path().join("missing"), + &["fetch"], + &CancellationToken::new(), + ) + .await + .unwrap_err(); + let source = error + .source() + .unwrap() + .downcast_ref::() + .unwrap(); + assert_eq!(source.kind(), std::io::ErrorKind::NotFound); + } + #[tokio::test] async fn remote_workflow_timeout_and_cancel_reap_owned_children() { for timeout in [true, false] { diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index 556e92d1d..049ddd7f2 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -2,8 +2,7 @@ use std::path::Path; use anyhow::{Context as _, anyhow, bail}; use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; -use fabro_types::settings::run::EnvironmentProvider; -use fabro_types::{DirtyStatus, RunTarget}; +use fabro_types::{DirtyStatus, RunTarget, SandboxProviderKind}; use tokio::task; use super::remote_workflow::{Interruption, NativeGit}; @@ -70,7 +69,7 @@ pub(super) async fn workflow( pub(super) async fn target( selection: &TargetSelection, - provider: EnvironmentProvider, + provider: &SandboxProviderKind, cwd: &Path, interruption: &Interruption, ) -> anyhow::Result<(RunTarget, bool)> { @@ -80,8 +79,8 @@ pub(super) async fn target( .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"); + if !provider.clones_workspace() { + bail!("Git targets require a clone-enabled environment"); } let git = NativeGit::new(); let (repository, branch) = (repository.clone(), branch.clone()); @@ -99,7 +98,8 @@ pub(super) async fn target( } // 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)) + let provider = provider.clone(); + task::spawn_blocking(move || run_target_for_environment(&provider, &path)) .await .context("target observation task failed")? } @@ -108,10 +108,10 @@ pub(super) async fn target( /// provider. Returns the target plus whether a clone-based observation found a /// dirty Git worktree, so the caller can warn about it. fn run_target_for_environment( - provider: EnvironmentProvider, + provider: &SandboxProviderKind, canonical_cwd: &Path, ) -> anyhow::Result<(RunTarget, bool)> { - if !provider.is_clone_based() { + if !provider.clones_workspace() { let path = canonical_cwd.to_str().ok_or_else(|| { anyhow!( "target directory is not valid UTF-8: {}", @@ -205,7 +205,7 @@ mod tests { let selected = TargetSelection::Path("target".into()); let interruption = Interruption::new(false); assert_eq!( - target(&selected, EnvironmentProvider::Local, &root, &interruption) + target(&selected, &SandboxProviderKind::LOCAL, &root, &interruption) .await .unwrap() .0, @@ -213,9 +213,13 @@ mod tests { path: root.join("target").to_str().unwrap().into(), } ); - for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { + for provider in [ + SandboxProviderKind::DOCKER, + SandboxProviderKind::DAYTONA, + SandboxProviderKind::try_new("host").unwrap(), + ] { assert_eq!( - target(&selected, provider, &root, &interruption) + target(&selected, &provider, &root, &interruption) .await .unwrap() .0, @@ -225,7 +229,7 @@ mod tests { assert_eq!( target( &TargetSelection::Path(".".into()), - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) @@ -242,20 +246,20 @@ mod tests { repository: "acme/app".parse().unwrap(), branch: None, }, - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) .await .unwrap_err() .to_string() - .contains("Docker or Daytona") + .contains("clone-enabled environment") ); for path in ["missing", ".fabro/workflows/review/workflow.toml"] { assert!( target( &TargetSelection::Path(path.into()), - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index bffcb070d..c0ca5f604 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -1627,6 +1627,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde let server = MockServer::start(); let local_env = mock_environment(&server, "local", "local"); let docker_env = mock_environment(&server, "docker", "docker"); + let plugin_env = mock_environment(&server, "plugin", "host"); 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)); @@ -1665,7 +1666,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde .root_id(); assert_ne!(local_id, remote_id); for source_kind in ["name", "file", "git"] { - for target_kind in ["inferred", "path", "git"] { + for target_kind in ["inferred", "path", "git", "git-plugin"] { let mut command = context.create_cmd(); command .current_dir(&caller) @@ -1695,14 +1696,18 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde "path" => { command.args(["--target-path", "../target", "--environment", "local"]); } - "git" => { + "git" | "git-plugin" => { command.args([ "--target-git", "acme/app", "--target-branch", "release", "--environment", - "docker", + if target_kind == "git-plugin" { + "plugin" + } else { + "docker" + }, ]); } _ => { @@ -1729,7 +1734,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde assert_eq!(intent["goal"], "Caller goal"); assert_eq!(intent["target"], match target_kind { "path" => json!({"kind":"folder","path":target.canonicalize().unwrap()}), - "git" => + "git" | "git-plugin" => json!({"kind":"git","repo":"acme/app","branch":"release","sha":target_sha}), _ => json!({"kind":"none"}), }); @@ -1737,8 +1742,9 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde } local_env.assert_calls(3); docker_env.assert_calls(6); - versions.assert_calls(9); - create.assert_calls(9); + plugin_env.assert_calls(3); + versions.assert_calls(12); + create.assert_calls(12); } #[test] From 94c7159ef8e4946273998b481a0bd209aa0acb87 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 12 Sep 2026 11:54:48 -0600 Subject: [PATCH 14/15] Preserve shared target inference with explicit CLI targets --- docs/public/core-concepts/workflows.mdx | 4 + lib/apps/fabro-cli/src/commands/run/create.rs | 11 ++ .../fabro-cli/src/commands/run/resolution.rs | 124 +++++------------- lib/apps/fabro-cli/tests/it/cmd/create.rs | 83 +++++++++++- 4 files changed, 126 insertions(+), 96 deletions(-) diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 334bb642f..61bb09b5a 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -181,6 +181,10 @@ Temporary files are removed after collection or failure. Interrupting acquisitio stops owned Git processes before cleanup; collection already in progress must finish before its files can be removed. +For local workflows without target flags, the existing `run.scm` repository +configuration still participates in target inference. Explicit `--target-path` +and `--target-git` selections take precedence over that inferred repository. + Target selection depends on the environment: | Selection | Local environment | Clone-based environment (Docker, Daytona, or plugin) | diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index 750e175d6..9fe8d061b 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -96,10 +96,21 @@ pub(crate) async fn create_run( Some(package) => package, None => resolve_workflow().await?, }; + // Preserve configured repository inference for the existing local workflow + // path. Explicit targets select their own repository independently. + let configured_repo_origin_url = match &package { + ResolvedWorkflow::Local(package) + if args.target_path.is_none() && args.target_git.is_none() => + { + fabro_manifest::configured_repo_origin_url_for_location(package.workflow_location())? + } + _ => None, + }; let (target, dirty_worktree) = resolution::target( &target_selection, &environment.settings.provider, &canonical_cwd, + configured_repo_origin_url.as_deref(), interruption, ) .await?; diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index 049ddd7f2..085030c2c 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -1,15 +1,16 @@ use std::path::Path; -use anyhow::{Context as _, anyhow, bail}; +use anyhow::{Context as _, bail}; use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; -use fabro_types::{DirtyStatus, RunTarget, SandboxProviderKind}; +use fabro_types::{RunTarget, SandboxProviderKind}; use tokio::task; use super::remote_workflow::{Interruption, NativeGit}; use super::selection::{TargetSelection, WorkflowSelection}; /// Owns the canonical collector result without copying its contents. Local -/// location metadata remains available solely for existing settings warnings. +/// location metadata remains available for settings warnings and target +/// inference. pub(super) enum ResolvedWorkflow { Local(ResolvedLocalWorkflowPackage), Git(CollectedWorkflowClosure), @@ -71,6 +72,7 @@ pub(super) async fn target( selection: &TargetSelection, provider: &SandboxProviderKind, cwd: &Path, + configured_repo_origin_url: Option<&str>, interruption: &Interruption, ) -> anyhow::Result<(RunTarget, bool)> { let path = match selection { @@ -99,92 +101,17 @@ pub(super) async fn target( // The existing observer can push/query Git synchronously. Preserve its // behavior without blocking a Tokio worker or promising a new timeout. let provider = provider.clone(); - task::spawn_blocking(move || run_target_for_environment(&provider, &path)) - .await - .context("target observation task failed")? -} - -/// Derives the run target from the selected directory for the environment's -/// provider. Returns the target plus whether a clone-based observation found a -/// dirty Git worktree, so the caller can warn about it. -fn run_target_for_environment( - provider: &SandboxProviderKind, - canonical_cwd: &Path, -) -> anyhow::Result<(RunTarget, bool)> { - if !provider.clones_workspace() { - let path = canonical_cwd.to_str().ok_or_else(|| { - anyhow!( - "target directory is not valid UTF-8: {}", - canonical_cwd.display() - ) - })?; - return Ok(( - RunTarget::Folder { - path: path.to_string(), - }, - false, - )); - } - let Some(observation) = fabro_manifest::observe_git_run_target(canonical_cwd, None) else { - return Ok((none_target_for_unversioned_directory(canonical_cwd)?, false)); - }; - let dirty = observation.legacy_git_context.dirty == DirtyStatus::Dirty; - let target = observation.run_target.ok_or_else(|| { - anyhow!("the target Git checkout cannot be represented as a canonical GitHub run target") - })?; - if target.sha.is_none() { - bail!( - "the exact local Git commit could not be made available from the canonical GitHub origin; push the commit and try again" - ); - } - Ok((RunTarget::Git(target), dirty)) -} - -fn none_target_for_unversioned_directory(canonical_cwd: &Path) -> anyhow::Result { - let repository = match git2::Repository::discover(canonical_cwd) { - Ok(repository) => repository, - Err(source) if source.code() == git2::ErrorCode::NotFound => return Ok(RunTarget::None {}), - Err(source) => { - return Err(anyhow::Error::new(source)).with_context(|| { - format!( - "failed to inspect target directory {} for Git metadata", - canonical_cwd.display() - ) - }); - } - }; - - if repository.is_bare() { - bail!( - "the target directory resolves to a bare Git repository; clone-based runs require a non-bare checkout with an attached branch" - ); - } - match repository.head() { - Err(source) - if matches!( - source.code(), - git2::ErrorCode::UnbornBranch | git2::ErrorCode::NotFound - ) => - { - bail!( - "the target Git checkout has no commits; create a commit before using a clone-based environment" - ); - } - Err(source) => { - return Err(anyhow::Error::new(source)) - .context("failed to inspect the target Git checkout HEAD"); - } - Ok(head) if !head.is_branch() => { - bail!( - "the target Git checkout has a detached HEAD; check out a branch before using a clone-based environment" - ); - } - Ok(_) => {} - } - - bail!( - "the target Git checkout does not have a usable attached branch for a clone-based run target" - ) + let configured_repo_origin_url = configured_repo_origin_url.map(str::to_owned); + let derived = task::spawn_blocking(move || { + fabro_manifest::derive_run_target_for_provider( + &provider, + &path, + configured_repo_origin_url.as_deref(), + ) + }) + .await + .context("target observation task failed")??; + Ok((derived.target, derived.dirty_worktree)) } #[cfg(test)] @@ -205,10 +132,16 @@ mod tests { let selected = TargetSelection::Path("target".into()); let interruption = Interruption::new(false); assert_eq!( - target(&selected, &SandboxProviderKind::LOCAL, &root, &interruption) - .await - .unwrap() - .0, + target( + &selected, + &SandboxProviderKind::LOCAL, + &root, + None, + &interruption + ) + .await + .unwrap() + .0, RunTarget::Folder { path: root.join("target").to_str().unwrap().into(), } @@ -219,7 +152,7 @@ mod tests { SandboxProviderKind::try_new("host").unwrap(), ] { assert_eq!( - target(&selected, &provider, &root, &interruption) + target(&selected, &provider, &root, None, &interruption) .await .unwrap() .0, @@ -231,6 +164,7 @@ mod tests { &TargetSelection::Path(".".into()), &SandboxProviderKind::LOCAL, &root, + None, &interruption ) .await @@ -248,6 +182,7 @@ mod tests { }, &SandboxProviderKind::LOCAL, &root, + None, &interruption ) .await @@ -261,6 +196,7 @@ mod tests { &TargetSelection::Path(path.into()), &SandboxProviderKind::LOCAL, &root, + None, &interruption ) .await diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index c0ca5f604..ab3e62578 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -675,6 +675,85 @@ fn create_preserves_named_user_other_checkout_and_loose_file_selection() { } } +#[test] +fn create_preserves_configured_repository_inference_but_explicit_target_path_wins() { + let context = test_context!(); + let server = MockServer::start(); + let environment = mock_environment(&server, "default", "docker"); + 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 root = tempfile::tempdir().unwrap(); + let checkout = root.path().join("checkout"); + let workflow = write_workflow(&checkout, "workflow", "ConfiguredRepository"); + std::fs::write(&workflow, "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.scm]\nowner = \"acme\"\nrepository = \"configured\"\n").unwrap(); + let sha = init_remote_fixture(&checkout, "topic"); + let origin = root.path().join("origin.git"); + let bare = git2::Repository::init_bare(&origin).unwrap(); + run_git(&checkout, &[ + "remote", + "add", + "origin", + "https://github.com/acme/actual.git", + ]); + run_git(&checkout, &[ + "remote", + "set-url", + "--push", + "origin", + &format!("file://{}", origin.display()), + ]); + let server_url = format!("{}/api/v1", server.base_url()); + let inferred = context + .create_cmd() + .current_dir(&checkout) + .args(["--server", &server_url, workflow.to_str().unwrap()]) + .output() + .unwrap(); + assert!(!inferred.status.success()); + assert!( + output_stderr(&inferred) + .contains("run.scm repository that is not the local checkout's origin") + ); + assert!( + bare.find_reference("refs/heads/topic").is_err(), + "rejected inference must not publish the branch" + ); + versions.assert_calls(0); + create.assert_calls(0); + + let explicit = context + .create_cmd() + .current_dir(&checkout) + .args([ + "--server", + &server_url, + workflow.to_str().unwrap(), + "--target-path", + ".", + ]) + .output() + .unwrap(); + assert!(explicit.status.success(), "{}", output_stderr(&explicit)); + assert_eq!( + requests.lock().unwrap()[0]["target"], + json!({ + "kind": "git", "repo": "acme/actual", "branch": "topic", "sha": sha + }) + ); + assert_eq!( + bare.find_reference("refs/heads/topic") + .unwrap() + .target() + .unwrap() + .to_string(), + sha + ); + environment.assert_calls(2); + versions.assert_calls(1); + create.assert_calls(1); +} + #[test] fn create_clone_targets_require_exact_git_observations() { let context = test_context!(); @@ -896,9 +975,9 @@ fn create_rejects_unusable_git_checkouts_instead_of_sending_an_empty_target() { for (working_directory, expected_error) in [ ( detached.path(), - "the target Git checkout has a detached HEAD", + "the caller Git checkout has a detached HEAD", ), - (unborn.path(), "the target Git checkout has no commits"), + (unborn.path(), "the caller Git checkout has no commits"), ] { let output = context .create_cmd() From 86bcc8f128d7766cc77721d731f9ad7fb98bd254 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 12 Sep 2026 14:18:34 -0600 Subject: [PATCH 15/15] Simplify CLI repository selectors and add workflow shorthand --- docs/public/core-concepts/workflows.mdx | 46 ++-- docs/public/reference/cli.mdx | 18 +- lib/apps/fabro-cli/src/args.rs | 28 ++- lib/apps/fabro-cli/src/commands/run/create.rs | 4 +- .../fabro-cli/src/commands/run/overrides.rs | 41 ++-- .../src/commands/run/remote_workflow.rs | 23 +- .../fabro-cli/src/commands/run/selection.rs | 212 +++++++++++++++--- lib/apps/fabro-cli/tests/it/cmd/create.rs | 121 ++++++---- lib/apps/fabro-cli/tests/it/cmd/run.rs | 55 +++-- 9 files changed, 391 insertions(+), 157 deletions(-) diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 61bb09b5a..af2866ed2 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -147,15 +147,17 @@ See the [Quick Start](/getting-started/quick-start) to try it out, or browse the registers the workflow and leaves a submitted run for you to start with `fabro start RUN`. `run` also starts it, then attaches unless you pass `--detach`. -The required positional argument selects the workflow. With no source flags, -names are found in the current checkout, then a marked project, then installed -user workflows. Explicit local paths retain their usual package roots. +The required positional argument selects the workflow. Local names are found in +the current checkout, then a marked project, then installed user workflows. +Explicit local paths retain their usual package roots. ```sh fabro run review -fabro create ./review.toml --target-path ../app -fabro run review --workflow-git acme/workflows --workflow-ref v1.2 \ - --target-git acme/app --target-branch release +fabro create ./review.toml --target-from ../app +fabro run acme/workflows@v1.2:review --target acme/app@release +# Equivalent explicit flags: +fabro run review --workflow-repo acme/workflows --workflow-ref v1.2 \ + --target-repo acme/app --target-branch release ``` In the last example, workflow instructions come from `acme/workflows`, and the @@ -164,13 +166,21 @@ run works on `acme/app`. Neither selection changes the other. Local workflow pat Without target flags, Fabro keeps its existing cwd/environment-based target inference. -`--workflow-git OWNER/REPO` acquires source using native Git on your machine. A +Remote workflow shorthand is `OWNER/REPO[@REF]:WORKFLOW`. The workflow selector +is required: `acme/workflows:review` selects a named workflow, and +`acme/workflows@v1.2:./reviews/security.toml` selects a file. Repository default +workflows are not supported; without `:WORKFLOW`, the positional argument retains +local lookup behavior. Prefix local paths containing a colon with `./`, `../`, +or `/` to avoid shorthand parsing. Shorthand cannot be combined with +`--workflow-repo` or `--workflow-ref`. Repository slugs currently imply GitHub.com. + +`--workflow-repo OWNER/REPO` acquires source using native Git on your machine. A workflow name selects `.fabro/workflows/NAME/workflow.toml` in that repository; you can also supply an explicit repository-relative `.toml` or `.fabro` file. Absolute paths, traversal, and directory selectors are rejected. A missing remote workflow never falls back to a local or installed workflow. -`--workflow-ref` requires `--workflow-git`. Omit it, or use `HEAD`, to select the +`--workflow-ref` requires `--workflow-repo`. Omit it, or use `HEAD`, to select the remote default branch. You can select a branch, tag, or full 40-hex commit SHA. If a branch and tag share a name, qualify it with `refs/heads/` or `refs/tags/`. Fabro resolves the revision once, fetches that exact commit into a temporary @@ -182,15 +192,19 @@ stops owned Git processes before cleanup; collection already in progress must finish before its files can be removed. For local workflows without target flags, the existing `run.scm` repository -configuration still participates in target inference. Explicit `--target-path` -and `--target-git` selections take precedence over that inferred repository. +configuration in `workflow.toml` or `.fabro/project.toml` still participates in +target inference, with workflow values overriding project values field by field. +For clone-based environments, the configured repository must match the checkout's +origin. Without that configuration, omission is equivalent to `--target-from .`. +Explicit `--target-from`, `--target-repo`, and `--target` selections take precedence +over the configured repository. Target selection depends on the environment: | Selection | Local environment | Clone-based environment (Docker, Daytona, or plugin) | | --- | --- | --- | -| Default cwd or `--target-path PATH` | Uses the live directory, including uncommitted files | Uses the enclosing Git repository and exact available commit; a non-Git directory selects an empty workspace | -| `--target-git OWNER/REPO` | Rejected | Uses the selected repository and exact observed branch commit; cloning must be enabled | +| Default cwd or `--target-from PATH` | Uses the live directory, including uncommitted files | Uses the enclosing Git repository and exact available commit; a non-Git directory selects an empty workspace | +| `--target-repo OWNER/REPO` or `--target OWNER/REPO[@BRANCH]` | Rejected | Uses the selected repository and exact observed branch commit; cloning must be enabled | For clone-based execution, a target path selects a repository, not a subdirectory working-directory override. Local target files are not uploaded. Existing target @@ -200,10 +214,14 @@ commits fail. Folder targets require the directory to be accessible to the server and its Local execution environment; passing a caller-local path does not transfer it to a remote server. -`--target-branch` requires `--target-git` and accepts a working branch name, not a +`--target-branch` requires `--target-repo` and accepts a working branch name, not a tag or SHA. Without it, Fabro resolves the repository's default branch. The CLI only looks up target metadata; the execution sandbox clones the target. -`--target-path` and `--target-git` conflict. +The shorthand `--target acme/app@release/v2` selects the working branch +`release/v2`; omit `@BRANCH` to use the remote default branch. Target suffixes +accept working branches, while workflow suffixes accept branches, tags, or SHAs. +`--target`, `--target-from`, and `--target-repo` are mutually exclusive. +`--target-branch` cannot be combined with `--target`. Local Git credential helpers, SSH-agent access through configured URL rewrites, and user network configuration govern source acquisition and remote target diff --git a/docs/public/reference/cli.mdx b/docs/public/reference/cli.mdx index 8a4822416..659b31bf2 100644 --- a/docs/public/reference/cli.mdx +++ b/docs/public/reference/cli.mdx @@ -342,7 +342,7 @@ fabro create [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Workflow name or path (repository-relative with --workflow-git) | +| `WORKFLOW` | Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW | #### Options @@ -361,12 +361,13 @@ fabro create [OPTIONS] | `--provider ` | Override default LLM provider | | `--server ` | Fabro server target: http(s) URL or absolute Unix socket path | | `--target-branch ` | Target working branch (default: remote default branch), pinned to its observed commit | -| `--target-git ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | -| `--target-path ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | +| `--target-from ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | +| `--target-repo ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | +| `--target ` | Target GitHub repository and optional working branch | | `-I, --input ` | Override a workflow input value (repeatable, format: KEY=VALUE) | | `-v, --verbose` | Enable verbose output | -| `--workflow-git ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | | `--workflow-ref ` | Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names | +| `--workflow-repo ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | ### `fabro deny` @@ -1076,7 +1077,7 @@ fabro run [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Workflow name or path (repository-relative with --workflow-git) | +| `WORKFLOW` | Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW | #### Options @@ -1095,12 +1096,13 @@ fabro run [OPTIONS] | `--provider ` | Override default LLM provider | | `--server ` | Fabro server target: http(s) URL or absolute Unix socket path | | `--target-branch ` | Target working branch (default: remote default branch), pinned to its observed commit | -| `--target-git ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | -| `--target-path ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | +| `--target-from ` | Observe this target directory instead of cwd; Folder targets require server filesystem access | +| `--target-repo ` | Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials | +| `--target ` | Target GitHub repository and optional working branch | | `-I, --input ` | Override a workflow input value (repeatable, format: KEY=VALUE) | | `-v, --verbose` | Enable verbose output | -| `--workflow-git ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | | `--workflow-ref ` | Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names | +| `--workflow-repo ` | Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials | ### `fabro sandbox` diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index fab071a5a..ac90101d9 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -233,33 +233,37 @@ pub(crate) struct RunArgs { #[command(flatten)] pub(crate) inputs: InputOverrideArgs, - /// Workflow name or path (repository-relative with --workflow-git) + /// Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW #[arg(required = true)] pub(crate) workflow: Option, /// Acquire workflow source locally from a GitHub OWNER/REPO using native /// Git credentials #[arg(long, value_name = "OWNER/REPO")] - pub(crate) workflow_git: Option, + pub(crate) workflow_repo: Option, /// Workflow branch, tag, HEAD (default), or full commit SHA; qualify /// ambiguous names - #[arg(long, requires = "workflow_git", value_name = "REF")] + #[arg(long, requires = "workflow_repo", value_name = "REF")] pub(crate) workflow_ref: Option, /// Observe this target directory instead of cwd; Folder targets require /// server filesystem access - #[arg(long, conflicts_with = "target_git", value_name = "PATH")] - pub(crate) target_path: Option, + #[arg(long, conflicts_with_all = ["target_repo", "target_repo_selector"], value_name = "PATH")] + pub(crate) target_from: Option, + + /// Target GitHub repository and optional working branch + #[arg(long = "target", conflicts_with_all = ["target_repo", "target_branch"], value_name = "OWNER/REPO[@BRANCH]")] + pub(crate) target_repo_selector: Option, /// Target GitHub OWNER/REPO; the execution sandbox still needs its own /// clone credentials #[arg(long, value_name = "OWNER/REPO")] - pub(crate) target_git: Option, + pub(crate) target_repo: Option, /// Target working branch (default: remote default branch), pinned to its /// observed commit - #[arg(long, requires = "target_git", value_name = "BRANCH")] + #[arg(long, requires = "target_repo", value_name = "BRANCH")] pub(crate) target_branch: Option, /// Simulate execution; workflow source may still be fetched and uploaded @@ -2011,16 +2015,16 @@ mod run_selection_grammar_tests { for flags in [ vec![ "review", - "--workflow-git", + "--workflow-repo", "acme/workflows", "--workflow-ref", "refs/tags/v1", - "--target-git", + "--target-repo", "acme/app", "--target-branch", "release/topic", ], - vec!["./review.toml", "--target-path", "../app"], + vec!["./review.toml", "--target-from", "../app"], ] { assert!(parse_run_args(flags).is_ok()); } @@ -2031,8 +2035,8 @@ mod run_selection_grammar_tests { for flags in [ vec!["review", "--workflow-ref", "v1"], vec!["review", "--target-branch", "release"], - vec!["review", "--target-path", ".", "--target-git", "acme/app"], - vec!["--workflow-git", "acme/workflows"], + vec!["review", "--target-from", ".", "--target-repo", "acme/app"], + vec!["--workflow-repo", "acme/workflows"], ] { 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 9fe8d061b..6e07c0a65 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -100,7 +100,9 @@ pub(crate) async fn create_run( // path. Explicit targets select their own repository independently. let configured_repo_origin_url = match &package { ResolvedWorkflow::Local(package) - if args.target_path.is_none() && args.target_git.is_none() => + if args.target_from.is_none() + && args.target_repo.is_none() + && args.target_repo_selector.is_none() => { fabro_manifest::configured_repo_origin_url_for_location(package.workflow_location())? } diff --git a/lib/apps/fabro-cli/src/commands/run/overrides.rs b/lib/apps/fabro-cli/src/commands/run/overrides.rs index a925b9b01..9f1f49d70 100644 --- a/lib/apps/fabro-cli/src/commands/run/overrides.rs +++ b/lib/apps/fabro-cli/src/commands/run/overrides.rs @@ -165,26 +165,27 @@ mod tests { fn run_args() -> RunArgs { RunArgs { - target: ServerTargetArgs::default(), - inputs: InputOverrideArgs::default(), - workflow: Some(PathBuf::from("workflow.fabro")), - workflow_git: None, - workflow_ref: None, - target_path: None, - target_git: None, - target_branch: None, - dry_run: false, - auto_approve: false, - goal: None, - goal_file: None, - model: None, - provider: None, - verbose: false, - environment: None, - label: Vec::new(), - parent: None, - preserve_sandbox: false, - detach: false, + target: ServerTargetArgs::default(), + inputs: InputOverrideArgs::default(), + workflow: Some(PathBuf::from("workflow.fabro")), + workflow_repo: None, + workflow_ref: None, + target_from: None, + target_repo_selector: None, + target_repo: None, + target_branch: None, + dry_run: false, + auto_approve: false, + goal: None, + goal_file: None, + model: None, + provider: None, + verbose: false, + environment: None, + label: Vec::new(), + parent: None, + preserve_sandbox: false, + detach: false, } } 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 d1244b907..8bec413d7 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -461,7 +461,15 @@ impl Interruption { } pub(crate) fn for_run_args(args: &RunArgs) -> Self { - Self::new(args.workflow_git.is_some() || args.target_git.is_some()) + Self::new( + args.workflow_repo.is_some() + || args.target_repo.is_some() + || args.target_repo_selector.is_some() + || args + .workflow + .as_deref() + .is_some_and(|path| super::selection::workflow_shorthand(path).is_some()), + ) } /// Run `work` to completion, or until Ctrl-C cancels it and every owned @@ -522,6 +530,19 @@ mod tests { use super::super::test_support::{commit_all, write_workflow}; use super::*; + #[test] + fn shorthand_acquisition_owns_interruption_but_local_paths_do_not() { + for (flags, listens) in [ + (vec!["acme/workflows:review"], true), + (vec!["review", "--target", "acme/app@main"], true), + (vec!["./acme/workflows:review"], false), + (vec!["review", "--target-from", "."], false), + ] { + let args = super::super::test_support::parse_run_args(flags).unwrap(); + assert_eq!(Interruption::for_run_args(&args).listens, listens); + } + } + struct Fixture { root: tempfile::TempDir, repo: git2::Repository, diff --git a/lib/apps/fabro-cli/src/commands/run/selection.rs b/lib/apps/fabro-cli/src/commands/run/selection.rs index e560a6b4f..d413487e7 100644 --- a/lib/apps/fabro-cli/src/commands/run/selection.rs +++ b/lib/apps/fabro-cli/src/commands/run/selection.rs @@ -88,12 +88,50 @@ pub(super) fn validate_remote_selector(path: &Path) -> anyhow::Result<()> { } } +/// Explicit local paths escape the shorthand grammar, including colons in +/// file names. A repository without `:WORKFLOW` retains local lookup behavior. +pub(super) fn workflow_shorthand(path: &Path) -> Option<(&str, &str)> { + let value = path.to_str()?; + if path.is_absolute() || value.starts_with("./") || value.starts_with("../") { + return None; + } + value.split_once(':') +} + +fn repository_revision(value: &str) -> anyhow::Result<(GitHubRepositorySlug, Option<&str>)> { + let (repository, revision) = value + .split_once('@') + .map_or((value, None), |(repository, revision)| { + (repository, Some(revision)) + }); + let repository = repository + .parse() + .context("repository must be a GitHub OWNER/REPO")?; + if revision == Some("") { + bail!("a revision or branch is required after '@'"); + } + Ok((repository, revision)) +} + 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")?; - let workflow = match &args.workflow_git { - None => WorkflowSelection::Local(workflow.clone()), - Some(repository) => { + let workflow = match (&args.workflow_repo, workflow_shorthand(workflow)) { + (_, Some(_)) if args.workflow_repo.is_some() || args.workflow_ref.is_some() => { + bail!("workflow shorthand cannot be combined with --workflow-repo or --workflow-ref"); + } + (None, Some((source, selector))) => { + let (repository, revision) = repository_revision(source)?; + let selector = PathBuf::from(selector); + validate_remote_selector(&selector)?; + WorkflowSelection::Git { + repository, + selector, + revision: RemoteWorkflowRevision::parse(revision)?, + } + } + (None, None) => WorkflowSelection::Local(workflow.clone()), + (Some(repository), _) => { validate_remote_selector(workflow)?; WorkflowSelection::Git { repository: repository.clone(), @@ -102,28 +140,32 @@ pub(super) fn parse(args: &RunArgs) -> anyhow::Result<(WorkflowSelection, Target } } }; - let target = match (&args.target_path, &args.target_git) { - (Some(path), _) => TargetSelection::Path(path.clone()), - (_, Some(repository)) => { - if args - .target_branch - .as_deref() - .is_some_and(|branch| !repository::is_valid_git_branch_name(branch)) - { - bail!( - "target branch must be a working branch name, not a tag, SHA, or qualified ref" - ); - } - TargetSelection::Git { - repository: repository.clone(), - branch: args.target_branch.clone(), - } + let target = if let Some(value) = &args.target_repo_selector { + let (repository, branch) = repository_revision(value)?; + git_target(repository, branch)? + } else { + match (&args.target_from, &args.target_repo) { + (Some(path), _) => TargetSelection::Path(path.clone()), + (_, Some(repository)) => git_target(repository.clone(), args.target_branch.as_deref())?, + _ => TargetSelection::Path(PathBuf::from(".")), } - _ => TargetSelection::Path(PathBuf::from(".")), }; Ok((workflow, target)) } +fn git_target( + repository: GitHubRepositorySlug, + branch: Option<&str>, +) -> anyhow::Result { + if branch.is_some_and(|branch| !repository::is_valid_git_branch_name(branch)) { + bail!("target branch must be a working branch name, not a tag, SHA, or qualified ref"); + } + Ok(TargetSelection::Git { + repository, + branch: branch.map(str::to_owned), + }) +} + #[cfg(test)] mod tests { use super::*; @@ -202,6 +244,118 @@ mod adapter_tests { use super::*; use crate::args::{Cli, Commands, RunCommands}; + #[test] + fn shorthand_matches_explicit_selections_for_both_commands() { + for command in ["run", "create"] { + for (suffix, reference) in [ + ("", None), + ("@v1.2", Some("v1.2")), + ("@release/v2", Some("release/v2")), + ("@refs/tags/v1", Some("refs/tags/v1")), + ( + "@abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd", + Some("abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd"), + ), + ] { + for selector in ["review", "./reviews/security.toml"] { + for branch in [None, Some("release/v2")] { + let workflow = format!("acme/workflows{suffix}:{selector}"); + let target = branch.map_or_else( + || "acme/app".to_owned(), + |branch| format!("acme/app@{branch}"), + ); + let short = ["fabro", command, &workflow, "--target", &target]; + let mut explicit = vec![ + "fabro", + command, + selector, + "--workflow-repo", + "acme/workflows", + "--target-repo", + "acme/app", + ]; + if let Some(reference) = reference { + explicit.extend(["--workflow-ref", reference]); + } + if let Some(branch) = branch { + explicit.extend(["--target-branch", branch]); + } + let selections = |argv: &[&str]| { + let cli = Cli::try_parse_from(argv).unwrap(); + let Commands::RunCmd( + RunCommands::Run(args) | RunCommands::Create(args), + ) = *cli.command.unwrap() + else { + panic!("expected run args") + }; + parse(&args).unwrap() + }; + assert_eq!(selections(&short), selections(&explicit)); + } + } + } + } + } + + #[test] + fn shorthand_preserves_local_paths_and_requires_remote_workflow_selector() { + for value in [ + "review", + "dir/review.toml", + "acme/workflows", + "acme/workflows@v1", + "./acme/workflows:review", + "../acme/workflows:review", + "/tmp/workflows:review", + ] { + let args = parse_run_args([value]).unwrap(); + assert_eq!( + parse(&args).unwrap(), + ( + WorkflowSelection::Local(value.into()), + TargetSelection::Path(".".into()) + ) + ); + } + } + + #[test] + fn shorthand_rejects_malformed_or_conflicting_selections_before_acquisition() { + for flags in [ + vec!["acme/workflows:"], + vec!["acme/workflows@:review"], + vec!["acme/workflows@HEAD~1:review"], + vec!["acme/workflows:../review.toml"], + vec!["acme/workflows:/review.toml"], + vec!["acme/workflows/extra:review"], + vec!["https://github.com/acme/workflows:review"], + vec!["acme/workflows:review", "--workflow-repo", "acme/other"], + vec!["acme/workflows:review", "--workflow-ref", "v1"], + vec!["review", "--target", "acme/app@"], + vec!["review", "--target", "acme/app@refs/tags/v1"], + vec![ + "review", + "--target", + "acme/app@abcdabcdabcdabcdabcdabcdabcdabcdabcdabcd", + ], + vec!["review", "--target", "acme/app@main..next"], + vec!["review", "--target", "acme/app/extra"], + vec!["review", "--target", "acme/app", "--target-from", "."], + vec![ + "review", + "--target", + "acme/app", + "--target-repo", + "acme/app", + ], + vec!["review", "--target", "acme/app", "--target-branch", "main"], + ] { + if let Ok(args) = parse_run_args(flags.iter().copied()) { + assert!(parse(&args).is_err(), "{flags:?}"); + } + } + } + #[test] fn run_selection_both_commands_share_the_adapter() { for command in ["run", "create"] { @@ -209,11 +363,11 @@ mod adapter_tests { "fabro", command, "review", - "--workflow-git", + "--workflow-repo", "acme/workflows", "--workflow-ref", "v1", - "--target-git", + "--target-repo", "acme/app", "--target-branch", "release", @@ -251,33 +405,33 @@ mod adapter_tests { for flags in [ [ "review", - "--workflow-git", + "--workflow-repo", "https://github.com/acme/workflows", ], - ["review", "--target-git", "acme/app/extra"], + ["review", "--target-repo", "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!["../review.toml", "--workflow-repo", "acme/workflows"], + vec!["/tmp/review.toml", "--workflow-repo", "acme/workflows"], vec![ "review", - "--workflow-git", + "--workflow-repo", "acme/workflows", "--workflow-ref", "HEAD~1", ], vec![ "review", - "--target-git", + "--target-repo", "acme/app", "--target-branch", "refs/tags/v1", ], vec![ "review", - "--target-git", + "--target-repo", "acme/app", "--target-branch", "1234567890123456789012345678901234567890", diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index ab3e62578..4b73e98fe 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -74,33 +74,34 @@ fn help() { Usage: fabro create [OPTIONS] Arguments: - Workflow name or path (repository-relative with --workflow-git) + Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --workflow-git Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names - --target-path Observe this target directory instead of cwd; Folder targets require server filesystem access - --target-git Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials - --target-branch Target working branch (default: remote default branch), pinned to its observed commit - --dry-run Simulate execution; workflow source may still be fetched and uploaded - --auto-approve Auto-approve all human gates - --goal Override the workflow goal (available as {{ goal }} in prompts) - --goal-file Read a per-run goal value from a local file - --model Override default LLM model - --provider Override default LLM provider - -v, --verbose Enable verbose output - --environment Named environment for agent tools - --label Attach a label to this run (repeatable, format: KEY=VALUE) - --parent Link this run to an existing orchestration parent run - --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) - -d, --detach Run the workflow in the background and print the run ID - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --workflow-repo Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names + --target-from Observe this target directory instead of cwd; Folder targets require server filesystem access + --target Target GitHub repository and optional working branch + --target-repo Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials + --target-branch Target working branch (default: remote default branch), pinned to its observed commit + --dry-run Simulate execution; workflow source may still be fetched and uploaded + --auto-approve Auto-approve all human gates + --goal Override the workflow goal (available as {{ goal }} in prompts) + --goal-file Read a per-run goal value from a local file + --model Override default LLM model + --provider Override default LLM provider + -v, --verbose Enable verbose output + --environment Named environment for agent tools + --label Attach a label to this run (repeatable, format: KEY=VALUE) + --parent Link this run to an existing orchestration parent run + --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) + -d, --detach Run the workflow in the background and print the run ID + -h, --help Print help ----- stderr ----- "); } @@ -729,7 +730,7 @@ fn create_preserves_configured_repository_inference_but_explicit_target_path_win "--server", &server_url, workflow.to_str().unwrap(), - "--target-path", + "--target-from", ".", ]) .output() @@ -749,9 +750,36 @@ fn create_preserves_configured_repository_inference_but_explicit_target_path_win .to_string(), sha ); - environment.assert_calls(2); - versions.assert_calls(1); - create.assert_calls(1); + let config = root.path().join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/acme/actual\n", + origin.display() + ), + ) + .unwrap(); + let shorthand = context + .create_cmd() + .current_dir(&checkout) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .args([ + "--server", + &server_url, + workflow.to_str().unwrap(), + "--target", + "acme/actual@topic", + ]) + .output() + .unwrap(); + assert!(shorthand.status.success(), "{}", output_stderr(&shorthand)); + let requests = requests.lock().unwrap(); + assert_eq!(requests[0]["target"], requests[1]["target"]); + environment.assert_calls(3); + versions.assert_calls(2); + create.assert_calls(2); } #[test] @@ -1744,8 +1772,8 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde .closure() .root_id(); assert_ne!(local_id, remote_id); - for source_kind in ["name", "file", "git"] { - for target_kind in ["inferred", "path", "git", "git-plugin"] { + for source_kind in ["name", "file", "git", "shorthand"] { + for target_kind in ["inferred", "path", "git", "git-plugin", "shorthand"] { let mut command = context.create_cmd(); command .current_dir(&caller) @@ -1758,26 +1786,31 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde "--goal-file", "goal.txt", ]); - command.arg(if source_kind == "file" { + command.arg(if source_kind == "shorthand" { + "acme/workflows@trunk:review" + } else if source_kind == "file" { ".fabro/workflows/review/workflow.toml" } else { "review" }); if source_kind == "git" { command.args([ - "--workflow-git", + "--workflow-repo", "acme/workflows", "--workflow-ref", "trunk", ]); } match target_kind { + "shorthand" => { + command.args(["--target", "acme/app@release", "--environment", "docker"]); + } "path" => { - command.args(["--target-path", "../target", "--environment", "local"]); + command.args(["--target-from", "../target", "--environment", "local"]); } "git" | "git-plugin" => { command.args([ - "--target-git", + "--target-repo", "acme/app", "--target-branch", "release", @@ -1804,7 +1837,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde assert_eq!( intent["workflow_version_id"], match source_kind { - "git" => remote_id, + "git" | "shorthand" => remote_id, "file" => file_id, _ => local_id, } @@ -1813,17 +1846,17 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde assert_eq!(intent["goal"], "Caller goal"); assert_eq!(intent["target"], match target_kind { "path" => json!({"kind":"folder","path":target.canonicalize().unwrap()}), - "git" | "git-plugin" => + "git" | "git-plugin" | "shorthand" => json!({"kind":"git","repo":"acme/app","branch":"release","sha":target_sha}), _ => json!({"kind":"none"}), }); } } - local_env.assert_calls(3); - docker_env.assert_calls(6); - plugin_env.assert_calls(3); - versions.assert_calls(12); - create.assert_calls(12); + local_env.assert_calls(4); + docker_env.assert_calls(12); + plugin_env.assert_calls(4); + versions.assert_calls(20); + create.assert_calls(20); } #[test] @@ -1866,7 +1899,7 @@ fn create_leaves_remote_workflow_run_submitted_without_starting() { .env("GIT_TRACE", &trace) .args([ "review", - "--workflow-git", + "--workflow-repo", "acme/workflows", "--server", &format!("{}/api/v1", server.base_url()), @@ -1945,7 +1978,7 @@ fn remote_workflow_explicit_acquisition_failure_has_no_fallback_or_server_mutati .env("GIT_CONFIG_COUNT", "0") .args([ "review", - "--workflow-git", + "--workflow-repo", "acme/workflows", "--workflow-ref", reference, diff --git a/lib/apps/fabro-cli/tests/it/cmd/run.rs b/lib/apps/fabro-cli/tests/it/cmd/run.rs index 19e389bfb..b5a00d326 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/run.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/run.rs @@ -124,33 +124,34 @@ fn help() { Usage: fabro run [OPTIONS] Arguments: - Workflow name or path (repository-relative with --workflow-git) + Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --workflow-git Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names - --target-path Observe this target directory instead of cwd; Folder targets require server filesystem access - --target-git Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials - --target-branch Target working branch (default: remote default branch), pinned to its observed commit - --dry-run Simulate execution; workflow source may still be fetched and uploaded - --auto-approve Auto-approve all human gates - --goal Override the workflow goal (available as {{ goal }} in prompts) - --goal-file Read a per-run goal value from a local file - --model Override default LLM model - --provider Override default LLM provider - -v, --verbose Enable verbose output - --environment Named environment for agent tools - --label Attach a label to this run (repeatable, format: KEY=VALUE) - --parent Link this run to an existing orchestration parent run - --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) - -d, --detach Run the workflow in the background and print the run ID - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + -I, --input Override a workflow input value (repeatable, format: KEY=VALUE) + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --workflow-repo Acquire workflow source locally from a GitHub OWNER/REPO using native Git credentials + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --workflow-ref Workflow branch, tag, HEAD (default), or full commit SHA; qualify ambiguous names + --target-from Observe this target directory instead of cwd; Folder targets require server filesystem access + --target Target GitHub repository and optional working branch + --target-repo Target GitHub OWNER/REPO; the execution sandbox still needs its own clone credentials + --target-branch Target working branch (default: remote default branch), pinned to its observed commit + --dry-run Simulate execution; workflow source may still be fetched and uploaded + --auto-approve Auto-approve all human gates + --goal Override the workflow goal (available as {{ goal }} in prompts) + --goal-file Read a per-run goal value from a local file + --model Override default LLM model + --provider Override default LLM provider + -v, --verbose Enable verbose output + --environment Named environment for agent tools + --label Attach a label to this run (repeatable, format: KEY=VALUE) + --parent Link this run to an existing orchestration parent run + --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) + -d, --detach Run the workflow in the background and print the run ID + -h, --help Print help ----- stderr ----- "); } @@ -1225,9 +1226,7 @@ fn run_starts_remote_workflow_once_and_failures_do_not_refetch() { .env("GIT_CONFIG_COUNT", "0") .env("GIT_TRACE", &trace) .args([ - "review", - "--workflow-git", - "acme/workflows", + "acme/workflows:review", "--server", &format!("{}/api/v1", server.base_url()), "--dry-run",