mirror of
https://github.com/openai/codex.git
synced 2026-10-25 18:28:38 +00:00
Harden local default-branch discovery variants
This commit is contained in:
Chris Bookholt
@@ -29,6 +29,7 @@ use std::fs;
|
||||
use std::io;
|
||||
#[cfg(unix)]
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::TempDir;
|
||||
use tokio::process::Command;
|
||||
@@ -302,53 +303,233 @@ async fn create_test_git_repo_with_remote(temp_dir: &TempDir) -> (PathBuf, Strin
|
||||
(repo_path, branch)
|
||||
}
|
||||
|
||||
async fn run_test_git(cwd: &Path, args: &[&str]) -> std::process::Output {
|
||||
Command::new("git")
|
||||
.args(args)
|
||||
.current_dir(cwd)
|
||||
.output()
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("failed to run git {args:?}: {error}"))
|
||||
}
|
||||
|
||||
async fn assert_test_git(cwd: &Path, args: &[&str]) {
|
||||
let output = run_test_git(cwd, args).await;
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"git {args:?} failed: {}",
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_default_branch_discovery_uses_verified_remote_head() {
|
||||
let temp_dir = TempDir::new().expect("Failed to create temp dir");
|
||||
let repo_path = create_test_git_repo(&temp_dir).await;
|
||||
assert_test_git(&repo_path, &["branch", "-M", "main"]).await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["remote", "add", "upstream", "/nonexistent/upstream.git"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["update-ref", "refs/remotes/upstream/release/v1", "HEAD"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"symbolic-ref",
|
||||
"refs/remotes/upstream/HEAD",
|
||||
"refs/remotes/upstream/release/v1",
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
default_branch_name(&repo_path).await,
|
||||
Some("release/v1".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_default_branch_discovery_rejects_stale_and_cross_remote_heads() {
|
||||
let temp_dir = TempDir::new().expect("Failed to create temp dir");
|
||||
let repo_path = create_test_git_repo(&temp_dir).await;
|
||||
assert_test_git(&repo_path, &["branch", "-M", "main"]).await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["remote", "add", "origin", "/nonexistent/origin.git"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["remote", "add", "upstream", "/nonexistent/upstream.git"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"symbolic-ref",
|
||||
"refs/remotes/origin/HEAD",
|
||||
"refs/remotes/upstream/poisoned",
|
||||
],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"symbolic-ref",
|
||||
"refs/remotes/upstream/HEAD",
|
||||
"refs/remotes/upstream/deleted",
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
default_branch_name(&repo_path).await,
|
||||
Some("main".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_default_branch_discovery_without_remotes_prefers_main_then_master() {
|
||||
let temp_dir = TempDir::new().expect("Failed to create temp dir");
|
||||
let repo_path = create_test_git_repo(&temp_dir).await;
|
||||
assert_test_git(&repo_path, &["branch", "-M", "master"]).await;
|
||||
|
||||
assert_eq!(
|
||||
default_branch_name(&repo_path).await,
|
||||
Some("master".to_string())
|
||||
);
|
||||
|
||||
assert_test_git(&repo_path, &["branch", "main"]).await;
|
||||
assert_eq!(
|
||||
default_branch_name(&repo_path).await,
|
||||
Some("main".to_string())
|
||||
);
|
||||
|
||||
assert_test_git(&repo_path, &["branch", "-D", "main"]).await;
|
||||
assert_test_git(&repo_path, &["branch", "-M", "feature"]).await;
|
||||
assert_eq!(default_branch_name(&repo_path).await, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_default_branch_discovery_from_linked_worktree() {
|
||||
let temp_dir = TempDir::new().expect("Failed to create temp dir");
|
||||
let repo_path = create_test_git_repo(&temp_dir).await;
|
||||
let worktree_path = temp_dir.path().join("linked-worktree");
|
||||
assert_test_git(&repo_path, &["branch", "-M", "main"]).await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["remote", "add", "mirror", "/nonexistent/mirror.git"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["update-ref", "refs/remotes/mirror/trunk", "HEAD"],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"symbolic-ref",
|
||||
"refs/remotes/mirror/HEAD",
|
||||
"refs/remotes/mirror/trunk",
|
||||
],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"worktree",
|
||||
"add",
|
||||
"-b",
|
||||
"feature",
|
||||
worktree_path.to_string_lossy().as_ref(),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
default_branch_name(&worktree_path).await,
|
||||
Some("trunk".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn test_default_branch_discovery_does_not_invoke_remote_transport() {
|
||||
let temp_dir = TempDir::new().expect("Failed to create temp dir");
|
||||
let repo_path = create_test_git_repo(&temp_dir).await;
|
||||
let helper_path = temp_dir.path().join("ssh-helper.sh");
|
||||
let marker_path = temp_dir.path().join("transport-ran");
|
||||
fs::write(
|
||||
&helper_path,
|
||||
format!(
|
||||
"#!/bin/sh\nprintf ran > \"{}\"\nexit 1\n",
|
||||
marker_path.to_string_lossy()
|
||||
),
|
||||
)
|
||||
.expect("write transport helper");
|
||||
let mut permissions = fs::metadata(&helper_path)
|
||||
.expect("read helper metadata")
|
||||
.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
fs::set_permissions(&helper_path, permissions).expect("mark helper executable");
|
||||
let ssh_helper = temp_dir.path().join("ssh-helper.sh");
|
||||
let ssh_marker = temp_dir.path().join("ssh-helper-ran");
|
||||
let ext_helper = temp_dir.path().join("ext-helper.sh");
|
||||
let ext_marker = temp_dir.path().join("ext-helper-ran");
|
||||
let credential_helper = temp_dir.path().join("credential-helper.sh");
|
||||
let credential_marker = temp_dir.path().join("credential-helper-ran");
|
||||
|
||||
let add_remote = Command::new("git")
|
||||
.args(["remote", "add", "origin", "ssh://example.invalid/repo"])
|
||||
.current_dir(&repo_path)
|
||||
.output()
|
||||
.await
|
||||
.expect("add remote");
|
||||
assert!(
|
||||
add_remote.status.success(),
|
||||
"add remote: {}",
|
||||
String::from_utf8_lossy(&add_remote.stderr)
|
||||
);
|
||||
let configure_helper = Command::new("git")
|
||||
.args([
|
||||
for (helper, marker, output) in [
|
||||
(&ssh_helper, &ssh_marker, ""),
|
||||
(&ext_helper, &ext_marker, ""),
|
||||
(
|
||||
&credential_helper,
|
||||
&credential_marker,
|
||||
"printf 'username=test\\npassword=test\\n'\n",
|
||||
),
|
||||
] {
|
||||
fs::write(
|
||||
helper,
|
||||
format!(
|
||||
"#!/bin/sh\nprintf ran > \"{}\"\n{output}exit 1\n",
|
||||
marker.to_string_lossy()
|
||||
),
|
||||
)
|
||||
.expect("write transport helper");
|
||||
let mut permissions = fs::metadata(helper)
|
||||
.expect("read helper metadata")
|
||||
.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
fs::set_permissions(helper, permissions).expect("mark helper executable");
|
||||
}
|
||||
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&["remote", "add", "origin", "ssh://example.invalid/repo"],
|
||||
)
|
||||
.await;
|
||||
let ext_url = format!("ext::{}", ext_helper.to_string_lossy());
|
||||
assert_test_git(&repo_path, &["remote", "add", "extension", &ext_url]).await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"remote",
|
||||
"add",
|
||||
"credentialed",
|
||||
"https://example.invalid/repo",
|
||||
],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"config",
|
||||
"core.sshCommand",
|
||||
helper_path.to_string_lossy().as_ref(),
|
||||
])
|
||||
.current_dir(&repo_path)
|
||||
.output()
|
||||
.await
|
||||
.expect("configure transport helper");
|
||||
assert!(
|
||||
configure_helper.status.success(),
|
||||
"configure transport helper: {}",
|
||||
String::from_utf8_lossy(&configure_helper.stderr)
|
||||
);
|
||||
ssh_helper.to_string_lossy().as_ref(),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
assert_test_git(&repo_path, &["config", "protocol.ext.allow", "always"]).await;
|
||||
assert_test_git(
|
||||
&repo_path,
|
||||
&[
|
||||
"config",
|
||||
"credential.helper",
|
||||
credential_helper.to_string_lossy().as_ref(),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let branch_output = Command::new("git")
|
||||
.args(["branch", "--show-current"])
|
||||
.current_dir(&repo_path)
|
||||
@@ -366,10 +547,13 @@ async fn test_default_branch_discovery_does_not_invoke_remote_transport() {
|
||||
.to_string();
|
||||
|
||||
assert_eq!(default_branch_name(&repo_path).await, Some(branch));
|
||||
assert!(
|
||||
!marker_path.exists(),
|
||||
"default branch discovery must stay on local refs"
|
||||
);
|
||||
for marker in [ssh_marker, ext_marker, credential_marker] {
|
||||
assert!(
|
||||
!marker.exists(),
|
||||
"default branch discovery invoked {}",
|
||||
marker.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -487,23 +487,8 @@ async fn get_default_branch(cwd: &Path) -> Option<String> {
|
||||
// Prefer the first remote (with origin prioritized)
|
||||
let remotes = get_git_remotes(cwd).await.unwrap_or_default();
|
||||
for remote in remotes {
|
||||
// Try symbolic-ref, which returns something like: refs/remotes/origin/main
|
||||
if let Some(symref_output) = run_git_command_with_timeout(
|
||||
&[
|
||||
"symbolic-ref",
|
||||
"--quiet",
|
||||
&format!("refs/remotes/{remote}/HEAD"),
|
||||
],
|
||||
cwd,
|
||||
)
|
||||
.await
|
||||
&& symref_output.status.success()
|
||||
&& let Ok(sym) = String::from_utf8(symref_output.stdout)
|
||||
{
|
||||
let trimmed = sym.trim();
|
||||
if let Some((_, name)) = trimmed.rsplit_once('/') {
|
||||
return Some(name.to_string());
|
||||
}
|
||||
if let Some(branch) = get_remote_default_branch_from_symbolic_ref(cwd, &remote).await {
|
||||
return Some(branch);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -511,6 +496,32 @@ async fn get_default_branch(cwd: &Path) -> Option<String> {
|
||||
get_default_branch_local(cwd).await
|
||||
}
|
||||
|
||||
/// Resolves one remote's symbolic HEAD using only local refs.
|
||||
///
|
||||
/// The symbolic target must remain inside the selected remote's namespace and
|
||||
/// resolve to an existing ref. This rejects stale or cross-remote symbolic refs
|
||||
/// while preserving branch names that contain `/`.
|
||||
async fn get_remote_default_branch_from_symbolic_ref(cwd: &Path, remote: &str) -> Option<String> {
|
||||
let remote_head = format!("refs/remotes/{remote}/HEAD");
|
||||
let symref_output =
|
||||
run_git_command_with_timeout(&["symbolic-ref", "--quiet", &remote_head], cwd).await?;
|
||||
if !symref_output.status.success() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let target = String::from_utf8(symref_output.stdout).ok()?;
|
||||
let target = target.trim();
|
||||
let remote_ref_prefix = format!("refs/remotes/{remote}/");
|
||||
let branch = target.strip_prefix(&remote_ref_prefix)?;
|
||||
if branch.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let verify =
|
||||
run_git_command_with_timeout(&["rev-parse", "--verify", "--quiet", target], cwd).await?;
|
||||
verify.status.success().then(|| branch.to_string())
|
||||
}
|
||||
|
||||
/// Determine the repository's default branch name, if available.
|
||||
///
|
||||
/// This inspects remote configuration first (including the symbolic `HEAD`
|
||||
|
||||
@@ -526,7 +526,7 @@ mod tests {
|
||||
deletions: 0,
|
||||
}
|
||||
);
|
||||
assert!(runner.saw(&["git", "merge-base", "HEAD", "refs/remotes/origin/main"]));
|
||||
runner.assert_finished();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -566,6 +566,140 @@ mod tests {
|
||||
|
||||
assert_eq!(stats.additions, 1);
|
||||
assert!(!runner.saw(&["git", "remote", "show", "origin"]));
|
||||
runner.assert_finished();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn branch_diff_stats_skips_malformed_and_stale_remote_heads() {
|
||||
let runner = FakeRunner::new(vec![
|
||||
response(
|
||||
&["git", "rev-parse", "--git-dir"],
|
||||
/*exit_code*/ 0,
|
||||
".git\n",
|
||||
),
|
||||
response(
|
||||
&["git", "remote"],
|
||||
/*exit_code*/ 0,
|
||||
"upstream\nbackup\norigin\n",
|
||||
),
|
||||
response(
|
||||
&["git", "symbolic-ref", "--quiet", "refs/remotes/origin/HEAD"],
|
||||
/*exit_code*/ 0,
|
||||
"refs/remotes/attacker/main\n",
|
||||
),
|
||||
response(
|
||||
&[
|
||||
"git",
|
||||
"symbolic-ref",
|
||||
"--quiet",
|
||||
"refs/remotes/upstream/HEAD",
|
||||
],
|
||||
/*exit_code*/ 0,
|
||||
"refs/remotes/upstream/deleted\n",
|
||||
),
|
||||
response(
|
||||
&[
|
||||
"git",
|
||||
"rev-parse",
|
||||
"--verify",
|
||||
"--quiet",
|
||||
"refs/remotes/upstream/deleted",
|
||||
],
|
||||
/*exit_code*/ 1,
|
||||
"",
|
||||
),
|
||||
response(
|
||||
&["git", "symbolic-ref", "--quiet", "refs/remotes/backup/HEAD"],
|
||||
/*exit_code*/ 0,
|
||||
"refs/remotes/backup/release/v1\n",
|
||||
),
|
||||
response(
|
||||
&[
|
||||
"git",
|
||||
"rev-parse",
|
||||
"--verify",
|
||||
"--quiet",
|
||||
"refs/remotes/backup/release/v1",
|
||||
],
|
||||
/*exit_code*/ 0,
|
||||
"remote-release-sha\n",
|
||||
),
|
||||
response(
|
||||
&[
|
||||
"git",
|
||||
"merge-base",
|
||||
"HEAD",
|
||||
"refs/remotes/backup/release/v1",
|
||||
],
|
||||
/*exit_code*/ 0,
|
||||
"base-sha\n",
|
||||
),
|
||||
response(
|
||||
&["git", "diff", "--numstat", "base-sha..HEAD"],
|
||||
/*exit_code*/ 0,
|
||||
"3\t2\tfile\n",
|
||||
),
|
||||
]);
|
||||
|
||||
let stats = branch_diff_stats_to_default_branch(&runner, Path::new("/repo"))
|
||||
.await
|
||||
.expect("branch diff stats");
|
||||
|
||||
assert_eq!(
|
||||
stats,
|
||||
GitBranchDiffStats {
|
||||
additions: 3,
|
||||
deletions: 2,
|
||||
}
|
||||
);
|
||||
runner.assert_finished();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn branch_diff_stats_without_remotes_uses_master_fallback() {
|
||||
let runner = FakeRunner::new(vec![
|
||||
response(
|
||||
&["git", "rev-parse", "--git-dir"],
|
||||
/*exit_code*/ 0,
|
||||
".git\n",
|
||||
),
|
||||
response(&["git", "remote"], /*exit_code*/ 0, ""),
|
||||
response(
|
||||
&["git", "rev-parse", "--verify", "--quiet", "refs/heads/main"],
|
||||
/*exit_code*/ 1,
|
||||
"",
|
||||
),
|
||||
response(
|
||||
&[
|
||||
"git",
|
||||
"rev-parse",
|
||||
"--verify",
|
||||
"--quiet",
|
||||
"refs/heads/master",
|
||||
],
|
||||
/*exit_code*/ 0,
|
||||
"master-sha\n",
|
||||
),
|
||||
response(
|
||||
&["git", "merge-base", "HEAD", "refs/heads/master"],
|
||||
/*exit_code*/ 0,
|
||||
"base-sha\n",
|
||||
),
|
||||
response(
|
||||
&["git", "diff", "--numstat", "base-sha..HEAD"],
|
||||
/*exit_code*/ 0,
|
||||
"0\t0\tfile\n",
|
||||
),
|
||||
]);
|
||||
|
||||
assert_eq!(
|
||||
branch_diff_stats_to_default_branch(&runner, Path::new("/repo")).await,
|
||||
Some(GitBranchDiffStats {
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
})
|
||||
);
|
||||
runner.assert_finished();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -671,9 +805,13 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
fn command(argv: &[&str]) -> Vec<String> {
|
||||
argv.iter().map(|arg| (*arg).to_string()).collect()
|
||||
}
|
||||
|
||||
fn response(argv: &[&str], exit_code: i32, stdout: &str) -> FakeResponse {
|
||||
FakeResponse {
|
||||
argv: argv.iter().map(|arg| (*arg).to_string()).collect(),
|
||||
argv: command(argv),
|
||||
output: WorkspaceCommandOutput {
|
||||
exit_code,
|
||||
stdout: stdout.to_string(),
|
||||
@@ -701,13 +839,20 @@ mod tests {
|
||||
}
|
||||
|
||||
fn saw(&self, argv: &[&str]) -> bool {
|
||||
let argv: Vec<String> = argv.iter().map(|arg| (*arg).to_string()).collect();
|
||||
let argv = command(argv);
|
||||
self.seen
|
||||
.lock()
|
||||
.expect("seen lock")
|
||||
.iter()
|
||||
.any(|seen| seen == &argv)
|
||||
}
|
||||
|
||||
fn assert_finished(&self) {
|
||||
assert!(
|
||||
self.responses.lock().expect("responses lock").is_empty(),
|
||||
"not all expected commands ran"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
impl WorkspaceCommandExecutor for FakeRunner {
|
||||
@@ -727,11 +872,10 @@ mod tests {
|
||||
.push(command.argv.clone());
|
||||
Box::pin(async move {
|
||||
let mut responses = self.responses.lock().expect("responses lock");
|
||||
let index = responses
|
||||
.iter()
|
||||
.position(|response| response.argv == command.argv)
|
||||
.unwrap_or_else(|| panic!("missing fake response for {:?}", command.argv));
|
||||
let response = responses.remove(index).expect("fake response");
|
||||
let response = responses.pop_front().unwrap_or_else(|| {
|
||||
panic!("missing fake response for command {:?}", command.argv)
|
||||
});
|
||||
assert_eq!(response.argv, command.argv, "unexpected command sequence");
|
||||
Ok(response.output)
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user