From eca63c20af4b36408fcd8bc6dfe4e2eb40821c62 Mon Sep 17 00:00:00 2001 From: ADD-SP Date: Sat, 11 Apr 2026 21:33:17 -0700 Subject: [PATCH] fix(uninstall): clean up Hermes skills and cron job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The uninstall function cleaned up OpenClaw skills and cron but had zero Hermes cleanup — skill dirs under ~/.hermes/skills/ were left behind and the clawshell-weekly-stats cron job kept firing (and failing) after uninstall. - Discover Hermes skill dirs for both get-clawshell-stats and get-email-messages via resolve_hermes_target_user, list them in the uninstall preview, and remove with confirmation. - Best-effort `hermes cron remove clawshell-weekly-stats` when any Hermes skills were found. - Remove #[allow(dead_code)] from remove_hermes_stats_cron now that it has a non-test caller. --- src/hermes_cli.rs | 116 ++++++++++++++++++++++++++++++++++++++-------- src/main.rs | 64 +++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 19 deletions(-) diff --git a/src/hermes_cli.rs b/src/hermes_cli.rs index f4d0ad5..7a022c0 100644 --- a/src/hermes_cli.rs +++ b/src/hermes_cli.rs @@ -216,23 +216,63 @@ pub fn setup_hermes_stats_cron( Ok(()) } -#[allow(dead_code)] -pub fn remove_hermes_stats_cron(runner: &mut R) -> Result<(), Box> { - let output = runner - .run(&["cron".into(), "remove".into(), STATS_CRON_JOB_NAME.into()]) - .map_err(|e| format!("failed to run `hermes cron remove`: {e}"))?; - if !output.success { - let status = output - .status_code - .map(|c| c.to_string()) - .unwrap_or_else(|| "unknown".to_string()); - return Err(format!( - "`hermes cron remove` exited with status {status}: {}", - output.stderr.trim() - ) - .into()); +/// Remove the ClawShell stats cron job by reading `~/.hermes/cron/jobs.json`, +/// finding job(s) whose `"name"` matches [`STATS_CRON_JOB_NAME`], and calling +/// `hermes cron remove ` for each. Returns the number of jobs removed. +/// `hermes cron remove` takes a job ID, not a name, so we must resolve the +/// ID from the jobs file first. +pub fn remove_hermes_stats_cron( + runner: &mut R, + home_dir: &Path, +) -> Result> { + let jobs_path = home_dir.join(".hermes").join("cron").join("jobs.json"); + let content = match std::fs::read_to_string(&jobs_path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(0), + Err(e) => return Err(format!("failed to read {}: {e}", jobs_path.display()).into()), + }; + // jobs.json is either a bare array or an object with a "jobs" key. + let parsed: serde_json::Value = serde_json::from_str(&content) + .map_err(|e| format!("failed to parse {}: {e}", jobs_path.display()))?; + let jobs = match &parsed { + serde_json::Value::Array(arr) => arr.as_slice(), + serde_json::Value::Object(obj) => obj + .get("jobs") + .and_then(serde_json::Value::as_array) + .map(Vec::as_slice) + .unwrap_or(&[]), + _ => &[], + }; + + let matching_ids: Vec = jobs + .iter() + .filter(|j| j.get("name").and_then(serde_json::Value::as_str) == Some(STATS_CRON_JOB_NAME)) + .filter_map(|j| { + j.get("id") + .and_then(serde_json::Value::as_str) + .map(String::from) + }) + .collect(); + + let mut removed = 0; + for id in &matching_ids { + let output = runner + .run(&["cron".into(), "remove".into(), id.clone()]) + .map_err(|e| format!("failed to run `hermes cron remove {id}`: {e}"))?; + if !output.success { + let status = output + .status_code + .map(|c| c.to_string()) + .unwrap_or_else(|| "unknown".to_string()); + return Err(format!( + "`hermes cron remove {id}` exited with status {status}: {}", + output.stderr.trim() + ) + .into()); + } + removed += 1; } - Ok(()) + Ok(removed) } #[cfg(test)] @@ -396,10 +436,48 @@ mod tests { } #[test] - fn test_remove_hermes_stats_cron_sends_correct_args() { + fn test_remove_hermes_stats_cron_resolves_id_from_jobs_json() { + let dir = tempfile::tempdir().unwrap(); + let cron_dir = dir.path().join(".hermes").join("cron"); + std::fs::create_dir_all(&cron_dir).unwrap(); + let jobs = serde_json::json!({ + "jobs": [ + {"id": "abc123", "name": STATS_CRON_JOB_NAME, "schedule": "0 9 * * 1"}, + {"id": "other1", "name": "unrelated-job", "schedule": "0 12 * * *"} + ], + "updated_at": "2026-01-01T00:00:00Z" + }); + std::fs::write(cron_dir.join("jobs.json"), jobs.to_string()).unwrap(); + let mut runner = FakeHermesRunner::default(); - remove_hermes_stats_cron(&mut runner).unwrap(); + let removed = remove_hermes_stats_cron(&mut runner, dir.path()).unwrap(); + assert_eq!(removed, 1); assert_eq!(runner.calls.len(), 1); - assert_eq!(runner.calls[0], vec!["cron", "remove", STATS_CRON_JOB_NAME]); + assert_eq!(runner.calls[0], vec!["cron", "remove", "abc123"]); + } + + #[test] + fn test_remove_hermes_stats_cron_no_matching_job() { + let dir = tempfile::tempdir().unwrap(); + let cron_dir = dir.path().join(".hermes").join("cron"); + std::fs::create_dir_all(&cron_dir).unwrap(); + std::fs::write( + cron_dir.join("jobs.json"), + r#"{"jobs": [{"id": "xyz", "name": "other-job"}]}"#, + ) + .unwrap(); + + let mut runner = FakeHermesRunner::default(); + let removed = remove_hermes_stats_cron(&mut runner, dir.path()).unwrap(); + assert_eq!(removed, 0); + assert_eq!(runner.calls.len(), 0); + } + + #[test] + fn test_remove_hermes_stats_cron_missing_jobs_file() { + let dir = tempfile::tempdir().unwrap(); + let mut runner = FakeHermesRunner::default(); + let removed = remove_hermes_stats_cron(&mut runner, dir.path()).unwrap(); + assert_eq!(removed, 0); } } diff --git a/src/main.rs b/src/main.rs index 1ed3808..3a3caf3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1830,6 +1830,30 @@ fn cmd_uninstall(skip_confirm: bool) -> Result<(), Box> { Vec::new() }; + // Hermes skill directories (if the user's ~/.hermes/skills/ exists). + let hermes_home_dir: Option = + resolve_hermes_target_user().ok().map(|(home, _, _)| home); + let hermes_skill_dirs: Vec<(&'static str, PathBuf)> = hermes_home_dir + .as_ref() + .map(|home| { + let skills_root = home.join(".hermes").join("skills"); + [ + onboard::ADMIN_STATS_SKILL_NAME, + onboard::EMAIL_MESSAGES_SKILL_NAME, + ] + .into_iter() + .filter_map(|name| { + let dir = skills_root.join(name); + if dir.exists() { + Some((name, dir)) + } else { + None + } + }) + .collect() + }) + .unwrap_or_default(); + tui::print_warning("This will remove the following:"); tui::print_info("ClawShell", "Stop if running"); tui::print_info("Config dir", &config_dir.display().to_string()); @@ -1871,6 +1895,9 @@ fn cmd_uninstall(skip_confirm: bool) -> Result<(), Box> { onboard::ManagedSkillUninstallState::Missing => {} } } + for (name, dir) in &hermes_skill_dirs { + tui::print_info("Hermes skill", &format!("{} ({})", dir.display(), name)); + } tui::print_info("Binary", &format!("{} (preserved)", exe_path.display())); tui::print_info("System user", "clawshell"); println!(); @@ -2003,6 +2030,43 @@ fn cmd_uninstall(skip_confirm: bool) -> Result<(), Box> { } } + // 0c. Remove ClawShell-managed Hermes skills if present. + for (name, dir) in &hermes_skill_dirs { + let remove = if skip_confirm { + true + } else { + tui::prompt_confirm( + &format!("Remove Hermes skill '{}' at {}?", name, dir.display()), + true, + )? + }; + if remove { + match std::fs::remove_dir_all(dir) { + Ok(()) => tui::print_success(&format!("Hermes skill removed: {}", dir.display())), + Err(e) => tui::print_warning(&format!( + "Failed to remove Hermes skill at {}: {e}", + dir.display() + )), + } + } + } + + // 0d. Remove the Hermes stats cron job (best-effort). We read + // ~/.hermes/cron/jobs.json to resolve the job ID by name, since + // `hermes cron remove` requires an ID, not a name. + if let Some(home) = hermes_home_dir.as_ref() { + let mut runner = hermes_cli::RealHermesRunner; + match hermes_cli::remove_hermes_stats_cron(&mut runner, home) { + Ok(n) if n > 0 => { + tui::print_success(&format!("Hermes stats cron job removed ({n} job(s)).")) + } + Ok(_) => tui::print_info("Hermes cron", "no clawshell-weekly-stats job found"), + Err(err) => { + tui::print_warning(&format!("Failed to remove Hermes stats cron job: {err}")) + } + } + } + // 1. Stop ClawShell and remove auto-start service if service_exists { tui::print_info("Action", "Stopping and removing auto-start service...");