fix: save new chats, find npx and hide agent consoles on Windows

.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

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"] }
 

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;

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<Spawned, AgentError> {
-    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("<unset>", 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("<unset>", 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<String, String>) -> 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<String, String>) -> 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<String, String>, 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<String, String>) -> Option<PathBuf> {
+    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<PathBuf> {
+    #[cfg(windows)]
+    {
+        dunce::canonicalize(path).ok()
+    }
+    #[cfg(not(windows))]
+    {
+        path.canonicalize().ok()
+    }
+}
+
 fn resolve(
     command: &str,
     env: &BTreeMap<String, String>,
@@ -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

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)?;
     }

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<String, String>) -> Vec<PathBuf> {
+    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())
+        );
+    }
+}

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;

crates/agent-chat/src/thread/client.rs

@@ -701,9 +701,7 @@ impl AgentThread {
         error: crate::runtime::AgentPersistenceError,
         cx: &mut Context<Self>,
     ) {
-        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;
         }

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"));
+        }
+    }
+}

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 {

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<dyn AgentStateRepository>,