From 78df92f18e697a85dae1a61e569cd0947819aee8 Mon Sep 17 00:00:00 2001 From: Adam Dalloul <47503782+Adam-Dalloul@users.noreply.github.com> Date: Thu, 10 Sep 2026 09:41:01 -0700 Subject: [PATCH] fix(acp): stop the binary-cache precheck from contradicting the launch path Agent settings could show "Version Status: pass" next to "Binary cache: warn" for the same OpenCode install, and the warn told the user to download a binary they already had. Two separate reasons, both in the read path. `check_binary_environment` only accepted a user-installed CLI in place of a codeg-managed download when the agent's registry entry carried a `dir_entry`, which is Cursor and nothing else. Every other binary agent fell through to "Binary is not installed. Download it from Agent Settings before connecting." even though `connection.rs` resolves exactly that CLI and spawns it, the connect gate in `verify_agent_installed` accepts it, and the version card counts it as installed through `detect_local_version`. OpenCode is a single-file binary agent, so a user whose only copy was `~/.local/bin/opencode.exe` got the contradiction on every settings open. The check now reads the same two facts in the same order the launcher uses, split into a pure `build_binary_cache_check` so the verdict is testable, and the passing message names the file that will be launched instead of pointing at a download button for one already present. `installed_binary_path` also deleted any cached binary whose first four bytes it could not read. `is_binary_file_compatible` returned false both for "opened it and the header belongs to another platform" and for "could not open it at all", and the caller could not tell those apart. On Windows the second case happens with nothing actually wrong: the anti-virus real-time scanner holds a freshly written ~180 MB agent binary for a moment, and a running agent holds its own image. A preflight landing in that window removed an install that had just succeeded, after which the version card fell back to the system copy and read pass while the cache check read nothing installed. The probe now returns a three-way `BinaryFormat` and only a positive wrong-platform verdict evicts. A file too short to hold any header stays evictable, since that is a permanent fact, and the post-download check stays strict because there codeg owns the file it just wrote. Reported in #631. --- src-tauri/src/acp/binary_cache.rs | 130 ++++++++++++++++++++- src-tauri/src/acp/preflight.rs | 184 +++++++++++++++++++++++------- 2 files changed, 266 insertions(+), 48 deletions(-) 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::*;