feat: lifecycle hooks - #1205

scripts/session-e2e.sh (1)

864-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two duplicated guard expressions couple these blocks implicitly.

hook_log_has is defined inside the block that starts at Line 467, under the condition [ -n "$SEED_DIR" ] || [ -n "$SEEDED_MFILE" ]. The restart assertion at Line 935 calls that function, but it is guarded by [ -n "$HOOK_RESTART_SID" ] instead. The call is safe today only because Line 865 repeats the same seed condition. If a later edit changes one guard, hook_log_has becomes an unbound command inside the restart block.

Set one flag when the hook proofs run, and gate all three sites on it.

♻️ Proposed refactor

+HOOKS_PROOFS=""
 if [ -n "$SEED_DIR" ] || [ -n "$SEEDED_MFILE" ]; then
+  HOOKS_PROOFS=1

 HOOK_RESTART_SID=""
-if [ -n "$SEED_DIR" ] || [ -n "$SEEDED_MFILE" ]; then
+if [ -n "$HOOKS_PROOFS" ]; then
   HOOK_SEED_DIR="$(mktemp -d /tmp/mnlp2.XXXXXX)"

Also applies to: 923-925

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/session-e2e.sh` around lines 864 - 866, Introduce a shared flag
indicating that hook proofs are initialized, set it when the seed condition
enables the hook-proof block containing hook_log_has, and replace the duplicated
seed-condition checks plus the HOOK_RESTART_SID-only assertion guard with this
flag at all three sites. Ensure hook_log_has is called only when the flag is
set, including the restart assertion path.

crates/sessions/src/wire/policy.rs (1)

93-119: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add a round-trip test for WireHookVerdict.

WireVarVerdict and WirePatchVerdict each have a round-trip test in this module. WireHookVerdict is a new cross-process contract and has none. The existing round_trip helper makes the test small.

♻️ Proposed test

#[test]
fn hook_verdict_round_trips_all_variants() {
    let cases = [
        WireHookVerdict::Approved {
            id: PendingId::new(1),
        },
        WireHookVerdict::Denied {
            id: PendingId::new(2),
        },
        WireHookVerdict::Ignored {
            id: PendingId::new(3),
        },
    ];
    for v in cases {
        assert_eq!(round_trip(&v), v);
    }
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/wire/policy.rs` around lines 93 - 119, Add a test
alongside the existing verdict round-trip tests that constructs Approved,
Denied, and Ignored WireHookVerdict values with distinct PendingId values, then
passes each through the existing round_trip helper and asserts equality with the
original.

crates/sessions/src/core/policy.rs (1)

528-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for HooksPolicy and ExpandedHooksPolicy.

VarsPolicy and PatchesPolicy each have a dedicated test block in this file. HooksPolicy has none. This gate decides arbitrary code execution, so the precedence rules deserve the same pinning.

Cover at least these cases:

• deny wins over ignore for the same project root.
• allow matches a project root and returns Decided(Allowed).
• An unmatched project root returns NeedsApproval.
• Source::UserLoadout returns Allowed and Source::Package returns Denied, independent of the pattern lists.
• A [hooks] section round-trips through UserPolicy TOML, and an empty HooksPolicy is omitted on serialize.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/core/policy.rs` around lines 528 - 724, Add a dedicated
unit-test block covering HooksPolicy::expand_with and
ExpandedHooksPolicy::check: verify deny takes precedence over ignore, allow
yields Decided(Allowed), unmatched project roots yield NeedsApproval, and
UserLoadout/Package sources remain Allowed/Denied regardless of patterns. Also
test UserPolicy TOML round-tripping for a [hooks] section and confirm an empty
HooksPolicy is omitted during serialization, following the existing VarsPolicy
and PatchesPolicy test patterns.

crates/sessions/src/daemon/composer.rs (1)

293-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Include the denied hook description in ComposeError::Denied::what.

Use ProvenancedHook::hook() and LifecycleHook::description(), with "lifecycle hook" as the fallback when no description exists.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/sessions/src/daemon/composer.rs` around lines 293 - 305, Update the
V::Denied handling to build ComposeError::Denied::what from the pending
ProvenancedHook’s hook().description(), falling back to "lifecycle hook" when no
description exists; preserve the existing source lookup and InvalidWireItem
behavior for unknown IDs.

crates/minimal/src/task.rs (1)

310-314: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider a --no-hooks opt-out for min task run.

min task run now always enables lifecycle hooks. Activation hooks block activation on failure. A project with a broken or slow on_activate hook therefore blocks every task run, and the operator has no flag to bypass it. min session activate offers --no-hooks for exactly this case. Add the same flag to TaskRunArgs and pass !args.no_hooks to hooks_enabled, stage_loadout_hook_scripts, and compose_user_contribution.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/src/task.rs` around lines 310 - 314, Add a `no_hooks` opt-out
field to `TaskRunArgs`, expose the corresponding `--no-hooks` CLI flag, and
propagate its inverse (`!args.no_hooks`) to `hooks_enabled`,
`stage_loadout_hook_scripts`, and `compose_user_contribution` in the task-run
flow. Preserve hooks as enabled by default.

crates/minimal/src/loadouts.rs (1)

244-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the loadouts-directory path into one helper.

resolve_active_loadouts and stage_loadout_hook_scripts both build resolve_minimal_config_dir(global).join("loadouts"). The two must agree, because a loadout's script anchor is <loadouts_dir>/<name>/ next to its <name>.toml. If one call site changes, staging resolves against the wrong anchor and every external script fails with MissingAnchor. Add a single loadouts_dir(global) helper and call it from both.

♻️ Proposed helper

/// The directory holding `<name>.toml` loadout files and their
/// `<name>/` script directories.
fn loadouts_dir(global: &GlobalArgs) -> std::path::PathBuf {
    resolve_minimal_config_dir(global).join("loadouts")
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/src/loadouts.rs` around lines 244 - 248, Extract the repeated
loadouts path construction into a loadouts_dir helper returning
resolve_minimal_config_dir(global).join("loadouts"). Update both
resolve_active_loadouts and stage_loadout_hook_scripts to use this helper,
preserving the existing UTF-8 validation and ensuring both loadout files and
script anchors share the same directory.

