diff --git a/crates/maestro-hooks/src/lib.rs b/crates/maestro-hooks/src/lib.rs index 429f317..fcd4359 100644 --- a/crates/maestro-hooks/src/lib.rs +++ b/crates/maestro-hooks/src/lib.rs @@ -25,13 +25,25 @@ pub const HOOK_MARKER: &str = "maestro-claude-hook"; pub const SCRIPT_FILE_NAME: &str = "maestro-claude-hook.sh"; /// Claude Code hook event → `ActivityState` spelling posted to the daemon. -pub const HOOK_EVENTS: [(&str, &str); 3] = [ - // Tool finished: the agent is demonstrably producing work. +/// +/// `PreToolUse` is what makes a session `Active` *during* a long tool call — +/// `PostToolUse` alone only fires once it ends (#114). `StopFailure` is +/// 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] = [ + // Tool starting: the agent is demonstrably producing work, even if the + // tool itself runs for minutes. + ("PreToolUse", "Active"), + // Tool finished successfully. ("PostToolUse", "Active"), // Permission / idle notices: Claude is blocked on the user. ("Notification", "AwaitingInput"), // Turn ended: waiting on the user again. ("Stop", "AwaitingInput"), + // Turn ended on an API error; the payload's `error` field is forwarded + // as the report's `detail` (#114). + ("StopFailure", "Error"), ]; /// The shipped hook script. POSIX sh so any `/bin/sh` works. @@ -39,17 +51,27 @@ 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). # Event -> ActivityState mapping: -# PostToolUse -> Active (tool finished; agent is working) +# 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) state="$1" [ -n "$MAESTRO_SESSION_ID" ] || exit 0 [ -n "$MAESTRO_URL" ] || exit 0 -# Drain the hook payload so the parent never sees a broken pipe. -cat >/dev/null 2>&1 +payload=$(cat 2>/dev/null) || true +body="{\"state\":\"$state\"}" +# StopFailure payloads carry the outcome in "error". Only a plain enum +# spelling is reported as the detail — anything else (quotes, backslashes, +# prose) would risk breaking the JSON body, and the spelling already names +# the failure. +detail=$(printf '%s' "$payload" | sed -n 's/.*"error"[[:space:]]*:[[:space:]]*"\([a-z_]*\)".*/\1/p') +if [ -n "$detail" ]; then + body="{\"state\":\"$state\",\"detail\":\"$detail\"}" +fi curl -fsS --connect-timeout 1 --max-time 2 \ -H 'Content-Type: application/json' \ - -d "{\"state\":\"$state\"}" \ + -d "$body" \ "$MAESTRO_URL/api/sessions/$MAESTRO_SESSION_ID/activity" \ >/dev/null 2>&1 || true exit 0 diff --git a/crates/maestro-hooks/src/tests.rs b/crates/maestro-hooks/src/tests.rs index 79acd80..6f73169 100644 --- a/crates/maestro-hooks/src/tests.rs +++ b/crates/maestro-hooks/src/tests.rs @@ -7,7 +7,7 @@ use std::process::{Command, Stdio}; use super::*; -fn sh(script: &str, args: &[&str], envs: &[(&str, &str)]) -> std::process::Output { +fn sh(script: &str, args: &[&str], envs: &[(&str, &str)], payload: &[u8]) -> std::process::Output { let mut cmd = Command::new("/bin/sh"); // `sh -c CMD NAME ARGS…`: NAME fills $0, so the real arguments start at $1. let mut argv = vec!["-c", script, HOOK_MARKER]; @@ -26,21 +26,48 @@ fn sh(script: &str, args: &[&str], envs: &[(&str, &str)]) -> std::process::Outpu .spawn() .expect("spawn /bin/sh"); // Feed a Claude-Code-style payload on stdin; the script must drain it. - child - .stdin - .take() - .unwrap() - .write_all(br#"{"session_id":"x"}"#) - .ok(); + child.stdin.take().unwrap().write_all(payload).ok(); child.wait_with_output().expect("wait") } +const SESSION: &str = "01234567-89ab-cdef-0123-456789abcdef"; + +/// Accept exactly one HTTP request on a throwaway listener, answer 204, and +/// hand back the raw request text. Runs `run` with the base URL once the +/// acceptor thread is in place. +fn capture_request(run: impl FnOnce(String) -> std::process::Output) -> String { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + let (mut sock, _) = listener.accept().unwrap(); + let mut buf = [0u8; 4096]; + let n = sock.read(&mut buf).unwrap(); + sock.write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n") + .unwrap(); + String::from_utf8_lossy(&buf[..n]).to_string() + }); + let out = run(format!("http://{addr}")); + assert!(out.status.success(), "{:?}", out.stderr); + handle.join().unwrap() +} + +fn session_envs(url: &str) -> Vec<(&'static str, String)> { + vec![ + ("MAESTRO_URL", url.to_string()), + ("MAESTRO_SESSION_ID", SESSION.to_string()), + ] +} + +fn owned_envs<'a>(envs: &'a [(&'static str, String)]) -> Vec<(&'static str, &'a str)> { + envs.iter().map(|(k, v)| (*k, v.as_str())).collect() +} + /// The hook must be invisible outside maestro: no env vars (#103 unset) ⇒ /// exit 0, no output, stdin drained. #[test] fn script_exits_silently_without_maestro_env() { for (event, _) in HOOK_EVENTS { - let out = sh(HOOK_SCRIPT, &[event], &[]); + 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); assert!(out.stderr.is_empty(), "{event:?}: {:?}", out.stderr); @@ -51,46 +78,82 @@ fn script_exits_silently_without_maestro_env() { /// activity endpoint and still exits 0. #[test] fn script_posts_mapped_state_to_the_activity_endpoint() { - let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let server = move || { - let (mut sock, _) = listener.accept().unwrap(); - let mut buf = [0u8; 4096]; - let n = sock.read(&mut buf).unwrap(); - let raw = String::from_utf8_lossy(&buf[..n]).to_string(); - sock.write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n") - .unwrap(); - raw - }; - let handle = std::thread::spawn(server); - - let url = format!("http://{addr}"); - let session = "01234567-89ab-cdef-0123-456789abcdef"; // The installer bakes the mapped state into the command's argv; the // script relays whatever it is given. - let out = sh( - HOOK_SCRIPT, - &["Active"], - &[("MAESTRO_URL", &url), ("MAESTRO_SESSION_ID", session)], - ); - assert!(out.status.success()); - - let request = handle.join().unwrap(); + let request = capture_request(|url| { + sh( + HOOK_SCRIPT, + &["Active"], + &owned_envs(&session_envs(&url)), + br#"{"session_id":"x"}"#, + ) + }); assert!(request.starts_with("POST "), "{request}"); assert!( - request.contains(&format!("/api/sessions/{session}/activity")), + request.contains(&format!("/api/sessions/{SESSION}/activity")), "{request}" ); assert!(request.contains(r#""state":"Active""#), "{request}"); } -/// Every event maps to the state the issue specifies. +/// 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(); + assert_eq!(map["PreToolUse"], "Active"); assert_eq!(map["PostToolUse"], "Active"); - assert_eq!(map["Stop"], "AwaitingInput"); assert_eq!(map["Notification"], "AwaitingInput"); + assert_eq!(map["Stop"], "AwaitingInput"); + assert_eq!(map["StopFailure"], "Error"); +} + +/// A `StopFailure` payload names the API error that ended the turn; the +/// script reports its `error` spelling as the report's `detail` so a card +/// can show why (#114). +#[test] +fn script_reports_the_error_spelling_as_detail() { + let request = capture_request(|url| { + sh( + HOOK_SCRIPT, + &["Error"], + &owned_envs(&session_envs(&url)), + br#"{"session_id":"abc","hook_event_name":"StopFailure","error":"rate_limit","error_details":"429 Too Many Requests"}"#, + ) + }); + assert!(request.contains(r#""state":"Error""#), "{request}"); + assert!(request.contains(r#""detail":"rate_limit""#), "{request}"); +} + +/// An `error` value that is not a plain enum spelling (or a payload without +/// one at all) must never produce a broken body — no detail is posted. +#[test] +fn script_posts_no_detail_for_an_unexpected_error_value() { + let request = capture_request(|url| { + sh( + HOOK_SCRIPT, + &["Error"], + &owned_envs(&session_envs(&url)), + br#"{"hook_event_name":"StopFailure","error":"429 \"Too Many\" \\ requests"}"#, + ) + }); + assert!(request.contains(r#""state":"Error""#), "{request}"); + assert!(!request.contains("detail"), "{request}"); +} + +/// Events whose payload carries no error fields post a bare body — never an +/// empty or stale detail. +#[test] +fn script_posts_no_detail_for_events_without_an_error() { + let request = capture_request(|url| { + sh( + HOOK_SCRIPT, + &["AwaitingInput"], + &owned_envs(&session_envs(&url)), + br#"{"hook_event_name":"Stop","stop_hook_active":false}"#, + ) + }); + assert!(request.contains(r#""state":"AwaitingInput""#), "{request}"); + assert!(!request.contains("detail"), "{request}"); } /// A dead or unreachable daemon must never fail — or slow down — a session. @@ -104,6 +167,7 @@ fn script_ignores_daemon_failure() { ("MAESTRO_URL", "http://127.0.0.1:1"), ("MAESTRO_SESSION_ID", "00000000-0000-0000-0000-000000000000"), ], + br#"{"session_id":"x"}"#, ); assert!(out.status.success()); } @@ -138,9 +202,10 @@ fn fresh_install_registers_every_event() { install(&settings_path(home), &script_path(home)).unwrap(); let cmds = installed_commands(&settings_path(home)); - assert_eq!(cmds.len(), 3); + assert_eq!(cmds.len(), HOOK_EVENTS.len()); let joined = cmds.join("\n"); - assert!(joined.contains("Active")); + assert_eq!(joined.matches("Active").count(), 2); + assert!(joined.contains("Error")); assert_eq!(joined.matches("AwaitingInput").count(), 2); for cmd in &cmds { assert!(cmd.contains(SCRIPT_FILE_NAME)); @@ -158,7 +223,10 @@ fn install_is_idempotent() { install(&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(), 3); + assert_eq!( + installed_commands(&settings_path(home)).len(), + HOOK_EVENTS.len() + ); } /// An older maestro entry under the same events is updated in place, not @@ -188,7 +256,11 @@ fn install_updates_an_older_maestro_hook_in_place() { install(&settings, &new_script).unwrap(); let cmds = installed_commands(&settings); - assert_eq!(cmds.len(), 3, "old entry replaced, not kept: {cmds:?}"); + assert_eq!( + cmds.len(), + HOOK_EVENTS.len(), + "old entry replaced, not kept: {cmds:?}" + ); assert!(!cmds.iter().any(|c| c == old_script)); let root: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&settings).unwrap()).unwrap(); @@ -231,11 +303,11 @@ fn install_preserves_foreign_hooks_and_removes_stale_maestro_ones() { assert_eq!(pre[0]["hooks"][0]["command"], "my-own-linter.sh"); // Stale maestro event gone. assert!(root["hooks"].get("UserPromptSubmit").is_none()); - // Our three events present, nothing else maestro-related anywhere. + // Our events present, nothing else maestro-related anywhere. let cmds = installed_commands(&settings); assert_eq!( cmds.iter().filter(|c| c.contains(HOOK_MARKER)).count(), - 3, + HOOK_EVENTS.len(), "{cmds:?}" ); } diff --git a/docs/SIDEBAR.md b/docs/SIDEBAR.md index e2341c4..82b0522 100644 --- a/docs/SIDEBAR.md +++ b/docs/SIDEBAR.md @@ -107,6 +107,20 @@ When the TTL expires, inference resumes and the session's reported detail — if any — clears with the report. Clients never blend the two: whichever source is in force supplies both the colour and the clock for `activity_since`. +Two rulings specific to a reported `Error` (#114): + +- **The five-minute TTL applies to `Error` like to any other state.** A + session that failed is still failed after five minutes, but a card pinned + red forever lies once the session moves on; when the report lapses the card + returns to the daemon's own inference — usually `Idle` for a session that + has gone quiet. +- **Only a new report clears an `Error` before its TTL.** The next + `ActivityState` of any kind replaces it (with #106's hook that is typically + `PreToolUse` → `Active` when the user prompts again). PTY output and + 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. + 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) until the daemon sends a variant it does know. Rust clients deserialize