Skip to content

Commit 172a2ad

Browse files
code-yeongyugaebal-gajae
andcommitted
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>
1 parent 647ff37 commit 172a2ad

1 file changed

Lines changed: 69 additions & 4 deletions

File tree

rust/crates/plugins/src/hooks.rs

Lines changed: 69 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,28 @@ impl CommandWithStdin {
337337
let mut child = self.command.spawn()?;
338338
if let Some(mut child_stdin) = child.stdin.take() {
339339
use std::io::Write as _;
340-
child_stdin.write_all(stdin)?;
340+
// Tolerate BrokenPipe: a hook script that runs to completion
341+
// (or exits early without reading stdin) closes its stdin
342+
// before the parent finishes writing the JSON payload, and
343+
// the kernel raises EPIPE on the parent's write_all. That is
344+
// not a hook failure — the child still exited cleanly and we
345+
// still need to wait_with_output() to capture stdout/stderr
346+
// and the real exit code. Other write errors (e.g. EIO,
347+
// permission, OOM) still propagate.
348+
//
349+
// This was the root cause of the Linux CI flake on
350+
// hooks::tests::collects_and_runs_hooks_from_enabled_plugins
351+
// (ROADMAP #25, runs 24120271422 / 24120538408 / 24121392171
352+
// / 24121776826): the test hook scripts run in microseconds
353+
// and the parent's stdin write races against child exit.
354+
// macOS pipes happen to buffer the small payload before the
355+
// child exits; Linux pipes do not, so the race shows up
356+
// deterministically on ubuntu runners.
357+
match child_stdin.write_all(stdin) {
358+
Ok(()) => {}
359+
Err(error) if error.kind() == std::io::ErrorKind::BrokenPipe => {}
360+
Err(error) => return Err(error),
361+
}
341362
}
342363
child.wait_with_output()
343364
}
@@ -359,6 +380,18 @@ mod tests {
359380
std::env::temp_dir().join(format!("plugins-hook-runner-{label}-{nanos}"))
360381
}
361382

383+
fn make_executable(path: &Path) {
384+
#[cfg(unix)]
385+
{
386+
use std::os::unix::fs::PermissionsExt;
387+
let perms = fs::Permissions::from_mode(0o755);
388+
fs::set_permissions(path, perms)
389+
.unwrap_or_else(|e| panic!("chmod +x {}: {e}", path.display()));
390+
}
391+
#[cfg(not(unix))]
392+
let _ = path;
393+
}
394+
362395
fn write_hook_plugin(
363396
root: &Path,
364397
name: &str,
@@ -368,21 +401,30 @@ mod tests {
368401
) {
369402
fs::create_dir_all(root.join(".claude-plugin")).expect("manifest dir");
370403
fs::create_dir_all(root.join("hooks")).expect("hooks dir");
404+
405+
let pre_path = root.join("hooks").join("pre.sh");
371406
fs::write(
372-
root.join("hooks").join("pre.sh"),
407+
&pre_path,
373408
format!("#!/bin/sh\nprintf '%s\\n' '{pre_message}'\n"),
374409
)
375410
.expect("write pre hook");
411+
make_executable(&pre_path);
412+
413+
let post_path = root.join("hooks").join("post.sh");
376414
fs::write(
377-
root.join("hooks").join("post.sh"),
415+
&post_path,
378416
format!("#!/bin/sh\nprintf '%s\\n' '{post_message}'\n"),
379417
)
380418
.expect("write post hook");
419+
make_executable(&post_path);
420+
421+
let failure_path = root.join("hooks").join("failure.sh");
381422
fs::write(
382-
root.join("hooks").join("failure.sh"),
423+
&failure_path,
383424
format!("#!/bin/sh\nprintf '%s\\n' '{failure_message}'\n"),
384425
)
385426
.expect("write failure hook");
427+
make_executable(&failure_path);
386428
fs::write(
387429
root.join(".claude-plugin").join("plugin.json"),
388430
format!(
@@ -496,4 +538,27 @@ mod tests {
496538
.iter()
497539
.any(|message| message == "later plugin hook"));
498540
}
541+
542+
#[test]
543+
#[cfg(unix)]
544+
fn generated_hook_scripts_are_executable() {
545+
use std::os::unix::fs::PermissionsExt;
546+
547+
// given
548+
let root = temp_dir("exec-guard");
549+
write_hook_plugin(&root, "exec-check", "pre", "post", "fail");
550+
551+
// then
552+
for script in ["pre.sh", "post.sh", "failure.sh"] {
553+
let path = root.join("hooks").join(script);
554+
let mode = fs::metadata(&path)
555+
.unwrap_or_else(|e| panic!("{script} metadata: {e}"))
556+
.permissions()
557+
.mode();
558+
assert!(
559+
mode & 0o111 != 0,
560+
"{script} must have at least one execute bit set, got mode {mode:#o}"
561+
);
562+
}
563+
}
499564
}

0 commit comments

Comments
 (0)