fix(plugins): chmod +x generated hook scripts + tolerate BrokenPipe in stdin write — closes ROADMAP #25 hotfix lane

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>
This commit is contained in:
YeonGyu-Kim
2026-04-08 15:41:49 +09:00
parent 647ff379a4
commit 172a2ad50a

View File

@@ -337,7 +337,28 @@ impl CommandWithStdin {
let mut child = self.command.spawn()?;
if let Some(mut child_stdin) = child.stdin.take() {
use std::io::Write as _;
child_stdin.write_all(stdin)?;
// 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()
}
@@ -359,6 +380,18 @@ mod tests {
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,
@@ -368,21 +401,30 @@ mod tests {
) {
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(
root.join("hooks").join("pre.sh"),
&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(
root.join("hooks").join("post.sh"),
&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(
root.join("hooks").join("failure.sh"),
&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!(
@@ -496,4 +538,27 @@ mod tests {
.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}"
);
}
}
}