diff --git a/crates/cli/src/update.rs b/crates/cli/src/update.rs index 4583b30fb..7a5507f9c 100644 --- a/crates/cli/src/update.rs +++ b/crates/cli/src/update.rs @@ -7,6 +7,10 @@ use std::cmp::Ordering; use std::collections::HashMap; +#[cfg(target_os = "android")] +use std::ffi::CStr; +#[cfg(any(target_os = "android", all(test, unix)))] +use std::ffi::OsStr; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; @@ -24,13 +28,15 @@ const GITHUB_RELEASE_DOWNLOAD_BASE_URL: &str = "https://github.com/Hmbown/CodeWhale/releases/download"; const UPDATE_HTTP_ATTEMPTS: usize = 3; const UPDATE_HTTP_RETRY_DELAY_MS: u64 = 100; +#[cfg(target_os = "android")] +const ANDROID_PROC_SELF_MAPS: &str = "/proc/self/maps"; /// Run the self-update workflow. /// /// OpenHarmony (HarmonyOS) won't compile this file, so no need to handle pub fn run_update(beta: bool, check_only: bool, proxy_arg: Option) -> Result<()> { - let current_exe = - std::env::current_exe().context("failed to determine current executable path")?; + let executable_identity = update_executable_identity()?; + let current_exe = executable_identity.path.clone(); let legacy_binary = is_legacy_binary(¤t_exe); ensure_supported_release_target(std::env::consts::OS, std::env::consts::ARCH)?; @@ -157,10 +163,12 @@ pub fn run_update(beta: bool, check_only: bool, proxy_arg: Option) -> Re println!("SHA256 checksum verified."); } - // Step 4: Replace binaries atomically after all downloads verify. - for (path, _, bytes) in downloads.iter().rev() { - replace_binary(path, bytes)?; - } + // Step 4: Replace binaries only after all downloads and the primary + // executable identity verify. The preflight happens before a colocated + // sibling can change, then the primary is checked again just in time. + replace_verified_downloads(&downloads, || { + validate_primary_update_identity(&executable_identity) + })?; println!( "\n✅ Successfully updated to {latest_tag}!\n\ @@ -177,6 +185,378 @@ pub fn run_update(beta: bool, check_only: bool, proxy_arg: Option) -> Re Ok(()) } +/// Resolve the executable that the updater is allowed to replace. +/// +/// Android's `std::env::current_exe()`, `AT_EXECFN`, and `/proc/self/exe` can +/// all identify Bionic's runtime linker rather than the launched program. On +/// Android, locate a marker compiled into this executable with `dladdr`, then +/// require the executable `/proc/self/maps` row containing that same address +/// to agree by canonical path, device, and inode. +#[derive(Debug, Clone)] +struct UpdateExecutableIdentity { + path: PathBuf, + #[cfg(target_os = "android")] + android_proof: AndroidExecutableProof, +} + +#[cfg(not(target_os = "android"))] +fn update_executable_identity() -> Result { + let path = std::env::current_exe().context("failed to determine current executable path")?; + Ok(UpdateExecutableIdentity { path }) +} + +#[cfg(target_os = "android")] +fn update_executable_identity() -> Result { + let android_proof = android_loaded_executable_proof()?; + Ok(UpdateExecutableIdentity { + path: android_proof.path.clone(), + android_proof, + }) +} + +#[cfg(target_os = "android")] +#[inline(never)] +extern "C" fn android_update_image_marker() -> usize { + android_update_image_marker as *const () as usize +} + +#[cfg(target_os = "android")] +fn android_loaded_executable_proof() -> Result { + let marker = android_update_image_marker as *const () as usize as u64; + let dladdr_path = android_dladdr_path(android_update_image_marker as *const libc::c_void)?; + let maps = std::fs::read_to_string(ANDROID_PROC_SELF_MAPS) + .context("failed to read Android executable mappings from /proc/self/maps")?; + android_loaded_executable_proof_report(&maps, marker, &dladdr_path) +} + +#[cfg(target_os = "android")] +fn android_dladdr_path(marker: *const libc::c_void) -> Result { + use std::os::unix::ffi::OsStrExt; + + let mut info = std::mem::MaybeUninit::::zeroed(); + // SAFETY: `marker` points to a function in this loaded image and `info` + // points to writable storage for the duration of the call. + let found = unsafe { libc::dladdr(marker, info.as_mut_ptr()) }; + if found == 0 { + bail!("Android dladdr could not locate the updater's loaded image"); + } + // SAFETY: A non-zero dladdr result initializes `info`. + let info = unsafe { info.assume_init() }; + if info.dli_fname.is_null() { + bail!("Android dladdr returned an empty loaded-image path"); + } + // SAFETY: `dli_fname` is a NUL-terminated string owned by the dynamic + // loader and remains valid while this image is loaded. + let bytes = unsafe { CStr::from_ptr(info.dli_fname) }.to_bytes(); + if bytes.is_empty() { + bail!("Android dladdr returned an empty loaded-image path"); + } + Ok(PathBuf::from(OsStr::from_bytes(bytes))) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +#[derive(Debug, Clone, PartialEq, Eq)] +struct AndroidImageMapping { + start: u64, + end: u64, + device_major: u32, + device_minor: u32, + inode: u64, + path: PathBuf, +} + +#[cfg(any(target_os = "android", all(test, unix)))] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AndroidExecutableProofKind { + DladdrAndProcMaps, +} + +#[cfg(any(target_os = "android", all(test, unix)))] +#[derive(Debug, Clone, PartialEq, Eq)] +struct AndroidExecutableProof { + path: PathBuf, + device_major: u32, + device_minor: u32, + inode: u64, + proof_kind: AndroidExecutableProofKind, +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn parse_android_image_mapping(maps: &str, marker: u64) -> Result { + let mut matching = None; + for (line_index, line) in maps.lines().enumerate() { + if line.trim().is_empty() { + continue; + } + let mut fields = line.split_whitespace(); + let range = fields + .next() + .with_context(|| format!("malformed /proc/self/maps line {}", line_index + 1))?; + let (start, end) = range + .split_once('-') + .with_context(|| format!("malformed mapping range `{range}`"))?; + let start = u64::from_str_radix(start, 16) + .with_context(|| format!("invalid mapping start `{start}`"))?; + let end = + u64::from_str_radix(end, 16).with_context(|| format!("invalid mapping end `{end}`"))?; + if !(start <= marker && marker < end) { + continue; + } + + let permissions = fields + .next() + .context("loaded-image mapping is missing permissions")?; + let _offset = fields + .next() + .context("loaded-image mapping is missing its file offset")?; + let device = fields + .next() + .context("loaded-image mapping is missing its device")?; + let inode = fields + .next() + .context("loaded-image mapping is missing its inode")? + .parse::() + .context("loaded-image mapping has an invalid inode")?; + let path = fields.collect::>().join(" "); + + if permissions.as_bytes().get(2) != Some(&b'x') { + bail!("loaded-image mapping for updater marker is not executable"); + } + if inode == 0 { + bail!("loaded-image mapping for updater marker has no file inode"); + } + let (device_major, device_minor) = device + .split_once(':') + .context("loaded-image mapping has an invalid device")?; + let device_major = u32::from_str_radix(device_major, 16) + .context("loaded-image mapping has an invalid device major number")?; + let device_minor = u32::from_str_radix(device_minor, 16) + .context("loaded-image mapping has an invalid device minor number")?; + if path.is_empty() { + bail!("loaded-image mapping for updater marker has no pathname"); + } + + let mapping = AndroidImageMapping { + start, + end, + device_major, + device_minor, + inode, + path: PathBuf::from(path), + }; + if matching.replace(mapping).is_some() { + bail!("multiple /proc/self/maps rows contain the updater marker"); + } + } + + matching.ok_or_else(|| anyhow!("no /proc/self/maps row contains the updater marker")) +} + +#[cfg(all(test, unix))] +fn resolve_android_loaded_executable_report( + maps: &str, + marker: u64, + dladdr_path: &Path, +) -> Result { + Ok(android_loaded_executable_proof_report(maps, marker, dladdr_path)?.path) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn android_loaded_executable_proof_report( + maps: &str, + marker: u64, + dladdr_path: &Path, +) -> Result { + let mapping = parse_android_image_mapping(maps, marker)?; + validate_android_reported_path("dladdr", dladdr_path)?; + validate_android_reported_path("/proc/self/maps", &mapping.path)?; + + let resolved_dladdr = dladdr_path.canonicalize().with_context(|| { + format!( + "failed to canonicalize Android dladdr path {}", + dladdr_path.display() + ) + })?; + let resolved_mapping = mapping.path.canonicalize().with_context(|| { + format!( + "failed to canonicalize Android loaded-image mapping {}", + mapping.path.display() + ) + })?; + if resolved_dladdr != resolved_mapping { + bail!( + "Android loaded-image authorities disagree: dladdr resolved to {}, but /proc/self/maps resolved to {}", + resolved_dladdr.display(), + resolved_mapping.display() + ); + } + if is_android_linker_name(&resolved_mapping) { + bail!( + "Android loaded-image authorities resolved to runtime linker {}; refusing to use the linker as an update target", + resolved_mapping.display() + ); + } + if !is_executable_file(&resolved_mapping) { + bail!( + "Android loaded image `{}` is not an executable regular file; refusing to select an update target", + resolved_mapping.display() + ); + } + + validate_android_mapping_identity(&mapping, &resolved_mapping)?; + Ok(AndroidExecutableProof { + path: resolved_mapping, + device_major: mapping.device_major, + device_minor: mapping.device_minor, + inode: mapping.inode, + proof_kind: AndroidExecutableProofKind::DladdrAndProcMaps, + }) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn validate_android_reported_path(authority: &str, path: &Path) -> Result<()> { + if !path.is_absolute() { + bail!( + "Android {authority} reported non-absolute loaded-image path `{}`", + path.display() + ); + } + if path.to_string_lossy().ends_with(" (deleted)") { + bail!( + "Android {authority} reported deleted loaded image `{}`", + path.display() + ); + } + if is_android_linker_name(path) { + bail!( + "Android {authority} identifies runtime linker `{}`; refusing to use the linker as an update target", + path.display() + ); + } + Ok(()) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn validate_android_mapping_identity( + mapping: &AndroidImageMapping, + candidate: &Path, +) -> Result<()> { + use std::os::unix::fs::MetadataExt; + + let candidate_metadata = std::fs::metadata(candidate).with_context(|| { + format!( + "failed to stat Android update target {}", + candidate.display() + ) + })?; + let (candidate_major, candidate_minor) = android_device_parts(candidate_metadata.dev()); + let identity_matches = mapping.device_major == candidate_major + && mapping.device_minor == candidate_minor + && mapping.inode == candidate_metadata.ino(); + if !identity_matches { + bail!( + "Android loaded-image identity changed: /proc/self/maps has device/inode {:x}:{:x}:{}, but update target {} is {:x}:{:x}:{}; refusing to replace it", + mapping.device_major, + mapping.device_minor, + mapping.inode, + candidate.display(), + candidate_major, + candidate_minor, + candidate_metadata.ino() + ); + } + Ok(()) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn android_device_parts(device: u64) -> (u32, u32) { + // Linux/Bionic's dev_t encoding, matching makedev(3), major(3), and + // minor(3). `/proc/self/maps` renders these components in hexadecimal. + let major = ((device >> 8) & 0xfff) as u32; + let minor = ((device & 0xff) | ((device >> 12) & 0xfff00)) as u32; + (major, minor) +} + +fn validate_primary_update_identity(identity: &UpdateExecutableIdentity) -> Result<()> { + #[cfg(target_os = "android")] + { + let fresh = android_loaded_executable_proof()?; + if fresh != identity.android_proof { + bail!( + "Android loaded-image proof changed from {:?} to {:?}; refusing to replace the update target", + identity.android_proof, + fresh + ); + } + return Ok(()); + } + + #[cfg(not(target_os = "android"))] + { + let _ = identity; + Ok(()) + } +} + +fn replace_verified_downloads( + downloads: &[(PathBuf, String, Vec)], + validate_primary_identity: F, +) -> Result<()> +where + F: Fn() -> Result<()>, +{ + // Fail before mutating a sibling if the primary pathname no longer names + // the process image that initiated this update. + validate_primary_identity()?; + for (path, _, bytes) in downloads.iter().rev() { + replace_binary_with_validation(path, bytes, || { + // Re-check after each temp file is fully staged and immediately + // before every destructive rename. This protects paired installs + // before the sibling as well as just in time for the primary. + validate_primary_identity() + })?; + } + Ok(()) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn is_android_linker_name(path: &Path) -> bool { + path.file_name() + .and_then(OsStr::to_str) + .is_some_and(|name| { + matches!( + name, + "linker" + | "linker64" + | "linker_asan" + | "linker_asan64" + | "linker_hwasan" + | "linker_hwasan64" + ) + }) +} + +#[cfg(any(target_os = "android", all(test, unix)))] +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = std::fs::metadata(path) else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + #[derive(Debug, Clone, PartialEq, Eq)] struct FetchedRelease { release: Release, @@ -951,7 +1331,19 @@ bypass this preflight at your own risk.", /// installs it in place. Unix can atomically replace the executable path. On /// Windows, replacing a running executable can fail, so rename the current file /// out of the way before moving the new binary into the original path. +#[cfg(test)] fn replace_binary(target: &Path, new_bytes: &[u8]) -> Result<()> { + replace_binary_with_validation(target, new_bytes, || Ok(())) +} + +fn replace_binary_with_validation( + target: &Path, + new_bytes: &[u8], + validate_before_replace: F, +) -> Result<()> +where + F: FnOnce() -> Result<()>, +{ let parent = target .parent() .filter(|path| !path.as_os_str().is_empty()) @@ -977,6 +1369,8 @@ fn replace_binary(target: &Path, new_bytes: &[u8]) -> Result<()> { } } + validate_before_replace()?; + #[cfg(windows)] { let backup = backup_path_for(target); @@ -1040,6 +1434,13 @@ mod tests { use std::sync::mpsc; use std::thread; + #[cfg(unix)] + fn write_test_executable(path: &Path) { + std::fs::write(path, b"test executable").unwrap(); + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + /// Verify the arch mapping used when constructing asset names. /// The mapping must use release-asset naming (arm64/x64), not Rust /// stdlib constants (aarch64/x86_64). @@ -1070,6 +1471,459 @@ mod tests { ensure_supported_release_target("macos", "aarch64").unwrap(); } + #[cfg(unix)] + const TEST_ANDROID_MARKER: u64 = 0x1800; + + #[cfg(unix)] + fn test_android_mapping_line(path: &Path, permissions: &str) -> String { + use std::os::unix::fs::MetadataExt; + + let metadata = std::fs::metadata(path).unwrap(); + let (device_major, device_minor) = android_device_parts(metadata.dev()); + format!( + "1000-2000 {permissions} 00000000 {:x}:{:x} {} {}\n", + device_major, + device_minor, + metadata.ino(), + path.display() + ) + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_resolves_agreed_mapping() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let maps = test_android_mapping_line(&executable, "r-xp"); + + let resolved = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &executable) + .unwrap(); + + assert_eq!(resolved, executable.canonicalize().unwrap()); + assert_eq!(update_targets_for_exe(&resolved)[0].path, resolved); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_canonicalizes_symlink_and_sibling_policy() { + use std::os::unix::fs::symlink; + + let dir = tempfile::TempDir::new().unwrap(); + let canonical_dir = dir.path().join("canonical"); + let install_dir = dir.path().join("install"); + std::fs::create_dir(&canonical_dir).unwrap(); + std::fs::create_dir(&install_dir).unwrap(); + let canonical_dispatcher = canonical_dir.join("codewhale"); + let canonical_tui = canonical_dir.join("codewhale-tui"); + let invoked = install_dir.join("codewhale"); + write_test_executable(&canonical_dispatcher); + write_test_executable(&canonical_tui); + symlink(&canonical_dispatcher, &invoked).unwrap(); + let maps = test_android_mapping_line(&invoked, "r-xp"); + + let resolved = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &invoked).unwrap(); + let target_paths = update_targets_for_exe(&resolved) + .into_iter() + .map(|target| target.path) + .collect::>(); + + assert_eq!( + target_paths, + vec![ + canonical_dispatcher.canonicalize().unwrap(), + canonical_tui.canonicalize().unwrap() + ] + ); + assert!(!target_paths.contains(&invoked)); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_requires_marker_mapping() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let maps = test_android_mapping_line(&executable, "r-xp"); + + let error = resolve_android_loaded_executable_report(&maps, 0x3000, &executable) + .expect_err("a marker outside every mapping must fail closed"); + + assert!( + error.to_string().contains("no /proc/self/maps row"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_requires_executable_mapping() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let maps = test_android_mapping_line(&executable, "rw-p"); + + let error = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &executable) + .expect_err("a non-executable marker mapping must fail closed"); + + assert!( + error + .to_string() + .contains("mapping for updater marker is not executable"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_anonymous_mapping() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let maps = "1000-2000 r-xp 00000000 00:00 0\n"; + + let error = + resolve_android_loaded_executable_report(maps, TEST_ANDROID_MARKER, &executable) + .expect_err("an anonymous marker mapping must fail closed"); + + assert!( + error.to_string().contains("has no file inode"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_relative_or_deleted_paths() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let metadata = std::fs::metadata(&executable).unwrap(); + use std::os::unix::fs::MetadataExt; + let (device_major, device_minor) = android_device_parts(metadata.dev()); + let relative_maps = format!( + "1000-2000 r-xp 00000000 {:x}:{:x} {} codewhale\n", + device_major, + device_minor, + metadata.ino() + ); + let deleted = PathBuf::from(format!("{} (deleted)", executable.display())); + + let relative_error = resolve_android_loaded_executable_report( + &relative_maps, + TEST_ANDROID_MARKER, + &executable, + ) + .expect_err("a relative maps pathname must fail closed"); + let deleted_error = resolve_android_loaded_executable_report( + &test_android_mapping_line(&executable, "r-xp"), + TEST_ANDROID_MARKER, + &deleted, + ) + .expect_err("a deleted dladdr pathname must fail closed"); + + assert!(relative_error.to_string().contains("non-absolute")); + assert!(deleted_error.to_string().contains("deleted loaded image")); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_linker_and_symlink_to_linker() { + use std::os::unix::fs::symlink; + + let dir = tempfile::TempDir::new().unwrap(); + let runtime_linker = dir.path().join("linker64"); + let invoked = dir.path().join("codewhale"); + write_test_executable(&runtime_linker); + symlink(&runtime_linker, &invoked).unwrap(); + let maps = test_android_mapping_line(&invoked, "r-xp"); + + let direct_error = resolve_android_loaded_executable_report( + &maps, + TEST_ANDROID_MARKER, + Path::new("/system/bin/linker64"), + ) + .expect_err("a directly reported Bionic linker must fail closed"); + let symlink_error = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &invoked) + .expect_err("a symlink to a linker must fail closed"); + + assert!( + direct_error + .to_string() + .contains("identifies runtime linker") + ); + assert!( + symlink_error + .to_string() + .contains("resolved to runtime linker") + ); + } + + #[cfg(unix)] + #[test] + fn android_linker_name_recognizes_bionic_loader_variants() { + for name in [ + "linker", + "linker64", + "linker_asan", + "linker_asan64", + "linker_hwasan", + "linker_hwasan64", + ] { + assert!( + is_android_linker_name( + Path::new("/apex/com.android.runtime/bin") + .join(name) + .as_path() + ), + "{name} must never become an updater target" + ); + } + assert!(!is_android_linker_name(Path::new("codewhale"))); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_authority_disagreement() { + let dir = tempfile::TempDir::new().unwrap(); + let mapped = dir.path().join("mapped-codewhale"); + let dladdr = dir.path().join("dladdr-codewhale"); + write_test_executable(&mapped); + write_test_executable(&dladdr); + let maps = test_android_mapping_line(&mapped, "r-xp"); + + let error = resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &dladdr) + .expect_err("dladdr and maps path disagreement must fail closed"); + + assert!( + error.to_string().contains("authorities disagree"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_non_executable_file() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + std::fs::write(&executable, b"not executable").unwrap(); + let maps = test_android_mapping_line(&executable, "r-xp"); + + let error = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &executable) + .expect_err("a non-executable target file must fail closed"); + + assert!( + error.to_string().contains("not an executable regular file"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_rejects_device_inode_mismatch() { + let dir = tempfile::TempDir::new().unwrap(); + let executable = dir.path().join("codewhale"); + write_test_executable(&executable); + let metadata = std::fs::metadata(&executable).unwrap(); + use std::os::unix::fs::MetadataExt; + let (device_major, device_minor) = android_device_parts(metadata.dev()); + let maps = format!( + "1000-2000 r-xp 00000000 {:x}:{:x} {} {}\n", + device_major, + device_minor, + metadata.ino() + 1, + executable.display() + ); + + let error = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &executable) + .expect_err("a different maps device/inode must fail closed"); + + assert!( + error.to_string().contains("loaded-image identity changed"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_loaded_image_recheck_detects_pre_replace_swap() { + let dir = tempfile::TempDir::new().unwrap(); + let candidate = dir.path().join("codewhale"); + let replacement = dir.path().join("replacement"); + write_test_executable(&candidate); + let maps = test_android_mapping_line(&candidate, "r-xp"); + + write_test_executable(&replacement); + std::fs::rename(&replacement, &candidate).unwrap(); + let error = + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &candidate) + .expect_err("a path swap after download must fail before replacement"); + + assert!( + error.to_string().contains("loaded-image identity changed"), + "unexpected error: {error:#}" + ); + } + + #[cfg(unix)] + #[test] + fn android_identity_preflight_prevents_all_paired_replacements() { + let dir = tempfile::TempDir::new().unwrap(); + let primary = dir.path().join("codewhale"); + let sibling = dir.path().join("codewhale-tui"); + let swapped_primary = dir.path().join("swapped-primary"); + + write_test_executable(&primary); + std::fs::write(&primary, b"original running primary").unwrap(); + let maps = test_android_mapping_line(&primary, "r-xp"); + write_test_executable(&sibling); + std::fs::write(&sibling, b"original sibling").unwrap(); + write_test_executable(&swapped_primary); + std::fs::write(&swapped_primary, b"externally swapped primary").unwrap(); + std::fs::rename(&swapped_primary, &primary).unwrap(); + + let downloads = vec![ + ( + primary.clone(), + "codewhale-android-arm64".to_string(), + b"downloaded primary".to_vec(), + ), + ( + sibling.clone(), + "codewhale-tui-android-arm64".to_string(), + b"downloaded sibling".to_vec(), + ), + ]; + let error = replace_verified_downloads(&downloads, || { + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &primary) + .map(|_| ()) + }) + .expect_err("identity mismatch must fail before either binary changes"); + + assert!( + error.to_string().contains("loaded-image identity changed"), + "unexpected error: {error:#}" + ); + assert_eq!( + std::fs::read(&primary).unwrap(), + b"externally swapped primary" + ); + assert_eq!(std::fs::read(&sibling).unwrap(), b"original sibling"); + } + + #[cfg(unix)] + #[test] + fn android_identity_recheck_before_sibling_prevents_pair_split() { + use std::cell::Cell; + + let dir = tempfile::TempDir::new().unwrap(); + let primary = dir.path().join("codewhale"); + let sibling = dir.path().join("codewhale-tui"); + let swapped_primary = dir.path().join("swapped-primary"); + write_test_executable(&primary); + std::fs::write(&primary, b"original running primary").unwrap(); + let maps = test_android_mapping_line(&primary, "r-xp"); + write_test_executable(&sibling); + std::fs::write(&sibling, b"original sibling").unwrap(); + write_test_executable(&swapped_primary); + std::fs::write(&swapped_primary, b"externally swapped primary").unwrap(); + + let downloads = vec![ + ( + primary.clone(), + "codewhale-android-arm64".to_string(), + b"downloaded primary".to_vec(), + ), + ( + sibling.clone(), + "codewhale-tui-android-arm64".to_string(), + b"downloaded sibling".to_vec(), + ), + ]; + let validation_calls = Cell::new(0); + let error = replace_verified_downloads(&downloads, || { + let call = validation_calls.get() + 1; + validation_calls.set(call); + if call == 1 { + return Ok(()); + } + std::fs::rename(&swapped_primary, &primary).unwrap(); + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &primary) + .map(|_| ()) + }) + .expect_err("identity mismatch must fail before the staged sibling persists"); + + assert_eq!(validation_calls.get(), 2); + assert!( + error.to_string().contains("loaded-image identity changed"), + "unexpected error: {error:#}" + ); + assert_eq!( + std::fs::read(&primary).unwrap(), + b"externally swapped primary" + ); + assert_eq!(std::fs::read(&sibling).unwrap(), b"original sibling"); + } + + #[cfg(unix)] + #[test] + fn android_identity_jit_recheck_runs_after_staging_before_persist() { + use std::cell::Cell; + + let dir = tempfile::TempDir::new().unwrap(); + let primary = dir.path().join("codewhale"); + let swapped_primary = dir.path().join("swapped-primary"); + write_test_executable(&primary); + std::fs::write(&primary, b"original running primary").unwrap(); + let maps = test_android_mapping_line(&primary, "r-xp"); + write_test_executable(&swapped_primary); + std::fs::write(&swapped_primary, b"externally swapped primary").unwrap(); + + let downloads = vec![( + primary.clone(), + "codewhale-android-arm64".to_string(), + b"downloaded primary".to_vec(), + )]; + let validation_calls = Cell::new(0); + let error = replace_verified_downloads(&downloads, || { + let call = validation_calls.get() + 1; + validation_calls.set(call); + if call == 1 { + return Ok(()); + } + std::fs::rename(&swapped_primary, &primary).unwrap(); + resolve_android_loaded_executable_report(&maps, TEST_ANDROID_MARKER, &primary) + .map(|_| ()) + }) + .expect_err("the post-staging identity swap must fail before persist"); + + assert_eq!(validation_calls.get(), 2); + assert!( + error.to_string().contains("loaded-image identity changed"), + "unexpected error: {error:#}" + ); + assert_eq!( + std::fs::read(&primary).unwrap(), + b"externally swapped primary" + ); + assert!( + std::fs::read_dir(dir.path()).unwrap().all(|entry| { + !entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".codewhale-update-") + }), + "failed validation must clean the staged temp file" + ); + } + /// Verify binary prefix detection for dispatcher vs TUI binary. #[test] fn test_binary_prefix_detection() { diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index cb8f26c4e..64520cd9b 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -228,6 +228,28 @@ impl StructuredState { } } +fn user_shell_turn_outcome( + result: &Result, + cancel_requested: bool, +) -> TurnOutcomeStatus { + let tool_reported_cancel = result.as_ref().is_ok_and(|tool_result| { + tool_result + .metadata + .as_ref() + .and_then(|metadata| metadata.get("canceled")) + .and_then(Value::as_bool) + .unwrap_or(false) + }); + + if cancel_requested || tool_reported_cancel { + TurnOutcomeStatus::Interrupted + } else if result.as_ref().is_ok_and(|tool_result| tool_result.success) { + TurnOutcomeStatus::Completed + } else { + TurnOutcomeStatus::Failed + } +} + fn append_plan_field(out: &mut String, label: &str, value: Option<&str>) { if let Some(value) = value.map(str::trim).filter(|value| !value.is_empty()) { out.push_str(&format!("- {label}: {value}\n")); @@ -1339,11 +1361,7 @@ impl Engine { })); } - let status = if result.is_err() { - TurnOutcomeStatus::Failed - } else { - TurnOutcomeStatus::Completed - }; + let status = user_shell_turn_outcome(&result, self.cancel_token.is_cancelled()); let error = result.as_ref().err().map(ToString::to_string); let _ = self diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 0630cfeb6..03de2cf7e 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -2643,6 +2643,50 @@ fn model_catalog_exposes_work_update_as_sole_progress_surface() { } } +#[test] +fn user_shell_turn_outcome_distinguishes_cancel_failure_and_success() { + let cancelled = Ok( + ToolResult::error("Command canceled; process killed.").with_metadata(json!({ + "status": "Killed", + "canceled": true, + })), + ); + assert_eq!( + user_shell_turn_outcome(&cancelled, false), + TurnOutcomeStatus::Interrupted + ); + + let cancelled_while_awaiting_approval = Err(ToolError::execution_failed( + "Request cancelled while awaiting approval", + )); + assert_eq!( + user_shell_turn_outcome(&cancelled_while_awaiting_approval, true), + TurnOutcomeStatus::Interrupted + ); + + let failed = Ok(ToolResult::error("Command failed (exit code: 1)")); + assert_eq!( + user_shell_turn_outcome(&failed, false), + TurnOutcomeStatus::Failed + ); + + let execution_error = Err(ToolError::execution_failed("shell manager unavailable")); + assert_eq!( + user_shell_turn_outcome(&execution_error, false), + TurnOutcomeStatus::Failed + ); + + let completed = Ok(ToolResult::success("done")); + assert_eq!( + user_shell_turn_outcome(&completed, true), + TurnOutcomeStatus::Interrupted + ); + assert_eq!( + user_shell_turn_outcome(&completed, false), + TurnOutcomeStatus::Completed + ); +} + #[tokio::test] async fn run_shell_command_op_requests_approval_and_executes_shell() { let (mut engine, handle) = Engine::new(EngineConfig::default(), &Config::default()); diff --git a/crates/tui/src/tui/live_transcript.rs b/crates/tui/src/tui/live_transcript.rs index 1abbcf05f..8d82dffd9 100644 --- a/crates/tui/src/tui/live_transcript.rs +++ b/crates/tui/src/tui/live_transcript.rs @@ -167,8 +167,10 @@ impl LiveTranscriptOverlay { let active_rev = app.active_cell_revision; for (idx, cell) in active.entries().iter().enumerate() { let salt = (idx as u64).wrapping_add(1); - // Salt mirrors the main-transcript scheme so cache keys are - // stable across the two overlays for the same active entry. + // This overlay has its own cache and `CellId::Active` already + // separates active entries from history. It only needs the + // positional salt; the main transcript additionally reserves + // bit 63 because active and history rows can reuse one slot. let revision = active_rev .wrapping_mul(0x9E37_79B9_7F4A_7C15) .wrapping_add(salt); diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index 8e3cc1350..98eb3464e 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -162,16 +162,17 @@ impl ChatWidget { if !has_collapsed { let mut cell_revisions: Vec = Vec::with_capacity(app.history.len() + active_entries.len()); - cell_revisions.extend_from_slice(&app.history_revisions); + cell_revisions.extend( + app.history_revisions + .iter() + .copied() + .map(history_entry_revision), + ); if !active_entries.is_empty() { let active_rev = app.active_cell_revision; for i in 0..active_entries.len() { let salt = (i as u64).wrapping_add(1); - cell_revisions.push( - active_rev - .wrapping_mul(0x9E37_79B9_7F4A_7C15) - .wrapping_add(salt), - ); + cell_revisions.push(active_entry_revision(active_rev, salt)); } } // Build identity mapping: filtered index == original index. @@ -235,7 +236,7 @@ impl ChatWidget { continue; } filtered_cells.push(cell); - filtered_revs.push(app.history_revisions[idx]); + filtered_revs.push(history_entry_revision(app.history_revisions[idx])); filtered_to_original.push(idx); } @@ -428,19 +429,49 @@ fn tool_run_summary_revision( ) -> u64 { let mut revision = 0xA11C_EA5E_D00D_2692u64 ^ ((run.start as u64) << 32) ^ (run.count as u64); for idx in run.start..run.start.saturating_add(run.count) { - let cell_revision = revisions.get(idx).copied().unwrap_or_else(|| { - let active_idx = idx.saturating_sub(history_len); - active_entry_revision(active_rev, (active_idx as u64).wrapping_add(1)) - }); + let cell_revision = revisions + .get(idx) + .copied() + .map(history_entry_revision) + .unwrap_or_else(|| { + let active_idx = idx.saturating_sub(history_len); + active_entry_revision(active_rev, (active_idx as u64).wrapping_add(1)) + }); revision = revision.rotate_left(7) ^ cell_revision; } - revision + let extends_into_active = run.start.saturating_add(run.count) > history_len; + revision_in_domain(revision, extends_into_active) +} + +const ACTIVE_REVISION_DOMAIN: u64 = 1 << 63; + +fn revision_in_domain(revision: u64, active: bool) -> u64 { + // The top bit is exclusively a cache-domain tag. Clearing it means raw + // counters that differ only by bit 63 can theoretically alias within one + // domain after 2^63 updates; that lifetime is acceptable, while active and + // committed-history keys must never alias each other. + let payload = revision & !ACTIVE_REVISION_DOMAIN; + if active { + ACTIVE_REVISION_DOMAIN | payload + } else { + payload + } +} + +fn history_entry_revision(revision: u64) -> u64 { + revision_in_domain(revision, false) } fn active_entry_revision(active_rev: u64, salt: u64) -> u64 { - active_rev + // Active entries and committed history cells can occupy the same + // positional cache slot across `flush_active_cell`. Keep their revision + // domains distinct so the first active entry (`active_rev = 0`, + // `salt = 1`) cannot collide with the first history revision (`1`) and + // reuse a stale `running` render after cancellation. + let mixed = active_rev .wrapping_mul(0x9E37_79B9_7F4A_7C15) - .wrapping_add(salt) + .wrapping_add(salt); + revision_in_domain(mixed, true) } impl Renderable for ChatWidget { @@ -3435,20 +3466,23 @@ fn line_spans_with_selection<'a>( #[cfg(test)] mod tests { use super::{ - ApprovalWidget, COMPOSER_PANEL_HEIGHT, COMPOSER_PLACEHOLDER, ChatWidget, ComposerWidget, - Renderable, SlashMenuEntry, apply_detail_target_highlight, apply_selection_to_line, - apply_send_flash, build_empty_state_lines, composer_height, composer_max_height, - composer_min_input_rows, composer_top_padding, cursor_row_col, empty_composer_visual_rows, + ACTIVE_REVISION_DOMAIN, ApprovalWidget, COMPOSER_PANEL_HEIGHT, COMPOSER_PLACEHOLDER, + ChatWidget, ComposerWidget, Renderable, SlashMenuEntry, active_entry_revision, + apply_detail_target_highlight, apply_selection_to_line, apply_send_flash, + build_empty_state_lines, composer_height, composer_max_height, composer_min_input_rows, + composer_top_padding, cursor_row_col, empty_composer_visual_rows, history_entry_revision, layout_input, pad_lines_to_bottom, placeholder_visual_lines, push_command_entry, - should_render_empty_state, slash_completion_hints, wrap_input_lines, - wrap_input_lines_for_mouse, wrap_text, + revision_in_domain, should_render_empty_state, slash_completion_hints, + tool_run_summary_revision, wrap_input_lines, wrap_input_lines_for_mouse, wrap_text, }; use crate::config::{ApiProvider, Config}; use crate::localization::Locale; use crate::palette; use crate::tui::active_cell::ActiveCell; use crate::tui::app::{App, ComposerDensity, ToolCollapseMode, TuiOptions}; - use crate::tui::history::{GenericToolCell, HistoryCell, ToolCell, ToolStatus}; + use crate::tui::history::{ + ExecCell, ExecSource, GenericToolCell, HistoryCell, ToolCell, ToolRun, ToolStatus, + }; use crate::tui::scrolling::{TranscriptLineMeta, TranscriptScroll}; use ratatui::{ buffer::Buffer, @@ -3498,6 +3532,39 @@ mod tests { text } + #[test] + fn first_active_tool_settles_when_flushed_to_history() { + let mut app = create_test_app(); + app.clear_history(); + app.next_history_revision = 1; + app.active_cell_revision = 0; + + let mut active = ActiveCell::new(); + active.push_tool("user_shell_1", running_user_shell_cell()); + app.active_cell = Some(active); + + let area = Rect::new(0, 0, 100, 20); + let mut running_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut running_buf); + let running = buffer_text(&running_buf, area); + assert!(running.contains("run running"), "{running}"); + + app.finalize_active_cell_as_interrupted(); + let HistoryCell::Tool(ToolCell::Exec(exec)) = &app.history[0] else { + panic!("expected settled exec history cell") + }; + assert_eq!(exec.status, ToolStatus::Failed); + + let mut settled_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut settled_buf); + let settled = buffer_text(&settled_buf, area); + assert!( + !settled.contains("run running"), + "flushed terminal state reused the active cache entry:\n{settled}" + ); + assert!(settled.contains("run issue"), "{settled}"); + } + fn render_approval_request( request: &crate::tui::approval::ApprovalRequest, area: Rect, @@ -3530,6 +3597,23 @@ mod tests { })) } + fn running_user_shell_cell() -> HistoryCell { + HistoryCell::Tool(ToolCell::Exec(ExecCell { + command: "sleep 30".to_string(), + status: ToolStatus::Running, + output: None, + live_output: None, + shell_task_id: None, + owner_agent_id: None, + owner_agent_name: None, + started_at: None, + duration_ms: None, + source: ExecSource::User, + interaction: None, + output_summary: None, + })) + } + fn add_dense_tool_run(app: &mut App) { app.add_message(success_tool_cell("read_file")); app.add_message(success_tool_cell("list_dir")); @@ -3575,6 +3659,47 @@ mod tests { assert_eq!(lines[0].spans[0].style.bg, Some(Color::Reset)); } + #[test] + fn tool_run_summary_revision_separates_128_entry_history_and_active_alias() { + let active_rev = 17; + let run = ToolRun { + start: 0, + count: 128, + tool_families: Vec::new(), + activity: Default::default(), + }; + let history_revisions = (1..=run.count) + .map(|salt| active_entry_revision(active_rev, salt as u64)) + .collect::>(); + + let history_key = + tool_run_summary_revision(&run, &history_revisions, run.count, active_rev); + let active_key = tool_run_summary_revision(&run, &[], 0, active_rev); + + // Rotating by seven over 128 entries cancels the 128 identical domain + // bits, reproducing the old untagged hash alias. The final domain tag + // must still keep the cache keys distinct. + assert_eq!( + history_key & !ACTIVE_REVISION_DOMAIN, + active_key & !ACTIVE_REVISION_DOMAIN, + "fixture must exercise the 128-entry payload alias" + ); + assert_eq!(history_key & ACTIVE_REVISION_DOMAIN, 0); + assert_eq!(active_key & ACTIVE_REVISION_DOMAIN, ACTIVE_REVISION_DOMAIN); + assert_ne!(history_key, active_key); + } + + #[test] + fn high_bit_raw_revision_remains_distinct_across_history_and_active_domains() { + let raw = ACTIVE_REVISION_DOMAIN | 0x2692; + let history_key = history_entry_revision(raw); + let active_key = revision_in_domain(raw, true); + + assert_eq!(history_key, 0x2692); + assert_eq!(active_key, ACTIVE_REVISION_DOMAIN | 0x2692); + assert_ne!(history_key, active_key); + } + #[test] fn chat_widget_collapses_dense_tool_runs_by_default() { let mut app = create_test_app(); @@ -3639,6 +3764,46 @@ mod tests { ); } + #[test] + fn collapsed_slow_path_does_not_reuse_running_active_cache_after_flush() { + let mut app = create_test_app(); + app.tool_collapse_mode = ToolCollapseMode::Compact; + app.tool_collapse_threshold = 3; + add_dense_tool_run(&mut app); + + // Force the next committed history revision to have the same raw key + // as active revision 0, salt 1. The prior collapsed run keeps both + // renders on the filtered slow path. + app.next_history_revision = ACTIVE_REVISION_DOMAIN | 1; + app.active_cell_revision = 0; + let mut active = ActiveCell::new(); + active.push_tool("user_shell_slow_path", running_user_shell_cell()); + app.active_cell = Some(active); + + let area = Rect::new(0, 0, 100, 20); + let mut running_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut running_buf); + let running = buffer_text(&running_buf, area); + assert!(running.contains("run running"), "{running}"); + assert_eq!(app.collapsed_cell_map, vec![0, 3]); + + app.finalize_active_cell_as_interrupted(); + assert_eq!( + app.history_revisions[3], + ACTIVE_REVISION_DOMAIN | 1, + "fixture must force the old raw-revision collision" + ); + + let mut settled_buf = Buffer::empty(area); + ChatWidget::new(&mut app, area).render(area, &mut settled_buf); + let settled = buffer_text(&settled_buf, area); + assert!( + !settled.contains("run running"), + "history cell reused the active slow-path cache entry:\n{settled}" + ); + assert!(settled.contains("run issue"), "{settled}"); + } + #[test] fn chat_widget_expands_dense_tool_runs_on_demand() { let mut app = create_test_app(); diff --git a/crates/tui/tests/qa_pty.rs b/crates/tui/tests/qa_pty.rs index 54c572711..6933bed10 100644 --- a/crates/tui/tests/qa_pty.rs +++ b/crates/tui/tests/qa_pty.rs @@ -350,6 +350,77 @@ fn work_and_permission_are_visible_at_release_terminal_sizes() -> anyhow::Result Ok(()) } +/// A composer `!` command is a host-owned shell turn. Cancelling it must +/// settle the transcript card instead of leaving a permanent `run running` +/// spinner after the process has been killed. +#[test] +fn cancelled_bang_shell_settles_transcript_card() -> anyhow::Result<()> { + let _guard = qa_pty_test_lock(); + let ws = make_sealed_workspace()?; + let mut h = Harness::builder(Harness::cargo_bin("codewhale-tui")) + .cwd(ws.workspace()) + .clear_env() + .seal_home(ws.home()) + // Match the Android/Termux release probe: `--skip-onboarding` with no + // provider credential leaves the bang shell as the first transcript + // cell, which is the cache-transition edge this regression covers. + .env("DEEPSEEK_API_KEY", "") + .env("DEEPSEEK_BASE_URL", "http://127.0.0.1:1") + .env("RUST_LOG", "warn") + .args([ + "--workspace", + ws.workspace().to_str().expect("utf-8 workspace path"), + "--no-project-config", + "--skip-onboarding", + "--yolo", + ]) + .size(32, 120) + .spawn()?; + + h.wait_for_text(COMPOSER_READY_TEXT, BOOT_TIMEOUT)?; + let command = "! echo $$ > shell.pid; sleep 30 & echo $! > sleep.pid; \ + echo CWQA_SHELL_STARTED; wait"; + h.send(keys::key::text(command))?; + h.wait_for_text("CWQA_SHELL_STARTED", KEY_TIMEOUT)?; + h.wait_for_idle(Duration::from_millis(150), Duration::from_secs(2))?; + h.send(keys::key::enter())?; + h.wait_for_text("run running", KEY_TIMEOUT)?; + let process_deadline = std::time::Instant::now() + KEY_TIMEOUT; + while (!ws.workspace().join("shell.pid").exists() || !ws.workspace().join("sleep.pid").exists()) + && std::time::Instant::now() < process_deadline + { + std::thread::sleep(Duration::from_millis(20)); + } + assert!(ws.workspace().join("shell.pid").exists()); + assert!(ws.workspace().join("sleep.pid").exists()); + + h.send(b"\x03")?; + h.wait_for_text("Request cancelled", KEY_TIMEOUT)?; + h.wait_for( + |frame| !frame.contains("run running"), + Duration::from_secs(5), + )?; + h.wait_for_idle(Duration::from_millis(250), Duration::from_secs(5))?; + + let frame = h.frame(); + let dump = frame.debug_dump(); + assert!( + !frame.contains("run running"), + "cancelled bang shell stayed live in transcript:\n{dump}" + ); + assert!( + frame.contains("run issue") || frame.contains("interrupted"), + "cancelled bang shell did not expose a terminal card:\n{dump}" + ); + assert!( + !frame.contains("turn completed"), + "cancelled bang shell was reported as a completed turn:\n{dump}" + ); + + let _ = h.shutdown(); + Ok(()) +} + /// Regression: `/skills` should reflect the same merged discovery set as the /// slash menu and model-visible skills block, not just the first selected /// skills directory.