From d911ce5d927c1b94f2961358af77c62990a4ccfa Mon Sep 17 00:00:00 2001 From: Bruno Charest Date: Wed, 26 Aug 2026 10:17:17 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20robustesse=20v1.1.6=20=E2=80=94=20timeou?= =?UTF-8?q?t=20runner/doctor,=20anti-deadlock,=20securite=20migrate,=20win?= =?UTF-8?q?32=20is=5Frunning,=20websocket=20RFC6455,=20keyring=20JSON?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runner.rs: timeout 30s sur les commandes capturees (doctor ne pend plus) + pipes stdout/stderr draines dans des threads (anti-deadlock) ; run_untimed() pour les installations longues (npm/cargo/go) — sh() des installateurs l'utilise, les sondes gardent le timeout - migrate_cmd.rs: rejet des chemins unsafe (path traversal) dans import_bundle - process.rs: is_running Windows via OpenProcess/GetExitCodeProcess (fiable vs tasklist) - probe.rs: drain des pipes en threads dans detect_external_version - web.rs: boucle de traduction i18n (curseur, plus de boucle infinie si la traduction contient {{) - serve.rs: frames WebSocket RFC 6455 avec vraies longueurs > 125 octets (126/127) - secrets.rs: index keyring serialise en JSON (cles contenant des virgules) - sandbox.rs: tests sans fuite de TempDir (plus de mem::forget) - version 1.1.6, man pages et ROADMAP maj --- Cargo.lock | 2 +- Cargo.toml | 2 +- ROADMAP.md | 10 +++ man/am.1 | 4 +- src/commands/migrate_cmd.rs | 6 +- src/installers/mod.rs | 4 +- src/probe.rs | 37 +++++++---- src/process.rs | 21 +++--- src/runner.rs | 125 ++++++++++++++++++++++++++++++++++-- src/sandbox.rs | 17 +++-- src/secrets.rs | 16 ++++- src/serve.rs | 13 +++- src/web.rs | 5 +- 13 files changed, 212 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 45c1bc4..89d1abc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -21,7 +21,7 @@ dependencies = [ [[package]] name = "agent-manager" -version = "1.1.5" +version = "1.1.6" dependencies = [ "anyhow", "base64", diff --git a/Cargo.toml b/Cargo.toml index 80b37d1..a0a1a23 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "agent-manager" -version = "1.1.5" +version = "1.1.6" edition = "2021" description = "Manage local AI coding agents: list, install, start, stop, update — with automatic dependency handling and a YAML-driven catalog." license = "MIT" diff --git a/ROADMAP.md b/ROADMAP.md index fa1f136..0525e8d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -119,6 +119,16 @@ > — aichat ne filtre rien et échoue sur `.git/index`. Génération `--exec` : > `--code` au lieu de `--execute --dry-run` (aichat ≥ 0.30 : `--dry-run` > n'appelle plus l'API, il écho la requête). +> +> 🔧 **Maintenance v1.1.6 (2026-08-24)** : correctifs de robustesse — +> timeout de 30 s sur les commandes capturées (doctor/runner ne pendent plus +> si un agent ne répond pas, pipes vidés dans des threads pour éviter les +> deadlocks), `is_running` Windows via l'API Win32 (OpenProcess / +> GetExitCodeProcess, plus fiable que tasklist), rejet des chemins unsafe +> (path traversal) dans `am migrate --import`, frames WebSocket RFC 6455 +> avec vraies longueurs > 125 octets, boucle de traduction i18n de la page +> web corrigée (plus de boucle infinie), index du trousseau (keyring) +> sérialisé en JSON (clés contenant des virgules). --- diff --git a/man/am.1 b/man/am.1 index 6578256..eb200a6 100644 --- a/man/am.1 +++ b/man/am.1 @@ -1,6 +1,6 @@ .ie \n(.g .ds Aq \(aq .el .ds Aq ' -.TH am 1 "am 1.1.5" +.TH am 1 "am 1.1.6" .SH NAME am \- agent\-manager (am) — manage local AI coding agents .SH SYNOPSIS @@ -250,4 +250,4 @@ Export the configuration and installation state (backup) am\-import(1) Import a previously exported configuration and state .SH VERSION -v1.1.5 +v1.1.6 diff --git a/src/commands/migrate_cmd.rs b/src/commands/migrate_cmd.rs index 0d7a704..f46f7b3 100644 --- a/src/commands/migrate_cmd.rs +++ b/src/commands/migrate_cmd.rs @@ -277,7 +277,11 @@ pub fn import_bundle(app: &App, bundle: &Path, confirm: bool) -> Result, cwd: Option<&Path>, ) -> Result { - let out = runner.run(cmd, args, env, cwd, true)?; + // Installs are legitimately long (npm/cargo/go/... can take minutes): + // no probe timeout here, only run() (doctor/version probes) is bounded. + let out = runner.run_untimed(cmd, args, env, cwd, true)?; if out.code != 0 { let mut detail = out.stderr.trim().to_string(); if detail.is_empty() { diff --git a/src/probe.rs b/src/probe.rs index 92ea656..59aebef 100644 --- a/src/probe.rs +++ b/src/probe.rs @@ -30,7 +30,6 @@ impl ExternalInfo { /// Detect the version of an external binary (--version), bounded by a /// timeout so a misbehaving agent can never hang the whole listing. pub fn detect_external_version(bin_path: &Path) -> Option { - use std::io::Read; let (prog, prefix) = crate::runner::resolve_program(&bin_path.display().to_string()); let mut cmd = std::process::Command::new(&prog); cmd.args(&prefix) @@ -41,6 +40,24 @@ pub fn detect_external_version(bin_path: &Path) -> Option { let Ok(mut child) = cmd.spawn() else { return None; }; + let stdout_pipe = child.stdout.take(); + let stderr_pipe = child.stderr.take(); + + let stdout_handle = std::thread::spawn(move || { + let mut out = String::new(); + if let Some(mut pipe) = stdout_pipe { + let _ = std::io::Read::read_to_string(&mut pipe, &mut out); + } + out + }); + let stderr_handle = std::thread::spawn(move || { + let mut err = String::new(); + if let Some(mut pipe) = stderr_pipe { + let _ = std::io::Read::read_to_string(&mut pipe, &mut err); + } + err + }); + const PROBE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(4); let status: Option = match child.wait_timeout(PROBE_TIMEOUT) { Ok(Some(s)) => Some(s), @@ -51,22 +68,14 @@ pub fn detect_external_version(bin_path: &Path) -> Option { } Err(_) => None, }; - let Some(status) = status else { - return None; - }; + let stdout = stdout_handle.join().unwrap_or_default(); + let stderr = stderr_handle.join().unwrap_or_default(); + + let status = status?; if !status.success() { return None; } - let mut stdout = String::new(); - let mut stderr = String::new(); - if let Some(mut o) = child.stdout.take() { - let _ = o.read_to_string(&mut stdout); - } - if let Some(mut e) = child.stderr.take() { - let _ = e.read_to_string(&mut stderr); - } - let combined = format!("{stdout} -{stderr}"); + let combined = format!("{stdout}\n{stderr}"); crate::version::find_version(&combined) } diff --git a/src/process.rs b/src/process.rs index e28db38..3071357 100644 --- a/src/process.rs +++ b/src/process.rs @@ -17,14 +17,19 @@ pub fn agent_log_path(app: &App, agent_name: &str) -> PathBuf { pub fn is_running(pid: u32) -> bool { #[cfg(windows)] { - let out = Command::new("tasklist") - .args(["/FI", &format!("PID eq {pid}"), "/NH"]) - .output(); - match out { - Ok(o) => String::from_utf8_lossy(&o.stdout) - .split_whitespace() - .any(|w| w == pid.to_string()), - Err(_) => false, + use windows_sys::Win32::Foundation::{CloseHandle, FALSE, STILL_ACTIVE}; + use windows_sys::Win32::System::Threading::{ + GetExitCodeProcess, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, + }; + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, pid); + if handle == 0 { + return false; + } + let mut code: u32 = 0; + let ok = GetExitCodeProcess(handle, &mut code); + CloseHandle(handle); + ok != 0 && code == STILL_ACTIVE as u32 } } #[cfg(not(windows))] diff --git a/src/runner.rs b/src/runner.rs index a9b84de..b3a4900 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -52,6 +52,20 @@ pub trait Runner { capture: bool, ) -> Result; + /// Run without the probe timeout: used for legitimately long + /// operations (package installs, git clones, ...). The default + /// implementation delegates to run(). + fn run_untimed( + &self, + cmd: &str, + args: &[String], + env: &BTreeMap, + cwd: Option<&Path>, + capture: bool, + ) -> Result { + self.run(cmd, args, env, cwd, capture) + } + /// Locate a binary on the PATH (like the which command). fn which(&self, bin: &str) -> Option; } @@ -73,14 +87,17 @@ impl<'a> SystemRunner<'a> { } } -impl<'a> Runner for SystemRunner<'a> { - fn run( +impl<'a> SystemRunner<'a> { + /// Shared implementation. The timeout applies only to captured + /// probes; long-running installs pass None via run_untimed(). + fn run_inner( &self, cmd: &str, args: &[String], env: &BTreeMap, cwd: Option<&Path>, capture: bool, + timeout: Option, ) -> Result { let display = if args.is_empty() { cmd.to_string() @@ -102,12 +119,70 @@ impl<'a> Runner for SystemRunner<'a> { c.current_dir(dir); } if capture { - let out = c - .output() + c.stdout(std::process::Stdio::piped()); + c.stderr(std::process::Stdio::piped()); + let mut child = c + .spawn() .with_context(|| format!("failed to execute: {display}"))?; - let code = out.status.code().unwrap_or(-1); - let stdout = String::from_utf8_lossy(&out.stdout).to_string(); - let stderr = String::from_utf8_lossy(&out.stderr).to_string(); + + let stdout_pipe = child.stdout.take(); + let stderr_pipe = child.stderr.take(); + + let stdout_handle = std::thread::spawn(move || { + let mut out = Vec::new(); + if let Some(mut pipe) = stdout_pipe { + let _ = std::io::Read::read_to_end(&mut pipe, &mut out); + } + out + }); + let stderr_handle = std::thread::spawn(move || { + let mut err = Vec::new(); + if let Some(mut pipe) = stderr_pipe { + let _ = std::io::Read::read_to_end(&mut pipe, &mut err); + } + err + }); + + let status = if let Some(timeout) = timeout { + use wait_timeout::ChildExt; + match child.wait_timeout(timeout) { + Ok(Some(s)) => s, + Ok(None) => { + let _ = child.kill(); + let _ = child.wait(); + self.log + .warn(format!("command timed out after {timeout:?}: {display}")); + let stdout = String::from_utf8_lossy(&stdout_handle.join().unwrap_or_default()) + .to_string(); + let stderr = String::from_utf8_lossy(&stderr_handle.join().unwrap_or_default()) + .to_string(); + return Ok(RunOutput { + code: -1, + stdout, + stderr, + }); + } + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + return Ok(RunOutput { + code: -1, + stdout: String::new(), + stderr: String::new(), + }); + } + } + } else { + child + .wait() + .with_context(|| format!("failed to wait: {display}"))? + }; + + let stdout_bytes = stdout_handle.join().unwrap_or_default(); + let stderr_bytes = stderr_handle.join().unwrap_or_default(); + let code = status.code().unwrap_or(-1); + let stdout = String::from_utf8_lossy(&stdout_bytes).to_string(); + let stderr = String::from_utf8_lossy(&stderr_bytes).to_string(); self.log.result(code, &stderr); Ok(RunOutput { code, @@ -126,6 +201,42 @@ impl<'a> Runner for SystemRunner<'a> { } } +} + +impl<'a> Runner for SystemRunner<'a> { + /// Probes are bounded by a 30 s timeout so a misbehaving process can + /// never hang doctor/version detection. + fn run( + &self, + cmd: &str, + args: &[String], + env: &BTreeMap, + cwd: Option<&Path>, + capture: bool, + ) -> Result { + self.run_inner( + cmd, + args, + env, + cwd, + capture, + Some(std::time::Duration::from_secs(30)), + ) + } + + /// Installs and other legitimately long operations run without the + /// probe timeout (npm/cargo/go installs can take minutes). + fn run_untimed( + &self, + cmd: &str, + args: &[String], + env: &BTreeMap, + cwd: Option<&Path>, + capture: bool, + ) -> Result { + self.run_inner(cmd, args, env, cwd, capture, None) + } + fn which(&self, bin: &str) -> Option { which::which(bin).ok() } diff --git a/src/sandbox.rs b/src/sandbox.rs index 748af36..f10c683 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -103,10 +103,9 @@ mod tests { use super::*; use clap::Parser; - fn test_app(sandbox_yaml: &str) -> App { + fn test_app(sandbox_yaml: &str) -> (tempfile::TempDir, App) { let guard = tempfile::tempdir().unwrap(); let dir = guard.path().to_path_buf(); - std::mem::forget(guard); let cfg = dir.join("config.yaml"); std::fs::write( &cfg, @@ -116,7 +115,7 @@ mod tests { ) .unwrap(); let cli = crate::cli::Cli::parse_from(["am", "--config", cfg.to_str().unwrap()]); - crate::app::App::from_cli(cli).unwrap() + (guard, crate::app::App::from_cli(cli).unwrap()) } fn agent(app: &App) -> &AgentDef { @@ -125,7 +124,7 @@ mod tests { #[test] fn no_profile_means_unsandboxed() { - let app = test_app(""); + let (_guard, app) = test_app(""); assert!(profile_of(agent(&app)).is_none()); let mut env = BTreeMap::new(); // No profile: everything allowed, nothing refused. @@ -135,7 +134,7 @@ mod tests { #[test] fn disallowed_command_is_refused_and_journalized() { - let app = test_app( + let (_guard, app) = test_app( "sandbox:\n enabled: true\n commands: [python]\n dirs: []\n network: true\n", ); let mut env = BTreeMap::new(); @@ -161,7 +160,7 @@ mod tests { #[test] fn allowed_command_and_cwd_pass() { let cwd = std::env::current_dir().unwrap(); - let app = test_app(&format!( + let (_guard, app) = test_app(&format!( "sandbox:\n enabled: true\n commands: [demo.exe]\n dirs: [\"{}\"]\n network: true\n", cwd.display().to_string().replace('\\', "\\\\") )); @@ -172,7 +171,7 @@ mod tests { #[test] fn cwd_outside_perimeter_is_blocked() { - let app = test_app( + let (_guard, app) = test_app( "sandbox:\n enabled: true\n commands: []\n dirs: [\"C:/definitely/not/here\"]\n network: true\n", ); let mut env = BTreeMap::new(); @@ -182,7 +181,7 @@ mod tests { #[test] fn network_off_injects_blocking_proxy_env() { - let app = test_app( + let (_guard, app) = test_app( "sandbox:\n enabled: true\n commands: []\n dirs: []\n network: false\n", ); let mut env = BTreeMap::new(); @@ -194,7 +193,7 @@ mod tests { #[test] fn no_sandbox_flag_bypasses_the_profile() { - let app = test_app( + let (_guard, app) = test_app( "sandbox:\n enabled: true\n commands: [python]\n dirs: []\n network: true\n", ); let mut env = BTreeMap::new(); diff --git a/src/secrets.rs b/src/secrets.rs index 8466b0b..0ecc9d9 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -25,8 +25,9 @@ impl SecretStore for KeyringStore { if !idx.iter().any(|k| k == key) { idx.push(key.to_string()); } + let json_idx = serde_json::to_string(&idx).unwrap_or_else(|_| idx.join("\0")); let index = keyring::Entry::new(SERVICE, INDEX_KEY)?; - let _ = index.set_password(&idx.join(",")); + let _ = index.set_password(&json_idx); Ok(()) } @@ -51,8 +52,9 @@ impl SecretStore for KeyringStore { let mut idx = self.list().unwrap_or_default(); if idx.iter().any(|k| k == key) { idx.retain(|k| k != key); + let json_idx = serde_json::to_string(&idx).unwrap_or_else(|_| idx.join("\0")); let index = keyring::Entry::new(SERVICE, INDEX_KEY)?; - let _ = index.set_password(&idx.join(",")); + let _ = index.set_password(&json_idx); } Ok(existed) } @@ -60,7 +62,15 @@ impl SecretStore for KeyringStore { fn list(&self) -> Result> { let index = keyring::Entry::new(SERVICE, INDEX_KEY)?; match index.get_password() { - Ok(v) => Ok(v.split(',').filter(|s| !s.is_empty()).map(String::from).collect()), + Ok(v) => { + if let Ok(vec) = serde_json::from_str::>(&v) { + Ok(vec) + } else if v.contains('\0') { + Ok(v.split('\0').filter(|s| !s.is_empty()).map(String::from).collect()) + } else { + Ok(v.split(',').filter(|s| !s.is_empty()).map(String::from).collect()) + } + } Err(keyring::Error::NoEntry) => Ok(Vec::new()), Err(e) => Err(anyhow!("keyring: {e}")), } diff --git a/src/serve.rs b/src/serve.rs index 06e27de..59cc019 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -370,9 +370,18 @@ fn ws_accept(key: &str) -> String { base64::engine::general_purpose::STANDARD.encode(hasher.finalize()) } -/// Write a single server frame (unmasked): opcode + 7-bit length + payload. +/// Write a single server frame (unmasked): opcode + RFC 6455 length + payload. fn write_frame(w: &mut W, opcode: u8, payload: &[u8]) -> Result<()> { - w.write_all(&[0x80 | opcode, payload.len() as u8])?; + let len = payload.len(); + if len <= 125 { + w.write_all(&[0x80 | opcode, len as u8])?; + } else if len <= 65535 { + w.write_all(&[0x80 | opcode, 126])?; + w.write_all(&(len as u16).to_be_bytes())?; + } else { + w.write_all(&[0x80 | opcode, 127])?; + w.write_all(&(len as u64).to_be_bytes())?; + } w.write_all(payload)?; w.flush()?; Ok(()) diff --git a/src/web.rs b/src/web.rs index dcefc60..6e97d7b 100644 --- a/src/web.rs +++ b/src/web.rs @@ -134,7 +134,9 @@ fn index_page(app: &App) -> HttpResult { let mut html = INDEX_HTML.replace("{{i18n_bundle}}", &bundle); // Then every {{label}} token: replaced by the localized label (the // French text is both the key and the fallback, so nothing survives). - while let Some(start) = html.find("{{") { + let mut cursor = 0; + while let Some(rel_start) = html[cursor..].find("{{") { + let start = cursor + rel_start; let Some(rel_end) = html[start + 2..].find("}}") else { break; }; @@ -142,6 +144,7 @@ fn index_page(app: &App) -> HttpResult { let key = html[start + 2..end].to_string(); let translated = crate::i18n::tr_in(lang, &key); html.replace_range(start..end + 2, translated); + cursor = start + translated.len(); } HttpResult { status: 200,