From 7d9d2bf5f3d8fcf97d2fc14bc36baaa45cb4a238 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 11 Sep 2026 12:55:39 -0600 Subject: [PATCH] Run fabro's git plumbing through the driver's verbs Fabro assembled its own hardened git command lines (maintenance, hooks, fsmonitor, path quoting, signing, the file transport, external diff drivers) in three crates and parsed raw diff, numstat, cat-file, and log output itself. The driver's git facet now carries fetch, rev-parse, ancestry, diff entries, numstat, patch, log, blob sizes and contents, config, untracked files, and stage-all, hardened by default and typed, so the checkpoint commit, the run diffs, the Run Files listing and blob reads, the commit log, the fork fetch, the agent's changed-files detection, and the git identity setup go through it. The parsers and the command prefixes go; the per-run capability probe keeps its own plumbing script. Checkpoint commits never run repository hooks now, so skip_git_hooks and commit_timeout are accepted for compatibility only. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 1 + docs/public/api-reference/fabro-api.yaml | 7 +- docs/public/execution/run-configuration.mdx | 4 +- lib/apps/fabro-server/Cargo.toml | 1 + lib/apps/fabro-server/src/run_files.rs | 184 ++-- .../fabro-sandbox/src/driver_sandbox.rs | 8 +- lib/components/fabro-sandbox/src/sandbox.rs | 54 +- .../src/handler/llm/changed_files.rs | 53 +- .../fabro-workflow/src/pipeline/initialize.rs | 38 +- .../fabro-workflow/src/sandbox_git.rs | 992 ++++++------------ .../fabro-workflow/src/sandbox_git_runtime.rs | 26 +- .../fabro-types/src/settings/run.rs | 14 +- 12 files changed, 505 insertions(+), 877 deletions(-) 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, }