Skip to content

Fall back to HTTP when Apple Git is unavailable #31496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fc-oai wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fall back to HTTP when Apple Git is unavailable #31496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions codex-rs/core-plugins/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ tokio = { workspace = true, features = ["fs", "macros", "rt", "time"] }
toml = { workspace = true }
tracing = { workspace = true }
url = { workspace = true }
which = { workspace = true }
zip = { workspace = true }

[dev-dependencies]
Expand Down
82 changes: 68 additions & 14 deletions codex-rs/core-plugins/src/startup_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ struct CuratedPluginsBackupArchiveResponse {
download_url: String,
}

#[derive(Debug, Eq, PartialEq)]
enum GitTransport {
Available(PathBuf),
Unavailable,
}

pub fn curated_plugins_repo_path(codex_home: &Path) -> PathBuf {
codex_home.join(CURATED_PLUGINS_RELATIVE_DIR)
}
Expand All @@ -93,23 +99,31 @@ fn curated_plugins_sha_path(codex_home: &Path) -> PathBuf {
}

pub fn sync_openai_plugins_repo(codex_home: &Path) -> Result<String, String> {
let git_transport = resolve_git_transport("git");
sync_openai_plugins_repo_with_transport_overrides(
codex_home,
"git",
&git_transport,
GITHUB_API_BASE_URL,
CURATED_PLUGINS_BACKUP_ARCHIVE_API_URL,
)
}

