From e874bc6a4467158d91d644783c497c8eca472874 Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Mon, 13 Apr 2026 12:44:52 +0000 Subject: [PATCH] Improve malformed hook failures so operators can diagnose broken JSON Malformed hook stdout that looks like JSON was collapsing into low-signal failure text during hook execution. This change preserves plain-text hook feedback for normal text hooks, but upgrades malformed JSON-like output into an explicit hook_invalid_json diagnostic that includes phase, tool, command, and bounded stdout/stderr previews. It also adds a regression test for malformed-but-nonempty output. Constraint: User scoped the implementation to rust/crates/runtime/src/hooks.rs and tests only Constraint: Existing plain-text hook feedback must remain intact for non-JSON hook output Rejected: Treat every non-JSON stdout payload as invalid JSON | would break legitimate plain-text hook feedback Confidence: high Scope-risk: narrow Directive: Keep malformed-hook diagnostics bounded and preserve the plain-text fallback for hooks that intentionally emit text Tested: cargo test --manifest-path rust/Cargo.toml -p runtime hooks::tests:: -- --nocapture Tested: cargo test --manifest-path rust/Cargo.toml -p runtime -- --nocapture Tested: cargo clippy --manifest-path rust/Cargo.toml -p runtime --all-targets -- -D warnings Not-tested: Full workspace clippy/test sweep outside runtime crate --- rust/crates/runtime/src/hooks.rs | 143 +++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 7 deletions(-) diff --git a/rust/crates/runtime/src/hooks.rs b/rust/crates/runtime/src/hooks.rs index c6970c0..6abd69f 100644 --- a/rust/crates/runtime/src/hooks.rs +++ b/rust/crates/runtime/src/hooks.rs @@ -1,4 +1,5 @@ use std::ffi::OsStr; +use std::fmt::Write as FmtWrite; use std::io::Write; use std::process::{Command, Stdio}; use std::sync::{ @@ -13,6 +14,8 @@ use serde_json::{json, Value}; use crate::config::{RuntimeFeatureConfig, RuntimeHookConfig}; use crate::permissions::PermissionOverride; +const HOOK_PREVIEW_CHAR_LIMIT: usize = 160; + pub type HookPermissionDecision = PermissionOverride; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -437,7 +440,7 @@ impl HookRunner { Ok(CommandExecution::Finished(output)) => { let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - let parsed = parse_hook_output(&stdout); + let parsed = parse_hook_output(event, tool_name, command, &stdout, &stderr); let primary_message = parsed.primary_message().map(ToOwned::to_owned); match output.status.code() { Some(0) => { @@ -532,16 +535,54 @@ fn merge_parsed_hook_output(target: &mut HookRunResult, parsed: ParsedHookOutput } } -fn parse_hook_output(stdout: &str) -> ParsedHookOutput { +fn parse_hook_output( + event: HookEvent, + tool_name: &str, + command: &str, + stdout: &str, + stderr: &str, +) -> ParsedHookOutput { if stdout.is_empty() { return ParsedHookOutput::default(); } - let Ok(Value::Object(root)) = serde_json::from_str::(stdout) else { - return ParsedHookOutput { - messages: vec![stdout.to_string()], - ..ParsedHookOutput::default() - }; + let root = match serde_json::from_str::(stdout) { + Ok(Value::Object(root)) => root, + Ok(value) => { + return ParsedHookOutput { + messages: vec![format_invalid_hook_output( + event, + tool_name, + command, + &format!( + "expected top-level JSON object, got {}", + json_type_name(&value) + ), + stdout, + stderr, + )], + ..ParsedHookOutput::default() + }; + } + Err(error) if looks_like_json_attempt(stdout) => { + return ParsedHookOutput { + messages: vec![format_invalid_hook_output( + event, + tool_name, + command, + &error.to_string(), + stdout, + stderr, + )], + ..ParsedHookOutput::default() + }; + } + Err(_) => { + return ParsedHookOutput { + messages: vec![stdout.to_string()], + ..ParsedHookOutput::default() + }; + } }; let mut parsed = ParsedHookOutput::default(); @@ -619,6 +660,69 @@ fn parse_tool_input(tool_input: &str) -> Value { serde_json::from_str(tool_input).unwrap_or_else(|_| json!({ "raw": tool_input })) } +fn format_invalid_hook_output( + event: HookEvent, + tool_name: &str, + command: &str, + detail: &str, + stdout: &str, + stderr: &str, +) -> String { + let stdout_preview = bounded_hook_preview(stdout).unwrap_or_else(|| "".to_string()); + let stderr_preview = bounded_hook_preview(stderr).unwrap_or_else(|| "".to_string()); + let command_preview = bounded_hook_preview(command).unwrap_or_else(|| "".to_string()); + + format!( + "hook_invalid_json: phase={} tool={} command={} detail={} stdout_preview={} stderr_preview={}", + event.as_str(), + tool_name, + command_preview, + detail, + stdout_preview, + stderr_preview + ) +} + +fn bounded_hook_preview(value: &str) -> Option { + let trimmed = value.trim(); + if trimmed.is_empty() { + return None; + } + + let mut preview = String::new(); + for (count, ch) in trimmed.chars().enumerate() { + if count == HOOK_PREVIEW_CHAR_LIMIT { + preview.push('…'); + break; + } + match ch { + '\n' => preview.push_str("\\n"), + '\r' => preview.push_str("\\r"), + '\t' => preview.push_str("\\t"), + control if control.is_control() => { + let _ = write!(&mut preview, "\\u{{{:x}}}", control as u32); + } + _ => preview.push(ch), + } + } + Some(preview) +} + +fn json_type_name(value: &Value) -> &'static str { + match value { + Value::Null => "null", + Value::Bool(_) => "boolean", + Value::Number(_) => "number", + Value::String(_) => "string", + Value::Array(_) => "array", + Value::Object(_) => "object", + } +} + +fn looks_like_json_attempt(value: &str) -> bool { + matches!(value.trim_start().chars().next(), Some('{' | '[')) +} + fn format_hook_failure(command: &str, code: i32, stdout: Option<&str>, stderr: &str) -> String { let mut message = format!("Hook `{command}` exited with status {code}"); if let Some(stdout) = stdout.filter(|stdout| !stdout.is_empty()) { @@ -935,6 +1039,31 @@ mod tests { assert!(!result.messages().iter().any(|message| message == "later")); } + #[test] + fn malformed_nonempty_hook_output_reports_explicit_diagnostic_with_previews() { + let runner = HookRunner::new(RuntimeHookConfig::new( + vec![shell_snippet( + "printf '{not-json\nsecond line'; printf 'stderr warning' >&2; exit 1", + )], + Vec::new(), + Vec::new(), + )); + + let result = runner.run_pre_tool_use("Edit", r#"{"file":"src/lib.rs"}"#); + + assert!(result.is_failed()); + let rendered = result.messages().join("\n"); + assert!(rendered.contains("hook_invalid_json:")); + assert!(rendered.contains("phase=PreToolUse")); + assert!(rendered.contains("tool=Edit")); + assert!(rendered.contains("command=printf '{not-json")); + assert!(rendered.contains("printf 'stderr warning' >&2; exit 1")); + assert!(rendered.contains("detail=key must be a string")); + assert!(rendered.contains("stdout_preview={not-json")); + assert!(rendered.contains("second line stderr_preview=stderr warning")); + assert!(rendered.contains("stderr_preview=stderr warning")); + } + #[test] fn abort_signal_cancels_long_running_hook_and_reports_progress() { let runner = HookRunner::new(RuntimeHookConfig::new(