[pablof7z]

Addresses review feedback on #117 (project hooks system).

Fixes

1. Kill orphaned child process on timeout (blocking). run_hook previously
   relied on tokio::time::timeout(...wait_with_output()); on timeout the
   Child was dropped and tokio detached the OS process, orphaning it. Set
   Command::kill_on_drop(true) so dropping the wait future on timeout sends
   SIGKILL — no orphaned process remains.

   Note: used kill_on_drop(true) rather than the literal select! /
   child.kill().await from the issue because wait_with_output() consumes the
   child, so the literal snippet does not compile. kill_on_drop is the
   idiomatic fix for this exact bug and satisfies the no-orphan requirement
   while preserving wait_with_output's concurrent pipe draining.

2. Pre-tool spawn failure now blocks. A pre-tool hook that fails to spawn
   (missing or non-executable binary) is a misconfiguration, not flakiness, so
   it now returns PreToolOutcome::Block(...) instead of silently continuing.
   Post-tool spawn failures remain warnings — the tool already ran. Added a
   pre_tool_spawn_failure_blocks test.

3. Use records.last_mut(). Replaced the records_before index with
   records.last_mut() when folding hook injections into the tool record, and
   removed the now-unused records_before binding.

4. Comment hygiene. Collapsed multi-line doc/comment blocks (module doc in
   project_hooks.rs, drain_hook_injections in hook.rs, and the
   hooks-related comments in tools.rs) to single WHY lines per the project
   comment policy.

Verification

• cargo build -p tenex-agent passes.
• cargo test -p tenex-agent passes (including the new test).

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR addresses review feedback on #117 (project hooks system) with four targeted fixes: orphan-process prevention on timeout, pre-tool spawn failure now blocking, a minor index simplification, and comment reduction. The changes are focused and well-reasoned.

---

What's Good

• kill_on_drop(true) is the right fix. The PR body correctly explains why the select!/child.kill().await pattern from the issue won't compile once wait_with_output() consumes the child. kill_on_drop is idiomatic Tokio for this exact scenario and satisfies the no-orphan requirement cleanly.

• Pre-tool vs post-tool asymmetry is correct. Blocking on pre-tool spawn failure (misconfiguration, wrong path) is the right call — the tool hasn't run yet so blocking is safe. Continuing with a warning for post-tool failures is also correct since the tool already ran and there's nothing to abort.

• The new test is well-structured. pre_tool_spawn_failure_blocks is direct, uses /nonexistent/hook-binary for a reliably-failing spawn, and asserts the right things. Good coverage of the new behavior.

---

Concerns

1. records.last_mut() is subtly weaker than records_before (medium risk)

// before: safe if the current tool call produced 0 records
if let Some(record) = records.get_mut(records_before) { … }

// after: falls through to the PREVIOUS tool call's record if current produced 0
if let Some(record) = records.last_mut() { … }

With records_before = records.len() captured before the call, records.get_mut(records_before) returns None if no record was appended for the current tool call — so hook injections are safely dropped rather than silently attached to the wrong record. records.last_mut() loses that guard.

This is only a bug if a tool call can produce zero new records. If the implementation guarantees exactly one record per call (which it likely does given the loop structure), the change is safe — but that invariant is now load-bearing and undocumented. Either add a comment at the push site that makes the one-record-per-call contract explicit, or keep records_before to preserve the defensive behavior without needing to rely on the invariant.

2. Module doc reduction removed non-obvious protocol details

The collapsed module doc in project_hooks.rs went from:

//! Project-local hooks: Claude Code-compatible shell commands that fire at
//! `pre-tool` and `post-tool` boundaries.
//!
//! Hooks are configured in `.tenex-hooks.json` in the agent's workspace
//! directory. Each hook receives JSON context on stdin and can:
//! - **block** a tool call (exit non-zero on `pre-tool`), or
//! - **inject** context into the conversation (non-empty stdout on exit 0).
//!
//! The hook protocol is intentionally language-agnostic: hooks are plain
//! shell commands, so any Claude Code-compatible hook works without bespoke
//! integration. This crate owns spawning and the stdin/stdout contract;
//! [`crate::hook::EmitHook`] owns wiring the outcomes into the tool-call
//! lifecycle.

to one line. The CLAUDE.md policy is "only add [a comment] when the WHY is non-obvious." The stdin/stdout protocol, the exit-code semantics (non-zero = block), and the split of ownership between this crate and EmitHook are all non-obvious — they're the why of the module's shape and can't be discovered by reading the type names. The inline kill_on_drop comment was correctly kept for exactly this reason. I'd restore at least the "each hook receives JSON on stdin; exit non-zero blocks (pre-tool only); stdout injects context" and the EmitHook ownership note.

3. No regression test for post-tool spawn-failure asymmetry

The new test proves pre-tool spawn failure blocks. There's no test that post-tool spawn failure doesn't block (i.e., returns the empty injections vec rather than propagating the error). Given the explicit design asymmetry, a post_tool_spawn_failure_is_non_blocking test would lock in that behavior.

---

Minor

• The single-line inline comment added to run_hook:

  // On timeout the wait future is dropped; without this the OS
  // process would be detached and orphaned.

  is exactly the right style and level — keep it.

• The comment reductions in hook.rs and tools.rs are fine; those comments were mostly restating what the code does.

---

Summary

The core fixes (orphan kill, spawn-failure blocking, kill_on_drop) are correct and idiomatic. The two items worth addressing before merge: (1) the records.last_mut() implicit invariant — either document or defend against it — and (2) restore the protocol-level details to the project_hooks.rs module doc since they describe non-obvious contracts, not implementation mechanics.