fn sync_openai_plugins_repo_with_transport_overrides(
codex_home: &Path,
git_binary: &str,
git_transport: &GitTransport,
api_base_url: &str,
backup_archive_api_url: &str,
) -> Result<String, String> {
let _file_guard = lock_curated_plugins_startup_sync(codex_home)?;

match sync_openai_plugins_repo_via_git(codex_home, git_binary) {
let git_sync_result = match git_transport {
GitTransport::Available(git_binary) => {
sync_openai_plugins_repo_via_git(codex_home, git_binary)
}
GitTransport::Unavailable => Err("git executable is unavailable".to_string()),
};

match git_sync_result {
Ok(remote_sha) => {
emit_curated_plugins_startup_sync_metric("git", "success");
emit_curated_plugins_startup_sync_final_metric("git", "success");
Expand All @@ -119,7 +133,6 @@ fn sync_openai_plugins_repo_with_transport_overrides(
emit_curated_plugins_startup_sync_metric("git", "failure");
warn!(
error = %err,
git_binary,
"git sync failed for curated plugin sync; falling back to GitHub HTTP"
);
match sync_openai_plugins_repo_via_http(codex_home, api_base_url) {
Expand Down Expand Up @@ -182,7 +195,10 @@ fn lock_curated_plugins_startup_sync(codex_home: &Path) -> Result<File, String>
Ok(lock_file)
}

fn sync_openai_plugins_repo_via_git(codex_home: &Path, git_binary: &str) -> Result<String, String> {
fn sync_openai_plugins_repo_via_git(
codex_home: &Path,
git_binary: &Path,
) -> Result<String, String> {
let repo_path = curated_plugins_repo_path(codex_home);
let sha_path = codex_home.join(CURATED_PLUGINS_SHA_FILE);
let remote_sha = git_ls_remote_head_sha(git_binary)?;
Expand Down Expand Up @@ -229,7 +245,7 @@ fn sync_openai_plugins_repo_via_git(codex_home: &Path, git_binary: &str) -> Resu
fn fetch_curated_plugins_commit(
repo_path: &Path,
remote_sha: &str,
git_binary: &str,
git_binary: &Path,
) -> Result<(), String> {
fetch_curated_plugins_commit_from(
repo_path,
Expand All @@ -244,7 +260,7 @@ fn fetch_curated_plugins_commit_from_source(
repo_path: &Path,
source_repo_path: &Path,
remote_sha: &str,
git_binary: &str,
git_binary: &Path,
) -> Result<(), String> {
fetch_curated_plugins_commit_from(
repo_path,
Expand All @@ -259,7 +275,7 @@ fn fetch_curated_plugins_commit_from(
repo_path: &Path,
source: &Path,
source_revision: &str,
git_binary: &str,
git_binary: &Path,
context: &str,
) -> Result<(), String> {
let fetch_refspec = format!("+{source_revision}:{CURATED_PLUGINS_FETCH_REF}");
Expand All @@ -274,7 +290,7 @@ fn fetch_curated_plugins_commit_from(
ensure_git_success(&output, context)
}

fn reset_curated_plugins_checkout(repo_path: &Path, git_binary: &str) -> Result<(), String> {
fn reset_curated_plugins_checkout(repo_path: &Path, git_binary: &Path) -> Result<(), String> {
run_git_in_repo(
repo_path,
git_binary,
Expand All @@ -291,7 +307,7 @@ fn reset_curated_plugins_checkout(repo_path: &Path, git_binary: &str) -> Result<

fn run_git_in_repo(
repo_path: &Path,
git_binary: &str,
git_binary: &Path,
args: &[&str],
context: &str,
) -> Result<(), String> {
Expand Down Expand Up @@ -583,7 +599,7 @@ fn write_curated_plugins_sha(sha_path: &Path, remote_sha: &str) -> Result<(), St
fn read_local_git_or_sha_file(
repo_path: &Path,
sha_path: &Path,
git_binary: &str,
git_binary: &Path,
) -> Option<String> {
if repo_path.join(".git").is_dir()
&& let Ok(sha) = git_head_sha(repo_path, git_binary)
Expand All @@ -594,7 +610,7 @@ fn read_local_git_or_sha_file(
read_sha_file(sha_path)
}

fn git_ls_remote_head_sha(git_binary: &str) -> Result<String, String> {
fn git_ls_remote_head_sha(git_binary: &Path) -> Result<String, String> {
let mut command = git_command(git_binary);
command
.arg("ls-remote")
Expand Down Expand Up @@ -622,7 +638,7 @@ fn git_ls_remote_head_sha(git_binary: &str) -> Result<String, String> {
Ok(sha.to_string())
}

fn git_head_sha(repo_path: &Path, git_binary: &str) -> Result<String, String> {
fn git_head_sha(repo_path: &Path, git_binary: &Path) -> Result<String, String> {
let output = git_command(git_binary)
.arg("-C")
.arg(repo_path)
Expand All @@ -647,7 +663,7 @@ fn git_head_sha(repo_path: &Path, git_binary: &str) -> Result<String, String> {
Ok(sha)
}

fn git_command(git_binary: &str) -> Command {
fn git_command(git_binary: &Path) -> Command {
let mut command = Command::new(git_binary);
command.env("GIT_OPTIONAL_LOCKS", "0");
for name in REPOSITORY_LOCAL_GIT_ENVIRONMENT_VARIABLES {
Expand All @@ -656,6 +672,44 @@ fn git_command(git_binary: &str) -> Command {
command
}

fn resolve_git_transport(git_binary: &str) -> GitTransport {
let Ok(git_path) = which::which(git_binary) else {
return GitTransport::Unavailable;
};

#[cfg(target_os = "macos")]
{
macos_git_transport_from_path(git_path, apple_developer_tools_available())
}
#[cfg(not(target_os = "macos"))]
{
GitTransport::Available(git_path)
}
}

#[cfg(any(target_os = "macos", test))]
fn macos_git_transport_from_path(
git_path: PathBuf,
apple_developer_tools_available: bool,
) -> GitTransport {
if git_path == Path::new("/usr/bin/git") && !apple_developer_tools_available {
GitTransport::Unavailable
} else {
GitTransport::Available(git_path)
}
}

#[cfg(target_os = "macos")]
fn apple_developer_tools_available() -> bool {
Command::new("/usr/bin/xcode-select")
.arg("-p")
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.status()
.is_ok_and(|status| status.success())
}

fn run_git_command_with_timeout(
command: &mut Command,
context: &str,
Expand Down
93 changes: 78 additions & 15 deletions codex-rs/core-plugins/src/startup_sync_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const TEST_CURATED_PLUGIN_SHA: &str = "0123456789abcdef0123456789abcdef01234567"

#[test]
fn git_command_sanitizes_ambient_repository_environment() {
let command = git_command("git");
let command = git_command(Path::new("git"));

for name in REPOSITORY_LOCAL_GIT_ENVIRONMENT_VARIABLES {
assert_eq!(
Expand Down Expand Up @@ -190,9 +190,29 @@ async fn run_sync_with_transport_overrides(
let api_base_url = api_base_url.into();
let backup_archive_api_url = backup_archive_api_url.into();
tokio::task::spawn_blocking(move || {
let git_transport = GitTransport::Available(PathBuf::from(git_binary));
sync_openai_plugins_repo_with_transport_overrides(
codex_home.as_path(),
&git_binary,
&git_transport,
&api_base_url,
&backup_archive_api_url,
)
})
.await
.expect("sync task should join")
}

async fn run_sync_without_git(
codex_home: PathBuf,
api_base_url: impl Into<String>,
backup_archive_api_url: impl Into<String>,
) -> Result<String, String> {
let api_base_url = api_base_url.into();
let backup_archive_api_url = backup_archive_api_url.into();
tokio::task::spawn_blocking(move || {
sync_openai_plugins_repo_with_transport_overrides(
codex_home.as_path(),
&GitTransport::Unavailable,
&api_base_url,
&backup_archive_api_url,
)
Expand Down Expand Up @@ -343,7 +363,7 @@ exit 1
barrier.wait();
sync_openai_plugins_repo_with_transport_overrides(
tmp.path(),
git_path.to_str().expect("utf8 path"),
&GitTransport::Available(git_path.clone()),
"http://127.0.0.1:9",
"http://127.0.0.1:9/backend-api/plugins/export/curated",
)
Expand Down Expand Up @@ -463,9 +483,8 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() {
),
);

let synced_sha =
sync_openai_plugins_repo_via_git(tmp.path(), git_wrapper.to_str().expect("utf8 path"))
.expect("git sync should succeed");
let synced_sha = sync_openai_plugins_repo_via_git(tmp.path(), &git_wrapper)
.expect("git sync should succeed");

assert_eq!(synced_sha, sha);
assert_curated_gmail_repo(&curated_plugins_repo_path(tmp.path()));
Expand Down Expand Up @@ -517,9 +536,8 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() {
.trim()
.to_string();

let synced_sha =
sync_openai_plugins_repo_via_git(tmp.path(), git_wrapper.to_str().expect("utf8 path"))
.expect("incremental git sync should succeed");
let synced_sha = sync_openai_plugins_repo_via_git(tmp.path(), &git_wrapper)
.expect("incremental git sync should succeed");

assert_eq!(synced_sha, updated_sha);
assert!(
Expand Down Expand Up @@ -573,9 +591,8 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() {
assert!(!has_plugins_clone_dirs(tmp.path()));

let unchanged_sync_invocation_count = invocation_log_contents.lines().count();
let synced_sha =
sync_openai_plugins_repo_via_git(tmp.path(), git_wrapper.to_str().expect("utf8 path"))
.expect("unchanged git sync should succeed");
let synced_sha = sync_openai_plugins_repo_via_git(tmp.path(), &git_wrapper)
.expect("unchanged git sync should succeed");

assert_eq!(synced_sha, updated_sha);
let invocation_log = std::fs::read_to_string(&invocation_log).expect("read sync invocations");
Expand Down Expand Up @@ -619,6 +636,52 @@ async fn sync_openai_plugins_repo_falls_back_to_http_when_git_is_unavailable() {
assert_eq!(read_curated_plugins_sha(tmp.path()).as_deref(), Some(sha));
}

#[test]
fn apple_git_without_developer_tools_is_unavailable() {
assert_eq!(
macos_git_transport_from_path(
PathBuf::from("/usr/bin/git"),
/*apple_developer_tools_available*/ false,
),
GitTransport::Unavailable
);
assert_eq!(
macos_git_transport_from_path(
PathBuf::from("/usr/bin/git"),
/*apple_developer_tools_available*/ true,
),
GitTransport::Available(PathBuf::from("/usr/bin/git"))
);
assert_eq!(
macos_git_transport_from_path(
PathBuf::from("/opt/homebrew/bin/git"),
/*apple_developer_tools_available*/ false,
),
GitTransport::Available(PathBuf::from("/opt/homebrew/bin/git"))
);
}

#[tokio::test]
async fn sync_openai_plugins_repo_uses_http_without_git_transport() {
let tmp = tempdir().expect("tempdir");
let server = MockServer::start().await;
let sha = "0123456789abcdef0123456789abcdef01234567";

mount_github_repo_and_ref(&server, sha).await;
mount_github_zipball(&server, sha, curated_repo_zipball_bytes(sha)).await;

let synced_sha = run_sync_without_git(
tmp.path().to_path_buf(),
server.uri(),
"http://127.0.0.1:9/backend-api/plugins/export/curated",
)
.await
.expect("HTTP sync should succeed");

assert_eq!(synced_sha, sha);
assert_curated_gmail_repo(&curated_plugins_repo_path(tmp.path()));
}

#[cfg(unix)]
#[tokio::test]
async fn sync_openai_plugins_repo_falls_back_to_http_when_git_sync_fails() {
Expand Down Expand Up @@ -690,8 +753,8 @@ exit 1
),
);

let err = sync_openai_plugins_repo_via_git(tmp.path(), git_path.to_str().expect("utf8 path"))
.expect_err("git sync should fail");
let err =
sync_openai_plugins_repo_via_git(tmp.path(), &git_path).expect_err("git sync should fail");

assert!(err.contains("fatal: early EOF"));
assert!(!has_plugins_clone_dirs(tmp.path()));
Expand Down Expand Up @@ -755,7 +818,7 @@ exit 1
),
);

let err = sync_openai_plugins_repo_via_git(tmp.path(), git_path.to_str().expect("utf8 path"))
let err = sync_openai_plugins_repo_via_git(tmp.path(), &git_path)
.expect_err("invalid staged checkout should fail");

assert!(err.contains("curated plugins archive missing marketplace manifest"));
Expand Down
Loading