mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-14 23:22:51 +00:00
Adapt workflow target selection to sandbox provider kinds
This commit is contained in:
parent
321a190315
commit
a9f28828a6
6 changed files with 77 additions and 36 deletions
|
|
@ -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 |
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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::<std::io::Error>()
|
||||
.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] {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue