diff --git a/src-tauri/src/acp/binary_cache.rs b/src-tauri/src/acp/binary_cache.rs index 9664f9b092..a4dcd1311f 100644 --- a/src-tauri/src/acp/binary_cache.rs +++ b/src-tauri/src/acp/binary_cache.rs @@ -306,7 +306,7 @@ fn installed_binary_path(agent_id: &str, version: &str, cmd_name: &str) -> Optio if !path.exists() { return None; } - if is_binary_file_compatible(path.as_path()) { + if probe_binary_format(path.as_path()).is_usable() { return Some(path); } let _ = std::fs::remove_file(path); @@ -564,6 +564,11 @@ async fn ensure_binary_with_progress( std::fs::copy(&extracted_bin, &final_path) .map_err(|e| AcpError::DownloadFailed(format!("failed to copy binary: {e}")))?; + // Strict here on purpose, unlike `installed_binary_path`: we own the + // file we just wrote, so anything short of a readable, correct header + // means this install must fail loudly rather than cache something + // unverified. Only the READ path tolerates `Unreadable`, where a + // transient lock on an already-installed binary is expected. if !is_binary_file_compatible(&final_path) { let _ = std::fs::remove_file(&final_path); return Err(AcpError::DownloadFailed( @@ -767,16 +772,62 @@ fn set_executable_permissions(path: &Path) -> Result<(), AcpError> { } } -pub(crate) fn is_binary_file_compatible(path: &Path) -> bool { +/// Outcome of the executable-header probe. `Unreadable` is deliberately NOT +/// folded into `Incompatible`: "this file is a binary for another platform" is +/// a permanent verdict that justifies evicting it from the cache, while "this +/// file is busy right now" is transient and must not. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum BinaryFormat { + Compatible, + Incompatible, + Unreadable, +} + +impl BinaryFormat { + /// Whether a cached file may be reported as installed. Only a positive + /// wrong-platform identification disqualifies it — see + /// [`installed_binary_path`], which deletes exactly what this rejects. + pub(crate) fn is_usable(self) -> bool { + !matches!(self, BinaryFormat::Incompatible) + } +} + +/// Classify the first four bytes of a cached agent binary. +/// +/// Split from the old boolean because the boolean could not tell "wrong +/// platform" from "could not open the file", and the caller deleted on both. +/// On Windows the anti-virus real-time scanner routinely holds a freshly +/// written ~180 MB agent binary for a moment, and a running agent holds its own +/// image — treating either as a bad download turned a transient read error into +/// a lost download plus a "Binary is not installed" report immediately after an +/// install that really did succeed (#631). +pub(crate) fn probe_binary_format(path: &Path) -> BinaryFormat { let mut file = match std::fs::File::open(path) { Ok(f) => f, - Err(_) => return false, + Err(_) => return BinaryFormat::Unreadable, }; let mut header = [0_u8; 4]; - if file.read_exact(&mut header).is_err() { - return false; + match file.read_exact(&mut header) { + Ok(()) => {} + // Fewer than four bytes on disk IS a permanent verdict — no executable + // format is that small, so a truncated or empty file stays evictable. + Err(e) if e.kind() == std::io::ErrorKind::UnexpectedEof => { + return BinaryFormat::Incompatible + } + Err(_) => return BinaryFormat::Unreadable, } + if header_matches_platform(header) { + BinaryFormat::Compatible + } else { + BinaryFormat::Incompatible + } +} + +pub(crate) fn is_binary_file_compatible(path: &Path) -> bool { + probe_binary_format(path) == BinaryFormat::Compatible +} +fn header_matches_platform(header: [u8; 4]) -> bool { #[cfg(target_os = "macos")] { matches!( @@ -804,6 +855,7 @@ pub(crate) fn is_binary_file_compatible(path: &Path) -> bool { #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] { + let _ = header; true } } @@ -871,6 +923,74 @@ mod tests { assert!(err.to_string().contains("not found in archive"), "{err}"); } + // Regression for #631: a cached binary we cannot open must NOT be read as + // "wrong platform" and deleted. `installed_binary_path` deletes exactly + // what `is_usable()` rejects, so this is the eviction rule itself. + #[test] + fn only_a_wrong_platform_verdict_evicts_a_cached_binary() { + assert!(BinaryFormat::Compatible.is_usable()); + assert!( + BinaryFormat::Unreadable.is_usable(), + "a busy/locked binary must survive the probe" + ); + assert!(!BinaryFormat::Incompatible.is_usable()); + } + + // A file the OS refuses to hand us (here: a directory, which fails at + // `open` on Windows and at `read` elsewhere) is transient-looking, not a + // foreign binary. + #[test] + fn unopenable_path_probes_as_unreadable() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("busy"); + std::fs::create_dir(&dir).unwrap(); + assert_eq!(probe_binary_format(&dir), BinaryFormat::Unreadable); + assert!(!is_binary_file_compatible(&dir)); + } + + // Too short to hold any executable header: a permanent verdict, so the + // truncated-download eviction the old boolean did is preserved. + #[test] + fn truncated_file_probes_as_incompatible() { + let tmp = tempfile::tempdir().unwrap(); + let empty = tmp.path().join("empty"); + std::fs::write(&empty, b"").unwrap(); + assert_eq!(probe_binary_format(&empty), BinaryFormat::Incompatible); + + let short = tmp.path().join("short"); + std::fs::write(&short, b"MZ").unwrap(); + assert_eq!(probe_binary_format(&short), BinaryFormat::Incompatible); + } + + // A readable file with a header for another platform stays evictable. + #[test] + fn readable_header_decides_compatibility() { + let tmp = tempfile::tempdir().unwrap(); + let good = tmp.path().join("good"); + let expected: [u8; 4] = if cfg!(target_os = "windows") { + [b'M', b'Z', 0x90, 0x00] + } else if cfg!(target_os = "macos") { + [0xCF, 0xFA, 0xED, 0xFE] + } else { + [0x7F, b'E', b'L', b'F'] + }; + std::fs::write(&good, expected).unwrap(); + assert_eq!(probe_binary_format(&good), BinaryFormat::Compatible); + assert!(is_binary_file_compatible(&good)); + + let foreign = tmp.path().join("foreign"); + std::fs::write(&foreign, b"#!/bin/sh\n").unwrap(); + // A shebang is a valid file on every platform and a valid executable + // image on none of the three codeg ships for, so it stays evictable. + if cfg!(any( + target_os = "macos", + target_os = "linux", + target_os = "windows" + )) { + assert_eq!(probe_binary_format(&foreign), BinaryFormat::Incompatible); + } + } + #[test] fn version_normalization_is_consistent() { assert_eq!(normalize_version_label("v1.2.15"), "1.2.15"); diff --git a/src-tauri/src/acp/preflight.rs b/src-tauri/src/acp/preflight.rs index 04ac90630a..c3304d66fb 100644 --- a/src-tauri/src/acp/preflight.rs +++ b/src-tauri/src/acp/preflight.rs @@ -493,12 +493,53 @@ fn build_uv_version_check(current: Option<&str>, required: &str) -> CheckItem { } } -/// The registry `dir_entry` for a binary agent (None for single-file agents -/// and non-binary distributions). -fn binary_dir_entry(agent_type: AgentType) -> Option { - match registry::get_agent_meta(agent_type).distribution { - AgentDistribution::Binary { dir_entry, .. } => dir_entry, - _ => None, +/// Build the `binary_cached` check from the two facts the launch path uses, in +/// the launch path's own order: the best cached version first, then a CLI the +/// user installed themselves (PATH or `~/.local/bin`). +/// +/// Kept pure — no cache dir, no PATH — so the verdict is unit-testable and so +/// it stays literally the same decision `connection.rs` makes when it picks the +/// binary to spawn. Preflight disagreeing with that choice is what users read +/// as a codeg bug. +fn build_binary_cache_check( + cached_version: Option<&str>, + recommended: &str, + cmd: &str, + system_path: Option<&std::path::Path>, +) -> CheckItem { + let (status, message) = match (cached_version, system_path) { + (Some(cached), _) if cached == recommended => { + (CheckStatus::Pass, "Binary is cached locally".to_string()) + } + (Some(cached), _) => ( + CheckStatus::Pass, + format!("Binary {cached} is cached locally (recommended: {recommended})"), + ), + // Nothing cached, but the user has their own CLI — the connect path + // launches exactly this file, so report ready and NAME it. Saying + // "not installed" here while the version card counts that same CLI as + // installed is the pass/warn split reported in #631. + (None, Some(path)) => ( + CheckStatus::Pass, + format!( + "No codeg-managed binary; will launch the system-installed {cmd} at {} \ + (download {recommended} from Agent Settings to use the pinned build instead)", + path.display() + ), + ), + (None, None) => ( + CheckStatus::Warn, + "Binary is not installed. Download it from Agent Settings before connecting." + .to_string(), + ), + }; + + CheckItem { + check_id: "binary_cached".into(), + label: "Binary cache".into(), + status, + message, + fixes: vec![], } } @@ -544,46 +585,23 @@ async fn check_binary_environment( // canonical place to surface "upgrade available". if platform_supported { let cache_check = match binary_cache::find_best_cached_binary_for_agent(agent_type, cmd) { - Ok(Some((_, cached_version))) => { - let message = if cached_version == version { - "Binary is cached locally".to_string() + Ok(cached) => { + // `connection.rs` falls back to `resolve_system_agent_binary` + // for EVERY binary agent, not just the dir-tree ones, and so do + // the connect gate and the version card. Probe the same + // fallback whenever nothing is cached so all four agree. + let system = if cached.is_none() { + crate::commands::acp::resolve_system_agent_binary(cmd) } else { - format!("Binary {cached_version} is cached locally (recommended: {version})") + None }; - CheckItem { - check_id: "binary_cached".into(), - label: "Binary cache".into(), - status: CheckStatus::Pass, - message, - fixes: vec![], - } - } - // Dir-tree agents (Cursor): a user-installed CLI on PATH / - // ~/.local/bin is launchable as-is — the connect path falls back - // to it — so report ready instead of a misleading warn. - Ok(None) - if binary_dir_entry(agent_type).is_some() - && crate::commands::acp::resolve_system_agent_binary(cmd).is_some() => - { - CheckItem { - check_id: "binary_cached".into(), - label: "Binary cache".into(), - status: CheckStatus::Pass, - message: format!( - "Using the system-installed {cmd} (codeg-managed download also available)" - ), - fixes: vec![], - } + build_binary_cache_check( + cached.as_ref().map(|(_, v)| v.as_str()), + version, + cmd, + system.as_deref(), + ) } - Ok(None) => CheckItem { - check_id: "binary_cached".into(), - label: "Binary cache".into(), - status: CheckStatus::Warn, - message: - "Binary is not installed. Download it from Agent Settings before connecting." - .into(), - fixes: vec![], - }, Err(_) => CheckItem { check_id: "binary_cached".into(), label: "Binary cache".into(), @@ -696,6 +714,86 @@ async fn check_binary_environment( checks } +#[cfg(test)] +mod binary_cache_check_tests { + use super::*; + use std::path::Path; + + // Regression for #631. `connection.rs` spawns the best cached binary and + // otherwise falls back to the user's own CLI — for every binary agent, not + // just the dir-tree one. The version card counts that same CLI as + // installed. Preflight used to gate the fallback on `dir_entry.is_some()`, + // so a user whose only OpenCode was `~/.local/bin/opencode.exe` saw + // "Version Status: pass" next to "Binary cache: not installed" — two + // sentences about the one file codeg was about to launch. + #[test] + fn system_install_reports_ready_for_single_file_agents() { + let path = Path::new("/home/u/.local/bin/opencode"); + let check = build_binary_cache_check(None, "1.18.18", "opencode", Some(path)); + + assert!( + matches!(check.status, CheckStatus::Pass), + "system install must not warn: {:?}", + check.status + ); + // Naming the file is the point: the old text sent the user to a + // download button for a binary they already had. + assert!( + check.message.contains("/home/u/.local/bin/opencode"), + "{}", + check.message + ); + assert!(check.message.contains("1.18.18"), "{}", check.message); + } + + // The warn is still correct when there is genuinely nothing to launch — + // that is the state the connect gate rejects. + #[test] + fn nothing_cached_and_nothing_on_path_still_warns() { + let check = build_binary_cache_check(None, "1.18.18", "opencode", None); + assert!(matches!(check.status, CheckStatus::Warn), "{check:?}"); + assert!(check.message.contains("not installed"), "{}", check.message); + } + + // Launch order, not "whatever we found": a cached binary wins, so the card + // must never describe the system copy when codeg will not spawn it. + #[test] + fn cached_binary_wins_over_a_system_install() { + let check = build_binary_cache_check( + Some("1.18.18"), + "1.18.18", + "opencode", + Some(Path::new("/usr/local/bin/opencode")), + ); + assert!(matches!(check.status, CheckStatus::Pass), "{check:?}"); + assert_eq!(check.message, "Binary is cached locally"); + } + + #[test] + fn older_cached_binary_passes_and_names_the_recommended_version() { + let check = build_binary_cache_check(Some("1.18.10"), "1.18.18", "opencode", None); + assert!(matches!(check.status, CheckStatus::Pass), "{check:?}"); + assert!(check.message.contains("1.18.10"), "{}", check.message); + assert!(check.message.contains("1.18.18"), "{}", check.message); + } + + // Every branch is a `binary_cached` item, because the frontend keys the + // card (and its position) off the id. + #[test] + fn every_branch_keeps_the_binary_cached_id() { + for check in [ + build_binary_cache_check(None, "1", "c", None), + build_binary_cache_check(None, "1", "c", Some(Path::new("/x/c"))), + build_binary_cache_check(Some("1"), "1", "c", None), + build_binary_cache_check(Some("0"), "1", "c", None), + ] { + assert_eq!(check.check_id, "binary_cached"); + assert_eq!(check.label, "Binary cache"); + assert!(check.fixes.is_empty()); + } + } +} + #[cfg(test)] mod adapter_tests { use super::*;