From 172a2ad50ad9a38cd93da9a89fe6a521e61141d4 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 8 Apr 2026 15:41:49 +0900 Subject: [PATCH] =?UTF-8?q?fix(plugins):=20chmod=20+x=20generated=20hook?= =?UTF-8?q?=20scripts=20+=20tolerate=20BrokenPipe=20in=20stdin=20write=20?= =?UTF-8?q?=E2=80=94=20closes=20ROADMAP=20#25=20hotfix=20lane?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ' (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 --- rust/crates/plugins/src/hooks.rs | 73 ++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/rust/crates/plugins/src/hooks.rs b/rust/crates/plugins/src/hooks.rs index 85c803d..ff02c2a 100644 --- a/rust/crates/plugins/src/hooks.rs +++ b/rust/crates/plugins/src/hooks.rs @@ -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}" + ); + } + } }