mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
Seal sandbox provider abstraction leaks in run.rs
Add is_remote(), ssh_access_command(), and origin_url() to the Sandbox trait so run.rs can use trait methods instead of matching on SandboxProvider post-construction. This removes daytona_sandbox_ref and exe_sandbox_ref, eliminating 11 provider-checking sites that reached through Arc<dyn Sandbox> to make provider-specific decisions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
48c0f61874
commit
0a42050ece
6 changed files with 128 additions and 101 deletions
|
|
@ -109,6 +109,18 @@ macro_rules! delegate_sandbox {
|
|||
self.$field.set_autostop_interval(minutes).await
|
||||
}
|
||||
|
||||
fn is_remote(&self) -> bool {
|
||||
self.$field.is_remote()
|
||||
}
|
||||
|
||||
async fn ssh_access_command(&self) -> Result<Option<String>, String> {
|
||||
self.$field.ssh_access_command().await
|
||||
}
|
||||
|
||||
fn origin_url(&self) -> Option<&str> {
|
||||
self.$field.origin_url()
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &str,
|
||||
|
|
@ -383,6 +395,21 @@ pub trait Sandbox: Send + Sync {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
/// Whether this sandbox runs on a remote machine (e.g. Daytona, exe.dev).
|
||||
fn is_remote(&self) -> bool {
|
||||
false
|
||||
}
|
||||
|
||||
/// Return an SSH command string for connecting to this sandbox, if supported.
|
||||
async fn ssh_access_command(&self) -> Result<Option<String>, String> {
|
||||
Ok(None)
|
||||
}
|
||||
|
||||
/// The display URL of the cloned origin remote, if known.
|
||||
fn origin_url(&self) -> Option<&str> {
|
||||
None
|
||||
}
|
||||
|
||||
/// Record that the agent has explicitly read (seen) the given file path.
|
||||
/// Called by tool executors after agent-visible reads (e.g. `read_file`, `grep`).
|
||||
/// Default is a no-op; `ReadBeforeWriteSandbox` overrides to populate its read set.
|
||||
|
|
|
|||
|
|
@ -806,6 +806,18 @@ impl Sandbox for ExeSandbox {
|
|||
_ => String::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn is_remote(&self) -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
async fn ssh_access_command(&self) -> Result<Option<String>, String> {
|
||||
self.ssh_command().map(Some)
|
||||
}
|
||||
|
||||
fn origin_url(&self) -> Option<&str> {
|
||||
self.origin_url.get().map(String::as_str)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -33,6 +33,12 @@ pub enum SandboxProvider {
|
|||
Exe,
|
||||
}
|
||||
|
||||
impl SandboxProvider {
|
||||
pub fn is_remote(&self) -> bool {
|
||||
matches!(self, Self::Daytona | Self::Exe)
|
||||
}
|
||||
}
|
||||
|
||||
impl fmt::Display for SandboxProvider {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
match self {
|
||||
|
|
|
|||
|
|
@ -395,11 +395,10 @@ pub async fn run_command(
|
|||
crate::daytona_sandbox::detect_repo_info(&original_cwd)
|
||||
.map(|(url, branch)| (Some(url), branch))
|
||||
.unwrap_or((None, None));
|
||||
let git_clean = match sandbox_provider {
|
||||
SandboxProvider::Local | SandboxProvider::Docker => {
|
||||
crate::git::ensure_clean(&original_cwd).is_ok()
|
||||
}
|
||||
SandboxProvider::Daytona | SandboxProvider::Exe => false,
|
||||
let git_clean = if sandbox_provider.is_remote() {
|
||||
false
|
||||
} else {
|
||||
crate::git::ensure_clean(&original_cwd).is_ok()
|
||||
};
|
||||
|
||||
if args.preflight {
|
||||
|
|
@ -587,8 +586,6 @@ pub async fn run_command(
|
|||
// Wrap emitter in Arc now so we can share it with exec env callbacks
|
||||
let emitter = Arc::new(emitter);
|
||||
|
||||
let mut daytona_sandbox_ref: Option<Arc<crate::daytona_sandbox::DaytonaSandbox>> = None;
|
||||
let mut exe_sandbox_ref: Option<Arc<arc_exe::ExeSandbox>> = None;
|
||||
let sandbox: Arc<dyn Sandbox> = match sandbox_provider {
|
||||
SandboxProvider::Docker => {
|
||||
let config = DockerSandboxConfig {
|
||||
|
|
@ -618,9 +615,7 @@ pub async fn run_command(
|
|||
env.set_event_callback(Arc::new(move |event| {
|
||||
emitter_cb.emit(&crate::event::WorkflowRunEvent::Sandbox { event });
|
||||
}));
|
||||
let daytona_arc = Arc::new(env);
|
||||
daytona_sandbox_ref = Some(Arc::clone(&daytona_arc));
|
||||
daytona_arc
|
||||
Arc::new(env)
|
||||
}
|
||||
SandboxProvider::Exe => {
|
||||
let clone_params = resolve_exe_clone_params(&original_cwd, github_app.as_ref()).await;
|
||||
|
|
@ -639,9 +634,7 @@ pub async fn run_command(
|
|||
env.set_event_callback(Arc::new(move |event| {
|
||||
emitter_cb.emit(&crate::event::WorkflowRunEvent::Sandbox { event });
|
||||
}));
|
||||
let exe_arc = Arc::new(env);
|
||||
exe_sandbox_ref = Some(Arc::clone(&exe_arc));
|
||||
exe_arc
|
||||
Arc::new(env)
|
||||
}
|
||||
SandboxProvider::Local => {
|
||||
let mut env = LocalSandbox::new(cwd.clone());
|
||||
|
|
@ -694,32 +687,33 @@ pub async fn run_command(
|
|||
container_mount_point: None,
|
||||
data_host: None,
|
||||
},
|
||||
SandboxProvider::Exe => crate::sandbox_record::SandboxRecord {
|
||||
provider: "exe".to_string(),
|
||||
working_directory: sandbox.working_directory().to_string(),
|
||||
identifier: exe_sandbox_ref
|
||||
.as_ref()
|
||||
.and_then(|e| e.vm_name().map(String::from)),
|
||||
host_working_directory: None,
|
||||
container_mount_point: None,
|
||||
data_host: exe_sandbox_ref
|
||||
.as_ref()
|
||||
.and_then(|e| e.data_host().map(String::from)),
|
||||
},
|
||||
SandboxProvider::Exe => {
|
||||
// Extract data_host from the ssh access command ("ssh <host>")
|
||||
let data_host = sandbox
|
||||
.ssh_access_command()
|
||||
.await
|
||||
.ok()
|
||||
.flatten()
|
||||
.and_then(|cmd| cmd.strip_prefix("ssh ").map(String::from));
|
||||
crate::sandbox_record::SandboxRecord {
|
||||
provider: "exe".to_string(),
|
||||
working_directory: sandbox.working_directory().to_string(),
|
||||
identifier: sandbox_info_opt,
|
||||
host_working_directory: None,
|
||||
container_mount_point: None,
|
||||
data_host,
|
||||
}
|
||||
}
|
||||
};
|
||||
if let Err(e) = record.save(&logs_dir.join("sandbox.json")) {
|
||||
tracing::warn!(error = %e, "Failed to save sandbox record");
|
||||
}
|
||||
}
|
||||
|
||||
// Wrap exe.dev sandbox with GitCredentialSandbox for push credential refresh
|
||||
let sandbox: Arc<dyn Sandbox> = if sandbox_provider == SandboxProvider::Exe {
|
||||
let origin_url = exe_sandbox_ref
|
||||
.as_ref()
|
||||
.and_then(|e| e.origin_url().map(String::from));
|
||||
// Wrap remote sandbox with GitCredentialSandbox for push credential refresh
|
||||
let sandbox: Arc<dyn Sandbox> = if sandbox.is_remote() {
|
||||
Arc::new(crate::git_credential_sandbox::GitCredentialSandbox::new(
|
||||
sandbox,
|
||||
origin_url,
|
||||
github_app.clone(),
|
||||
))
|
||||
} else {
|
||||
|
|
@ -744,10 +738,7 @@ pub async fn run_command(
|
|||
});
|
||||
|
||||
// Set up git inside remote sandbox (Daytona or exe.dev) for checkpoint commits
|
||||
let (remote_base_sha, remote_branch, remote_base_branch) = if matches!(
|
||||
sandbox_provider,
|
||||
SandboxProvider::Daytona | SandboxProvider::Exe
|
||||
) {
|
||||
let (remote_base_sha, remote_branch, remote_base_branch) = if sandbox.is_remote() {
|
||||
match setup_remote_git(&*sandbox, &run_id).await {
|
||||
Ok((base, branch, base_br)) => (Some(base), Some(branch), base_br),
|
||||
Err(e) => {
|
||||
|
|
@ -764,35 +755,22 @@ pub async fn run_command(
|
|||
|
||||
// Create SSH access if requested
|
||||
if args.ssh {
|
||||
if let Some(ref daytona) = daytona_sandbox_ref {
|
||||
match daytona.create_ssh_access().await {
|
||||
Ok(ssh_command) => {
|
||||
emitter.emit(&crate::event::WorkflowRunEvent::SshAccessReady { ssh_command });
|
||||
}
|
||||
Err(e) => {
|
||||
eprintln!(
|
||||
"{} Failed to create SSH access: {e}",
|
||||
styles.yellow.apply_to("Warning:"),
|
||||
);
|
||||
}
|
||||
match sandbox.ssh_access_command().await {
|
||||
Ok(Some(ssh_command)) => {
|
||||
emitter.emit(&crate::event::WorkflowRunEvent::SshAccessReady { ssh_command });
|
||||
}
|
||||
} else if let Some(ref exe) = exe_sandbox_ref {
|
||||
match exe.ssh_command() {
|
||||
Ok(ssh_command) => {
|
||||
emitter.emit(&crate::event::WorkflowRunEvent::SshAccessReady { ssh_command });
|
||||
}
|
||||
Err(e) => {
|
||||
eprintln!(
|
||||
"{} Failed to get exe.dev SSH command: {e}",
|
||||
styles.yellow.apply_to("Warning:"),
|
||||
);
|
||||
}
|
||||
Ok(None) => {
|
||||
eprintln!(
|
||||
"{} --ssh only works with --sandbox daytona or exe, skipping.",
|
||||
styles.yellow.apply_to("Warning:"),
|
||||
);
|
||||
}
|
||||
Err(e) => {
|
||||
eprintln!(
|
||||
"{} Failed to create SSH access: {e}",
|
||||
styles.yellow.apply_to("Warning:"),
|
||||
);
|
||||
}
|
||||
} else {
|
||||
eprintln!(
|
||||
"{} --ssh only works with --sandbox daytona or exe, skipping.",
|
||||
styles.yellow.apply_to("Warning:"),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -940,13 +918,12 @@ pub async fn run_command(
|
|||
cancel_token: None,
|
||||
dry_run: dry_run_mode,
|
||||
run_id,
|
||||
git_checkpoint: match sandbox_provider {
|
||||
SandboxProvider::Local | SandboxProvider::Docker => {
|
||||
worktree_work_dir.map(GitCheckpointMode::Host)
|
||||
}
|
||||
SandboxProvider::Daytona | SandboxProvider::Exe => remote_base_sha
|
||||
git_checkpoint: if sandbox.is_remote() {
|
||||
remote_base_sha
|
||||
.as_ref()
|
||||
.map(|_| GitCheckpointMode::Remote(original_cwd.clone())),
|
||||
.map(|_| GitCheckpointMode::Remote(original_cwd.clone()))
|
||||
} else {
|
||||
worktree_work_dir.map(GitCheckpointMode::Host)
|
||||
},
|
||||
base_sha: worktree_base_sha.or(remote_base_sha),
|
||||
run_branch: worktree_branch.or(remote_branch),
|
||||
|
|
@ -1337,7 +1314,6 @@ async fn run_from_branch(
|
|||
|
||||
let emitter = Arc::new(EventEmitter::new());
|
||||
let mut worktree_path: Option<PathBuf> = None;
|
||||
let mut exe_sandbox_ref: Option<Arc<arc_exe::ExeSandbox>> = None;
|
||||
|
||||
let sandbox: Arc<dyn arc_agent::Sandbox> = match sandbox_provider {
|
||||
SandboxProvider::Local | SandboxProvider::Docker => {
|
||||
|
|
@ -1371,9 +1347,7 @@ async fn run_from_branch(
|
|||
env.set_event_callback(Arc::new(move |event| {
|
||||
emitter_cb.emit(&crate::event::WorkflowRunEvent::Sandbox { event });
|
||||
}));
|
||||
let exe_arc = Arc::new(env);
|
||||
exe_sandbox_ref = Some(Arc::clone(&exe_arc));
|
||||
exe_arc
|
||||
Arc::new(env)
|
||||
}
|
||||
SandboxProvider::Daytona => {
|
||||
bail!("--run-branch resume is not yet supported with --sandbox daytona");
|
||||
|
|
@ -1381,11 +1355,11 @@ async fn run_from_branch(
|
|||
};
|
||||
|
||||
// Initialize remote sandboxes and checkout the run branch
|
||||
if sandbox_provider == SandboxProvider::Exe {
|
||||
if sandbox.is_remote() {
|
||||
sandbox
|
||||
.initialize()
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("Failed to initialize exe.dev sandbox: {e}"))?;
|
||||
.map_err(|e| anyhow::anyhow!("Failed to initialize sandbox: {e}"))?;
|
||||
|
||||
// Fetch and checkout the run branch inside the sandbox
|
||||
let fetch_cmd = format!("git fetch origin {run_branch} && git checkout {run_branch}");
|
||||
|
|
@ -1402,14 +1376,10 @@ async fn run_from_branch(
|
|||
}
|
||||
}
|
||||
|
||||
// Wrap exe.dev sandbox with GitCredentialSandbox for push credential refresh
|
||||
let sandbox: Arc<dyn arc_agent::Sandbox> = if sandbox_provider == SandboxProvider::Exe {
|
||||
let origin_url = exe_sandbox_ref
|
||||
.as_ref()
|
||||
.and_then(|e| e.origin_url().map(String::from));
|
||||
// Wrap remote sandbox with GitCredentialSandbox for push credential refresh
|
||||
let sandbox: Arc<dyn arc_agent::Sandbox> = if sandbox.is_remote() {
|
||||
Arc::new(crate::git_credential_sandbox::GitCredentialSandbox::new(
|
||||
sandbox,
|
||||
origin_url,
|
||||
github_app.clone(),
|
||||
))
|
||||
} else {
|
||||
|
|
@ -1468,13 +1438,12 @@ async fn run_from_branch(
|
|||
cancel_token: None,
|
||||
dry_run: dry_run_mode,
|
||||
run_id,
|
||||
git_checkpoint: match sandbox_provider {
|
||||
SandboxProvider::Local | SandboxProvider::Docker => worktree_path
|
||||
git_checkpoint: if sandbox.is_remote() {
|
||||
Some(GitCheckpointMode::Remote(original_cwd.clone()))
|
||||
} else {
|
||||
worktree_path
|
||||
.as_ref()
|
||||
.map(|wt| GitCheckpointMode::Host(wt.clone())),
|
||||
SandboxProvider::Daytona | SandboxProvider::Exe => {
|
||||
Some(GitCheckpointMode::Remote(original_cwd.clone()))
|
||||
}
|
||||
.map(|wt| GitCheckpointMode::Host(wt.clone()))
|
||||
},
|
||||
base_sha,
|
||||
run_branch: Some(run_branch.to_string()),
|
||||
|
|
@ -1496,9 +1465,7 @@ async fn run_from_branch(
|
|||
|
||||
// Restore cwd (worktree is kept for `arc cp` access; pruned separately)
|
||||
let _ = std::env::set_current_dir(&original_cwd);
|
||||
if sandbox_provider == SandboxProvider::Exe {
|
||||
let _ = sandbox.cleanup().await;
|
||||
}
|
||||
let _ = sandbox.cleanup().await;
|
||||
|
||||
// Auto-derive retro
|
||||
if !args.no_retro {
|
||||
|
|
|
|||
|
|
@ -762,6 +762,18 @@ impl Sandbox for DaytonaSandbox {
|
|||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
fn is_remote(&self) -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
async fn ssh_access_command(&self) -> Result<Option<String>, String> {
|
||||
self.create_ssh_access().await.map(Some)
|
||||
}
|
||||
|
||||
fn origin_url(&self) -> Option<&str> {
|
||||
self.origin_url.get().map(String::as_str)
|
||||
}
|
||||
|
||||
async fn refresh_push_credentials(&self) -> Result<(), String> {
|
||||
let origin_url = match self.origin_url.get() {
|
||||
Some(url) => url,
|
||||
|
|
|
|||
|
|
@ -14,21 +14,12 @@ use crate::github_app::GitHubAppCredentials;
|
|||
/// without depending on `github_app` (which lives in `arc-workflows`).
|
||||
pub struct GitCredentialSandbox {
|
||||
inner: Arc<dyn Sandbox>,
|
||||
origin_url: Option<String>,
|
||||
github_app: Option<GitHubAppCredentials>,
|
||||
}
|
||||
|
||||
impl GitCredentialSandbox {
|
||||
pub fn new(
|
||||
inner: Arc<dyn Sandbox>,
|
||||
origin_url: Option<String>,
|
||||
github_app: Option<GitHubAppCredentials>,
|
||||
) -> Self {
|
||||
Self {
|
||||
inner,
|
||||
origin_url,
|
||||
github_app,
|
||||
}
|
||||
pub fn new(inner: Arc<dyn Sandbox>, github_app: Option<GitHubAppCredentials>) -> Self {
|
||||
Self { inner, github_app }
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -134,7 +125,7 @@ impl Sandbox for GitCredentialSandbox {
|
|||
}
|
||||
|
||||
async fn refresh_push_credentials(&self) -> Result<(), String> {
|
||||
let origin_url = match &self.origin_url {
|
||||
let origin_url = match self.inner.origin_url() {
|
||||
Some(url) => url,
|
||||
None => return Ok(()),
|
||||
};
|
||||
|
|
@ -170,4 +161,16 @@ impl Sandbox for GitCredentialSandbox {
|
|||
async fn set_autostop_interval(&self, minutes: i32) -> Result<(), String> {
|
||||
self.inner.set_autostop_interval(minutes).await
|
||||
}
|
||||
|
||||
fn is_remote(&self) -> bool {
|
||||
self.inner.is_remote()
|
||||
}
|
||||
|
||||
async fn ssh_access_command(&self) -> Result<Option<String>, String> {
|
||||
self.inner.ssh_access_command().await
|
||||
}
|
||||
|
||||
fn origin_url(&self) -> Option<&str> {
|
||||
self.inner.origin_url()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue