diff --git a/docs/public/core-concepts/workflows.mdx b/docs/public/core-concepts/workflows.mdx index 4119ef9e4..334bb642f 100644 --- a/docs/public/core-concepts/workflows.mdx +++ b/docs/public/core-concepts/workflows.mdx @@ -183,7 +183,7 @@ finish before its files can be removed. Target selection depends on the environment: -| Selection | Local environment | Docker/Daytona environment | +| Selection | Local environment | Clone-based environment (Docker, Daytona, or plugin) | | --- | --- | --- | | Default cwd or `--target-path PATH` | Uses the live directory, including uncommitted files | Uses the enclosing Git repository and exact available commit; a non-Git directory selects an empty workspace | | `--target-git OWNER/REPO` | Rejected | Uses the selected repository and exact observed branch commit; cloning must be enabled | diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index a7a34873c..fab071a5a 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -6,9 +6,9 @@ use clap::{Args, Parser, Subcommand, ValueEnum}; use fabro_config::{CliLayer, CliLoggingLayer, CliOutputLayer, CliUpdatesLayer}; use fabro_server::serve::DEFAULT_TCP_PORT; use fabro_static::EnvVars; -use fabro_types::{GitHubRepositorySlug, PermissionLevel}; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_types::settings::run::MergeStrategy; +use fabro_types::{GitHubRepositorySlug, PermissionLevel}; use fabro_util::printer::Printer; use lithos_llm::catalog::ProviderId; use lithos_llm::types::ReasoningEffort; diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index ed69e165c..750e175d6 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -98,7 +98,7 @@ pub(crate) async fn create_run( }; let (target, dirty_worktree) = resolution::target( &target_selection, - environment.settings.provider, + &environment.settings.provider, &canonical_cwd, interruption, ) diff --git a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs index cefe892be..d1244b907 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -109,18 +109,14 @@ impl NativeGit { .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES"); #[cfg(test)] command.envs(self.environment.iter().cloned()); - let output = fabro_proc::capture( - &mut command, - Some(self.timeout), - cancel, - Some(OUTPUT_LIMIT), - ) - .await - .map_err(|error| match error { - ProcessError::TimedOut => RemoteWorkflowError::Timeout, - ProcessError::Cancelled => RemoteWorkflowError::Cancelled, - ProcessError::Io(source) => RemoteWorkflowError::Io(source), - })?; + let output = + fabro_proc::capture(&mut command, Some(self.timeout), cancel, Some(OUTPUT_LIMIT)) + .await + .map_err(|error| match error { + ProcessError::TimedOut => RemoteWorkflowError::Timeout, + ProcessError::Cancelled => RemoteWorkflowError::Cancelled, + ProcessError::Io(source) => RemoteWorkflowError::Io(source), + })?; if !output.output.status.success() { // Output may contain arbitrary helper/config secrets, even after pattern // redaction. Never retain it in an error/cause chain or tracing event. @@ -1099,6 +1095,41 @@ mod tests { )); } + #[tokio::test] + async fn remote_workflow_cancelled_command_never_spawns() { + let (root, git) = fake_git("printf started > spawned"); + let cancel = CancellationToken::new(); + cancel.cancel(); + let error = git + .command("fetch", root.path(), &["fetch"], &cancel) + .await + .unwrap_err(); + assert!(matches!(error, RemoteWorkflowError::Cancelled)); + assert!(!root.path().join("spawned").exists()); + } + + #[tokio::test] + async fn remote_workflow_spawn_failure_preserves_io_source() { + use std::error::Error as _; + + let (root, git) = fake_git("exit 0"); + let error = git + .command( + "fetch", + &root.path().join("missing"), + &["fetch"], + &CancellationToken::new(), + ) + .await + .unwrap_err(); + let source = error + .source() + .unwrap() + .downcast_ref::() + .unwrap(); + assert_eq!(source.kind(), std::io::ErrorKind::NotFound); + } + #[tokio::test] async fn remote_workflow_timeout_and_cancel_reap_owned_children() { for timeout in [true, false] { diff --git a/lib/apps/fabro-cli/src/commands/run/resolution.rs b/lib/apps/fabro-cli/src/commands/run/resolution.rs index 556e92d1d..049ddd7f2 100644 --- a/lib/apps/fabro-cli/src/commands/run/resolution.rs +++ b/lib/apps/fabro-cli/src/commands/run/resolution.rs @@ -2,8 +2,7 @@ use std::path::Path; use anyhow::{Context as _, anyhow, bail}; use fabro_manifest::{CollectedWorkflowClosure, ResolvedLocalWorkflowPackage}; -use fabro_types::settings::run::EnvironmentProvider; -use fabro_types::{DirtyStatus, RunTarget}; +use fabro_types::{DirtyStatus, RunTarget, SandboxProviderKind}; use tokio::task; use super::remote_workflow::{Interruption, NativeGit}; @@ -70,7 +69,7 @@ pub(super) async fn workflow( pub(super) async fn target( selection: &TargetSelection, - provider: EnvironmentProvider, + provider: &SandboxProviderKind, cwd: &Path, interruption: &Interruption, ) -> anyhow::Result<(RunTarget, bool)> { @@ -80,8 +79,8 @@ pub(super) async fn target( .canonicalize() .context("failed to canonicalize target directory")?, TargetSelection::Git { repository, branch } => { - if !provider.is_clone_based() { - bail!("Git targets require a clone-enabled Docker or Daytona environment"); + if !provider.clones_workspace() { + bail!("Git targets require a clone-enabled environment"); } let git = NativeGit::new(); let (repository, branch) = (repository.clone(), branch.clone()); @@ -99,7 +98,8 @@ pub(super) async fn target( } // The existing observer can push/query Git synchronously. Preserve its // behavior without blocking a Tokio worker or promising a new timeout. - task::spawn_blocking(move || run_target_for_environment(provider, &path)) + let provider = provider.clone(); + task::spawn_blocking(move || run_target_for_environment(&provider, &path)) .await .context("target observation task failed")? } @@ -108,10 +108,10 @@ pub(super) async fn target( /// provider. Returns the target plus whether a clone-based observation found a /// dirty Git worktree, so the caller can warn about it. fn run_target_for_environment( - provider: EnvironmentProvider, + provider: &SandboxProviderKind, canonical_cwd: &Path, ) -> anyhow::Result<(RunTarget, bool)> { - if !provider.is_clone_based() { + if !provider.clones_workspace() { let path = canonical_cwd.to_str().ok_or_else(|| { anyhow!( "target directory is not valid UTF-8: {}", @@ -205,7 +205,7 @@ mod tests { let selected = TargetSelection::Path("target".into()); let interruption = Interruption::new(false); assert_eq!( - target(&selected, EnvironmentProvider::Local, &root, &interruption) + target(&selected, &SandboxProviderKind::LOCAL, &root, &interruption) .await .unwrap() .0, @@ -213,9 +213,13 @@ mod tests { path: root.join("target").to_str().unwrap().into(), } ); - for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { + for provider in [ + SandboxProviderKind::DOCKER, + SandboxProviderKind::DAYTONA, + SandboxProviderKind::try_new("host").unwrap(), + ] { assert_eq!( - target(&selected, provider, &root, &interruption) + target(&selected, &provider, &root, &interruption) .await .unwrap() .0, @@ -225,7 +229,7 @@ mod tests { assert_eq!( target( &TargetSelection::Path(".".into()), - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) @@ -242,20 +246,20 @@ mod tests { repository: "acme/app".parse().unwrap(), branch: None, }, - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) .await .unwrap_err() .to_string() - .contains("Docker or Daytona") + .contains("clone-enabled environment") ); for path in ["missing", ".fabro/workflows/review/workflow.toml"] { assert!( target( &TargetSelection::Path(path.into()), - EnvironmentProvider::Local, + &SandboxProviderKind::LOCAL, &root, &interruption ) diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index bffcb070d..c0ca5f604 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -1627,6 +1627,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde let server = MockServer::start(); let local_env = mock_environment(&server, "local", "local"); let docker_env = mock_environment(&server, "docker", "docker"); + let plugin_env = mock_environment(&server, "plugin", "host"); let versions = mock_workflow_version_registrations(&server); let requests = Arc::new(Mutex::new(Vec::new())); let create = mock_intent_create(&server, &unique_run_id(), Arc::clone(&requests)); @@ -1665,7 +1666,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde .root_id(); assert_ne!(local_id, remote_id); for source_kind in ["name", "file", "git"] { - for target_kind in ["inferred", "path", "git"] { + for target_kind in ["inferred", "path", "git", "git-plugin"] { let mut command = context.create_cmd(); command .current_dir(&caller) @@ -1695,14 +1696,18 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde "path" => { command.args(["--target-path", "../target", "--environment", "local"]); } - "git" => { + "git" | "git-plugin" => { command.args([ "--target-git", "acme/app", "--target-branch", "release", "--environment", - "docker", + if target_kind == "git-plugin" { + "plugin" + } else { + "docker" + }, ]); } _ => { @@ -1729,7 +1734,7 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde assert_eq!(intent["goal"], "Caller goal"); assert_eq!(intent["target"], match target_kind { "path" => json!({"kind":"folder","path":target.canonicalize().unwrap()}), - "git" => + "git" | "git-plugin" => json!({"kind":"git","repo":"acme/app","branch":"release","sha":target_sha}), _ => json!({"kind":"none"}), }); @@ -1737,8 +1742,9 @@ fn run_selection_source_target_cross_product_keeps_workflow_goal_and_target_inde } local_env.assert_calls(3); docker_env.assert_calls(6); - versions.assert_calls(9); - create.assert_calls(9); + plugin_env.assert_calls(3); + versions.assert_calls(12); + create.assert_calls(12); } #[test]