crates/minimald/src/rpc.rs (1)

1340-1368: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Handle unsupported RENAME_EXCHANGE filesystems

When renameat2(..., RENAME_EXCHANGE) returns EINVAL, EOPNOTSUPP, or ENOSYS, use a documented non-atomic fallback (remove_dir_all(dir) followed by plain rename). Native deployments can place --minimal-state-dir on arbitrary filesystems, and the shared helper now covers hook uploads. Without this fallback, the missing HOOKS_READY_MARKER can block FinalizeSession for compositions that require staged hooks. Preserve the atomic path when supported.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/rpc.rs` around lines 1340 - 1368, Handle unsupported
RENAME_EXCHANGE errors in the swap_result match by treating EINVAL, EOPNOTSUPP,
and ENOSYS as fallback cases. Document the non-atomic fallback, remove the
existing dir with remove_dir_all, then plain-rename staging_dir into dir;
preserve the current atomic exchange and ENOENT paths unchanged.

crates/minimald/src/session.rs (1)

1346-1375: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Tear the activation host down when an activation hook fails.

The code launches a host at Line 1347 and keeps it in self.inner. On a hook failure the function returns Err at Line 1360, so the record stays Materializing. The session is then unattachable, because attach and context both gate on Active. The sandbox stays up until someone destroys the session.

Stop the host on the failure arm so a refused activation does not hold a sandbox open.

♻️ Proposed change

                     if let Some(failed) = outcomes.iter().find(|o| o.failed()) {
+                        // The session will not become `Active`, so the host
+                        // this launch minted has no one left to serve.
+                        self.stop_running(false).await;
                         return Err(std::io::Error::new(
                             std::io::ErrorKind::InvalidInput,

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimald/src/session.rs` around lines 1346 - 1375, Update the
activation-hook failure branch in the session transition flow after
launch_host_for_hooks(LaunchPhase::Activating) and before returning the
InvalidInput error to tear down the activation host through the existing
host-shutdown mechanism. Ensure cleanup completes before returning Err, while
preserving the current failure message and preventing the sandbox from remaining
active.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimal/src/lib.rs (3)

2622-2640: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Handle an inline body whose first line is blank.

render_hook_script takes only the first line. If the script starts with a blank line or a shebang-free leading newline, the row shows an empty body with a trailing ellipsis. Use the first non-empty line instead.

♻️ Proposed change

         WireHookScript::Inline { body, timeout_secs } => {
-            let first = body.lines().next().unwrap_or("").trim();
-            let shown = if body.lines().count() > 1 {
+            let mut lines = body.lines().map(str::trim).filter(|l| !l.is_empty());
+            let first = lines.next().unwrap_or("");
+            let shown = if lines.next().is_some() {
                 format!("{first} …")
             } else {
                 first.to_string()
             };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/src/lib.rs` around lines 2622 - 2640, Update
render_hook_script’s inline-body preview to select the first non-empty trimmed
line instead of always using body.lines().next(), while preserving the ellipsis
behavior for multi-line bodies and the existing empty preview when no non-empty
line exists.

---

2585-2603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider printing the events in lifecycle order and adding a header row.

The loop prints on_activate, on_destroy, on_attach, on_detach. on_destroy appears before the attach and detach events, so the row order does not match the session lifecycle order described in the doc comment. A short header row would also make the unlabeled columns (event, type, timeout, source) readable.

♻️ Proposed ordering change

         for (event, script) in [
             ("on_activate", hook.on_activate.as_ref()),
-            ("on_destroy", hook.on_destroy.as_ref()),
             ("on_attach", hook.on_attach.as_ref()),
             ("on_detach", hook.on_detach.as_ref()),
+            ("on_destroy", hook.on_destroy.as_ref()),
         ] {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/src/lib.rs` around lines 2585 - 2603, Update the hook-printing
loop to emit events in lifecycle order: on_activate, on_attach, on_detach, then
on_destroy. Add a concise header row before iterating hooks that labels the
event, type, timeout, and source columns, while preserving the existing
description and body output.

---

240-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add parse tests for session hooks and --no-hooks.

The file tests setup-zed parsing but not the two new surfaces. Add tests that min session hooks <session> --json parses into HooksArgs, and that no_hooks defaults to false and is set by --no-hooks. These tests protect the flag that disables hook execution for the whole session lifetime.

Also applies to: 489-497

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/minimal/src/lib.rs` around lines 240 - 249, Add parser tests covering
the new session hook CLI surfaces: verify `session hooks <session> --json`
parses into `HooksArgs` with the session and JSON flag set, and verify
`no_hooks` defaults to false and becomes true when `--no-hooks` is provided.
Place these alongside the existing `setup-zed` parsing tests and use the
existing argument-parsing test conventions.