mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Fall back to HTTP when Apple Git is unavailable (#31496)
## Summary - on macOS, resolve the curated plugin sync Git executable without executing it - treat Apple’s `/usr/bin/git` shim as unavailable when `xcode-select -p` reports that developer tools are absent - skip directly to the existing GitHub HTTP fallback in that case - preserve the original `git` command lookup on Windows and Linux, including CCA ## Root cause Curated plugin startup sync invokes `git ls-remote` before its HTTP fallback. On a clean Mac, `git` resolves to Apple’s `/usr/bin/git` shim, and executing the shim opens the Xcode Command Line Tools installer before the process can fail and reach HTTP. On macOS, this change resolves Git through `PATH` without executing it. If the selected binary is Apple’s shim and developer tools are unavailable, startup sync marks the Git transport unavailable and enters the existing HTTP fallback immediately. The new availability detection is macOS-only by construction. Windows and Linux still execute the literal `git` command as before. If Git is missing on Windows, the existing spawn-error path falls back to HTTP; Linux/CCA receives no new lookup or startup behavior. ## Eager Git audit I also audited production Git process spawns in `codex-rs`. - This curated catalog sync is the only default projectless app-server startup path found. - Configured Git marketplace auto-upgrade runs Git at plugin startup, but only after a user has explicitly configured a Git marketplace. - Experimental Memories has background Git metadata/baseline paths when the feature is enabled. - The separate cloud-tasks UI probes Git during environment autodetection. - Normal thread/turn Git metadata is gated by filesystem discovery of an existing `.git` entry. - Marketplace add/install, patch apply, doctor, and TUI `/diff` paths are explicitly user-invoked. ## Validation - `just fmt` - `just bazel-lock-update` — succeeded with no lockfile delta - `just test -p codex-core-plugins` — 313 passed - `just fix -p codex-core-plugins` — completed; emitted one pre-existing unrelated `large_enum_variant` warning in `manifest.rs` - `git diff --check`
This commit is contained in:
Generated
+1
@@ -2805,6 +2805,7 @@ dependencies = [
|
||||
"tracing-subscriber",
|
||||
"tracing-test",
|
||||
"url",
|
||||
"which 8.0.0",
|
||||
"wiremock",
|
||||
"zip 2.4.2",
|
||||
]
|
||||
|
||||
@@ -50,6 +50,9 @@ tracing = { workspace = true }
|
||||
url = { workspace = true }
|
||||
zip = { workspace = true }
|
||||
|
||||
[target.'cfg(target_os = "macos")'.dependencies]
|
||||
which = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
libc = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
|
||||
@@ -93,9 +93,17 @@ fn curated_plugins_sha_path(codex_home: &Path) -> PathBuf {
|
||||
}
|
||||
|
||||
pub fn sync_openai_plugins_repo(codex_home: &Path) -> Result<String, String> {
|
||||
#[cfg(target_os = "macos")]
|
||||
let git_binary = match which::which("git") {
|
||||
Ok(git_path) => macos_git_binary_from_path(git_path, apple_developer_tools_available()),
|
||||
Err(_) => None,
|
||||
};
|
||||
#[cfg(not(target_os = "macos"))]
|
||||
let git_binary = Some(PathBuf::from("git"));
|
||||
|
||||
sync_openai_plugins_repo_with_transport_overrides(
|
||||
codex_home,
|
||||
"git",
|
||||
git_binary.as_deref(),
|
||||
GITHUB_API_BASE_URL,
|
||||
CURATED_PLUGINS_BACKUP_ARCHIVE_API_URL,
|
||||
)
|
||||
@@ -103,13 +111,18 @@ pub fn sync_openai_plugins_repo(codex_home: &Path) -> Result<String, String> {
|
||||
|
||||
fn sync_openai_plugins_repo_with_transport_overrides(
|
||||
codex_home: &Path,
|
||||
git_binary: &str,
|
||||
git_binary: Option<&Path>,
|
||||
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_binary {
|
||||
Some(git_binary) => sync_openai_plugins_repo_via_git(codex_home, git_binary),
|
||||
None => 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");
|
||||
@@ -119,7 +132,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) {
|
||||
@@ -182,7 +194,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)?;
|
||||
@@ -229,7 +244,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,
|
||||
@@ -244,7 +259,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,
|
||||
@@ -259,7 +274,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}");
|
||||
@@ -274,7 +289,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,
|
||||
@@ -291,7 +306,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> {
|
||||
@@ -583,7 +598,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)
|
||||
@@ -594,7 +609,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")
|
||||
@@ -622,7 +637,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)
|
||||
@@ -647,7 +662,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 {
|
||||
@@ -656,6 +671,29 @@ fn git_command(git_binary: &str) -> Command {
|
||||
command
|
||||
}
|
||||
|
||||
#[cfg(any(target_os = "macos", test))]
|
||||
fn macos_git_binary_from_path(
|
||||
git_path: PathBuf,
|
||||
apple_developer_tools_available: bool,
|
||||
) -> Option<PathBuf> {
|
||||
if git_path == Path::new("/usr/bin/git") && !apple_developer_tools_available {
|
||||
None
|
||||
} else {
|
||||
Some(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,
|
||||
|
||||
@@ -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!(
|
||||
@@ -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_binary = PathBuf::from(git_binary);
|
||||
sync_openai_plugins_repo_with_transport_overrides(
|
||||
codex_home.as_path(),
|
||||
&git_binary,
|
||||
Some(git_binary.as_path()),
|
||||
&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(),
|
||||
/*git_binary*/ None,
|
||||
&api_base_url,
|
||||
&backup_archive_api_url,
|
||||
)
|
||||
@@ -343,7 +363,7 @@ exit 1
|
||||
barrier.wait();
|
||||
sync_openai_plugins_repo_with_transport_overrides(
|
||||
tmp.path(),
|
||||
git_path.to_str().expect("utf8 path"),
|
||||
Some(git_path.as_path()),
|
||||
"http://127.0.0.1:9",
|
||||
"http://127.0.0.1:9/backend-api/plugins/export/curated",
|
||||
)
|
||||
@@ -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()));
|
||||
@@ -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!(
|
||||
@@ -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");
|
||||
@@ -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_binary_from_path(
|
||||
PathBuf::from("/usr/bin/git"),
|
||||
/*apple_developer_tools_available*/ false,
|
||||
),
|
||||
None
|
||||
);
|
||||
assert_eq!(
|
||||
macos_git_binary_from_path(
|
||||
PathBuf::from("/usr/bin/git"),
|
||||
/*apple_developer_tools_available*/ true,
|
||||
),
|
||||
Some(PathBuf::from("/usr/bin/git"))
|
||||
);
|
||||
assert_eq!(
|
||||
macos_git_binary_from_path(
|
||||
PathBuf::from("/opt/homebrew/bin/git"),
|
||||
/*apple_developer_tools_available*/ false,
|
||||
),
|
||||
Some(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() {
|
||||
@@ -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()));
|
||||
@@ -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"));
|
||||
|
||||
Reference in New Issue
Block a user