Trust the driver's pinned clone instead of re-reading HEAD

A clone pinned to a commit or tag ran a second `git rev-parse HEAD`
through exec and compared it with the pin. The pre-driver clone could
land on the branch head when a pin was unavailable, and the check
existed for that case. The driver's clone fetches the pin directly and
attaches the branch with `checkout -B <branch> <pin>`, which fails when
the pin is absent, so a successful clone already has the pin checked
out; the driver's conformance suite verifies that on every provider.

`PinnedRevision` keeps only what the clone decision still uses: which
kind of pin was asked for, for the error messages.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-09-11 09:32:09 -06:00
parent 20a5500510
commit c313e34605
No known key found for this signature in database
2 changed files with 27 additions and 96 deletions

View file

@ -6,9 +6,8 @@
//! the repository checks out under `<repos_root>/<owner>/<repo>` and the
//! run works in `<workspace_root>/<repo>`, a symlink to the checkout. An
//! exact commit or a tag is pinned by the driver's clone options, which
//! fetch a tag by its fully qualified ref so a same-named branch is never
//! consulted; fabro verifies the checked-out head afterwards. Neither path
//! ever falls back to the branch head.
//! fetch the pin directly and attach the branch to it; an unavailable pin
//! fails the clone and never falls back to the branch head.
use std::time::Duration;
@ -20,7 +19,7 @@ use sandbox_driver::{
};
use tokio::time;
use crate::clone_source::{self, GitHubRepoLayout, PinnedRevision};
use crate::clone_source::{self, GitHubRepoLayout};
use crate::exec::{ExecResultExt, SandboxExec};
use crate::git_retry::{self, CredentialContext, GitRetryReason, RetryPlan};
use crate::push_credentials::PushCredentialState;
@ -158,20 +157,6 @@ pub(crate) async fn clone_github_repo(
)
.await
.map_err(|failure| failure.error)?;
if let Some(pin) =
PinnedRevision::from_selectors(plan.tag.as_deref(), plan.commit_sha.as_deref())
{
let head = run_local_step(
exec,
&clone_source::exact_head_revision_command(&layout.primary_repo_path),
"git rev-parse HEAD (pinned checkout)",
deadline,
auth_url.as_ref(),
has_app,
)
.await?;
pin.verify_head(&head.stdout_lossy())?;
}
run_local_step(
exec,

View file

@ -73,19 +73,21 @@ pub(crate) fn repo_symlink_command(layout: &GitHubRepoLayout) -> String {
)
}
/// A revision the checkout is pinned to instead of the branch's current HEAD.
/// The kind of revision a checkout is pinned to instead of the branch's
/// current HEAD.
///
/// The working branch names the checkout the run works on; it never constrains
/// which revision is fetched. No layer proves branch/revision ancestry, and an
/// unavailable revision fails without falling back to branch HEAD.
#[derive(Debug, Clone, PartialEq, Eq)]
/// which revision is fetched. No layer proves branch/revision ancestry. The
/// driver fetches the pin directly and attaches the branch to it, so an
/// unavailable revision fails the clone without falling back to branch HEAD,
/// and a successful clone has the pin checked out.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub(crate) enum PinnedRevision {
/// An exact commit SHA, already normalized by
/// [`normalize_exact_commit_sha`].
Commit(String),
/// An exact commit SHA.
Commit,
/// A bare tag name; the driver fetches it as `refs/tags/<tag>` so a
/// same-named branch is never consulted.
Tag(String),
Tag,
}
impl PinnedRevision {
@ -93,60 +95,19 @@ impl PinnedRevision {
/// target as durable identity but does not drive the checkout.
pub(crate) fn from_selectors(tag: Option<&str>, commit_sha: Option<&str>) -> Option<Self> {
match (commit_sha, tag) {
(Some(sha), _) => Some(Self::Commit(sha.to_string())),
(None, Some(tag)) => Some(Self::Tag(tag.to_string())),
(Some(_), _) => Some(Self::Commit),
(None, Some(_)) => Some(Self::Tag),
(None, None) => None,
}
}
/// Human-readable prefix for error messages.
pub(crate) fn label(&self) -> &'static str {
pub(crate) fn label(self) -> &'static str {
match self {
Self::Commit(_) => "Exact commit checkout",
Self::Tag(_) => "Tag checkout",
Self::Commit => "Exact commit checkout",
Self::Tag => "Tag checkout",
}
}
/// The commit HEAD must resolve to after checkout, when one is known.
pub(crate) fn expected_sha(&self) -> Option<&str> {
match self {
Self::Commit(sha) => Some(sha),
Self::Tag(_) => None,
}
}
/// Validate the `rev-parse HEAD` output of a pinned checkout and return the
/// resolved commit ID.
pub(crate) fn verify_head(&self, output: &str) -> crate::Result<String> {
let actual_sha = verify_resolved_head(output)?;
if self
.expected_sha()
.is_some_and(|expected| expected != actual_sha)
{
return Err(crate::Error::message(
"Exact checkout HEAD did not match the requested commit",
));
}
Ok(actual_sha)
}
}
/// Print the current HEAD commit and nothing else, for
/// [`PinnedRevision::verify_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,
)
}
/// Validate that a `rev-parse HEAD` output is a single commit ID and return it
/// normalized.
pub(crate) fn verify_resolved_head(output: &str) -> crate::Result<String> {
normalize_exact_commit_sha(output.trim()).map_err(|err| {
crate::Error::context("Pinned checkout produced an invalid HEAD commit ID", err)
})
}
fn trim_root(root: &str) -> &str {
@ -334,13 +295,17 @@ mod tests {
}
#[test]
fn pinned_revision_prefers_exact_commit_and_qualifies_tags() {
fn pinned_revision_prefers_exact_commit_over_a_tag() {
let sha = "0123456789abcdef0123456789abcdef01234567";
assert_eq!(PinnedRevision::from_selectors(None, None), None);
let tag = PinnedRevision::from_selectors(Some("release/v1"), None).unwrap();
assert_eq!(tag.expected_sha(), None);
let commit = PinnedRevision::from_selectors(Some("release/v1"), Some(sha)).unwrap();
assert_eq!(commit.expected_sha(), Some(sha));
assert_eq!(
PinnedRevision::from_selectors(Some("release/v1"), None),
Some(PinnedRevision::Tag)
);
assert_eq!(
PinnedRevision::from_selectors(Some("release/v1"), Some(sha)),
Some(PinnedRevision::Commit)
);
}
#[test]
@ -468,25 +433,6 @@ mod tests {
assert!(empty_tag.to_string().contains("non-empty tag"));
}
#[test]
fn exact_checkout_verification_rejects_invalid_or_mismatched_head() {
let expected = "0123456789abcdef0123456789abcdef01234567";
let pin = PinnedRevision::Commit(expected.to_string());
pin.verify_head("0123456789ABCDEF0123456789ABCDEF01234567\n")
.expect("uppercase command output should normalize");
let invalid = pin
.verify_head("fatal: not a revision")
.expect_err("non-SHA output should fail verification");
assert!(invalid.to_string().contains("invalid HEAD commit ID"));
assert!(!invalid.to_string().contains("fatal: not a revision"));
let mismatched = pin
.verify_head("1123456789abcdef0123456789abcdef01234567")
.expect_err("mismatched SHA should fail verification");
assert!(mismatched.to_string().contains("did not match"));
}
#[test]
fn github_layout_maps_ssh_origin_to_repos_checkout_and_workspace_link() {
let layout = github_repo_layout(