merge: sidebar-cards/114-hook-error

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

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:?}"
     );
 }

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