diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ffa59a32..55ce98a4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -352,6 +352,14 @@ jobs: - name: Check the application builds for Windows if: runner.os == 'Windows' run: cargo check --locked -p nocterm + # Held directories, chat history and agent launch have Windows-only code paths: + # run them against the real filesystem and process APIs there. + - name: Test Windows held directories, chat history and agent launch + if: runner.os == 'Windows' + run: | + cargo clippy --locked -p nocterm-localfs -p nocterm-agent-storage -p nocterm-agent-runtime --all-targets -- -D warnings + cargo test --locked -p nocterm-localfs -p nocterm-ai -p nocterm-agent-storage + cargo test --locked -p nocterm --test native_chat_history # The one check to require in branch protection: it passes only when every # job above passed or was skipped by design, so adding a job never needs a diff --git a/Cargo.lock b/Cargo.lock index 7f345ea8..4c7b917e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5125,6 +5125,7 @@ dependencies = [ "async-channel", "async-io", "async-process", + "dunce", "futures", "getrandom 0.4.3", "nix 0.28.0", @@ -5754,6 +5755,7 @@ dependencies = [ "nocterm-session", "rustix 1.1.5", "tempfile", + "winapi-util", "windows-sys 0.61.2", ] diff --git a/THIRD_PARTY_NOTICES.txt b/THIRD_PARTY_NOTICES.txt index fe6a0008..e2f378d4 100644 --- a/THIRD_PARTY_NOTICES.txt +++ b/THIRD_PARTY_NOTICES.txt @@ -2,9 +2,9 @@ Nocterm third-party licenses and notices ======================================== Generated by scripts/generate-license-notices.py using cargo-about 0.9.2. -Content SHA-256: 0ae784cc76fcd35b8b92606da1fa1473f168a85fb6faa2a841081804479c9c4b -Input fingerprint: 26aa78e6379ef0913ad6e111b9a8e0f0630d7d60937c762824cc783217844bbf -Cargo.lock SHA-256 (release version masked): 2b2bf6b37fd5a884b37b99d97b196e2f5f4989e0e95914a9a09dd70dae34c1d8 +Content SHA-256: 2f360cfb2a7cd01c7ce3ef0e3dd344f3a2ca39dbfdf737e4d17724bd52fe913b +Input fingerprint: d1693647fcfff0284baa3b8a794e4462e07e469e5dd606539116650db970e01b +Cargo.lock SHA-256 (release version masked): 3636147a9a00b37439d037e23d79a100118c4ed220e768415a1da6fadf46a302 Scope: runtime and build dependencies of Nocterm and its optional vault broker for Linux x86_64, Windows x86_64, macOS x86_64 and macOS arm64. diff --git a/crates/acp/Cargo.toml b/crates/acp/Cargo.toml index 11226a1d..07a1b8c3 100644 --- a/crates/acp/Cargo.toml +++ b/crates/acp/Cargo.toml @@ -26,6 +26,9 @@ subtle = "2" [target.'cfg(unix)'.dependencies] nix = { version = "0.28", features = ["signal", "process"] } +[target.'cfg(windows)'.dependencies] +dunce = "1.0.5" + [target.'cfg(target_os = "linux")'.dependencies] rustix = { version = "1.1", features = ["process", "event"] } diff --git a/crates/acp/src/lib.rs b/crates/acp/src/lib.rs index e7a87390..bc230dcd 100644 --- a/crates/acp/src/lib.rs +++ b/crates/acp/src/lib.rs @@ -10,6 +10,8 @@ //! Invariants: SDK types never leave this crate; relay registrations authenticate callers; revocation closes owned sockets. //! The bridge lists and admits only the tools a registration's chat offers, and tells its //! connected agents when the offer changes without blocking the chat. +//! Windows agents and taskkill start without a console window; bare commands fall back to +//! the usual Node.js locations after PATH (ADR 0163). //! Native taskkill diagnostics retain the original I/O error through shared typed causes; //! the public process shutdown port retains its existing string diagnostic. mod bridge; diff --git a/crates/acp/src/process.rs b/crates/acp/src/process.rs index 7244332e..574ab76d 100644 --- a/crates/acp/src/process.rs +++ b/crates/acp/src/process.rs @@ -8,6 +8,7 @@ use std::{ use nocterm_ai::{AgentError, ConnectRequest}; mod lifecycle; +mod locations; pub(crate) use lifecycle::{CleanupError, ProcessGroup}; #[cfg(target_os = "linux")] mod host; @@ -26,20 +27,29 @@ pub(crate) fn spawn( request: &ConnectRequest, helper: Option<&Path>, ) -> Result { - let env = nocterm_ai::env::sanitized_environment( + let mut env = nocterm_ai::env::sanitized_environment( &request.launch, std::env::vars_os() .filter_map(|(key, value)| Some((key.into_string().ok()?, value.into_string().ok()?))), ); let secrets = secret_values(request, &env); - let executable = resolve(&request.launch.command, &env, &request.working_directory) - .ok_or_else(|| { - AgentError::Io(nocterm_ai::redact::redact(&format!( - "Cannot find agent command '{}'. Configure an absolute executable path; inherited PATH is {}", - request.launch.command, - env.get("PATH").map_or("", String::as_str) - ))) - })?; + let executable = match resolve(&request.launch.command, &env, &request.working_directory) { + Some(executable) => executable, + None => { + let (executable, directory) = resolve_fallback(&request.launch.command, &env) + .ok_or_else(|| { + AgentError::Io(nocterm_ai::redact::redact(&format!( + "Cannot find agent command '{}' in PATH or the usual Node.js locations. \ + Install it or configure an absolute executable path; inherited PATH is {}", + request.launch.command, + env.get("PATH").map_or("", String::as_str) + ))) + })?; + // Scripts such as npx.cmd start `node` from the same search path. + prepend_path(&mut env, directory); + executable + } + }; let (executable, process_args) = match &request.sandbox { None => (executable, request.launch.args.clone()), Some(policy) => crate::sandbox::SandboxLauncher::prepare( @@ -100,6 +110,14 @@ fn spawn_child( use std::os::unix::process::CommandExt as _; command.process_group(0); } + #[cfg(windows)] + { + use std::os::windows::process::CommandExt as _; + // The application has no console of its own. Without this flag Windows opens + // a console window for every console agent (npx.cmd, node) it starts. The + // hidden console is inherited, so the agent's own tools open none either. + command.creation_flags(CREATE_NO_WINDOW); + } async_process::Command::from(command) .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -129,6 +147,62 @@ fn secret_values(request: &ConnectRequest, env: &BTreeMap) -> Ar Arc::new(values) } +/// `CREATE_NO_WINDOW` from the Windows process creation flags. +#[cfg(windows)] +pub(crate) const CREATE_NO_WINDOW: u32 = 0x0800_0000; + +/// Look for a bare command in the usual install locations once `PATH` failed. +fn resolve_fallback(command: &str, env: &BTreeMap) -> Option<(PathBuf, PathBuf)> { + if Path::new(command).components().count() != 1 || Path::new(command).is_absolute() { + return None; + } + locations::fallback(env) + .into_iter() + .find_map(|directory| Some((find_in(&directory, command, env)?, directory))) +} +fn prepend_path(env: &mut BTreeMap, directory: PathBuf) { + let inherited = env.get("PATH").cloned().unwrap_or_default(); + let paths = std::iter::once(directory).chain(std::env::split_paths(&inherited)); + if let Ok(path) = std::env::join_paths(paths) { + env.insert("PATH".into(), path.to_string_lossy().into_owned()); + } +} +fn find_in(directory: &Path, command: &str, env: &BTreeMap) -> Option { + let candidate = directory.join(command); + if executable(&candidate) { + return canonical(&candidate); + } + #[cfg(windows)] + for suffix in env + .get("PATHEXT") + .map_or(".EXE;.CMD;.BAT", String::as_str) + .split(';') + .filter(|suffix| !suffix.is_empty()) + { + let candidate = directory.join(format!("{command}{suffix}")); + if executable(&candidate) { + return canonical(&candidate); + } + } + #[cfg(not(windows))] + let _ = env; + None +} + +/// The resolved program path. On Windows it keeps the ordinary `C:\` form where +/// possible: `cmd.exe`, which runs `.cmd` scripts such as npx.cmd, does not accept +/// verbatim `\\?\` paths. +fn canonical(path: &Path) -> Option { + #[cfg(windows)] + { + dunce::canonicalize(path).ok() + } + #[cfg(not(windows))] + { + path.canonicalize().ok() + } +} + fn resolve( command: &str, env: &BTreeMap, @@ -141,9 +215,7 @@ fn resolve( } else { working_directory.join(path) }; - return executable(&path) - .then(|| path.canonicalize().ok()) - .flatten(); + return executable(&path).then(|| canonical(&path)).flatten(); } let paths = env.get("PATH")?; for directory in std::env::split_paths(paths) { @@ -156,20 +228,8 @@ fn resolve( } else { working_directory.join(directory) }; - let candidate = directory.join(command); - if executable(&candidate) { - return candidate.canonicalize().ok(); - } - #[cfg(windows)] - for suffix in env - .get("PATHEXT") - .map_or(".EXE;.CMD;.BAT", String::as_str) - .split(';') - { - let candidate = directory.join(format!("{command}{suffix}")); - if executable(&candidate) { - return candidate.canonicalize().ok(); - } + if let Some(found) = find_in(&directory, command, env) { + return Some(found); } } None diff --git a/crates/acp/src/process/lifecycle.rs b/crates/acp/src/process/lifecycle.rs index d4f12346..e74213ca 100644 --- a/crates/acp/src/process/lifecycle.rs +++ b/crates/acp/src/process/lifecycle.rs @@ -156,10 +156,12 @@ fn stop_group(pid: u32, force: bool) -> Result<(), CleanupError> { #[cfg(windows)] { let _ = force; + use std::os::windows::process::CommandExt as _; let status = std::process::Command::new("taskkill") .args(["/PID", &pid.to_string(), "/T", "/F"]) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) + .creation_flags(super::CREATE_NO_WINDOW) .status(); confirm_taskkill(status)?; } diff --git a/crates/acp/src/process/locations.rs b/crates/acp/src/process/locations.rs new file mode 100644 index 00000000..54f24666 --- /dev/null +++ b/crates/acp/src/process/locations.rs @@ -0,0 +1,55 @@ +//! Where a bare agent command such as `npx` is looked for when the inherited +//! search path does not contain it. Applications started from the Windows shell +//! or the macOS Finder often inherit a shorter search path than a terminal, so +//! the usual Node.js install locations are tried after it, in this order. +use std::{collections::BTreeMap, path::PathBuf}; + +/// Directories to search after `PATH`, built only from the sanitized environment. +pub(super) fn fallback(env: &BTreeMap) -> Vec { + let var = |name: &str| { + env.get(name) + .filter(|value| !value.is_empty()) + .map(PathBuf::from) + }; + let mut directories = Vec::new(); + if cfg!(windows) { + directories.extend(var("NVM_SYMLINK")); + directories.extend(var("VOLTA_HOME").map(|home| home.join("bin"))); + directories.extend(var("LOCALAPPDATA").map(|local| local.join("Volta").join("bin"))); + for program_files in ["PROGRAMFILES", "PROGRAMW6432"] { + directories.extend(var(program_files).map(|root| root.join("nodejs"))); + } + directories.extend(var("LOCALAPPDATA").map(|local| local.join("Programs").join("nodejs"))); + directories.extend(var("APPDATA").map(|roaming| roaming.join("npm"))); + directories.extend(var("USERPROFILE").map(|home| home.join("scoop").join("shims"))); + directories.extend(var("PROGRAMDATA").map(|data| data.join("chocolatey").join("bin"))); + } else if cfg!(target_os = "macos") { + directories.push(PathBuf::from("/opt/homebrew/bin")); + directories.push(PathBuf::from("/usr/local/bin")); + directories.extend(var("HOME").map(|home| home.join(".volta").join("bin"))); + } + // Only absolute locations: a relative one would search the working directory. + directories.retain(|directory| directory.is_absolute()); + directories.dedup(); + directories +} + +#[cfg(test)] +mod tests { + use super::fallback; + use std::collections::BTreeMap; + + #[test] + fn relative_and_empty_locations_are_never_searched() { + let env = BTreeMap::from([ + ("APPDATA".to_owned(), "relative".to_owned()), + ("HOME".to_owned(), String::new()), + ("PROGRAMFILES".to_owned(), String::new()), + ]); + assert!( + fallback(&env) + .iter() + .all(|directory| directory.is_absolute()) + ); + } +} diff --git a/crates/agent-chat/src/thread.rs b/crates/agent-chat/src/thread.rs index c54e67c6..287deebf 100644 --- a/crates/agent-chat/src/thread.rs +++ b/crates/agent-chat/src/thread.rs @@ -46,6 +46,7 @@ mod tool_runtime; pub use restart::RestartData; mod document; pub(crate) mod local_payloads; +mod save_failure; mod saved; mod sign_in_wait; mod title; diff --git a/crates/agent-chat/src/thread/client.rs b/crates/agent-chat/src/thread/client.rs index 618388f2..07d5965a 100644 --- a/crates/agent-chat/src/thread/client.rs +++ b/crates/agent-chat/src/thread/client.rs @@ -701,9 +701,7 @@ impl AgentThread { error: crate::runtime::AgentPersistenceError, cx: &mut Context, ) { - let message = format!( - "Could not save chat: {error}. Queued messages are retained; check disk space before continuing." - ); + let message = super::save_failure::message(&error); if !self.state.failed(revision, error) { return; } diff --git a/crates/agent-chat/src/thread/save_failure.rs b/crates/agent-chat/src/thread/save_failure.rs new file mode 100644 index 00000000..ce5a4cab --- /dev/null +++ b/crates/agent-chat/src/thread/save_failure.rs @@ -0,0 +1,54 @@ +//! Guidance shown when a chat snapshot could not be saved. Queued messages stay +//! queued in every case; only the advice depends on why the save failed. +use crate::runtime::{AgentPersistenceError, PersistenceFailure}; + +pub(super) fn message(error: &AgentPersistenceError) -> String { + match PersistenceFailure::of(error) { + PersistenceFailure::Unsupported => format!( + "Could not save chat: {error}. This is a limitation of the history storage on this \ + system, not a disk or agent problem. Queued messages are retained and are not sent \ + until the chat can be saved." + ), + PersistenceFailure::StorageFull => format!( + "Could not save chat: the disk holding chat history is full ({error}). Queued \ + messages are retained; free disk space before continuing." + ), + PersistenceFailure::AccessDenied => format!( + "Could not save chat: access to the chat history folder was denied ({error}). Queued \ + messages are retained; check the folder's permissions before continuing." + ), + PersistenceFailure::Write => format!( + "Could not save chat: {error}. Queued messages are retained and are sent once the \ + chat has been saved." + ), + } +} + +#[cfg(test)] +mod tests { + use super::message; + use nocterm_ai::storage::AgentStorageError; + use nocterm_session::local_fs::LocalFsError; + use std::{io, sync::Arc}; + + #[test] + fn only_a_full_disk_suggests_freeing_space() { + let unsupported: crate::runtime::AgentPersistenceError = + AgentStorageError::EnvironmentUnsupported(Arc::new(Default::default())).into(); + let full: crate::runtime::AgentPersistenceError = AgentStorageError::from( + LocalFsError::from_io("history", io::Error::from(io::ErrorKind::StorageFull)), + ) + .into(); + let failed: crate::runtime::AgentPersistenceError = AgentStorageError::from( + LocalFsError::from_io("history", io::Error::other("device error")), + ) + .into(); + assert!(message(&unsupported).contains("limitation of the history storage")); + assert!(!message(&unsupported).contains("disk space")); + assert!(message(&full).contains("free disk space")); + assert!(!message(&failed).contains("disk space")); + for text in [message(&unsupported), message(&full), message(&failed)] { + assert!(text.contains("Queued messages are retained")); + } + } +} diff --git a/crates/agent-runtime/src/lib.rs b/crates/agent-runtime/src/lib.rs index f441cc31..adeb4ffd 100644 --- a/crates/agent-runtime/src/lib.rs +++ b/crates/agent-runtime/src/lib.rs @@ -63,7 +63,8 @@ use nocterm_ai::{ }; use std::{collections::HashMap, path::PathBuf, sync::Arc}; pub use storage::{ - AgentPersistenceError, AgentStorage, HistoryReader, LoadedHistory, PreparedHistoryRead, + AgentPersistenceError, AgentStorage, HistoryReader, LoadedHistory, PersistenceFailure, + PreparedHistoryRead, }; pub struct TerminalAuthRequest { diff --git a/crates/agent-runtime/src/storage.rs b/crates/agent-runtime/src/storage.rs index 36e6478a..feef5b94 100644 --- a/crates/agent-runtime/src/storage.rs +++ b/crates/agent-runtime/src/storage.rs @@ -5,7 +5,9 @@ use nocterm_ai::{ }; use std::sync::Arc; mod admitted; +mod failure; pub use admitted::LoadedHistory; +pub use failure::PersistenceFailure; pub struct AgentStorage { state: Arc, diff --git a/crates/agent-runtime/src/storage/failure.rs b/crates/agent-runtime/src/storage/failure.rs new file mode 100644 index 00000000..79c74220 --- /dev/null +++ b/crates/agent-runtime/src/storage/failure.rs @@ -0,0 +1,123 @@ +//! Why a chat save failed, read from its typed cause chain for user guidance. +use super::AgentPersistenceError; +use nocterm_ai::storage::AgentStorageError; +use nocterm_session::local_fs::LocalFsError; +use std::error::Error; + +/// The class of a failed save. Each class needs different guidance: only a full +/// volume is fixed by freeing space, and an unsupported location is not fixed by +/// retrying at all. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum PersistenceFailure { + /// The history storage cannot provide the guarantees this chat's location needs. + Unsupported, + /// The volume holding the history is out of space or quota. + StorageFull, + /// The operating system refused access to the history location. + AccessDenied, + /// Any other filesystem, encoding or persistence-worker failure. + Write, +} +impl PersistenceFailure { + pub fn of(error: &AgentPersistenceError) -> Self { + match error { + AgentPersistenceError::Storage(error) => classify(error), + AgentPersistenceError::Worker(error) => classify(error), + } + } +} + +fn classify(error: &(dyn Error + 'static)) -> PersistenceFailure { + let mut frame = Some(error); + let mut denied = false; + while let Some(current) = frame { + if let Some( + AgentStorageError::EnvironmentUnsupported(_) | AgentStorageError::ScopeUnsupported(_), + ) = current.downcast_ref::() + { + return PersistenceFailure::Unsupported; + } + if let Some(LocalFsError::PermissionDenied(_)) = current.downcast_ref::() { + denied = true; + } + if let Some(io) = current.downcast_ref::() { + match io.kind() { + std::io::ErrorKind::StorageFull | std::io::ErrorKind::QuotaExceeded => { + return PersistenceFailure::StorageFull; + } + std::io::ErrorKind::PermissionDenied => denied = true, + _ => {} + } + } + frame = current.source(); + } + if denied { + PersistenceFailure::AccessDenied + } else { + PersistenceFailure::Write + } +} + +#[cfg(test)] +mod tests { + use super::*; + use nocterm_core::{SharedCause, SharedError}; + use std::{io, sync::Arc}; + + fn filesystem(error: io::Error) -> AgentStorageError { + LocalFsError::from_io("history", error).into() + } + fn worker(error: AgentStorageError) -> AgentPersistenceError { + nocterm_store::StoreError::Backend(SharedCause::new(error)).into() + } + + #[test] + fn a_missing_capability_is_unsupported_directly_and_behind_the_writer() { + let error = AgentStorageError::EnvironmentUnsupported(Arc::new(Default::default())); + assert_eq!( + PersistenceFailure::of(&error.clone().into()), + PersistenceFailure::Unsupported + ); + assert_eq!( + PersistenceFailure::of(&worker(error)), + PersistenceFailure::Unsupported + ); + } + #[test] + fn only_a_full_volume_or_quota_is_storage_full() { + for kind in [io::ErrorKind::StorageFull, io::ErrorKind::QuotaExceeded] { + assert_eq!( + PersistenceFailure::of(&worker(filesystem(io::Error::from(kind)))), + PersistenceFailure::StorageFull + ); + } + let cleanup = AgentStorageError::StageCleanup { + operation: SharedError::from_error(LocalFsError::from_io( + "history", + io::Error::from(io::ErrorKind::StorageFull), + )), + cleanup: SharedError::from_error(LocalFsError::Cancelled), + }; + assert_eq!( + PersistenceFailure::of(&worker(cleanup)), + PersistenceFailure::StorageFull + ); + } + #[test] + fn refused_access_and_other_failures_are_distinguished() { + assert_eq!( + PersistenceFailure::of(&worker(filesystem(io::Error::from( + io::ErrorKind::PermissionDenied + )))), + PersistenceFailure::AccessDenied + ); + assert_eq!( + PersistenceFailure::of(&worker(filesystem(io::Error::other("device error")))), + PersistenceFailure::Write + ); + assert_eq!( + PersistenceFailure::of(&nocterm_store::StoreError::Closing.into()), + PersistenceFailure::Write + ); + } +} diff --git a/crates/agent-storage/src/state.rs b/crates/agent-storage/src/state.rs index c2ce5e3f..0a5faf7c 100644 --- a/crates/agent-storage/src/state.rs +++ b/crates/agent-storage/src/state.rs @@ -72,6 +72,7 @@ mod tests { assert!(error.source().is_some()); assert_eq!(std::fs::read(&path).unwrap(), before); } + #[cfg(unix)] #[test] fn state_port_round_trips_latest_preferences_with_private_publication() { use std::os::unix::fs::PermissionsExt as _; diff --git a/crates/ai/src/env.rs b/crates/ai/src/env.rs index 79c406a3..bfedb9f9 100644 --- a/crates/ai/src/env.rs +++ b/crates/ai/src/env.rs @@ -39,55 +39,122 @@ pub fn credential_override(name: &str) -> bool { || name.contains("SECRET") || name == "AUTHORIZATION" } -fn allowed(name: &str) -> bool { - [ - "PATH", - "HOME", - "USER", - "LOGNAME", - "LANG", - "TMPDIR", - "TMP", - "TEMP", - "DISPLAY", - "WAYLAND_DISPLAY", - "DBUS_SESSION_BUS_ADDRESS", - "SYSTEMROOT", - "SystemRoot", - "WINDIR", - "COMSPEC", - "PATHEXT", - "APPDATA", - "LOCALAPPDATA", - "USERPROFILE", - "HTTPS_PROXY", - "HTTP_PROXY", - "ALL_PROXY", - "NO_PROXY", - "https_proxy", - "http_proxy", - "all_proxy", - "no_proxy", - "SSL_CERT_FILE", - "SSL_CERT_DIR", - "NODE_EXTRA_CA_CERTS", - ] - .contains(&name) - || name.starts_with("LC_") - || name.starts_with("XDG_") +const ALLOWED: &[&str] = &[ + "PATH", + "HOME", + "USER", + "LOGNAME", + "LANG", + "TMPDIR", + "TMP", + "TEMP", + "DISPLAY", + "WAYLAND_DISPLAY", + "DBUS_SESSION_BUS_ADDRESS", + "SYSTEMROOT", + "SystemRoot", + "WINDIR", + "COMSPEC", + "PATHEXT", + "APPDATA", + "LOCALAPPDATA", + "USERPROFILE", + "HTTPS_PROXY", + "HTTP_PROXY", + "ALL_PROXY", + "NO_PROXY", + "https_proxy", + "http_proxy", + "all_proxy", + "no_proxy", + "SSL_CERT_FILE", + "SSL_CERT_DIR", + "NODE_EXTRA_CA_CERTS", +]; +/// Windows system locations that Node.js, npm and npx resolve their install, +/// cache and shell paths from. None of them carries a credential. +const WINDOWS_ALLOWED: &[&str] = &[ + "SYSTEMDRIVE", + "PROGRAMFILES", + "PROGRAMW6432", + "PROGRAMDATA", + "COMMONPROGRAMFILES", + "ALLUSERSPROFILE", + "PUBLIC", + "HOMEDRIVE", + "HOMEPATH", + "USERNAME", + "USERDOMAIN", + "COMPUTERNAME", + "OS", + "PROCESSOR_ARCHITECTURE", + "NUMBER_OF_PROCESSORS", + "NVM_HOME", + "NVM_SYMLINK", + "VOLTA_HOME", +]; +/// How the native platform compares environment names. Windows names are +/// case-insensitive: the system spells the search path `Path`, not `PATH`. +#[derive(Clone, Copy)] +pub(crate) struct NameRules { + windows: bool, +} +impl NameRules { + pub(crate) const fn native() -> Self { + Self { + windows: cfg!(windows), + } + } + #[cfg(test)] + pub(crate) const fn windows() -> Self { + Self { windows: true } + } + fn same(self, left: &str, right: &str) -> bool { + if self.windows { + left.eq_ignore_ascii_case(right) + } else { + left == right + } + } + /// One spelling per variable, so an override replaces rather than duplicates it. + fn key(self, name: &str) -> String { + if self.windows { + name.to_ascii_uppercase() + } else { + name.to_owned() + } + } + fn allowed(self, name: &str) -> bool { + let listed = |names: &[&str]| names.iter().any(|known| self.same(known, name)); + listed(ALLOWED) + || (self.windows && listed(WINDOWS_ALLOWED)) + || name.starts_with("LC_") + || name.starts_with("XDG_") + } } pub fn sanitized_environment( launch: &AgentLaunch, parent: impl IntoIterator, ) -> BTreeMap { + sanitized_with(launch, parent, NameRules::native()) +} +pub(crate) fn sanitized_with( + launch: &AgentLaunch, + parent: impl IntoIterator, + rules: NameRules, +) -> BTreeMap { + let inherited = |name: &str| { + launch + .inherit_env + .iter() + .any(|known| rules.same(known, name)) + }; let mut result: BTreeMap<_, _> = parent .into_iter() .filter(|(k, v)| { - valid_name(k) - && !blocked(k) - && !v.contains('\0') - && (allowed(k) || launch.inherit_env.contains(k)) + valid_name(k) && !blocked(k) && !v.contains('\0') && (rules.allowed(k) || inherited(k)) }) + .map(|(k, v)| (rules.key(&k), v)) .collect(); result.extend( launch @@ -96,7 +163,73 @@ pub fn sanitized_environment( .filter(|(k, v)| { valid_name(k) && !blocked(k) && !credential_override(k) && !v.contains('\0') }) - .map(|(k, v)| (k.clone(), v.clone())), + .map(|(k, v)| (rules.key(k), v.clone())), ); result } + +#[cfg(test)] +mod tests { + use super::*; + fn launch() -> AgentLaunch { + AgentLaunch { + id: "codex".into(), + name: "Codex".into(), + command: "npx".into(), + args: Vec::new(), + env: Default::default(), + inherit_env: vec!["Custom_Inherited".into()], + hub: None, + } + } + fn windows_parent() -> Vec<(String, String)> { + [ + ("Path", r"C:\Program Files\nodejs;C:\Windows\system32"), + ("PATHEXT", ".COM;.EXE;.BAT;.CMD"), + ("SystemRoot", r"C:\Windows"), + ("ComSpec", r"C:\Windows\system32\cmd.exe"), + ("ProgramFiles", r"C:\Program Files"), + ("APPDATA", r"C:\Users\user\AppData\Roaming"), + ("CUSTOM_INHERITED", "kept"), + ("Unrequested_Secret", "dropped"), + ("ssh_auth_sock", "dropped"), + ] + .map(|(key, value)| (key.into(), value.into())) + .into() + } + #[test] + fn windows_names_match_without_case_and_get_one_spelling() { + let env = sanitized_with(&launch(), windows_parent(), NameRules::windows()); + assert_eq!(env["PATH"], r"C:\Program Files\nodejs;C:\Windows\system32"); + assert_eq!(env["SYSTEMROOT"], r"C:\Windows"); + assert_eq!(env["COMSPEC"], r"C:\Windows\system32\cmd.exe"); + assert_eq!(env["PROGRAMFILES"], r"C:\Program Files"); + assert_eq!(env["CUSTOM_INHERITED"], "kept"); + assert!(!env.contains_key("Path")); + assert!(!env.values().any(|value| value == "dropped")); + } + #[test] + fn a_windows_override_replaces_the_inherited_path_instead_of_duplicating_it() { + let mut launch = launch(); + launch.env.insert("path".into(), r"D:\node".into()); + let env = sanitized_with(&launch, windows_parent(), NameRules::windows()); + assert_eq!(env["PATH"], r"D:\node"); + assert_eq!( + env.keys() + .filter(|key| key.eq_ignore_ascii_case("path")) + .count(), + 1 + ); + } + #[test] + fn unix_names_stay_case_sensitive() { + let parent = [ + ("Path".to_owned(), "/wrong".to_owned()), + ("PATH".into(), "/bin".into()), + ]; + let env = sanitized_with(&launch(), parent, NameRules { windows: false }); + assert_eq!(env["PATH"], "/bin"); + assert!(!env.contains_key("Path")); + assert!(!env.contains_key("PROGRAMFILES")); + } +} diff --git a/crates/ai/src/storage.rs b/crates/ai/src/storage.rs index d36a70c3..38207fcb 100644 --- a/crates/ai/src/storage.rs +++ b/crates/ai/src/storage.rs @@ -40,13 +40,17 @@ pub enum AgentStorageError { expected: Arc, actual: Arc, }, - #[error("This environment requires held native storage directory capabilities")] + #[error( + "The local filesystem cannot hold the directories this chat environment's history requires" + )] EnvironmentUnsupported(Arc), #[error("The hub account could not be captured")] AccountCapture(#[from] nocterm_hubs_model::HubError), #[error("The captured account source is no longer available")] AccountSource(#[from] nocterm_hubs_model::source_identity::LocalAccountSourceError), - #[error("This context requires held native storage directory capabilities: {0:?}")] + #[error( + "The local filesystem cannot hold the directories this context's history requires: {0:?}" + )] ScopeUnsupported(nocterm_project_model::ContextId), #[error("The local history filesystem operation failed: {0}")] Filesystem(#[source] nocterm_core::SharedError), diff --git a/crates/localfs/Cargo.toml b/crates/localfs/Cargo.toml index d322cca4..13426d28 100644 --- a/crates/localfs/Cargo.toml +++ b/crates/localfs/Cargo.toml @@ -21,6 +21,7 @@ nix = { version = "0.28", features = ["fs"] } [target.'cfg(windows)'.dependencies] atomicwrites = "0.4.4" +winapi-util = "0.1.11" windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem"] } [lints] diff --git a/crates/localfs/src/directory.rs b/crates/localfs/src/directory.rs index 2418f54c..574e4922 100644 --- a/crates/localfs/src/directory.rs +++ b/crates/localfs/src/directory.rs @@ -8,6 +8,8 @@ use std::{ mod portable; #[cfg(unix)] mod unix; +#[cfg(windows)] +mod windows; /// The single private acquisition/publication capability. Browse handles cannot /// escape to the public strict-capability contract. @@ -16,6 +18,9 @@ pub(crate) struct DirectoryCapability { path: PathBuf, #[cfg(unix)] handle: Arc, + /// Strict Windows capabilities hold their whole ancestry; browse ones hold nothing. + #[cfg(windows)] + held: Option>, identity: DirectoryIdentity, } impl DirectoryCapability { @@ -25,7 +30,11 @@ impl DirectoryCapability { { unix::strict(path) } - #[cfg(not(unix))] + #[cfg(windows)] + { + windows::strict(path) + } + #[cfg(not(any(unix, windows)))] { portable::strict(path) } @@ -63,7 +72,11 @@ impl DirectoryCapability { { unix::child(self, name, create) } - #[cfg(not(unix))] + #[cfg(windows)] + { + windows::child(self, name, create) + } + #[cfg(not(any(unix, windows)))] { portable::child(self, name, create) } @@ -111,7 +124,11 @@ impl LocalDirectory for DirectoryCapability { { unix::remove_file(self, name) } - #[cfg(not(unix))] + #[cfg(windows)] + { + windows::remove_file(self, name) + } + #[cfg(not(any(unix, windows)))] { portable::remove_file(self, name) } diff --git a/crates/localfs/src/directory/portable.rs b/crates/localfs/src/directory/portable.rs index c85f1fba..795e7204 100644 --- a/crates/localfs/src/directory/portable.rs +++ b/crates/localfs/src/directory/portable.rs @@ -1,3 +1,4 @@ +//! Path-checked fallbacks: Windows browse handles and platforms without held directories. use super::*; pub(crate) fn is_link(metadata: &std::fs::Metadata) -> bool { #[cfg(windows)] @@ -10,6 +11,7 @@ pub(crate) fn is_link(metadata: &std::fs::Metadata) -> bool { metadata.is_symlink() } } +#[cfg(not(windows))] pub(super) fn strict(path: &Path) -> LocalResult { let mut current = PathBuf::new(); for component in path.components() { @@ -35,9 +37,12 @@ pub(super) fn browse(path: &Path) -> LocalResult { } Ok(DirectoryCapability { path: path.to_owned(), + #[cfg(windows)] + held: None, identity: DirectoryIdentity::portable(path.to_owned()), }) } +#[cfg(not(windows))] pub(super) fn child( directory: &DirectoryCapability, name: &OsStr, @@ -72,6 +77,7 @@ pub(super) fn metadata( pub(super) fn target_is_directory(directory: &DirectoryCapability, name: &OsStr) -> bool { std::fs::metadata(directory.path.join(name)).is_ok_and(|metadata| metadata.is_dir()) } +#[cfg(not(windows))] pub(super) fn remove_file(directory: &DirectoryCapability, name: &OsStr) -> LocalResult<()> { strict(directory.path())?; let path = directory.path.join(name); diff --git a/crates/localfs/src/directory/windows.rs b/crates/localfs/src/directory/windows.rs new file mode 100644 index 00000000..15df8b98 --- /dev/null +++ b/crates/localfs/src/directory/windows.rs @@ -0,0 +1,108 @@ +//! Windows held directories. A strict capability keeps an open handle to every +//! component from the volume root down. Handles are opened without +//! `FILE_SHARE_DELETE`, so while the capability lives no held component can be +//! renamed, removed or replaced by a junction; path operations below it therefore +//! resolve through the same directories that were checked. Reparse points are +//! opened as themselves and refused. +use super::*; +use std::os::windows::fs::OpenOptionsExt as _; +use windows_sys::Win32::Storage::FileSystem::{ + FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, FILE_FLAG_BACKUP_SEMANTICS, + FILE_FLAG_OPEN_REPARSE_POINT, FILE_LIST_DIRECTORY, FILE_READ_ATTRIBUTES, FILE_SHARE_READ, + FILE_SHARE_WRITE, SYNCHRONIZE, +}; + +/// One held directory and, through `parent`, every ancestor above it. +pub(crate) struct Held { + _file: std::fs::File, + _parent: Option>, +} + +/// Open `path` as a directory without following a final reparse point. The +/// requested list access takes part in share accounting, so the omitted delete +/// sharing actually pins the directory's name. +fn open(path: &Path, parent: Option>) -> LocalResult<(Arc, DirectoryIdentity)> { + let file = std::fs::OpenOptions::new() + .access_mode(FILE_LIST_DIRECTORY | FILE_READ_ATTRIBUTES | SYNCHRONIZE) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .custom_flags(FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT) + .open(path) + .map_err(|error| LocalFsError::from_io(path, error))?; + // The opened handle, not the path, supplies the attributes and identity. + let information = winapi_util::file::information(&file) + .map_err(|error| LocalFsError::from_io(path, error))?; + let attributes = information.file_attributes(); + if attributes & u64::from(FILE_ATTRIBUTE_REPARSE_POINT) != 0 + || attributes & u64::from(FILE_ATTRIBUTE_DIRECTORY) == 0 + { + return Err(LocalFsError::LinkRefused(path.to_owned())); + } + Ok(( + Arc::new(Held { + _file: file, + _parent: parent, + }), + DirectoryIdentity::native(information.volume_serial_number(), information.file_index()), + )) +} + +pub(super) fn strict(path: &Path) -> LocalResult { + let mut current = PathBuf::new(); + let mut held = None; + for component in path.components() { + current.push(component); + if matches!(component, Component::Prefix(_)) { + continue; + } + let parent = held.take().map(|(handle, _)| handle); + held = Some(open(¤t, parent)?); + } + let (handle, identity) = held.ok_or_else(|| LocalFsError::InvalidPath(path.to_owned()))?; + Ok(DirectoryCapability { + path: path.to_owned(), + held: Some(handle), + identity, + }) +} + +pub(super) fn child( + directory: &DirectoryCapability, + name: &OsStr, + create: bool, +) -> LocalResult { + // A browse capability followed links and holds nothing; re-establish the chain. + let parent = match &directory.held { + Some(held) => held.clone(), + None => strict(directory.path())? + .held + .ok_or_else(|| LocalFsError::InvalidPath(directory.path().to_owned()))?, + }; + let path = directory.path.join(name); + if create { + match std::fs::create_dir(&path) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => return Err(LocalFsError::from_io(&path, error)), + } + } + let (handle, identity) = open(&path, Some(parent))?; + Ok(DirectoryCapability { + path, + held: Some(handle), + identity, + }) +} + +pub(super) fn remove_file(directory: &DirectoryCapability, name: &OsStr) -> LocalResult<()> { + let _pinned = match directory.held { + Some(_) => None, + None => Some(strict(directory.path())?), + }; + let path = directory.path.join(name); + if super::portable::metadata(directory, name)?.is_some_and(|metadata| { + metadata.kind == nocterm_session::EntryKind::Directory && !metadata.is_symlink + }) { + return Err(LocalFsError::NotRegular(path)); + } + std::fs::remove_file(&path).map_err(|error| LocalFsError::from_io(path, error)) +} diff --git a/crates/localfs/src/lib.rs b/crates/localfs/src/lib.rs index fe4ceedd..471b6728 100644 --- a/crates/localfs/src/lib.rs +++ b/crates/localfs/src/lib.rs @@ -5,8 +5,9 @@ //! Does not own: views, transfer scheduling or remote filesystem protocols. //! Ports used: none. Ports provided: `LocalFs`, `LocalMutations`. //! Settings sections: none. Globals: none. -//! Invariants: strict operations pin every Unix ancestor; read-only browsing alone -//! may follow links. Portable traversal rechecks paths and retains its active-ancestor +//! Invariants: strict operations pin every ancestor (Unix descriptors; Windows handles +//! held without delete sharing, ADR 0162); read-only browsing alone may follow links. +//! Portable traversal on other targets rechecks paths and retains its active-ancestor //! race limitation. Publication errors precede commit; durability warnings follow it. //! A committed stage relinquishes its temporary name before it can be reused. mod cursor; diff --git a/crates/localfs/src/mutations.rs b/crates/localfs/src/mutations.rs index c01d0534..9bfdf5c4 100644 --- a/crates/localfs/src/mutations.rs +++ b/crates/localfs/src/mutations.rs @@ -30,8 +30,8 @@ pub(crate) fn rename(path: &Path, name: &OsStr) -> LocalResult<()> { #[cfg(windows)] { let _ = old; - return atomicwrites::move_atomic(path, &directory.path().join(name)) - .map_err(|error| LocalFsError::from_io(path, error)); + atomicwrites::move_atomic(path, &directory.path().join(name)) + .map_err(|error| LocalFsError::from_io(path, error)) } #[cfg(not(any( target_os = "linux", diff --git a/crates/localfs/tests/capabilities.rs b/crates/localfs/tests/capabilities.rs index 9d1a31d4..70922b85 100644 --- a/crates/localfs/tests/capabilities.rs +++ b/crates/localfs/tests/capabilities.rs @@ -1,6 +1,8 @@ use nocterm_localfs::LocalFilesystem; use nocterm_session::local_fs::{CancelFlag, ListingLimits, LocalFs, LocalFsError}; -use std::{ffi::OsStr, path::Path}; +#[cfg(unix)] +use std::ffi::OsStr; +use std::path::Path; #[cfg(unix)] use std::{io::Write, time::Duration}; diff --git a/crates/localfs/tests/windows_held.rs b/crates/localfs/tests/windows_held.rs new file mode 100644 index 00000000..bafd6f9b --- /dev/null +++ b/crates/localfs/tests/windows_held.rs @@ -0,0 +1,168 @@ +//! Windows strict capabilities hold their ancestry and refuse reparse points. +#![cfg(windows)] +use nocterm_localfs::LocalFilesystem; +use nocterm_session::{ + fs::UploadMode, + local_fs::{LocalDirectory, LocalFs, LocalFsError, PublicationOutcome}, +}; +use std::{ffi::OsStr, path::Path, process::Command, sync::Arc}; + +fn filesystem(root: &Path) -> LocalFilesystem { + LocalFilesystem::with_base(root.to_owned()).unwrap() +} +fn junction(link: &Path, target: &Path) { + let status = Command::new("cmd") + .arg("/C") + .arg("mklink") + .arg("/J") + .arg(link) + .arg(target) + .status() + .unwrap(); + assert!(status.success(), "mklink /J failed"); +} +fn publish(directory: &Arc, name: &str, bytes: &[u8]) { + let mut stage = directory.stage().unwrap(); + stage.write(bytes).unwrap(); + stage.sync().unwrap(); + assert!(matches!( + stage + .publish(OsStr::new(name), UploadMode::Replace) + .unwrap(), + PublicationOutcome::Published | PublicationOutcome::PublishedWithWarning(_) + )); +} + +#[test] +fn strict_directories_have_native_identity_and_pin_every_ancestor() { + let root = tempfile::tempdir().unwrap(); + let root_path = std::fs::canonicalize(root.path()).unwrap(); + std::fs::create_dir_all(root_path.join("a").join("b")).unwrap(); + let fs = filesystem(&root_path); + let held = fs.open_directory(&root_path.join("a").join("b")).unwrap(); + assert!(held.identity().is_held()); + let child = held.create_child(OsStr::new("c")).unwrap(); + assert!(child.identity().is_held()); + assert_ne!(held.identity(), child.identity()); + assert_eq!( + fs.open_directory(&root_path.join("a").join("b").join("c")) + .unwrap() + .identity(), + child.identity() + ); + // Neither the leaf nor an ancestor can be renamed away while it is held. + assert!( + std::fs::rename( + root_path.join("a").join("b").join("c"), + root_path.join("moved-c") + ) + .is_err() + ); + assert!(std::fs::rename(root_path.join("a"), root_path.join("moved-a")).is_err()); + drop(child); + drop(held); + std::fs::rename(root_path.join("a"), root_path.join("moved-a")).unwrap(); +} + +#[test] +fn junctions_are_refused_as_roots_ancestors_and_generated_children() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let root_path = std::fs::canonicalize(root.path()).unwrap(); + std::fs::write(outside.path().join("secret.json"), b"secret").unwrap(); + junction(&root_path.join("link"), outside.path()); + let fs = filesystem(&root_path); + assert!(matches!( + fs.open_directory(&root_path.join("link")), + Err(LocalFsError::LinkRefused(_)) + )); + assert!(matches!( + fs.open_directory(&root_path.join("link").join("inner")), + Err(LocalFsError::LinkRefused(_) | LocalFsError::NotFound(_)) + )); + let directory = fs.open_directory(&root_path).unwrap(); + for create in [false, true] { + let opened = if create { + directory.create_child(OsStr::new("link")) + } else { + directory.child(OsStr::new("link")) + }; + assert!(matches!(opened, Err(LocalFsError::LinkRefused(_)))); + } + assert!( + directory + .metadata(OsStr::new("link")) + .unwrap() + .unwrap() + .is_symlink + ); + assert!(matches!( + directory.open_source(OsStr::new("link")), + Err(LocalFsError::NotRegular(_) | LocalFsError::LinkRefused(_)) + )); + assert_eq!( + std::fs::read(outside.path().join("secret.json")).unwrap(), + b"secret" + ); +} + +#[test] +fn staged_publication_replaces_atomically_and_cancel_leaves_the_old_document() { + let root = tempfile::tempdir().unwrap(); + let root_path = std::fs::canonicalize(root.path()).unwrap(); + let fs = filesystem(&root_path); + let directory = fs + .open_directory(&root_path) + .unwrap() + .create_child(OsStr::new("history")) + .unwrap(); + publish(&directory, "chat.json", b"first"); + publish(&directory, "chat.json", b"second"); + assert_eq!( + std::fs::read(root_path.join("history").join("chat.json")).unwrap(), + b"second" + ); + let mut stage = directory.stage().unwrap(); + stage.write(b"interrupted").unwrap(); + stage.cancel().unwrap(); + assert_eq!( + std::fs::read(root_path.join("history").join("chat.json")).unwrap(), + b"second" + ); + let mut names = Vec::new(); + let mut entries = directory.entries().unwrap(); + while let Some(name) = entries.next().unwrap() { + names.push(name); + } + assert_eq!(names, vec![std::ffi::OsString::from("chat.json")]); + let mut source = directory.open_source(OsStr::new("chat.json")).unwrap(); + assert_eq!(source.read(1024).unwrap(), b"second"); + directory.remove_file(OsStr::new("chat.json")).unwrap(); + assert!( + directory + .metadata(OsStr::new("chat.json")) + .unwrap() + .is_none() + ); +} + +#[test] +fn a_read_only_target_is_reported_as_a_failed_write_without_losing_the_old_document() { + let root = tempfile::tempdir().unwrap(); + let root_path = std::fs::canonicalize(root.path()).unwrap(); + let fs = filesystem(&root_path); + let directory = fs.open_directory(&root_path).unwrap(); + publish(&directory, "chat.json", b"kept"); + let target = root_path.join("chat.json"); + let original = std::fs::metadata(&target).unwrap().permissions(); + let mut read_only = original.clone(); + read_only.set_readonly(true); + std::fs::set_permissions(&target, read_only).unwrap(); + let mut stage = directory.stage().unwrap(); + stage.write(b"replacement").unwrap(); + let refused = stage.publish(OsStr::new("chat.json"), UploadMode::Replace); + assert!(refused.is_err()); + stage.cancel().unwrap(); + assert_eq!(std::fs::read(&target).unwrap(), b"kept"); + std::fs::set_permissions(&target, original).unwrap(); +} diff --git a/docs/AI_AGENTS.md b/docs/AI_AGENTS.md index 6dcfe9e3..ff68d404 100644 --- a/docs/AI_AGENTS.md +++ b/docs/AI_AGENTS.md @@ -45,8 +45,11 @@ configured defaults, not proof that all agents have been authenticated or successfully tested on your machine. Desktop applications may receive a different PATH from an interactive shell, -particularly with nvm. Set an absolute executable path in AI Settings when the -program cannot be found. Arguments are a JSON array of individual strings; +particularly with nvm. When a bare command such as `npx` is not on PATH, Nocterm +also looks in the usual Node.js locations: nvm-windows, Volta, `Program Files\nodejs`, +the per-user Node.js install, npm's global directory, Scoop and Chocolatey on Windows, +and Homebrew and Volta on macOS. Set an absolute executable path in AI Settings when the +program still cannot be found. On Windows agents start without a console window. Arguments are a JSON array of individual strings; Nocterm does not run them through a shell. A blank override keeps the built-in value; an empty arguments array removes the built-in arguments. Custom agent ids use letters, numbers, underscore or dash and need an executable. diff --git a/docs/adr/0162-windows-holds-directory-handles-for-strict-local-capabilities.md b/docs/adr/0162-windows-holds-directory-handles-for-strict-local-capabilities.md new file mode 100644 index 00000000..241d4caf --- /dev/null +++ b/docs/adr/0162-windows-holds-directory-handles-for-strict-local-capabilities.md @@ -0,0 +1,46 @@ +# 0162. Windows holds directory handles for strict local filesystem capabilities + +Status: accepted. +- Amends [0110](0110-local-history-environment-partitions.md): Windows now supplies the held + directory capabilities that new v3 chat namespaces require. + +## Context + +New chats are environment-scoped v3 documents ([0110](0110-local-history-environment-partitions.md)). +Their partitions require held directory capabilities, so that one account's or project's +history cannot be redirected into another through a swapped directory or link. + +`nocterm-localfs` implemented held capabilities only on Unix, with directory descriptors and +`*at` calls. Windows used the portable fallback, whose identities are locations rather than +held directories. Every save of a new chat on Windows was refused with +`EnvironmentUnsupported`, the queue paused, and the notice advised checking disk space. + +## Decision + +- On Windows a strict `DirectoryCapability` holds an open handle to every directory from the + volume root down to itself. Each handle is opened with list access, without + `FILE_SHARE_DELETE` and with `FILE_FLAG_OPEN_REPARSE_POINT`. While the capability lives, + no held directory can be renamed, removed or replaced by a junction, so path operations + below it resolve through the same directories that were checked. +- Every component is opened as itself; a reparse point (symbolic link, junction, mount + point) or a non-directory is refused as `LinkRefused`. +- The identity is the volume serial number and file index read from the held handle, and + is reported as native, so `is_held` is true because the handles are actually held. +- A child capability is opened below its parent's held chain. A child of a browse + capability, which followed links and holds nothing, first re-establishes a strict chain. +- Staged publication, sources, listings and removal keep their existing Windows code. They + run below a held chain, and publication still replaces the target atomically. +- Other non-Unix targets keep the portable fallback and the typed refusal of 0110. +- A failed save names its class: unsupported storage, a full volume, refused access or + another failure. Only a full volume suggests freeing disk space. + +## Consequences + +- New chats save on Windows in every environment and context, with the same partition and + owner checks as on Unix. +- While a capability is held, other programs cannot rename or delete the held directories; + capabilities are short-lived, so this lasts for one operation. +- A directory another program holds open with delete access cannot be opened strictly until + that program closes it. +- On ReFS the 64-bit file index is the lower part of the 128-bit file ID; identities are only + compared within one operation. diff --git a/docs/adr/0163-windows-agents-start-hidden-with-the-systems-search-path.md b/docs/adr/0163-windows-agents-start-hidden-with-the-systems-search-path.md new file mode 100644 index 00000000..58e17862 --- /dev/null +++ b/docs/adr/0163-windows-agents-start-hidden-with-the-systems-search-path.md @@ -0,0 +1,37 @@ +# 0163. Windows agents start without a console window and with the system's search path + +Status: accepted. + +## Context + +Local agents such as Codex start through `npx`. On Windows two things went wrong: + +- The agent environment allow-list compared names case-sensitively. Windows spells the + search path `Path`, so it was dropped and `npx` was never found unless the user typed + its absolute path. +- The release build is a GUI application without a console. Every console program it + started (`npx.cmd`, `node`, `taskkill`) opened its own console window. + +## Decision + +- On Windows the allow-list and the explicitly inherited names match without case, and every + name is stored in upper case, so a settings override replaces the inherited value rather + than adding a second spelling. The Windows system locations Node.js, npm and npx read + (`SystemDrive`, `ProgramFiles`, `ProgramData`, `USERNAME`, `NVM_SYMLINK`, `VOLTA_HOME` and + similar) are allowed; none carries a credential. +- When a bare command is not on `PATH`, the usual Node.js install locations are searched: + nvm-windows, Volta, `Program Files\nodejs`, the per-user Node.js install, npm's global + directory, Scoop and Chocolatey on Windows; Homebrew and Volta on macOS. A command found + there has its directory put first on the agent's `PATH`, so `npx.cmd` finds `node`. +- A resolved program keeps the ordinary `C:\` form instead of a verbatim `\\?\` path, which + `cmd.exe` cannot run. +- The agent process and the `taskkill` that stops its tree start with `CREATE_NO_WINDOW`. + The agent gets a hidden console, which its own tools inherit, so they open no windows + either. + +## Consequences + +- A default Codex or Claude launch works on Windows without an absolute path when Node.js is + installed in one of the usual places. +- No console window appears when a chat starts or stops an agent. +- Interactive sign-in still runs in Nocterm's own terminal tab. diff --git a/docs/adr/README.md b/docs/adr/README.md index 41ac0e8d..03cc04b2 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -157,3 +157,5 @@ acceptance; a later record supersedes an earlier one. | [0159](0159-plugin-charts-toolbar-axis-and-toolbar-moves.md) | Plugin views draw charts of recent values (`chart`, ABI stays 1), a toolbar's calls name its axis in the zone (`cx.is_column()`), toolbars use the status bar's small text, and moving a toolbar is a layout edit | | [0160](0160-ci-compiles-each-kind-once-and-tests-the-plugin-feature-where-it-changes-code.md) | CI compiles each kind of build once (lint checks, build and tests), tests `user-plugins` only in the packages that declare it, saves caches from `dev` and `master`, and builds installers only for releases | | [0161](0161-user-plugins-ship-in-every-build.md) | User plugins ship in every build: the `user-plugins` cargo feature and the compile-out rule are gone, the `[plugins] user_plugins` switch and `--safe-mode` still decide whether a run loads them (supersedes the compile-out part of 0093) | +| [0162](0162-windows-holds-directory-handles-for-strict-local-capabilities.md) | Windows strict local filesystem capabilities hold every ancestor's handle without delete sharing and refuse reparse points, so new v3 chats save on Windows; a failed save names its class (amends 0110) | +| [0163](0163-windows-agents-start-hidden-with-the-systems-search-path.md) | Windows agents start without a console window, the agent environment matches Windows names without case, and bare commands such as `npx` are also looked for in the usual Node.js locations | diff --git a/docs/architecture/adapter.md b/docs/architecture/adapter.md index 7856d663..db7eedc8 100644 --- a/docs/architecture/adapter.md +++ b/docs/architecture/adapter.md @@ -196,9 +196,12 @@ through operations; lossy strings are presentation only. Read-only browsing may follow directory links and accepts linked parent navigation. Strict traversal, source streams and mutations reject linked ancestors and pin each -Unix directory. Walks and removals have entry/depth limits; cancellation can leave -an explicitly reported partial removal. Portable targets check reparse/link state -but do not claim Unix held-handle race guarantees. Staged publication distinguishes +directory: Unix holds descriptors and uses `*at` calls; Windows holds a handle to every +ancestor without delete sharing and refuses reparse points +([ADR 0162](../adr/0162-windows-holds-directory-handles-for-strict-local-capabilities.md)). +Walks and removals have entry/depth limits; cancellation can leave an explicitly +reported partial removal. Other portable targets check reparse/link state but do not +claim held-handle race guarantees. Staged publication distinguishes failure before commit from a durability warning after successful publication. Successful publication relinquishes its temporary name before returning; Drop cannot remove a later owner of that name. Explicit cancellation confirms removal diff --git a/tests/native_chat_history.rs b/tests/native_chat_history.rs new file mode 100644 index 00000000..ae25565d --- /dev/null +++ b/tests/native_chat_history.rs @@ -0,0 +1,158 @@ +//! New environment-scoped (v3) chats save and reload through the production +//! held-directory filesystem on every native platform, including Windows. +use futures::executor::block_on; +use nocterm_agent_storage::FileChatRepository; +use nocterm_ai::{ + history::{ContextId, SavedChat, SavedPrompt, SharedChat, TranscriptVersion}, + local_environment::{ChatScope, LocalEnvironment}, + storage::{AgentStorageError, ChatRepository}, + thread::Entry, +}; +use nocterm_hubs_model::source_identity::{ + AccountIdentity, AccountScope, Fingerprint, HubIdentity, LocalAccountPartition, NamespaceId, +}; +use std::{ + path::{Path, PathBuf}, + sync::Arc, +}; + +const ID: &str = "00000000-0000-4000-8000-0000000000c3"; + +fn root() -> (tempfile::TempDir, PathBuf) { + let directory = tempfile::tempdir().unwrap(); + let root = std::fs::canonicalize(directory.path()) + .unwrap() + .join("agent-history"); + (directory, root) +} +fn repository(root: &Path) -> FileChatRepository { + FileChatRepository::new( + root.to_owned(), + Arc::new(nocterm_localfs::LocalFilesystem::new().unwrap()), + ) +} +fn account(hub: u8) -> LocalEnvironment { + LocalEnvironment::HubAccount(LocalAccountPartition::Stable(AccountScope::new( + HubIdentity::new( + NamespaceId::new("instance").unwrap(), + Fingerprint::from_bytes([hub; 32]), + ), + AccountIdentity::new( + NamespaceId::new("account").unwrap(), + Fingerprint::from_bytes([7; 32]), + ), + ))) +} +fn project() -> ContextId { + ContextId::Project(nocterm_project_model::ProjectId::from_bytes([5; 16])) +} +/// The shape a new chat document has when its first message is queued. +fn new_chat(scope: ChatScope, title: &str) -> SharedChat { + let mut chat = SavedChat::for_environment("codex".into(), ID.into(), 1, scope); + assert_eq!(chat.version, TranscriptVersion::ENVIRONMENT.wire()); + chat.header.title = Some(title.into()); + chat.entries.push(Entry::Agent(format!("{title} answer"))); + let queued = Arc::new(SavedPrompt { + id: 1, + text: format!("{title} queued"), + images: Vec::new(), + attachments: Vec::new(), + }); + chat.queue = vec![(*queued).clone()]; + SharedChat { + metadata: chat, + queue: vec![queued], + } +} +fn document(root: &Path, scope: &ChatScope) -> PathBuf { + let base = root + .join("scoped-v3") + .join(scope.environment().storage_key()); + let context = match scope.context() { + ContextId::General => base.join("general"), + ContextId::Project(id) => base.join("projects").join(id.to_string()), + }; + context.join(format!("{ID}.json")) +} + +#[test] +fn new_chats_in_every_environment_and_context_save_reload_and_stay_isolated() { + let (_directory, root) = root(); + let scopes = [ + (ChatScope::this_computer(ContextId::General), "device"), + (ChatScope::this_computer(project()), "device project"), + (ChatScope::new(account(1), ContextId::General), "account"), + ( + ChatScope::new(account(2), project()), + "other account project", + ), + ]; + for (scope, title) in &scopes { + block_on(repository(&root).save(Arc::new(new_chat(scope.clone(), title)))).unwrap(); + assert!(document(&root, scope).is_file(), "{title} was not written"); + } + // A fresh repository stands for a restarted application. + let restarted = repository(&root); + for (scope, title) in &scopes { + let rows = block_on(restarted.catalog_in(scope.clone())).unwrap(); + assert_eq!(rows.len(), 1, "{title}"); + let chat = block_on(restarted.load_in(scope.clone(), ID.into())).unwrap(); + assert_eq!(chat.header.title.as_deref(), Some(*title)); + assert_eq!(chat.version, TranscriptVersion::ENVIRONMENT.wire()); + assert_eq!(chat.queue.len(), 1, "{title} lost its queued message"); + assert_eq!(chat.queue[0].text, format!("{title} queued")); + } + assert!(matches!( + block_on(restarted.load_in(ChatScope::new(account(3), ContextId::General), ID.into())), + Err(AgentStorageError::NotFound(_)) + )); + block_on(restarted.delete_in(scopes[2].0.clone(), ID.into())).unwrap(); + assert!(!document(&root, &scopes[2].0).exists()); + for index in [0, 1, 3] { + assert!(document(&root, &scopes[index].0).is_file()); + } +} + +#[test] +fn a_later_snapshot_replaces_the_saved_chat_without_leaving_staging_files() { + let (_directory, root) = root(); + let scope = ChatScope::this_computer(ContextId::General); + let repository = repository(&root); + block_on(repository.save(Arc::new(new_chat(scope.clone(), "first")))).unwrap(); + let mut later = new_chat(scope.clone(), "second"); + later.metadata.queue.clear(); + later.queue.clear(); + block_on(repository.save(Arc::new(later))).unwrap(); + let chat = block_on(repository.load_in(scope.clone(), ID.into())).unwrap(); + assert_eq!(chat.header.title.as_deref(), Some("second")); + assert!(chat.queue.is_empty()); + let names: Vec<_> = std::fs::read_dir(document(&root, &scope).parent().unwrap()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(names, vec![std::ffi::OsString::from(format!("{ID}.json"))]); +} + +#[cfg(windows)] +#[test] +fn a_junctioned_account_partition_is_refused_without_touching_its_target() { + let (_directory, root) = root(); + let outside = tempfile::tempdir().unwrap(); + let scope = ChatScope::new(account(1), ContextId::General); + let partition = root + .join("scoped-v3") + .join(scope.environment().storage_key()); + std::fs::create_dir_all(partition.parent().unwrap()).unwrap(); + let status = std::process::Command::new("cmd") + .arg("/C") + .arg("mklink") + .arg("/J") + .arg(&partition) + .arg(outside.path()) + .status() + .unwrap(); + assert!(status.success()); + let saved = block_on(repository(&root).save(Arc::new(new_chat(scope, "account")))); + assert!(saved.is_err(), "a junctioned partition accepted a write"); + assert_eq!(std::fs::read_dir(outside.path()).unwrap().count(), 0); +}