scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Harden exact-commit checkout in clone-based sandboxes

Browse source 
Run the local git steps of the Docker exact checkout under the shared
clone deadline instead of a fixed 10s timeout, so materializing a large
working tree cannot time out and abandon a running checkout in the
container.

Check the admitted commit out onto the admitted branch rather than
detaching. A detached HEAD makes `rev-parse --abbrev-ref HEAD` return
"HEAD", which the git setup helper maps to no base branch, silently
dropping it for callers that rely on it. Daytona does the same after its
native clone and now verifies the resulting HEAD the way Docker does.

Fetch the exact commit at the same depth a branch clone uses, so both
paths can reach the same number of parent commits, and stop suggesting
GitHub App credentials when a purely local git step fails.

Document that reachability of the commit from the branch is an
admission-time invariant that the sandbox layer does not re-verify.
This commit is contained in:
Scott Werner 2026-08-20 13:41:54 -04:00
parent e7a32d12d5
commit ebf6f92724
4 changed files with 247 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
AGENTS.md
View file

@ -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.

86
lib/components/fabro-sandbox/src/clone_source.rs
View file

@ -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",

79
lib/components/fabro-sandbox/src/daytona/mod.rs
View file

@ -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<String> {
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(

105
lib/components/fabro-sandbox/src/docker.rs
View file

@ -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<ExecResult> {
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 {