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
This commit is contained in:
Yeachan-Heo
2026-04-13 12:44:52 +00:00
parent 6a957560bd
commit e874bc6a44

View File

@@ -1,4 +1,5 @@
use std::ffi::OsStr; use std::ffi::OsStr;
use std::fmt::Write as FmtWrite;
use std::io::Write; use std::io::Write;
use std::process::{Command, Stdio}; use std::process::{Command, Stdio};
use std::sync::{ use std::sync::{
@@ -13,6 +14,8 @@ use serde_json::{json, Value};
use crate::config::{RuntimeFeatureConfig, RuntimeHookConfig}; use crate::config::{RuntimeFeatureConfig, RuntimeHookConfig};
use crate::permissions::PermissionOverride; use crate::permissions::PermissionOverride;
const HOOK_PREVIEW_CHAR_LIMIT: usize = 160;
pub type HookPermissionDecision = PermissionOverride; pub type HookPermissionDecision = PermissionOverride;
#[derive(Debug, Clone, Copy, PartialEq, Eq)] #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -437,7 +440,7 @@ impl HookRunner {
Ok(CommandExecution::Finished(output)) => { Ok(CommandExecution::Finished(output)) => {
let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string();
let stderr = String::from_utf8_lossy(&output.stderr).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); let primary_message = parsed.primary_message().map(ToOwned::to_owned);
match output.status.code() { match output.status.code() {
Some(0) => { 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() { if stdout.is_empty() {
return ParsedHookOutput::default(); return ParsedHookOutput::default();
} }
let Ok(Value::Object(root)) = serde_json::from_str::<Value>(stdout) else { let root = match serde_json::from_str::<Value>(stdout) {
return ParsedHookOutput { Ok(Value::Object(root)) => root,
messages: vec![stdout.to_string()], Ok(value) => {
..ParsedHookOutput::default() 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(); 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 })) 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(|| "<empty>".to_string());
let stderr_preview = bounded_hook_preview(stderr).unwrap_or_else(|| "<empty>".to_string());
let command_preview = bounded_hook_preview(command).unwrap_or_else(|| "<empty>".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<String> {
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 { fn format_hook_failure(command: &str, code: i32, stdout: Option<&str>, stderr: &str) -> String {
let mut message = format!("Hook `{command}` exited with status {code}"); let mut message = format!("Hook `{command}` exited with status {code}");
if let Some(stdout) = stdout.filter(|stdout| !stdout.is_empty()) { if let Some(stdout) = stdout.filter(|stdout| !stdout.is_empty()) {
@@ -935,6 +1039,31 @@ mod tests {
assert!(!result.messages().iter().any(|message| message == "later")); 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] #[test]
fn abort_signal_cancels_long_running_hook_and_reports_progress() { fn abort_signal_cancels_long_running_hook_and_reports_progress() {
let runner = HookRunner::new(RuntimeHookConfig::new( let runner = HookRunner::new(RuntimeHookConfig::new(