diff --git a/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs b/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs index e8c597ea8a..d38d2c9a8e 100644 --- a/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs +++ b/codex-rs/app-server/tests/suite/v2/marketplace_upgrade.rs @@ -427,25 +427,59 @@ async fn marketplace_upgrade_named_marketplace_only() -> Result<()> { Ok(()) } +#[cfg_attr(unix, test_case(Some("git version 2.19.6"); "older Git"))] +#[cfg_attr(unix, test_case(Some("unknown"); "unknown Git version"))] +#[test_case(None; "installed Git")] #[tokio::test] -async fn marketplace_upgrade_returns_empty_roots_when_already_up_to_date() -> Result<()> { +async fn marketplace_upgrade_reuses_the_installed_revision_when_git_supports_it( + reported_version: Option<&str>, +) -> Result<()> { let codex_home = TempDir::new()?; let source = TempDir::new()?; - init_marketplace_repo(source.path(), "debug", "debug old")?; - commit_marketplace_marker(source.path(), "debug new")?; - record_git_marketplace( + let installed_revision = init_marketplace_repo(source.path(), "debug", "debug old")?; + // file:// exercises fetch negotiation even though the server is local. + let url = url::Url::from_directory_path(source.path()) + .map_err(|()| anyhow::anyhow!("invalid marketplace source path"))?; + record_user_marketplace( codex_home.path(), "debug", - source.path(), - /*ref_name*/ None, + &configured_git_marketplace_update(url.as_str(), /*ref_name*/ None), )?; disable_plugin_startup_tasks(codex_home.path())?; - let mut mcp = TestAppServer::builder() + let packets = codex_home.path().join("packets.trace"); + let builder = TestAppServer::builder() .with_codex_home(codex_home.path()) - .without_auto_env() - .build_initialized() - .await?; + .with_env_overrides(&[("GIT_TRACE_PACKET", packets.to_str())]) + .without_auto_env(); + #[cfg(unix)] + let builder = if let Some(version) = reported_version { + let path = std::env::var("PATH")?; + let bin = codex_home.path().join("bin"); + std::fs::create_dir(&bin)?; + codex_utils_cargo_bin::write_executable( + &bin.join("git"), + r#"#!/bin/sh +for arg do + if [ "$arg" = "--version" ]; then + printf '%s\n' "$TEST_GIT_VERSION" + exit 0 + fi +done +PATH=$TEST_GIT_PATH exec git "$@" +"#, + )?; + let wrapped_path = + std::env::join_paths(std::iter::once(bin).chain(std::env::split_paths(&path)))?; + builder.with_env_overrides(&[ + ("PATH", wrapped_path.to_str()), + ("TEST_GIT_PATH", Some(&path)), + ("TEST_GIT_VERSION", Some(version)), + ]) + } else { + builder + }; + let mut mcp = builder.build_initialized().await?; let first_response = send_marketplace_upgrade(&mut mcp, Some("debug")).await?; assert!(first_response.errors.is_empty()); @@ -459,6 +493,53 @@ async fn marketplace_upgrade_returns_empty_roots_when_already_up_to_date() -> Re errors: Vec::new(), } ); + + let root = expected_installed_root(codex_home.path(), "debug")?; + run_git(root.as_path(), &["checkout", "-b", "private"])?; + run_git( + root.as_path(), + &[ + "-c", + "user.name=Codex", + "-c", + "user.email=codex@example.com", + "commit", + "--allow-empty", + "-m", + "private", + ], + )?; + let private_revision = run_git(root.as_path(), &["rev-parse", "HEAD"])?; + std::fs::write(&packets, "")?; + let updated_revision = commit_marketplace_marker(source.path(), "debug new")?; + let update = send_marketplace_upgrade(&mut mcp, Some("debug")).await?; + + assert!(update.errors.is_empty(), "{:?}", update.errors); + assert_eq!( + run_git(root.as_path(), &["rev-parse", "HEAD"])?, + updated_revision + ); + let packets = std::fs::read_to_string(packets)?; + let can_reuse = reported_version.is_none() + && run_git(source.path(), &["help", "--config"]) + .is_ok_and(|keys| keys.lines().any(|key| key == "core.alternateRefsCommand")); + assert_eq!( + packets.contains(&format!("have {installed_revision}")), + can_reuse + ); + assert!(!packets.contains(&format!("have {private_revision}"))); + assert!(!root.join(".git/objects/info/alternates").exists()); + run_git(root.as_path(), &["fsck", "--full"])?; + + // Git refuses to use a shallow repository as a reference. A clean retry still upgrades it. + std::fs::write(root.join(".git/shallow"), format!("{updated_revision}\n"))?; + let next_revision = commit_marketplace_marker(source.path(), "another update")?; + let update = send_marketplace_upgrade(&mut mcp, Some("debug")).await?; + assert!(update.errors.is_empty(), "{:?}", update.errors); + assert_eq!( + run_git(root.as_path(), &["rev-parse", "HEAD"])?, + next_revision + ); Ok(()) } diff --git a/codex-rs/core-plugins/src/marketplace_upgrade.rs b/codex-rs/core-plugins/src/marketplace_upgrade.rs index bcf03d7c8a..6890433c15 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade.rs @@ -5,8 +5,9 @@ use self::activation::activate_marketplace_root; use self::activation::installed_marketplace_metadata_matches; use self::activation::read_installed_marketplace_snapshot; use self::activation::write_installed_marketplace_metadata; -use self::git::clone_git_source; +use self::git::checkout_git_source; use self::git::git_remote_revision; +use self::git::stage_git_clone; use crate::PluginGitMode; use crate::installed_marketplaces::marketplace_install_root; use crate::marketplace::has_symlinked_marketplace_manifest; @@ -14,7 +15,6 @@ use crate::marketplace::validate_marketplace_root; use crate::marketplace_add::MarketplaceSource; use crate::marketplace_policy::MarketplacePolicy; use crate::marketplace_policy::validate_marketplace_name_for_add; -use crate::marketplace_staging::MarketplaceStagingDir; use crate::marketplace_staging::cleanup_marketplace_staging; use codex_config::ConfigLayerStack; use codex_config::types::MarketplaceConfig; @@ -254,7 +254,7 @@ fn upgrade_configured_git_marketplace( mode: PluginGitMode, ) -> Result, String> { validate_plugin_segment(&marketplace.name, "marketplace name")?; - let (source, ref_name) = match normalized_source { + let (resolved_source, ref_name) = match normalized_source { Some(MarketplaceSource::Git { url, ref_name }) => (url.as_str(), ref_name.as_deref()), Some(MarketplaceSource::Local { .. }) => { return Err("validated Git marketplace source resolved to a local path".to_string()); @@ -263,7 +263,7 @@ fn upgrade_configured_git_marketplace( }; let remote_revision = git_remote_revision( codex_home, - source, + resolved_source, ref_name, MARKETPLACE_UPGRADE_GIT_TIMEOUT, mode, @@ -293,7 +293,7 @@ fn upgrade_configured_git_marketplace( .join(format!("{}.json", marketplace.name)); let inputs = CheckoutInputs { configured_name: Box::from(marketplace.name.as_str()), - source_sha256: format!("{:x}", Sha256::digest(source)).into_boxed_str(), + source_sha256: format!("{:x}", Sha256::digest(resolved_source)).into_boxed_str(), ref_name: ref_name.map(Box::from), sparse_paths: marketplace .sparse_paths @@ -313,22 +313,17 @@ fn upgrade_configured_git_marketplace( return Err(name_mismatch(&cached.found_name)); } - let staged_dir = MarketplaceStagingDir::new(install_root).map_err(|err| { - format!( - "failed to create temporary marketplace upgrade directory in {}: {err}", - install_root.display() - ) - })?; - - let activated_revision = clone_git_source( + let reference = previous_snapshot.git_reference(&destination, marketplace, resolved_source); + let staged_dir = stage_git_clone( codex_home, - source, - ref_name, + install_root, + resolved_source, &marketplace.sparse_paths, - staged_dir.path(), - MARKETPLACE_UPGRADE_GIT_TIMEOUT, + reference, mode, )?; + let activated_revision = + checkout_git_source(staged_dir.path(), ref_name, &marketplace.sparse_paths, mode)?; let marketplace_name = validate_marketplace_root(staged_dir.path()) .map_err(|err| format!("failed to validate upgraded marketplace root: {err}"))?; if marketplace_name != marketplace.name { diff --git a/codex-rs/core-plugins/src/marketplace_upgrade/activation.rs b/codex-rs/core-plugins/src/marketplace_upgrade/activation.rs index 51a8974389..d7ffe7f9f5 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade/activation.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade/activation.rs @@ -1,4 +1,5 @@ use super::ConfiguredGitMarketplace; +use super::git::CloneReference; use codex_config::types::MarketplaceSourceType; use serde::Deserialize; use serde::Serialize; @@ -72,6 +73,43 @@ pub(super) fn installed_marketplace_metadata_matches( snapshot.metadata.as_ref() == Some(&installed_marketplace_metadata(marketplace, revision)) } +impl InstalledMarketplaceSnapshot { + /// Selects a self-contained Git object store and the commit Codex installed from this source. + pub(super) fn git_reference( + &self, + root: &Path, + marketplace: &ConfiguredGitMarketplace, + resolved_source: &str, + ) -> Option> { + let metadata = self.metadata.as_ref()?; + // A plain local clone already hard-links objects from the source. + if metadata.source_type != MarketplaceSourceType::Git + || metadata.source != marketplace.source + || metadata.sparse_paths != marketplace.sparse_paths + || Path::new(resolved_source).is_dir() + { + return None; + } + let revision = metadata.revision.as_str(); + if !matches!(revision.len(), 40 | 64) + || !revision.bytes().all(|byte| byte.is_ascii_hexdigit()) + { + return None; + } + + let git_dir = root.join(".git"); + let objects = git_dir.join("objects"); + let standalone = [root, &git_dir, &objects].into_iter().all(|dir| { + dir.symlink_metadata() + .is_ok_and(|metadata| metadata.is_dir()) + }) && !objects.join("info/alternates").exists(); + standalone.then_some(CloneReference { + git_dir, + installed_revision: revision, + }) + } +} + pub(super) fn write_installed_marketplace_metadata( root: &Path, marketplace: &ConfiguredGitMarketplace, diff --git a/codex-rs/core-plugins/src/marketplace_upgrade/git.rs b/codex-rs/core-plugins/src/marketplace_upgrade/git.rs index c35c4444d9..4d7446e60d 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade/git.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade/git.rs @@ -1,4 +1,6 @@ +use super::MARKETPLACE_UPGRADE_GIT_TIMEOUT; use crate::PluginGitMode; +use crate::marketplace_staging::MarketplaceStagingDir; use std::path::Path; use std::path::PathBuf; use std::process::Command; @@ -47,79 +49,167 @@ pub(super) fn git_remote_revision( Ok(revision.to_string()) } -pub(super) fn clone_git_source( +/// Git before 2.20 silently ignores the setting that limits which alternate refs it advertises. +fn git_supports_alternate_refs_command(mode: PluginGitMode) -> bool { + let Ok(output) = run_git_command_with_timeout( + git_command(mode).arg("--version"), + "git --version", + MARKETPLACE_UPGRADE_GIT_TIMEOUT, + ) else { + return false; + }; + output.status.success() + && git_release(&output.stdout).is_some_and(|version| version >= (2, 20, 0)) +} + +/// Extracts the release number from `git --version`, ignoring the Apple or Windows build suffix. +/// An unrecognized version (such as `2.20.0.rc1`) disables reference cloning. +fn git_release(stdout: &[u8]) -> Option<(u32, u32, u32)> { + let version = std::str::from_utf8(stdout) + .ok()? + .strip_prefix("git version ")? + .split_ascii_whitespace() + .next()?; + let (major, rest) = version.split_once('.')?; + let (minor, patch) = rest.split_once('.')?; + let patch = match patch.split_once(".windows.") { + Some((patch, build)) => { + build.parse::().ok()?; + patch + } + None => patch, + }; + Some(( + major.parse().ok()?, + minor.parse().ok()?, + patch.parse().ok()?, + )) +} + +pub(super) struct CloneReference<'a> { + pub(super) git_dir: PathBuf, + pub(super) installed_revision: &'a str, +} + +/// Stages a clone for checkout. If the reference clone fails (for example, because another +/// upgrade replaced the installed object store), retries a plain clone in a fresh directory. +pub(super) fn stage_git_clone( + codex_home: &Path, + install_root: &Path, + source: &str, + sparse_paths: &[String], + reference: Option>, + mode: PluginGitMode, +) -> Result { + let run_git_clone = |reference| -> Result<_, String> { + let staged_dir = MarketplaceStagingDir::new(install_root).map_err(|err| { + format!( + "failed to create temporary marketplace upgrade directory in {}: {err}", + install_root.display() + ) + })?; + clone_git_source( + codex_home, + source, + sparse_paths, + staged_dir.path(), + reference, + mode, + )?; + Ok(staged_dir) + }; + + if let Some(reference) = reference + && git_supports_alternate_refs_command(mode) + && let Ok(staged_dir) = run_git_clone(Some(reference)) + { + return Ok(staged_dir); + } + run_git_clone(/*reference*/ None) +} + +/// Clones the marketplace source, optionally borrowing objects from the installed copy. +/// Git uses the recorded installation commit for alternate-ref negotiation and copies the +/// borrowed objects it needs before the upgrade can replace the installed directory. +fn clone_git_source( codex_home: &Path, source: &str, - ref_name: Option<&str>, sparse_paths: &[String], destination: &Path, - timeout: Duration, + reference: Option>, mode: PluginGitMode, -) -> Result { - let git_destination = git_path_arg(destination); +) -> Result<(), String> { + let timeout = MARKETPLACE_UPGRADE_GIT_TIMEOUT; + let destination = git_path_arg(destination); let mut command = git_command(mode); let _trusted_repository = matches!(mode, PluginGitMode::Automatic) .then(|| crate::configure_trusted_git_repository(&mut command, codex_home)) .transpose()?; - if sparse_paths.is_empty() { - let output = run_git_command_with_timeout( - command.arg("clone").arg(source).arg(&git_destination), - "git clone marketplace source", - timeout, - )?; - ensure_git_success(&output, "git clone marketplace source")?; - if let Some(ref_name) = ref_name { - let output = run_git_command_with_timeout( - git_command(mode) - .arg("-C") - .arg(&git_destination) - .arg("checkout") - .arg(ref_name), - "git checkout marketplace ref", - timeout, - )?; - ensure_git_success(&output, "git checkout marketplace ref")?; - } - return git_worktree_revision(&git_destination, timeout, mode); - } - - let output = run_git_command_with_timeout( + if let Some(reference) = reference { + // Git normally enumerates all refs in the alternate, including private local branches. + // Seed negotiation from the installed commit. The shell builtin `:` consumes the + // repository path that Git appends to the command. command - .arg("clone") - .arg("--filter=blob:none") - .arg("--no-checkout") - .arg(source) - .arg(&git_destination), + .env("CODEX_MARKETPLACE_HAVE", reference.installed_revision) + .args([ + "-c", + r#"core.alternateRefsCommand=echo "$CODEX_MARKETPLACE_HAVE" && :"#, + "clone", + "--reference", + ]) + .arg(git_path_arg(&reference.git_dir)) + .arg("--dissociate"); + } else { + command.arg("clone"); + } + if !sparse_paths.is_empty() { + command.args(["--filter=blob:none", "--no-checkout"]); + } + let output = run_git_command_with_timeout( + command.arg("--").arg(source).arg(&destination), "git clone marketplace source", timeout, )?; - ensure_git_success(&output, "git clone marketplace source")?; + ensure_git_success(&output, "git clone marketplace source") +} - let mut sparse_checkout = git_command(mode); - sparse_checkout - .arg("-C") - .arg(&git_destination) - .arg("sparse-checkout") - .arg("set") - .args(sparse_paths); - let output = run_git_command_with_timeout( - &mut sparse_checkout, - "git sparse-checkout marketplace source", - timeout, - )?; - ensure_git_success(&output, "git sparse-checkout marketplace source")?; - - let output = run_git_command_with_timeout( - git_command(mode) - .arg("-C") - .arg(&git_destination) - .arg("checkout") - .arg(ref_name.unwrap_or("HEAD")), - "git checkout marketplace ref", - timeout, - )?; - ensure_git_success(&output, "git checkout marketplace ref")?; - git_worktree_revision(&git_destination, timeout, mode) +/// Checks out the requested ref and returns the revision that will be validated for activation. +pub(super) fn checkout_git_source( + destination: &Path, + ref_name: Option<&str>, + sparse_paths: &[String], + mode: PluginGitMode, +) -> Result { + let destination = git_path_arg(destination); + let timeout = MARKETPLACE_UPGRADE_GIT_TIMEOUT; + let checkout_ref = if sparse_paths.is_empty() { + ref_name + } else { + let output = run_git_command_with_timeout( + git_command(mode) + .arg("-C") + .arg(&destination) + .args(["sparse-checkout", "set"]) + .args(sparse_paths), + "git sparse-checkout marketplace source", + timeout, + )?; + ensure_git_success(&output, "git sparse-checkout marketplace source")?; + Some(ref_name.unwrap_or("HEAD")) + }; + if let Some(checkout_ref) = checkout_ref { + let output = run_git_command_with_timeout( + git_command(mode) + .arg("-C") + .arg(&destination) + .arg("checkout") + .arg(checkout_ref), + "git checkout marketplace ref", + timeout, + )?; + ensure_git_success(&output, "git checkout marketplace ref")?; + } + git_worktree_revision(&destination, timeout, mode) } fn git_worktree_revision( @@ -241,6 +331,7 @@ fn ensure_git_success(output: &Output, context: &str) -> Result<(), String> { #[cfg(test)] mod tests { use super::git_command; + use super::git_release; use super::is_full_git_sha; use super::strip_windows_verbatim_path_prefix; use pretty_assertions::assert_eq; @@ -253,6 +344,21 @@ mod tests { assert!(!is_full_git_sha("0123456")); } + #[test] + fn recognizes_released_git_versions() { + for (output, release) in [ + ("git version 2.19.6\n", Some((2, 19, 6))), + ("git version 2.20.0\n", Some((2, 20, 0))), + ("git version 2.48.1.windows.1\r\n", Some((2, 48, 1))), + ("git version 2.39.5 (Apple Git-154)\n", Some((2, 39, 5))), + ("git version 2.20.0.rc1\n", None), + ("git version 2.20\n", None), + ("git version unknown\n", None), + ] { + assert_eq!(git_release(output.as_bytes()), release, "{output}"); + } + } + #[test] fn git_command_uses_path_lookup_with_stable_noninteractive_env() { let command = git_command(crate::PluginGitMode::Automatic); diff --git a/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs index 0ff6c6a220..70ce46bf48 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::PluginGitMode; +use crate::marketplace_staging::MarketplaceStagingDir; use codex_config::CONFIG_TOML_FILE; use codex_config::LoaderOverrides; use codex_config::loader::load_config_layers_state; @@ -232,8 +233,7 @@ fn upgrade_uses_validated_source_for_git_operations() { let normalized_url = url::Url::from_directory_path(remote_repo.path()) .expect("remote repository URL") .to_string(); - let raw_source = codex_home.path().join("missing-raw-source"); - let raw_source = raw_source.to_string_lossy().into_owned(); + let raw_source = "example/good"; let config = format!( r#" [marketplaces.good] @@ -245,12 +245,12 @@ ref = "missing-ref" std::fs::write(codex_home.path().join(CONFIG_TOML_FILE), config).expect("write config"); let marketplace = ConfiguredGitMarketplace { name: "good".to_string(), - source: raw_source, + source: raw_source.to_string(), ref_name: Some("missing-ref".to_string()), sparse_paths: Vec::new(), }; let normalized_source = MarketplaceSource::Git { - url: normalized_url, + url: normalized_url.clone(), ref_name: Some("HEAD".to_string()), }; let install_root = marketplace_install_root(codex_home.path()); @@ -270,6 +270,12 @@ ref = "missing-ref" upgraded_root, AbsolutePathBuf::try_from(install_root.join("good")).expect("installed marketplace root") ); + let snapshot = read_installed_marketplace_snapshot(upgraded_root.as_path(), &marketplace.name); + assert!( + snapshot + .git_reference(upgraded_root.as_path(), &marketplace, &normalized_url) + .is_some() + ); } #[test]