From 0f627fdc91295cde83bebd1d8f8fdc094f7e6080 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:21 +0000 Subject: [PATCH 1/6] fix(localfs): hold directory handles for strict capabilities on Windows New v3 chats need held directory capabilities, which Windows never supplied: every save of a new chat was refused and its queue paused. A strict capability now keeps a handle to every ancestor, opened without delete sharing and as a reparse point, so no held directory can be renamed or replaced by a junction while it lives; its identity comes from the handle's volume serial and file index. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- Cargo.lock | 1 + THIRD_PARTY_NOTICES.txt | 6 +- crates/localfs/Cargo.toml | 1 + crates/localfs/src/directory.rs | 23 +++- crates/localfs/src/directory/portable.rs | 6 + crates/localfs/src/directory/windows.rs | 108 +++++++++++++++ crates/localfs/src/lib.rs | 5 +- crates/localfs/src/mutations.rs | 4 +- crates/localfs/tests/capabilities.rs | 4 +- crates/localfs/tests/windows_held.rs | 168 +++++++++++++++++++++++ 10 files changed, 315 insertions(+), 11 deletions(-) create mode 100644 crates/localfs/src/directory/windows.rs create mode 100644 crates/localfs/tests/windows_held.rs diff --git a/Cargo.lock b/Cargo.lock index 7f345ea8..3b79e9db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5754,6 +5754,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/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(); +} From 6d4be08992a00338c09d8ee926f10c991b3e9753 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:34 +0000 Subject: [PATCH 2/6] fix(agent-chat): say why a chat could not be saved Every failed save advised checking disk space, including a missing storage capability. The runtime now classifies the typed cause chain as unsupported storage, a full volume, refused access or another failure, and only a full volume suggests freeing space. Queued messages stay queued in every case. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- crates/agent-chat/src/thread.rs | 1 + crates/agent-chat/src/thread/client.rs | 4 +- crates/agent-chat/src/thread/save_failure.rs | 54 ++++++++ crates/agent-runtime/src/lib.rs | 3 +- crates/agent-runtime/src/storage.rs | 2 + crates/agent-runtime/src/storage/failure.rs | 123 +++++++++++++++++++ crates/ai/src/storage.rs | 8 +- 7 files changed, 189 insertions(+), 6 deletions(-) create mode 100644 crates/agent-chat/src/thread/save_failure.rs create mode 100644 crates/agent-runtime/src/storage/failure.rs 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/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), From fbac72b5fc2827500395a816e114c8a440d361c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:34 +0000 Subject: [PATCH 3/6] fix(acp): find npx and start agents without a console window on Windows The agent environment allow-list compared names case-sensitively, so Windows' `Path` was dropped and `npx` was never found without an absolute path. Names now match without case on Windows and get one spelling, and bare commands also search the usual Node.js locations. Agents and taskkill start with CREATE_NO_WINDOW, so a GUI build no longer opens console windows when a chat starts or stops an agent. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- Cargo.lock | 1 + crates/acp/Cargo.toml | 3 + crates/acp/src/lib.rs | 2 + crates/acp/src/process.rs | 112 +++++++++++---- crates/acp/src/process/lifecycle.rs | 2 + crates/acp/src/process/locations.rs | 55 +++++++ crates/ai/src/env.rs | 215 ++++++++++++++++++++++------ 7 files changed, 323 insertions(+), 67 deletions(-) create mode 100644 crates/acp/src/process/locations.rs diff --git a/Cargo.lock b/Cargo.lock index 3b79e9db..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", 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/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")); + } +} From 9a09aab5c64d3c1f67a3b3354f8de67992b73838 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:34 +0000 Subject: [PATCH 4/6] test(app): save and reload new chats through the native filesystem Runs the chat repository on the production LocalFilesystem for every environment and context, including restart and queue recovery, and refuses a junctioned account partition on Windows. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- crates/agent-storage/src/state.rs | 1 + tests/native_chat_history.rs | 158 ++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 tests/native_chat_history.rs 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/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); +} From 270d609d3fbb2e893dd933f43d8ac5110ce414fc Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:34 +0000 Subject: [PATCH 5/6] ci: test held directories, chat history and agent launch on Windows Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- .github/workflows/ci.yml | 8 ++++++++ 1 file changed, 8 insertions(+) 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 From 3b85ee3136dcddcf35fe016664663411ff51ac82 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Nov 2026 19:56:34 +0000 Subject: [PATCH 6/6] docs: record Windows held directories and hidden agent launch ADR 0162 amends 0110: Windows supplies held directories for v3 chat history. ADR 0163 covers the agent environment, Node.js fallback locations and windowless start. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WAgRyZmbmoaRukKppczMRc --- docs/AI_AGENTS.md | 7 ++- ...y-handles-for-strict-local-capabilities.md | 46 +++++++++++++++++++ ...art-hidden-with-the-systems-search-path.md | 37 +++++++++++++++ docs/adr/README.md | 2 + docs/architecture/adapter.md | 9 ++-- 5 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 docs/adr/0162-windows-holds-directory-handles-for-strict-local-capabilities.md create mode 100644 docs/adr/0163-windows-agents-start-hidden-with-the-systems-search-path.md 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