From 06f9cb83616b384fd53224e903e16b02efb0dcfb Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 19 Sep 2026 09:18:29 -0400 Subject: [PATCH] Delete fabro-sandbox's clone and push chain The engine prepares every run's checkout, so fabro's clone orchestration, the per-checkout GitHub credentials, the run-branch setup, the push retries, and the push policies had no production caller. RepoWorkspace::plan still validates the clone request and now refuses one that asks for a clone; initialize creates an empty workspace root. SandboxWorkspaceLayout and snapshot_info stay: the run record projection in sandbox_spec.rs reads them. The run tool regression keeps its assertion (a child targets the parent's pushed run branch) over a plain git fixture instead of the deleted setup. The Docker, Daytona, and Daytona-wire clone layout tests go: they proved only the legacy clone. fabro-sandbox drops base64, uuid, fabro-proc, serde, strum, and sandbox-driver-daytona-config; chrono is test-only. Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 21 +- Cargo.lock | 6 - lib/apps/fabro-server/src/run_manifest.rs | 13 +- lib/apps/fabro-server/src/run_tool_create.rs | 21 +- lib/components/fabro-sandbox/Cargo.toml | 9 +- lib/components/fabro-sandbox/src/clone.rs | 380 --------- .../fabro-sandbox/src/clone_source.rs | 45 +- .../fabro-sandbox/src/credentials.rs | 154 ---- lib/components/fabro-sandbox/src/daytona.rs | 95 --- .../fabro-sandbox/src/driver_sandbox.rs | 184 +---- .../fabro-sandbox/src/environment.rs | 10 +- .../fabro-sandbox/src/git_policy.rs | 136 +-- lib/components/fabro-sandbox/src/lib.rs | 11 +- .../fabro-sandbox/src/provider_sandbox.rs | 7 +- lib/components/fabro-sandbox/src/sandbox.rs | 782 +----------------- .../fabro-sandbox/src/sandbox_spec.rs | 17 +- .../tests/daytona_streaming_live.rs | 64 -- .../fabro-sandbox/tests/docker_streaming.rs | 65 -- .../fabro-sandbox/tests/driver_bench.rs | 1 - 19 files changed, 74 insertions(+), 1947 deletions(-) delete mode 100644 lib/components/fabro-sandbox/src/clone.rs delete mode 100644 lib/components/fabro-sandbox/src/credentials.rs diff --git a/AGENTS.md b/AGENTS.md index a7f26cb0b..ad10f05ef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,21 +30,12 @@ macOS note: if `cargo nextest run` fails with `Too many open files (os error 24) ### Docker sandbox provider - Docker is the default runtime sandbox provider from `defaults.toml`. The Fabro process must have a working Docker client environment (`DOCKER_HOST`, socket access, Docker Desktop behavior, TLS settings, groups/permissions, and any remote daemon policy are operator responsibilities). - The packaged compose service mounts `/var/run/docker.sock` so the server can create sibling run containers on the host daemon. This is host-root-equivalent under Docker's security model; only use it in the trusted, single-tenant deployment model described by the sandbox code/docs. -- Docker and Daytona are clone-based providers. When a run manifest has a GitHub origin, they clone it into the provider workspace. Present non-GitHub origins fail unless the provider has `skip_clone = true`; absent origins or `skip_clone = true` create an empty workspace without repository files. For an exact commit, the submitted branch names the working branch and the syntactically valid SHA is requested directly. No layer proves branch/SHA ancestry: a fetchable commit is checked out, an unavailable commit fails setup, and branch HEAD is never substituted. -- The sandbox layer also accepts an optional exact commit for future admitted - runs. An exact commit always requires a non-empty branch. The sandbox driver - performs the pin the same way on every provider: it initializes an empty - repository, fetches the SHA directly at the requested depth, and attaches - the admitted branch to it, so the workspace reports the admitted branch - name. Daytona's native toolbox clone serves plain branch clones only; its - commit pin checks the branch head out first, so the driver does not use - it. A successful clone has the pin checked out; the driver's - conformance suite verifies that on every provider, and fabro does not - re-verify HEAD. Never fall back to a newer branch HEAD, and do not wire - this capability directly from legacy `GitContext.sha`. The sandbox layer - does not verify that the commit is reachable from the branch; admission - owns that check. Current production callers remain branch-only until the - RunIntent admission cutover supplies a validated branch/SHA pair. +- Fabro no longer clones a repository into a sandbox: the engine prepares + every run's checkout. `CloneRequest` still travels beside the sandbox spec + so the run record names the origin and branch; fabro validates it (a pin + needs a branch, a non-GitHub origin needs `skip_clone`) and refuses a + request that asks for a clone. Preflight and `fabro exec` initialize + sandboxes with `CloneRequest::none()`, which creates an empty workspace. ### Release automation - `cargo dev release` — creates the next stable release tag. Use `cargo dev release --nightly` for a nightly prerelease. Use `--dry-run` to print planned commands without mutating git or running Cargo, `--skip-tests` only after running the release-mode smoke yourself, and `--release-date YYYY-MM-DD` or `FABRO_RELEASE_DATE` for deterministic version computation. diff --git a/Cargo.lock b/Cargo.lock index de3ec254c..4bba62a53 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2657,10 +2657,8 @@ version = "0.361.0-nightly.0" dependencies = [ "anyhow", "async-trait", - "base64", "chrono", "fabro-github", - "fabro-proc", "fabro-redact", "fabro-static", "fabro-test", @@ -2671,22 +2669,18 @@ dependencies = [ "reqwest 0.13.4", "sandbox-driver", "sandbox-driver-daytona", - "sandbox-driver-daytona-config", "sandbox-driver-docker", "sandbox-driver-docker-config", "sandbox-driver-host", "sandbox-driver-protocol", "sandbox-driver-testing", - "serde", "serde_json", - "strum 0.28.0", "tempfile", "thiserror 2.0.18", "tokio", "tokio-util", "toml 0.8.23", "tracing", - "uuid", ] [[package]] diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 2225d6f23..83dfa2fe5 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -520,7 +520,6 @@ async fn build_preflight_report( &sandbox_provider, prepared, &resolved_run, - github_app.clone(), &access, ) .await; @@ -882,7 +881,6 @@ fn preflight_sandbox_spec( sandbox_provider: &SandboxProviderKind, prepared: &PreparedManifest, resolved_run: &RunNamespace, - github_app: Option, access: &ProviderAccess, ) -> std::result::Result { let clone_origin_url = prepared @@ -919,7 +917,6 @@ fn preflight_sandbox_spec( access: access.clone(), spec, clone, - github_app, run_id: None, }) } @@ -929,16 +926,9 @@ async fn run_sandbox_check( sandbox_provider: &SandboxProviderKind, prepared: &PreparedManifest, resolved_run: &RunNamespace, - github_app: Option, access: &ProviderAccess, ) -> bool { - let spec = match preflight_sandbox_spec( - sandbox_provider, - prepared, - resolved_run, - github_app.clone(), - access, - ) { + let spec = match preflight_sandbox_spec(sandbox_provider, prepared, resolved_run, access) { Ok(spec) => spec, Err(err) => { checks.push(CheckResult { @@ -2199,7 +2189,6 @@ provider = "local" &SandboxProviderKind::DOCKER, &prepared, &resolved, - None, &ProviderAccess::default(), ); diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index 01d7ae7df..81171bf20 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -1,4 +1,5 @@ -// Integration regression for the native run tool using production Git setup. +// Integration regression for the native run tool: a child run targets the +// branch its parent pushed to. use std::collections::HashMap; use std::path::Path; use std::process::Command; @@ -81,26 +82,22 @@ async fn run_create_child_checkout_contains_the_parents_pushed_work() { repo: "acme/widgets".to_owned(), branch: "main".to_owned(), tag: Some("v1.0.0".to_owned()), - sha: Some(base_sha), + sha: Some(base_sha.clone()), }))); - let sandbox = fabro_sandbox::local_sandbox(&workspace).await.unwrap(); - // Docker and Daytona use this same setup operation to create the run branch. - let git = fabro_sandbox::setup_git(&sandbox, &fabro_sandbox::GitSetupIntent::NewRun { - run_id: parent.spec.id().to_string(), - }) - .await - .unwrap(); + // The run branch the engine's checkout creates for the parent. + let run_branch = format!("fabro/run/{}", parent.spec.id()); + run_git(&workspace, &["checkout", "--quiet", "-b", &run_branch]); parent.start = Some(fabro_types::StartRecord { start_time: chrono::Utc::now(), - run_branch: Some(git.run_branch.clone()), - base_sha: Some(git.base_sha), + run_branch: Some(run_branch.clone()), + base_sha: Some(base_sha.clone()), }); fs::write(workspace.join("result.txt"), "parent implementation") .await .unwrap(); run_git(&workspace, &["add", "."]); run_git(&workspace, &["commit", "--quiet", "-m", "implement"]); - run_git(&workspace, &["push", "--quiet", "origin", &git.run_branch]); + run_git(&workspace, &["push", "--quiet", "origin", &run_branch]); let server = MockServer::start_async().await; let state_request = mock_parent(&server, &parent).await; let client = fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); diff --git a/lib/components/fabro-sandbox/Cargo.toml b/lib/components/fabro-sandbox/Cargo.toml index 093047e01..c36d72404 100644 --- a/lib/components/fabro-sandbox/Cargo.toml +++ b/lib/components/fabro-sandbox/Cargo.toml @@ -24,7 +24,6 @@ sandbox-driver-host.workspace = true sandbox-driver-docker.workspace = true sandbox-driver-docker-config.workspace = true sandbox-driver-daytona.workspace = true -sandbox-driver-daytona-config.workspace = true sandbox-driver-testing = { workspace = true, optional = true } pebble-coding-agent.workspace = true anyhow.workspace = true @@ -32,14 +31,9 @@ async-trait.workspace = true thiserror.workspace = true tokio.workspace = true tokio-util = { workspace = true, features = ["compat"] } -serde.workspace = true serde_json.workspace = true -strum.workspace = true tracing.workspace = true reqwest.workspace = true -base64.workspace = true -uuid.workspace = true -fabro-proc = { path = "../../foundation/fabro-proc" } fabro-static.workspace = true fabro-util = { path = "../../foundation/fabro-util" } fabro-redact.workspace = true @@ -49,9 +43,8 @@ futures = { workspace = true } fabro-github = { path = "../fabro-github" } fabro-types = { path = "../../foundation/fabro-types" } -chrono = { workspace = true } - [dev-dependencies] +chrono = { workspace = true } fabro-github = { path = "../fabro-github", features = ["test-support"] } pebble-coding-agent = { workspace = true, features = ["test-util"] } sandbox-driver-testing.workspace = true diff --git a/lib/components/fabro-sandbox/src/clone.rs b/lib/components/fabro-sandbox/src/clone.rs deleted file mode 100644 index 0d0f38226..000000000 --- a/lib/components/fabro-sandbox/src/clone.rs +++ /dev/null @@ -1,380 +0,0 @@ -//! Fabro's clone orchestration over the sandbox-driver [`Git`] and [`Exec`] -//! facets. -//! -//! The driver clones; fabro decides what to clone, where it lands, which -//! credentials it carries, and how failures retry. The layout is fabro's: -//! the repository checks out under `//` and the -//! run works in `/`, a symlink to the checkout. An -//! exact commit or a tag is pinned by the driver's clone options, which -//! fetch the pin directly and attach the branch to it; an unavailable pin -//! fails the clone and never falls back to the branch head. The GitHub App -//! token travels with the clone per call and is then installed as the -//! checkout's ambient credentials, so the agent's own git commands can -//! push; the remote URL never carries it. - -use std::time::Duration; - -use fabro_types::SandboxProviderKind; -use sandbox_driver::{ - ExecResult, Git as _, GitCloneOptions, GitFailureKind, Sandbox as DriverHandle, -}; -use tokio::time; - -use crate::clone_source::{self, GitHubRepoLayout}; -use crate::credentials::{self, RepoCredentials}; -use crate::exec::{ExecResultExt, SandboxExec}; -use crate::git_policy; - -/// Whole-clone budget, shared by every network and local step. -pub(crate) const GIT_CLONE_TIMEOUT: Duration = Duration::from_mins(5); - -/// What the operator hears when the image has no `git`: the driver classifies -/// the failing command, and fabro names the fix. -const GIT_UNAVAILABLE_MESSAGE: &str = "The sandbox image must include git for repository \ - clone and git lifecycle operations. Use an image with \ - bash and git, such as buildpack-deps:noble."; - -/// A GitHub clone fabro decided to perform. -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct GitHubClone { - pub(crate) origin_url: String, - pub(crate) branch: Option, - pub(crate) tag: Option, - pub(crate) commit_sha: Option, - pub(crate) depth: Option, -} - -/// What the clone left behind: the layout it checked out into. -pub(crate) struct CloneOutcome { - pub(crate) layout: GitHubRepoLayout, -} - -/// Whether a failing git step talked to the remote. Local steps cannot fail -/// on credentials, so they must not suggest reconfiguring the GitHub App. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum CloneStep { - Network, - Local, -} - -/// Clone `plan` into `handle`, laid out under `workspace_root` and -/// `repos_root`, with a GitHub App token from `credentials` when one is -/// available: the clone carries it per call, and the checkout keeps it as -/// ambient credentials afterwards. -pub(crate) async fn clone_github_repo( - kind: &SandboxProviderKind, - handle: &dyn DriverHandle, - exec: &SandboxExec<'_>, - plan: &GitHubClone, - workspace_root: &str, - repos_root: &str, - credentials: &RepoCredentials, -) -> crate::Result { - let layout = clone_source::github_repo_layout(&plan.origin_url, workspace_root, repos_root)?; - let token = credentials.mint_for_clone().await?; - - let fs = handle.fs(); - for dir in [workspace_root, layout.repos_owner_path.as_str()] { - fs.create_dir(dir) - .await - .map_err(|error| crate::Error::context(format!("Failed to create {dir}"), error))?; - } - - let deadline = time::Instant::now() + GIT_CLONE_TIMEOUT; - let has_app = credentials.managed(); - let git = handle.git().ok_or_else(|| { - crate::Error::message(format!( - "sandbox provider `{kind}` does not support git operations" - )) - })?; - // `decide_clone` already requires a branch for a pin; the branch names - // the checkout the run works on, and the driver attaches it to the - // pinned commit or tag. - let mut options = GitCloneOptions::default(); - options.branch = plan - .branch - .clone() - .filter(|branch| !branch.trim().is_empty()); - options.commit = plan.commit_sha.clone(); - options.tag = plan.tag.clone().filter(|_| plan.commit_sha.is_none()); - options.depth = plan.depth; - options.credentials = token.as_ref().map(credentials::git_credentials); - // The driver retries a clone the remote refused while the token may - // still be replicating, inside what is left of the clone budget. - let policy = git_policy::clone_policy(deadline.saturating_duration_since(time::Instant::now())); - let target = layout.primary_repo_path.clone(); - sandbox_driver::retry_git( - &policy, - options.credentials.as_ref(), - "git clone", - |_attempt, _timeout| { - let git = &git; - let options = &options; - let target = ⌖ - let origin_url = &plan.origin_url; - async move { git.clone_repo(origin_url, target, options).await } - }, - ) - .await - .map_err(|failure| { - clone_failure_error( - crate::Error::from(failure.error), - CloneStep::Network, - has_app, - ) - })?; - - run_local_step( - exec, - &clone_source::repo_symlink_command(&layout), - "create workspace repo symlink", - deadline, - has_app, - ) - .await?; - - if let Some(token) = &token { - RepoCredentials::install(&git, &layout.primary_repo_path, token).await?; - } - Ok(CloneOutcome { layout }) -} - -/// Run a local (non-network) step under the shared clone deadline. -/// -/// Materializing a large working tree takes far longer than the short fixed -/// timeout used for trivial commands, so these steps get the same budget the -/// network steps have. -async fn run_local_step( - exec: &SandboxExec<'_>, - command: &str, - label: &'static str, - deadline: time::Instant, - has_app: bool, -) -> crate::Result { - let remaining = deadline.saturating_duration_since(time::Instant::now()); - if remaining.is_zero() { - return Err(crate::Error::message(format!( - "{label} deadline expired before the step could run" - ))); - } - let result = exec - .run(command, Some(remaining), Some("/"), None, None) - .await - .map_err(|error| crate::Error::context(format!("{label} transport failed"), error))?; - if result.success() { - return Ok(result); - } - Err(clone_failure_error( - result.into_exec_error(label), - CloneStep::Local, - has_app, - )) -} - -fn clone_failure_error(error: crate::Error, step: CloneStep, has_app: bool) -> crate::Error { - if git_unavailable(&error) { - return crate::Error::context(GIT_UNAVAILABLE_MESSAGE, error); - } - let message = match step { - CloneStep::Network if !has_app => { - "Git clone failed. If this is a private repository, configure a GitHub App with \ - `fabro install` and install it for your organization." - } - CloneStep::Network => "Failed to clone repository into the sandbox", - CloneStep::Local => "Failed to prepare the cloned repository in the sandbox", - }; - crate::Error::context(message, error) -} - -/// Whether the driver found no usable `git` in the sandbox. -fn git_unavailable(error: &crate::Error) -> bool { - matches!( - error.driver(), - Some(sandbox_driver::Error::Git(failure)) - if failure.kind() == GitFailureKind::GitUnavailable - ) -} - -#[cfg(test)] -mod tests { - use fabro_github::token_source::InstallationTokenSource; - use sandbox_driver::{ExecFailure, GitFailure, Termination}; - use sandbox_driver_testing::ScriptedSandbox; - - use super::*; - - const ORIGIN: &str = "https://github.com/acme/widgets"; - - fn ok() -> ExecResult { - ExecResult::new(Termination::Exited, Some(0), Duration::from_millis(1)) - } - - /// A scripted sandbox whose `origin` answers with the fixture URL and - /// whose every other command succeeds. - fn scripted_handle() -> ScriptedSandbox { - let handle = ScriptedSandbox::with_id_and_working_dir("scripted", "/workspace") - .runtime_directory("/tmp/sandbox-driver/runtime"); - handle.scripted_exec().respond_with(|spec| { - let script = spec.args.last().map(String::as_str).unwrap_or_default(); - script.contains("'remote' 'get-url' 'origin'").then(|| { - let mut result = ok(); - result.stdout = format!("{ORIGIN}\n").into_bytes(); - result - }) - }); - handle.scripted_exec().set_default(ok()); - handle - } - - fn plan() -> GitHubClone { - GitHubClone { - origin_url: ORIGIN.to_owned(), - branch: Some("main".to_owned()), - tag: None, - commit_sha: None, - depth: Some(1), - } - } - - async fn clone_with(handle: &ScriptedSandbox, credentials: &RepoCredentials) -> CloneOutcome { - let exec = SandboxExec::new(handle.exec()); - clone_github_repo( - &SandboxProviderKind::DOCKER, - handle, - &exec, - &plan(), - "/workspace", - "/repos", - credentials, - ) - .await - .expect("clone succeeds") - } - - #[tokio::test] - async fn a_clone_carries_the_token_per_call_and_installs_it_for_the_checkout() { - let handle = scripted_handle(); - let credentials = - RepoCredentials::new(Some(InstallationTokenSource::pat("ghp_test".to_owned()))); - - let outcome = clone_with(&handle, &credentials).await; - assert_eq!(outcome.layout.primary_repo_path, "/repos/acme/widgets"); - - let commands = handle.scripted_exec().commands(); - assert!( - commands - .iter() - .all(|command| !command.contains("git --version")), - "no probe runs ahead of the clone: {commands:#?}" - ); - assert!( - commands.iter().all(|command| !command.contains("set-url")), - "the remote URL is never rewritten: {commands:#?}" - ); - let clone = commands - .iter() - .find(|command| command.contains("'clone'")) - .expect("the clone ran"); - assert!( - clone.contains( - "x-access-token:ghp_test@github.com/acme/widgets.insteadOf=https://github.com/acme/widgets" - ), - "the clone carries the token per call: {clone}" - ); - assert!( - commands.iter().any(|command| command.starts_with("ln -s ")), - "{commands:#?}" - ); - let install = commands - .iter() - .find(|command| command.contains("--add credential.helper")) - .expect("the checkout's credential store is installed"); - assert!( - install.contains("/tmp/sandbox-driver/runtime/git-credentials/"), - "{install}" - ); - assert!( - commands - .iter() - .all(|command| !command.contains("ghp_test") || command.contains("insteadOf")), - "the secret enters no command but the clone's own rewrite: {commands:#?}" - ); - assert!( - handle.scripted_exec().recorded().iter().any(|spec| { - spec.env - .get("SANDBOX_DRIVER_GIT_CREDENTIAL") - .map(String::as_str) - == Some("https://x-access-token:ghp_test@github.com") - }), - "the store line travels in the environment" - ); - } - - #[tokio::test] - async fn a_clone_without_managed_credentials_installs_nothing() { - let handle = scripted_handle(); - - clone_with(&handle, &RepoCredentials::none()).await; - - let commands = handle.scripted_exec().commands(); - assert!( - commands.iter().any(|command| command.contains("'clone'")), - "{commands:#?}" - ); - assert!( - commands - .iter() - .all(|command| !command.contains("insteadOf") - && !command.contains("credential.helper")), - "{commands:#?}" - ); - } - - fn git_failure(exit_code: i32, stderr: &str) -> crate::Error { - crate::Error::from(sandbox_driver::Error::Git(GitFailure::from_command( - "git clone", - ExecFailure::new( - "git clone", - Termination::Exited, - Some(exit_code), - Vec::new(), - stderr.as_bytes().to_vec(), - ), - ))) - } - - #[test] - fn a_missing_git_executable_names_the_image_requirement() { - let error = clone_failure_error( - git_failure(127, "bash: line 1: git: command not found"), - CloneStep::Network, - true, - ); - assert!( - error.to_string().contains("image must include git"), - "{error}" - ); - } - - #[test] - fn other_network_failures_keep_the_credential_guidance() { - let without_app = clone_failure_error( - git_failure(128, "remote: Repository not found."), - CloneStep::Network, - false, - ); - assert!(without_app.to_string().contains("fabro install")); - let with_app = clone_failure_error( - git_failure(128, "remote: Repository not found."), - CloneStep::Network, - true, - ); - assert!( - with_app - .to_string() - .contains("Failed to clone repository into the sandbox") - ); - let local = clone_failure_error(git_failure(1, "ln: failed"), CloneStep::Local, true); - assert!(local.to_string().contains("prepare the cloned repository")); - } -} diff --git a/lib/components/fabro-sandbox/src/clone_source.rs b/lib/components/fabro-sandbox/src/clone_source.rs index dbd0e3264..b91c1a96b 100644 --- a/lib/components/fabro-sandbox/src/clone_source.rs +++ b/lib/components/fabro-sandbox/src/clone_source.rs @@ -1,5 +1,3 @@ -use fabro_util::shell; - use crate::sandbox; #[derive(Clone, Debug, PartialEq, Eq)] @@ -17,12 +15,8 @@ pub(crate) enum CloneDecision { #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct GitHubRepoLayout { - pub(crate) owner: String, - pub(crate) repo: String, - pub(crate) repos_owner_path: String, - pub(crate) primary_repo_path: String, - pub(crate) primary_repo_link: String, - pub(crate) execution_directory: String, + pub(crate) primary_repo_path: String, + pub(crate) primary_repo_link: String, } pub(crate) fn github_repo_layout( @@ -45,11 +39,7 @@ pub(crate) fn github_repo_layout( let primary_repo_link = sandbox::join_sandbox_path(workspace_root, &repo); Ok(GitHubRepoLayout { - owner, - repo, - repos_owner_path, primary_repo_path, - execution_directory: primary_repo_link.clone(), primary_repo_link, }) } @@ -67,14 +57,6 @@ fn validate_path_component(label: &str, component: &str) -> crate::Result<()> { Ok(()) } -pub(crate) fn repo_symlink_command(layout: &GitHubRepoLayout) -> String { - format!( - "ln -s {} {}", - shell::shell_quote(&layout.primary_repo_path), - shell::shell_quote(&layout.primary_repo_link), - ) -} - /// The kind of revision a checkout is pinned to instead of the branch's /// current HEAD. /// @@ -444,12 +426,8 @@ mod tests { ) .unwrap(); - assert_eq!(layout.owner, "brynary"); - assert_eq!(layout.repo, "rack-test"); - assert_eq!(layout.repos_owner_path, "/repos/brynary"); assert_eq!(layout.primary_repo_path, "/repos/brynary/rack-test"); assert_eq!(layout.primary_repo_link, "/workspace/rack-test"); - assert_eq!(layout.execution_directory, "/workspace/rack-test"); } #[test] @@ -461,12 +439,8 @@ mod tests { ) .unwrap(); - assert_eq!(layout.owner, "fabro-sh"); - assert_eq!(layout.repo, "fabro"); - assert_eq!(layout.repos_owner_path, "/repos/fabro-sh"); assert_eq!(layout.primary_repo_path, "/repos/fabro-sh/fabro"); assert_eq!(layout.primary_repo_link, "/workspace/fabro"); - assert_eq!(layout.execution_directory, "/workspace/fabro"); } #[test] @@ -485,21 +459,6 @@ mod tests { } } - #[test] - fn repo_symlink_command_quotes_both_paths() { - let layout = github_repo_layout( - "https://github.com/fabro-sh/fabro", - "/work space", - "/repo root", - ) - .unwrap(); - - assert_eq!( - repo_symlink_command(&layout), - "ln -s '/repo root/fabro-sh/fabro' '/work space/fabro'" - ); - } - #[test] fn record_origin_strips_credentials() { assert_eq!( diff --git a/lib/components/fabro-sandbox/src/credentials.rs b/lib/components/fabro-sandbox/src/credentials.rs deleted file mode 100644 index a1cc6fbbe..000000000 --- a/lib/components/fabro-sandbox/src/credentials.rs +++ /dev/null @@ -1,154 +0,0 @@ -//! GitHub credentials for a clone-based sandbox's repository. -//! -//! Fabro decides which credential a checkout works with and when it is -//! renewed; the sandbox driver applies it. The facet's own network -//! operations, fabro's clone and pushes, take the token per call and never -//! write it into the repository. The agent's own git commands read it from -//! the credential store the driver installs beside the checkout, which the -//! workflow's refresh tick rewrites as the token is renewed. The remote URL -//! is never touched, so no secret shows in `git remote -v` or in -//! `.git/config`. The token cache itself sits below, in -//! [`InstallationTokenSource`]. - -use std::sync::Arc; -use std::time::SystemTime; - -use fabro_github::GitHubCredentials; -use fabro_github::token_source::{InstallationTokenSource, ResolvedToken}; -use sandbox_driver::{Git as _, GitCredentials, GitFacet}; - -/// The username GitHub expects with an installation token or PAT. -pub(crate) const GITHUB_TOKEN_USERNAME: &str = "x-access-token"; - -/// Build the shared installation-token source for a clone-based sandbox. -/// -/// Returns `None` when there are no managed credentials or no GitHub origin -/// to scope them to. Minted tokens carry the same `contents: write` -/// permission the clone token uses. -pub(crate) fn build_token_source( - github_app: Option<&GitHubCredentials>, - clone_origin_url: Option<&str>, -) -> crate::Result>> { - let Some(creds) = github_app else { - return Ok(None); - }; - let Some(origin_url) = clone_origin_url.filter(|url| !url.trim().is_empty()) else { - return Ok(None); - }; - let normalized = fabro_github::normalize_repo_origin_url(origin_url); - let Ok((owner, repo)) = fabro_github::parse_github_owner_repo(&normalized) else { - // Non-GitHub origins never clone in these providers, so there is no - // remote to keep credentials fresh for. - return Ok(None); - }; - InstallationTokenSource::for_repository( - creds, - owner, - repo, - serde_json::json!({ "contents": "write" }), - ) - .map(Some) - .map_err(|err| crate::Error::context_anyhow("Failed to build GitHub token source", err)) -} - -/// The GitHub credentials a run's checkout works with: a token source when -/// fabro manages them, nothing when the repository was cloned without a -/// GitHub App or the sandbox was reattached by a later process. -pub(crate) struct RepoCredentials { - source: Option>, -} - -impl RepoCredentials { - pub(crate) fn new(source: Option>) -> Self { - Self { source } - } - - /// No managed credentials: pushes and the agent's git commands use - /// whatever the checkout already has. - pub(crate) fn none() -> Self { - Self::new(None) - } - - pub(crate) fn managed(&self) -> bool { - self.source.is_some() - } - - /// Mint the clone token. Never a warm-cache reuse: a clone retried on - /// replication lag must hold the token minted for it. The mint seeds - /// the source, so later resolves reuse this token until it nears - /// expiry. - pub(crate) async fn mint_for_clone(&self) -> crate::Result> { - let Some(source) = &self.source else { - return Ok(None); - }; - source.mint_for_clone().await.map(Some).map_err(|err| { - crate::Error::context_anyhow("Failed to get GitHub App credentials for clone", err) - }) - } - - /// The token one operation works with, reused from the cache until it - /// nears expiry. A refresh that fails while the cached token is still - /// valid returns that token. - pub(crate) async fn resolve(&self) -> crate::Result> { - let Some(source) = &self.source else { - return Ok(None); - }; - source.resolve().await.map(Some).map_err(|err| { - crate::Error::context_anyhow("Failed to refresh GitHub App credentials", err) - }) - } - - /// Install `token` as the credentials every git command run inside the - /// sandbox picks up for the checkout at `repo_path`. The driver keeps - /// them in a credential store beside the checkout and points the - /// repository's helper configuration at it; calling again replaces - /// them in place. - pub(crate) async fn install( - git: &GitFacet<'_>, - repo_path: &str, - token: &ResolvedToken, - ) -> crate::Result<()> { - git.set_ambient_credentials(repo_path, Some(&git_credentials(token))) - .await - .map_err(|error| { - crate::Error::context("Failed to install the checkout's GitHub credentials", error) - }) - } -} - -/// The per-call form of `token` for the driver's network operations. The -/// mint time travels with a minted token so the driver's retry knows a -/// rejection may be replication lag; a static credential carries none. -pub(crate) fn git_credentials(token: &ResolvedToken) -> GitCredentials { - let credentials = GitCredentials::new(GITHUB_TOKEN_USERNAME, token.token.expose()); - match token.snapshot.minted_at() { - Some(minted_at) => credentials.minted_at(SystemTime::from(minted_at)), - None => credentials, - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[tokio::test] - async fn unmanaged_credentials_resolve_to_nothing() { - let credentials = RepoCredentials::none(); - assert!(!credentials.managed()); - assert!(credentials.mint_for_clone().await.unwrap().is_none()); - assert!(credentials.resolve().await.unwrap().is_none()); - } - - #[tokio::test] - async fn a_pat_becomes_per_call_credentials_under_the_github_username() { - let source = InstallationTokenSource::pat("ghp_static".to_owned()); - let token = source.resolve().await.unwrap(); - let credentials = git_credentials(&token); - assert_eq!(credentials.username, GITHUB_TOKEN_USERNAME); - assert_eq!(credentials.password, "ghp_static"); - assert!( - credentials.minted_at.is_none(), - "a static credential has no mint time" - ); - } -} diff --git a/lib/components/fabro-sandbox/src/daytona.rs b/lib/components/fabro-sandbox/src/daytona.rs index 952892a28..6faf88bae 100644 --- a/lib/components/fabro-sandbox/src/daytona.rs +++ b/lib/components/fabro-sandbox/src/daytona.rs @@ -343,98 +343,3 @@ mod tests { ); } } - -/// The git clone contract over the plugin wire against live Daytona. -/// -/// Host and Docker derive their git facet from `Exec`, so only Daytona -/// exercises the driver's native clone through the JSON-RPC protocol. The -/// provider is served over an in-process duplex pipe exactly as a plugin -/// executable would serve it on stdio. -#[cfg(test)] -mod wire_gate { - use std::sync::Arc; - - use fabro_static::EnvVars; - use fabro_types::SandboxProviderKind; - use sandbox_driver::SandboxProvider; - use sandbox_driver_protocol::{PluginProvider, serve}; - use tokio::io::{duplex, split}; - - use super::*; - use crate::driver_sandbox::{LayoutSource, RepoWorkspace, RunSandbox}; - use crate::environment::CloneRequest; - - #[expect( - clippy::disallowed_methods, - reason = "the live gate takes Daytona credentials from the developer's environment" - )] - fn live_credentials() -> Option { - let api_key = std::env::var(EnvVars::DAYTONA_API_KEY).ok()?; - Some(DaytonaCredentials::from_api_key(api_key, |name| { - std::env::var(name).ok() - })) - } - - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - #[ignore = "requires live Daytona credentials and provisions a sandbox"] - async fn native_clone_over_the_wire_lays_out_the_repository() { - let credentials = live_credentials().expect("DAYTONA_API_KEY must be set"); - let in_process = connect(&credentials).await.expect("connect to Daytona"); - - let (host_side, plugin_side) = duplex(1024 * 1024); - let (host_read, host_write) = split(host_side); - let (plugin_read, plugin_write) = split(plugin_side); - tokio::spawn(serve(Arc::clone(&in_process), plugin_read, plugin_write)); - let remote = PluginProvider::connect(host_read, host_write) - .await - .expect("protocol handshake"); - assert_eq!(remote.kind().as_str(), "daytona"); - let remote: Arc = Arc::new(remote); - - let workspace = RepoWorkspace::plan( - LayoutSource::Fixed(layout()), - &CloneRequest { - origin_url: Some("https://github.com/brynary/rack-test".to_string()), - depth: Some(100), - ..CloneRequest::default() - }, - None, - ) - .expect("clone plan"); - // No image: the overlay creates from Daytona's default snapshot. - let spec = overlay(DriverSpec::new(SandboxSource::HostDirectory), None); - let sandbox = RunSandbox::pending(SandboxProviderKind::DAYTONA, remote, spec, workspace); - sandbox - .initialize() - .await - .expect("initialize over the wire"); - - let checks = async { - assert_eq!( - sandbox.working_directory(), - "/home/daytona/workspace/rack-test" - ); - let result = sandbox - .exec_command( - "test -d /home/daytona/repos/brynary/rack-test/.git && \ - test -L /home/daytona/workspace/rack-test && \ - git rev-parse --is-inside-work-tree", - 30_000, - None, - None, - None, - ) - .await - .expect("layout check"); - assert!(result.success(), "{result:?}"); - assert!(result.stdout_lossy().contains("true")); - let layout = sandbox.workspace_layout().expect("layout record"); - assert_eq!( - layout.primary_repo_path.as_deref(), - Some("/home/daytona/repos/brynary/rack-test") - ); - }; - checks.await; - sandbox.delete().await.expect("cleanup"); - } -} diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 6328c53f5..73487f4c3 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -15,29 +15,24 @@ use std::collections::HashMap; use std::path::Path; use std::sync::{Arc, OnceLock}; -use std::time::{Duration, Instant}; +use std::time::Duration; -use fabro_github::GitHubCredentials; -use fabro_github::token_source::TokenSnapshot; use fabro_types::SandboxProviderKind; use fabro_util::workspace_glob::WorkspaceGlob; use pebble_coding_agent::mcp::{PortRoute, PortRouteError, PortRoutes}; use sandbox_driver::{ DirEntry, EventContext, ExecControls, ExecResult, ExecSpec, ExecStreamingResult, FileKind, - GitRetryPolicy, GrepMatch, GrepOptions, PreviewUrls, PtyOptions, PtySession, PtySize, - Sandbox as DriverHandle, SandboxProvider as DriverProvider, SandboxSpec as DriverSpec, - SandboxState, Search as _, StdioProcess, WaitOptions, WalkOptions, + GrepMatch, GrepOptions, PreviewUrls, PtyOptions, PtySession, PtySize, Sandbox as DriverHandle, + SandboxProvider as DriverProvider, SandboxSpec as DriverSpec, SandboxState, Search as _, + StdioProcess, WaitOptions, WalkOptions, }; use tokio::sync::OnceCell; use tokio_util::sync::CancellationToken; -use crate::clone::{self, GitHubClone}; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; -use crate::credentials::{self, RepoCredentials}; use crate::environment::CloneRequest; use crate::exec::SandboxExec; -use crate::sandbox::{self, PushError, PushReport, SandboxFile, SandboxWorkspaceLayout}; -use crate::{GitRunInfo, GitSetupIntent}; +use crate::sandbox::{self, SandboxFile, SandboxWorkspaceLayout}; /// Where a clone-based provider puts its files: the run works under /// `workspace_root`, and repositories check out under `repos_root`. @@ -70,22 +65,18 @@ pub(crate) enum LayoutSource { /// What `initialize` does to the workspace once the sandbox runs. enum WorkspacePlan { - /// Clone this GitHub repository into the layout. - Clone(GitHubClone), /// Create the empty workspace root and nothing else. Empty(EmptyWorkspaceReason), /// The workspace was prepared by an earlier process; leave it alone. Attached, } -/// The run's workspace on a sandbox: the layout, the clone fabro performs -/// into it (if any), and the GitHub credentials its checkout carries. A -/// workspace fabro did not clone into is still a checkout the run may push -/// from, with whatever credentials the checkout carries itself. +/// The run's workspace on a sandbox: the layout and what fabro does to it +/// at `initialize`. Fabro no longer clones into a sandbox; a workspace an +/// earlier process prepared is described by the run record. pub(crate) struct RepoWorkspace { layout: OnceLock, plan: WorkspacePlan, - credentials: RepoCredentials, repo_cloned: OnceLock, origin_url: OnceLock, /// The directory the run works in once known: the repository link for a @@ -97,14 +88,11 @@ pub(crate) struct RepoWorkspace { } impl RepoWorkspace { - /// Decide the clone for a new sandbox. Fails before any provider call + /// Plan the workspace for a new sandbox. Fails before any provider call /// when the selectors are inconsistent (a pin without a branch, a - /// non-GitHub origin without `skip`). - pub(crate) fn plan( - layout: LayoutSource, - clone: &CloneRequest, - github_app: Option<&GitHubCredentials>, - ) -> crate::Result { + /// non-GitHub origin without `skip`), and when the request asks for a + /// clone: fabro no longer clones into a sandbox. + pub(crate) fn plan(layout: LayoutSource, clone: &CloneRequest) -> crate::Result { let decision = clone_source::decide_clone( clone.skip, clone.origin_url.as_deref(), @@ -112,10 +100,6 @@ impl RepoWorkspace { clone.tag.as_deref(), clone.commit_sha.as_deref(), )?; - let credentials = RepoCredentials::new(credentials::build_token_source( - github_app, - clone.origin_url.as_deref(), - )?); let plan = match decision { CloneDecision::EmptyWorkspace { reason } => WorkspacePlan::Empty(reason), CloneDecision::GitHub { @@ -123,18 +107,17 @@ impl RepoWorkspace { branch, tag, commit_sha, - } => WorkspacePlan::Clone(GitHubClone { - origin_url, - branch, - tag, - commit_sha, - depth: clone.depth, - }), + } => { + return Err(crate::Error::message(format!( + "fabro no longer clones a repository into a sandbox (requested {origin_url}, \ + branch {branch:?}, tag {tag:?}, commit {commit_sha:?}); the run's checkout \ + is prepared by the engine" + ))); + } }; Ok(Self { layout: layout.into_cell(), plan, - credentials, repo_cloned: OnceLock::new(), origin_url: OnceLock::new(), execution_directory: OnceLock::new(), @@ -143,8 +126,7 @@ impl RepoWorkspace { } /// A workspace prepared by an earlier process, described by the run - /// record. Pushes from a reattached sandbox use whatever credentials the - /// checkout's credential store already carries. + /// record. pub(crate) fn attached( layout: LayoutSource, repo_cloned: bool, @@ -154,7 +136,6 @@ impl RepoWorkspace { let workspace = Self { layout: layout.into_cell(), plan: WorkspacePlan::Attached, - credentials: RepoCredentials::none(), repo_cloned: OnceLock::new(), origin_url: OnceLock::new(), execution_directory: OnceLock::new(), @@ -178,7 +159,6 @@ impl RepoWorkspace { Self { layout: LayoutSource::ProviderWorkingDirectory.into_cell(), plan: WorkspacePlan::Attached, - credentials: RepoCredentials::none(), repo_cloned: OnceLock::new(), origin_url: OnceLock::new(), execution_directory: OnceLock::new(), @@ -461,8 +441,8 @@ impl RunSandbox { self.learn_platform().await } - /// Prepare the workspace after the sandbox runs for the first time: - /// an empty root, or fabro's clone. + /// Prepare the workspace after the sandbox runs for the first time: an + /// empty root. async fn prepare_workspace(&self) -> crate::Result<()> { let workspace = &self.workspace; let layout = workspace @@ -494,55 +474,6 @@ impl RunSandbox { .set(layout.workspace_root.clone()); Ok(()) } - WorkspacePlan::Clone(plan) => { - tracing::debug!( - url = plan.origin_url.as_str(), - branch = plan.branch.as_deref().unwrap_or(""), - "Git clone started" - ); - let started = Instant::now(); - let handle = self.handle()?; - // The clone names every directory it touches, so it runs - // without fabro's working-directory override. - let exec = SandboxExec::new(handle.exec()); - let outcome = clone::clone_github_repo( - &self.kind, - handle.as_ref(), - &exec, - plan, - &layout.workspace_root, - &layout.repos_root, - &workspace.credentials, - ) - .await; - match outcome { - Ok(outcome) => { - let _ = workspace.repo_cloned.set(true); - let _ = workspace.origin_url.set(plan.origin_url.clone()); - let _ = workspace - .checkout_path - .set(outcome.layout.primary_repo_path.clone()); - let _ = workspace - .execution_directory - .set(outcome.layout.execution_directory.clone()); - tracing::debug!( - url = plan.origin_url.as_str(), - duration_ms = elapsed_ms(started), - "Git clone completed" - ); - Ok(()) - } - Err(error) => { - tracing::error!( - url = plan.origin_url.as_str(), - error = %error, - causes = ?error.causes(), - "Git clone failed" - ); - Err(error) - } - } - } } } @@ -834,7 +765,7 @@ impl RunSandbox { } /// Create the sandbox when it is pending, bring it to `Running`, and - /// prepare fabro's workspace (empty root or clone) on first use. + /// prepare fabro's empty workspace root on first use. pub async fn initialize(&self) -> crate::Result<()> { self.ensure_created().await?; self.make_ready().await?; @@ -876,8 +807,9 @@ impl RunSandbox { self.release().await } - /// The directory the run works in: the cloned repository's link for a - /// clone-based workspace, the provider's working directory otherwise. + /// The directory the run works in: the repository link for a workspace + /// an earlier process cloned into, the provider's working directory + /// otherwise. pub fn working_directory(&self) -> &str { self.workspace .working_directory() @@ -922,44 +854,6 @@ impl RunSandbox { self.workspace.record() } - pub async fn setup_git(&self, intent: &GitSetupIntent) -> crate::Result> { - if !self.repo_cloned() { - return Ok(None); - } - sandbox::setup_git(self, intent).await.map(Some) - } - - /// Push `refspec` from the run's checkout. A checkout fabro cloned - /// pushes with the credentials it was cloned with. Any other checkout - /// pushes only when it has an origin, with whatever credentials it - /// carries itself; a workspace without one has nothing to push. - pub async fn git_push_ref( - &self, - refspec: &str, - policy: &GitRetryPolicy, - ) -> Result { - let workspace = &self.workspace; - if workspace.repo_cloned() { - return sandbox::git_push(self, Some(&workspace.credentials), refspec, policy).await; - } - let has_origin = match self - .exec_command("git remote get-url origin", 10_000, None, None, None) - .await - { - Ok(result) => result.success(), - Err(err) => { - return Err(PushError { - report: PushReport::default(), - error: crate::Error::context("git remote get-url origin", err), - }); - } - }; - if !has_origin { - return Ok(PushReport::default()); - } - sandbox::git_push(self, None, refspec, policy).await - } - pub fn origin_url(&self) -> Option<&str> { if !self.workspace.repo_cloned() { return None; @@ -967,24 +861,6 @@ impl RunSandbox { self.workspace.origin_url.get().map(String::as_str) } - /// Renew the credentials the agent's own git commands read for the - /// checkout: resolve the current token and rewrite the checkout's - /// credential store with it. Returns the token's non-secret description, - /// or `None` when this sandbox has no managed credentials or no - /// checkout to install them in. - #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] - pub async fn refresh_ambient_credentials(&self) -> crate::Result> { - let workspace = &self.workspace; - let Some(checkout) = workspace.checkout_path.get() else { - return Ok(None); - }; - let Some(token) = workspace.credentials.resolve().await? else { - return Ok(None); - }; - RepoCredentials::install(&self.git()?, checkout, &token).await?; - Ok(Some(token.snapshot)) - } - /// The local command that opens a shell in the sandbox, from the /// provider's access facet. `None` when the provider has no such /// command (the local sandbox is the host). @@ -1074,10 +950,6 @@ impl PortRoutes for SandboxPortRoutes { } impl RunSandbox { - fn repo_cloned(&self) -> bool { - self.workspace.repo_cloned() - } - /// Delete the sandbox on the provider. A pending sandbox that was never /// created has nothing to release. async fn release(&self) -> crate::Result<()> { @@ -1089,10 +961,6 @@ impl RunSandbox { } } -fn elapsed_ms(started: Instant) -> u64 { - u64::try_from(started.elapsed().as_millis()).unwrap_or(u64::MAX) -} - #[cfg(test)] mod tests { use std::sync::Mutex; diff --git a/lib/components/fabro-sandbox/src/environment.rs b/lib/components/fabro-sandbox/src/environment.rs index 76461fbce..7abdd744d 100644 --- a/lib/components/fabro-sandbox/src/environment.rs +++ b/lib/components/fabro-sandbox/src/environment.rs @@ -5,9 +5,9 @@ //! the same driver [`SandboxSpec`] built here; a bundled provider adds only //! what its backend needs on top (the Docker working directory and default //! image, the Daytona snapshot and timers) in its own overlay, and the -//! ownership scope adds fabro's labels. The clone policy travels beside the -//! spec as a [`CloneRequest`]: cloning is fabro's work once the sandbox -//! exists, not the provider's. +//! ownership scope adds fabro's labels. The clone request travels beside +//! the spec as a [`CloneRequest`]: fabro validates and records it, and +//! refuses one that asks for a clone. use std::collections::BTreeMap; @@ -19,7 +19,9 @@ use sandbox_driver::{ Capabilities, LifecycleTimers, NetworkPolicy, Resources, SandboxSource, SandboxSpec, }; -/// What to clone into a provider sandbox, if anything. +/// The repository a provider sandbox is named for, if any. Fabro validates +/// and records the request; it no longer clones, so a request that asks +/// for a clone is refused when the sandbox is planned. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct CloneRequest { pub origin_url: Option, diff --git a/lib/components/fabro-sandbox/src/git_policy.rs b/lib/components/fabro-sandbox/src/git_policy.rs index 2f1bff0c3..5c4a39e04 100644 --- a/lib/components/fabro-sandbox/src/git_policy.rs +++ b/lib/components/fabro-sandbox/src/git_policy.rs @@ -1,12 +1,12 @@ -//! Fabro's retry budgets for git operations against GitHub. +//! Fabro's retry budget for git operations against GitHub. //! //! The driver owns the retry loop and the decision //! ([`sandbox_driver::retry_git`]): a remote that cannot be reached is retried, //! a rejected credential is retried only while the token is fresh enough to //! still be replicating to GitHub's git endpoints, a static credential fails //! fast, and a command whose outcome is unknown is never replayed. Fabro keeps -//! what is policy: how many attempts each operation gets, how long the -//! operation may take, and when the credential it pushes with was minted. +//! what is policy: how many attempts the host-side repository probe gets, +//! how it paces them, and when the credential it runs with was minted. //! //! Retries reuse the same token on purpose. Replication of a given token //! only makes progress, so each attempt strictly improves the odds, while @@ -18,20 +18,9 @@ use std::time::{Duration, SystemTime}; use fabro_github::token_source::TokenSnapshot; use sandbox_driver::{GitBackoff, GitCredentials, GitFailure, GitFailureKind, GitRetryPolicy}; -use serde::{Deserialize, Serialize}; -use crate::credentials::GITHUB_TOKEN_USERNAME; - -/// Why a failed git push attempt is safe to retry. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display)] -#[serde(rename_all = "snake_case")] -#[strum(serialize_all = "snake_case")] -pub enum GitRetryReason { - /// A recently minted token may not have reached every GitHub git endpoint. - TokenReplication, - /// The failure came from transient network or service infrastructure. - TransientInfra, -} +/// The username GitHub expects with an installation token or PAT. +const GITHUB_TOKEN_USERNAME: &str = "x-access-token"; /// Backoff between attempts: 3s, then 9s. /// @@ -42,53 +31,13 @@ fn replication_backoff() -> GitBackoff { GitBackoff::new(Duration::from_secs(3), 3.0, Duration::from_secs(10)) } -/// The clone policy: 3 attempts at replication pacing, inside whatever is -/// left of the whole-clone budget. -pub(crate) fn clone_policy(remaining: Duration) -> GitRetryPolicy { - GitRetryPolicy::new(3, replication_backoff()).max_elapsed(remaining) -} - -/// Host-side repository probes use the clone's attempt count and pacing, -/// with no deadline of their own. +/// Host-side repository probes get 3 attempts at replication pacing, with +/// no deadline of their own. #[must_use] pub fn repository_probe_policy() -> GitRetryPolicy { GitRetryPolicy::new(3, replication_backoff()) } -/// Checkpoint pushes stay cheap: the next checkpoint re-pushes the same -/// branch anyway. Worst case about 90 seconds of wall clock. -#[must_use] -pub fn checkpoint_push_policy() -> GitRetryPolicy { - GitRetryPolicy::new(3, replication_backoff()) - .max_elapsed(Duration::from_secs(90)) - .per_attempt_timeout(Duration::from_mins(1)) -} - -/// The terminal publish push guards the whole run's value, so it gets a -/// real budget: 5 attempts with growing backoff (about 3s, 10s, 33s, 60s), -/// bounded at 4 minutes of wall clock. The bound must stay under the token -/// source's `REFRESH_MARGIN` (see the margin-invariant test) so a pinned -/// token always outlives the operation. -#[must_use] -pub fn publish_push_policy() -> GitRetryPolicy { - GitRetryPolicy::new( - 5, - GitBackoff::new(Duration::from_secs(3), 10.0 / 3.0, Duration::from_mins(1)), - ) - .max_elapsed(Duration::from_mins(4)) - .per_attempt_timeout(Duration::from_mins(1)) -} - -/// The reason fabro records for a driver retry reason. A reason this build -/// does not know still retried the attempt, so it is recorded under the -/// broader class. -pub(crate) fn recorded_reason(reason: sandbox_driver::GitRetryReason) -> GitRetryReason { - match reason { - sandbox_driver::GitRetryReason::TokenReplication => GitRetryReason::TokenReplication, - _ => GitRetryReason::TransientInfra, - } -} - /// Credentials carrying only the token's mint time, which is all the /// driver's decision reads for git that ran outside a sandbox. The token /// itself never leaves its snapshot. @@ -112,19 +61,6 @@ fn classified_failure(operation: &str, message: &str) -> sandbox_driver::Error { )) } -/// Whether a rendered git failure `message` is worth retrying with the -/// token behind `snapshot`: `None` means the failure is permanent for -/// these credentials or unrecognized. -#[must_use] -pub fn transient_git_failure( - message: &str, - snapshot: Option<&TokenSnapshot>, -) -> Option { - let credentials = credential_age(snapshot); - sandbox_driver::retry_reason(&classified_failure("git", message), credentials.as_ref()) - .map(recorded_reason) -} - /// Runs a host-side git operation that reports failures as rendered /// messages under `policy`, retrying while the driver's decision says the /// message is transient for the token behind `snapshot`. The final failure @@ -172,7 +108,7 @@ where #[cfg(test)] mod tests { use chrono::Utc; - use fabro_github::token_source::{REFRESH_MARGIN, TokenProvenance}; + use fabro_github::token_source::TokenProvenance; use super::*; @@ -195,60 +131,10 @@ mod tests { } #[test] - fn not_found_follows_the_credential_age() { - let message = "repository not found: Repository not found."; - assert_eq!( - transient_git_failure(message, Some(&snapshot(Duration::from_secs(5)))), - Some(GitRetryReason::TokenReplication) - ); - assert_eq!( - transient_git_failure(message, Some(&snapshot(Duration::from_mins(2)))), - Some(GitRetryReason::TransientInfra) - ); - assert_eq!( - transient_git_failure(message, Some(&static_snapshot())), - None - ); - assert_eq!(transient_git_failure(message, None), None); - } - - #[test] - fn infrastructure_failures_retry_without_credentials() { - assert_eq!( - transient_git_failure("fatal: unable to access: Could not resolve host", None), - Some(GitRetryReason::TransientInfra) - ); - assert_eq!( - transient_git_failure("fatal: something else entirely", None), - None - ); - } - - /// `REFRESH_MARGIN` must exceed every push policy's `max_elapsed`: a - /// push resolves its token once, and the token the source returns has - /// at least the margin of validity left, so the pinned token outlives - /// the operation. - #[test] - fn refresh_margin_exceeds_every_push_policy_elapsed_bound() { - for policy in [checkpoint_push_policy(), publish_push_policy()] { - let max_elapsed = policy.max_elapsed.expect("push policies are bounded"); - assert!( - REFRESH_MARGIN > max_elapsed, - "margin invariant violated: {max_elapsed:?}" - ); - } - } - - #[test] - fn publish_backoff_grows_toward_a_one_minute_cap() { - let backoff = publish_push_policy().backoff; + fn probe_backoff_paces_at_replication_intervals() { + let backoff = repository_probe_policy().backoff; assert_eq!(backoff.delay_after(1), Duration::from_secs(3)); - assert_eq!(backoff.delay_after(2), Duration::from_secs(10)); - assert_eq!(backoff.delay_after(4), Duration::from_mins(1)); - assert_eq!( - repository_probe_policy().backoff.delay_after(2), - Duration::from_secs(9) - ); + assert_eq!(backoff.delay_after(2), Duration::from_secs(9)); } #[tokio::test(start_paused = true)] diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index 1a4253db7..cad02ba8e 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -10,8 +10,6 @@ mod git_policy; mod managed_labels; -mod credentials; - pub mod details; pub mod driver; @@ -23,7 +21,6 @@ mod pebble_environment; pub mod reconnect; mod redact; -mod clone; pub mod docker; pub mod provider_sandbox; @@ -46,17 +43,13 @@ pub use fabro_github::token_source::{ InstallationTokenSource, ResolvedToken, TokenProvenance, TokenSnapshot, }; pub use fabro_types::{RunSandboxInstance, SandboxProviderKind}; -pub use git_policy::{ - GitRetryReason, checkpoint_push_policy, publish_push_policy, repository_probe_policy, - retry_git_messages, transient_git_failure, -}; +pub use git_policy::{repository_probe_policy, retry_git_messages}; pub use provider::{SandboxInventory, SandboxLookupError}; pub use provider_sandbox::{attach_provider_sandbox, local_sandbox, provider_sandbox}; pub use reconnect::{open_terminal_for_run, reconnect_for_run}; pub use redact::SecretRedactor; pub use sandbox::{ - DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport, - SandboxFile, SandboxWorkspaceLayout, redacted_output_tail, setup_git, + DEFAULT_EXEC_OUTPUT_TAIL_BYTES, SandboxFile, SandboxWorkspaceLayout, redacted_output_tail, }; /// Driver types a run sandbox speaks: what a command is and how it ended, /// what the file and search operations return, and what an environment diff --git a/lib/components/fabro-sandbox/src/provider_sandbox.rs b/lib/components/fabro-sandbox/src/provider_sandbox.rs index a9a4f078f..c447ad40b 100644 --- a/lib/components/fabro-sandbox/src/provider_sandbox.rs +++ b/lib/components/fabro-sandbox/src/provider_sandbox.rs @@ -13,7 +13,6 @@ use std::path::PathBuf; use std::sync::Arc; -use fabro_github::GitHubCredentials; use fabro_types::{BundledProvider, RunId, SandboxProviderKind}; use sandbox_driver::{ EventContext, OwnedProvider, SandboxId, SandboxProvider, SandboxSource, @@ -36,10 +35,9 @@ pub async fn provider_sandbox( access: &ProviderAccess, spec: DriverSpec, clone: &CloneRequest, - github_app: Option<&GitHubCredentials>, run_id: Option, ) -> crate::Result { - let workspace = RepoWorkspace::plan(layout_source(&kind), clone, github_app)?; + let workspace = RepoWorkspace::plan(layout_source(&kind), clone)?; let provider = connect(&kind, access, run_id.as_ref()).await?; let mut spec = spec; if let Some(run_id) = &run_id { @@ -88,8 +86,7 @@ async fn designate_directory(spec: &DriverSpec) -> crate::Result<()> { /// its [`SandboxSpec`] and initializes it itself. pub async fn local_sandbox(working_directory: impl Into) -> crate::Result { let spec = SandboxSpec::local(working_directory, ProviderAccess::default()); - let sandbox = - provider_sandbox(spec.kind, &spec.access, spec.spec, &spec.clone, None, None).await?; + let sandbox = provider_sandbox(spec.kind, &spec.access, spec.spec, &spec.clone, None).await?; sandbox.initialize().await?; Ok(sandbox) } diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 101a9092e..d998db0a6 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1,22 +1,7 @@ -use std::time::Duration; - -use chrono::{DateTime, Utc}; -use fabro_github::token_source::TokenSnapshot; -use sandbox_driver::{ - 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::git_policy::{self, GitRetryReason}; - -/// Git command prefix that disables background maintenance. +/// How much of each output stream a redacted tail keeps by default. pub const DEFAULT_EXEC_OUTPUT_TAIL_BYTES: usize = 8 * 1024; -/// Where a clone-based sandbox put its files, as persisted on the run. +/// Where a sandbox's workspace lives, as persisted on the run. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SandboxWorkspaceLayout { pub workspace_root: String, @@ -27,27 +12,6 @@ pub struct SandboxWorkspaceLayout { pub primary_repo_link: Option, } -/// Information returned when a sandbox sets up git for a workflow run. -#[derive(Debug, Clone)] -pub struct GitRunInfo { - pub base_sha: String, - pub run_branch: String, - pub base_branch: Option, -} - -/// Git setup requested by the workflow layer. -#[derive(Debug, Clone)] -pub enum GitSetupIntent { - NewRun { - run_id: String, - }, - ForkFromCheckpoint { - new_run_id: String, - source_run_id: String, - checkpoint_sha: String, - }, -} - /// Build a redacted `ExecOutputTail` from stdout/stderr text without /// fabricating a synthetic `ExecResult`. Each stream is redacted, then /// capped to its newest `max_bytes_per_stream`. Terminal control sequences @@ -123,748 +87,6 @@ pub(crate) fn join_sandbox_path(base: &str, relative_path: &str) -> String { /// git facet: a new run branches from `HEAD`, a fork from the source run's /// checkpoint. The branch is created at that base, or moved to it when an /// earlier attempt already created it. -pub async fn setup_git(sandbox: &RunSandbox, intent: &GitSetupIntent) -> crate::Result { - let git = sandbox.git()?; - let repo = sandbox.working_directory().to_owned(); - let status = git - .status(&repo) - .await - .map_err(|error| crate::Error::context("git status", error))?; - let base_branch = status - .current_branch - .filter(|name| !name.is_empty() && name != "HEAD"); - - let (base_sha, branch_name) = match intent { - GitSetupIntent::NewRun { run_id } => { - let head = status.head.ok_or_else(|| { - crate::Error::message("the repository has no commit to branch the run from") - })?; - (head, format!("fabro/run/{run_id}")) - } - GitSetupIntent::ForkFromCheckpoint { - new_run_id, - source_run_id, - checkpoint_sha, - } => { - fetch_source_run_ref(sandbox, source_run_id, checkpoint_sha).await?; - (checkpoint_sha.clone(), format!("fabro/run/{new_run_id}")) - } - }; - - git.checkout( - &repo, - &GitCheckoutOptions::new(&branch_name) - .create_or_reset() - .start_point(&base_sha), - ) - .await - .map_err(|error| crate::Error::context("git checkout -B", error))?; - - Ok(GitRunInfo { - base_sha, - run_branch: branch_name, - base_branch, - }) -} - -#[tracing::instrument(name = "git_op", skip_all, fields(op = "fetch"))] -pub(crate) async fn fetch_source_run_ref( - sandbox: &RunSandbox, - source_run_id: &str, - checkpoint_sha: &str, -) -> 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 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 { - 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; - } - - Err(crate::Error::message(last_error)) -} - -/// One push attempt inside a retried push operation. Runtime detail only — -/// the durable serialized shape lives in `fabro-types` and the workflow layer -/// owns the conversion. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct PushAttempt { - /// 1-based attempt number within this operation. - pub attempt: u32, - pub started_at: chrono::DateTime, - pub success: bool, - /// The classifier's verdict for a failed attempt — recorded on the - /// terminal attempt too; whether a retry actually followed is positional - /// (every entry except the last). - pub retry_reason: Option, - /// Redacted, bounded output tail; failed attempts only. - pub exec_output_tail: Option, - /// The token this attempt pushed with; `None` without managed - /// credentials. - pub token: Option, -} - -/// The attempt history of one push operation. -#[derive(Debug, Clone, Default)] -pub struct PushReport { - pub attempts: Vec, -} - -/// A failed push operation: the final typed error plus the attempt history. -/// The error type stays the safety boundary for output tails. -#[derive(Debug, thiserror::Error)] -#[error("git push failed")] -pub struct PushError { - pub report: PushReport, - #[source] - pub error: crate::Error, -} - -/// Pushes a refspec to origin through the driver's git facet, retried by -/// the driver under `policy` with one token for the whole operation. -/// `credentials` is the checkout's managed credentials; `None` pushes with -/// whatever the checkout already has (a checkout fabro did not clone, or a -/// clone made without a GitHub App). -#[tracing::instrument(name = "git_op", skip_all, fields(op = "push"))] -pub(crate) async fn git_push( - sandbox: &RunSandbox, - credentials: Option<&RepoCredentials>, - refspec: &str, - policy: &GitRetryPolicy, -) -> Result { - let start = time::Instant::now(); - let git = match sandbox.git() { - Ok(git) => git, - Err(error) => { - return Err(PushError { - report: PushReport::default(), - error, - }); - } - }; - let repo = sandbox.working_directory().to_owned(); - - // One token for the whole operation. A retry after replication lag must - // present the same token, because replication of a given token only - // makes progress, and a fresh mint would restart that clock. - let token = match credentials { - Some(credentials) => { - let resolved = match policy.max_elapsed { - Some(max_elapsed) => { - match time::timeout(max_elapsed, credentials.resolve()).await { - Ok(resolved) => resolved, - Err(_) => { - return Err(push_deadline_error( - Vec::new(), - "while acquiring credentials", - )); - } - } - } - None => credentials.resolve().await, - }; - match resolved { - Ok(token) => token, - Err(error) => { - return Err(PushError { - report: PushReport::default(), - error, - }); - } - } - } - None => None, - }; - let snapshot = token.as_ref().map(|token| token.snapshot); - let git_credentials = token.as_ref().map(credentials::git_credentials); - // Resolving the token spent part of the operation's budget. - let policy = match policy.max_elapsed { - Some(max_elapsed) => policy.max_elapsed(max_elapsed.saturating_sub(start.elapsed())), - None => *policy, - }; - - let label = format!("git push origin {refspec}"); - let result = retry_git( - &policy, - git_credentials.as_ref(), - &label, - |_attempt, timeout| { - let mut options = GitPushOptions::default(); - options.remote = Some("origin".to_owned()); - options.refspec = Some(refspec.to_owned()); - options.timeout = Some(timeout.unwrap_or(Duration::from_mins(1))); - options.credentials.clone_from(&git_credentials); - let git = &git; - let repo = &repo; - async move { git.push(repo, &options).await } - }, - ) - .await; - match result { - Ok(report) => { - tracing::info!( - refspec = %refspec, - attempts = report.attempts.len(), - token_generation = snapshot.map(|token| token.generation), - token_age_ms = snapshot.and_then(|token| token.age_ms()), - "Pushed git ref to origin" - ); - Ok(PushReport { - attempts: push_attempts(report.attempts, Ok(()), snapshot), - }) - } - Err(GitRetryError { attempts, error }) => { - let error = crate::Error::context(label, error); - Err(PushError { - report: PushReport { - attempts: push_attempts(attempts, Err(&error), snapshot), - }, - error, - }) - } - } -} - -/// The driver's attempt history as fabro records it. In a completed -/// operation every attempt but the last failed; in a failed one every -/// attempt failed, and the last attempt's failure is `outcome`'s error. -fn push_attempts( - attempts: Vec, - outcome: Result<(), &crate::Error>, - token: Option, -) -> Vec { - let last = attempts.len(); - attempts - .into_iter() - .enumerate() - .map(|(index, attempt)| { - let is_last = index + 1 == last; - let exec_output_tail = match (attempt.failure, &outcome) { - (Some(failure), _) => crate::Error::from(failure).default_redacted_output_tail(), - (None, Err(error)) if is_last => error.default_redacted_output_tail(), - (None, _) => None, - }; - PushAttempt { - attempt: attempt.attempt, - started_at: DateTime::::from(attempt.started_at), - success: is_last && outcome.is_ok(), - retry_reason: attempt.retry_reason.map(git_policy::recorded_reason), - exec_output_tail, - token, - } - }) - .collect() -} - -fn push_deadline_error(attempts: Vec, stage: &str) -> PushError { - PushError { - report: PushReport { attempts }, - error: crate::Error::message(format!("Git push retry deadline expired {stage}")), - } -} - -#[cfg(test)] -mod push_tests { - use std::collections::VecDeque; - use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::{Arc, Mutex}; - - use async_trait::async_trait; - use chrono::Utc; - use fabro_github::InstallationToken; - use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; - use fabro_github::token_source::{InstallationTokenSource, REFRESH_MARGIN}; - use fabro_types::SandboxProviderKind; - use sandbox_driver::{ExecResult, Termination}; - use sandbox_driver_testing::ScriptedSandbox; - use tokio::sync::Mutex as AsyncMutex; - - use super::*; - use crate::credentials::RepoCredentials; - use crate::git_policy::{GitRetryReason, checkpoint_push_policy, publish_push_policy}; - - const ORIGIN: &str = "https://github.com/fabro-testing/repo"; - const REFSPEC: &str = "refs/heads/fabro/run/01M0DH033P2XSTHAGVBHG6922F"; - - fn ok_exec() -> ExecResult { - ExecResult::new(Termination::Exited, Some(0), Duration::from_millis(5)) - } - - fn failed_exec(stderr: &str) -> ExecResult { - let mut result = ExecResult::new(Termination::Exited, Some(128), Duration::from_millis(5)); - result.stderr = stderr.as_bytes().to_vec(); - result - } - - fn timed_out_exec() -> ExecResult { - let mut result = ExecResult::new(Termination::TimedOut, None, Duration::from_mins(1)); - result.stderr = b"Command timed out".to_vec(); - result - } - - /// A run sandbox over a scripted driver double. The driver's push reads - /// `origin`'s URL when it carries credentials and then runs `git push`; - /// push answers come from a script, and every command is recorded. - struct ScriptedGitSandbox { - run: RunSandbox, - driver: Arc, - } - - impl ScriptedGitSandbox { - fn new(push_results: Vec) -> Self { - let driver = Arc::new(ScriptedSandbox::with_id_and_working_dir( - "scripted-git", - "/workspace", - )); - let pushes = Mutex::new(VecDeque::from(push_results)); - driver.scripted_exec().respond_with(move |spec| { - let script = spec.args.last().map(String::as_str).unwrap_or_default(); - if script.contains("'remote' 'get-url' 'origin'") { - let mut url = ok_exec(); - url.stdout = format!("{ORIGIN}\n").into_bytes(); - return Some(url); - } - assert!( - script.contains("'push' 'origin'"), - "unexpected exec: {script}" - ); - Some( - pushes - .lock() - .unwrap() - .pop_front() - .expect("push script exhausted"), - ) - }); - let run = RunSandbox::new(SandboxProviderKind::LOCAL, Arc::clone(&driver) as _); - Self { run, driver } - } - - fn commands(&self) -> Vec { - self.driver.scripted_exec().commands() - } - - /// The `git push` commands that ran, in order. - fn pushes(&self) -> Vec { - self.commands() - .into_iter() - .filter(|command| command.contains("'push' 'origin'")) - .collect() - } - - fn push_count(&self) -> usize { - self.pushes().len() - } - - /// The token each push carried in its per-call rewrite; `None` for - /// a push without credentials. - fn push_tokens(&self) -> Vec> { - self.pushes() - .iter() - .map(|push| { - let start = push.find("x-access-token:")? + "x-access-token:".len(); - let end = push[start..].find('@')? + start; - Some(push[start..end].to_owned()) - }) - .collect() - } - } - - enum MintAction { - Token(&'static str, chrono::Duration), - Error(&'static str), - } - - struct ScriptedMinter { - calls: AtomicUsize, - script: AsyncMutex>, - } - - impl ScriptedMinter { - fn new(script: Vec) -> Arc { - Arc::new(Self { - calls: AtomicUsize::new(0), - script: AsyncMutex::new(script.into()), - }) - } - - fn calls(&self) -> usize { - self.calls.load(Ordering::SeqCst) - } - } - - #[async_trait] - impl InstallationTokenMinter for ScriptedMinter { - async fn mint(&self) -> anyhow::Result { - self.calls.fetch_add(1, Ordering::SeqCst); - match self.script.lock().await.pop_front().expect("mint script") { - MintAction::Token(token, ttl) => Ok(InstallationToken { - token: token.to_string(), - expires_at: Utc::now() + ttl, - }), - MintAction::Error(message) => Err(anyhow::anyhow!(message)), - } - } - } - - struct SlowMinter; - - #[async_trait] - impl InstallationTokenMinter for SlowMinter { - async fn mint(&self) -> anyhow::Result { - time::sleep(Duration::from_secs(2)).await; - Ok(InstallationToken { - token: "ghs_slow".to_string(), - expires_at: Utc::now() + chrono::Duration::hours(1), - }) - } - } - - fn minting_credentials(script: Vec) -> (RepoCredentials, Arc) { - let minter = ScriptedMinter::new(script); - let source = installation_token_source( - "fabro-testing/repo", - Arc::clone(&minter) as Arc, - ); - (RepoCredentials::new(Some(source)), minter) - } - - /// Mint the clone token first, the way `initialize` does, so the push - /// resolves the cached token instead of minting one. - async fn seed_clone_token(credentials: &RepoCredentials) { - credentials - .mint_for_clone() - .await - .expect("clone mint succeeds") - .expect("managed credentials mint"); - } - - /// Regression for run `01M0DH033P2XSTHAGVBHG6922F` (the push variant of - /// `clone_not_found_after_a_successful_mint_is_retried`): GitHub rejected - /// pushes with 404 "Repository not found" milliseconds after a token - /// mint. The retry must reuse the same token — replication of a given - /// token only makes progress — and recover inside the plan's budget. - #[tokio::test(start_paused = true)] - async fn push_not_found_after_a_successful_mint_is_retried_with_the_same_token() { - let (credentials, minter) = minting_credentials(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &checkpoint_push_policy(), - ) - .await - .expect("push should recover within the checkpoint plan"); - - assert_eq!(report.attempts.len(), 3); - assert_eq!(minter.calls(), 1, "retries must not re-mint"); - for attempt in &report.attempts { - assert_eq!(attempt.token.expect("token recorded").generation, 1); - } - assert_eq!( - report.attempts[0].retry_reason, - Some(GitRetryReason::TokenReplication) - ); - assert!(report.attempts[0].exec_output_tail.is_some()); - assert!(report.attempts[2].success); - assert!(report.attempts[2].exec_output_tail.is_none()); - assert_eq!( - sandbox.push_tokens(), - vec![Some("ghs_gen1".to_owned()); 3], - "every attempt presents the same token" - ); - } - - /// The publish plan gives the terminal push a real budget: four - /// replication-lag failures still recover on the fifth attempt. - #[tokio::test(start_paused = true)] - async fn publish_plan_survives_four_not_found_failures() { - let (credentials, minter) = minting_credentials(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &publish_push_policy(), - ) - .await - .expect("push should recover within the publish plan"); - - assert_eq!(report.attempts.len(), 5); - assert_eq!(minter.calls(), 1); - assert!(report.attempts[4].success); - } - - /// Margin-boundary pinning: a token resolved just above the refresh - /// margin stays pinned through a full retry sequence — the operation - /// never re-resolves mid-flight, so no fresh mint can restart the - /// replication clock. - #[tokio::test(start_paused = true)] - async fn token_resolved_just_above_the_margin_stays_pinned_through_retries() { - let ttl = REFRESH_MARGIN + Duration::from_secs(5); - let (credentials, minter) = minting_credentials(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::from_std(ttl).unwrap()), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("remote: Repository not found."), - failed_exec("remote: Repository not found."), - ok_exec(), - ]); - - let report = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &checkpoint_push_policy(), - ) - .await - .expect("push recovers"); - - assert_eq!(minter.calls(), 1, "the operation never re-resolves"); - assert_eq!(sandbox.push_tokens(), vec![Some("ghs_gen1".to_owned()); 3]); - assert!( - report - .attempts - .iter() - .all(|attempt| attempt.token.map(|token| token.generation) == Some(1)) - ); - } - - #[tokio::test(start_paused = true)] - async fn static_credential_auth_failure_fails_fast() { - let credentials = - RepoCredentials::new(Some(InstallationTokenSource::pat("ghp_static".to_owned()))); - let sandbox = ScriptedGitSandbox::new(vec![failed_exec("remote: Repository not found.")]); - - let push_error = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &publish_push_policy(), - ) - .await - .expect_err("static credentials cannot become valid by waiting"); - - assert_eq!(push_error.report.attempts.len(), 1); - assert_eq!(push_error.report.attempts[0].retry_reason, None); - assert_eq!( - push_error.report.attempts[0] - .token - .map(|token| token.generation), - Some(0) - ); - assert_eq!(sandbox.push_tokens(), vec![Some("ghp_static".to_owned())]); - } - - /// A refresh that fails while the cached token is still valid pushes - /// with the cached token. - #[tokio::test(start_paused = true)] - async fn mint_failure_falls_back_to_the_cached_token() { - // The clone token is already inside the refresh margin, so the - // push's resolve tries to re-mint and fails. - let (credentials, minter) = minting_credentials(vec![ - MintAction::Token( - "ghs_clone", - chrono::Duration::from_std( - REFRESH_MARGIN - .checked_sub(Duration::from_mins(1)) - .expect("the margin is longer than a minute"), - ) - .unwrap(), - ), - MintAction::Error("github unavailable"), - ]); - seed_clone_token(&credentials).await; - let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - - let report = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &checkpoint_push_policy(), - ) - .await - .expect("the cached token still pushes"); - - assert_eq!(minter.calls(), 2, "the push tried to refresh once"); - assert_eq!(sandbox.push_tokens(), vec![Some("ghs_clone".to_owned())]); - assert_eq!( - report.attempts[0].token.map(|token| token.generation), - Some(1) - ); - } - - #[tokio::test(start_paused = true)] - async fn mint_failure_without_a_cached_token_fails_before_any_push() { - let (credentials, minter) = - minting_credentials(vec![MintAction::Error("github unavailable")]); - let sandbox = ScriptedGitSandbox::new(vec![]); - - let push_error = git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &checkpoint_push_policy(), - ) - .await - .expect_err("no token to push with"); - - assert!(push_error.report.attempts.is_empty()); - assert_eq!(sandbox.push_count(), 0); - assert_eq!(minter.calls(), 1); - assert!( - push_error - .error - .to_string() - .contains("Failed to refresh GitHub App credentials"), - "{}", - push_error.error - ); - } - - /// The token reaches git through the driver's per-call rewrite and never - /// through the remote URL. - #[tokio::test(start_paused = true)] - async fn credentials_travel_per_call_and_never_touch_the_remote() { - let (credentials, _minter) = minting_credentials(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::minutes(60), - )]); - let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - - git_push( - &sandbox.run, - Some(&credentials), - REFSPEC, - &checkpoint_push_policy(), - ) - .await - .expect("push succeeds"); - - let commands = sandbox.commands(); - assert!( - commands.iter().all(|command| !command.contains("set-url")), - "{commands:#?}" - ); - let push = &sandbox.pushes()[0]; - assert!( - push.contains("insteadOf=https://github.com/fabro-testing/repo"), - "{push}" - ); - assert!( - push.contains("'push' 'origin' 'refs/heads/fabro/run/"), - "{push}" - ); - } - - #[tokio::test(start_paused = true)] - async fn push_without_managed_credentials_reports_no_token() { - let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - - let report = git_push(&sandbox.run, None, REFSPEC, &checkpoint_push_policy()) - .await - .expect("push succeeds"); - - assert_eq!(report.attempts.len(), 1); - assert_eq!(report.attempts[0].token, None); - assert_eq!(sandbox.push_tokens(), vec![None]); - } - - #[tokio::test(start_paused = true)] - async fn unauthenticated_auth_failure_is_permanent() { - let sandbox = ScriptedGitSandbox::new(vec![failed_exec( - "fatal: Authentication failed for 'https://github.com/fabro-testing/repo'", - )]); - - let push_error = git_push(&sandbox.run, None, REFSPEC, &publish_push_policy()) - .await - .expect_err("no credentials to wait on"); - - assert_eq!(push_error.report.attempts.len(), 1); - assert_eq!(push_error.report.attempts[0].retry_reason, None); - } - - #[tokio::test(start_paused = true)] - async fn timed_out_push_is_not_retried_while_the_remote_process_may_still_run() { - let sandbox = ScriptedGitSandbox::new(vec![timed_out_exec()]); - - let push_error = git_push(&sandbox.run, None, REFSPEC, &publish_push_policy()) - .await - .expect_err("an unconfirmed timeout must fail without another push"); - - assert_eq!(sandbox.push_count(), 1); - assert_eq!(push_error.report.attempts.len(), 1); - assert_eq!(push_error.report.attempts[0].retry_reason, None); - } - - #[tokio::test(start_paused = true)] - async fn retry_deadline_includes_credential_resolution() { - let source = installation_token_source("fabro-testing/repo", Arc::new(SlowMinter)); - let credentials = RepoCredentials::new(Some(source)); - let sandbox = ScriptedGitSandbox::new(vec![]); - let policy = checkpoint_push_policy().max_elapsed(Duration::from_secs(1)); - - let push_error = git_push(&sandbox.run, Some(&credentials), REFSPEC, &policy) - .await - .expect_err("credential resolution must stop at the operation deadline"); - - assert!(push_error.report.attempts.is_empty()); - assert_eq!(sandbox.push_count(), 0); - assert!(push_error.error.to_string().contains("deadline expired")); - } - - #[tokio::test(start_paused = true)] - async fn expired_retry_deadline_does_not_launch_a_zero_timeout_push() { - let sandbox = ScriptedGitSandbox::new(vec![]); - let policy = checkpoint_push_policy().max_elapsed(Duration::ZERO); - - let push_error = git_push(&sandbox.run, None, REFSPEC, &policy) - .await - .expect_err("an expired operation must stop before exec"); - - assert!(push_error.report.attempts.is_empty()); - assert_eq!(sandbox.push_count(), 0); - } -} #[cfg(test)] mod tests { diff --git a/lib/components/fabro-sandbox/src/sandbox_spec.rs b/lib/components/fabro-sandbox/src/sandbox_spec.rs index 2daf2fe0c..e3890cd3a 100644 --- a/lib/components/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/components/fabro-sandbox/src/sandbox_spec.rs @@ -2,7 +2,6 @@ use std::path::PathBuf; use std::sync::Arc; use anyhow::Context as _; -use fabro_github::GitHubCredentials; use fabro_types::{RunId, RunSandboxInstance, RunSandboxRuntime, SandboxProviderKind}; use sandbox_driver::{EventContext, SandboxSource, SandboxSpec as DriverSpec}; @@ -13,18 +12,17 @@ use crate::{clone_source, provider_sandbox}; /// A run's sandbox on any provider fabro can name: a bundled kind in /// process or a sandbox-driver plugin. What the environment asked for, and -/// how the repository is cloned into it. +/// the repository the run record names for it. #[derive(Clone, Debug)] pub struct SandboxSpec { - pub kind: SandboxProviderKind, + pub kind: SandboxProviderKind, /// The provider settings and vault credentials the kind needs. - pub access: ProviderAccess, + pub access: ProviderAccess, /// The environment's request, as the driver spec every provider /// starts from. - pub spec: DriverSpec, - pub clone: CloneRequest, - pub github_app: Option, - pub run_id: Option, + pub spec: DriverSpec, + pub clone: CloneRequest, + pub run_id: Option, } impl SandboxSpec { @@ -41,7 +39,6 @@ impl SandboxSpec { spec: DriverSpec::new(SandboxSource::HostDirectory) .working_directory(working_directory.into().display().to_string()), clone: CloneRequest::none(), - github_app: None, run_id: None, } } @@ -129,7 +126,6 @@ impl SandboxSpec { &self.access, self.spec.clone(), &self.clone, - self.github_app.as_ref(), self.run_id, ) .await @@ -165,7 +161,6 @@ mod tests { access: ProviderAccess::default(), spec: DriverSpec::new(SandboxSource::HostDirectory), clone, - github_app: None, run_id: None, } } diff --git a/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs b/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs index 2d05dce0f..39a6b0c39 100644 --- a/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs +++ b/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs @@ -35,7 +35,6 @@ mod daytona_streaming_live { SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::none(), None, - None, ) .await?, ); @@ -69,7 +68,6 @@ mod daytona_streaming_live { SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::none(), None, - None, ) .await?; sandbox.initialize().await?; @@ -169,7 +167,6 @@ mod daytona_streaming_live { SandboxSpec::new(SandboxSource::HostDirectory) .label("team".to_string(), "platform".to_string()), &CloneRequest::none(), - None, Some(run_id), ) .await?; @@ -204,66 +201,6 @@ mod daytona_streaming_live { Ok(()) } - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - #[ignore = "requires live Daytona credentials and provisions a sandbox"] - async fn daytona_clone_layout_live_smoke() -> Result<()> { - ensure!( - daytona_api_key_present(), - "DAYTONA_API_KEY must be set to run this live smoke test" - ); - - let sandbox = provider_sandbox( - SandboxProviderKind::DAYTONA, - &daytona_access(live_credentials()?), - SandboxSpec::new(SandboxSource::HostDirectory), - &CloneRequest { - origin_url: Some("https://github.com/brynary/rack-test".to_string()), - ..CloneRequest::default() - }, - None, - None, - ) - .await?; - - sandbox.initialize().await?; - ensure_eq( - &sandbox.working_directory(), - &"/home/daytona/workspace/rack-test", - "working directory should be the workspace symlink", - )?; - - let result = sandbox - .exec_command( - "test -d /home/daytona/repos/brynary/rack-test/.git && \ - test -L /home/daytona/workspace/rack-test && \ - test \"$(readlink /home/daytona/workspace/rack-test)\" = /home/daytona/repos/brynary/rack-test && \ - test \"$(git -C /home/daytona/repos/brynary/rack-test rev-parse HEAD)\" = \ - \"$(git -C /home/daytona/workspace/rack-test rev-parse HEAD)\" && \ - git rev-parse --is-inside-work-tree", - 30_000, - None, - None, - None, - ) - .await?; - let cleanup_result = sandbox.delete().await.context("clean up Daytona sandbox"); - - ensure!( - result.success(), - "layout verification failed: stdout={} stderr={}", - result.stdout_lossy(), - result.stderr_lossy() - ); - ensure_contains( - &result.stdout_lossy(), - "true", - "default cwd should be inside the work tree", - )?; - cleanup_result?; - - Ok(()) - } - // Regression test for glob patterns that contain a path separator. Before // the glob fix, Daytona ran `find -name `, and `find -name` // matches only the basename and rejects patterns containing `/`. So @@ -285,7 +222,6 @@ mod daytona_streaming_live { SandboxSpec::new(SandboxSource::HostDirectory), &CloneRequest::none(), None, - None, ) .await?; diff --git a/lib/components/fabro-sandbox/tests/docker_streaming.rs b/lib/components/fabro-sandbox/tests/docker_streaming.rs index 7ba4f2951..c339077b1 100644 --- a/lib/components/fabro-sandbox/tests/docker_streaming.rs +++ b/lib/components/fabro-sandbox/tests/docker_streaming.rs @@ -49,7 +49,6 @@ async fn streaming_timeout_terminates_docker_exec_before_returning() { }), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); @@ -119,7 +118,6 @@ async fn streaming_command_receives_exact_stdin_and_eof() { }), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); @@ -161,65 +159,6 @@ async fn streaming_command_receives_exact_stdin_and_eof() { ); } -#[tokio::test] -#[ignore = "requires real Docker container lifecycle, image, network, and a public GitHub clone"] -async fn cloned_docker_sandbox_uses_repos_checkout_and_workspace_symlink() { - let image = "buildpack-deps:noble"; - if !docker_image_available(image).await { - return; - } - - let sandbox = provider_sandbox( - SandboxProviderKind::DOCKER, - &ProviderAccess::default(), - SandboxSpec::new(SandboxSource::Image { - reference: image.to_string(), - }), - &CloneRequest { - origin_url: Some("https://github.com/brynary/rack-test".to_string()), - ..CloneRequest::default() - }, - None, - None, - ) - .await - .expect("docker sandbox should construct"); - sandbox - .initialize() - .await - .expect("docker sandbox should initialize"); - - assert_eq!(sandbox.working_directory(), "/workspace/rack-test"); - - let result = sandbox - .exec_command( - "test -d /repos/brynary/rack-test/.git && \ - test -L /workspace/rack-test && \ - test \"$(readlink /workspace/rack-test)\" = /repos/brynary/rack-test && \ - test \"$(git -C /repos/brynary/rack-test rev-parse HEAD)\" = \ - \"$(git -C /workspace/rack-test rev-parse HEAD)\" && \ - git rev-parse --is-inside-work-tree", - 10_000, - None, - None, - None, - ) - .await - .expect("layout verification command should run"); - sandbox - .delete() - .await - .expect("docker cleanup should succeed"); - - assert!( - result.success(), - "layout verification failed: stdout={} stderr={}", - result.stdout_lossy(), - result.stderr_lossy() - ); - assert!(result.stdout_lossy().contains("true")); -} - // Both command paths must evaluate the same interpreter, so Bash-only syntax // that `sh` rejects has to behave identically through `exec_command` and // `exec_command_streaming`. Neither path is evidence for the other: they build @@ -242,7 +181,6 @@ async fn docker_runs_clean_bash_through_both_command_paths() { .env_var("BASH_ENV".to_string(), "/tmp/fabro-bash-env".to_string()), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); @@ -333,7 +271,6 @@ async fn docker_glob_matches_patterns_containing_a_path_separator() { }), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); @@ -413,7 +350,6 @@ async fn docker_runtime_directory_is_private_and_outside_workspace() { }), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); @@ -488,7 +424,6 @@ async fn docker_sandbox_satisfies_pebbles_environment_contract() { }), &CloneRequest::none(), None, - None, ) .await .expect("docker sandbox should construct"); diff --git a/lib/components/fabro-sandbox/tests/driver_bench.rs b/lib/components/fabro-sandbox/tests/driver_bench.rs index dfb2771c5..78c08270e 100644 --- a/lib/components/fabro-sandbox/tests/driver_bench.rs +++ b/lib/components/fabro-sandbox/tests/driver_bench.rs @@ -367,7 +367,6 @@ async fn agent_tool_call_latency_through_the_driver() { }), &CloneRequest::none(), None, - None, ) .await .expect("fabro docker sandbox");