From 706942ddd7ad48c0be88e19cbac4f5c9a01742d3 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Fri, 4 Sep 2026 10:44:14 +0100 Subject: [PATCH] feat(maestro): install activity hooks for codex too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hook installer only ever wrote ~/.claude/settings.json, so a codex session reported no activity and never posted its first prompt — leaving it on the daemon's generated handle with no haiku name. Adds a Target enum (claude-code, codex) over the hook file's location and event set. One script and one marker serve both; codex swaps Notification for PermissionRequest and has no StopFailure, so it never reports Error. Both agents spell the prompt event UserPromptSubmit and put the text in the payload's `prompt`, so autoname needs no change. Codex will not run a hook it has not been told to trust; `maestro hooks install` says so, and the approval stays the operator's. Co-Authored-By: Claude Opus 5 (1M context) --- crates/maestro-cli/src/dispatch.rs | 34 +++-- crates/maestro-daemon/src/cli.rs | 26 ++-- crates/maestro-daemon/src/hooks.rs | 144 +++++++++++++++------ crates/maestro-hooks/src/lib.rs | 197 +++++++++++++++++++++-------- crates/maestro-hooks/src/tests.rs | 168 +++++++++++++++++++----- docs/SIDEBAR.md | 4 + 6 files changed, 428 insertions(+), 145 deletions(-) diff --git a/crates/maestro-cli/src/dispatch.rs b/crates/maestro-cli/src/dispatch.rs index 4b689ee..26f9084 100644 --- a/crates/maestro-cli/src/dispatch.rs +++ b/crates/maestro-cli/src/dispatch.rs @@ -96,20 +96,32 @@ pub async fn dispatch( } /// `maestro hooks install` (#106): write the shipped hook script under the -/// user's data dir and register it in their Claude Code settings. Idempotent -/// and foreign-hook-preserving — the guarantees live in [`maestro_hooks`]. +/// user's data dir and register it in every agent's hook file. Idempotent and +/// foreign-hook-preserving — the guarantees live in [`maestro_hooks`]. One +/// agent failing is reported and does not stop the others. #[allow(clippy::result_large_err)] fn run_hooks_install() -> Result<(), CliError> { let home = dirs::home_dir().ok_or(CliError::NoHome)?; - let changed = maestro_hooks::install_for_home(&home)?; - if changed { - println!( - "installed claude-code activity hook: {} → {}", - maestro_hooks::script_path(&home).display(), - maestro_hooks::settings_path(&home).display() - ); - } else { - println!("claude-code activity hook already up to date"); + let script = maestro_hooks::script_path(&home); + for (target, result) in maestro_hooks::install_for_home(&home) { + let agent = target.label(); + let settings = target.settings_path(&home); + match result { + Ok(true) => { + println!( + "installed {agent} activity hook: {} → {}", + script.display(), + settings.display() + ); + if target == maestro_hooks::Target::Codex { + // Codex will not run a hook it has not been told to + // trust, and that approval is the operator's to give. + println!(" codex will ask you to trust this hook the next time it starts"); + } + } + Ok(false) => println!("{agent} activity hook already up to date"), + Err(e) => eprintln!("{agent} activity hook not installed: {e}"), + } } Ok(()) } diff --git a/crates/maestro-daemon/src/cli.rs b/crates/maestro-daemon/src/cli.rs index c61a5e9..47b5dc3 100644 --- a/crates/maestro-daemon/src/cli.rs +++ b/crates/maestro-daemon/src/cli.rs @@ -51,17 +51,23 @@ async fn run(config_override: Option) -> Result<(), Box> { // First-start hook auto-install (#106). Best-effort, before serving: a // session spawned right away should already carry the hook. if config.hooks_auto_install { - match crate::hooks::auto_install(&config.home_dir) { - crate::hooks::AutoInstallOutcome::Installed => { - tracing::info!("installed claude-code activity hook (#106)"); + for (target, outcome) in crate::hooks::auto_install(&config.home_dir) { + let agent = target.label(); + match outcome { + crate::hooks::AutoInstallOutcome::Installed => { + tracing::info!(agent, "installed activity hook (#106)"); + } + crate::hooks::AutoInstallOutcome::Upgraded => { + tracing::info!(agent, "updated activity hook to this build (#106)"); + } + crate::hooks::AutoInstallOutcome::Failed => { + tracing::warn!( + agent, + "hooks auto-install failed; run `maestro hooks install` manually" + ); + } + _ => {} } - 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"); - } - _ => {} } } // No worktree dir to pre-create: without an override the worktrees live diff --git a/crates/maestro-daemon/src/hooks.rs b/crates/maestro-daemon/src/hooks.rs index e1dc4d8..767ff4d 100644 --- a/crates/maestro-daemon/src/hooks.rs +++ b/crates/maestro-daemon/src/hooks.rs @@ -1,37 +1,53 @@ //! First-start hook auto-install (#106). //! -//! 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. +//! The daemon runs [`auto_install`] once at startup: for every agent maestro +//! knows how to hook ([`maestro_hooks::Target`]), unless that agent's hook +//! file 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, and one target failing +//! does not stop the others. The same global hook serves every 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 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 { +use maestro_hooks::Target; + +/// Install the activity hook for every [`Target`] under `home_dir`, skipping +/// any whose install is already current. Returns what happened per target — +/// an older install being brought up to date is +/// [`AutoInstallOutcome::Upgraded`], so the log can say so. An unresolvable +/// home dir (`""`) reports every target as [`AutoInstallOutcome::Skipped`]. +pub fn auto_install(home_dir: &str) -> Vec<(Target, AutoInstallOutcome)> { + Target::ALL + .iter() + .map(|&target| (target, auto_install_target(target, home_dir))) + .collect() +} + +fn auto_install_target(target: Target, home_dir: &str) -> AutoInstallOutcome { if home_dir.is_empty() { return AutoInstallOutcome::Skipped; } - let settings = maestro_hooks::settings_path(Path::new(home_dir)); - let script = maestro_hooks::script_path(Path::new(home_dir)); - if maestro_hooks::is_current(&settings, &script) { + let home = Path::new(home_dir); + let settings = target.settings_path(home); + let script = maestro_hooks::script_path(home); + if maestro_hooks::is_current(target, &settings, &script) { return AutoInstallOutcome::AlreadyPresent; } let stale = maestro_hooks::is_installed(&settings); - match maestro_hooks::install_for_home(Path::new(home_dir)) { + match maestro_hooks::install_target(target, home) { Ok(_) if stale => AutoInstallOutcome::Upgraded, Ok(_) => AutoInstallOutcome::Installed, Err(e) => { - tracing::warn!(error = %e, "hooks auto-install failed (non-fatal)"); + tracing::warn!( + target = target.label(), + error = %e, + "hooks auto-install failed (non-fatal)" + ); AutoInstallOutcome::Failed } } @@ -52,15 +68,50 @@ pub enum AutoInstallOutcome { mod tests { use super::*; + /// The outcome recorded for one target. + fn outcome(outcomes: &[(Target, AutoInstallOutcome)], target: Target) -> AutoInstallOutcome { + outcomes + .iter() + .find(|(t, _)| *t == target) + .map(|(_, o)| *o) + .expect("every target is reported") + } + /// Everything happens under a temp `$HOME`; the developer's own - /// `~/.claude/settings.json` is never read or written. + /// `~/.claude/settings.json` and `~/.codex/hooks.json` are never read or + /// written. #[test] fn installs_when_settings_carry_no_maestro_hook() { let dir = tempfile::tempdir().unwrap(); - let outcome = auto_install(dir.path().to_str().unwrap()); - assert_eq!(outcome, AutoInstallOutcome::Installed); - let settings = dir.path().join(".claude/settings.json"); - assert!(maestro_hooks::is_installed(&settings)); + let outcomes = auto_install(dir.path().to_str().unwrap()); + for target in Target::ALL { + assert_eq!( + outcome(&outcomes, target), + AutoInstallOutcome::Installed, + "{}", + target.label() + ); + assert!(maestro_hooks::is_installed( + &target.settings_path(dir.path()) + )); + } + } + + /// Every agent maestro knows about is hooked, not just Claude Code: a + /// codex session reports activity and its first prompt the same way. + #[test] + fn codex_gets_its_own_hook_file() { + let dir = tempfile::tempdir().unwrap(); + auto_install(dir.path().to_str().unwrap()); + let hooks = dir.path().join(".codex/hooks.json"); + let root: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&hooks).unwrap()).unwrap(); + let events = root["hooks"].as_object().unwrap(); + assert!(events.contains_key("PermissionRequest")); + assert!( + !events.contains_key("Notification"), + "codex has no Notification" + ); } #[test] @@ -69,14 +120,22 @@ mod tests { let home = dir.path().to_str().unwrap(); auto_install(home); // 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); - assert_eq!( - before, - std::fs::read_to_string(&settings).unwrap(), - "already-installed settings must not be rewritten" - ); + let before: Vec = Target::ALL + .iter() + .map(|t| std::fs::read_to_string(t.settings_path(dir.path())).unwrap()) + .collect(); + let outcomes = auto_install(home); + for (i, target) in Target::ALL.iter().enumerate() { + assert_eq!( + outcome(&outcomes, *target), + AutoInstallOutcome::AlreadyPresent + ); + assert_eq!( + before[i], + std::fs::read_to_string(target.settings_path(dir.path())).unwrap(), + "already-installed settings must not be rewritten" + ); + } } /// An install from an older maestro — present, but missing an event this @@ -87,7 +146,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let home = dir.path().to_str().unwrap(); auto_install(home); - let settings = dir.path().join(".claude/settings.json"); + let settings = Target::ClaudeCode.settings_path(dir.path()); let script = maestro_hooks::script_path(dir.path()); let mut root: serde_json::Value = @@ -96,12 +155,23 @@ mod tests { 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)); + let outcomes = auto_install(home); + assert_eq!( + outcome(&outcomes, Target::ClaudeCode), + AutoInstallOutcome::Upgraded + ); + assert!(maestro_hooks::is_current( + Target::ClaudeCode, + &settings, + &script + )); } #[test] fn unresolvable_home_is_skipped() { - assert_eq!(auto_install(""), AutoInstallOutcome::Skipped); + let outcomes = auto_install(""); + for target in Target::ALL { + assert_eq!(outcome(&outcomes, target), AutoInstallOutcome::Skipped); + } } } diff --git a/crates/maestro-hooks/src/lib.rs b/crates/maestro-hooks/src/lib.rs index be1a216..9216fce 100644 --- a/crates/maestro-hooks/src/lib.rs +++ b/crates/maestro-hooks/src/lib.rs @@ -1,4 +1,4 @@ -//! Claude Code activity hook (#106). +//! Agent activity hook (#106). //! //! Two halves, both shared by the `maestro hooks install` CLI subcommand and //! the daemon's first-start auto-install: @@ -6,28 +6,40 @@ //! * [`HOOK_SCRIPT`] — the hook itself, written to disk at install time. It //! posts an [`ActivityState`](maestro-protocol) to //! `$MAESTRO_URL/api/sessions/$MAESTRO_SESSION_ID/activity` (#105). The same -//! global hook runs in every Claude Code session on the machine, so it exits +//! global hook runs in every agent session on the machine, so it exits //! 0 and silently when either #103 env var is unset, and it never lets a //! daemon failure escape (short curl timeout, status ignored). -//! * [`install`] — idempotent registration of that script in -//! `~/.claude/settings.json`. Only entries maestro itself wrote (identified -//! by [`HOOK_MARKER`]) are touched; every other hook and every other field -//! in the file is preserved verbatim. +//! * [`install`] — idempotent registration of that script in one [`Target`]'s +//! hook file. Only entries maestro itself wrote (identified by +//! [`HOOK_MARKER`]) are touched; every other hook and every other field in +//! the file is preserved verbatim. +//! +//! One script serves every [`Target`]: Claude Code and Codex read the same +//! `{"hooks": {Event: [{"hooks": [{"type": "command", "command": …}]}]}}` +//! shape and hand the hook the same JSON on stdin. Only the file's location +//! and the event set differ. use std::path::Path; /// Marker substring every maestro-installed hook command carries. The /// installer rewrites or removes exactly these entries — nothing else in the /// user's settings file. +/// +/// It still says `claude` although the script now serves every [`Target`]: +/// the marker is how an install from an older maestro is recognised, and +/// renaming it would orphan every entry already on disk. pub const HOOK_MARKER: &str = "maestro-claude-hook"; -/// File name of the installed script (carries [`HOOK_MARKER`]). +/// File name of the installed script (carries [`HOOK_MARKER`]). One script +/// for every [`Target`] — see the note on [`HOOK_MARKER`] for the name. pub const SCRIPT_FILE_NAME: &str = "maestro-claude-hook.sh"; -/// Hook events installed only with the `haiku-autoname` feature (#131). -/// `UserPromptSubmit` carries the prompt the user just submitted; the script -/// forwards the whole payload to `/api/sessions/:id/prompt`, which names a -/// still-provisionally-named session after its first one. +/// Hook events installed only with the `haiku-autoname` feature (#131), for +/// every [`Target`] — Claude Code and Codex both spell the event +/// `UserPromptSubmit` and both put the submitted text in the payload's +/// `prompt`. The script forwards the whole payload to +/// `/api/sessions/:id/prompt`, which names a still-provisionally-named +/// session after its first one. #[cfg(feature = "haiku-autoname")] pub const PROMPT_EVENTS: [(&str, &str); 1] = [("UserPromptSubmit", "Prompt")]; @@ -43,7 +55,7 @@ pub const PROMPT_EVENTS: [(&str, &str); 0] = []; /// Claude Code's own name for "the turn ended on an API error"; there is no /// failure signal in `Stop` or `SessionEnd` payloads, so those never report /// `Error`. -pub const HOOK_EVENTS: [(&str, &str); 5] = [ +pub const CLAUDE_EVENTS: [(&str, &str); 5] = [ // Tool starting: the agent is demonstrably producing work, even if the // tool itself runs for minutes. ("PreToolUse", "Active"), @@ -58,17 +70,86 @@ pub const HOOK_EVENTS: [(&str, &str); 5] = [ ("StopFailure", "Error"), ]; +/// Codex hook event → `ActivityState` spelling posted to the daemon. +/// +/// Codex's event set is close to Claude Code's but not identical: a pending +/// approval arrives as `PermissionRequest` rather than `Notification`, and +/// there is no `StopFailure`, so a codex session never reports `Error`. +pub const CODEX_EVENTS: [(&str, &str); 4] = [ + ("PreToolUse", "Active"), + ("PostToolUse", "Active"), + // Codex is blocked on the operator approving a tool call. + ("PermissionRequest", "AwaitingInput"), + ("Stop", "AwaitingInput"), +]; + +/// An agent whose hook file maestro writes into. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Target { + /// `~/.claude/settings.json`. + ClaudeCode, + /// `~/.codex/hooks.json`. Codex asks the operator to trust a hook it has + /// not seen before, so an install here does nothing until they approve it + /// once inside codex — maestro never writes that approval itself. + Codex, +} + +impl Target { + /// Every target an install covers. Both files are written whether or not + /// the agent is on the machine: an unused hook file costs nothing, and + /// the alternative is a session spawned after the daemon started missing + /// the hook because its agent arrived later. + pub const ALL: [Target; 2] = [Target::ClaudeCode, Target::Codex]; + + /// How the target is named in logs and CLI output. + pub fn label(self) -> &'static str { + match self { + Target::ClaudeCode => "claude-code", + Target::Codex => "codex", + } + } + + /// The file this target's hook entries are registered in, under `home`. + pub fn settings_path(self, home: &Path) -> std::path::PathBuf { + match self { + Target::ClaudeCode => home.join(".claude/settings.json"), + Target::Codex => home.join(".codex/hooks.json"), + } + } + + /// The activity events this target emits. + pub fn activity_events(self) -> &'static [(&'static str, &'static str)] { + match self { + Target::ClaudeCode => &CLAUDE_EVENTS, + Target::Codex => &CODEX_EVENTS, + } + } + + /// Every event installed for this target: its activity events plus, with + /// `haiku-autoname` on, the prompt event both agents spell the same way. + pub fn events(self) -> impl Iterator { + self.activity_events() + .iter() + .chain(PROMPT_EVENTS.iter()) + .copied() + } +} + /// The shipped hook script. POSIX sh so any `/bin/sh` works. pub const HOOK_SCRIPT: &str = r#"#!/bin/sh -# Installed by maestro (#106): report Claude Code activity to its daemon. -# Exits silently unless running inside a session maestro spawned (#103 env). +# Installed by maestro (#106): report agent activity to its daemon. +# One script for every agent maestro installs hooks into; the state to post +# is chosen by the caller, so the two event sets differ only in what is +# registered where. Exits silently unless running inside a session maestro +# spawned (#103 env). # Event -> ActivityState mapping: -# PreToolUse -> Active (tool started; agent is working) -# PostToolUse -> Active (tool finished) -# Notification -> AwaitingInput (permission / idle notice) -# Stop -> AwaitingInput (turn ended; waiting on the user) -# StopFailure -> Error (turn ended on an API error) -# Prompt -> the payload is forwarded to the prompt route (#131) +# PreToolUse -> Active (tool started; agent is working) +# PostToolUse -> Active (tool finished) +# Notification -> AwaitingInput (claude code: permission / idle) +# PermissionRequest -> AwaitingInput (codex: waiting on an approval) +# Stop -> AwaitingInput (turn ended; waiting on the user) +# StopFailure -> Error (claude code: turn ended on an error) +# Prompt -> the payload is forwarded to the prompt route (#131) state="$1" [ -n "$MAESTRO_SESSION_ID" ] || exit 0 [ -n "$MAESTRO_URL" ] || exit 0 @@ -115,34 +196,29 @@ pub fn script_path(home: &Path) -> std::path::PathBuf { home.join(".local/share/maestro").join(SCRIPT_FILE_NAME) } -/// The user-level Claude Code settings file for a given home directory. -pub fn settings_path(home: &Path) -> std::path::PathBuf { - home.join(".claude/settings.json") -} - -/// 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`]. +/// Whether the file at `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. +/// Whether what is on disk is what this build ships for `target`: the script +/// at `script_path` byte-for-byte, and exactly one settings entry per one of +/// the target's [`Target::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 { +/// This is the upgrade question, asked per [`Target`]. 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(target: Target, 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)) + .map(|root| maestro_entries(&root) == expected_entries(target, script_path)) .unwrap_or(false) } @@ -173,14 +249,13 @@ fn maestro_entries(root: &serde_json::Value) -> Vec<(String, String)> { 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()) +/// What [`install`] writes for `target` and `script_path`, in the same shape. +fn expected_entries(target: Target, script_path: &Path) -> Vec<(String, String)> { + let mut want: Vec<(String, String)> = target + .events() .map(|(event, state)| { ( - (*event).to_string(), + event.to_string(), format!("{} {state}", script_path.display()), ) }) @@ -207,28 +282,40 @@ pub fn write_script(path: &Path) -> Result<(), HooksError> { Ok(()) } -/// Install the full hook setup for `home`: write the script under -/// `.local/share/maestro/` and register it in `.claude/settings.json`. +/// Install the full hook setup for one target under `home`: write the script +/// under `.local/share/maestro/` and register it in the target's hook file. /// Idempotent; returns whether anything changed on disk. -pub fn install_for_home(home: &Path) -> Result { +pub fn install_target(target: Target, home: &Path) -> Result { let script = script_path(home); - let settings = settings_path(home); + let settings = target.settings_path(home); // 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); + let changed = !is_current(target, &settings, &script); write_script(&script)?; - install(&settings, &script)?; + install(target, &settings, &script)?; Ok(changed) } -/// Register the hook command for every event in [`HOOK_EVENTS`] in the -/// settings file at `settings_path`. Idempotent: existing maestro entries are -/// updated in place wherever they sit, stale ones removed, duplicates folded; -/// every non-maestro entry and every unrelated top-level field survives. +/// Install for every [`Target::ALL`], reporting each one separately. One +/// target failing — an unwritable file, unparseable JSON — never stops the +/// others: an agent whose hook could not be written is one agent without +/// activity reporting, not a failed install. +pub fn install_for_home(home: &Path) -> Vec<(Target, Result)> { + Target::ALL + .iter() + .map(|&target| (target, install_target(target, home))) + .collect() +} + +/// Register the hook command for every one of `target`'s [`Target::events`] +/// in the settings file at `settings_path`. Idempotent: existing maestro +/// entries are updated in place wherever they sit, stale ones removed, +/// duplicates folded; every non-maestro entry and every unrelated top-level +/// field survives. /// /// Never inspects or writes anywhere but `settings_path` — callers pass the /// user's real home or a temp one in tests, never a repository. -pub fn install(settings_path: &Path, script_path: &Path) -> Result<(), HooksError> { +pub fn install(target: Target, settings_path: &Path, script_path: &Path) -> Result<(), HooksError> { let mut root = settings_json(settings_path)?; // Strip every maestro entry wherever it currently sits, so an older @@ -236,7 +323,7 @@ pub fn install(settings_path: &Path, script_path: &Path) -> Result<(), HooksErro strip_maestro_entries(&mut root); let hooks = ensure_object(&mut root, "hooks"); - for (event, state) in HOOK_EVENTS.iter().chain(PROMPT_EVENTS.iter()).copied() { + for (event, state) in target.events() { let groups = ensure_array(hooks, event); let mut group = serde_json::Map::new(); group.insert( diff --git a/crates/maestro-hooks/src/tests.rs b/crates/maestro-hooks/src/tests.rs index 7e689c3..59cbef4 100644 --- a/crates/maestro-hooks/src/tests.rs +++ b/crates/maestro-hooks/src/tests.rs @@ -1,15 +1,24 @@ //! Tests for the shipped hook script (run as a real `/bin/sh` subprocess) //! and the settings-file installer. Everything happens under a temp `$HOME`; -//! the developer's own `~/.claude/settings.json` is never touched. +//! the developer's own `~/.claude/settings.json` and `~/.codex/hooks.json` +//! are never touched. use std::io::{Read, Write}; use std::process::{Command, Stdio}; use super::*; -/// How many commands one install writes: every activity event, plus the -/// prompt event when `haiku-autoname` is on (#131). -const INSTALLED_EVENTS: usize = HOOK_EVENTS.len() + PROMPT_EVENTS.len(); +/// How many commands one install writes for a target: its activity events, +/// plus the prompt event when `haiku-autoname` is on (#131). +fn installed_events(target: Target) -> usize { + target.events().count() +} + +/// The Claude Code hook file under a temp home. Most installer tests only +/// need one target, and this is the one they were written against. +fn settings_path(home: &Path) -> std::path::PathBuf { + Target::ClaudeCode.settings_path(home) +} fn sh(script: &str, args: &[&str], envs: &[(&str, &str)], payload: &[u8]) -> std::process::Output { let mut cmd = Command::new("/bin/sh"); @@ -70,7 +79,7 @@ fn owned_envs<'a>(envs: &'a [(&'static str, String)]) -> Vec<(&'static str, &'a /// exit 0, no output, stdin drained. #[test] fn script_exits_silently_without_maestro_env() { - for (event, _) in HOOK_EVENTS { + for (event, _) in CLAUDE_EVENTS { let out = sh(HOOK_SCRIPT, &[event], &[], br#"{"session_id":"x"}"#); assert!(out.status.success(), "{event:?}: {:?}", out.stderr); assert!(out.stdout.is_empty(), "{event:?}: {:?}", out.stdout); @@ -103,7 +112,7 @@ fn script_posts_mapped_state_to_the_activity_endpoint() { /// Every event maps to the state the contract specifies. #[test] fn event_state_mapping_matches_the_contract() { - let map: std::collections::HashMap<&str, &str> = HOOK_EVENTS.into_iter().collect(); + let map: std::collections::HashMap<&str, &str> = CLAUDE_EVENTS.into_iter().collect(); assert_eq!(map["PreToolUse"], "Active"); assert_eq!(map["PostToolUse"], "Active"); assert_eq!(map["Notification"], "AwaitingInput"); @@ -203,10 +212,10 @@ fn fresh_install_registers_every_event() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); write_script(&script_path(home)).unwrap(); - install(&settings_path(home), &script_path(home)).unwrap(); + install(Target::ClaudeCode, &settings_path(home), &script_path(home)).unwrap(); let cmds = installed_commands(&settings_path(home)); - assert_eq!(cmds.len(), INSTALLED_EVENTS); + assert_eq!(cmds.len(), installed_events(Target::ClaudeCode)); let joined = cmds.join("\n"); assert_eq!(joined.matches("Active").count(), 2); assert!(joined.contains("Error")); @@ -221,15 +230,15 @@ fn fresh_install_registers_every_event() { fn install_is_idempotent() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - install(&settings_path(home), &script_path(home)).unwrap(); + install(Target::ClaudeCode, &settings_path(home), &script_path(home)).unwrap(); let once = std::fs::read_to_string(settings_path(home)).unwrap(); - install(&settings_path(home), &script_path(home)).unwrap(); + install(Target::ClaudeCode, &settings_path(home), &script_path(home)).unwrap(); let twice = std::fs::read_to_string(settings_path(home)).unwrap(); assert_eq!(once, twice); assert_eq!( installed_commands(&settings_path(home)).len(), - INSTALLED_EVENTS + installed_events(Target::ClaudeCode) ); } @@ -257,12 +266,12 @@ fn install_updates_an_older_maestro_hook_in_place() { .path() .join(".local/share/maestro") .join(SCRIPT_FILE_NAME); - install(&settings, &new_script).unwrap(); + install(Target::ClaudeCode, &settings, &new_script).unwrap(); let cmds = installed_commands(&settings); assert_eq!( cmds.len(), - INSTALLED_EVENTS, + installed_events(Target::ClaudeCode), "old entry replaced, not kept: {cmds:?}" ); assert!(!cmds.iter().any(|c| c == old_script)); @@ -298,7 +307,7 @@ fn install_preserves_foreign_hooks_and_removes_stale_maestro_ones() { .path() .join(".local/share/maestro") .join(SCRIPT_FILE_NAME); - install(&settings, &new_script).unwrap(); + install(Target::ClaudeCode, &settings, &new_script).unwrap(); let root: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&settings).unwrap()).unwrap(); @@ -311,7 +320,7 @@ fn install_preserves_foreign_hooks_and_removes_stale_maestro_ones() { let cmds = installed_commands(&settings); assert_eq!( cmds.iter().filter(|c| c.contains(HOOK_MARKER)).count(), - INSTALLED_EVENTS, + installed_events(Target::ClaudeCode), "{cmds:?}" ); } @@ -321,9 +330,15 @@ fn install_preserves_foreign_hooks_and_removes_stale_maestro_ones() { fn is_installed_tracks_reality() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - assert!(!is_installed(&settings_path(home))); - install_for_home(home).unwrap(); - assert!(is_installed(&settings_path(home))); + for target in Target::ALL { + assert!(!is_installed(&target.settings_path(home))); + } + for (_, result) in install_for_home(home) { + result.unwrap(); + } + for target in Target::ALL { + assert!(is_installed(&target.settings_path(home))); + } } /// The full home-level install leaves the script executable. @@ -331,7 +346,9 @@ fn is_installed_tracks_reality() { fn install_for_home_writes_an_executable_script() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - install_for_home(home).unwrap(); + for (_, result) in install_for_home(home) { + result.unwrap(); + } #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -350,7 +367,7 @@ fn install_for_home_writes_an_executable_script() { fn install_registers_the_prompt_event() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - install(&settings_path(home), &script_path(home)).unwrap(); + install(Target::ClaudeCode, &settings_path(home), &script_path(home)).unwrap(); let root: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(settings_path(home)).unwrap()).unwrap(); @@ -367,7 +384,7 @@ fn install_registers_the_prompt_event() { fn install_registers_no_prompt_event_without_the_feature() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - install(&settings_path(home), &script_path(home)).unwrap(); + install(Target::ClaudeCode, &settings_path(home), &script_path(home)).unwrap(); let root: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(settings_path(home)).unwrap()).unwrap(); @@ -436,8 +453,12 @@ fn the_script_posts_a_prompt_payload_unchanged() { 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))); + install_target(Target::ClaudeCode, home).unwrap(); + assert!(is_current( + Target::ClaudeCode, + &settings_path(home), + &script_path(home) + )); // Drop one event, as an older install would have left the file. let mut root: serde_json::Value = @@ -450,7 +471,11 @@ fn is_current_rejects_an_install_missing_an_event() { .unwrap(); assert!(is_installed(&settings_path(home))); - assert!(!is_current(&settings_path(home), &script_path(home))); + assert!(!is_current( + Target::ClaudeCode, + &settings_path(home), + &script_path(home) + )); } /// A settings file that is complete but points at a script from an older @@ -459,9 +484,13 @@ fn is_current_rejects_an_install_missing_an_event() { fn is_current_rejects_a_stale_script() { let dir = tempfile::tempdir().unwrap(); let home = dir.path(); - install_for_home(home).unwrap(); + install_target(Target::ClaudeCode, home).unwrap(); std::fs::write(script_path(home), "#!/bin/sh\nexit 0\n").unwrap(); - assert!(!is_current(&settings_path(home), &script_path(home))); + assert!(!is_current( + Target::ClaudeCode, + &settings_path(home), + &script_path(home) + )); } /// Nothing installed at all is not current. @@ -469,18 +498,93 @@ fn is_current_rejects_a_stale_script() { 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))); + assert!(!is_current( + Target::ClaudeCode, + &settings_path(home), + &script_path(home) + )); } -/// `install_for_home` reports a stale script as a change, even when the +/// `install_target` 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() { +fn install_target_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()); + assert!(install_target(Target::ClaudeCode, home).unwrap()); + assert!(!install_target(Target::ClaudeCode, home).unwrap()); std::fs::write(script_path(home), "#!/bin/sh\nexit 0\n").unwrap(); - assert!(install_for_home(home).unwrap()); + assert!(install_target(Target::ClaudeCode, home).unwrap()); +} + +/// Codex is hooked in its own file, with its own event set: the two agents +/// share the script but not the events, so neither file carries the other's +/// spellings (#131 follow-up — a codex session was never named because +/// nothing installed its `UserPromptSubmit` hook). +#[test] +fn each_target_gets_its_own_file_and_events() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + for (_, result) in install_for_home(home) { + result.unwrap(); + } + + let claude = events_in(&Target::ClaudeCode.settings_path(home)); + let codex = events_in(&Target::Codex.settings_path(home)); + assert_eq!( + Target::Codex.settings_path(home), + home.join(".codex/hooks.json") + ); + + // Claude Code's failure signal has no codex equivalent, and codex's + // approval notice is spelled differently. + assert!(claude.contains(&"StopFailure".to_string())); + assert!(claude.contains(&"Notification".to_string())); + assert!(!codex.contains(&"StopFailure".to_string())); + assert!(!codex.contains(&"Notification".to_string())); + assert!(codex.contains(&"PermissionRequest".to_string())); + + // Both report work and both report a finished turn. + for event in ["PreToolUse", "PostToolUse", "Stop"] { + assert!(claude.contains(&event.to_string()), "claude {event}"); + assert!(codex.contains(&event.to_string()), "codex {event}"); + } + + assert_eq!( + installed_commands(&Target::Codex.settings_path(home)).len(), + installed_events(Target::Codex) + ); +} + +/// #131: a codex session names itself the same way a Claude Code one does — +/// same event spelling, same script, same `Prompt` argument. +#[cfg(feature = "haiku-autoname")] +#[test] +fn codex_reports_its_first_prompt_too() { + let dir = tempfile::tempdir().unwrap(); + let home = dir.path(); + install_target(Target::Codex, home).unwrap(); + + let root: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(Target::Codex.settings_path(home)).unwrap()) + .unwrap(); + let cmd = root["hooks"]["UserPromptSubmit"][0]["hooks"][0]["command"] + .as_str() + .unwrap() + .to_string(); + assert!(cmd.ends_with(" Prompt"), "{cmd}"); + assert!(cmd.contains(SCRIPT_FILE_NAME)); +} + +/// Every event name a settings file registers a maestro hook under. +fn events_in(settings: &Path) -> Vec { + serde_json::from_str::(&std::fs::read_to_string(settings).unwrap()) + .unwrap() + .get("hooks") + .and_then(|h| h.as_object()) + .into_iter() + .flatten() + .map(|(event, _)| event.clone()) + .collect() } diff --git a/docs/SIDEBAR.md b/docs/SIDEBAR.md index 8cc4fb2..c1af7e3 100644 --- a/docs/SIDEBAR.md +++ b/docs/SIDEBAR.md @@ -250,6 +250,10 @@ Two rulings specific to a reported `Error` (#114): process changes do not clear it — only the agent itself may withdraw what the agent itself reported. Re-reporting `Error` extends the TTL rather than stacking reports. +- **Only Claude Code reports `Error`.** Codex has no counterpart to + `StopFailure` in its hook event set, so a codex turn that ends on an API + error reports `AwaitingInput` and its card carries no error mark. A card + without the mark means "nothing reported a failure", not "nothing failed". If a client receives an `ActivityState` variant it does not know, it must not fail rendering: treat it as `Idle` (neutral/dim, line-2 rules apply as usual)