diff --git a/AGENTS.md b/AGENTS.md index 68d143074..acc2f2dec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,12 +33,16 @@ macOS note: if `cargo nextest run` fails with `Too many open files (os error 24) - 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. - The sandbox layer also accepts an optional exact commit for future admitted runs. An exact commit always requires a non-empty branch. Docker initializes - an empty repository, shallow-fetches the SHA, detaches, and verifies HEAD; - Daytona uses its official SDK clone with both `branch` and `commit_id`. Keep - those provider transports distinct, never fall back to a newer branch HEAD, - and do not wire this capability directly from legacy `GitContext.sha`. - Current production callers remain branch-only until the RunIntent admission - cutover supplies a validated branch/SHA pair. + an empty repository, shallow-fetches the SHA at the same depth as a branch + clone, and checks it out; Daytona uses its official SDK clone with both + `branch` and `commit_id`. Both providers then point the admitted branch at + the commit and verify HEAD, so the workspace still reports the admitted + branch name. Keep those provider transports distinct, 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. ### 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/lib/components/fabro-sandbox/src/clone_source.rs b/lib/components/fabro-sandbox/src/clone_source.rs index cd6d9113c..ea343bbf4 100644 --- a/lib/components/fabro-sandbox/src/clone_source.rs +++ b/lib/components/fabro-sandbox/src/clone_source.rs @@ -81,13 +81,20 @@ pub(crate) fn exact_repository_init_command(clone_url: &str, checkout_path: &str ) } +/// Fetch a single admitted commit with the same history depth a branch clone +/// gets, so both paths can reach the same number of parent commits. +/// +/// The fetch names the commit directly rather than the branch: reachability of +/// the commit from the admitted branch is an admission-time invariant, not +/// something this layer re-verifies. pub(crate) fn exact_fetch_command( checkout_path: &str, fetch_source: &str, commit_sha: &str, + depth: usize, ) -> String { format!( - "{git} -C {} fetch --depth 1 --no-tags {} -- {}", + "{git} -C {} fetch --depth {depth} --no-tags {} -- {}", sandbox::shell_quote(checkout_path), sandbox::shell_quote(fetch_source), sandbox::shell_quote(commit_sha), @@ -95,13 +102,45 @@ pub(crate) fn exact_fetch_command( ) } -/// Detach onto the fetched commit and print the resulting HEAD in one shell -/// command; stdout is the `rev-parse HEAD` output for [`verify_exact_head`]. -pub(crate) fn exact_checkout_verify_command(checkout_path: &str) -> String { +/// Point the admitted branch at `revision` and attach HEAD to it. +/// +/// The checkout attaches to a real branch instead of detaching so callers that +/// read the current branch back out of the workspace still see the admitted +/// branch name. +pub(crate) fn exact_branch_checkout_command( + checkout_path: &str, + branch: &str, + revision: &str, +) -> String { format!( - "git -C {path} -c advice.detachedHead=false checkout --detach FETCH_HEAD && git -C {path} \ - rev-parse HEAD", + "{git} -C {path} checkout -B {branch} {revision}", path = sandbox::shell_quote(checkout_path), + branch = sandbox::shell_quote(branch), + revision = sandbox::shell_quote(revision), + git = sandbox::GIT, + ) +} + +/// Print the current HEAD commit and nothing else, for [`verify_exact_head`]. +pub(crate) fn exact_head_revision_command(checkout_path: &str) -> String { + format!( + "{git} -C {path} rev-parse HEAD", + path = sandbox::shell_quote(checkout_path), + git = sandbox::GIT, + ) +} + +/// Check out the admitted branch and print the resulting HEAD in one shell +/// command; stdout is the `rev-parse HEAD` output for [`verify_exact_head`]. +pub(crate) fn exact_checkout_verify_command( + checkout_path: &str, + branch: &str, + revision: &str, +) -> String { + format!( + "{} && {}", + exact_branch_checkout_command(checkout_path, branch, revision), + exact_head_revision_command(checkout_path), ) } @@ -161,6 +200,9 @@ pub(crate) fn decide_clone( "Exact commit checkout requires a repository origin", )); } + // The branch names the checkout the run works on; it is not used to + // constrain which commits may be fetched. Admission is responsible for + // proving the commit belongs to the branch before it reaches here. if clone_branch.is_none_or(|branch| branch.trim().is_empty()) { return Err(crate::Error::message( "Exact commit checkout requires a repository branch", @@ -432,8 +474,10 @@ mod tests { "/repos/acme's widgets", "https://token@example.com/acme/widgets.git?x=a b", sha, + 10, ); - let checkout = exact_checkout_verify_command("/repos/acme's widgets"); + let checkout = + exact_checkout_verify_command("/repos/acme's widgets", "feature/a b", "FETCH_HEAD"); assert_eq!( init, @@ -441,11 +485,11 @@ mod tests { ); assert_eq!( fetch, - "git -c maintenance.auto=0 -c gc.auto=0 -C \"/repos/acme's widgets\" fetch --depth 1 --no-tags 'https://token@example.com/acme/widgets.git?x=a b' -- 0123456789abcdef0123456789abcdef01234567" + "git -c maintenance.auto=0 -c gc.auto=0 -C \"/repos/acme's widgets\" fetch --depth 10 --no-tags 'https://token@example.com/acme/widgets.git?x=a b' -- 0123456789abcdef0123456789abcdef01234567" ); assert_eq!( checkout, - "git -C \"/repos/acme's widgets\" -c advice.detachedHead=false checkout --detach FETCH_HEAD && git -C \"/repos/acme's widgets\" rev-parse HEAD" + "git -c maintenance.auto=0 -c gc.auto=0 -C \"/repos/acme's widgets\" checkout -B 'feature/a b' FETCH_HEAD && git -c maintenance.auto=0 -c gc.auto=0 -C \"/repos/acme's widgets\" rev-parse HEAD" ); } @@ -510,23 +554,27 @@ mod tests { ); run_shell( temp.path(), - &exact_fetch_command(checkout_path, remote_path, &admitted_sha), + &exact_fetch_command(checkout_path, remote_path, &admitted_sha, 10), + ); + let checked_out_sha = run_shell( + temp.path(), + &exact_checkout_verify_command(checkout_path, "main", "FETCH_HEAD"), ); - let checked_out_sha = run_shell(temp.path(), &exact_checkout_verify_command(checkout_path)); assert_eq!(checked_out_sha.trim(), admitted_sha); assert_eq!( fs::read_to_string(checkout.join("revision.txt")).expect("checked-out contents"), "A\n" ); - let symbolic_head = isolated_command(Command::new("git").args([ - "-C", - checkout_path, - "symbolic-ref", - "-q", - "HEAD", - ])); - assert!(!symbolic_head.status.success(), "HEAD should be detached"); + assert_eq!( + run_git(&checkout, &["symbolic-ref", "HEAD"]).trim(), + "refs/heads/main", + "HEAD should stay attached to the admitted branch" + ); + assert_eq!( + run_git(&checkout, &["rev-parse", "--abbrev-ref", "HEAD"]).trim(), + "main" + ); assert_eq!( run_git(temp.path(), &[ "--git-dir", diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index c03ea734c..c18cdad27 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -531,6 +531,63 @@ impl DaytonaSandbox { err } + /// Point the admitted branch at the exact commit and verify the resulting + /// HEAD. + /// + /// Daytona's native clone honors `commit_id`, but leaves the workspace on + /// whatever ref its own checkout produced. Re-pointing the branch keeps the + /// admitted branch name readable back out of the workspace, matching what + /// the Docker provider produces for the same inputs. + async fn attach_exact_commit_branch( + process_svc: &daytona_sdk::ProcessService, + checkout_path: &str, + branch: &str, + expected_sha: &str, + ) -> crate::Result<()> { + Self::run_clone_step( + process_svc, + &clone_source::exact_branch_checkout_command(checkout_path, branch, expected_sha), + "git checkout exact commit", + ) + .await?; + let head = Self::run_clone_step( + process_svc, + &clone_source::exact_head_revision_command(checkout_path), + "git rev-parse HEAD after exact checkout", + ) + .await?; + clone_source::verify_exact_head(&head, expected_sha) + } + + /// Run one local git step of the clone in the sandbox and return its + /// output. + async fn run_clone_step( + process_svc: &daytona_sdk::ProcessService, + command: &str, + label: &'static str, + ) -> crate::Result { + let result = process_svc + .execute_command( + &wrap_bash_command(command), + daytona_sdk::ExecuteCommandOptions { + cwd: Some("/".to_string()), + ..Default::default() + }, + ) + .await + .map_err(|e| crate::Error::context(format!("Failed to run {label}"), e))?; + if result.exit_code != 0 { + return Err(crate::Error::exec(label, ExecResult { + stdout: result.result, + stderr: String::new(), + exit_code: Some(result.exit_code), + termination: CommandTermination::Exited, + duration_ms: 0, + })); + } + Ok(result.result) + } + fn fail_init(&self, init_start: Instant, err: crate::Error) -> crate::Error { let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { @@ -1227,6 +1284,28 @@ impl Sandbox for DaytonaSandbox { self.fail_init(init_start, err) })?; + if let Some(expected_sha) = commit_sha.as_deref() { + let Some(branch) = branch.as_deref().filter(|branch| !branch.trim().is_empty()) + else { + let err = crate::Error::message( + "Exact commit checkout requires a repository branch", + ); + let err = self.report_clone_failure(&origin_url, err); + return Err(self.fail_init(init_start, err)); + }; + if let Err(err) = Self::attach_exact_commit_branch( + &process_svc, + &layout.primary_repo_path, + branch, + expected_sha, + ) + .await + { + let err = self.report_clone_failure(&origin_url, err); + return Err(self.fail_init(init_start, err)); + } + } + let symlink_cmd = clone_source::repo_symlink_command(&layout); let symlink_result = process_svc .execute_command( diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 891cd6cad..1ee22ce90 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -56,6 +56,14 @@ const EXEC_TERM_GRACE_SECONDS: &str = "0.02"; #[cfg(not(test))] const EXEC_TERM_GRACE_SECONDS: &str = "0.2"; +/// 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, +} + fn env_entry_name(entry: &str) -> &str { entry.split_once('=').map_or(entry, |(name, _)| name) } @@ -717,20 +725,23 @@ impl DockerSandbox { Ok(()) } - /// Preserve a failed git transfer result while masking the auth URL. + /// Preserve a failed git step result while masking the auth URL. fn clone_failure_error( &self, result: ExecResult, label: &'static str, auth_url: Option<&fabro_redact::DisplaySafeUrl>, + step: CloneStep, ) -> crate::Error { let error = result.into_exec_error_with_redactor(label, |output| redact_auth_url(output, auth_url)); - let message = if self.github_app.is_none() { - "Git clone failed. If this is a private repository, configure a GitHub App with \ - `fabro install` and install it for your organization." - } else { - "Failed to clone repository into Docker sandbox" + let message = match step { + CloneStep::Network if self.github_app.is_none() => { + "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 Docker sandbox", + CloneStep::Local => "Failed to prepare the cloned repository in the Docker sandbox", }; crate::Error::context(message, error) } @@ -744,20 +755,34 @@ impl DockerSandbox { err } - async fn run_exact_checkout_command( + /// Run a local (non-network) step of the exact checkout under the shared + /// clone deadline. + /// + /// Materializing a large working tree takes far longer than the short fixed + /// timeout used for trivial container commands, so these steps get the same + /// budget the branch clone path gives its fetch and checkout. + async fn run_exact_local_git_command( &self, command: &str, label: &'static str, + clone_deadline: time::Instant, auth_url: Option<&fabro_redact::DisplaySafeUrl>, ) -> crate::Result { + let remaining = clone_deadline.saturating_duration_since(time::Instant::now()); + let timeout_ms = u64::try_from(remaining.as_millis()).unwrap_or(u64::MAX); + if timeout_ms == 0 { + return Err(crate::Error::message(format!( + "{label} deadline expired before the step could run" + ))); + } let result = self - .docker_exec_shell(command, 10_000, Some("/"), None, None) + .docker_exec_shell(command, timeout_ms, Some("/"), None, None) .await .map_err(|error| crate::Error::context(format!("{label} transport failed"), error))?; if result.is_success() { Ok(result) } else { - Err(self.clone_failure_error(result, label, auth_url)) + Err(self.clone_failure_error(result, label, auth_url, CloneStep::Local)) } } @@ -806,7 +831,12 @@ impl DockerSandbox { } let retry_reason = classify_docker_clone_result(&result, token_was_freshly_minted); Err(clone_retry::CloneAttemptFailure { - error: self.clone_failure_error(result, exec_label, auth_url), + error: self.clone_failure_error( + result, + exec_label, + auth_url, + CloneStep::Network, + ), retry_reason, }) }, @@ -873,12 +903,22 @@ impl DockerSandbox { let clone_deadline = time::Instant::now() + GIT_CLONE_TIMEOUT; if let Some(expected_sha) = commit_sha.as_deref() { + // `decide_clone` already rejects an exact commit without a branch; + // re-check here so the checkout can never silently drop the branch + // name callers read back out of the workspace. + let Some(branch) = branch.as_deref().filter(|branch| !branch.trim().is_empty()) else { + let error = + crate::Error::message("Exact commit checkout requires a repository branch"); + return Err(self.report_clone_failure(&origin_url, error)); + }; + let init_command = clone_source::exact_repository_init_command(clone_url, &layout.primary_repo_path); if let Err(error) = self - .run_exact_checkout_command( + .run_exact_local_git_command( &init_command, "initialize Docker exact repository checkout", + clone_deadline, auth_url.as_ref(), ) .await @@ -890,6 +930,7 @@ impl DockerSandbox { &layout.primary_repo_path, "origin", expected_sha, + GIT_CLONE_DEPTH, ); if let Err(failure) = self .retry_git_transfer( @@ -905,12 +946,16 @@ impl DockerSandbox { return Err(self.report_clone_failure(&origin_url, failure.error)); } - let checkout_command = - clone_source::exact_checkout_verify_command(&layout.primary_repo_path); + let checkout_command = clone_source::exact_checkout_verify_command( + &layout.primary_repo_path, + branch, + "FETCH_HEAD", + ); let head = match self - .run_exact_checkout_command( + .run_exact_local_git_command( &checkout_command, "git checkout exact commit", + clone_deadline, auth_url.as_ref(), ) .await @@ -2588,6 +2633,7 @@ mod tests { }, "git fetch exact commit", Some(&auth_url), + CloneStep::Network, ); let causes = error.causes(); @@ -2603,6 +2649,37 @@ mod tests { assert!(rendered.contains("synthetic low-level failure")); } + #[test] + fn local_checkout_failure_does_not_blame_github_credentials() { + let docker = Docker::connect_with_http("http://127.0.0.1:2375", 5, API_DEFAULT_VERSION) + .expect("mock Docker client should connect"); + let sandbox = test_docker_sandbox(docker, "test-container"); + let failure = ExecResult { + stdout: String::new(), + stderr: "error: pathspec 'FETCH_HEAD' did not match".to_string(), + exit_code: Some(1), + termination: CommandTermination::Exited, + duration_ms: 1, + }; + + let local = crate::display_for_log(&sandbox.clone_failure_error( + failure.clone(), + "git checkout exact commit", + None, + CloneStep::Local, + )); + assert!(!local.contains("fabro install"), "{local}"); + assert!(local.contains("prepare the cloned repository"), "{local}"); + + let network = crate::display_for_log(&sandbox.clone_failure_error( + failure, + "git fetch exact commit", + None, + CloneStep::Network, + )); + assert!(network.contains("fabro install"), "{network}"); + } + #[test] fn clone_result_uses_stderr_before_stdout() { let result = ExecResult {