diff --git a/crates/maestro-daemon/src/cli.rs b/crates/maestro-daemon/src/cli.rs index d5bcf3c..3ac374d 100644 --- a/crates/maestro-daemon/src/cli.rs +++ b/crates/maestro-daemon/src/cli.rs @@ -55,6 +55,9 @@ async fn run(config_override: Option) -> Result<(), Box> { crate::hooks::AutoInstallOutcome::Installed => { tracing::info!("installed claude-code activity hook (#106)"); } + crate::hooks::AutoInstallOutcome::Upgraded => { + tracing::info!("updated claude-code activity hook to this build (#106)"); + } crate::hooks::AutoInstallOutcome::Failed => { tracing::warn!("hooks auto-install failed; run `maestro hooks install` manually"); } diff --git a/crates/maestro-daemon/src/hooks.rs b/crates/maestro-daemon/src/hooks.rs index 1eba19b..e1dc4d8 100644 --- a/crates/maestro-daemon/src/hooks.rs +++ b/crates/maestro-daemon/src/hooks.rs @@ -1,26 +1,34 @@ //! First-start hook auto-install (#106). //! -//! The daemon runs [`auto_install`] once at startup: when the user's -//! `~/.claude/settings.json` carries no maestro hook yet (and -//! `[hooks].auto_install` hasn't opted out — checked by the caller), the same -//! installer `maestro hooks install` uses writes it. Best-effort: a failure is -//! warned about and never blocks startup. The same global hook serves every -//! Claude Code session on the machine, so nothing here targets repositories -//! or worktrees. +//! The daemon runs [`auto_install`] once at startup: unless the user's +//! `~/.claude/settings.json` already carries exactly what this build ships +//! (and `[hooks].auto_install` hasn't opted out — checked by the caller), the +//! same installer `maestro hooks install` uses writes it. That covers +//! upgrades as well as first starts: an install from a maestro that had fewer +//! events — #131 added `UserPromptSubmit` to a hook #106 had already put in +//! everyone's settings — is rewritten rather than left half-wired. Best-effort: +//! a failure is warned about and never blocks startup. The same global hook +//! serves every Claude Code session on the machine, so nothing here targets +//! repositories or worktrees. use std::path::Path; -/// Install the Claude Code activity hook for `home_dir` unless one is already -/// present or the home dir is unresolvable (`""`). Returns what happened. +/// Install the Claude Code activity hook for `home_dir` unless the current +/// one is already there or the home dir is unresolvable (`""`). Returns what +/// happened; an older install being brought up to date is +/// [`AutoInstallOutcome::Upgraded`], so the log can say so. pub fn auto_install(home_dir: &str) -> AutoInstallOutcome { if home_dir.is_empty() { return AutoInstallOutcome::Skipped; } let settings = maestro_hooks::settings_path(Path::new(home_dir)); - if maestro_hooks::is_installed(&settings) { + let script = maestro_hooks::script_path(Path::new(home_dir)); + if maestro_hooks::is_current(&settings, &script) { return AutoInstallOutcome::AlreadyPresent; } + let stale = maestro_hooks::is_installed(&settings); match maestro_hooks::install_for_home(Path::new(home_dir)) { + Ok(_) if stale => AutoInstallOutcome::Upgraded, Ok(_) => AutoInstallOutcome::Installed, Err(e) => { tracing::warn!(error = %e, "hooks auto-install failed (non-fatal)"); @@ -33,6 +41,8 @@ pub fn auto_install(home_dir: &str) -> AutoInstallOutcome { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AutoInstallOutcome { Installed, + /// An older maestro's hook was there and has been rewritten. + Upgraded, AlreadyPresent, Failed, Skipped, @@ -58,7 +68,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let home = dir.path().to_str().unwrap(); auto_install(home); - // A foreign edit between starts survives; presence alone skips. + // A current install is left exactly as it is. let settings = dir.path().join(".claude/settings.json"); let before = std::fs::read_to_string(&settings).unwrap(); assert_eq!(auto_install(home), AutoInstallOutcome::AlreadyPresent); @@ -69,6 +79,27 @@ mod tests { ); } + /// An install from an older maestro — present, but missing an event this + /// build ships — is upgraded rather than left alone (#131 shipped a new + /// event after #106 had already installed itself). + #[test] + fn a_stale_install_is_upgraded() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path().to_str().unwrap(); + auto_install(home); + let settings = dir.path().join(".claude/settings.json"); + let script = maestro_hooks::script_path(dir.path()); + + let mut root: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&settings).unwrap()).unwrap(); + root["hooks"].as_object_mut().unwrap().remove("Stop"); + std::fs::write(&settings, serde_json::to_string_pretty(&root).unwrap()).unwrap(); + std::fs::write(&script, "#!/bin/sh\nexit 0\n").unwrap(); + + assert_eq!(auto_install(home), AutoInstallOutcome::Upgraded); + assert!(maestro_hooks::is_current(&settings, &script)); + } + #[test] fn unresolvable_home_is_skipped() { assert_eq!(auto_install(""), AutoInstallOutcome::Skipped); diff --git a/crates/maestro-hooks/src/lib.rs b/crates/maestro-hooks/src/lib.rs index d801119..be1a216 100644 --- a/crates/maestro-hooks/src/lib.rs +++ b/crates/maestro-hooks/src/lib.rs @@ -120,13 +120,75 @@ pub fn settings_path(home: &Path) -> std::path::PathBuf { home.join(".claude/settings.json") } -/// Whether `settings_path` already carries a maestro hook (#106). +/// Whether `settings_path` already carries a maestro hook (#106). Presence +/// only: an install from an older release answers `true` even when it is +/// missing events this build ships — see [`is_current`]. pub fn is_installed(settings_path: &Path) -> bool { settings_json(settings_path) .map(|root| find_maestro_commands(&root).next().is_some()) .unwrap_or(false) } +/// Whether what is on disk is what this build ships: the script at +/// `script_path` byte-for-byte, and exactly one settings entry per event in +/// [`HOOK_EVENTS`] and [`PROMPT_EVENTS`], pointing at that script. +/// +/// This is the upgrade question. A maestro that gains an event — #131 added +/// `UserPromptSubmit` long after #106 had installed itself — leaves every +/// existing user with a present but incomplete install, which +/// [`is_installed`] cannot tell apart from a current one. +pub fn is_current(settings_path: &Path, script_path: &Path) -> bool { + if std::fs::read_to_string(script_path).ok().as_deref() != Some(HOOK_SCRIPT) { + return false; + } + settings_json(settings_path) + .map(|root| maestro_entries(&root) == expected_entries(script_path)) + .unwrap_or(false) +} + +/// Every maestro entry in the file as `(event, command)`, sorted — so a +/// duplicate, a stale event or a wrong script path all show up as a +/// difference against [`expected_entries`]. +fn maestro_entries(root: &serde_json::Value) -> Vec<(String, String)> { + let mut found: Vec<(String, String)> = root + .get("hooks") + .and_then(|h| h.as_object()) + .into_iter() + .flatten() + .flat_map(|(event, groups)| { + groups + .as_array() + .into_iter() + .flatten() + .filter_map(|group| group.get("hooks")) + .filter_map(|inner| inner.as_array()) + .flatten() + .filter_map(|hook| hook.get("command")) + .filter_map(|cmd| cmd.as_str()) + .filter(|cmd| cmd.contains(HOOK_MARKER)) + .map(move |cmd| (event.clone(), cmd.to_string())) + }) + .collect(); + found.sort(); + found +} + +/// What [`install`] writes for `script_path`, in the same shape. +fn expected_entries(script_path: &Path) -> Vec<(String, String)> { + let mut want: Vec<(String, String)> = HOOK_EVENTS + .iter() + .chain(PROMPT_EVENTS.iter()) + .map(|(event, state)| { + ( + (*event).to_string(), + format!("{} {state}", script_path.display()), + ) + }) + .collect(); + want.sort(); + want +} + /// Write [`HOOK_SCRIPT`] to `path`, executable. Parent directories are /// created as needed — this is always under the caller's chosen data dir, /// never inside a repository. @@ -151,11 +213,12 @@ pub fn write_script(path: &Path) -> Result<(), HooksError> { pub fn install_for_home(home: &Path) -> Result { let script = script_path(home); let settings = settings_path(home); - let before = std::fs::read_to_string(&settings).unwrap_or_default(); + // A script left over from an older release counts as a change even when + // the settings entries pointing at it are already right. + let changed = !is_current(&settings, &script); write_script(&script)?; install(&settings, &script)?; - let after = std::fs::read_to_string(&settings).unwrap_or_default(); - Ok(before != after) + Ok(changed) } /// Register the hook command for every event in [`HOOK_EVENTS`] in the diff --git a/crates/maestro-hooks/src/tests.rs b/crates/maestro-hooks/src/tests.rs index d597fbb..7e689c3 100644 --- a/crates/maestro-hooks/src/tests.rs +++ b/crates/maestro-hooks/src/tests.rs @@ -428,3 +428,59 @@ fn the_script_posts_a_prompt_payload_unchanged() { "{argv}" ); } + +/// `is_current` answers the upgrade question `is_installed` cannot: an +/// install left over from an older maestro is present, but not what this +/// build ships (#131 — the prompt event arrived after #106 was installed). +#[test] +fn is_current_rejects_an_install_missing_an_event() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + install_for_home(home).unwrap(); + assert!(is_current(&settings_path(home), &script_path(home))); + + // Drop one event, as an older install would have left the file. + let mut root: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(settings_path(home)).unwrap()).unwrap(); + root["hooks"].as_object_mut().unwrap().remove("Stop"); + std::fs::write( + settings_path(home), + serde_json::to_string_pretty(&root).unwrap(), + ) + .unwrap(); + + assert!(is_installed(&settings_path(home))); + assert!(!is_current(&settings_path(home), &script_path(home))); +} + +/// A settings file that is complete but points at a script from an older +/// release is not current either — the entries are right, the script is not. +#[test] +fn is_current_rejects_a_stale_script() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + install_for_home(home).unwrap(); + std::fs::write(script_path(home), "#!/bin/sh\nexit 0\n").unwrap(); + assert!(!is_current(&settings_path(home), &script_path(home))); +} + +/// Nothing installed at all is not current. +#[test] +fn is_current_rejects_a_missing_install() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + assert!(!is_current(&settings_path(home), &script_path(home))); +} + +/// `install_for_home` reports a stale script as a change, even when the +/// settings entries pointing at it already read correctly. +#[test] +fn install_for_home_reports_a_stale_script_as_a_change() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + assert!(install_for_home(home).unwrap()); + assert!(!install_for_home(home).unwrap()); + + std::fs::write(script_path(home), "#!/bin/sh\nexit 0\n").unwrap(); + assert!(install_for_home(home).unwrap()); +}