From 467e2cbfff0a8679a6a2c80d42054fb56655ea75 Mon Sep 17 00:00:00 2001 From: ningjingkun Date: Thu, 28 May 2026 20:58:26 +0800 Subject: [PATCH 1/3] feat(hooks): allow message_submit to transform submitted text --- crates/tui/src/commands/hooks.rs | 2 +- crates/tui/src/hooks.rs | 549 +++++++++++++++++++++++++++++- crates/tui/src/tui/ui.rs | 26 +- crates/tui/src/tui/ui/tests.rs | 108 ++++++ docs/CONFIGURATION.md | 49 +++ docs/rfcs/1364-hooks-lifecycle.md | 276 +++++++++++++++ web/app/[locale]/docs/page.tsx | 12 +- 7 files changed, 1009 insertions(+), 13 deletions(-) create mode 100644 docs/rfcs/1364-hooks-lifecycle.md diff --git a/crates/tui/src/commands/hooks.rs b/crates/tui/src/commands/hooks.rs index fbf7d760b..d9589c6c1 100644 --- a/crates/tui/src/commands/hooks.rs +++ b/crates/tui/src/commands/hooks.rs @@ -45,7 +45,7 @@ fn events() -> CommandResult { (HookEvent::SessionEnd, "fires once on graceful shutdown"), ( HookEvent::MessageSubmit, - "fires when the user submits a turn (before model dispatch)", + "fires before model dispatch; can transform or block submitted text", ), ( HookEvent::ToolCallBefore, diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index 2fbc5f55a..685fea762 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -14,8 +14,9 @@ #[allow(unused_imports)] use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; +use serde_json::json; use std::collections::HashMap; -use std::io::Read; +use std::io::{Read, Write}; use std::path::PathBuf; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; @@ -423,6 +424,24 @@ pub struct HookResult { pub error: Option, } +/// Result of running mutable `message_submit` hooks. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum MessageSubmitOutcome { + /// No hook changed the submitted text. + Unchanged, + /// One or more hooks replaced the submitted text. + Replaced(String), + /// A hook intentionally blocked the submission. + Blocked { reason: String }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum MessageSubmitStdout { + Unchanged, + Replaced(String), + Invalid(String), +} + /// Executor for running hooks #[derive(Debug, Clone)] pub struct HookExecutor { @@ -500,6 +519,100 @@ impl HookExecutor { self.config.enabled && self.config.hooks.iter().any(|h| h.event == event) } + /// Run configured `message_submit` hooks as a mutable submit pipeline. + /// + /// This is deliberately separate from [`Self::execute`]: most hook events + /// are observer-only, while `message_submit` has a narrow stdout JSON + /// contract that can replace or block the submitted text. + pub fn execute_message_submit_transform( + &self, + context: &HookContext, + original_text: &str, + ) -> MessageSubmitOutcome { + if !self.config.enabled { + return MessageSubmitOutcome::Unchanged; + } + + let hooks = self.config.hooks_for_event(HookEvent::MessageSubmit); + if hooks.is_empty() { + return MessageSubmitOutcome::Unchanged; + } + + let mut current_text = original_text.to_string(); + + for hook in hooks { + let hook_context = context.clone().with_message(¤t_text); + if !self.matches_condition(hook, &hook_context) { + continue; + } + + let env_vars = hook_context.to_env_vars(); + if hook.background { + let _ = self.execute_background(hook, &env_vars); + continue; + } + + let payload = message_submit_payload(&hook_context, ¤t_text); + let result = self.execute_sync_with_stdin(hook, &env_vars, &payload); + + if result.exit_code == Some(2) { + return MessageSubmitOutcome::Blocked { + reason: message_submit_block_reason( + &result, + "message_submit hook blocked submission", + ), + }; + } + + if !result.success { + let label = result.name.as_deref().unwrap_or("(unnamed)"); + tracing::warn!( + target: "hooks", + hook = label, + event = "message_submit", + exit_code = ?result.exit_code, + duration_ms = result.duration.as_millis() as u64, + error = result.error.as_deref().unwrap_or(""), + stderr_head = %result.stderr.lines().next().unwrap_or(""), + "message_submit hook failed" + ); + + if hook.continue_on_error { + continue; + } + + return MessageSubmitOutcome::Blocked { + reason: message_submit_block_reason( + &result, + "message_submit hook failed and blocked submission", + ), + }; + } + + match parse_message_submit_stdout(&result.stdout) { + MessageSubmitStdout::Unchanged => {} + MessageSubmitStdout::Replaced(text) => { + current_text = text; + } + MessageSubmitStdout::Invalid(reason) => { + tracing::warn!( + target: "hooks", + hook = result.name.as_deref().unwrap_or("(unnamed)"), + event = "message_submit", + reason = %reason, + "ignored invalid message_submit hook stdout" + ); + } + } + } + + if current_text == original_text { + MessageSubmitOutcome::Unchanged + } else { + MessageSubmitOutcome::Replaced(current_text) + } + } + /// Run every `ShellEnv` hook for this context and merge their stdout /// (`KEY=VALUE\n` lines) into a single env-var map. Used by the /// `exec_shell` tool to inject ephemeral credentials, per-skill PATH @@ -659,6 +772,28 @@ impl HookExecutor { /// Execute a hook synchronously fn execute_sync(&self, hook: &Hook, env_vars: &HashMap) -> HookResult { + self.execute_sync_inner(hook, env_vars, None) + } + + /// Execute a hook synchronously with a structured JSON stdin payload. + /// + /// Used by mutable `message_submit` hooks. Existing observer hooks keep the + /// stdin-less [`Self::execute_sync`] path so their behavior is unchanged. + fn execute_sync_with_stdin( + &self, + hook: &Hook, + env_vars: &HashMap, + stdin_json: &serde_json::Value, + ) -> HookResult { + self.execute_sync_inner(hook, env_vars, Some(stdin_json)) + } + + fn execute_sync_inner( + &self, + hook: &Hook, + env_vars: &HashMap, + stdin_json: Option<&serde_json::Value>, + ) -> HookResult { let started = Instant::now(); let working_dir = self .config @@ -672,13 +807,32 @@ impl HookExecutor { .unwrap_or(hook.timeout_secs); let timeout = Duration::from_secs(timeout_secs); - let mut child = match Self::build_shell_command(&hook.command) + let stdin_bytes = match stdin_json.map(serde_json::to_vec).transpose() { + Ok(bytes) => bytes, + Err(e) => { + return HookResult { + name: hook.name.clone(), + success: false, + exit_code: None, + stdout: String::new(), + stderr: String::new(), + duration: started.elapsed(), + error: Some(format!("Failed to encode hook stdin: {e}")), + }; + } + }; + + let mut command = Self::build_shell_command(&hook.command); + command .current_dir(&working_dir) .envs(env_vars) .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - { + .stderr(Stdio::piped()); + if stdin_bytes.is_some() { + command.stdin(Stdio::piped()); + } + + let mut child = match command.spawn() { Ok(child) => child, Err(e) => { return HookResult { @@ -693,6 +847,12 @@ impl HookExecutor { } }; + if let (Some(bytes), Some(mut stdin)) = (stdin_bytes, child.stdin.take()) { + let _ = stdin.write_all(&bytes); + let _ = stdin.write_all(b"\n"); + let _ = stdin.flush(); + } + fn read_pipe(mut pipe: impl Read) -> String { let mut buf = String::new(); let _ = pipe.read_to_string(&mut buf); @@ -768,6 +928,84 @@ impl HookExecutor { } } +fn message_submit_payload(context: &HookContext, text: &str) -> serde_json::Value { + json!({ + "event": HookEvent::MessageSubmit.as_str(), + "text": text, + "session_id": context.session_id.as_deref(), + "workspace": context.workspace.as_ref().map(|path| path.display().to_string()), + "mode": context.mode.as_deref(), + "model": context.model.as_deref(), + "total_tokens": context.total_tokens, + }) +} + +fn parse_message_submit_stdout(stdout: &str) -> MessageSubmitStdout { + let trimmed = stdout.trim(); + if trimmed.is_empty() { + return MessageSubmitStdout::Unchanged; + } + + let value: serde_json::Value = match serde_json::from_str(trimmed) { + Ok(value) => value, + Err(e) => return MessageSubmitStdout::Invalid(format!("invalid JSON: {e}")), + }; + + let Some(object) = value.as_object() else { + return MessageSubmitStdout::Invalid("stdout JSON must be an object".to_string()); + }; + + match object.get("text") { + Some(serde_json::Value::String(text)) => MessageSubmitStdout::Replaced(text.clone()), + Some(_) => MessageSubmitStdout::Invalid("stdout `text` field must be a string".to_string()), + None => MessageSubmitStdout::Unchanged, + } +} + +fn message_submit_block_reason(result: &HookResult, fallback: &str) -> String { + if let Some(reason) = message_submit_stdout_reason(&result.stdout) { + return reason; + } + if let Some(reason) = first_non_empty_line(&result.stderr) { + return reason; + } + if let Some(reason) = first_non_empty_line(&result.stdout) { + return reason; + } + if let Some(reason) = result.error.as_deref().and_then(first_non_empty_line) { + return reason; + } + fallback.to_string() +} + +fn message_submit_stdout_reason(stdout: &str) -> Option { + let value: serde_json::Value = serde_json::from_str(stdout.trim()).ok()?; + value + .get("reason") + .and_then(serde_json::Value::as_str) + .map(truncate_hook_message) +} + +fn first_non_empty_line(text: &str) -> Option { + text.lines() + .map(str::trim) + .find(|line| !line.is_empty()) + .map(truncate_hook_message) +} + +fn truncate_hook_message(message: &str) -> String { + const MAX_CHARS: usize = 240; + let mut out = String::new(); + for (idx, ch) in message.chars().enumerate() { + if idx >= MAX_CHARS { + out.push('…'); + return out; + } + out.push(ch); + } + out +} + /// Parse `KEY=VALUE\n` lines from a `shell_env` hook's stdout into a map. /// /// Tolerated: blank lines, leading whitespace, `#` comment lines (ignored), @@ -850,6 +1088,58 @@ NOEQUAL line dropped assert!(parsed.is_empty()); } + #[test] + fn parse_message_submit_stdout_replaces_text() { + assert_eq!( + super::parse_message_submit_stdout(r#"{"text":"changed"}"#), + MessageSubmitStdout::Replaced("changed".to_string()) + ); + } + + #[test] + fn parse_message_submit_stdout_empty_is_unchanged() { + assert_eq!( + super::parse_message_submit_stdout(" \n\t "), + MessageSubmitStdout::Unchanged + ); + } + + #[test] + fn parse_message_submit_stdout_without_text_is_unchanged() { + assert_eq!( + super::parse_message_submit_stdout(r#"{"reason":"only used for blocks"}"#), + MessageSubmitStdout::Unchanged + ); + } + + #[test] + fn parse_message_submit_stdout_rejects_malformed_json() { + assert!(matches!( + super::parse_message_submit_stdout("not json"), + MessageSubmitStdout::Invalid(_) + )); + } + + #[test] + fn parse_message_submit_stdout_rejects_non_string_text() { + assert!(matches!( + super::parse_message_submit_stdout(r#"{"text":123}"#), + MessageSubmitStdout::Invalid(_) + )); + } + + #[test] + fn parse_message_submit_stdout_rejects_non_object_json() { + assert!(matches!( + super::parse_message_submit_stdout(r#"["not", "an", "object"]"#), + MessageSubmitStdout::Invalid(_) + )); + assert!(matches!( + super::parse_message_submit_stdout(r#""not an object""#), + MessageSubmitStdout::Invalid(_) + )); + } + #[test] fn test_hook_event_as_str() { assert_eq!(HookEvent::SessionStart.as_str(), "session_start"); @@ -995,6 +1285,255 @@ NOEQUAL line dropped assert_eq!(executor.session_id().len(), 13); // "sess_" + 8 chars } + #[cfg(not(windows))] + fn write_hook_script(dir: &tempfile::TempDir, name: &str, content: &str) -> String { + let path = dir.path().join(name); + std::fs::write(&path, content).expect("write hook script"); + format!("sh {}", path.display()) + } + + #[cfg(not(windows))] + fn submit_context(dir: &tempfile::TempDir) -> HookContext { + HookContext::new() + .with_session_id("sess_test") + .with_workspace(dir.path().to_path_buf()) + .with_mode("agent") + .with_model("deepseek-test") + .with_tokens(42) + } + + #[cfg(not(windows))] + #[test] + fn message_submit_transform_applies_hooks_in_order() { + let dir = tempfile::tempdir().expect("tempdir"); + let first = write_hook_script( + &dir, + "first.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"first"}' +"#, + ); + let second = write_hook_script( + &dir, + "second.sh", + r#"#!/bin/sh +payload=$(cat) +case "$payload" in + *'"text":"first"'*) printf '%s\n' '{"text":"first second"}' ;; + *) printf '%s\n' '{"text":"wrong"}' ;; +esac +"#, + ); + let config = HooksConfig { + enabled: true, + hooks: vec![ + Hook::new(HookEvent::MessageSubmit, &first), + Hook::new(HookEvent::MessageSubmit, &second), + ], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Replaced("first second".to_string()) + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_transform_exit_two_blocks_submission() { + let dir = tempfile::tempdir().expect("tempdir"); + let command = write_hook_script( + &dir, + "block.sh", + r#"#!/bin/sh +printf '%s\n' '{"reason":"policy blocked this prompt"}' +exit 2 +"#, + ); + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::MessageSubmit, &command)], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Blocked { + reason: "policy blocked this prompt".to_string() + } + ); + } + + #[cfg(not(windows))] + #[test] + fn background_message_submit_hook_is_observer_only() { + let dir = tempfile::tempdir().expect("tempdir"); + let command = write_hook_script( + &dir, + "background.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"ignored"}' +"#, + ); + let config = HooksConfig { + enabled: true, + hooks: vec![Hook::new(HookEvent::MessageSubmit, &command).background()], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Unchanged + ); + } + + #[test] + fn message_submit_transform_without_configured_hooks_is_unchanged() { + let executor = HookExecutor::new(HooksConfig::default(), PathBuf::from(".")); + + assert_eq!( + executor.execute_message_submit_transform(&HookContext::new(), "original"), + MessageSubmitOutcome::Unchanged + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_transform_skips_non_matching_condition() { + let dir = tempfile::tempdir().expect("tempdir"); + let command = write_hook_script( + &dir, + "replace.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"should not apply"}' +"#, + ); + let hook = + Hook::new(HookEvent::MessageSubmit, &command).with_condition(HookCondition::Mode { + mode: "plan".into(), + }); + let config = HooksConfig { + enabled: true, + hooks: vec![hook], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Unchanged + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_continue_on_error_true_keeps_text_and_runs_later_hooks() { + let dir = tempfile::tempdir().expect("tempdir"); + let failing = write_hook_script( + &dir, + "fail_continue.sh", + r#"#!/bin/sh +printf '%s\n' 'soft failure' >&2 +exit 9 +"#, + ); + let replacing = write_hook_script( + &dir, + "replace_after_failure.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"recovered"}' +"#, + ); + let config = HooksConfig { + enabled: true, + hooks: vec![ + Hook::new(HookEvent::MessageSubmit, &failing), + Hook::new(HookEvent::MessageSubmit, &replacing), + ], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Replaced("recovered".to_string()) + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_invalid_stdout_keeps_text_and_runs_later_hooks() { + let dir = tempfile::tempdir().expect("tempdir"); + let invalid = write_hook_script( + &dir, + "invalid_stdout.sh", + r#"#!/bin/sh +printf '%s\n' 'not json' +"#, + ); + let replacing = write_hook_script( + &dir, + "replace_after_invalid.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"valid later"}' +"#, + ); + let config = HooksConfig { + enabled: true, + hooks: vec![ + Hook::new(HookEvent::MessageSubmit, &invalid), + Hook::new(HookEvent::MessageSubmit, &replacing), + ], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Replaced("valid later".to_string()) + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_continue_on_error_false_blocks_on_failure() { + let dir = tempfile::tempdir().expect("tempdir"); + let command = write_hook_script( + &dir, + "fail.sh", + r#"#!/bin/sh +printf '%s\n' 'hard failure' >&2 +exit 7 +"#, + ); + let mut hook = Hook::new(HookEvent::MessageSubmit, &command); + hook.continue_on_error = false; + let config = HooksConfig { + enabled: true, + hooks: vec![hook], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::Blocked { + reason: "hard failure".to_string() + } + ); + } + #[test] fn has_hooks_for_event_fast_path_returns_false_for_empty_config() { let executor = HookExecutor::disabled(); diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 0f34531ed..7b6c5e7d0 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -4500,19 +4500,33 @@ async fn dispatch_user_message( app: &mut App, config: &Config, engine_handle: &EngineHandle, - message: QueuedMessage, + mut message: QueuedMessage, ) -> Result<()> { - // #455 (observer-only): fire `message_submit` hooks before - // dispatch. Hooks see the user's display text via the - // `with_message` builder. Read-only — they can log, audit, or - // notify but cannot mutate the message that goes to the engine. + // #1364: run mutable `message_submit` hooks before dispatch. Hooks see the + // user's display text and may replace or block it before file mentions, + // skill wrapping, history, and model input are resolved. // Fast-path skip when no hooks configured. if app .hooks .has_hooks_for_event(crate::hooks::HookEvent::MessageSubmit) { let context = app.base_hook_context().with_message(&message.display); - let _ = app.execute_hooks(crate::hooks::HookEvent::MessageSubmit, &context); + match app + .hooks + .execute_message_submit_transform(&context, &message.display) + { + crate::hooks::MessageSubmitOutcome::Unchanged => {} + crate::hooks::MessageSubmitOutcome::Replaced(text) => { + message.display = text; + } + crate::hooks::MessageSubmitOutcome::Blocked { reason } => { + app.status_message = Some(reason); + app.is_loading = false; + app.dispatch_started_at = None; + app.runtime_turn_status = None; + return Ok(()); + } + } } // Set immediately to prevent double-dispatch before TurnStarted event arrives. diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index a666712a8..9fc844825 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -2072,6 +2072,114 @@ async fn dispatch_user_message_failed_send_clears_loading_state() { assert!(app.dispatch_started_at.is_none()); } +#[cfg(not(windows))] +fn write_message_submit_hook(dir: &TempDir, name: &str, body: &str) -> String { + let path = dir.path().join(name); + std::fs::write(&path, body).expect("write message_submit hook"); + format!("sh {}", path.display()) +} + +#[cfg(not(windows))] +fn configure_single_message_submit_hook(app: &mut App, dir: &TempDir, command: String) { + app.hooks = crate::hooks::HookExecutor::new( + crate::hooks::HooksConfig { + enabled: true, + hooks: vec![crate::hooks::Hook::new( + crate::hooks::HookEvent::MessageSubmit, + &command, + )], + working_dir: Some(dir.path().to_path_buf()), + ..crate::hooks::HooksConfig::default() + }, + dir.path().to_path_buf(), + ); +} + +#[cfg(not(windows))] +#[tokio::test] +async fn dispatch_user_message_uses_transformed_message_submit_text() { + let dir = TempDir::new().expect("tempdir"); + let command = write_message_submit_hook( + &dir, + "replace.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"[hooked] hello"}' +"#, + ); + let mut app = create_test_app(); + configure_single_message_submit_hook(&mut app, &dir, command); + let mut engine = crate::core::engine::mock_engine_handle(); + let config = Config::default(); + + dispatch_user_message( + &mut app, + &config, + &engine.handle, + QueuedMessage::new("hello".to_string(), None), + ) + .await + .expect("dispatch user message"); + + assert_eq!(app.last_submitted_prompt.as_deref(), Some("[hooked] hello")); + assert!(app.history.iter().any(|cell| matches!( + cell, + HistoryCell::User { content } if content == "[hooked] hello" + ))); + assert_eq!(app.api_messages.len(), 1); + assert!(matches!( + &app.api_messages[0].content[0], + ContentBlock::Text { text, .. } if text == "[hooked] hello" + )); + match engine.rx_op.recv().await.expect("send message op") { + crate::core::ops::Op::SendMessage { content, .. } => { + assert_eq!(content, "[hooked] hello"); + } + other => panic!("expected SendMessage, got {other:?}"), + } +} + +#[cfg(not(windows))] +#[tokio::test] +async fn dispatch_user_message_blocked_by_message_submit_hook_does_not_start_turn() { + let dir = TempDir::new().expect("tempdir"); + let command = write_message_submit_hook( + &dir, + "block.sh", + r#"#!/bin/sh +printf '%s\n' '{"reason":"blocked by test hook"}' +exit 2 +"#, + ); + let mut app = create_test_app(); + configure_single_message_submit_hook(&mut app, &dir, command); + let mut engine = crate::core::engine::mock_engine_handle(); + let config = Config::default(); + + dispatch_user_message( + &mut app, + &config, + &engine.handle, + QueuedMessage::new("hello".to_string(), None), + ) + .await + .expect("blocked submit is handled locally"); + + assert_eq!(app.status_message.as_deref(), Some("blocked by test hook")); + assert!(app.api_messages.is_empty()); + assert!( + app.history + .iter() + .all(|cell| !matches!(cell, HistoryCell::User { .. })) + ); + assert!(!app.is_loading); + assert!(app.dispatch_started_at.is_none()); + assert!(app.runtime_turn_status.is_none()); + assert!( + engine.rx_op.try_recv().is_err(), + "blocked submit must not send any engine operation" + ); +} + #[test] fn turn_liveness_watchdog_clears_stale_dispatch() { let mut app = create_test_app(); diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 0fe1dd3fe..0132c07bf 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -376,6 +376,55 @@ obvious when hooks are globally suppressed. Hooks are configured under `[[hooks.hooks]]` entries — see the existing hook-system documentation for the full schema. +### Mutable `message_submit` hooks + +`message_submit` hooks run before a submitted message is added to +history or sent to the model. Unlike observer-only lifecycle hooks, +non-background `message_submit` hooks can replace or block the +submitted text. + +```toml +[[hooks.hooks]] +event = "message_submit" +command = "~/.codewhale/hooks/inject-context.sh" +timeout_secs = 2 +continue_on_error = true +``` + +The hook receives JSON on stdin: + +```json +{ + "event": "message_submit", + "text": "original user text", + "session_id": "sess_12345678", + "workspace": "/path/to/workspace", + "mode": "agent", + "model": "deepseek-chat", + "total_tokens": 1234 +} +``` + +If the hook exits `0` and prints JSON with a string `text` field, +that value replaces the submitted text: + +```json +{ "text": "replacement user text" } +``` + +Exit `0` with empty stdout, or stdout JSON without `text`, leaves +the current text unchanged. Exit `2` blocks the submission before +the turn starts; a `reason` field, stderr, or stdout can provide the +status message shown in the TUI. Other non-zero exits follow the +hook's `continue_on_error` setting. + +Multiple `message_submit` hooks run in config order, and each hook +receives the text produced by the previous hook. Hooks marked +`background = true` are observer-only and cannot transform or block +the message. Existing environment variables remain available. +`shell_env` hooks keep their existing `KEY=VALUE` stdout contract; +the JSON stdout contract applies only to `message_submit`. + ### Composer stash (`/stash`, Ctrl+S) Press **Ctrl+S** in the composer to park the current draft to diff --git a/docs/rfcs/1364-hooks-lifecycle.md b/docs/rfcs/1364-hooks-lifecycle.md new file mode 100644 index 000000000..f7f759c11 --- /dev/null +++ b/docs/rfcs/1364-hooks-lifecycle.md @@ -0,0 +1,276 @@ +# RFC: Hook Lifecycle Data Flow + +**Issue:** #1364 +**Status:** Draft +**Date:** 2026-05-28 + +## 1. Problem + +CodeWhale already has lifecycle hooks and MCP support, but the current hook +surface is mostly observer-only. This blocks portable extensions that need to +participate in the agent data flow: + +- memory/context injection before a user message reaches the model +- post-turn background analysis that prepares context for the next turn +- sub-agent lifecycle visibility for orchestration and audit extensions + +The current `message_submit` event fires before dispatch, but its output is +ignored. `TurnComplete`, `AgentSpawned`, and `AgentComplete` exist internally, +but they are not exposed as configurable hook events. + +## 2. PR split + +This issue should be implemented as three PRs. Each PR should be independently +reviewable and should leave the hook system in a useful state. + +### PR 1: Mutable `message_submit` + +Add a structured hook execution path for `message_submit` that can transform or +block the user's submitted text before it is sent to the engine. + +Scope: + +- keep the existing `[[hooks.hooks]]` config shape +- pass a JSON payload to the hook on stdin +- interpret stdout JSON containing `text` as the replacement user text +- treat exit code `2` as an intentional block +- run multiple submit hooks serially in config order +- keep existing env vars for compatibility +- keep `shell_env` stdout parsing unchanged + +Non-goals: + +- no tool argument mutation +- no global stdout JSON semantics for all hook events +- no transcript or model response mutation + +### PR 2: `turn_end` + +Expose the existing turn completion lifecycle as a hook event. + +Scope: + +- add `HookEvent::TurnEnd` with event name `turn_end` +- fire from the UI's `EngineEvent::TurnComplete` branch after core app state, + usage, cost, notifications, and receipt state have been updated +- pass turn metadata on stdin as JSON +- make failures non-blocking and warn-only +- include a `stop_hook_active` field in the payload, initially `false`, so the + contract can support re-entry protection later + +Non-goals: + +- no change to turn status +- no blocking of user input +- no transcript mutation from `turn_end` + +### PR 3: Subagent lifecycle observer hooks + +Expose subagent start and completion as observer-only hook events. + +Scope: + +- add `HookEvent::SubagentSpawn` with event name `subagent_spawn` +- add `HookEvent::SubagentComplete` with event name `subagent_complete` +- fire from the existing `AgentSpawned` and `AgentComplete` UI branches +- pass subagent metadata on stdin as JSON +- make failures non-blocking and warn-only + +Non-goals: + +- no subagent spawn gating in the first version +- no subagent prompt/result mutation +- no changes to subagent scheduling + +## 3. PR 1 detailed plan + +### 3.1 Contract + +Configuration: + +```toml +[[hooks.hooks]] +event = "message_submit" +command = "~/.deepseek/hooks/inject-memory.sh" +timeout_secs = 2 +continue_on_error = true +``` + +Input payload on stdin: + +```json +{ + "event": "message_submit", + "text": "original user text", + "session_id": "sess_xxxx", + "workspace": "/path/to/workspace", + "mode": "agent", + "model": "deepseek-chat", + "total_tokens": 1234 +} +``` + +Output payload on stdout: + +```json +{ "text": "replacement user text" } +``` + +Rules: + +- exit `0` with stdout JSON containing `text: string` replaces the current text +- exit `0` with empty stdout leaves the current text unchanged +- exit `0` with JSON that does not contain `text` leaves the current text + unchanged +- exit `2` blocks submission before the message is appended to history or sent + to the engine +- other non-zero exits follow `continue_on_error` + - `true`: warn, keep the current text, continue later hooks + - `false`: stop later hooks and block submission with an error message +- `background = true` on `message_submit` remains observer-only and cannot + transform or block submission + +Multiple hooks: + +- hooks run in config order +- each hook receives the latest transformed text +- the final transformed text is the only text used by file mention expansion, + skill wrapping, auto routing, history, and `api_messages` + +### 3.2 Implementation steps + +1. Add structured submit outcome types in `crates/tui/src/hooks.rs`: + +```rust +pub enum MessageSubmitOutcome { + Unchanged, + Replaced(String), + Blocked { reason: String }, +} +``` + +2. Add a stdin-capable sync executor: + +```rust +fn execute_sync_with_stdin( + &self, + hook: &Hook, + env_vars: &HashMap, + stdin_json: &serde_json::Value, +) -> HookResult +``` + +This should reuse the existing timeout, working directory, stdout, stderr, and +error handling behavior from `execute_sync`. + +3. Add a `message_submit` transform entrypoint: + +```rust +pub fn execute_message_submit_transform( + &self, + context: &HookContext, + original_text: &str, +) -> MessageSubmitOutcome +``` + +This method should: + +- filter configured `MessageSubmit` hooks through existing condition matching +- build a JSON payload for each hook using the current text +- run non-background hooks through `execute_sync_with_stdin` +- run background hooks with the existing observer-only path +- parse stdout JSON only for non-background hooks +- return the final text or a block result + +4. Apply the transformed message in `dispatch_user_message`: + +- run the transform before `last_submitted_prompt`, file mentions, history, and + `api_messages` +- create a local mutable `QueuedMessage` or replacement display text +- if blocked, show a status message or toast and return without dispatch + +5. Update `/hooks events`: + +- keep `message_submit` listed +- update description to say it can transform or block user text + +6. Update user-facing docs: + +- document the stdin/stdout contract +- document exit code `2` +- document that `shell_env` still uses `KEY=VALUE` stdout + +### 3.3 Test plan + +Unit tests in `crates/tui/src/hooks.rs`: + +- parses stdout `{"text":"changed"}` as replacement +- empty stdout means unchanged +- JSON without `text` means unchanged +- malformed stdout means unchanged with warning semantics +- exit `2` maps to blocked +- multiple hooks apply transforms in order +- background `message_submit` hook cannot transform +- `continue_on_error = false` blocks on non-zero failure + +TUI integration or focused dispatch tests: + +- transformed text is written to `api_messages` +- transformed text is written to visible history +- transformed text is used by file mention expansion +- blocked submit does not append user history +- blocked submit does not push an API message +- blocked submit leaves loading state false + +Manual smoke test: + +1. Add a config hook that prepends `[hooked] ` to every submitted message. +2. Submit `hello`. +3. Verify the transcript and model input use `[hooked] hello`. +4. Replace the hook with one that exits `2`. +5. Submit `hello`. +6. Verify no turn starts and the TUI shows the block reason. + +## 4. Shared payload conventions + +All new structured hook payloads should include: + +- `event` +- `session_id` +- `workspace` +- `mode` +- `model` + +Event-specific payloads should add only fields that are stable and useful for +extension authors. Avoid leaking secrets, full tool outputs, or unbounded +transcript content in the first version. + +## 5. Compatibility + +- Existing hook config remains valid. +- Existing observer-only hooks keep working. +- Existing env vars remain available. +- `shell_env` keeps its existing stdout `KEY=VALUE` contract. +- Structured stdout is interpreted only by `message_submit` in PR 1. + +## 6. Review checkpoints + +PR 1 should be accepted only if: + +- submit mutation is covered by tests +- submit blocking is covered by tests +- the unchanged path preserves current behavior +- `shell_env` tests still prove the old stdout contract +- the docs clearly mark `message_submit` as the only mutable hook + +PR 2 should be accepted only if: + +- `turn_end` fires after `TurnComplete` app state updates +- failure is warn-only +- payload contains status and usage + +PR 3 should be accepted only if: + +- subagent hooks are observer-only +- failures do not affect subagent lifecycle +- payloads do not include unbounded or secret data diff --git a/web/app/[locale]/docs/page.tsx b/web/app/[locale]/docs/page.tsx index cfe384b50..aacdbc066 100644 --- a/web/app/[locale]/docs/page.tsx +++ b/web/app/[locale]/docs/page.tsx @@ -184,6 +184,11 @@ command = "~/.codewhale/hooks/pre.sh" # / message_submit / mode_change /

完整参考：config.example.toml。

+

+ message_submit hooks run before a user message is sent to the model. A non-background hook can print + {'{"text":"replacement"}'} on stdout to replace the message, or exit with code 2 to block the submission. + shell_env keeps its existing KEY=VALUE stdout contract. +

{/* MCP */} @@ -434,6 +439,11 @@ command = "~/.codewhale/hooks/pre.sh" # / message_submit / mode_change /

Full reference: config.example.toml.

+

+ message_submit hooks run before a user message is sent to the model. A non-background hook can print + {'{"text":"replacement"}'} on stdout to replace the message, or exit with code 2 to block the submission. + shell_env keeps its existing KEY=VALUE stdout contract. +

@@ -547,4 +557,4 @@ command = "~/.codewhale/hooks/pre.sh" # / message_submit / mode_change / )} ); -} \ No newline at end of file +} From 4146ec617ecab1713961c59f07a2fc2b84ad5182 Mon Sep 17 00:00:00 2001 From: ningjingkun Date: Fri, 29 May 2026 11:40:20 +0800 Subject: [PATCH 2/3] fix(hooks): harden message_submit review cases --- crates/tui/src/hooks.rs | 220 ++++++++++++++++++++++++++------- crates/tui/src/tui/ui.rs | 13 +- crates/tui/src/tui/ui/tests.rs | 71 ++++++++++- docs/CONFIGURATION.md | 13 +- web/app/[locale]/docs/page.tsx | 4 +- 5 files changed, 263 insertions(+), 58 deletions(-) diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index 685fea762..84f65a1ba 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -19,6 +19,7 @@ use std::collections::HashMap; use std::io::{Read, Write}; use std::path::PathBuf; use std::process::{Command, Stdio}; +use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; use wait_timeout::ChildExt; @@ -428,13 +429,44 @@ pub struct HookResult { #[derive(Debug, Clone, PartialEq, Eq)] pub enum MessageSubmitOutcome { /// No hook changed the submitted text. - Unchanged, + Unchanged { warning: Option }, /// One or more hooks replaced the submitted text. - Replaced(String), + Replaced { + text: String, + warning: Option, + }, /// A hook intentionally blocked the submission. Blocked { reason: String }, } +impl MessageSubmitOutcome { + pub fn unchanged() -> Self { + Self::Unchanged { warning: None } + } + + pub fn replaced(text: String) -> Self { + Self::Replaced { + text, + warning: None, + } + } + + fn with_warning(self, warning: Option) -> Self { + match self { + Self::Unchanged { .. } => Self::Unchanged { warning }, + Self::Replaced { text, .. } => Self::Replaced { text, warning }, + Self::Blocked { reason } => Self::Blocked { reason }, + } + } + + pub fn warning(&self) -> Option<&str> { + match self { + Self::Unchanged { warning } | Self::Replaced { warning, .. } => warning.as_deref(), + Self::Blocked { .. } => None, + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] enum MessageSubmitStdout { Unchanged, @@ -530,15 +562,16 @@ impl HookExecutor { original_text: &str, ) -> MessageSubmitOutcome { if !self.config.enabled { - return MessageSubmitOutcome::Unchanged; + return MessageSubmitOutcome::unchanged(); } let hooks = self.config.hooks_for_event(HookEvent::MessageSubmit); if hooks.is_empty() { - return MessageSubmitOutcome::Unchanged; + return MessageSubmitOutcome::unchanged(); } let mut current_text = original_text.to_string(); + let mut warning = None; for hook in hooks { let hook_context = context.clone().with_message(¤t_text); @@ -578,6 +611,7 @@ impl HookExecutor { ); if hook.continue_on_error { + warning = message_submit_continue_warning(&result).or(warning); continue; } @@ -607,9 +641,9 @@ impl HookExecutor { } if current_text == original_text { - MessageSubmitOutcome::Unchanged + MessageSubmitOutcome::unchanged().with_warning(warning) } else { - MessageSubmitOutcome::Replaced(current_text) + MessageSubmitOutcome::replaced(current_text).with_warning(warning) } } @@ -847,31 +881,29 @@ impl HookExecutor { } }; - if let (Some(bytes), Some(mut stdin)) = (stdin_bytes, child.stdin.take()) { - let _ = stdin.write_all(&bytes); - let _ = stdin.write_all(b"\n"); - let _ = stdin.flush(); - } - - fn read_pipe(mut pipe: impl Read) -> String { - let mut buf = String::new(); - let _ = pipe.read_to_string(&mut buf); - buf - } + let stdout_reader = child.stdout.take().map(spawn_pipe_reader); + let stderr_reader = child.stderr.take().map(spawn_pipe_reader); + let _stdin_writer = match (stdin_bytes, child.stdin.take()) { + (Some(bytes), Some(stdin)) => Some(spawn_stdin_writer(stdin, bytes)), + _ => None, + }; match child.wait_timeout(timeout) { Ok(Some(status)) => HookResult { name: hook.name.clone(), success: status.success(), exit_code: status.code(), - stdout: child.stdout.take().map(read_pipe).unwrap_or_default(), - stderr: child.stderr.take().map(read_pipe).unwrap_or_default(), + stdout: join_reader(stdout_reader), + stderr: join_reader(stderr_reader), duration: started.elapsed(), error: None, }, Ok(None) => { let _ = child.kill(); let _ = child.wait(); + // Do not join pipe threads on timeout: descendant processes can + // inherit pipe fds, and waiting for those threads would defeat + // the hook timeout we just enforced. HookResult { name: hook.name.clone(), success: false, @@ -882,15 +914,19 @@ impl HookExecutor { error: Some(format!("Hook timed out after {timeout_secs}s")), } } - Err(e) => HookResult { - name: hook.name.clone(), - success: false, - exit_code: None, - stdout: String::new(), - stderr: String::new(), - duration: started.elapsed(), - error: Some(format!("Failed to wait for hook: {e}")), - }, + Err(e) => { + let _ = child.kill(); + let _ = child.wait(); + HookResult { + name: hook.name.clone(), + success: false, + exit_code: None, + stdout: String::new(), + stderr: String::new(), + duration: started.elapsed(), + error: Some(format!("Failed to wait for hook: {e}")), + } + } } } @@ -928,6 +964,28 @@ impl HookExecutor { } } +fn spawn_pipe_reader(mut pipe: impl Read + Send + 'static) -> JoinHandle { + thread::spawn(move || { + let mut buf = String::new(); + let _ = pipe.read_to_string(&mut buf); + buf + }) +} + +fn join_reader(reader: Option>) -> String { + reader + .and_then(|handle| handle.join().ok()) + .unwrap_or_default() +} + +fn spawn_stdin_writer(mut stdin: std::process::ChildStdin, mut bytes: Vec) -> JoinHandle<()> { + thread::spawn(move || { + bytes.push(b'\n'); + let _ = stdin.write_all(&bytes); + let _ = stdin.flush(); + }) +} + fn message_submit_payload(context: &HookContext, text: &str) -> serde_json::Value { json!({ "event": HookEvent::MessageSubmit.as_str(), @@ -956,12 +1014,21 @@ fn parse_message_submit_stdout(stdout: &str) -> MessageSubmitStdout { }; match object.get("text") { - Some(serde_json::Value::String(text)) => MessageSubmitStdout::Replaced(text.clone()), + Some(serde_json::Value::String(text)) if !text.is_empty() => { + MessageSubmitStdout::Replaced(text.clone()) + } + Some(serde_json::Value::String(_)) => { + MessageSubmitStdout::Invalid("stdout `text` field must not be empty".to_string()) + } Some(_) => MessageSubmitStdout::Invalid("stdout `text` field must be a string".to_string()), None => MessageSubmitStdout::Unchanged, } } +fn message_submit_continue_warning(result: &HookResult) -> Option { + result.error.as_deref().and_then(first_non_empty_line) +} + fn message_submit_block_reason(result: &HookResult, fallback: &str) -> String { if let Some(reason) = message_submit_stdout_reason(&result.stdout) { return reason; @@ -995,13 +1062,10 @@ fn first_non_empty_line(text: &str) -> Option { fn truncate_hook_message(message: &str) -> String { const MAX_CHARS: usize = 240; - let mut out = String::new(); - for (idx, ch) in message.chars().enumerate() { - if idx >= MAX_CHARS { - out.push('…'); - return out; - } - out.push(ch); + let mut chars = message.chars(); + let mut out: String = chars.by_ref().take(MAX_CHARS).collect(); + if chars.next().is_some() { + out.push('…'); } out } @@ -1128,6 +1192,14 @@ NOEQUAL line dropped )); } + #[test] + fn parse_message_submit_stdout_rejects_empty_text() { + assert_eq!( + super::parse_message_submit_stdout(r#"{"text":""}"#), + MessageSubmitStdout::Invalid("stdout `text` field must not be empty".to_string()) + ); + } + #[test] fn parse_message_submit_stdout_rejects_non_object_json() { assert!(matches!( @@ -1277,6 +1349,35 @@ NOEQUAL line dropped ); } + #[cfg(not(windows))] + #[test] + fn message_submit_stdin_write_does_not_deadlock_when_hook_writes_first() { + let dir = tempfile::tempdir().expect("tempdir"); + let command = write_hook_script( + &dir, + "write_before_read.sh", + r#"#!/bin/sh +dd if=/dev/zero bs=1024 count=256 2>/dev/null | tr '\000' x +dd if=/dev/zero bs=1024 count=256 2>/dev/null | tr '\000' e >&2 +payload=$(cat) +printf '\ndone:%s\n' "${#payload}" +"#, + ); + let hook = Hook::new(HookEvent::MessageSubmit, &command).with_timeout(5); + let executor = HookExecutor::new(HooksConfig::default(), dir.path().to_path_buf()); + let env_vars = HashMap::new(); + let payload = json!({ + "event": "message_submit", + "text": "x".repeat(256 * 1024), + }); + + let result = executor.execute_sync_with_stdin(&hook, &env_vars, &payload); + + assert!(result.success, "hook should complete: {result:?}"); + assert!(result.stdout.contains("done:"), "stdout was drained"); + assert!(result.stderr.len() >= 256 * 1024, "stderr was drained"); + } + #[test] fn test_executor_session_id() { let executor = HookExecutor::new(HooksConfig::default(), PathBuf::from(".")); @@ -1337,7 +1438,7 @@ esac assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), - MessageSubmitOutcome::Replaced("first second".to_string()) + MessageSubmitOutcome::replaced("first second".to_string()) ); } @@ -1390,7 +1491,7 @@ printf '%s\n' '{"text":"ignored"}' assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), - MessageSubmitOutcome::Unchanged + MessageSubmitOutcome::unchanged() ); } @@ -1400,7 +1501,7 @@ printf '%s\n' '{"text":"ignored"}' assert_eq!( executor.execute_message_submit_transform(&HookContext::new(), "original"), - MessageSubmitOutcome::Unchanged + MessageSubmitOutcome::unchanged() ); } @@ -1429,7 +1530,7 @@ printf '%s\n' '{"text":"should not apply"}' assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), - MessageSubmitOutcome::Unchanged + MessageSubmitOutcome::unchanged() ); } @@ -1465,7 +1566,42 @@ printf '%s\n' '{"text":"recovered"}' assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), - MessageSubmitOutcome::Replaced("recovered".to_string()) + MessageSubmitOutcome::replaced("recovered".to_string()) + ); + } + + #[cfg(not(windows))] + #[test] + fn message_submit_timeout_continue_surfaces_warning_and_runs_later_hooks() { + let dir = tempfile::tempdir().expect("tempdir"); + let slow = write_hook_script( + &dir, + "slow_continue.sh", + r#"#!/bin/sh +sleep 2 +"#, + ); + let replacing = write_hook_script( + &dir, + "replace_after_timeout.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"after timeout"}' +"#, + ); + let mut slow_hook = Hook::new(HookEvent::MessageSubmit, &slow).with_timeout(1); + slow_hook.continue_on_error = true; + let config = HooksConfig { + enabled: true, + hooks: vec![slow_hook, Hook::new(HookEvent::MessageSubmit, &replacing)], + working_dir: Some(dir.path().to_path_buf()), + ..HooksConfig::default() + }; + let executor = HookExecutor::new(config, dir.path().to_path_buf()); + + assert_eq!( + executor.execute_message_submit_transform(&submit_context(&dir), "original"), + MessageSubmitOutcome::replaced("after timeout".to_string()) + .with_warning(Some("Hook timed out after 1s".to_string())) ); } @@ -1500,7 +1636,7 @@ printf '%s\n' '{"text":"valid later"}' assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), - MessageSubmitOutcome::Replaced("valid later".to_string()) + MessageSubmitOutcome::replaced("valid later".to_string()) ); } diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 7b6c5e7d0..f9a87483f 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -4511,12 +4511,15 @@ async fn dispatch_user_message( .has_hooks_for_event(crate::hooks::HookEvent::MessageSubmit) { let context = app.base_hook_context().with_message(&message.display); - match app + let outcome = app .hooks - .execute_message_submit_transform(&context, &message.display) - { - crate::hooks::MessageSubmitOutcome::Unchanged => {} - crate::hooks::MessageSubmitOutcome::Replaced(text) => { + .execute_message_submit_transform(&context, &message.display); + if let Some(warning) = outcome.warning() { + app.status_message = Some(warning.to_string()); + } + match outcome { + crate::hooks::MessageSubmitOutcome::Unchanged { .. } => {} + crate::hooks::MessageSubmitOutcome::Replaced { text, .. } => { message.display = text; } crate::hooks::MessageSubmitOutcome::Blocked { reason } => { diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 9fc844825..a98ad0433 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -2081,13 +2081,20 @@ fn write_message_submit_hook(dir: &TempDir, name: &str, body: &str) -> String { #[cfg(not(windows))] fn configure_single_message_submit_hook(app: &mut App, dir: &TempDir, command: String) { + configure_message_submit_hooks(app, dir, vec![command]); +} + +#[cfg(not(windows))] +fn configure_message_submit_hooks(app: &mut App, dir: &TempDir, commands: Vec) { app.hooks = crate::hooks::HookExecutor::new( crate::hooks::HooksConfig { enabled: true, - hooks: vec![crate::hooks::Hook::new( - crate::hooks::HookEvent::MessageSubmit, - &command, - )], + hooks: commands + .iter() + .map(|command| { + crate::hooks::Hook::new(crate::hooks::HookEvent::MessageSubmit, command) + }) + .collect(), working_dir: Some(dir.path().to_path_buf()), ..crate::hooks::HooksConfig::default() }, @@ -2095,6 +2102,62 @@ fn configure_single_message_submit_hook(app: &mut App, dir: &TempDir, command: S ); } +#[cfg(not(windows))] +#[tokio::test] +async fn dispatch_user_message_surfaces_continued_message_submit_timeout() { + let dir = TempDir::new().expect("tempdir"); + let slow = write_message_submit_hook( + &dir, + "slow.sh", + r#"#!/bin/sh +sleep 2 +"#, + ); + let replacing = write_message_submit_hook( + &dir, + "replace.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"after timeout"}' +"#, + ); + let mut app = create_test_app(); + app.hooks = crate::hooks::HookExecutor::new( + crate::hooks::HooksConfig { + enabled: true, + hooks: vec![ + crate::hooks::Hook::new(crate::hooks::HookEvent::MessageSubmit, &slow) + .with_timeout(1), + crate::hooks::Hook::new(crate::hooks::HookEvent::MessageSubmit, &replacing), + ], + working_dir: Some(dir.path().to_path_buf()), + ..crate::hooks::HooksConfig::default() + }, + dir.path().to_path_buf(), + ); + let mut engine = crate::core::engine::mock_engine_handle(); + let config = Config::default(); + + dispatch_user_message( + &mut app, + &config, + &engine.handle, + QueuedMessage::new("hello".to_string(), None), + ) + .await + .expect("dispatch user message"); + + assert_eq!( + app.status_message.as_deref(), + Some("Hook timed out after 1s") + ); + match engine.rx_op.recv().await.expect("send message op") { + crate::core::ops::Op::SendMessage { content, .. } => { + assert_eq!(content, "after timeout"); + } + other => panic!("expected SendMessage, got {other:?}"), + } +} + #[cfg(not(windows))] #[tokio::test] async fn dispatch_user_message_uses_transformed_message_submit_text() { diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 0132c07bf..6377b7fb8 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -405,7 +405,7 @@ The hook receives JSON on stdin: } ``` -If the hook exits `0` and prints JSON with a string `text` field, +If the hook exits `0` and prints JSON with a non-empty string `text` field, that value replaces the submitted text: ```json @@ -413,10 +413,13 @@ that value replaces the submitted text: ``` Exit `0` with empty stdout, or stdout JSON without `text`, leaves -the current text unchanged. Exit `2` blocks the submission before -the turn starts; a `reason` field, stderr, or stdout can provide the -status message shown in the TUI. Other non-zero exits follow the -hook's `continue_on_error` setting. +the current text unchanged. A JSON `text` field must not be empty; +`{"text":""}` is treated as invalid stdout and ignored. Exit `2` +blocks the submission before the turn starts; a `reason` field, +stderr, or stdout can provide the status message shown in the TUI. +Other non-zero exits follow the hook's `continue_on_error` setting. +Timeouts and spawn failures are also surfaced as transient TUI status +messages when `continue_on_error = true` lets submission continue. Multiple `message_submit` hooks run in config order, and each hook receives the text produced by the previous hook. Hooks marked diff --git a/web/app/[locale]/docs/page.tsx b/web/app/[locale]/docs/page.tsx index aacdbc066..f141001a0 100644 --- a/web/app/[locale]/docs/page.tsx +++ b/web/app/[locale]/docs/page.tsx @@ -186,7 +186,7 @@ command = "~/.codewhale/hooks/pre.sh" # / message_submit / mode_change /

message_submit hooks run before a user message is sent to the model. A non-background hook can print - {'{"text":"replacement"}'} on stdout to replace the message, or exit with code 2 to block the submission. + {'{"text":"replacement"}'} on stdout to replace the message; text must be non-empty. Exit with code 2 to block the submission. shell_env keeps its existing KEY=VALUE stdout contract.

@@ -441,7 +441,7 @@ command = "~/.codewhale/hooks/pre.sh" # / message_submit / mode_change /

message_submit hooks run before a user message is sent to the model. A non-background hook can print - {'{"text":"replacement"}'} on stdout to replace the message, or exit with code 2 to block the submission. + {'{"text":"replacement"}'} on stdout to replace the message; text must be non-empty. Exit with code 2 to block the submission. shell_env keeps its existing KEY=VALUE stdout contract.

From 0d3b81db2ee3a094cf4bbd4770509c2c1d250bf3 Mon Sep 17 00:00:00 2001 From: Hunter B Date: Sun, 31 May 2026 04:43:54 -0700 Subject: [PATCH 3/3] fix(hooks): surface continued submit hook stderr --- crates/tui/src/hooks.rs | 6 ++++- crates/tui/src/tui/ui/tests.rs | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/crates/tui/src/hooks.rs b/crates/tui/src/hooks.rs index 84f65a1ba..e5715dc2c 100644 --- a/crates/tui/src/hooks.rs +++ b/crates/tui/src/hooks.rs @@ -1026,7 +1026,10 @@ fn parse_message_submit_stdout(stdout: &str) -> MessageSubmitStdout { } fn message_submit_continue_warning(result: &HookResult) -> Option { - result.error.as_deref().and_then(first_non_empty_line) + message_submit_stdout_reason(&result.stdout) + .or_else(|| first_non_empty_line(&result.stderr)) + .or_else(|| first_non_empty_line(&result.stdout)) + .or_else(|| result.error.as_deref().and_then(first_non_empty_line)) } fn message_submit_block_reason(result: &HookResult, fallback: &str) -> String { @@ -1567,6 +1570,7 @@ printf '%s\n' '{"text":"recovered"}' assert_eq!( executor.execute_message_submit_transform(&submit_context(&dir), "original"), MessageSubmitOutcome::replaced("recovered".to_string()) + .with_warning(Some("soft failure".to_string())) ); } diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index a98ad0433..535f20e80 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -2158,6 +2158,48 @@ printf '%s\n' '{"text":"after timeout"}' } } +#[cfg(not(windows))] +#[tokio::test] +async fn dispatch_user_message_surfaces_continued_message_submit_stderr() { + let dir = TempDir::new().expect("tempdir"); + let failing = write_message_submit_hook( + &dir, + "fail.sh", + r#"#!/bin/sh +printf '%s\n' 'soft failure' >&2 +exit 9 +"#, + ); + let replacing = write_message_submit_hook( + &dir, + "replace.sh", + r#"#!/bin/sh +printf '%s\n' '{"text":"after soft failure"}' +"#, + ); + let mut app = create_test_app(); + configure_message_submit_hooks(&mut app, &dir, vec![failing, replacing]); + let mut engine = crate::core::engine::mock_engine_handle(); + let config = Config::default(); + + dispatch_user_message( + &mut app, + &config, + &engine.handle, + QueuedMessage::new("hello".to_string(), None), + ) + .await + .expect("dispatch user message"); + + assert_eq!(app.status_message.as_deref(), Some("soft failure")); + match engine.rx_op.recv().await.expect("send message op") { + crate::core::ops::Op::SendMessage { content, .. } => { + assert_eq!(content, "after soft failure"); + } + other => panic!("expected SendMessage, got {other:?}"), + } +} + #[cfg(not(windows))] #[tokio::test] async fn dispatch_user_message_uses_transformed_message_submit_text() {