mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-05 08:10:39 +00:00
Add GitSyncStatus and WorkdirStrategy enums to fix cloud sandbox pre-run checks
The git sync check and auto-push logic was gated on should_create_worktree, which was always false for remote sandboxes. This meant Daytona/Exe/SSH runs silently proceeded without verifying commits were pushed or warning about uncommitted changes. Replace the git_clean boolean and should_create_worktree boolean with two enums (GitSyncStatus: Synced/Unsynced/Dirty and WorkdirStrategy: LocalDirectory/LocalWorktree/Cloud) so every combination is handled explicitly via match arms. Also display the base commit SHA for cloud runs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5b098d1882
commit
a5273e17a3
2 changed files with 135 additions and 43 deletions
|
|
@ -398,6 +398,17 @@ async fn mint_github_token(
|
||||||
Ok(token)
|
Ok(token)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// How the workflow run's working directory is set up.
|
||||||
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
|
enum WorkdirStrategy {
|
||||||
|
/// Run directly in the current working directory.
|
||||||
|
LocalDirectory,
|
||||||
|
/// Create a local git worktree for isolation.
|
||||||
|
LocalWorktree,
|
||||||
|
/// Remote sandbox clones from origin (Daytona, Exe, SSH).
|
||||||
|
Cloud,
|
||||||
|
}
|
||||||
|
|
||||||
/// Accumulates token usage and cost across all workflow stages.
|
/// Accumulates token usage and cost across all workflow stages.
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
struct CostAccumulator {
|
struct CostAccumulator {
|
||||||
|
|
@ -557,16 +568,8 @@ pub async fn run_command(
|
||||||
let (origin_url, detected_base_branch) = fabro_daytona::detect_repo_info(&original_cwd)
|
let (origin_url, detected_base_branch) = fabro_daytona::detect_repo_info(&original_cwd)
|
||||||
.map(|(url, branch)| (Some(url), branch))
|
.map(|(url, branch)| (Some(url), branch))
|
||||||
.unwrap_or((None, None));
|
.unwrap_or((None, None));
|
||||||
let git_clean = if sandbox_provider.is_remote() {
|
let git_status =
|
||||||
fabro_workflows::git::ensure_clean_and_pushed(
|
fabro_workflows::git::sync_status(&original_cwd, "origin", detected_base_branch.as_deref());
|
||||||
&original_cwd,
|
|
||||||
"origin",
|
|
||||||
detected_base_branch.as_deref(),
|
|
||||||
)
|
|
||||||
.is_ok()
|
|
||||||
} else {
|
|
||||||
fabro_workflows::git::ensure_clean(&original_cwd).is_ok()
|
|
||||||
};
|
|
||||||
|
|
||||||
if args.preflight {
|
if args.preflight {
|
||||||
return run_preflight(
|
return run_preflight(
|
||||||
|
|
@ -574,7 +577,7 @@ pub async fn run_command(
|
||||||
&run_cfg,
|
&run_cfg,
|
||||||
&args,
|
&args,
|
||||||
&run_defaults,
|
&run_defaults,
|
||||||
git_clean,
|
git_status,
|
||||||
sandbox_provider,
|
sandbox_provider,
|
||||||
styles,
|
styles,
|
||||||
github_app,
|
github_app,
|
||||||
|
|
@ -720,44 +723,84 @@ pub async fn run_command(
|
||||||
))
|
))
|
||||||
};
|
};
|
||||||
|
|
||||||
// Set up git worktree for local execution (must happen before cwd is captured).
|
// Determine the working directory strategy.
|
||||||
// Remote sandboxes (Daytona, Exe) clone into their own environment, so a local
|
// Remote sandboxes clone from origin; local runs may use a git worktree.
|
||||||
// worktree is unnecessary and wastes disk.
|
let workdir_strategy = if sandbox_provider.is_remote() {
|
||||||
let worktree_mode = resolve_worktree_mode(run_cfg.as_ref(), &run_defaults);
|
WorkdirStrategy::Cloud
|
||||||
let should_create_worktree = if sandbox_provider.is_remote() {
|
|
||||||
false
|
|
||||||
} else {
|
} else {
|
||||||
|
let worktree_mode = resolve_worktree_mode(run_cfg.as_ref(), &run_defaults);
|
||||||
match worktree_mode {
|
match worktree_mode {
|
||||||
sandbox_config::WorktreeMode::Always => true,
|
sandbox_config::WorktreeMode::Always => WorkdirStrategy::LocalWorktree,
|
||||||
sandbox_config::WorktreeMode::Clean => git_clean,
|
sandbox_config::WorktreeMode::Clean => {
|
||||||
sandbox_config::WorktreeMode::Dirty => !git_clean,
|
if git_status.is_clean() {
|
||||||
sandbox_config::WorktreeMode::Never => false,
|
WorkdirStrategy::LocalWorktree
|
||||||
|
} else {
|
||||||
|
WorkdirStrategy::LocalDirectory
|
||||||
|
}
|
||||||
|
}
|
||||||
|
sandbox_config::WorktreeMode::Dirty => {
|
||||||
|
if git_status.is_clean() {
|
||||||
|
WorkdirStrategy::LocalDirectory
|
||||||
|
} else {
|
||||||
|
WorkdirStrategy::LocalWorktree
|
||||||
|
}
|
||||||
|
}
|
||||||
|
sandbox_config::WorktreeMode::Never => WorkdirStrategy::LocalDirectory,
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
debug!(
|
debug!(
|
||||||
?worktree_mode,
|
?workdir_strategy,
|
||||||
?sandbox_provider,
|
?sandbox_provider,
|
||||||
git_clean,
|
?git_status,
|
||||||
should_create_worktree,
|
"Resolved workdir strategy"
|
||||||
"Resolved worktree mode"
|
|
||||||
);
|
);
|
||||||
|
|
||||||
if should_create_worktree && !git_clean {
|
// Warn about uncommitted changes that won't be available in the execution environment.
|
||||||
eprintln!(
|
if git_status == fabro_workflows::git::GitSyncStatus::Dirty {
|
||||||
"{} Uncommitted changes will not be included in the worktree.",
|
match workdir_strategy {
|
||||||
styles.yellow.apply_to("Warning:"),
|
WorkdirStrategy::LocalWorktree => {
|
||||||
);
|
eprintln!(
|
||||||
|
"{} Uncommitted changes will not be included in the worktree.",
|
||||||
|
styles.yellow.apply_to("Warning:"),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
WorkdirStrategy::Cloud => {
|
||||||
|
eprintln!(
|
||||||
|
"{} Uncommitted changes will not be included in the remote sandbox.",
|
||||||
|
styles.yellow.apply_to("Warning:"),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
WorkdirStrategy::LocalDirectory => {}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if should_create_worktree && !args.dry_run {
|
// Auto-push when the execution environment needs commits on the remote.
|
||||||
|
if !args.dry_run
|
||||||
|
&& matches!(
|
||||||
|
workdir_strategy,
|
||||||
|
WorkdirStrategy::LocalWorktree | WorkdirStrategy::Cloud
|
||||||
|
)
|
||||||
|
{
|
||||||
if let Some(ref branch) = detected_base_branch {
|
if let Some(ref branch) = detected_base_branch {
|
||||||
let check_repo = original_cwd.clone();
|
// For Synced we know no push is needed; for Unsynced we know it is;
|
||||||
let check_branch = branch.clone();
|
// for Dirty the push status wasn't checked, so check now.
|
||||||
let needs_push = tokio::task::spawn_blocking(move || {
|
let needs_push = match git_status {
|
||||||
fabro_workflows::git::branch_needs_push(&check_repo, "origin", &check_branch)
|
fabro_workflows::git::GitSyncStatus::Synced => false,
|
||||||
})
|
fabro_workflows::git::GitSyncStatus::Unsynced => true,
|
||||||
.await
|
fabro_workflows::git::GitSyncStatus::Dirty => {
|
||||||
.unwrap_or(true);
|
let check_repo = original_cwd.clone();
|
||||||
|
let check_branch = branch.clone();
|
||||||
|
tokio::task::spawn_blocking(move || {
|
||||||
|
fabro_workflows::git::branch_needs_push(
|
||||||
|
&check_repo,
|
||||||
|
"origin",
|
||||||
|
&check_branch,
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.unwrap_or(true)
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
if needs_push {
|
if needs_push {
|
||||||
let repo_path = original_cwd.clone();
|
let repo_path = original_cwd.clone();
|
||||||
|
|
@ -788,8 +831,9 @@ pub async fn run_command(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Set up git worktree for local isolation.
|
||||||
let (worktree_work_dir, worktree_path, worktree_branch, worktree_base_sha) =
|
let (worktree_work_dir, worktree_path, worktree_branch, worktree_base_sha) =
|
||||||
if should_create_worktree {
|
if workdir_strategy == WorkdirStrategy::LocalWorktree {
|
||||||
match setup_worktree(&original_cwd, &run_dir, &run_id) {
|
match setup_worktree(&original_cwd, &run_dir, &run_id) {
|
||||||
Ok((wd, wt, branch, base)) => (Some(wd), Some(wt), Some(branch), Some(base)),
|
Ok((wd, wt, branch, base)) => (Some(wd), Some(wt), Some(branch), Some(base)),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
|
|
@ -811,7 +855,15 @@ pub async fn run_command(
|
||||||
.show_worktree(wt);
|
.show_worktree(wt);
|
||||||
}
|
}
|
||||||
|
|
||||||
if let Some(ref sha) = worktree_base_sha {
|
// Show base SHA for both worktree and cloud strategies.
|
||||||
|
let base_sha_display = worktree_base_sha.clone().or_else(|| {
|
||||||
|
if workdir_strategy == WorkdirStrategy::Cloud {
|
||||||
|
fabro_workflows::git::head_sha(&original_cwd).ok()
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
});
|
||||||
|
if let Some(ref sha) = base_sha_display {
|
||||||
progress_ui
|
progress_ui
|
||||||
.lock()
|
.lock()
|
||||||
.expect("progress lock poisoned")
|
.expect("progress lock poisoned")
|
||||||
|
|
@ -2040,7 +2092,7 @@ async fn run_preflight(
|
||||||
run_cfg: &Option<run_config::WorkflowRunConfig>,
|
run_cfg: &Option<run_config::WorkflowRunConfig>,
|
||||||
args: &RunArgs,
|
args: &RunArgs,
|
||||||
run_defaults: &RunDefaults,
|
run_defaults: &RunDefaults,
|
||||||
git_clean: bool,
|
git_status: fabro_workflows::git::GitSyncStatus,
|
||||||
sandbox_provider: SandboxProvider,
|
sandbox_provider: SandboxProvider,
|
||||||
styles: &'static Styles,
|
styles: &'static Styles,
|
||||||
github_app: Option<fabro_github::GitHubAppCredentials>,
|
github_app: Option<fabro_github::GitHubAppCredentials>,
|
||||||
|
|
@ -2083,8 +2135,8 @@ async fn run_preflight(
|
||||||
details: vec![
|
details: vec![
|
||||||
CheckDetail::new(format!("Setup commands: {setup_command_count}")),
|
CheckDetail::new(format!("Setup commands: {setup_command_count}")),
|
||||||
CheckDetail {
|
CheckDetail {
|
||||||
text: format!("Git clean: {git_clean}"),
|
text: format!("Git: {git_status}"),
|
||||||
warn: !git_clean,
|
warn: git_status != fabro_workflows::git::GitSyncStatus::Synced,
|
||||||
},
|
},
|
||||||
],
|
],
|
||||||
remediation: None,
|
remediation: None,
|
||||||
|
|
|
||||||
|
|
@ -278,6 +278,46 @@ pub fn ensure_clean_and_pushed(repo: &Path, remote: &str, branch: Option<&str>)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Tri-state summary of the local repository's readiness for a workflow run.
|
||||||
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||||
|
pub enum GitSyncStatus {
|
||||||
|
/// Working tree is clean and the branch is pushed to the remote.
|
||||||
|
Synced,
|
||||||
|
/// Working tree is clean but the branch has unpushed commits
|
||||||
|
/// (or push status could not be verified, e.g. detached HEAD).
|
||||||
|
Unsynced,
|
||||||
|
/// Working tree has uncommitted changes.
|
||||||
|
Dirty,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl GitSyncStatus {
|
||||||
|
/// Whether the working tree has no uncommitted changes.
|
||||||
|
pub fn is_clean(&self) -> bool {
|
||||||
|
matches!(self, Self::Synced | Self::Unsynced)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
impl std::fmt::Display for GitSyncStatus {
|
||||||
|
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||||
|
match self {
|
||||||
|
Self::Synced => write!(f, "synced"),
|
||||||
|
Self::Unsynced => write!(f, "unsynced (unpushed commits)"),
|
||||||
|
Self::Dirty => write!(f, "dirty (uncommitted changes)"),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Determine the sync status of the repository relative to a remote.
|
||||||
|
pub fn sync_status(repo: &Path, remote: &str, branch: Option<&str>) -> GitSyncStatus {
|
||||||
|
if ensure_clean(repo).is_err() {
|
||||||
|
return GitSyncStatus::Dirty;
|
||||||
|
}
|
||||||
|
match branch {
|
||||||
|
Some(b) if !branch_needs_push(repo, remote, b) => GitSyncStatus::Synced,
|
||||||
|
_ => GitSyncStatus::Unsynced,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Sanitize a string for use as a git ref component.
|
/// Sanitize a string for use as a git ref component.
|
||||||
/// Lowercases, replaces non-alphanumeric chars with dashes, collapses runs.
|
/// Lowercases, replaces non-alphanumeric chars with dashes, collapses runs.
|
||||||
pub fn sanitize_ref_component(s: &str) -> String {
|
pub fn sanitize_ref_component(s: &str) -> String {
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue