diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 558334219..af2866ed2 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -140,3 +140,93 @@ 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. 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-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 +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. + +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-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 +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. + +For local workflows without target flags, the existing `run.scm` 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-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 +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-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. +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 +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..659b31bf2 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, path, or OWNER/REPO[@REF]:WORKFLOW | #### 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,14 @@ 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-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-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` @@ -1061,7 +1067,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 +1077,7 @@ fabro run [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Local workflow name, checkout path, .fabro file, or workflow TOML | +| `WORKFLOW` | Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW | #### Options @@ -1079,7 +1085,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 +1095,14 @@ 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-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-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/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 3d43c52f1..e953d0a26 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -96,12 +96,11 @@ 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 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 } @@ -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/args.rs b/lib/apps/fabro-cli/src/args.rs index 16435b955..ac90101d9 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::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; @@ -233,11 +233,40 @@ pub(crate) struct RunArgs { #[command(flatten)] pub(crate) inputs: InputOverrideArgs, - /// Local workflow name, checkout path, .fabro file, or workflow TOML + /// Workflow name, path, or OWNER/REPO[@REF]:WORKFLOW #[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_repo: Option, + + /// Workflow branch, tag, HEAD (default), or full commit SHA; qualify + /// ambiguous names + #[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_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_repo: Option, + + /// Target working branch (default: remote default branch), pinned to its + /// observed commit + #[arg(long, requires = "target_repo", 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 +1272,12 @@ 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), + // 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 + Create(Box), /// Start a created workflow run on the server Start(StartArgs), /// Attach to a running or finished workflow run @@ -1974,3 +2005,40 @@ fn parse_reasoning_effort_arg(value: &str) -> Result { ) }) } + +#[cfg(test)] +mod run_selection_grammar_tests { + use crate::commands::run::test_support::parse_run_args; + + #[test] + fn run_selection_accepts_independent_resource_flags() { + for flags in [ + vec![ + "review", + "--workflow-repo", + "acme/workflows", + "--workflow-ref", + "refs/tags/v1", + "--target-repo", + "acme/app", + "--target-branch", + "release/topic", + ], + vec!["./review.toml", "--target-from", "../app"], + ] { + assert!(parse_run_args(flags).is_ok()); + } + } + + #[test] + fn run_selection_requires_modifier_owners_and_exclusive_targets() { + for flags in [ + vec!["review", "--workflow-ref", "v1"], + vec!["review", "--target-branch", "release"], + 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/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 10365ca4a..6e07c0a65 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -7,6 +7,10 @@ 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}; use crate::args::RunArgs; use crate::command_context::CommandContext; use crate::commands::resolve_run_id; @@ -16,19 +20,20 @@ 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. +/// +/// 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_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 +41,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), - )?; + let resolve_workflow = || { + resolution::workflow( + &workflow_selection, + &canonical_cwd, + Some(&user_workflows_root), + interruption, + ) + }; + // Preserve local lookup diagnostics before contacting the server. Remote + // acquisition waits until the parent and environment are validated. + let local_package = match &workflow_selection { + WorkflowSelection::Local(_) => Some(resolve_workflow().await?), + WorkflowSelection::Git { .. } => None, + }; let prepared = prepare_intent_overrides(args, &canonical_cwd).await?; warn_untransmitted_settings( @@ -49,7 +63,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,20 +90,36 @@ 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( + // 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?, + }; + // 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_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())? + } + _ => None, + }; + let (target, dirty_worktree) = resolution::target( + &target_selection, &environment.settings.provider, &canonical_cwd, configured_repo_origin_url.as_deref(), - )?; + interruption, + ) + .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:"), ); } diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index 7d0c2e3ed..b904549b4 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/lib/apps/fabro-cli/src/commands/run/mod.rs @@ -21,13 +21,18 @@ 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; +#[cfg(test)] +pub(crate) mod test_support; pub(crate) mod wait; pub(crate) async fn dispatch( @@ -38,11 +43,19 @@ 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)?; - 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/overrides.rs b/lib/apps/fabro-cli/src/commands/run/overrides.rs index 3c0ffca45..9f1f49d70 100644 --- a/lib/apps/fabro-cli/src/commands/run/overrides.rs +++ b/lib/apps/fabro-cli/src/commands/run/overrides.rs @@ -165,21 +165,27 @@ mod tests { fn run_args() -> RunArgs { RunArgs { - target: ServerTargetArgs::default(), - inputs: InputOverrideArgs::default(), - workflow: Some(PathBuf::from("workflow.fabro")), - 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 new file mode 100644 index 000000000..8bec413d7 --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -0,0 +1,1188 @@ +//! 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_proc::ProcessError; +use fabro_types::{GitHubRepositorySlug, GitRunTarget, repository}; +use tokio::process::Command; +use tokio::{fs, signal as tokio_signal, task}; +use tokio_util::sync::CancellationToken; +use tokio_util::task::TaskTracker; + +use super::selection::RemoteWorkflowRevision; +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( + "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), +} + +/// 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 { + timeout: Duration, + #[cfg(test)] + pub(super) environment: Vec<(String, String)>, +} + +impl NativeGit { + pub(super) fn new() -> Self { + Self { + timeout: Duration::from_mins(2), + #[cfg(test)] + environment: Vec::new(), + } + } + + /// Run one Git command; `operation` labels it in failure diagnostics. + async fn command( + &self, + operation: &'static str, + cwd: &Path, + args: &[&str], + cancel: &CancellationToken, + ) -> Result, RemoteWorkflowError> { + if cancel.is_cancelled() { + return Err(RemoteWorkflowError::Cancelled); + } + let mut command = Command::new("git"); + command + .stdin(Stdio::null()) + .current_dir(cwd) + .args(HARDENED_GIT_CONFIG) + .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"); + #[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), + })?; + 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, + }); + } + if output.stdout_truncated { + return Err(RemoteWorkflowError::OutputLimit); + } + Ok(output.output.stdout) + } + + /// 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, + ) -> 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("metadata lookup", root, &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 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(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(root, &repository, &["HEAD".into()], cancel) + .await?; + default_target_branch(&records)? + }; + let target = GitRunTarget { + repo: repository.to_string(), + branch, + tag: None, + sha: Some(sha), + }; + Ok(target.validate()?.into_target()) + } + + /// 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, + ) -> anyhow::Result { + match revision { + RemoteWorkflowRevision::Commit(sha) => Ok(sha.clone()), + RemoteWorkflowRevision::DefaultBranch => { + let records = self + .records(root, repository, &["HEAD".into()], cancel) + .await?; + Ok(default_head(&records)?.1) + } + RemoteWorkflowRevision::Branch(reference) => { + let records = self + .records(root, repository, std::slice::from_ref(reference), cancel) + .await?; + 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) + } + } + } + + pub(super) async fn collect( + &self, + repository: GitHubRepositorySlug, + selector: PathBuf, + revision: RemoteWorkflowRevision, + cancel: CancellationToken, + ) -> anyhow::Result { + let checkout = self.scratch_repository(&cancel).await?; + let sha = self + .resolve_revision(checkout.path(), &repository, &revision, &cancel) + .await?; + 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 || { + 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) + } + + /// Fetch and check out `sha` into `root`, an initialized scratch + /// repository. + async fn checkout( + &self, + repository: &GitHubRepositorySlug, + sha: &str, + root: &Path, + cancel: &CancellationToken, + ) -> anyhow::Result<()> { + self.command( + "fetch", + root, + &[ + "fetch", + "--quiet", + "--depth=1", + "--no-tags", + "--no-recurse-submodules", + &repository.https_url(), + 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"); + } + // 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( + "checkout", + root, + &[ + "-c", + &format!("core.worktree={}", root.display()), + "checkout", + "--quiet", + "--detach", + sha, + "--", + ], + cancel, + ) + .await?; + let head = self + .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"); + } + Ok(()) + } +} + +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")?, + )) +} + +/// 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)) +} + +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}^{{}}")] +} + +/// 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), + )) +} + +/// 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), + } +} + +/// 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_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 + /// 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)] +#[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 std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + 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::*; + + #[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, + 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(); + write_workflow(&repo_dir, ".fabro/workflows/review"); + let sha = commit_all(&repo, "workflow"); + 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(); + 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() + } + } + + #[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(); + // 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", + &format!("touch '{}'; cat", sentinel.display()), + ) + .unwrap(); + 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 = fixture.git.scratch_repository(&cancel).await.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 = fixture.git.scratch_repository(&cancel).await.unwrap(); + let path = checkout.path().to_path_buf(); + assert!( + fixture + .git + .collect_checkout( + Fixture::repository(), + "review".into(), + "1111111111111111111111111111111111111111".into(), + checkout, + cancel + ) + .await + .is_err() + ); + 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(); + 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 = commit_all(&fixture.repo, "update workflow"); + let error = fixture + .git + .collect( + Fixture::repository(), + "review".into(), + RemoteWorkflowRevision::Commit(sha), + CancellationToken::new(), + ) + .await + .unwrap_err(); + assert!( + format!("{error:?}").contains("outside its source root"), + "{error:?}" + ); + } + + #[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(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; + } + }) + .await + .unwrap(); + let pid: u32 = 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!(!fabro_proc::process_exists(pid)); + 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(); + 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(); + let scratch = fixture.git.scratch_repository(&cancel).await.unwrap(); + 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(scratch.path(), &Fixture::repository(), &revision, &cancel) + .await + .unwrap(), + fixture.sha + ); + } + fixture + .repo + .tag_lightweight("trunk", &object, false) + .unwrap(); + assert!( + fixture + .git + .resolve_revision( + scratch.path(), + &Fixture::repository(), + &RemoteWorkflowRevision::Name("trunk".into()), + &cancel + ) + .await + .unwrap_err() + .to_string() + .contains("ambiguous") + ); + for reference in ["missing", "refs/tags/missing", "refs/heads/missing"] { + assert!( + fixture + .git + .resolve_revision( + scratch.path(), + &Fixture::repository(), + &RemoteWorkflowRevision::parse(Some(reference)).unwrap(), + &cancel + ) + .await + .unwrap_err() + .to_string() + .contains("was not found"), + "{reference}" + ); + } + 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 checkout = fixture.git.scratch_repository(&cancel).await.unwrap(); + let captured = fixture + .git + .resolve_revision( + checkout.path(), + &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); + 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_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() { + let sha = "1234567890123456789012345678901234567890"; + assert!( + resolve_name( + &format!("{sha}\trefs/heads/nested/main\n"), + "refs/heads/main", + "refs/tags/main" + ) + .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(); + 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("fetch", 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( + "metadata lookup", + root.path(), + &["ls-remote"], + &CancellationToken::new() + ) + .await + .unwrap_err(), + RemoteWorkflowError::OutputLimit + )); + } + + #[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] { + 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("fetch", root.path(), &["fetch"], &cancel), + trigger + ); + let error = result.unwrap_err(); + assert!(matches!( + error, + RemoteWorkflowError::Timeout | RemoteWorkflowError::Cancelled + )); + let pid: u32 = std::fs::read_to_string(root.path().join("pid")) + .unwrap() + .parse() + .unwrap(); + 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 new file mode 100644 index 000000000..085030c2c --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -0,0 +1,247 @@ +use std::path::Path; + +use anyhow::{Context as _, bail}; +use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; +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 for settings warnings and target +/// inference. +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>, + interruption: &Interruption, +) -> 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(); + let (repository, selector, revision) = + (repository.clone(), selector.clone(), revision.clone()); + let closure = interruption + .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: &SandboxProviderKind, + cwd: &Path, + configured_repo_origin_url: Option<&str>, + interruption: &Interruption, +) -> anyhow::Result<(RunTarget, bool)> { + let path = match selection { + TargetSelection::Path(path) => cwd + .join(path) + .canonicalize() + .context("failed to canonicalize target directory")?, + TargetSelection::Git { repository, branch } => { + if !provider.clones_workspace() { + bail!("Git targets require a clone-enabled environment"); + } + let git = NativeGit::new(); + let (repository, branch) = (repository.clone(), branch.clone()); + 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)); + } + }; + if !path.is_dir() { + bail!("target path must be a directory"); + } + // 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(); + 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)] +#[expect( + clippy::disallowed_methods, + reason = "resolver tests construct small local workflow fixtures" +)] +mod tests { + use super::super::test_support::write_workflow; + use super::*; + + #[tokio::test] + 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 selected = TargetSelection::Path("target".into()); + let interruption = Interruption::new(false); + assert_eq!( + target( + &selected, + &SandboxProviderKind::LOCAL, + &root, + None, + &interruption + ) + .await + .unwrap() + .0, + RunTarget::Folder { + path: root.join("target").to_str().unwrap().into(), + } + ); + for provider in [ + SandboxProviderKind::DOCKER, + SandboxProviderKind::DAYTONA, + SandboxProviderKind::try_new("host").unwrap(), + ] { + assert_eq!( + target(&selected, &provider, &root, None, &interruption) + .await + .unwrap() + .0, + RunTarget::None {} + ); + } + assert_eq!( + target( + &TargetSelection::Path(".".into()), + &SandboxProviderKind::LOCAL, + &root, + None, + &interruption + ) + .await + .unwrap() + .0, + RunTarget::Folder { + path: root.to_str().unwrap().into(), + } + ); + assert!( + target( + &TargetSelection::Git { + repository: "acme/app".parse().unwrap(), + branch: None, + }, + &SandboxProviderKind::LOCAL, + &root, + None, + &interruption + ) + .await + .unwrap_err() + .to_string() + .contains("clone-enabled environment") + ); + for path in ["missing", ".fabro/workflows/review/workflow.toml"] { + assert!( + target( + &TargetSelection::Path(path.into()), + &SandboxProviderKind::LOCAL, + &root, + None, + &interruption + ) + .await + .is_err(), + "{path}" + ); + } + } + + #[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()); + 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), &interruption) + .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), + &interruption + ) + .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..d413487e7 --- /dev/null +++ b/lib/apps/fabro-cli/src/commands/run/selection.rs @@ -0,0 +1,444 @@ +//! CLI syntax ends here. Resolvers receive selections and explicit caller +//! context. +use std::path::{Path, PathBuf}; + +use anyhow::{Context as _, bail}; +use fabro_types::{GitHubRepositorySlug, WorkflowPath, 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 { + /// Directory relative to the caller; the default is the caller directory + /// itself. + Path(PathBuf), + Git { + repository: GitHubRepositorySlug, + branch: Option, + }, +} + +/// 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, + /// 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), +} + +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) { + bail!("workflow ref must be a branch, tag, HEAD, or full 40-hex commit SHA"); + } + 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)) + } + } + } + } +} + +pub(super) fn validate_remote_selector(path: &Path) -> anyhow::Result<()> { + let value = path + .to_str() + .context("remote workflow selector must be valid UTF-8")?; + 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" + ); + } + 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" + ), + } +} + +/// 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_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(), + selector: workflow.clone(), + revision: RemoteWorkflowRevision::parse(args.workflow_ref.as_deref())?, + } + } + }; + 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(".")), + } + }; + 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::*; + + #[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, 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()), + ), + ] { + assert_eq!( + RemoteWorkflowRevision::parse(Some(value)).unwrap(), + expected + ); + } + assert_eq!( + RemoteWorkflowRevision::parse(None).unwrap(), + RemoteWorkflowRevision::DefaultBranch + ); + 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 super::super::test_support::parse_run_args; + 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"] { + let cli = Cli::try_parse_from([ + "fabro", + command, + "review", + "--workflow-repo", + "acme/workflows", + "--workflow-ref", + "v1", + "--target-repo", + "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::Name("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() { + // Malformed repository slugs never reach the adapter. + for flags in [ + [ + "review", + "--workflow-repo", + "https://github.com/acme/workflows", + ], + ["review", "--target-repo", "acme/app/extra"], + ] { + assert!(parse_run_args(flags).is_err()); + } + for flags in [ + vec!["../review.toml", "--workflow-repo", "acme/workflows"], + vec!["/tmp/review.toml", "--workflow-repo", "acme/workflows"], + vec![ + "review", + "--workflow-repo", + "acme/workflows", + "--workflow-ref", + "HEAD~1", + ], + vec![ + "review", + "--target-repo", + "acme/app", + "--target-branch", + "refs/tags/v1", + ], + vec![ + "review", + "--target-repo", + "acme/app", + "--target-branch", + "1234567890123456789012345678901234567890", + ], + ] { + 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 0818a83b6..4b73e98fe 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!(); @@ -87,33 +69,39 @@ 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, 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) - --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 - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --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 ----- "); } @@ -688,6 +676,112 @@ 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-from", + ".", + ]) + .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 + ); + 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] fn create_clone_targets_require_exact_git_observations() { let context = test_context!(); @@ -1633,3 +1727,278 @@ draft = false assert!(pull_request.enabled); assert!(!pull_request.draft); } + +#[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 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)); + 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", "shorthand"] { + for target_kind in ["inferred", "path", "git", "git-plugin", "shorthand"] { + 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 == "shorthand" { + "acme/workflows@trunk:review" + } else if source_kind == "file" { + ".fabro/workflows/review/workflow.toml" + } else { + "review" + }); + if source_kind == "git" { + command.args([ + "--workflow-repo", + "acme/workflows", + "--workflow-ref", + "trunk", + ]); + } + match target_kind { + "shorthand" => { + command.args(["--target", "acme/app@release", "--environment", "docker"]); + } + "path" => { + command.args(["--target-from", "../target", "--environment", "local"]); + } + "git" | "git-plugin" => { + command.args([ + "--target-repo", + "acme/app", + "--target-branch", + "release", + "--environment", + if target_kind == "git-plugin" { + "plugin" + } else { + "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" | "shorthand" => 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" | "git-plugin" | "shorthand" => + json!({"kind":"git","repo":"acme/app","branch":"release","sha":target_sha}), + _ => json!({"kind":"none"}), + }); + } + } + local_env.assert_calls(4); + docker_env.assert_calls(12); + plugin_env.assert_calls(4); + versions.assert_calls(20); + create.assert_calls(20); +} + +#[test] +fn create_leaves_remote_workflow_run_submitted_without_starting() { + 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(); + 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-repo", + "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] +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"); + // 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", + "Installed", + ); + 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(), + origin.display() + ), + ) + .unwrap(); + for reference in [ + "trunk", + "missing", + "1111111111111111111111111111111111111111", + ] { + let output = context + .create_cmd() + .current_dir(&caller) + .env("GIT_CONFIG_GLOBAL", &config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .args([ + "review", + "--workflow-repo", + "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); + // 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" + ); +} 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..b5a00d326 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}; @@ -119,33 +119,39 @@ 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, 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) - --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 - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --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 ----- "); } @@ -1161,3 +1167,98 @@ 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([ + "acme/workflows:review", + "--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) 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();