mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-13 03:24:49 +08:00
Two bugs found in the plugin hook test harness that together caused Linux CI to fail on 'hooks::tests::collects_and_runs_hooks_from_enabled_plugins' with 'Broken pipe (os error 32)'. Three reproductions plus one rerun failure on main today: 24120271422, 24120538408, 24121392171. Root cause 1 (chmod, defense-in-depth): write_hook_plugin writes pre.sh/post.sh/failure.sh via fs::write without setting the execute bit. While the runtime hook runner invokes hooks via 'sh <path>' (so the script file does not strictly need +x), missing exec perms can cause subtle fork/exec races on Linux in edge cases. Root cause 2 (the actual CI failure): output_with_stdin unconditionally propagated write_all errors on the child's stdin pipe, including BrokenPipe. A hook script that runs to completion in microseconds (e.g. a one-line printf) can exit and close its stdin before the parent finishes writing the JSON payload. Linux pipes surface this as EPIPE immediately; macOS pipes happen to buffer the small payload, so the race only shows on ubuntu CI runners. The parent's write_all raised BrokenPipe, which output_with_stdin returned as Err, which run_command classified as 'failed to start', making the test assertion fail. Fix: (a) make_executable helper sets mode 0o755 via PermissionsExt on each generated hook script, with a #[cfg(unix)] gate and a no-op #[cfg(not(unix))] branch. (b) output_with_stdin now matches the write_all result and swallows BrokenPipe specifically (the child still ran; wait_with_output still captures stdout/stderr/exit code), while propagating all other write errors. (c) New regression guard generated_hook_scripts_are_executable under #[cfg(unix)] asserts each generated .sh file has at least one execute bit set. Surgical scope per gaebal-gajae's direction: chmod + pipe tolerance + regression guard only. The deeper plugin-test sealing pass for ROADMAP #25 + #27 stays in gaebal-gajae's OMX lane. Verification: - cargo test --release -p plugins → 35 passing, 0 failing - cargo fmt -p plugins → clean - cargo clippy -p plugins -- -D warnings → clean Co-authored-by: gaebal-gajae <gaebal-gajae@layofflabs.com>
565 lines
17 KiB
Rust
565 lines
17 KiB
Rust
use std::ffi::OsStr;
|
|
use std::path::Path;
|
|
use std::process::Command;
|
|
|
|
use serde_json::json;
|
|
|
|
use crate::{PluginError, PluginHooks, PluginRegistry};
|
|
|
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
|
pub enum HookEvent {
|
|
PreToolUse,
|
|
PostToolUse,
|
|
PostToolUseFailure,
|
|
}
|
|
|
|
impl HookEvent {
|
|
fn as_str(self) -> &'static str {
|
|
match self {
|
|
Self::PreToolUse => "PreToolUse",
|
|
Self::PostToolUse => "PostToolUse",
|
|
Self::PostToolUseFailure => "PostToolUseFailure",
|
|
}
|
|
}
|
|
}
|
|
|
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
|
pub struct HookRunResult {
|
|
denied: bool,
|
|
failed: bool,
|
|
messages: Vec<String>,
|
|
}
|
|
|
|
impl HookRunResult {
|
|
#[must_use]
|
|
pub fn allow(messages: Vec<String>) -> Self {
|
|
Self {
|
|
denied: false,
|
|
failed: false,
|
|
messages,
|
|
}
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn is_denied(&self) -> bool {
|
|
self.denied
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn is_failed(&self) -> bool {
|
|
self.failed
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn messages(&self) -> &[String] {
|
|
&self.messages
|
|
}
|
|
}
|
|
|
|
#[derive(Debug, Clone, PartialEq, Eq, Default)]
|
|
pub struct HookRunner {
|
|
hooks: PluginHooks,
|
|
}
|
|
|
|
impl HookRunner {
|
|
#[must_use]
|
|
pub fn new(hooks: PluginHooks) -> Self {
|
|
Self { hooks }
|
|
}
|
|
|
|
pub fn from_registry(plugin_registry: &PluginRegistry) -> Result<Self, PluginError> {
|
|
Ok(Self::new(plugin_registry.aggregated_hooks()?))
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn run_pre_tool_use(&self, tool_name: &str, tool_input: &str) -> HookRunResult {
|
|
Self::run_commands(
|
|
HookEvent::PreToolUse,
|
|
&self.hooks.pre_tool_use,
|
|
tool_name,
|
|
tool_input,
|
|
None,
|
|
false,
|
|
)
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn run_post_tool_use(
|
|
&self,
|
|
tool_name: &str,
|
|
tool_input: &str,
|
|
tool_output: &str,
|
|
is_error: bool,
|
|
) -> HookRunResult {
|
|
Self::run_commands(
|
|
HookEvent::PostToolUse,
|
|
&self.hooks.post_tool_use,
|
|
tool_name,
|
|
tool_input,
|
|
Some(tool_output),
|
|
is_error,
|
|
)
|
|
}
|
|
|
|
#[must_use]
|
|
pub fn run_post_tool_use_failure(
|
|
&self,
|
|
tool_name: &str,
|
|
tool_input: &str,
|
|
tool_error: &str,
|
|
) -> HookRunResult {
|
|
Self::run_commands(
|
|
HookEvent::PostToolUseFailure,
|
|
&self.hooks.post_tool_use_failure,
|
|
tool_name,
|
|
tool_input,
|
|
Some(tool_error),
|
|
true,
|
|
)
|
|
}
|
|
|
|
fn run_commands(
|
|
event: HookEvent,
|
|
commands: &[String],
|
|
tool_name: &str,
|
|
tool_input: &str,
|
|
tool_output: Option<&str>,
|
|
is_error: bool,
|
|
) -> HookRunResult {
|
|
if commands.is_empty() {
|
|
return HookRunResult::allow(Vec::new());
|
|
}
|
|
|
|
let payload = hook_payload(event, tool_name, tool_input, tool_output, is_error).to_string();
|
|
|
|
let mut messages = Vec::new();
|
|
|
|
for command in commands {
|
|
match Self::run_command(
|
|
command,
|
|
event,
|
|
tool_name,
|
|
tool_input,
|
|
tool_output,
|
|
is_error,
|
|
&payload,
|
|
) {
|
|
HookCommandOutcome::Allow { message } => {
|
|
if let Some(message) = message {
|
|
messages.push(message);
|
|
}
|
|
}
|
|
HookCommandOutcome::Deny { message } => {
|
|
messages.push(message.unwrap_or_else(|| {
|
|
format!("{} hook denied tool `{tool_name}`", event.as_str())
|
|
}));
|
|
return HookRunResult {
|
|
denied: true,
|
|
failed: false,
|
|
messages,
|
|
};
|
|
}
|
|
HookCommandOutcome::Failed { message } => {
|
|
messages.push(message);
|
|
return HookRunResult {
|
|
denied: false,
|
|
failed: true,
|
|
messages,
|
|
};
|
|
}
|
|
}
|
|
}
|
|
|
|
HookRunResult::allow(messages)
|
|
}
|
|
|
|
#[allow(clippy::too_many_arguments)]
|
|
fn run_command(
|
|
command: &str,
|
|
event: HookEvent,
|
|
tool_name: &str,
|
|
tool_input: &str,
|
|
tool_output: Option<&str>,
|
|
is_error: bool,
|
|
payload: &str,
|
|
) -> HookCommandOutcome {
|
|
let mut child = shell_command(command);
|
|
child.stdin(std::process::Stdio::piped());
|
|
child.stdout(std::process::Stdio::piped());
|
|
child.stderr(std::process::Stdio::piped());
|
|
child.env("HOOK_EVENT", event.as_str());
|
|
child.env("HOOK_TOOL_NAME", tool_name);
|
|
child.env("HOOK_TOOL_INPUT", tool_input);
|
|
child.env("HOOK_TOOL_IS_ERROR", if is_error { "1" } else { "0" });
|
|
if let Some(tool_output) = tool_output {
|
|
child.env("HOOK_TOOL_OUTPUT", tool_output);
|
|
}
|
|
|
|
match child.output_with_stdin(payload.as_bytes()) {
|
|
Ok(output) => {
|
|
let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string();
|
|
let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
|
|
let message = (!stdout.is_empty()).then_some(stdout);
|
|
match output.status.code() {
|
|
Some(0) => HookCommandOutcome::Allow { message },
|
|
Some(2) => HookCommandOutcome::Deny { message },
|
|
Some(code) => HookCommandOutcome::Failed {
|
|
message: format_hook_warning(
|
|
command,
|
|
code,
|
|
message.as_deref(),
|
|
stderr.as_str(),
|
|
),
|
|
},
|
|
None => HookCommandOutcome::Failed {
|
|
message: format!(
|
|
"{} hook `{command}` terminated by signal while handling `{tool_name}`",
|
|
event.as_str()
|
|
),
|
|
},
|
|
}
|
|
}
|
|
Err(error) => HookCommandOutcome::Failed {
|
|
message: format!(
|
|
"{} hook `{command}` failed to start for `{tool_name}`: {error}",
|
|
event.as_str()
|
|
),
|
|
},
|
|
}
|
|
}
|
|
}
|
|
|
|
enum HookCommandOutcome {
|
|
Allow { message: Option<String> },
|
|
Deny { message: Option<String> },
|
|
Failed { message: String },
|
|
}
|
|
|
|
fn hook_payload(
|
|
event: HookEvent,
|
|
tool_name: &str,
|
|
tool_input: &str,
|
|
tool_output: Option<&str>,
|
|
is_error: bool,
|
|
) -> serde_json::Value {
|
|
match event {
|
|
HookEvent::PostToolUseFailure => json!({
|
|
"hook_event_name": event.as_str(),
|
|
"tool_name": tool_name,
|
|
"tool_input": parse_tool_input(tool_input),
|
|
"tool_input_json": tool_input,
|
|
"tool_error": tool_output,
|
|
"tool_result_is_error": true,
|
|
}),
|
|
_ => json!({
|
|
"hook_event_name": event.as_str(),
|
|
"tool_name": tool_name,
|
|
"tool_input": parse_tool_input(tool_input),
|
|
"tool_input_json": tool_input,
|
|
"tool_output": tool_output,
|
|
"tool_result_is_error": is_error,
|
|
}),
|
|
}
|
|
}
|
|
|
|
fn parse_tool_input(tool_input: &str) -> serde_json::Value {
|
|
serde_json::from_str(tool_input).unwrap_or_else(|_| json!({ "raw": tool_input }))
|
|
}
|
|
|
|
fn format_hook_warning(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()) {
|
|
message.push_str(": ");
|
|
message.push_str(stdout);
|
|
} else if !stderr.is_empty() {
|
|
message.push_str(": ");
|
|
message.push_str(stderr);
|
|
}
|
|
message
|
|
}
|
|
|
|
fn shell_command(command: &str) -> CommandWithStdin {
|
|
#[cfg(windows)]
|
|
let command_builder = {
|
|
let mut command_builder = Command::new("cmd");
|
|
command_builder.arg("/C").arg(command);
|
|
CommandWithStdin::new(command_builder)
|
|
};
|
|
|
|
#[cfg(not(windows))]
|
|
let command_builder = if Path::new(command).exists() {
|
|
let mut command_builder = Command::new("sh");
|
|
command_builder.arg(command);
|
|
CommandWithStdin::new(command_builder)
|
|
} else {
|
|
let mut command_builder = Command::new("sh");
|
|
command_builder.arg("-lc").arg(command);
|
|
CommandWithStdin::new(command_builder)
|
|
};
|
|
|
|
command_builder
|
|
}
|
|
|
|
struct CommandWithStdin {
|
|
command: Command,
|
|
}
|
|
|
|
impl CommandWithStdin {
|
|
fn new(command: Command) -> Self {
|
|
Self { command }
|
|
}
|
|
|
|
fn stdin(&mut self, cfg: std::process::Stdio) -> &mut Self {
|
|
self.command.stdin(cfg);
|
|
self
|
|
}
|
|
|
|
fn stdout(&mut self, cfg: std::process::Stdio) -> &mut Self {
|
|
self.command.stdout(cfg);
|
|
self
|
|
}
|
|
|
|
fn stderr(&mut self, cfg: std::process::Stdio) -> &mut Self {
|
|
self.command.stderr(cfg);
|
|
self
|
|
}
|
|
|
|
fn env<K, V>(&mut self, key: K, value: V) -> &mut Self
|
|
where
|
|
K: AsRef<OsStr>,
|
|
V: AsRef<OsStr>,
|
|
{
|
|
self.command.env(key, value);
|
|
self
|
|
}
|
|
|
|
fn output_with_stdin(&mut self, stdin: &[u8]) -> std::io::Result<std::process::Output> {
|
|
let mut child = self.command.spawn()?;
|
|
if let Some(mut child_stdin) = child.stdin.take() {
|
|
use std::io::Write as _;
|
|
// Tolerate BrokenPipe: a hook script that runs to completion
|
|
// (or exits early without reading stdin) closes its stdin
|
|
// before the parent finishes writing the JSON payload, and
|
|
// the kernel raises EPIPE on the parent's write_all. That is
|
|
// not a hook failure — the child still exited cleanly and we
|
|
// still need to wait_with_output() to capture stdout/stderr
|
|
// and the real exit code. Other write errors (e.g. EIO,
|
|
// permission, OOM) still propagate.
|
|
//
|
|
// This was the root cause of the Linux CI flake on
|
|
// hooks::tests::collects_and_runs_hooks_from_enabled_plugins
|
|
// (ROADMAP #25, runs 24120271422 / 24120538408 / 24121392171
|
|
// / 24121776826): the test hook scripts run in microseconds
|
|
// and the parent's stdin write races against child exit.
|
|
// macOS pipes happen to buffer the small payload before the
|
|
// child exits; Linux pipes do not, so the race shows up
|
|
// deterministically on ubuntu runners.
|
|
match child_stdin.write_all(stdin) {
|
|
Ok(()) => {}
|
|
Err(error) if error.kind() == std::io::ErrorKind::BrokenPipe => {}
|
|
Err(error) => return Err(error),
|
|
}
|
|
}
|
|
child.wait_with_output()
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::{HookRunResult, HookRunner};
|
|
use crate::{PluginManager, PluginManagerConfig};
|
|
use std::fs;
|
|
use std::path::{Path, PathBuf};
|
|
use std::time::{SystemTime, UNIX_EPOCH};
|
|
|
|
fn temp_dir(label: &str) -> PathBuf {
|
|
let nanos = SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.expect("time should be after epoch")
|
|
.as_nanos();
|
|
std::env::temp_dir().join(format!("plugins-hook-runner-{label}-{nanos}"))
|
|
}
|
|
|
|
fn make_executable(path: &Path) {
|
|
#[cfg(unix)]
|
|
{
|
|
use std::os::unix::fs::PermissionsExt;
|
|
let perms = fs::Permissions::from_mode(0o755);
|
|
fs::set_permissions(path, perms)
|
|
.unwrap_or_else(|e| panic!("chmod +x {}: {e}", path.display()));
|
|
}
|
|
#[cfg(not(unix))]
|
|
let _ = path;
|
|
}
|
|
|
|
fn write_hook_plugin(
|
|
root: &Path,
|
|
name: &str,
|
|
pre_message: &str,
|
|
post_message: &str,
|
|
failure_message: &str,
|
|
) {
|
|
fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
|
|
fs::create_dir_all(root.join("hooks")).expect("hooks dir");
|
|
|
|
let pre_path = root.join("hooks").join("pre.sh");
|
|
fs::write(
|
|
&pre_path,
|
|
format!("#!/bin/sh\nprintf '%s\\n' '{pre_message}'\n"),
|
|
)
|
|
.expect("write pre hook");
|
|
make_executable(&pre_path);
|
|
|
|
let post_path = root.join("hooks").join("post.sh");
|
|
fs::write(
|
|
&post_path,
|
|
format!("#!/bin/sh\nprintf '%s\\n' '{post_message}'\n"),
|
|
)
|
|
.expect("write post hook");
|
|
make_executable(&post_path);
|
|
|
|
let failure_path = root.join("hooks").join("failure.sh");
|
|
fs::write(
|
|
&failure_path,
|
|
format!("#!/bin/sh\nprintf '%s\\n' '{failure_message}'\n"),
|
|
)
|
|
.expect("write failure hook");
|
|
make_executable(&failure_path);
|
|
fs::write(
|
|
root.join(".claude-plugin").join("plugin.json"),
|
|
format!(
|
|
"{{\n \"name\": \"{name}\",\n \"version\": \"1.0.0\",\n \"description\": \"hook plugin\",\n \"hooks\": {{\n \"PreToolUse\": [\"./hooks/pre.sh\"],\n \"PostToolUse\": [\"./hooks/post.sh\"],\n \"PostToolUseFailure\": [\"./hooks/failure.sh\"]\n }}\n}}"
|
|
),
|
|
)
|
|
.expect("write plugin manifest");
|
|
}
|
|
|
|
#[test]
|
|
fn collects_and_runs_hooks_from_enabled_plugins() {
|
|
// given
|
|
let config_home = temp_dir("config");
|
|
let first_source_root = temp_dir("source-a");
|
|
let second_source_root = temp_dir("source-b");
|
|
write_hook_plugin(
|
|
&first_source_root,
|
|
"first",
|
|
"plugin pre one",
|
|
"plugin post one",
|
|
"plugin failure one",
|
|
);
|
|
write_hook_plugin(
|
|
&second_source_root,
|
|
"second",
|
|
"plugin pre two",
|
|
"plugin post two",
|
|
"plugin failure two",
|
|
);
|
|
|
|
let mut manager = PluginManager::new(PluginManagerConfig::new(&config_home));
|
|
manager
|
|
.install(first_source_root.to_str().expect("utf8 path"))
|
|
.expect("first plugin install should succeed");
|
|
manager
|
|
.install(second_source_root.to_str().expect("utf8 path"))
|
|
.expect("second plugin install should succeed");
|
|
let registry = manager.plugin_registry().expect("registry should build");
|
|
|
|
// when
|
|
let runner = HookRunner::from_registry(®istry).expect("plugin hooks should load");
|
|
|
|
// then
|
|
assert_eq!(
|
|
runner.run_pre_tool_use("Read", r#"{"path":"README.md"}"#),
|
|
HookRunResult::allow(vec![
|
|
"plugin pre one".to_string(),
|
|
"plugin pre two".to_string(),
|
|
])
|
|
);
|
|
assert_eq!(
|
|
runner.run_post_tool_use("Read", r#"{"path":"README.md"}"#, "ok", false),
|
|
HookRunResult::allow(vec![
|
|
"plugin post one".to_string(),
|
|
"plugin post two".to_string(),
|
|
])
|
|
);
|
|
assert_eq!(
|
|
runner.run_post_tool_use_failure("Read", r#"{"path":"README.md"}"#, "tool failed",),
|
|
HookRunResult::allow(vec![
|
|
"plugin failure one".to_string(),
|
|
"plugin failure two".to_string(),
|
|
])
|
|
);
|
|
|
|
let _ = fs::remove_dir_all(config_home);
|
|
let _ = fs::remove_dir_all(first_source_root);
|
|
let _ = fs::remove_dir_all(second_source_root);
|
|
}
|
|
|
|
#[test]
|
|
fn pre_tool_use_denies_when_plugin_hook_exits_two() {
|
|
// given
|
|
let runner = HookRunner::new(crate::PluginHooks {
|
|
pre_tool_use: vec!["printf 'blocked by plugin'; exit 2".to_string()],
|
|
post_tool_use: Vec::new(),
|
|
post_tool_use_failure: Vec::new(),
|
|
});
|
|
|
|
// when
|
|
let result = runner.run_pre_tool_use("Bash", r#"{"command":"pwd"}"#);
|
|
|
|
// then
|
|
assert!(result.is_denied());
|
|
assert_eq!(result.messages(), &["blocked by plugin".to_string()]);
|
|
}
|
|
|
|
#[test]
|
|
fn propagates_plugin_hook_failures() {
|
|
// given
|
|
let runner = HookRunner::new(crate::PluginHooks {
|
|
pre_tool_use: vec![
|
|
"printf 'broken plugin hook'; exit 1".to_string(),
|
|
"printf 'later plugin hook'".to_string(),
|
|
],
|
|
post_tool_use: Vec::new(),
|
|
post_tool_use_failure: Vec::new(),
|
|
});
|
|
|
|
// when
|
|
let result = runner.run_pre_tool_use("Bash", r#"{"command":"pwd"}"#);
|
|
|
|
// then
|
|
assert!(result.is_failed());
|
|
assert!(result
|
|
.messages()
|
|
.iter()
|
|
.any(|message| message.contains("broken plugin hook")));
|
|
assert!(!result
|
|
.messages()
|
|
.iter()
|
|
.any(|message| message == "later plugin hook"));
|
|
}
|
|
|
|
#[test]
|
|
#[cfg(unix)]
|
|
fn generated_hook_scripts_are_executable() {
|
|
use std::os::unix::fs::PermissionsExt;
|
|
|
|
// given
|
|
let root = temp_dir("exec-guard");
|
|
write_hook_plugin(&root, "exec-check", "pre", "post", "fail");
|
|
|
|
// then
|
|
for script in ["pre.sh", "post.sh", "failure.sh"] {
|
|
let path = root.join("hooks").join(script);
|
|
let mode = fs::metadata(&path)
|
|
.unwrap_or_else(|e| panic!("{script} metadata: {e}"))
|
|
.permissions()
|
|
.mode();
|
|
assert!(
|
|
mode & 0o111 != 0,
|
|
"{script} must have at least one execute bit set, got mode {mode:#o}"
|
|
);
|
|
}
|
|
}
|
|
}
|