diff --git a/Cargo.lock b/Cargo.lock index dca7a053e..1f8141636 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3043,6 +3043,7 @@ dependencies = [ "rand 0.9.4", "regex", "reqwest 0.12.28", + "sandbox-driver", "semver", "serde", "serde_json", diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 75d60fb0b..bd6e1cc7f 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14977,9 +14977,10 @@ components: type: boolean default: false description: | - When true, Fabro-managed run-branch checkpoint commits bypass - local Git commit hooks. Does not affect Fabro `[[run.hooks]]` - or metadata-branch snapshots. Defaults to false. + Accepted for compatibility. Fabro-managed run-branch checkpoint + commits never run local Git commit hooks: the sandbox driver + disables repository hooks on every git command it runs. Does not + affect Fabro `[[run.hooks]]`. Defaults to false. RunCloneSettings: type: object diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 587c92d8e..1a617e977 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -428,8 +428,8 @@ commit_timeout = "30s" | Field | Description | |---|---| | `exclude_globs` | Glob patterns for files to exclude from checkpoint commits. Uses git pathspec `:(glob,exclude)` syntax. | -| `skip_git_hooks` | When `true`, Fabro-managed run-branch checkpoint commits bypass local Git commit hooks (e.g. `pre-commit`, `commit-msg`). Defaults to `false`. Does not affect Fabro workflow `[[run.hooks]]` or metadata-branch snapshots. | -| `commit_timeout` | Max duration for the per-node run-branch checkpoint commit (e.g. `"30s"`, `"10m"`). This commit runs repository commit hooks unless `skip_git_hooks` is `true`. Defaults to `"30s"`. | +| `skip_git_hooks` | Accepted for compatibility. Fabro-managed run-branch checkpoint commits never run local Git commit hooks (e.g. `pre-commit`, `commit-msg`); the sandbox driver disables repository hooks on every git command it runs. Does not affect Fabro workflow `[[run.hooks]]`. | +| `commit_timeout` | Accepted for compatibility. The per-node run-branch checkpoint commit runs under the sandbox driver's git command budget; no repository hook can prolong it. | `exclude_globs` replaces across layers — the higher-precedence layer wins wholesale. `skip_git_hooks` and `commit_timeout` use normal override semantics: the highest layer that sets the field wins. diff --git a/lib/apps/fabro-server/Cargo.toml b/lib/apps/fabro-server/Cargo.toml index 0371ee8c0..8e34b51f8 100644 --- a/lib/apps/fabro-server/Cargo.toml +++ b/lib/apps/fabro-server/Cargo.toml @@ -35,6 +35,7 @@ fabro-workflow = { path = "../../components/fabro-workflow" } fabro-workflow-version = { path = "../../components/fabro-workflow-version" } fabro-validate = { path = "../../components/fabro-validate" } fabro-sandbox = { path = "../../components/fabro-sandbox" } +sandbox-driver.workspace = true fabro-github = { path = "../../components/fabro-github" } fabro-agent = { path = "../../components/fabro-agent" } fabro-llm = { path = "../../components/fabro-llm" } diff --git a/lib/apps/fabro-server/src/run_files.rs b/lib/apps/fabro-server/src/run_files.rs index 4387a3017..ab93b4d6d 100644 --- a/lib/apps/fabro-server/src/run_files.rs +++ b/lib/apps/fabro-server/src/run_files.rs @@ -20,7 +20,7 @@ use std::future::Future; use std::num::NonZeroU64; use std::panic::AssertUnwindSafe; use std::sync::Arc; -use std::time::Instant; +use std::time::{Duration, Instant}; use axum::Json; use axum::extract::{Path, Query, State}; @@ -43,6 +43,9 @@ use fabro_workflow::sandbox_git::{ list_diff_numstat, stream_blob_metadata, stream_blobs, }; use futures_util::FutureExt; +use sandbox_driver::{ + Git as _, GitCommit, GitDiffOptions, GitFacet, GitLogOptions, GitRevisionRange, +}; use serde::Deserialize; use tokio::sync::{Mutex, watch}; @@ -60,6 +63,7 @@ pub(crate) const AGGREGATE_BYTES_CAP: u64 = 5 * 1024 * 1024; pub(crate) const FILE_COUNT_CAP: usize = 200; /// Sandbox git timeout. Matches Unit 3 helpers (10 s). const SANDBOX_GIT_TIMEOUT_MS: u64 = 10_000; +const SANDBOX_GIT_TIMEOUT: Duration = Duration::from_millis(SANDBOX_GIT_TIMEOUT_MS); /// Below this SHA count the phase-1 `cat-file --batch-check` pre-filter is /// skipped — its ~100 ms round-trip dominates for small diffs, and phase-2 @@ -333,8 +337,7 @@ async fn materialize_run_commits( .ok_or_else(|| ApiError::new(StatusCode::CONFLICT, "Run has no base SHA."))?; let sandbox = reconnect_run_sandbox(state, run_id, &projection).await?; let (head_sha, _) = resolve_ref_sha_and_time(&sandbox, "HEAD").await?; - let output = git_log_commits(&sandbox, &base_sha, &head_sha, limit + 1).await?; - let mut commits = parse_git_log_commits(&output)?; + let mut commits = git_log_commits(&sandbox, &base_sha, &head_sha, limit + 1).await?; let truncated = commits.len() > usize::try_from(limit).unwrap_or(usize::MAX); commits.truncate(usize::try_from(limit).unwrap_or(usize::MAX)); let total_returned = u64::try_from(commits.len()).unwrap_or(u64::MAX); @@ -357,57 +360,26 @@ async fn git_log_commits( base_sha: &str, head_sha: &str, limit: u64, -) -> std::result::Result { - let base_q = shell_quote(base_sha); - let head_q = shell_quote(head_sha); - let format_q = - shell_quote("%H%x1f%T%x1f%P%x1f%an%x1f%ae%x1f%aI%x1f%cn%x1f%ce%x1f%cI%x1f%B%x1e"); - sandbox_git_stdout( - sandbox, - &format!( - "git -c maintenance.auto=0 -c gc.auto=0 -c core.hooksPath=/dev/null -c core.fsmonitor=false -c core.quotePath=false log --first-parent --reverse --max-count={limit} --format={format_q} {base_q}..{head_q}" - ), - "git log", - ) - .await +) -> std::result::Result, ApiError> { + let git = sandbox_git(sandbox)?; + let options = GitLogOptions::new(GitRevisionRange::new(base_sha).to(head_sha)) + .first_parent() + .reverse() + .max_count(limit) + .timeout(SANDBOX_GIT_TIMEOUT); + let commits = git + .log(sandbox.working_directory(), &options) + .await + .map_err(|error| sandbox_git_error("git log", &error))?; + commits.iter().map(run_commit).collect() } -fn parse_git_log_commits(stdout: &str) -> std::result::Result, ApiError> { - stdout - .split('\x1e') - .filter_map(|record| { - let record = record.trim_matches('\n'); - (!record.is_empty()).then_some(record) - }) - .map(parse_git_log_commit) - .collect() -} - -fn parse_git_log_commit(record: &str) -> std::result::Result { - let mut fields = record.splitn(10, '\x1f'); - let sha = fields.next().unwrap_or_default(); - let tree_sha = fields.next().unwrap_or_default(); - let parents = fields.next().unwrap_or_default(); - let author_name = fields.next().unwrap_or_default(); - let author_email = fields.next().unwrap_or_default(); - let author_date = fields.next().unwrap_or_default(); - let committer_name = fields.next().unwrap_or_default(); - let committer_email = fields.next().unwrap_or_default(); - let committer_date = fields.next().unwrap_or_default(); - let message = fields - .next() - .unwrap_or_default() - .trim_end_matches('\n') - .to_string(); - if sha.is_empty() { - return Err(ApiError::bad_request( - "Malformed git log output: missing commit SHA.", - )); - } - +fn run_commit(commit: &GitCommit) -> std::result::Result { + let message = commit.message.trim_end_matches('\n').to_string(); let (subject, body) = split_commit_message(&message); - let parents = parents - .split_whitespace() + let parents = commit + .parents + .iter() .map(|parent| { Ok(RunCommitParent { sha: sha_newtype::(parent)?, @@ -417,27 +389,27 @@ fn parse_git_log_commit(record: &str) -> std::result::Result, ApiError>>()?; Ok(RunCommit { - sha: sha_newtype::(sha)?, - short_sha: short_sha_newtype::(sha)?, + sha: sha_newtype::(&commit.sha)?, + short_sha: short_sha_newtype::(&commit.sha)?, parents, author: RunCommitPerson { - name: author_name.to_string(), - email: author_email.to_string(), - date: parse_git_date(author_date), + name: commit.author.name.clone(), + email: commit.author.email.clone(), + date: parse_git_date(&commit.author.date), }, committer: RunCommitPerson { - name: committer_name.to_string(), - email: committer_email.to_string(), - date: parse_git_date(committer_date), + name: commit.committer.name.clone(), + email: commit.committer.email.clone(), + date: parse_git_date(&commit.committer.date), }, subject, body, message: message.clone(), trailers: parse_commit_trailers(&message), - tree_sha: if tree_sha.is_empty() { + tree_sha: if commit.tree.is_empty() { None } else { - Some(sha_newtype::(tree_sha)?) + Some(sha_newtype::(&commit.tree)?) }, }) } @@ -733,15 +705,15 @@ async fn materialize_working_tree_sandbox_path( start: Instant, ) -> ListRunFilesResult { let (to_sha, to_sha_committed_at) = resolve_head_sha_and_time(sandbox).await?; - let base_q = shell_quote(base_ref); - let patch = sandbox_git_stdout( - sandbox, - &format!( - "git -c maintenance.auto=0 -c gc.auto=0 -c core.hooksPath=/dev/null -c core.fsmonitor=false -c core.quotePath=false diff --patch --find-renames=50% {base_q}" - ), - "git diff --patch", - ) - .await?; + let git = sandbox_git(sandbox)?; + // No head: the driver diffs `base_ref` against the working tree. + let options = GitDiffOptions::new(GitRevisionRange::new(base_ref)) + .find_renames(50) + .timeout(SANDBOX_GIT_TIMEOUT); + let patch = git + .diff_patch(sandbox.working_directory(), &options) + .await + .map_err(|error| sandbox_git_error("git diff --patch", &error))?; let entries: Vec = split_patch_sections(&patch) .into_iter() @@ -763,22 +735,25 @@ async fn materialize_working_tree_sandbox_path( )) } -async fn sandbox_git_stdout( - sandbox: &RunSandbox, - command: &str, - op: &str, -) -> std::result::Result { - let res = sandbox - .exec_command(command, SANDBOX_GIT_TIMEOUT_MS, None, None, None) - .await - .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.display_with_causes()))?; - if res.termination == Termination::TimedOut { - return Err(transient_503(op, "command timed out")); +/// The sandbox's git facet; a provider without git cannot serve files. +fn sandbox_git(sandbox: &RunSandbox) -> std::result::Result, ApiError> { + sandbox + .git() + .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.display_with_causes())) +} + +/// A driver git failure as the endpoint's transient 503, so the client +/// retries; a command that timed out says so. +fn sandbox_git_error(op: &str, error: &sandbox_driver::Error) -> ApiError { + let timed_out = matches!( + error, + sandbox_driver::Error::Git(failure) + if failure.output().is_some_and(|output| output.termination() == Termination::TimedOut) + ); + if timed_out { + return transient_503(op, "command timed out"); } - if !res.success() { - return Err(transient_503(op, res.stderr_lossy().trim())); - } - Ok(res.stdout_lossy()) + transient_503(op, &fabro_sandbox::display_for_log(error)) } /// Build the degraded response from the stored terminal diff patch. @@ -1752,7 +1727,7 @@ mod tests { sandbox.respond_with(|command| { let stdout = if command.contains(" show -s --format=") { "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb 2026-05-09T17:12:40Z\n".to_string() - } else if command.contains(" diff --patch --find-renames=50% ") { + } else if command.contains("'diff'") && command.contains("'--find-renames=50%'") { "\ diff --git a/src/live.rs b/src/live.rs --- a/src/live.rs @@ -1784,12 +1759,18 @@ diff --git a/src/live.rs b/src/live.rs let commands = sandbox.driver().scripted_exec().commands(); assert_eq!(commands.len(), 2); assert!(commands[0].contains(" show -s --format=")); - assert!(commands[1].contains(" diff --patch --find-renames=50% HEAD")); + assert!( + commands[1].contains("'diff'") + && commands[1].contains("'--find-renames=50%'") + && commands[1].contains("'HEAD'"), + "{}", + commands[1] + ); assert!(!commands.iter().any(|command| command.contains("ls-files"))); } - #[test] - fn parse_git_log_commits_keeps_external_and_fabro_metadata() { + #[tokio::test] + async fn git_log_commits_keeps_external_and_fabro_metadata() { let stdout = concat!( "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\x1f", "cccccccccccccccccccccccccccccccccccccccc\x1f", @@ -1804,8 +1785,26 @@ diff --git a/src/live.rs b/src/live.rs "Alice\x1falice@example.com\x1f2026-05-09T18:00:00Z\x1f", "external tool update\n\nLonger body.\n\x1e", ); + let sandbox = fabro_sandbox::test_support::MockSandbox::default(); + sandbox + .driver() + .scripted_exec() + .push_result(fabro_sandbox::test_support::exec_result( + stdout, + "", + Some(0), + Termination::Exited, + 1, + )); - let commits = parse_git_log_commits(stdout).expect("git log should parse"); + let commits = git_log_commits( + &sandbox.sandbox(), + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "dddddddddddddddddddddddddddddddddddddddd", + 50, + ) + .await + .expect("git log should parse"); assert_eq!(commits.len(), 2); assert_eq!(commits[0].subject, "fabro(run_1): implement (succeeded)"); @@ -1818,6 +1817,11 @@ diff --git a/src/live.rs b/src/live.rs assert_eq!(commits[1].subject, "external tool update"); assert_eq!(commits[1].body.as_deref(), Some("Longer body.")); assert!(commits[1].trailers.is_empty()); + let command = &sandbox.driver().scripted_exec().commands()[0]; + assert!( + command.contains("'--first-parent'") && command.contains("'--max-count=50'"), + "{command}" + ); } #[tokio::test] diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 6d21ccbca..263534c6b 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -472,9 +472,11 @@ impl RunSandbox { } } - /// The driver's git facet for this sandbox's checkout. Absent until a - /// pending sandbox is initialized, or when the provider has no git. - pub(crate) fn git(&self) -> crate::Result> { + /// The driver's git facet for this sandbox's checkout, for fabro's own + /// git operations (checkpoints, diffs, the Run Files listing). Absent + /// until a pending sandbox is initialized, or when the provider has no + /// git. Pass [`Self::working_directory`] as the repository path. + pub fn git(&self) -> crate::Result> { self.handle()?.git().ok_or_else(|| { crate::Error::message(format!( "sandbox provider `{}` does not support git", diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index e1633832f..083f8e039 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -5,20 +5,17 @@ use chrono::{DateTime, Utc}; use fabro_github::token_source::TokenSnapshot; use fabro_util::shell; use sandbox_driver::{ - Git as _, GitAttempt, GitCheckoutOptions, GitPushOptions, GitRetryError, GitRetryPolicy, - retry_git, + Git as _, GitAttempt, GitCheckoutOptions, GitFetchOptions, GitPushOptions, GitRetryError, + GitRetryPolicy, retry_git, }; use serde::{Deserialize, Serialize}; use tokio::time; use crate::credentials::{self, RepoCredentials}; use crate::driver_sandbox::RunSandbox; -use crate::exec::ExecResultExt; use crate::git_policy::{self, GitRetryReason}; /// Git command prefix that disables background maintenance. -pub(crate) const GIT: &str = "git -c maintenance.auto=0 -c gc.auto=0"; - pub const DEFAULT_EXEC_OUTPUT_TAIL_BYTES: usize = 8 * 1024; /// Where a clone-based sandbox put its files, as persisted on the run. @@ -245,38 +242,27 @@ pub(crate) async fn fetch_source_run_ref( ) -> crate::Result<()> { let remote_ref = format!("refs/heads/fabro/run/{source_run_id}"); let tracking_ref = format!("refs/remotes/origin/fabro/run/{source_run_id}"); - let fetch_cmd = format!( - "{GIT} fetch origin {}:{}", - shell_quote(&remote_ref), - shell_quote(&tracking_ref) - ); - let check_cmd = format!( - "{GIT} merge-base --is-ancestor {} {}", - shell_quote(checkpoint_sha), - shell_quote(&tracking_ref) - ); + let git = sandbox.git()?; + let repo = sandbox.working_directory(); + let mut fetch = GitFetchOptions::default(); + fetch.remote = Some("origin".to_owned()); + fetch.refspecs = vec![format!("{remote_ref}:{tracking_ref}")]; + fetch.timeout = Some(Duration::from_secs(30)); + // The source run's checkpoint may still be landing on the remote; a + // few short retries cover the replication. let mut last_error = String::new(); for _ in 0..5 { - let fetch = sandbox - .exec_command(&fetch_cmd, 30_000, None, None, None) - .await?; - if fetch.success() { - let check = sandbox - .exec_command(&check_cmd, 10_000, None, None, None) - .await?; - if check.success() { - return Ok(()); - } - last_error = check - .into_exec_error(format!( - "checkpoint {checkpoint_sha} is not reachable from {remote_ref}" - )) - .to_string(); - } else { - last_error = fetch - .into_exec_error("git fetch source run ref") - .to_string(); + match git.fetch(repo, &fetch).await { + Ok(()) => match git.is_ancestor(repo, checkpoint_sha, &tracking_ref).await { + Ok(true) => return Ok(()), + Ok(false) => { + last_error = + format!("checkpoint {checkpoint_sha} is not reachable from {remote_ref}"); + } + Err(error) => last_error = format!("git merge-base --is-ancestor: {error}"), + }, + Err(error) => last_error = format!("git fetch source run ref: {error}"), } time::sleep(Duration::from_millis(500)).await; } diff --git a/lib/components/fabro-workflow/src/handler/llm/changed_files.rs b/lib/components/fabro-workflow/src/handler/llm/changed_files.rs index e0526648e..51c003efd 100644 --- a/lib/components/fabro-workflow/src/handler/llm/changed_files.rs +++ b/lib/components/fabro-workflow/src/handler/llm/changed_files.rs @@ -2,25 +2,26 @@ use std::collections::HashSet; use std::sync::Arc; use fabro_agent::{RunSandbox, shell_quote}; +use sandbox_driver::{Git as _, GitDiffOptions, GitRevisionRange}; -const DIFF_MARKER: &str = "__FABRO_CHANGED_FILES_DIFF__"; -const UNTRACKED_MARKER: &str = "__FABRO_CHANGED_FILES_UNTRACKED__"; - +/// The paths the working tree changed against `HEAD`, plus the untracked +/// files git does not ignore, sorted and deduplicated. A sandbox without +/// git, or a working directory that is not a repository, has no changed +/// files. pub async fn detect_changed_files(sandbox: &Arc) -> Vec { + let Ok(git) = sandbox.git() else { + return Vec::new(); + }; + let repo = sandbox.working_directory(); let mut files: Vec = Vec::new(); - let command = format!( - "printf '%s\\n' {diff}; git diff --name-only || true; \ - printf '%s\\n' {untracked}; git ls-files --others --exclude-standard || true", - diff = shell_quote(DIFF_MARKER), - untracked = shell_quote(UNTRACKED_MARKER), - ); - if let Ok(result) = sandbox - .exec_command(&command, 30_000, None, None, None) + if let Ok(entries) = git + .diff_entries(repo, &GitDiffOptions::new(GitRevisionRange::new("HEAD"))) .await { - if result.success() { - files.extend(parse_changed_files(&result.stdout_lossy())); - } + files.extend(entries.into_iter().map(|entry| entry.path)); + } + if let Ok(untracked) = git.untracked_files(repo).await { + files.extend(untracked); } files.sort(); @@ -57,27 +58,3 @@ pub async fn files_touched_since( (files_touched, last_file_touched) } - -fn parse_changed_files(stdout: &str) -> impl Iterator + '_ { - stdout.lines().filter_map(|line| { - let trimmed = line.trim(); - (!trimmed.is_empty() && trimmed != DIFF_MARKER && trimmed != UNTRACKED_MARKER) - .then(|| trimmed.to_string()) - }) -} - -#[cfg(test)] -mod tests { - use super::parse_changed_files; - - #[test] - fn parse_changed_files_ignores_section_markers() { - let files = parse_changed_files( - "__FABRO_CHANGED_FILES_DIFF__\nsrc/main.rs\n\ - __FABRO_CHANGED_FILES_UNTRACKED__\nREADME.md\n", - ) - .collect::>(); - - assert_eq!(files, vec!["src/main.rs", "README.md"]); - } -} diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index d7101a5b2..4688f4721 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -12,13 +12,13 @@ use fabro_llm::credentials::{CredentialProvider, readiness}; use fabro_llm::lithos_catalog::Catalog; use fabro_sandbox::{ DaytonaCredentials, ExecResultExt, GitSetupIntent, ProviderAccess, SandboxSpec, - reconnect_for_run_with_events, shell_quote, + reconnect_for_run_with_events, }; use fabro_static::EnvVars; use fabro_types::RunSandboxKind; use fabro_util::time::elapsed_ms; use fabro_vault::Vault; -use sandbox_driver::{CorrelationId, EventContext}; +use sandbox_driver::{CorrelationId, EventContext, Git as _}; use tokio::runtime::Handle; use tokio::sync::RwLock as AsyncRwLock; @@ -79,18 +79,15 @@ async fn configure_sandbox_git_identity( sandbox: &RunSandbox, author: &GitAuthor, ) -> Result<(), Error> { - let command = format!( - "git config --local user.name {} && git config --local user.email {}", - shell_quote(&author.name), - shell_quote(&author.email) - ); - sandbox - .exec_command(&command, 10_000, None, None, None) - .await - .map_err(|err| Error::engine_with_source("Sandbox git identity setup failed", err))? - .into_result("git config user identity") + let git = sandbox + .git() .map_err(|err| Error::engine_with_source("Sandbox git identity setup failed", err))?; - + let repo = sandbox.working_directory(); + for (key, value) in [("user.name", &author.name), ("user.email", &author.email)] { + git.config_set(repo, key, value) + .await + .map_err(|err| Error::engine_with_source("Sandbox git identity setup failed", err))?; + } Ok(()) } @@ -1071,10 +1068,17 @@ mod tests { .expect("git identity should configure"); let commands = sandbox.driver().scripted_exec().commands(); - assert_eq!(commands, vec![ - "git config --local user.name 'Fabro Bot' && git config --local user.email \ - fabro-bot@example.com" - ]); + assert_eq!(commands.len(), 2, "{commands:#?}"); + assert!( + commands[0].contains("'config' '--local' '--' 'user.name' 'Fabro Bot'"), + "{}", + commands[0] + ); + assert!( + commands[1].contains("'config' '--local' '--' 'user.email' 'fabro-bot@example.com'"), + "{}", + commands[1] + ); } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/sandbox_git.rs b/lib/components/fabro-workflow/src/sandbox_git.rs index 0b86b0161..c4b04ae24 100644 --- a/lib/components/fabro-workflow/src/sandbox_git.rs +++ b/lib/components/fabro-workflow/src/sandbox_git.rs @@ -1,11 +1,23 @@ +//! Fabro's git operations on a run's sandbox, over the driver's git facet. +//! +//! The driver runs every command hardened (no auto maintenance or gc, no +//! repository hooks, no fsmonitor, unquoted paths, no signing; read verbs +//! refuse the file transport and external diff drivers) and returns typed +//! results. Fabro decides what to stage, what to say in a checkpoint +//! commit, and which ranges the Run Files endpoint reads. + use std::collections::{HashMap, HashSet}; +use std::time::Duration; use fabro_agent::RunSandbox; use fabro_checkpoint::trailer as trailerlink; use fabro_checkpoint::trailer::Trailer; -use fabro_sandbox::{ExecResult, ExecResultExt, Termination, shell_quote}; use fabro_types::settings::run::RunCheckpointSettings; use fabro_util::error::SharedError; +use sandbox_driver::{ + Git as _, GitChange, GitCommitOptions, GitDiffEntry, GitDiffOptions, GitFacet, GitFailureKind, + GitRevisionRange, +}; use crate::artifact_snapshot; use crate::git::GitAuthor; @@ -19,32 +31,33 @@ pub struct GitCommandError { pub source: fabro_sandbox::Error, } -pub const GIT_REMOTE: &str = - "git -c maintenance.auto=0 -c gc.auto=0 -c commit.gpgsign=false -c tag.gpgsign=false"; +/// Rename detection threshold for the diffs the Run Files endpoint and the +/// checkpoint summaries read. +const FIND_RENAMES_PERCENT: u8 = 50; -pub(crate) fn exec_err(label: &str, r: ExecResult) -> GitCommandError { - let duration_ms = r.duration_ms(); - let message = match r.termination { - Termination::TimedOut => format!("{label} timed out after {duration_ms}ms"), - Termination::Cancelled | Termination::Killed => { - format!("{label} cancelled after {duration_ms}ms") - } - _ => format!( - "{label} failed (exit {})", - r.program_exit_code().unwrap_or(-1) - ), - }; +/// Budget for the machine-readable diffs behind the Run Files endpoint. +const RUN_FILES_TIMEOUT: Duration = Duration::from_secs(10); + +/// The sandbox's git facet, or the error a git operation reports when the +/// provider has none. +fn facet<'a>(sandbox: &'a RunSandbox, label: &str) -> Result, GitCommandError> { + sandbox.git().map_err(|source| GitCommandError { + message: format!("{label} failed"), + source, + }) +} + +fn git_error(label: &str, error: sandbox_driver::Error) -> GitCommandError { GitCommandError { - message, - source: r.into_exec_error(label), + message: format!("{label} failed"), + source: fabro_sandbox::Error::from(error), } } -/// Run a git checkpoint commit via the sandbox. -#[allow( - clippy::too_many_arguments, - reason = "Checkpointing needs explicit run metadata, checkpoint settings, and author inputs." -)] +/// Commit the run's checkpoint: everything under the working directory +/// except the built-in and configured excludes, as an allow-empty commit +/// carrying fabro's trailers. Repository hooks never run: the driver +/// disables them on every command it issues. pub async fn git_checkpoint( sandbox: &RunSandbox, run_id: &str, @@ -55,30 +68,24 @@ pub async fn git_checkpoint( checkpoint: &RunCheckpointSettings, author: &GitAuthor, ) -> std::result::Result { - let mut all_excludes: Vec = artifact_snapshot::EXCLUDE_DIRS - .iter() - .map(|d| format!("**/{d}/**")) - .collect(); - all_excludes.extend(checkpoint.exclude_globs.iter().cloned()); + let git = facet(sandbox, "git add")?; + let repo = sandbox.working_directory(); - let pathspecs: Vec = all_excludes - .iter() - .map(|g| format!("':(glob,exclude){g}'")) - .collect(); - let add_cmd = format!("{GIT_REMOTE} add -A -- . {}", pathspecs.join(" ")); - let add_result = sandbox - .exec_command(&add_cmd, checkpoint.commit_timeout_ms, None, None, None) - .await; - match add_result { - Ok(r) if r.success() => {} - Ok(r) => return Err(exec_err("git add", r)), - Err(e) => { - return Err(GitCommandError { - message: "git add failed".to_string(), - source: e, - }); - } - } + let mut pathspecs = vec![".".to_owned()]; + pathspecs.extend( + artifact_snapshot::EXCLUDE_DIRS + .iter() + .map(|dir| format!(":(glob,exclude)**/{dir}/**")), + ); + pathspecs.extend( + checkpoint + .exclude_globs + .iter() + .map(|glob| format!(":(glob,exclude){glob}")), + ); + git.add_all(repo, &pathspecs) + .await + .map_err(|error| git_error("git add", error))?; let subject = format!("fabro({run_id}): {node_id} ({status})"); let completed_str = completed_count.to_string(); @@ -102,52 +109,11 @@ pub async fn git_checkpoint( let mut message = trailerlink::format_message(&subject, "", &trailers); author.append_footer(&mut message); - let msg_path = format!("/tmp/fabro-commit-msg-{}", uuid::Uuid::new_v4()); - if let Err(e) = sandbox.write_file(&msg_path, &message).await { - return Err(GitCommandError { - message: "failed to write commit message file".to_string(), - source: e, - }); - } - - let msg_path_q = shell_quote(&msg_path); - let no_verify = if checkpoint.skip_git_hooks { - " --no-verify" - } else { - "" - }; - let commit_cmd = format!( - "{GIT_REMOTE} -c user.name={name} -c user.email={email} commit --allow-empty{no_verify} -F {msg_path_q}", - name = shell_quote(&author.name), - email = shell_quote(&author.email), - ); - let commit_result = sandbox - .exec_command(&commit_cmd, checkpoint.commit_timeout_ms, None, None, None) - .await; - let _ = sandbox.delete_file(&msg_path).await; - match commit_result { - Ok(r) if r.success() => {} - Ok(r) => return Err(exec_err("git commit", r)), - Err(e) => { - return Err(GitCommandError { - message: "git commit failed".to_string(), - source: e, - }); - } - } - - let sha_cmd = format!("{GIT_REMOTE} rev-parse HEAD"); - let sha_result = sandbox - .exec_command(&sha_cmd, 10_000, None, None, None) - .await; - match sha_result { - Ok(r) if r.success() => Ok(r.stdout_lossy().trim().to_string()), - Ok(r) => Err(exec_err("git rev-parse HEAD", r)), - Err(e) => Err(GitCommandError { - message: "git rev-parse HEAD failed".to_string(), - source: e, - }), - } + let mut options = GitCommitOptions::new(message, &author.name, &author.email); + options.allow_empty = true; + git.commit(repo, &options) + .await + .map_err(|error| git_error("git commit", error)) } /// Run a git checkpoint after the per-run sandbox git capability probe. @@ -184,7 +150,7 @@ pub(crate) async fn checked_git_checkpoint( .map_err(|err| SharedError::new(anyhow::Error::new(err))) } -/// Run a git diff via the sandbox (30 s default timeout). +/// The unified diff from `base` to `HEAD` (30 s default timeout). pub(crate) async fn git_diff( sandbox: &RunSandbox, base: &str, @@ -192,67 +158,33 @@ pub(crate) async fn git_diff( git_diff_with_timeout(sandbox, base, 30_000).await } -/// Run a git diff via the sandbox with a caller-supplied timeout in -/// milliseconds. +/// The unified diff from `base` to `HEAD` under a caller-supplied timeout +/// in milliseconds. /// /// Failure-path capture uses a shorter timeout than the checkpoint path so a /// pathological workspace (FS locks, corrupted index) doesn't stall terminal -/// event emission downstream (Slack notifier, SSE, CI hooks). +/// event emission downstream (Slack notifier, SSE, CI hooks). Paths come +/// back unquoted, which the Run Files denylist parser relies on. pub(crate) async fn git_diff_with_timeout( sandbox: &RunSandbox, base: &str, timeout_ms: u64, ) -> std::result::Result { - // `-c core.quotePath=false` forces paths with non-ASCII, tabs, quotes, - // or backslashes to emit unquoted. The Run Files Changed endpoint's - // `strip_denylisted_sections` parser only recognizes unquoted - // `diff --git a/ b/` headers; without this flag git would - // wrap such paths in `"a/…"` / `"b/…"` and evade the denylist (see - // docs/agent/reviews/2026-04-19-run-files-security-review.md). - let cmd = format!("{GIT_REMOTE} -c core.quotePath=false diff {base} HEAD"); - match sandbox - .exec_command(&cmd, timeout_ms, None, None, None) + let git = facet(sandbox, "git diff")?; + let options = GitDiffOptions::new(GitRevisionRange::new(base).to("HEAD")) + .timeout(Duration::from_millis(timeout_ms)); + git.diff_patch(sandbox.working_directory(), &options) .await - { - Ok(r) if r.success() => Ok(r.stdout_lossy()), - Ok(r) => Err(exec_err("git diff", r)), - Err(e) => Err(GitCommandError { - message: "git diff failed".to_string(), - source: e, - }), - } + .map_err(|error| git_error("git diff", error)) } // ── Machine-readable diff enumeration (Run Files endpoint) ───────────────── -/// Hardened git-command prefix for the Run Files endpoint. +/// A single changed-file entry of a range, as the Run Files endpoint reads +/// it. /// -/// Layers on top of [`GIT_REMOTE`]: -/// - `core.hooksPath=/dev/null`: repo-supplied hooks do not run. -/// - `core.fsmonitor=false`: no fsmonitor daemon interactions. -/// - `protocol.file.allow=never`: blocks local-protocol fetches. -/// -/// These invocations use [`sandbox_git_hardening_env`] via `exec_command` to -/// disable terminal prompts and external diff drivers. -const GIT_HARDENED: &str = "git -c maintenance.auto=0 -c gc.auto=0 -c core.hooksPath=/dev/null -c core.fsmonitor=false -c protocol.file.allow=never -c core.quotePath=false"; - -/// Environment additions applied to every hardened sandbox-side git invocation. -/// -/// `GIT_TERMINAL_PROMPT=0` prevents git from stalling on credential prompts -/// when a remote or subprocess triggers one. Clearing `GIT_EXTERNAL_DIFF` -/// neutralizes any inherited custom diff driver. -fn sandbox_git_hardening_env() -> std::collections::HashMap { - std::collections::HashMap::from([ - ("GIT_TERMINAL_PROMPT".to_string(), "0".to_string()), - ("GIT_EXTERNAL_DIFF".to_string(), String::new()), - ]) -} - -/// A single changed-file entry from `git diff --raw -z --find-renames=50%`. -/// -/// Paths are repo-relative, UTF-8; non-UTF-8 filenames are rejected by the -/// parser. Blob SHAs are lowercase hex. Modes are octal integers (`100644`, -/// `100755`, `120000`, `160000`, …). +/// Paths are repo-relative, UTF-8. Blob SHAs are lowercase hex. Modes are +/// octal strings (`100644`, `100755`, `120000`, `160000`, …). #[derive(Debug, Clone, PartialEq, Eq)] pub enum RawDiffEntry { Added { @@ -279,107 +211,111 @@ pub enum RawDiffEntry { new_mode: String, similarity: u8, }, + /// Symlink creation, deletion, or target change. No blob contents are + /// fetched for these: the "content" is the link target, which is + /// not meaningful to diff as file text. Symlink { path: String, change_kind: SymlinkChange, old_blob: Option, new_blob: Option, }, + /// Submodule (gitlink) pointer change. No blob contents exist for + /// these in the parent repo. Submodule { path: String, change_kind: SubmoduleChange, }, } -/// Lifecycle of a symlink entry (mode `120000`). -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SymlinkChange { Added, - Modified, Deleted, + Modified, } -/// Lifecycle of a submodule entry (mode `160000`). -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SubmoduleChange { Added, - Modified, Deleted, + Modified, } -/// Error produced by the sandbox-git helpers. -/// -/// Callers discriminate between transient (retry-safe) and permanent -/// conditions: a 503 can be returned to the client on `Transient`, while -/// `Permanent` errors should fall through to the patch-only fallback. -#[derive(Debug, Clone, PartialEq, Eq)] +/// Errors from the machine-readable diff paths, classified so the server +/// can fall back or retry. +#[derive(Debug, thiserror::Error)] pub enum DiffError { - /// Retry-safe failure: timeout, process kill, transient I/O. - Transient { message: String }, - /// Non-retryable failure: unknown revision, malformed output, etc. + /// Unknown revision, missing object, or a repository the driver could + /// not read: retrying will not help. + #[error("permanent git error: {message}")] Permanent { message: String }, + /// A timeout, a transport failure, or any other failure worth retrying. + #[error("transient git error: {message}")] + Transient { message: String }, } -impl std::fmt::Display for DiffError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Transient { message } => write!(f, "transient: {message}"), - Self::Permanent { message } => write!(f, "permanent: {message}"), - } - } -} - -impl std::error::Error for DiffError {} - -/// Size metadata for a single blob, as reported by `git cat-file -/// --batch-check`. +/// Blob metadata from a batch lookup. #[derive(Debug, Clone, PartialEq, Eq)] pub struct BlobMeta { pub sha: String, - /// `None` if the blob is missing (git reports `missing`). + /// `None` when git reports the blob as missing. pub size: Option, } /// Enumerate files changed between `base_sha` and `to_sha` via the sandbox. /// -/// Uses `git diff --raw -z --find-renames=50%` to get a machine-readable, -/// null-separated, SHA-addressed listing. Paths from this output are treated -/// as metadata only — blob reads use the SHAs, not the paths. -/// -/// The `--numstat` side-call classifies text vs binary so callers can skip -/// binary contents without ever invoking `git cat-file --batch` on them. +/// Paths from this listing are treated as metadata only; blob reads use +/// the SHAs, not the paths. The `--numstat` companion classifies text vs +/// binary so callers can skip binary contents without ever fetching them. pub async fn list_changed_files_raw( sandbox: &RunSandbox, base_sha: &str, to_sha: &str, ) -> std::result::Result, DiffError> { - let base_q = shell_quote(base_sha); - let to_q = shell_quote(to_sha); - let env = sandbox_git_hardening_env(); - let cmd = format!("{GIT_HARDENED} diff --raw -z --find-renames=50% {base_q}..{to_q}"); - let res = sandbox - .exec_command(&cmd, 10_000, None, Some(&env), None) + let git = diff_facet(sandbox)?; + let options = GitDiffOptions::new(GitRevisionRange::new(base_sha).to(to_sha)) + .find_renames(FIND_RENAMES_PERCENT) + .timeout(RUN_FILES_TIMEOUT); + let entries = git + .diff_entries(sandbox.working_directory(), &options) .await - .map_err(|e| DiffError::Transient { - message: e.display_with_causes(), - })?; + .map_err(|error| diff_error(&error))?; + entries + .into_iter() + .map(raw_diff_entry) + .collect::, String>>() + .map_err(|message| DiffError::Permanent { message }) +} - if res.termination == Termination::TimedOut { - return Err(DiffError::Transient { - message: "git diff --raw timed out".to_string(), - }); - } - if !res.success() { - // An unknown-object / bad-revision error is permanent; everything - // else we treat as transient so the server can retry safely. - let stderr = res.stderr_lossy().trim().to_string(); - if is_permanent_git_error(&stderr) { - return Err(DiffError::Permanent { message: stderr }); +fn diff_facet(sandbox: &RunSandbox) -> std::result::Result, DiffError> { + sandbox.git().map_err(|error| DiffError::Permanent { + message: fabro_sandbox::display_for_log(&error), + }) +} + +/// What a driver failure means for the Run Files endpoint: an unknown +/// revision or missing object is permanent (the handler falls through to +/// the stored patch), and so is output the driver could not read, since a +/// retry reads the same object; a timeout, a transport failure, or anything +/// else is transient and surfaces as a 503 for the client to retry. +fn diff_error(error: &sandbox_driver::Error) -> DiffError { + let message = fabro_sandbox::display_for_log(error); + match error { + sandbox_driver::Error::Io { .. } => DiffError::Permanent { message }, + sandbox_driver::Error::Git(failure) => { + let stderr = failure + .output() + .map(|output| String::from_utf8_lossy(output.stderr()).into_owned()) + .unwrap_or_default(); + if failure.kind() == GitFailureKind::RefNotFound || is_permanent_git_error(&stderr) { + DiffError::Permanent { message } + } else { + DiffError::Transient { message } + } } - return Err(DiffError::Transient { message: stderr }); + _ => DiffError::Transient { message }, } - - parse_raw_z(&res.stdout_lossy()).map_err(|message| DiffError::Permanent { message }) } fn is_permanent_git_error(stderr: &str) -> bool { @@ -388,137 +324,89 @@ fn is_permanent_git_error(stderr: &str) -> bool { let lower = stderr.to_lowercase(); lower.contains("unknown revision") || lower.contains("bad revision") + || lower.contains("bad object") || lower.contains("invalid revision") || lower.contains("no such path") || lower.contains("not a valid object name") } -fn parse_raw_z(stdout: &str) -> std::result::Result, String> { - // git diff --raw -z format: - // ": \0\0" - // For renames/copies: - // ": R\0\0\0" - // - // Multiple entries are concatenated with no separator between them. - let mut entries = Vec::new(); - let mut tokens = stdout.split('\0').peekable(); - while let Some(header) = tokens.next() { - if header.is_empty() { - continue; - } - if !header.starts_with(':') { - return Err(format!("unexpected token in diff --raw: {header:?}")); - } - let fields: Vec<&str> = header[1..].split(' ').collect(); - if fields.len() < 5 { - return Err(format!("short raw-diff header: {header:?}")); - } - let src_mode = fields[0]; - let dst_mode = fields[1]; - let src_sha = fields[2]; - let dst_sha = fields[3]; - let status = fields[4]; +/// The Run Files entry for one path of the driver's diff. Mode 120000 is a +/// symlink, 160000 a submodule. +fn raw_diff_entry(entry: GitDiffEntry) -> std::result::Result { + let is_mode = |mode: &Option, expected: &str| mode.as_deref() == Some(expected); + let is_symlink = is_mode(&entry.old_mode, "120000") || is_mode(&entry.new_mode, "120000"); + let is_submodule = is_mode(&entry.old_mode, "160000") || is_mode(&entry.new_mode, "160000"); + let path = entry.path; + let old_blob = entry.old_blob.unwrap_or_default(); + let new_blob = entry.new_blob.unwrap_or_default(); + let old_mode = entry.old_mode.unwrap_or_default(); + let new_mode = entry.new_mode.unwrap_or_default(); - let entry = if status.starts_with('R') || status.starts_with('C') { - let score: u8 = status[1..].parse().unwrap_or(0); - let old_path = tokens - .next() - .ok_or_else(|| "missing old_path for rename".to_string())? - .to_string(); - let new_path = tokens - .next() - .ok_or_else(|| "missing new_path for rename".to_string())? - .to_string(); - RawDiffEntry::Renamed { - old_path, - new_path, - old_blob: src_sha.to_string(), - new_blob: dst_sha.to_string(), - new_mode: dst_mode.to_string(), - similarity: score, - } - } else { - let path = tokens - .next() - .ok_or_else(|| "missing path for diff entry".to_string())? - .to_string(); - classify_entry(status, src_mode, dst_mode, src_sha, dst_sha, &path)? - }; - entries.push(entry); - } - Ok(entries) -} - -fn classify_entry( - status: &str, - src_mode: &str, - dst_mode: &str, - src_sha: &str, - dst_sha: &str, - path: &str, -) -> std::result::Result { - // Mode 120000 = symlink, 160000 = submodule (gitlink). - let is_symlink_change = src_mode == "120000" || dst_mode == "120000"; - let is_submodule_change = src_mode == "160000" || dst_mode == "160000"; - - Ok(match (status, is_symlink_change, is_submodule_change) { - ("A", true, _) => RawDiffEntry::Symlink { - path: path.to_string(), - change_kind: SymlinkChange::Added, - old_blob: None, - new_blob: Some(dst_sha.to_string()), + Ok(match (entry.change, is_symlink, is_submodule) { + (GitChange::Renamed | GitChange::Copied, _, _) => RawDiffEntry::Renamed { + old_path: entry.old_path.unwrap_or_default(), + new_path: path, + old_blob, + new_blob, + new_mode, + similarity: entry.similarity.unwrap_or(0), }, - ("A", _, true) => RawDiffEntry::Submodule { - path: path.to_string(), + (GitChange::Added, true, _) => RawDiffEntry::Symlink { + path, + change_kind: SymlinkChange::Added, + old_blob: None, + new_blob: Some(new_blob), + }, + (GitChange::Added, _, true) => RawDiffEntry::Submodule { + path, change_kind: SubmoduleChange::Added, }, - ("A", _, _) => RawDiffEntry::Added { - path: path.to_string(), - new_blob: dst_sha.to_string(), - new_mode: dst_mode.to_string(), + (GitChange::Added, _, _) => RawDiffEntry::Added { + path, + new_blob, + new_mode, }, - ("D", true, _) => RawDiffEntry::Symlink { - path: path.to_string(), + (GitChange::Deleted, true, _) => RawDiffEntry::Symlink { + path, change_kind: SymlinkChange::Deleted, - old_blob: Some(src_sha.to_string()), - new_blob: None, + old_blob: Some(old_blob), + new_blob: None, }, - ("D", _, true) => RawDiffEntry::Submodule { - path: path.to_string(), + (GitChange::Deleted, _, true) => RawDiffEntry::Submodule { + path, change_kind: SubmoduleChange::Deleted, }, - ("D", _, _) => RawDiffEntry::Deleted { - path: path.to_string(), - old_blob: src_sha.to_string(), - old_mode: src_mode.to_string(), + (GitChange::Deleted, _, _) => RawDiffEntry::Deleted { + path, + old_blob, + old_mode, }, - ("M" | "T", true, _) => RawDiffEntry::Symlink { - path: path.to_string(), + (GitChange::Modified | GitChange::TypeChanged, true, _) => RawDiffEntry::Symlink { + path, change_kind: SymlinkChange::Modified, - old_blob: Some(src_sha.to_string()), - new_blob: Some(dst_sha.to_string()), + old_blob: Some(old_blob), + new_blob: Some(new_blob), }, - ("M" | "T", _, true) => RawDiffEntry::Submodule { - path: path.to_string(), + (GitChange::Modified | GitChange::TypeChanged, _, true) => RawDiffEntry::Submodule { + path, change_kind: SubmoduleChange::Modified, }, - ("M" | "T", _, _) => RawDiffEntry::Modified { - path: path.to_string(), - old_blob: src_sha.to_string(), - new_blob: dst_sha.to_string(), - new_mode: dst_mode.to_string(), + (GitChange::Modified | GitChange::TypeChanged, _, _) => RawDiffEntry::Modified { + path, + old_blob, + new_blob, + new_mode, }, (other, _, _) => { - return Err(format!("unknown raw-diff status {other:?} for {path:?}")); + return Err(format!("unknown diff status {other:?} for {path:?}")); } }) } pub use fabro_types::{DiffStats, DiffSummary}; -/// Output of `git diff --numstat`: which paths are binary, plus per-path -/// `+/-` line totals for text files in the range. Both pieces come from a -/// single git invocation so callers don't need to run two diffs. +/// What `git diff --numstat` says about a range: which paths are binary, +/// plus per-path `+/-` line totals for text files. #[derive(Debug, Default)] pub struct DiffNumstat { /// Repo-relative paths (post-rename) that git classifies as binary. @@ -548,96 +436,41 @@ pub fn summarize_diff_numstat(numstat: &DiffNumstat) -> DiffSummary { } } -/// Run `git diff --numstat` once and return both the set of binary paths and -/// text-file `+/-` totals. The single call replaces the previous binary-only -/// helper. +/// The numstat of `base_sha..to_sha`: the set of binary paths and the +/// text-file `+/-` totals, from one driver call. pub async fn list_diff_numstat( sandbox: &RunSandbox, base_sha: &str, to_sha: &str, ) -> std::result::Result { - let base_q = shell_quote(base_sha); - let to_q = shell_quote(to_sha); - let env = sandbox_git_hardening_env(); - let cmd = format!("{GIT_HARDENED} diff --numstat --find-renames=50% {base_q}..{to_q}"); - let res = sandbox - .exec_command(&cmd, 10_000, None, Some(&env), None) + let git = diff_facet(sandbox)?; + let options = GitDiffOptions::new(GitRevisionRange::new(base_sha).to(to_sha)) + .find_renames(FIND_RENAMES_PERCENT) + .timeout(RUN_FILES_TIMEOUT); + let rows = git + .diff_numstat(sandbox.working_directory(), &options) .await - .map_err(|e| DiffError::Transient { - message: e.display_with_causes(), - })?; - - if res.termination == Termination::TimedOut { - return Err(DiffError::Transient { - message: "git diff --numstat timed out".to_string(), - }); - } - if !res.success() { - let stderr = res.stderr_lossy().trim().to_string(); - if is_permanent_git_error(&stderr) { - return Err(DiffError::Permanent { message: stderr }); - } - return Err(DiffError::Transient { message: stderr }); - } + .map_err(|error| diff_error(&error))?; let mut out = DiffNumstat::default(); - for line in res.stdout_lossy().lines() { - // `-\t-\t` marks binary. Rename lines read `<+>\t<->\t => - // ` or `<+>\t<->\t{ => }`. - if let Some(rest) = line.strip_prefix("-\t-\t") { - out.binary_paths.insert(extract_new_path_from_numstat(rest)); - continue; + for row in rows { + match (row.additions, row.deletions) { + (Some(additions), Some(deletions)) => { + out.line_stats_by_path.insert(row.path, DiffStats { + additions: i64::try_from(additions).unwrap_or(i64::MAX), + deletions: i64::try_from(deletions).unwrap_or(i64::MAX), + }); + } + _ => { + out.binary_paths.insert(row.path); + } } - // Text rows: `\t\t`. Tolerate malformed lines - // (e.g. trailing whitespace) by skipping rather than failing the - // whole diff — the rest of the response stays usable. - let mut parts = line.splitn(3, '\t'); - let adds_s = parts.next().unwrap_or(""); - let dels_s = parts.next().unwrap_or(""); - let Some(path_s) = parts.next() else { - continue; - }; - let Ok(adds) = adds_s.parse::() else { - continue; - }; - let Ok(dels) = dels_s.parse::() else { - continue; - }; - let path = extract_new_path_from_numstat(path_s); - out.line_stats_by_path.insert(path, DiffStats { - additions: adds, - deletions: dels, - }); } Ok(out) } -fn extract_new_path_from_numstat(rest: &str) -> String { - // Forms seen: - // "simple/path" - // "old => new" - // "prefix/{old => new}/suffix" - if let Some(open_idx) = rest.find('{') { - if let Some(close_idx) = rest[open_idx..].find('}') { - let before = &rest[..open_idx]; - let after = &rest[open_idx + close_idx + 1..]; - let inside = &rest[open_idx + 1..open_idx + close_idx]; - if let Some((_, new)) = inside.split_once(" => ") { - return format!("{before}{new}{after}"); - } - } - } - if let Some((_, new)) = rest.split_once(" => ") { - return new.to_string(); - } - rest.to_string() -} - -/// Fetch blob metadata (size) for many SHAs in one sandbox invocation via -/// `git cat-file --batch-check`. -/// -/// The order of returned `BlobMeta` entries matches the input `shas` order. -/// SHAs reported as `missing` by git yield `BlobMeta { size: None, .. }`. +/// Blob sizes for many SHAs in one driver call, in the order of `shas`. +/// A blob git does not have yields `BlobMeta { size: None, .. }`. pub async fn stream_blob_metadata( sandbox: &RunSandbox, shas: &[String], @@ -645,68 +478,27 @@ pub async fn stream_blob_metadata( if shas.is_empty() { return Ok(Vec::new()); } - let env = sandbox_git_hardening_env(); - let quoted_shas: Vec = shas.iter().map(|s| shell_quote(s)).collect(); - let cmd = format!( - "printf '%s\\n' {} | {GIT_HARDENED} cat-file --batch-check", - quoted_shas.join(" ") - ); - let res = sandbox - .exec_command(&cmd, 10_000, None, Some(&env), None) + let git = diff_facet(sandbox)?; + let sizes = git + .blob_sizes(sandbox.working_directory(), shas) .await - .map_err(|e| DiffError::Transient { - message: e.display_with_causes(), - })?; - - if res.termination == Termination::TimedOut { - return Err(DiffError::Transient { - message: "git cat-file --batch-check timed out".to_string(), - }); - } - if !res.success() { - return Err(DiffError::Transient { - message: format!( - "git cat-file --batch-check failed: {}", - res.stderr_lossy().trim() - ), - }); - } - - let mut metas = Vec::with_capacity(shas.len()); - for line in res.stdout_lossy().lines() { - // Lines: " " OR " missing" - let mut parts = line.split(' '); - let sha = parts - .next() - .ok_or_else(|| DiffError::Permanent { - message: format!("empty cat-file line: {line:?}"), - })? - .to_string(); - let second = parts.next().unwrap_or(""); - if second == "missing" { - metas.push(BlobMeta { sha, size: None }); - continue; - } - let size_str = parts.next().unwrap_or(""); - let size = size_str.parse::().map_err(|e| DiffError::Permanent { - message: format!("unparseable size {size_str:?} for {sha}: {e}"), - })?; - metas.push(BlobMeta { - sha, - size: Some(size), - }); - } - Ok(metas) + .map_err(|error| diff_error(&error))?; + Ok(shas + .iter() + .zip(sizes) + .map(|(sha, size)| BlobMeta { + sha: sha.clone(), + size, + }) + .collect()) } -/// Fetch blob contents for many SHAs in one sandbox invocation via -/// `git cat-file --batch`. +/// Blob contents for many SHAs in one driver call, in the order of `shas`. /// /// Contents are size-capped per blob: any blob exceeding `size_cap_bytes` -/// returns `None` in its slot (the caller should flag that entry as -/// truncated). Callers are expected to have pre-filtered binary blobs via -/// [`list_diff_numstat`] — `--batch` output stream is text-oriented and -/// non-UTF-8 bytes are lossy through the sandbox `String` channel. +/// returns `None` in its slot (the caller flags that entry as truncated), +/// as does a blob git does not have or one that is not UTF-8. Callers are +/// expected to have pre-filtered binary blobs via [`list_diff_numstat`]. pub async fn stream_blobs( sandbox: &RunSandbox, shas: &[String], @@ -715,94 +507,15 @@ pub async fn stream_blobs( if shas.is_empty() { return Ok(Vec::new()); } - let env = sandbox_git_hardening_env(); - let quoted_shas: Vec = shas.iter().map(|s| shell_quote(s)).collect(); - let cmd = format!( - "printf '%s\\n' {} | {GIT_HARDENED} cat-file --batch", - quoted_shas.join(" ") - ); - let res = sandbox - .exec_command(&cmd, 10_000, None, Some(&env), None) + let git = diff_facet(sandbox)?; + let blobs = git + .blobs(sandbox.working_directory(), shas, size_cap_bytes) .await - .map_err(|e| DiffError::Transient { - message: e.display_with_causes(), - })?; - - if res.termination == Termination::TimedOut { - return Err(DiffError::Transient { - message: "git cat-file --batch timed out".to_string(), - }); - } - if !res.success() { - return Err(DiffError::Transient { - message: format!("git cat-file --batch failed: {}", res.stderr_lossy().trim()), - }); - } - - parse_batch_output(&res.stdout_lossy(), shas, size_cap_bytes) - .map_err(|message| DiffError::Permanent { message }) -} - -fn parse_batch_output( - stdout: &str, - shas: &[String], - size_cap_bytes: u64, -) -> std::result::Result>, String> { - // `git cat-file --batch` output per blob: - // " \n\n" - // `missing` blob: " missing\n" (no content). - let mut results: Vec> = Vec::with_capacity(shas.len()); - let bytes = stdout.as_bytes(); - let mut pos = 0; - - while pos < bytes.len() { - // Find end of header line. - let Some(nl_rel) = bytes[pos..].iter().position(|&b| b == b'\n') else { - break; - }; - let header = std::str::from_utf8(&bytes[pos..pos + nl_rel]) - .map_err(|e| format!("non-utf8 header in cat-file output: {e}"))?; - pos += nl_rel + 1; - - let mut parts = header.split(' '); - let _sha = parts.next().unwrap_or(""); - let second = parts.next().unwrap_or(""); - if second == "missing" { - results.push(None); - continue; - } - let size_str = parts.next().unwrap_or(""); - let size: usize = size_str - .parse() - .map_err(|e| format!("unparseable size {size_str:?}: {e}"))?; - - let end = pos + size; - if end > bytes.len() { - return Err(format!( - "cat-file stream truncated: expected {size} bytes, have {}", - bytes.len() - pos - )); - } - if (size as u64) > size_cap_bytes { - results.push(None); - } else { - let content = std::str::from_utf8(&bytes[pos..end]) - .map_err(|e| format!("non-utf8 blob contents: {e}"))?; - results.push(Some(content.to_string())); - } - pos = end; - // Trailing newline that delimits the next entry. - if pos < bytes.len() && bytes[pos] == b'\n' { - pos += 1; - } - } - - // Pad with None if the stream didn't cover every requested SHA (e.g. - // duplicate-sha deduping by git). - while results.len() < shas.len() { - results.push(None); - } - Ok(results) + .map_err(|error| diff_error(&error))?; + Ok(blobs + .into_iter() + .map(|blob| blob.and_then(|bytes| String::from_utf8(bytes).ok())) + .collect()) } #[cfg(test)] @@ -813,6 +526,7 @@ mod tests { )] use fabro_sandbox::test_support::{MockSandbox, exec_result}; + use fabro_sandbox::{ExecResult, Termination}; use super::*; @@ -837,12 +551,6 @@ mod tests { exec_result(stdout, stderr, Some(exit_code), Termination::Exited, 1) } - #[test] - fn git_remote_disables_commit_and_tag_signing() { - assert!(GIT_REMOTE.contains("-c commit.gpgsign=false")); - assert!(GIT_REMOTE.contains("-c tag.gpgsign=false")); - } - #[tokio::test] async fn git_checkpoint_reports_add_timeout() { let sandbox = scripted(&[exec_timed_out(77)]); @@ -859,7 +567,13 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "git add timed out after 77ms"); + assert_eq!(err.to_string(), "git add failed"); + let timed_out = matches!( + err.source.driver(), + Some(sandbox_driver::Error::Git(failure)) + if failure.output().is_some_and(|output| output.termination() == Termination::TimedOut) + ); + assert!(timed_out, "{}", fabro_sandbox::display_for_log(&err)); assert!( fabro_sandbox::default_redacted_output_tail(&err).is_none(), "empty exec streams should not produce a tail" @@ -915,11 +629,12 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "git commit timed out after 88ms"); + assert_eq!(err.to_string(), "git commit failed"); } #[tokio::test] - async fn git_checkpoint_reports_rev_parse_killed_without_output() { + async fn git_checkpoint_reports_a_failed_sha_read_as_the_commit_failing() { + // add, commit, then the driver's own rev-parse of the new HEAD. let sandbox = scripted(&[exec_ok(), exec_ok(), exec_failed(-1, "", "")]); let err = git_checkpoint( &sandbox.sandbox(), @@ -934,103 +649,66 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "git rev-parse HEAD failed (exit -1)"); + assert_eq!(err.to_string(), "git commit failed"); } + /// The commit message and author travel in the driver's own commit + /// command, and repository hooks never run: the driver disables them + /// whatever the checkpoint settings say. #[tokio::test] - async fn git_checkpoint_uses_unique_commit_message_paths_for_same_run_and_node() { - let sandbox = scripted(&[ - exec_ok(), - exec_ok(), - exec_ok(), - exec_ok(), - exec_ok(), - exec_ok(), - ]); - let author = crate::git::GitAuthor::default(); - - let first = git_checkpoint( - &sandbox.sandbox(), - "run1", - "work", - "success", - 1, - None, - &RunCheckpointSettings::default(), - &author, - ) - .await; - let second = git_checkpoint( - &sandbox.sandbox(), - "run1", - "work", - "success", - 1, - None, - &RunCheckpointSettings::default(), - &author, - ) - .await; - - assert!(first.is_ok(), "first checkpoint failed: {:?}", first.err()); - assert!( - second.is_ok(), - "second checkpoint failed: {:?}", - second.err() - ); - - let write_paths: Vec = sandbox - .written_files() - .into_iter() - .map(|(path, _)| path) - .collect(); - assert_eq!(write_paths.len(), 2); - assert!( - write_paths - .iter() - .all(|path| path.starts_with("/tmp/fabro-commit-msg-")), - "unexpected commit message paths: {write_paths:?}" - ); - assert_ne!(write_paths[0], write_paths[1]); - - let delete_paths = sandbox.driver().memory_fs().deletes(); - assert_eq!(delete_paths, write_paths); - - let commands = sandbox.driver().scripted_exec().commands(); - let commit_commands = commands - .iter() - .filter(|command| command.contains(" commit ")) - .collect::>(); - assert_eq!(commit_commands.len(), 2); - for (command, path) in commit_commands.iter().zip(write_paths.iter()) { - assert!( - command.contains(&format!("-F {}", shell_quote(path))), - "expected commit command to use {path:?}, got {command:?}" - ); - } - } - - #[tokio::test] - async fn git_checkpoint_uses_configured_timeout_for_add_and_commit() { - let sandbox = scripted(&[exec_ok(), exec_ok(), exec_ok()]); + async fn git_checkpoint_commits_through_the_hardened_driver_command() { + let mut sha = exec_ok(); + sha.stdout = b"abc123\n".to_vec(); + let sandbox = scripted(&[exec_ok(), exec_ok(), sha]); let checkpoint = RunCheckpointSettings { - commit_timeout_ms: 600_000, + skip_git_hooks: false, ..RunCheckpointSettings::default() }; - git_checkpoint( + let author = crate::git::GitAuthor::default(); + + let sha = git_checkpoint( &sandbox.sandbox(), "run1", "work", "success", 1, - None, + Some("feedface".to_owned()), &checkpoint, - &crate::git::GitAuthor::default(), + &author, ) .await - .expect("checkpoint should succeed"); + .expect("checkpoint succeeds"); + assert_eq!(sha, "abc123"); - assert_eq!(sandbox.captured_timeouts(), vec![600_000, 600_000, 10_000]); + let commands = sandbox.driver().scripted_exec().commands(); + let add = commands + .iter() + .find(|command| command.contains("'add' '-A'")) + .expect("the add ran"); + assert!( + add.contains(":(glob,exclude)**/node_modules/**"), + "built-in excludes are pathspecs: {add}" + ); + let commit = commands + .iter() + .find(|command| command.contains("'commit'")) + .expect("the commit ran"); + assert!(commit.contains("core.hooksPath=/dev/null"), "{commit}"); + assert!(commit.contains("commit.gpgsign=false"), "{commit}"); + assert!(commit.contains("'--allow-empty'"), "{commit}"); + assert!( + commit.contains("fabro(run1): work (success)") + && commit.contains("Fabro-Checkpoint: feedface"), + "{commit}" + ); + assert!( + commit.contains(&format!("user.name={}", author.name)), + "{commit}" + ); + assert!( + sandbox.written_files().is_empty(), + "no message file is written" + ); } #[tokio::test] @@ -1040,7 +718,13 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "git diff timed out after 99ms"); + assert_eq!(err.to_string(), "git diff failed"); + let timed_out = matches!( + err.source.driver(), + Some(sandbox_driver::Error::Git(failure)) + if failure.output().is_some_and(|output| output.termination() == Termination::TimedOut) + ); + assert!(timed_out, "{}", fabro_sandbox::display_for_log(&err)); } #[tokio::test] @@ -1050,7 +734,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err.to_string(), "git diff failed (exit 128)"); + assert_eq!(err.to_string(), "git diff failed"); assert!(!err.to_string().contains("fatal: bad revision")); let tail = fabro_sandbox::default_redacted_output_tail(&err).expect("tail present"); @@ -1058,62 +742,21 @@ mod tests { } #[tokio::test] - async fn git_checkpoint_appends_no_verify_when_skip_hooks_enabled() { - // add, commit, rev-parse - let sandbox = scripted(&[exec_ok(), exec_ok(), exec_ok()]); - let checkpoint = RunCheckpointSettings { - skip_git_hooks: true, - ..RunCheckpointSettings::default() - }; - git_checkpoint( - &sandbox.sandbox(), - "run1", - "work", - "success", - 1, - None, - &checkpoint, - &crate::git::GitAuthor::default(), - ) - .await - .expect("checkpoint should succeed"); - + async fn git_diff_passes_the_range_and_timeout_to_the_driver() { + let mut patch = exec_ok(); + patch.stdout = b"diff --git a/x b/x\n".to_vec(); + let sandbox = scripted(&[patch]); + let diff = git_diff_with_timeout(&sandbox.sandbox(), "base-sha", 5_000) + .await + .expect("diff succeeds"); + assert_eq!(diff, "diff --git a/x b/x\n"); let commands = sandbox.driver().scripted_exec().commands(); - let commit_cmd = commands - .iter() - .find(|c| c.contains(" commit ")) - .expect("commit command should be issued"); assert!( - commit_cmd.contains("--no-verify"), - "commit command should include --no-verify when skip_git_hooks=true; got {commit_cmd:?}" - ); - } - - #[tokio::test] - async fn git_checkpoint_omits_no_verify_when_skip_hooks_disabled() { - let sandbox = scripted(&[exec_ok(), exec_ok(), exec_ok()]); - git_checkpoint( - &sandbox.sandbox(), - "run1", - "work", - "success", - 1, - None, - &RunCheckpointSettings::default(), - &crate::git::GitAuthor::default(), - ) - .await - .expect("checkpoint should succeed"); - - let commands = sandbox.driver().scripted_exec().commands(); - let commit_cmd = commands - .iter() - .find(|c| c.contains(" commit ")) - .expect("commit command should be issued"); - assert!( - !commit_cmd.contains("--no-verify"), - "commit command should omit --no-verify when skip_git_hooks=false; got {commit_cmd:?}" + commands[0].contains("'diff'") && commands[0].contains("'base-sha..HEAD'"), + "{}", + commands[0] ); + assert_eq!(sandbox.captured_timeouts(), vec![5_000]); } #[tokio::test] @@ -1450,17 +1093,4 @@ mod tests { .expect_err("expected error for unknown base sha"); assert!(matches!(err, DiffError::Permanent { .. }), "err: {err:?}"); } - - #[test] - fn extract_new_path_from_numstat_handles_brace_renames() { - assert_eq!(extract_new_path_from_numstat("simple/path"), "simple/path"); - assert_eq!( - extract_new_path_from_numstat("old.txt => new.txt"), - "new.txt" - ); - assert_eq!( - extract_new_path_from_numstat("src/{old => new}/file.rs"), - "src/new/file.rs" - ); - } } diff --git a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs index 053bab66c..97b5daec2 100644 --- a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs +++ b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs @@ -1,9 +1,9 @@ use fabro_agent::RunSandbox; -use fabro_sandbox::shell_quote; +use fabro_sandbox::{ExecResult, ExecResultExt, Termination, shell_quote}; use fabro_util::error::SharedError; use tokio::sync::OnceCell; -use crate::sandbox_git::{GIT_REMOTE, exec_err}; +use crate::sandbox_git::GitCommandError; pub(crate) struct SandboxGitRuntime { probe: OnceCell>, @@ -69,7 +69,7 @@ async fn probe_sandbox_git(sandbox: &RunSandbox) -> Result<(), SharedError> { temp_q = shell_quote(&temp), probe_file_q = shell_quote(&probe_file), index_q = shell_quote(&index), - git = GIT_REMOTE, + git = "git -c maintenance.auto=0 -c gc.auto=0", ); exec_ok(sandbox, &command).await } @@ -95,3 +95,23 @@ async fn exec_ok(sandbox: &RunSandbox, command: &str) -> Result<(), SharedError> )))) } } + +/// The probe's failure, named by how the command ended; the output tail +/// travels in the source. +fn exec_err(label: &str, result: ExecResult) -> GitCommandError { + let duration_ms = result.duration_ms(); + let message = match result.termination { + Termination::TimedOut => format!("{label} timed out after {duration_ms}ms"), + Termination::Cancelled | Termination::Killed => { + format!("{label} cancelled after {duration_ms}ms") + } + _ => format!( + "{label} failed (exit {})", + result.program_exit_code().unwrap_or(-1) + ), + }; + GitCommandError { + message, + source: result.into_exec_error(label), + } +} diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index 26ceafa40..5dd6143af 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -1039,14 +1039,16 @@ impl Default for RunExecutionSettings { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunCheckpointSettings { pub exclude_globs: Vec, - /// When `true`, Fabro-managed run-branch checkpoint commits bypass - /// local Git commit hooks (e.g. `pre-commit`, `commit-msg`). This does - /// not affect Fabro workflow `[[run.hooks]]` or metadata-branch - /// snapshots, which already bypass repository hooks. + /// Accepted for compatibility. Fabro-managed run-branch checkpoint + /// commits never run local Git commit hooks (e.g. `pre-commit`, + /// `commit-msg`): the sandbox driver disables repository hooks on every + /// git command it runs, whatever this field says. Fabro workflow + /// `[[run.hooks]]` are unaffected. #[serde(default)] pub skip_git_hooks: bool, - /// Timeout (ms) for the per-node run-branch checkpoint commit, which runs - /// repository commit hooks unless `skip_git_hooks` is set. Default 30_000. + /// Accepted for compatibility. The per-node run-branch checkpoint commit + /// runs under the sandbox driver's own git command budget now that no + /// repository hook can prolong it. Default 30_000. #[serde(default = "default_checkpoint_commit_timeout_ms")] pub commit_timeout_ms: u64, }