diff --git a/assets/config-template.yaml b/assets/config-template.yaml index 4ac410d..d4b16c9 100644 --- a/assets/config-template.yaml +++ b/assets/config-template.yaml @@ -149,6 +149,18 @@ mcp_tools: null # Per-server MCP tool allowlists (glob patterns # - list_* # slack: [] +# ---- Hooks ---- +# Fire-and-forget external commands that run when lifecycle events fire. Each event name +# maps to a list of named hooks; every listed hook's command runs when that event fires. +# Hooks never block coyote and never change its output or exit code. +# See the [Hooks documentation](https://github.com/Dark-Alex-17/coyote/wiki/Hooks) for more details +hooks: {} # Event -> list of named hook commands (default: none) + # Example: + # hooks: + # turn.completed: + # - name: notify + # command: ./hooks/notify.sh + # ---- Auto-Continue (Todo System) ---- # The auto-continue system provides built-in task tracking for improved reliability. # When enabled, the model can create todo lists and the system will automatically diff --git a/assets/hooks/log-events.sh b/assets/hooks/log-events.sh new file mode 100755 index 0000000..a5563d6 --- /dev/null +++ b/assets/hooks/log-events.sh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# Event-logging hook: appends a timestamped snapshot of every COYOTE_* env var +# to a log file, so you can see exactly which events fire and what context +# each one carries. +# +# Env vars read: +# COYOTE_HOOK_LOG log file to append to (default: /tmp/coyote-hooks.log) +# COYOTE_* everything coyote sets for the event is dumped verbatim +# +# Secrets (COYOTE_SECRET_*) are excluded from the snapshot; widening the grep +# filter below opts into logging them. +# +# Wire it up under `hooks:` in your config.yaml (any event works): +# hooks: +# tool.completed: +# - name: log-events +# command: ./hooks/log-events.sh +# +# A relative command path like ./hooks/log-events.sh resolves against the +# coyote config directory for global hooks, so this works with the default +# hooks dir. +# +# See https://github.com/Dark-Alex-17/coyote/wiki/Hooks for the full list of +# events and env vars. Hooks are fire-and-forget: this script never fails +# loudly, and coyote ignores its exit code either way. + +set -u + +log_file="${COYOTE_HOOK_LOG:-/tmp/coyote-hooks.log}" + +# The default lives in shared /tmp: refuse to append through a symlink someone +# else planted there. Point COYOTE_HOOK_LOG at a private path to avoid the +# shared directory entirely. +[ -L "$log_file" ] && exit 0 +umask 077 +{ + echo "=== $(date '+%Y-%m-%dT%H:%M:%S%z') ${COYOTE_EVENT:-unknown}" + env | grep '^COYOTE_' | grep -v '^COYOTE_SECRET_' | sort + echo +} >> "$log_file" 2> /dev/null || true diff --git a/assets/hooks/notify.sh b/assets/hooks/notify.sh new file mode 100755 index 0000000..1c010a3 --- /dev/null +++ b/assets/hooks/notify.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash +# Desktop-notification hook: posts a notification describing the event that +# fired, falling back to a message on the controlling terminal when no +# notifier is present. +# +# Env vars read (set by coyote when the hook fires): +# COYOTE_EVENT dotted event name, e.g. turn.completed +# COYOTE_AGENT_NAME agent name, when the event carries one +# COYOTE_TOOL_NAME tool name, when the event carries one +# +# Wire it up under `hooks:` in your config.yaml: +# hooks: +# turn.completed: +# - name: notify +# command: ./hooks/notify.sh +# +# A relative command path like ./hooks/notify.sh resolves against the coyote +# config directory for global hooks, so this works with the default hooks dir. +# +# See https://github.com/Dark-Alex-17/coyote/wiki/Hooks for the full list of +# events and env vars. Hooks are fire-and-forget: this script never fails +# loudly, and coyote ignores its exit code either way. + +set -u + +title="coyote" +body="${COYOTE_EVENT:-event}" +[ -n "${COYOTE_AGENT_NAME:-}" ] && body="$body agent=$COYOTE_AGENT_NAME" +[ -n "${COYOTE_TOOL_NAME:-}" ] && body="$body tool=$COYOTE_TOOL_NAME" + +if command -v notify-send > /dev/null 2>&1; then + notify-send "$title" "$body" 2> /dev/null || true +elif command -v osascript > /dev/null 2>&1; then + # $body carries model-influenced values (e.g. tool names); strip quotes and + # backslashes so it cannot escape the AppleScript string literal. + safe_body=${body//[\"\\]/} + osascript -e "display notification \"$safe_body\" with title \"$title\"" 2> /dev/null || true +else + # coyote discards hook stdout, so aim the fallback at the controlling + # terminal when one exists; plain stdout keeps direct/manual runs working. + { echo "[$title] $body" > /dev/tty; } 2> /dev/null || echo "[$title] $body" +fi diff --git a/config.example.yaml b/config.example.yaml index 8be737d..33d00e6 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -206,6 +206,18 @@ enabled_macros: null # Which macros are invocable by default (no ro # Example (comma-separated form): # enabled_macros: generate-commit-message,review +# ---- Hooks ---- +# Fire-and-forget external commands that run when lifecycle events fire. Each event name +# maps to a list of named hooks; every listed hook's command runs when that event fires. +# Hooks never block coyote and never change its output or exit code. +# See the [Hooks documentation](https://github.com/Dark-Alex-17/coyote/wiki/Hooks) for more details +hooks: {} # Event -> list of named hook commands (default: none) + # Example: + # hooks: + # turn.completed: + # - name: notify + # command: ./hooks/notify.sh + # ---- Auto-Continue (Todo System) ---- # The auto-continue system provides built-in task tracking for improved reliability. # When enabled, the model can create todo lists and the system will automatically diff --git a/scripts/usage-probe-hooks.sh b/scripts/usage-probe-hooks.sh new file mode 100755 index 0000000..7096535 --- /dev/null +++ b/scripts/usage-probe-hooks.sh @@ -0,0 +1,232 @@ +#!/usr/bin/env bash +# Black-box usage-pattern probe for the coyote hook system. +# +# Verifies, against a REAL built `coyote` binary in a clean/isolated +# COYOTE_CONFIG_DIR (dry_run:true, no network/LLM credentials required): +# A) exactly one turn.started + one turn.completed + one llm.request.completed +# fire per headless run, with correctly scoped env vars (no cross-event +# extras leakage). +# B) a failing/missing-script hook never changes coyote's stdout, stderr, or +# exit code. +# C) `coyote --install-builtins hooks` installs both bundled example scripts +# 0755 into /hooks via the real CLI, and a second invocation +# (force reinstall) overwrites local edits back to the packaged content. +# D) `coyote --agent --info` (an inspection flag) fires ZERO +# agent.* hook events. +# +# Not covered here (see src/function/mod.rs tests for tool.* + payload-file +# coverage instead): a live tool.started firing requires a real LLM round +# trip that actually decides to call a tool; dry_run's echo_messages() path +# never invokes tools, so it cannot be exercised via this recipe. +# +# Usage: scripts/usage-probe-hooks.sh +# Requires: a `coyote` binary on PATH or at ./target/debug/coyote or +# ./target/release/coyote (build with `cargo build --bin coyote` first). + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +BIN="${COYOTE_BIN:-}" +if [ -z "$BIN" ]; then + if [ -x "$REPO_ROOT/target/debug/coyote" ]; then + BIN="$REPO_ROOT/target/debug/coyote" + elif [ -x "$REPO_ROOT/target/release/coyote" ]; then + BIN="$REPO_ROOT/target/release/coyote" + else + echo "No coyote binary found; run 'cargo build --bin coyote' first." >&2 + exit 2 + fi +fi + +WORKDIR="$(mktemp -d)" +trap 'rm -rf "$WORKDIR"' EXIT + +fail() { + echo "FAIL: $1" >&2 + exit 1 +} + +write_dryrun_config() { + # $1 = config dir, $2..$n = extra YAML fragment lines appended verbatim + local dir="$1" + shift + cat > "$dir/config.yaml" <<'BASE' +model: dryrun:dry-model +dry_run: true +clients: + - type: openai + name: dryrun + auth: none + api_key: 'unused' + models: + - name: dry-model + max_input_tokens: 100000 + supports_function_calling: true +save: false +memory: false +stream: false +BASE + if [ "$#" -gt 0 ]; then + printf '%s\n' "$@" >> "$dir/config.yaml" + fi +} + +echo "== Scenario A: turn/llm hook cadence and env scoping ==" +CFG_A="$WORKDIR/a" +mkdir -p "$CFG_A" +TS_LOG="$WORKDIR/turn-started.log" +TC_LOG="$WORKDIR/turn-completed.log" +LC_LOG="$WORKDIR/llm-completed.log" +write_dryrun_config "$CFG_A" "hooks:" \ + " turn.started:" \ + " - name: probe" \ + " command: \"{ echo \\\"EVENT=\$COYOTE_EVENT NAME=\$COYOTE_HOOK_NAME\\\"; } >> $TS_LOG\"" \ + " turn.completed:" \ + " - name: probe" \ + " command: \"{ echo \\\"EVENT=\$COYOTE_EVENT NAME=\$COYOTE_HOOK_NAME\\\"; } >> $TC_LOG\"" \ + " llm.request.completed:" \ + " - name: probe-llm" \ + " command: \"{ echo \\\"EVENT=\$COYOTE_EVENT PROVIDER=\$COYOTE_LLM_PROVIDER MODEL=\$COYOTE_LLM_MODEL\\\"; } >> $LC_LOG\"" + +COYOTE_CONFIG_DIR="$CFG_A" "$BIN" --no-stream "say exactly: hi" > /dev/null +sleep 1 + +[ "$(grep -c '^EVENT=turn.started' "$TS_LOG" 2>/dev/null || echo 0)" = "1" ] \ + || fail "expected exactly one turn.started, got: $(cat "$TS_LOG" 2>/dev/null)" +[ "$(grep -c '^EVENT=turn.completed' "$TC_LOG" 2>/dev/null || echo 0)" = "1" ] \ + || fail "expected exactly one turn.completed, got: $(cat "$TC_LOG" 2>/dev/null)" +[ "$(grep -c '^EVENT=llm.request.completed' "$LC_LOG" 2>/dev/null || echo 0)" = "1" ] \ + || fail "expected exactly one llm.request.completed, got: $(cat "$LC_LOG" 2>/dev/null)" +grep -q 'PROVIDER=dryrun MODEL=dryrun:dry-model' "$LC_LOG" \ + || fail "llm.request.completed missing expected COYOTE_LLM_PROVIDER/MODEL: $(cat "$LC_LOG")" +echo "PASS: exactly 1 turn.started + 1 turn.completed + 1 llm.request.completed, correctly scoped" + +echo "== Scenario B: failing/missing-script hooks never affect output or exit code ==" +CFG_B="$WORKDIR/b" +mkdir -p "$CFG_B" +write_dryrun_config "$CFG_B" "hooks:" \ + " turn.completed:" \ + " - name: failing" \ + " command: /nonexistent/script/does/not/exist" \ + " - name: exit1" \ + " command: \"exit 1\"" +CFG_BASE="$WORKDIR/base" +mkdir -p "$CFG_BASE" +write_dryrun_config "$CFG_BASE" + +exit_b=0 +COYOTE_CONFIG_DIR="$CFG_B" "$BIN" --no-stream "say exactly: hi" > "$WORKDIR/out_b.txt" 2> "$WORKDIR/err_b.txt" || exit_b=$? +exit_base=0 +COYOTE_CONFIG_DIR="$CFG_BASE" "$BIN" --no-stream "say exactly: hi" > "$WORKDIR/out_base.txt" 2> "$WORKDIR/err_base.txt" || exit_base=$? + +[ "$exit_b" = "$exit_base" ] || fail "exit code differs: with-hooks=$exit_b baseline=$exit_base" +diff -q "$WORKDIR/out_b.txt" "$WORKDIR/out_base.txt" > /dev/null || fail "stdout differs when a hook fails/is missing" +diff -q "$WORKDIR/err_b.txt" "$WORKDIR/err_base.txt" > /dev/null || fail "stderr differs when a hook fails/is missing" +echo "PASS: failing/missing-script hooks are invisible to stdout/stderr/exit code" + +echo "== Scenario C: coyote --install-builtins hooks (real CLI, clean config dir) ==" +CFG_C="$WORKDIR/c" +mkdir -p "$CFG_C" + +out_install1="$WORKDIR/install1.out" +COYOTE_CONFIG_DIR="$CFG_C" "$BIN" --install-builtins hooks > "$out_install1" 2>&1 + +[ -f "$CFG_C/hooks/notify.sh" ] || fail "notify.sh was not installed: $(cat "$out_install1")" +[ -f "$CFG_C/hooks/log-events.sh" ] || fail "log-events.sh was not installed: $(cat "$out_install1")" +grep -q "Reinstalled bundled hooks" "$out_install1" \ + || fail "expected confirmation message, got: $(cat "$out_install1")" + +if command -v stat > /dev/null 2>&1; then + mode_notify="$(stat -c '%a' "$CFG_C/hooks/notify.sh" 2>/dev/null || stat -f '%Lp' "$CFG_C/hooks/notify.sh")" + mode_log="$(stat -c '%a' "$CFG_C/hooks/log-events.sh" 2>/dev/null || stat -f '%Lp' "$CFG_C/hooks/log-events.sh")" + [ "$mode_notify" = "755" ] || fail "notify.sh mode is $mode_notify, expected 755" + [ "$mode_log" = "755" ] || fail "log-events.sh mode is $mode_log, expected 755" +fi + +# Modify a script locally, then re-run --install-builtins hooks: it always +# forces (no confirmation prompt on non-tty stdout), so the local edit must +# be overwritten back to the packaged content. +echo "# LOCALLY MODIFIED" > "$CFG_C/hooks/notify.sh" +COYOTE_CONFIG_DIR="$CFG_C" "$BIN" --install-builtins hooks > "$WORKDIR/install2.out" 2>&1 +grep -q "LOCALLY MODIFIED" "$CFG_C/hooks/notify.sh" \ + && fail "force reinstall did not overwrite the locally modified notify.sh" +grep -q '^#!/usr/bin/env bash' "$CFG_C/hooks/notify.sh" \ + || fail "notify.sh was not restored to the packaged script" +echo "PASS: --install-builtins hooks installs both scripts 0755 and force-overwrites local edits" + +echo "== Scenario C2: cold-start auto-bootstrap fills the hooks dir but never forces ==" +CFG_C2="$WORKDIR/c2" +mkdir -p "$CFG_C2/hooks" +write_dryrun_config "$CFG_C2" + +# Every coyote invocation calls install_builtins() (force=false) before +# dispatching any flag, so a plain run on a config dir with no hooks/ yet +# must auto-populate it... +rmdir "$CFG_C2/hooks" 2>/dev/null || true +COYOTE_CONFIG_DIR="$CFG_C2" "$BIN" --no-stream "say exactly: hi" > /dev/null +[ -f "$CFG_C2/hooks/notify.sh" ] || fail "cold-start run did not auto-install notify.sh" + +# ...but once a file exists, the non-force path must NEVER touch it again. +echo "# LOCALLY MODIFIED" > "$CFG_C2/hooks/notify.sh" +COYOTE_CONFIG_DIR="$CFG_C2" "$BIN" --no-stream "say exactly: hi" > /dev/null +grep -q "LOCALLY MODIFIED" "$CFG_C2/hooks/notify.sh" \ + || fail "a plain (non --install-builtins) invocation overwrote a locally modified hook script" +echo "PASS: plain invocations auto-bootstrap missing hooks but never force-overwrite existing ones" + +echo "== Scenario C3: installed log-events.sh fires end-to-end via the documented relative path ==" +# Exercises the exact wiring promised in the script headers: +# command: ./hooks/log-events.sh (resolves against the config dir) +# and the script's own contract: COYOTE_HOOK_LOG override honored, log file +# created 0600, COYOTE_SECRET_* values excluded from the snapshot. +CFG_C3="$WORKDIR/c3" +mkdir -p "$CFG_C3" +HOOK_LOG="$WORKDIR/c3-hook.log" +write_dryrun_config "$CFG_C3" "hooks:" \ + " turn.completed:" \ + " - name: log-events" \ + " command: ./hooks/log-events.sh" +COYOTE_CONFIG_DIR="$CFG_C3" "$BIN" --install-builtins hooks > /dev/null 2>&1 +COYOTE_CONFIG_DIR="$CFG_C3" COYOTE_HOOK_LOG="$HOOK_LOG" COYOTE_SECRET_PROBE=hunter2 \ + "$BIN" --no-stream "say exactly: hi" > /dev/null +sleep 1 + +[ -f "$HOOK_LOG" ] || fail "installed log-events.sh did not write to COYOTE_HOOK_LOG via relative-path wiring" +[ "$(grep -c '^=== ' "$HOOK_LOG")" = "1" ] \ + || fail "expected exactly one event block in the hook log, got: $(cat "$HOOK_LOG")" +grep -q 'COYOTE_EVENT=turn.completed' "$HOOK_LOG" \ + || fail "hook log missing COYOTE_EVENT=turn.completed: $(cat "$HOOK_LOG")" +grep -q 'hunter2' "$HOOK_LOG" \ + && fail "COYOTE_SECRET_* value leaked into the hook log" +grep -q 'COYOTE_SECRET_' "$HOOK_LOG" \ + && fail "COYOTE_SECRET_ variable name leaked into the hook log" +if command -v stat > /dev/null 2>&1; then + mode_hook_log="$(stat -c '%a' "$HOOK_LOG" 2>/dev/null || stat -f '%Lp' "$HOOK_LOG")" + [ "$mode_hook_log" = "600" ] || fail "hook log mode is $mode_hook_log, expected 600" +fi +echo "PASS: relative-path wiring fires the installed log-events.sh; log is 0600 with secrets excluded" + +echo "== Scenario D: --agent --info fires zero agent.* events ==" +CFG_D="$WORKDIR/d" +mkdir -p "$CFG_D/agents" +AGENT_LOG="$WORKDIR/agent-events.log" +write_dryrun_config "$CFG_D" "hooks:" \ + " agent.started:" \ + " - name: probe-agent" \ + " command: \"echo AGENT_STARTED >> $AGENT_LOG\"" \ + " agent.completed:" \ + " - name: probe-agent" \ + " command: \"echo AGENT_DONE >> $AGENT_LOG\"" \ + " agent.failed:" \ + " - name: probe-agent" \ + " command: \"echo AGENT_FAILED >> $AGENT_LOG\"" + +AGENT_NAME="$(ls "$REPO_ROOT/assets/agents" | head -1)" +cp -r "$REPO_ROOT/assets/agents/$AGENT_NAME" "$CFG_D/agents/$AGENT_NAME" + +COYOTE_CONFIG_DIR="$CFG_D" "$BIN" --agent "$AGENT_NAME" --info > /dev/null 2>&1 +sleep 1 +[ -s "$AGENT_LOG" ] && fail "expected zero agent.* events from --agent --info, got: $(cat "$AGENT_LOG")" +echo "PASS: --agent $AGENT_NAME --info fired zero agent.* hook events" + +echo +echo "ALL SCENARIOS PASSED" diff --git a/src/cli/mod.rs b/src/cli/mod.rs index f53658b..4a262e5 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -609,6 +609,10 @@ mod tests { parse(&["--install-builtins", "mcp_config"]).install_builtins, Some(AssetCategory::McpConfig) ); + assert_eq!( + parse(&["--install-builtins", "hooks"]).install_builtins, + Some(AssetCategory::Hooks) + ); } #[test] @@ -620,12 +624,14 @@ mod tests { assert!(category.get_name_and_aliases().any(|n| n == "mcp_config")); assert!(AssetCategory::NAMES.contains(&"mcp-config")); assert!(!AssetCategory::NAMES.contains(&"mcp_config")); + assert!(AssetCategory::NAMES.contains(&"hooks")); let filter = InstallFilter::McpConfig.to_possible_value().unwrap(); assert_eq!(filter.get_name(), "mcp-config"); assert!(filter.get_name_and_aliases().any(|n| n == "mcp_config")); assert!(InstallFilter::NAMES.contains(&"mcp-config")); assert!(!InstallFilter::NAMES.contains(&"mcp_config")); + assert!(InstallFilter::NAMES.contains(&"hooks")); } #[test] @@ -685,6 +691,10 @@ mod tests { .filter, Some(InstallFilter::McpConfig) ); + assert_eq!( + parse(&["--install", "https://github.com/x/y", "--filter", "hooks"]).filter, + Some(InstallFilter::Hooks) + ); } #[test] diff --git a/src/client/common.rs b/src/client/common.rs index 7a223cb..c24eebb 100644 --- a/src/client/common.rs +++ b/src/client/common.rs @@ -2,6 +2,7 @@ use super::*; use super::access_token::{distrust_access_token, get_access_token}; use crate::config::{RenderMode, paths}; +use crate::hooks::{self, HookEvent}; use crate::{ config::{AppConfig, Input, RequestContext}, function::{FunctionDeclaration, ToolCall, ToolResult, eval_tool_calls}, @@ -20,7 +21,7 @@ use reqwest::{Client as ReqwestClient, RequestBuilder}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use std::sync::LazyLock; -use std::time::Duration; +use std::time::{Duration, Instant}; use tokio::sync::mpsc::unbounded_channel; pub const MODELS_YAML: &str = include_str!("../../models.yaml"); @@ -546,6 +547,47 @@ pub async fn create_openai_compatible_client_config( Ok(Some((model, clients))) } +fn llm_request_extras(client: &dyn Client) -> Vec<(&'static str, String)> { + vec![ + ("COYOTE_LLM_PROVIDER", client.name().to_string()), + ("COYOTE_LLM_MODEL", client.model().id()), + ] +} + +fn llm_completed_extras(client: &dyn Client, started_at: Instant) -> Vec<(&'static str, String)> { + let mut extras = llm_request_extras(client); + extras.push(( + "COYOTE_LLM_DURATION_MS", + started_at.elapsed().as_millis().to_string(), + )); + extras +} + +fn llm_failed_extras( + client: &dyn Client, + err: Option<&anyhow::Error>, + aborted: bool, +) -> Vec<(&'static str, String)> { + let mut extras = llm_request_extras(client); + if aborted { + extras.push(("COYOTE_LLM_ERROR_KIND", "aborted".to_string())); + } else if let Some(api_err) = err.and_then(|err| err.downcast_ref::()) { + extras.push(("COYOTE_LLM_ERROR_KIND", "api_error".to_string())); + extras.push(("COYOTE_LLM_STATUS", api_err.status.to_string())); + } else { + extras.push(("COYOTE_LLM_ERROR_KIND", "other".to_string())); + } + extras.push(( + "COYOTE_LLM_ERROR", + // `{:#}` renders the whole context chain, so the root cause survives + // the `context(...)` wrapping the client trait defaults apply. + err.map(|err| format!("{err:#}")) + .unwrap_or_else(|| "Aborted.".to_string()), + )); + + extras +} + pub async fn call_chat_completions( input: &Input, print: bool, @@ -557,15 +599,28 @@ pub async fn call_chat_completions( let is_child_agent = ctx.current_depth > 0; let suppress_spinner = is_child_agent || ctx.render_mode == RenderMode::Silent; let spinner_message = if suppress_spinner { "" } else { "Generating" }; + hooks::fire( + HookEvent::LlmRequestStarted, + ctx, + &llm_request_extras(client), + None, + ); + let request_started_at = Instant::now(); let ret = abortable_run_with_spinner( client.chat_completions(input.clone()), spinner_message, - abort_signal, + abort_signal.clone(), ) .await; match ret { Ok(ret) => { + hooks::fire( + HookEvent::LlmRequestCompleted, + ctx, + &llm_completed_extras(client, request_started_at), + None, + ); let ChatCompletionsOutput { mut text, tool_calls, @@ -583,7 +638,15 @@ pub async fn call_chat_completions( ctx.record_token_usage(usage, client.model()); finish_completion(ctx, text, tool_calls, thinking).await } - Err(err) => Err(err), + Err(err) => { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, Some(&err), abort_signal.aborted()), + None, + ); + Err(err) + } } } @@ -600,6 +663,13 @@ pub async fn call_chat_completions_streaming( handler.set_silent(true); } + hooks::fire( + HookEvent::LlmRequestStarted, + ctx, + &llm_request_extras(client), + None, + ); + let request_started_at = Instant::now(); let (send_ret, render_ret) = tokio::join!( client.chat_completions_streaming(input, &mut handler), render_stream(rx, client.app_config(), abort_signal.clone(), silent), @@ -615,12 +685,32 @@ pub async fn call_chat_completions_streaming( ctx.record_token_usage(usage, client.model()); if aborted_ctrld { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, None, true), + None, + ); bail!("Aborted."); } - render_ret?; + if let Err(err) = render_ret { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, Some(&err), false), + None, + ); + return Err(err); + } if aborted_ctrlc { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, None, true), + None, + ); if !ctx.working_mode.is_repl() || ctx.session.is_none() { bail!("Aborted."); } @@ -644,6 +734,12 @@ pub async fn call_chat_completions_streaming( match send_ret { Ok(_) => { + hooks::fire( + HookEvent::LlmRequestCompleted, + ctx, + &llm_completed_extras(client, request_started_at), + None, + ); if !silent && !text.is_empty() && !text.ends_with('\n') { println!(); } @@ -653,6 +749,12 @@ pub async fn call_chat_completions_streaming( if !silent && !text.is_empty() { println!(); } + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, Some(&err), false), + None, + ); Err(err) } } @@ -675,10 +777,36 @@ pub async fn call_chat_completions_streaming_quiet( ctx: &mut RequestContext, abort_signal: AbortSignal, ) -> Result<(String, Vec)> { + hooks::fire( + HookEvent::LlmRequestStarted, + ctx, + &llm_request_extras(client), + None, + ); + let request_started_at = Instant::now(); if client.model().no_stream() { let output = tokio::select! { - ret = client.chat_completions(input.clone()) => ret?, - _ = wait_abort_signal(&abort_signal) => bail!("Aborted."), + ret = client.chat_completions(input.clone()) => match ret { + Ok(output) => output, + Err(err) => { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, Some(&err), false), + None, + ); + return Err(err); + } + }, + _ = wait_abort_signal(&abort_signal) => { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, None, true), + None, + ); + bail!("Aborted.") + } }; let ChatCompletionsOutput { text, @@ -688,8 +816,20 @@ pub async fn call_chat_completions_streaming_quiet( } = output; ctx.record_token_usage(usage, client.model()); if abort_signal.aborted() { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, None, true), + None, + ); bail!("Aborted."); } + hooks::fire( + HookEvent::LlmRequestCompleted, + ctx, + &llm_completed_extras(client, request_started_at), + None, + ); return finish_completion(ctx, text, tool_calls, thinking).await; } @@ -705,10 +845,30 @@ pub async fn call_chat_completions_streaming_quiet( ctx.record_token_usage(usage, client.model()); if abort_signal.aborted() { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, None, true), + None, + ); bail!("Aborted."); } - send_ret?; + if let Err(err) = send_ret { + hooks::fire( + HookEvent::LlmRequestFailed, + ctx, + &llm_failed_extras(client, Some(&err), false), + None, + ); + return Err(err); + } + hooks::fire( + HookEvent::LlmRequestCompleted, + ctx, + &llm_completed_extras(client, request_started_at), + None, + ); finish_completion(ctx, text, tool_calls, thinking).await } @@ -969,6 +1129,7 @@ mod tests { use super::super::access_token::{is_rejected, set_access_token}; use crate::config::{AppState, WorkingMode}; + use anyhow::Error; use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use tokio::time::Instant; @@ -1185,8 +1346,8 @@ mod tests { /// Wrapped in `.context(...)` so every test below proves the downcast /// works through an anyhow context chain, as in the trait methods. - fn api_status_error(status: u16) -> anyhow::Error { - anyhow::Error::new(ApiStatusError { + fn api_status_error(status: u16) -> Error { + Error::new(ApiStatusError { status, message: format!("error (status: {status})"), }) @@ -1524,4 +1685,331 @@ mod tests { assert_eq!(client.streaming_calls.load(Ordering::SeqCst), 1); assert_paced_reply(&text, &results); } + + /// Fake provider whose every request fails with an [`ApiStatusError`], + /// wrapped in the same context chain the real trait defaults produce. + struct FailingStatusClient { + config: AppConfig, + model: Model, + status: u16, + } + + impl FailingStatusClient { + fn new(status: u16) -> Self { + Self { + config: AppConfig::default(), + model: Model::default(), + status, + } + } + + fn error(&self) -> Error { + Error::new(ApiStatusError { + status: self.status, + message: format!("error (status: {})", self.status), + }) + } + } + + #[async_trait::async_trait] + impl Client for FailingStatusClient { + fn app_config(&self) -> &AppConfig { + &self.config + } + + fn extra_config(&self) -> Option<&ExtraConfig> { + None + } + + fn patch_config(&self) -> Option<&RequestPatch> { + None + } + + fn name(&self) -> &str { + "failing-status" + } + + fn model(&self) -> &Model { + &self.model + } + + async fn chat_completions_inner( + &self, + _client: &ReqwestClient, + _data: ChatCompletionsData, + ) -> Result { + Err(self.error()) + } + + async fn chat_completions_streaming_inner( + &self, + _client: &ReqwestClient, + _handler: &mut SseHandler, + _data: ChatCompletionsData, + ) -> Result<()> { + Err(self.error()) + } + } + + fn llm_hooks_map(marker: &str) -> hooks::HooksMap { + [ + "llm.request.started", + "llm.request.completed", + "llm.request.failed", + ] + .into_iter() + .map(|event| { + ( + event.to_string(), + vec![crate::hooks::HookDef { + name: marker.to_string(), + command: "true".to_string(), + }], + ) + }) + .collect() + } + + fn llm_hooked_ctx(marker: &str) -> RequestContext { + let mut app = AppState::test_default(); + app.config = Arc::new(AppConfig { + hooks: llm_hooks_map(marker), + ..Default::default() + }); + let mut ctx = RequestContext::new(Arc::new(app), WorkingMode::Cmd); + ctx.render_mode = RenderMode::Silent; + ctx + } + + fn llm_captures(marker: &str) -> Vec { + hooks::test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name == marker) + .collect() + } + + #[tokio::test(start_paused = true)] + #[serial_test::serial] + async fn plain_transport_fires_llm_request_started_and_completed() { + let marker = "llm-hooks-plain-ok-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let mut client = paced_client(false); + client.nonstream_replies = true; + let _guard = hooks::test_sink::install(); + + call_chat_completions( + &input, + false, + false, + &client, + &mut ctx, + create_abort_signal(), + ) + .await + .unwrap(); + + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::LlmRequestStarted); + assert_eq!( + captures[0] + .envs + .get("COYOTE_LLM_PROVIDER") + .map(String::as_str), + Some("paced-stream") + ); + assert!(captures[0].envs.contains_key("COYOTE_LLM_MODEL")); + assert_eq!(captures[1].event, HookEvent::LlmRequestCompleted); + assert!(captures[1].envs.contains_key("COYOTE_LLM_DURATION_MS")); + assert!(!captures[1].envs.contains_key("COYOTE_LLM_ERROR_KIND")); + } + + #[tokio::test] + #[serial_test::serial] + async fn plain_transport_fires_llm_request_failed_with_api_error_kind() { + let marker = "llm-hooks-plain-api-err-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = FailingStatusClient::new(429); + let _guard = hooks::test_sink::install(); + + let err = call_chat_completions( + &input, + false, + false, + &client, + &mut ctx, + create_abort_signal(), + ) + .await + .unwrap_err(); + + assert!( + err.downcast_ref::().is_some(), + "hooks must not change the error: {err:?}" + ); + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::LlmRequestStarted); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::LlmRequestFailed); + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR_KIND").map(String::as_str), + Some("api_error") + ); + assert_eq!( + failed.envs.get("COYOTE_LLM_STATUS").map(String::as_str), + Some("429") + ); + assert!( + failed + .envs + .get("COYOTE_LLM_ERROR") + .unwrap() + .contains("error (status: 429)") + ); + assert!(!failed.envs.contains_key("COYOTE_LLM_DURATION_MS")); + } + + #[tokio::test(start_paused = true)] + #[serial_test::serial] + async fn streaming_transport_fires_llm_request_started_and_completed() { + let marker = "llm-hooks-stream-ok-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = paced_client(false); + let _guard = hooks::test_sink::install(); + + call_chat_completions_streaming(&input, &client, &mut ctx, create_abort_signal()) + .await + .unwrap(); + + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::LlmRequestStarted); + assert_eq!(captures[1].event, HookEvent::LlmRequestCompleted); + assert!(captures[1].envs.contains_key("COYOTE_LLM_DURATION_MS")); + } + + #[tokio::test(start_paused = true)] + #[serial_test::serial] + async fn streaming_transport_abort_fires_llm_request_failed_aborted() { + let marker = "llm-hooks-stream-abort-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = paced_client(true); + let abort = create_abort_signal(); + let trigger = abort.clone(); + tokio::spawn(async move { + tokio::time::sleep(PACED_GAP * PACED_DELTAS as u32 + Duration::from_secs(30)).await; + trigger.set_ctrlc(); + }); + let _guard = hooks::test_sink::install(); + + let err = call_chat_completions_streaming(&input, &client, &mut ctx, abort) + .await + .expect_err("an aborted stream must not succeed"); + + assert_eq!(err.to_string(), "Aborted."); + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::LlmRequestFailed); + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR_KIND").map(String::as_str), + Some("aborted") + ); + assert!(!failed.envs.contains_key("COYOTE_LLM_STATUS")); + } + + #[tokio::test(start_paused = true)] + #[serial_test::serial] + async fn quiet_transport_fires_llm_request_started_and_completed() { + let marker = "llm-hooks-quiet-ok-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = paced_client(false); + let _guard = hooks::test_sink::install(); + + call_chat_completions_streaming_quiet(&input, &client, &mut ctx, create_abort_signal()) + .await + .unwrap(); + + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::LlmRequestStarted); + assert_eq!( + captures[0] + .envs + .get("COYOTE_LLM_PROVIDER") + .map(String::as_str), + Some("paced-stream") + ); + assert_eq!(captures[1].event, HookEvent::LlmRequestCompleted); + assert!(captures[1].envs.contains_key("COYOTE_LLM_DURATION_MS")); + } + + #[tokio::test(start_paused = true)] + #[serial_test::serial] + async fn quiet_transport_abort_fires_llm_request_failed_aborted() { + let marker = "llm-hooks-quiet-abort-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = paced_client(true); + let abort = create_abort_signal(); + let trigger = abort.clone(); + tokio::spawn(async move { + tokio::time::sleep(PACED_GAP * PACED_DELTAS as u32 + Duration::from_secs(30)).await; + trigger.set_ctrlc(); + }); + let _guard = hooks::test_sink::install(); + + let err = call_chat_completions_streaming_quiet(&input, &client, &mut ctx, abort) + .await + .expect_err("an aborted stream must not succeed"); + + assert_eq!(err.to_string(), "Aborted."); + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::LlmRequestFailed); + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR_KIND").map(String::as_str), + Some("aborted") + ); + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR").map(String::as_str), + Some("Aborted.") + ); + } + + #[tokio::test] + #[serial_test::serial] + async fn quiet_transport_fires_llm_request_failed_with_api_error_kind() { + let marker = "llm-hooks-quiet-api-err-x7x"; + let mut ctx = llm_hooked_ctx(marker); + let input = Input::from_str(&ctx, "hi", None).unwrap(); + let client = FailingStatusClient::new(500); + let _guard = hooks::test_sink::install(); + + let err = + call_chat_completions_streaming_quiet(&input, &client, &mut ctx, create_abort_signal()) + .await + .unwrap_err(); + + assert!(err.downcast_ref::().is_some()); + let captures = llm_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::LlmRequestFailed); + assert_eq!( + failed.envs.get("COYOTE_LLM_ERROR_KIND").map(String::as_str), + Some("api_error") + ); + assert_eq!( + failed.envs.get("COYOTE_LLM_STATUS").map(String::as_str), + Some("500") + ); + } } diff --git a/src/config/agent.rs b/src/config/agent.rs index dd1a690..c7ba01c 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -1,5 +1,6 @@ use super::*; +use crate::hooks::HooksMap; use crate::{ client::Model, config::memory, @@ -12,6 +13,7 @@ use crate::{ }; use super::rag_cache::RagKey; +use crate::config::builtin_manifest; use crate::config::paths; use crate::config::prompts::{ DEFAULT_JOB_INSTRUCTIONS, DEFAULT_SPAWN_INSTRUCTIONS, DEFAULT_TEAMMATE_INSTRUCTIONS, @@ -27,6 +29,7 @@ use fancy_regex::Captures; use inquire::{Text, validator::Validation}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; +use std::collections::BTreeSet; use std::{env, ffi::OsStr, path::Path}; const DEFAULT_AGENT_NAME: &str = "rag"; @@ -46,6 +49,20 @@ pub type AgentVariables = IndexMap; #[folder = "assets/agents/"] struct AgentAssets; +/// Splits an embedded asset path of the form `/hooks/` into +/// its agent and hook-file names. Only direct children of `hooks/` count: +/// they are the only shape the builtin manifest tracks. +fn parse_direct_hook_path(path: &str) -> Option<(&str, &str)> { + let (agent, rest) = path.split_once('/')?; + let hook = direct_hook_name(rest)?; + Some((agent, hook)) +} + +fn direct_hook_name(rest: &str) -> Option<&str> { + let name = rest.strip_prefix("hooks/")?; + (!name.is_empty() && !name.contains('/')).then_some(name) +} + #[derive(Debug, Clone)] pub struct Agent { name: String, @@ -70,6 +87,7 @@ impl Agent { paths::agents_data_dir().display() ); + let mut written_hooks: HashMap> = HashMap::new(); for file in AgentAssets::iter() { debug!("Processing agent file: {}", file.as_ref()); @@ -96,6 +114,12 @@ impl Agent { info!("Creating agent file: {}", file_path.display()); let mut agent_file = File::create(&file_path)?; agent_file.write_all(content.as_bytes())?; + if let Some((agent, hook)) = parse_direct_hook_path(file.as_ref()) { + written_hooks + .entry(agent.to_string()) + .or_default() + .insert(hook.to_string()); + } #[cfg(unix)] if is_script { @@ -134,6 +158,27 @@ impl Agent { } } + // Hook filenames are open-ended (unlike AGENT_DEFINITION_FILES), so + // each bundled agent's hooks/ directory reconciles through its + // builtin manifest: only files the installer previously shipped are + // removal candidates, and user-created files in the same directory + // are never touched. Only direct children of hooks/ are tracked; + // nested shipped files install but are not reconciled. + for (agent, files) in &bundled_files { + let shipped: BTreeSet = files + .iter() + .filter_map(|rest| direct_hook_name(rest)) + .map(str::to_string) + .collect(); + let written = written_hooks.remove(agent.as_str()).unwrap_or_default(); + let hooks_dir = paths::agents_data_dir().join(agent).join("hooks"); + if let Err(err) = + builtin_manifest::reconcile_builtin_dir(&hooks_dir, &shipped, &written) + { + warn!("Failed to reconcile builtin hooks for agent '{agent}': {err}"); + } + } + Ok(()) } @@ -639,6 +684,14 @@ impl Agent { self.config.compression_model.as_deref() } + pub fn hooks(&self) -> &HooksMap { + &self.config.hooks + } + + pub fn global_hooks(&self) -> &[String] { + &self.config.global_hooks + } + pub fn is_dynamic_instructions(&self) -> bool { self.config.dynamic_instructions } @@ -848,6 +901,10 @@ pub struct AgentConfig { pub mcp_tools: Option>>, #[serde(default)] pub global_tools: Vec, + #[serde(default)] + pub hooks: HooksMap, + #[serde(default)] + pub global_hooks: Vec, #[serde(skip_serializing_if = "Option::is_none")] pub skills_enabled: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -917,6 +974,8 @@ impl AgentConfig { reasoning_effort: graph.reasoning_effort.clone(), description: graph.description.clone(), global_tools: graph.global_tools.clone(), + hooks: graph.hooks.clone(), + global_hooks: graph.global_hooks.clone(), mcp_servers: graph.mcp_servers.clone(), mcp_tools: graph.mcp_tools.clone(), skills_enabled: graph.skills_enabled, @@ -965,6 +1024,14 @@ impl AgentConfig { { self.global_tools = v; } + if let Ok(v) = env::var(with_prefix("global_hooks")) { + match serde_json::from_str(&v) { + Ok(v) => self.global_hooks = v, + Err(err) => { + debug!("Ignoring malformed global_hooks env override for agent '{name}': {err}") + } + } + } if let Ok(v) = env::var(with_prefix("mcp_servers")) && let Ok(v) = serde_json::from_str(&v) { @@ -1295,6 +1362,16 @@ pub fn complete_agent_variables(agent_name: &str) -> Vec<(String, Option mod tests { use super::*; + #[test] + fn parse_direct_hook_path_accepts_only_direct_hook_children() { + assert_eq!(parse_direct_hook_path("x/hooks/a.sh"), Some(("x", "a.sh"))); + assert_eq!(parse_direct_hook_path("x/hooks/nested/a.sh"), None); + assert_eq!(parse_direct_hook_path("hooks/a.sh"), None); + assert_eq!(parse_direct_hook_path("x/hooks/"), None); + assert_eq!(parse_direct_hook_path("x/config.yaml"), None); + assert_eq!(parse_direct_hook_path("config.yaml"), None); + } + #[test] fn agent_config_parses_from_yaml() { let yaml = r#" @@ -1336,6 +1413,92 @@ variables: assert_eq!(config.variables[0].name, "username"); } + #[test] + fn agent_config_hooks_and_global_hooks_round_trip() { + let yaml = "\ +name: hooked +instructions: hi +hooks: + tool.started: + - name: notify + command: ./hooks/notify.sh +global_hooks: + - tool.started.notify +"; + let config: AgentConfig = serde_yaml::from_str(yaml).unwrap(); + + assert_eq!(config.hooks["tool.started"][0].name, "notify"); + assert_eq!(config.hooks["tool.started"][0].command, "./hooks/notify.sh"); + assert_eq!(config.global_hooks, vec!["tool.started.notify"]); + + let serialized = serde_yaml::to_string(&config).unwrap(); + let reparsed: AgentConfig = serde_yaml::from_str(&serialized).unwrap(); + + assert_eq!(reparsed.hooks, config.hooks); + assert_eq!(reparsed.global_hooks, config.global_hooks); + } + + #[test] + fn load_envs_overrides_global_hooks() { + let yaml = "name: hooks-env-probe\ninstructions: hi\nglobal_hooks:\n - initial.hook\n"; + let mut config: AgentConfig = serde_yaml::from_str(yaml).unwrap(); + let env_name = normalize_env_name("hooks-env-probe_global_hooks"); + let prev = env::var_os(&env_name); + + unsafe { + env::set_var( + &env_name, + r#"["tool.started.notify","turn.completed.webhook"]"#, + ) + }; + config.load_envs(&AppConfig::default()); + assert_eq!( + config.global_hooks, + vec!["tool.started.notify", "turn.completed.webhook"] + ); + + unsafe { env::set_var(&env_name, "not json") }; + config.load_envs(&AppConfig::default()); + assert_eq!( + config.global_hooks, + vec!["tool.started.notify", "turn.completed.webhook"] + ); + + unsafe { + match prev { + Some(v) => env::set_var(&env_name, v), + None => env::remove_var(&env_name), + } + } + } + + #[test] + fn from_graph_carries_hooks_and_global_hooks() { + let yaml = "\ +name: g +start: e +hooks: + turn.completed: + - name: webhook + command: curl -s https://example.com/hook +global_hooks: + - turn.completed.webhook +nodes: + e: + id: e + type: end + output: done +"; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + + let config = AgentConfig::from_graph("g", &graph); + + assert_eq!(config.hooks, graph.hooks); + assert_eq!(config.global_hooks, graph.global_hooks); + assert_eq!(config.hooks["turn.completed"][0].name, "webhook"); + assert_eq!(config.global_hooks, vec!["turn.completed.webhook"]); + } + #[test] fn agent_config_defaults() { let yaml = "name: minimal\ninstructions: hi\n"; @@ -1350,6 +1513,8 @@ variables: assert_eq!(config.escalation_timeout, 0); assert!(config.mcp_servers.is_empty()); assert!(config.global_tools.is_empty()); + assert!(config.hooks.is_empty()); + assert!(config.global_hooks.is_empty()); assert!(config.conversation_starters.is_empty()); assert!(config.variables.is_empty()); assert!(config.model_id.is_none()); diff --git a/src/config/app_config.rs b/src/config/app_config.rs index 7682c6a..bfa2fb9 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -1,4 +1,5 @@ use crate::client::{ClientConfig, Model, ModelType, list_models}; +use crate::hooks::HooksMap; use crate::render::{MarkdownRender, RenderOptions}; use crate::utils::{IS_STDOUT_TERMINAL, NO_COLOR, decode_bin, drain_stale_tty_input, get_env_name}; @@ -52,6 +53,9 @@ pub struct AppConfig { pub enabled_mcp_servers: Option>, pub mcp_tools: Option>>, + #[serde(default)] + pub hooks: HooksMap, + pub auto_continue: bool, pub max_auto_continues: usize, pub inject_todo_instructions: bool, @@ -143,6 +147,8 @@ impl Default for AppConfig { enabled_mcp_servers: None, mcp_tools: None, + hooks: Default::default(), + auto_continue: false, max_auto_continues: 10, inject_todo_instructions: true, @@ -235,6 +241,8 @@ impl AppConfig { enabled_mcp_servers: config.enabled_mcp_servers, mcp_tools: config.mcp_tools, + hooks: config.hooks, + auto_continue: config.auto_continue, max_auto_continues: config.max_auto_continues, inject_todo_instructions: config.inject_todo_instructions, @@ -572,6 +580,15 @@ impl AppConfig { self.enabled_mcp_servers = v.map(|raw| super::csv_to_vec(&raw)); } + if let Ok(v) = env::var(get_env_name("hooks")) { + match serde_json::from_str(&v) { + Ok(v) => self.hooks = v, + Err(err) => { + debug!("Ignoring malformed hooks env override: {err}") + } + } + } + if let Some(v) = super::read_env_value::(&get_env_name("repl_prelude")) { self.repl_prelude = v; } @@ -788,6 +805,77 @@ mod tests { assert_eq!(app.clients.len(), 1); } + #[test] + fn from_config_copies_hooks() { + let mut hooks = HooksMap::new(); + hooks.insert( + "turn.completed".to_string(), + vec![crate::hooks::HookDef { + name: "notify".to_string(), + command: "./hooks/notify.sh".to_string(), + }], + ); + let cfg = Config { + model_id: "test-model".to_string(), + clients: vec![ClientConfig::default()], + hooks: hooks.clone(), + ..Config::default() + }; + + let app = AppConfig::from_config(cfg).unwrap(); + + assert_eq!(app.hooks, hooks); + } + + #[test] + fn from_config_defaults_hooks_to_empty() { + let cfg = Config { + model_id: "test-model".to_string(), + clients: vec![ClientConfig::default()], + ..Config::default() + }; + let app = AppConfig::from_config(cfg).unwrap(); + + assert!(app.hooks.is_empty()); + } + + #[test] + #[serial_test::serial] + fn load_envs_overrides_hooks() { + let env_name = get_env_name("hooks"); + let prev = env::var_os(&env_name); + + let mut hooks = HooksMap::new(); + hooks.insert( + "turn.completed".to_string(), + vec![crate::hooks::HookDef { + name: "notify".to_string(), + command: "./hooks/notify.sh".to_string(), + }], + ); + + let mut app = AppConfig::default(); + + unsafe { env::set_var(&env_name, serde_json::to_string(&hooks).unwrap()) }; + app.load_envs(); + assert_eq!(app.hooks, hooks); + + unsafe { env::set_var(&env_name, "[not json") }; + app.load_envs(); + assert_eq!(app.hooks, hooks); + + unsafe { env::remove_var(&env_name) }; + app.load_envs(); + assert_eq!(app.hooks, hooks); + + unsafe { + match prev { + Some(v) => env::set_var(&env_name, v), + None => env::remove_var(&env_name), + } + } + } + #[test] fn from_config_copies_mapping_fields() { let mut cfg = Config { diff --git a/src/config/builtin_manifest.rs b/src/config/builtin_manifest.rs new file mode 100644 index 0000000..7e3bc31 --- /dev/null +++ b/src/config/builtin_manifest.rs @@ -0,0 +1,293 @@ +//! Per-directory tracking of files the builtin installer shipped, so a later +//! release that stops shipping an asset can remove the stale copy without +//! ever touching user-created files that live in the same directory. +//! +//! Unlike agent definition files, hook filenames are open-ended: no closed +//! candidate list can distinguish "previously bundled" from "user-authored". +//! The manifest records exactly what the installer shipped; anything absent +//! from it is user-owned and untouchable. + +use crate::config::ensure_parent_exists; +use crate::function::write_file_atomic; + +use anyhow::{Context, Result}; +use std::collections::BTreeSet; +use std::fs; +use std::path::Path; + +pub(crate) const BUILTIN_MANIFEST_FILE: &str = ".builtin-manifest"; + +pub(crate) fn is_builtin_manifest_name(name: &str) -> bool { + name.trim_end_matches(['.', ' ']) + .to_uppercase() + .eq_ignore_ascii_case(BUILTIN_MANIFEST_FILE) +} + +/// Reconciles `dir` against the currently shipped set of builtin filenames: +/// a file is removed ONLY if it appears in the previous manifest AND is +/// absent from `shipped`. The manifest is then rewritten to the names this +/// installer actually owns: files written this run (`written`), plus shipped +/// names it already owned, plus stale files whose removal failed (kept so a +/// later run retries). A shipped name the installer declined to write (e.g., a +/// pre-existing user file with a colliding name) is never claimed. +/// +/// Failure modes degrade toward keeping files: a missing or unreadable +/// manifest yields no deletions, and entries that are not plain filenames +/// are ignored. +pub(crate) fn reconcile_builtin_dir( + dir: &Path, + shipped: &BTreeSet, + written: &BTreeSet, +) -> Result<()> { + let previous = read_manifest(dir); + let mut retained: BTreeSet = written.clone(); + retained.extend(shipped.intersection(&previous).cloned()); + for name in previous.difference(shipped) { + let path = dir.join(name); + if !path.is_file() { + continue; + } + info!( + "Removing stale builtin file no longer shipped: {}", + path.display() + ); + if let Err(err) = fs::remove_file(&path) { + warn!( + "Failed to remove stale builtin file {}: {err}", + path.display() + ); + retained.insert(name.clone()); + } + } + + write_manifest(dir, &retained) +} + +fn read_manifest(dir: &Path) -> BTreeSet { + let path = dir.join(BUILTIN_MANIFEST_FILE); + let content = match fs::read_to_string(&path) { + Ok(content) => content, + Err(err) => { + debug!( + "No readable builtin manifest at {} ({err}); skipping stale-file removal", + path.display() + ); + return BTreeSet::new(); + } + }; + + content + .lines() + .map(str::trim) + .filter(|line| { + if line.is_empty() || is_builtin_manifest_name(line) { + return false; + } + if !is_plain_file_name(line) { + debug!( + "Ignoring suspicious builtin manifest entry in {}: {line:?}", + path.display() + ); + return false; + } + true + }) + .map(str::to_string) + .collect() +} + +/// Manifest entries name direct children of the manifest's directory and +/// nothing else; anything path-like is rejected so a corrupted or crafted +/// manifest can never direct a deletion outside that directory. The same +/// component rules the bundle store applies to recorded paths are reused so +/// Windows drive prefixes (`C:x`), ADS colons, trailing dots/spaces, and +/// reserved device names are rejected too. +fn is_plain_file_name(name: &str) -> bool { + !name.contains(['/', '\\']) + && name != "." + && name != ".." + && super::install_remote::is_safe_component(name) +} + +fn write_manifest(dir: &Path, names: &BTreeSet) -> Result<()> { + let path = dir.join(BUILTIN_MANIFEST_FILE); + if names.is_empty() { + if path.is_file() { + fs::remove_file(&path) + .with_context(|| format!("failed to remove builtin manifest {}", path.display()))?; + } + return Ok(()); + } + + let mut content = names.iter().cloned().collect::>().join("\n"); + content.push('\n'); + ensure_parent_exists(&path)?; + write_file_atomic(&path, &content, None) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::utils; + use std::path::PathBuf; + + fn fresh_dir(label: &str) -> PathBuf { + let dir = utils::temp_file(label, ""); + fs::create_dir_all(&dir).unwrap(); + dir + } + + fn shipped(names: &[&str]) -> BTreeSet { + names.iter().map(|n| n.to_string()).collect() + } + + #[test] + fn removes_only_manifest_listed_files_absent_from_shipped() { + let dir = fresh_dir("builtin-manifest-stale-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), "old.sh\nkept.sh\n").unwrap(); + fs::write(dir.join("old.sh"), "stale").unwrap(); + fs::write(dir.join("kept.sh"), "current").unwrap(); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["kept.sh"]), &shipped(&[])).unwrap(); + + assert!(!dir.join("old.sh").exists(), "stale bundled file removed"); + assert!(dir.join("kept.sh").exists(), "still-shipped file kept"); + assert!(dir.join("user.sh").exists(), "unlisted user file untouched"); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "kept.sh\n" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn missing_manifest_deletes_nothing_and_writes_shipped_set() { + let dir = fresh_dir("builtin-manifest-missing-"); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["new.sh"]), &shipped(&["new.sh"])).unwrap(); + + assert!(dir.join("user.sh").exists()); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "new.sh\n" + ); + let _ = fs::remove_dir_all(&dir); + } + + /// A shipped name the installer skipped (pre-existing user file with a + /// colliding name) is never claimed by the manifest, so a later release + /// dropping that hook can never delete the user's file. + #[test] + fn skipped_preexisting_user_file_is_never_claimed() { + let dir = fresh_dir("builtin-manifest-unclaimed-"); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["user.sh"]), &shipped(&[])).unwrap(); + assert!( + !dir.join(BUILTIN_MANIFEST_FILE).exists(), + "nothing was written this run and nothing was owned before" + ); + + reconcile_builtin_dir(&dir, &shipped(&[]), &shipped(&[])).unwrap(); + assert!( + dir.join("user.sh").exists(), + "the unclaimed user file survives the hook being dropped" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn suspicious_manifest_entries_never_direct_deletions() { + let dir = fresh_dir("builtin-manifest-suspicious-"); + let outside = fresh_dir("builtin-manifest-outside-"); + let escape = format!( + "../{}/victim.sh", + outside.file_name().unwrap().to_string_lossy() + ); + fs::write( + dir.join(BUILTIN_MANIFEST_FILE), + format!("{escape}\nsub/nested.sh\nC:victim.sh\ntrailing.\n..\n.\n\n"), + ) + .unwrap(); + fs::write(outside.join("victim.sh"), "keep").unwrap(); + fs::create_dir_all(dir.join("sub")).unwrap(); + fs::write(dir.join("sub/nested.sh"), "keep").unwrap(); + + reconcile_builtin_dir(&dir, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + + assert!(outside.join("victim.sh").exists()); + assert!(dir.join("sub/nested.sh").exists()); + let _ = fs::remove_dir_all(&dir); + let _ = fs::remove_dir_all(&outside); + } + + #[test] + fn unreadable_manifest_deletes_nothing() { + let dir = fresh_dir("builtin-manifest-unreadable-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), [0xff, 0xfe, 0x00]).unwrap(); + fs::write(dir.join("old.sh"), "keep").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["new.sh"]), &shipped(&["new.sh"])).unwrap(); + + assert!( + dir.join("old.sh").exists(), + "a bad manifest must fail safe toward keeping files" + ); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "new.sh\n", + "manifest self-heals to the current shipped set" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn empty_shipped_set_removes_manifest_and_creates_nothing() { + let dir = fresh_dir("builtin-manifest-empty-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), "old.sh\n").unwrap(); + fs::write(dir.join("old.sh"), "stale").unwrap(); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + + assert!(!dir.join("old.sh").exists()); + assert!(dir.join("user.sh").exists()); + assert!(!dir.join(BUILTIN_MANIFEST_FILE).exists()); + + let absent = dir.join("never-created"); + reconcile_builtin_dir(&absent, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + assert!(!absent.exists(), "no directory is created for an empty set"); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn manifest_listing_itself_never_deletes_it() { + let dir = fresh_dir("builtin-manifest-self-"); + fs::write( + dir.join(BUILTIN_MANIFEST_FILE), + format!("{BUILTIN_MANIFEST_FILE}\n.BUILTIN-MANIFEST\nkept.sh\n"), + ) + .unwrap(); + fs::write(dir.join("kept.sh"), "current").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["kept.sh"]), &shipped(&[])).unwrap(); + + assert!(dir.join(BUILTIN_MANIFEST_FILE).exists()); + assert!(dir.join("kept.sh").exists()); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn manifest_name_matcher_covers_filesystem_aliases() { + assert!(is_builtin_manifest_name(".builtin-manifest")); + assert!(is_builtin_manifest_name(".BUILTIN-MANIFEST")); + assert!(is_builtin_manifest_name(".Builtin-Manifest.")); + assert!(is_builtin_manifest_name(".builtin-manifest ")); + assert!(is_builtin_manifest_name(".builtin-manifeſt")); + assert!(!is_builtin_manifest_name("builtin-manifest")); + assert!(!is_builtin_manifest_name(".builtin-manifesto")); + assert!(!is_builtin_manifest_name("notify.sh")); + } +} diff --git a/src/config/bundles.rs b/src/config/bundles.rs index c48c560..caa3ce2 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -1183,6 +1183,13 @@ mod tests { assert_eq!(resolved.name, "y/mcp_config"); assert_eq!(resolved.qualified_from.as_deref(), Some("mcp_config")); + + let resolved = store + .resolve_bundle_name("https://github.com/z/hooks", None) + .unwrap(); + + assert_eq!(resolved.name, "z/hooks"); + assert_eq!(resolved.qualified_from.as_deref(), Some("hooks")); } #[test] diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 18fdcea..01d9424 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -2,6 +2,7 @@ use super::bundles::{ BundleStore, FileAction, FileRecord, InstallMetadata, McpAction, McpServerRecord, hash_bytes, hash_file, }; +use crate::config::builtin_manifest::is_builtin_manifest_name; use crate::config::{AssetCategory, BUNDLE_MANIFEST_FILE, InstallFilter, paths}; #[cfg(not(windows))] use crate::function::Language; @@ -32,7 +33,7 @@ pub fn install_remote(git_url: &str, filter: Option, force: bool) if layout.is_empty() { println!( "No recognized assets found in {git_url}. Expected one or more of: \ - agents/, roles/, skills/, macros/, functions/tools/, mcp.json" + agents/, roles/, skills/, macros/, functions/tools/, hooks/, mcp.json" ); return Ok(()); } @@ -805,7 +806,7 @@ fn is_safe_relative_path(path: &str) -> bool { /// Rejects names Windows refuses or silently rewrites (alternate data stream /// colons, reserved device names, trailing dots or spaces) so a recorded path /// denotes the same regular file on every platform. -fn is_safe_component(name: &str) -> bool { +pub(crate) fn is_safe_component(name: &str) -> bool { !name.contains(':') && !name.ends_with('.') && !name.ends_with(' ') @@ -1270,6 +1271,7 @@ struct RemoteLayout { roles: Option, skills: Option, macros: Option, + hooks: Option, functions_tools: Option, mcp_json: Option, manifest: Option, @@ -1282,6 +1284,7 @@ impl RemoteLayout { && self.roles.is_none() && self.skills.is_none() && self.macros.is_none() + && self.hooks.is_none() && self.functions_tools.is_none() && self.mcp_json.is_none() } @@ -1313,6 +1316,11 @@ fn scan_remote_layout(root: &Path) -> Result { layout.macros = Some(macros); } + let hooks = root.join("hooks"); + if hooks.is_dir() { + layout.hooks = Some(hooks); + } + let root_mcp = root.join("mcp.json"); if root_mcp.is_file() { layout.mcp_json = Some(root_mcp); @@ -1540,6 +1548,10 @@ fn apply_filter(mut layout: RemoteLayout, filter: Option) -> Remo functions_tools: layout.functions_tools.take(), ..base }, + InstallFilter::Hooks => RemoteLayout { + hooks: layout.hooks.take(), + ..base + }, InstallFilter::McpConfig => RemoteLayout { mcp_json: layout.mcp_json.take(), ..base @@ -1591,6 +1603,7 @@ enum TopCategory { Roles, Skills, Macros, + Hooks, FunctionsTools, } @@ -1601,6 +1614,7 @@ impl TopCategory { TopCategory::Roles => "roles", TopCategory::Skills => "skills", TopCategory::Macros => "macros", + TopCategory::Hooks => "hooks", TopCategory::FunctionsTools => "functions/tools", } } @@ -1619,6 +1633,8 @@ enum PlannedKind { struct PlannedFile { src: PathBuf, dst: PathBuf, + /// Path relative to the category's source directory in the bundle. + rel: PathBuf, kind: PlannedKind, top_category: TopCategory, } @@ -1660,6 +1676,9 @@ fn plan_changes(layout: &RemoteLayout) -> Result { &mut files, )?; } + if let Some(src_dir) = &layout.hooks { + plan_dir_into(src_dir, &paths::hooks_dir(), TopCategory::Hooks, &mut files)?; + } if let Some(src_dir) = &layout.functions_tools { plan_dir_into( src_dir, @@ -1686,7 +1705,28 @@ fn plan_dir_into( for src in walk_files(src_dir)? { let rel = src .strip_prefix(src_dir) - .expect("walk_files only returns paths under src_dir"); + .expect("walk_files only returns paths under src_dir") + .to_path_buf(); + + // The builtin installer's per-directory manifests drive stale-file + // deletion; letting a bundle install one would hand it control over + // which local files the next builtin reconcile removes. Every path + // component is checked (a bundle-shipped DIRECTORY of that name would + // permanently occupy the manifest path and disable reconciliation), + // under the filesystem-alias-aware match of is_builtin_manifest_name. + let is_builtin_manifest = rel.components().any(|c| match c.as_os_str().to_str() { + Some(name) => is_builtin_manifest_name(name), + // A non-UTF-8 name can never equal the ASCII manifest constant. + None => false, + }); + if is_builtin_manifest { + log::warn!( + "Ignoring bundle file {} ({}): the name is reserved for the builtin installer", + rel.display(), + category.label() + ); + continue; + } if category == TopCategory::Skills { let skill_name = rel @@ -1707,11 +1747,12 @@ fn plan_dir_into( })?; } - let dst = dst_dir.join(rel); + let dst = dst_dir.join(&rel); let kind = classify_file(&src, &dst)?; out.push(PlannedFile { src, dst, + rel, kind, top_category: category, }); @@ -1790,6 +1831,7 @@ fn print_plan_summary(plan: &InstallPlan) { TopCategory::Roles, TopCategory::Skills, TopCategory::Macros, + TopCategory::Hooks, TopCategory::FunctionsTools, ] { let new_ = count_kind(plan, cat, PlannedKind::New); @@ -1808,6 +1850,33 @@ fn print_plan_summary(plan: &InstallPlan) { println!("{line}"); } } + + for line in hook_script_lines(plan) { + println!("{line}"); + } +} + +/// Bundle-shipped hook scripts execute on the user's machine once wired into +/// config, so every planned file landing under a hooks/ directory is called +/// out individually. Visibility only: installing them needs no extra approval. +fn hook_script_lines(plan: &InstallPlan) -> Vec { + plan.files + .iter() + .filter(|planned| { + planned.top_category == TopCategory::Hooks + || planned + .rel + .parent() + .is_some_and(|parent| parent.components().any(|c| c.as_os_str() == "hooks")) + }) + .map(|planned| { + let rel = planned.rel.to_string_lossy().replace('\\', "/"); + format!( + "bundle defines hook script: {}/{rel}", + planned.top_category.label() + ) + }) + .collect() } fn count_kind(plan: &InstallPlan, cat: TopCategory, kind: PlannedKind) -> usize { @@ -2349,6 +2418,8 @@ fn print_secret_summary(added: &[String], deferred: &[String]) { #[cfg(test)] mod tests { use super::*; + use crate::config; + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; use crate::sandbox::SANDBOX_ENV_FLAG; use crate::utils::get_env_name; use serial_test::serial; @@ -2551,6 +2622,7 @@ mod tests { roles: Some(PathBuf::from("r")), skills: Some(PathBuf::from("s")), macros: Some(PathBuf::from("m")), + hooks: Some(PathBuf::from("h")), functions_tools: Some(PathBuf::from("f")), mcp_json: Some(PathBuf::from("j")), ..RemoteLayout::default() @@ -2560,6 +2632,7 @@ mod tests { assert!(out.agents.is_some() && out.roles.is_some() && out.skills.is_some()); assert!(out.macros.is_some() && out.functions_tools.is_some() && out.mcp_json.is_some()); + assert!(out.hooks.is_some()); } #[test] @@ -2638,6 +2711,37 @@ mod tests { assert!(out.functions_tools.is_none() && out.mcp_json.is_none()); } + #[test] + fn apply_filter_hooks_keeps_only_hooks() { + let l = RemoteLayout { + agents: Some(PathBuf::from("a")), + roles: Some(PathBuf::from("r")), + skills: Some(PathBuf::from("s")), + macros: Some(PathBuf::from("m")), + hooks: Some(PathBuf::from("h")), + functions_tools: Some(PathBuf::from("f")), + mcp_json: Some(PathBuf::from("j")), + ..RemoteLayout::default() + }; + + let out = apply_filter(l, Some(InstallFilter::Hooks)); + + assert_eq!(out.hooks, Some(PathBuf::from("h"))); + assert!(!out.is_empty()); + assert!(out.agents.is_none() && out.roles.is_none() && out.skills.is_none()); + assert!(out.macros.is_none() && out.functions_tools.is_none() && out.mcp_json.is_none()); + } + + #[test] + fn remote_layout_with_only_hooks_is_not_empty() { + let l = RemoteLayout { + hooks: Some(PathBuf::from("h")), + ..RemoteLayout::default() + }; + + assert!(!l.is_empty()); + } + #[test] fn walk_files_skips_dot_git_and_collects_regular_files() { let root = fresh_temp_dir("walk-test-"); @@ -2684,6 +2788,7 @@ mod tests { fs::create_dir_all(root.join("roles")).unwrap(); fs::create_dir_all(root.join("skills")).unwrap(); fs::create_dir_all(root.join("macros")).unwrap(); + fs::create_dir_all(root.join("hooks")).unwrap(); fs::create_dir_all(root.join("functions/tools")).unwrap(); touch(&root.join("functions/mcp.json")); touch(&root.join("README.md")); @@ -2693,11 +2798,29 @@ mod tests { assert!(layout.roles.is_some()); assert!(layout.skills.is_some()); assert!(layout.macros.is_some()); + assert!(layout.hooks.is_some()); assert!(layout.functions_tools.is_some()); assert!(layout.mcp_json.is_some()); let _ = fs::remove_dir_all(&root); } + #[test] + fn scan_remote_layout_finds_hooks_only() { + let root = fresh_temp_dir("scan-hooks-only-"); + fs::create_dir_all(root.join("hooks")).unwrap(); + touch(&root.join("hooks/notify.sh")); + + let layout = scan_remote_layout(&root).unwrap(); + + assert_eq!(layout.hooks, Some(root.join("hooks"))); + assert!(layout.agents.is_none()); + assert!(layout.roles.is_none()); + assert!(layout.macros.is_none()); + assert!(layout.functions_tools.is_none()); + assert!(layout.mcp_json.is_none()); + let _ = fs::remove_dir_all(&root); + } + #[test] fn scan_remote_layout_finds_root_mcp_json() { let root = fresh_temp_dir("scan-root-mcp-test-"); @@ -2752,6 +2875,71 @@ mod tests { let _ = fs::remove_dir_all(&root); } + #[test] + fn plan_dir_into_skips_bundle_shipped_builtin_manifests() { + let root = fresh_temp_dir("plan-manifest-skip-"); + let src_dir = root.join("hooks"); + touch(&src_dir.join("notify.sh")); + touch(&src_dir.join(BUILTIN_MANIFEST_FILE)); + touch(&src_dir.join(format!("sub/{BUILTIN_MANIFEST_FILE}"))); + // Case and trailing-dot variants alias the real manifest path on + // case-insensitive filesystems; they live in separate subdirs so this + // fixture also builds on such filesystems without name collisions. + touch(&src_dir.join("sub2/.BUILTIN-MANIFEST")); + touch(&src_dir.join("sub3/.Builtin-Manifest.")); + // A DIRECTORY of the manifest name would occupy the manifest path and + // disable reconciliation; a Unicode case-fold alias (U+017F ſ → s) + // aliases it on case-insensitive APFS/NTFS volumes. + touch(&src_dir.join("sub4/.builtin-manifest/payload.sh")); + touch(&src_dir.join("sub5/.builtin-manifeſt")); + let dst_dir = root.join("dst"); + + let mut files = Vec::new(); + plan_dir_into(&src_dir, &dst_dir, TopCategory::Hooks, &mut files).unwrap(); + + let rels: Vec<&Path> = files.iter().map(|p| p.rel.as_path()).collect(); + assert_eq!( + rels, + vec![Path::new("notify.sh")], + "a bundle must never install a builtin manifest: it would hand the \ + bundle control over builtin stale-file deletion" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn hook_script_lines_cover_every_hooks_dir_and_nothing_else() { + let file = |rel: &str, cat: TopCategory| PlannedFile { + src: PathBuf::from("src").join(rel), + dst: PathBuf::from("dst").join(rel), + rel: PathBuf::from(rel), + kind: PlannedKind::New, + top_category: cat, + }; + let plan = InstallPlan { + files: vec![ + file("notify.sh", TopCategory::Hooks), + file("x/hooks/pre.sh", TopCategory::Agents), + file("hooks/role-hook.sh", TopCategory::Roles), + file("x/config.yaml", TopCategory::Agents), + file("reviewer.md", TopCategory::Roles), + file("hello.yaml", TopCategory::Macros), + ], + mcp_json: None, + }; + + let lines = hook_script_lines(&plan); + + assert_eq!( + lines, + vec![ + "bundle defines hook script: hooks/notify.sh", + "bundle defines hook script: agents/x/hooks/pre.sh", + "bundle defines hook script: roles/hooks/role-hook.sh", + ] + ); + } + #[test] fn classify_file_new_when_dst_missing() { let dir = fresh_temp_dir("classify-new-"); @@ -3420,6 +3608,91 @@ mod tests { let _ = fs::remove_dir_all(&src_root); } + fn hooks_fixture_bundle(label: &str) -> (PathBuf, PathBuf) { + let src_root = fresh_temp_dir(label); + let repo = src_root.join("hooky"); + write_src(&repo, "hooks/notify-me.sh", "#!/bin/sh\necho hi\n"); + write_src(&repo, "roles/hooks/role-hook.sh", "#!/bin/sh\n"); + write_src(&repo, "agents/x/hooks/pre.sh", "#!/bin/sh\n"); + write_src(&repo, "macros/hello.yaml", "name: hello\n"); + init_bundle_repo(&repo); + (src_root, repo) + } + + #[cfg(unix)] + fn assert_executable(path: &Path) { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(path).unwrap().permissions().mode(); + assert_ne!( + mode & 0o111, + 0, + "{} must carry the executable bit", + path.display() + ); + } + + #[test] + #[serial] + fn install_remote_installs_all_hooks_dirs_and_records_hooks_category() { + let _guard = TestVaultConfigGuard::new("prov-hooks-dirs"); + let (src_root, repo) = hooks_fixture_bundle("prov-hooks-dirs-src-"); + + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + + let top = paths::hooks_dir().join("notify-me.sh"); + let role = paths::roles_dir().join("hooks/role-hook.sh"); + let agent = paths::agents_data_dir().join("x/hooks/pre.sh"); + assert!(top.is_file(), "top-level hooks/ installs to hooks_dir"); + assert!(role.is_file(), "roles/hooks/ installs via the roles walk"); + assert!( + agent.is_file(), + "agents//hooks/ installs via the agents walk" + ); + #[cfg(unix)] + { + assert_executable(&top); + assert_executable(&role); + assert_executable(&agent); + } + + let store = BundleStore::load().unwrap(); + let record = store.get("hooky").unwrap(); + let category_of = |path: &str| { + record + .files + .iter() + .find(|f| f.path == path) + .unwrap_or_else(|| panic!("no provenance row for {path}")) + .category + .clone() + }; + assert_eq!(category_of("hooks/notify-me.sh"), "hooks"); + assert_eq!(category_of("roles/hooks/role-hook.sh"), "roles"); + assert_eq!(category_of("agents/x/hooks/pre.sh"), "agents"); + let _ = fs::remove_dir_all(&src_root); + } + + #[test] + #[serial] + fn install_remote_filter_hooks_restricts_to_top_level_hooks_dir() { + let _guard = TestVaultConfigGuard::new("prov-hooks-filter"); + let (src_root, repo) = hooks_fixture_bundle("prov-hooks-filter-src-"); + + install_remote(repo.to_str().unwrap(), Some(InstallFilter::Hooks), false).unwrap(); + + assert!(paths::hooks_dir().join("notify-me.sh").is_file()); + assert!(!paths::roles_dir().join("hooks/role-hook.sh").exists()); + assert!(!paths::agents_data_dir().join("x/hooks/pre.sh").exists()); + assert!(!paths::macros_dir().join("hello.yaml").exists()); + + let store = BundleStore::load().unwrap(); + let record = store.get("hooky").unwrap(); + assert_eq!(record.files.len(), 1); + assert_eq!(record.files[0].path, "hooks/notify-me.sh"); + assert_eq!(record.files[0].category, "hooks"); + let _ = fs::remove_dir_all(&src_root); + } + #[test] #[serial] fn install_remote_without_manifest_uses_repo_slug_and_short_sha_version() { @@ -3467,12 +3740,14 @@ mod tests { PlannedFile { src: src_new, dst: dst_new.clone(), + rel: PathBuf::from("new.yaml"), kind: PlannedKind::New, top_category: TopCategory::Macros, }, PlannedFile { src: src_conflict, dst: dst_conflict.clone(), + rel: PathBuf::from("conflict.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }, @@ -3525,6 +3800,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3656,12 +3932,50 @@ mod tests { } let _guard = TestVaultConfigGuard::new("prov-builtin"); - crate::config::install_assets(crate::config::AssetCategory::Macros).unwrap(); + config::install_assets(config::AssetCategory::Macros).unwrap(); assert!(fs::read_dir(paths::macros_dir()).unwrap().next().is_some()); assert!(!paths::installed_bundles_file().exists()); } + #[test] + #[serial] + fn builtin_hooks_category_installs_example_scripts() { + if *IS_STDOUT_TERMINAL { + eprintln!( + "Skipping builtin_hooks_category_installs_example_scripts: requires non-TTY stdout" + ); + return; + } + let _guard = TestVaultConfigGuard::new("prov-hooks"); + let hooks_key = get_env_name("hooks_dir"); + let previous_hooks = env::var_os(&hooks_key); + let hooks_dir = fresh_temp_dir("prov-hooks-dir-"); + unsafe { + env::set_var(&hooks_key, &hooks_dir); + } + + // Capture outcomes before restoring the env var so a failed assertion + // cannot leave the hooks-dir override pointing at a deleted temp dir. + let result = config::install_assets(AssetCategory::Hooks); + let installed = ( + hooks_dir.join("notify.sh").is_file(), + hooks_dir.join("log-events.sh").is_file(), + ); + + unsafe { + match previous_hooks { + Some(v) => env::set_var(&hooks_key, v), + None => env::remove_var(&hooks_key), + } + } + let _ = fs::remove_dir_all(&hooks_dir); + + result.unwrap(); + assert!(installed.0, "notify.sh must be installed"); + assert!(installed.1, "log-events.sh must be installed"); + } + #[test] #[serial] fn update_silently_refreshes_owned_unmodified_files() { @@ -3798,6 +4112,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3844,6 +4159,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3923,6 +4239,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("shared.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -4195,6 +4512,7 @@ mod tests { files: vec![PlannedFile { src: dir.join("bundle-src/agents/foo/graph.yaml"), dst: dir.join("agents/foo/graph.yaml"), + rel: PathBuf::from("foo/graph.yaml"), kind: PlannedKind::New, top_category: TopCategory::Agents, }], @@ -4237,6 +4555,7 @@ mod tests { files: vec![PlannedFile { src: dir.join("bundle-src/agents/foo/graph.yaml"), dst: dir.join("agents/foo/graph.yaml"), + rel: PathBuf::from("foo/graph.yaml"), kind: PlannedKind::New, top_category: TopCategory::Agents, }], diff --git a/src/config/mod.rs b/src/config/mod.rs index e59d670..7cfed24 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,6 +1,7 @@ mod agent; mod app_config; mod app_state; +pub(crate) mod builtin_manifest; mod bundles; mod input; mod install_remote; @@ -75,12 +76,14 @@ use crate::client::{ oauth, set_client_models_config, }; use crate::function::{FunctionDeclaration, Functions}; +use crate::hooks::HooksMap; use crate::rag::Rag; use crate::sandbox::SANDBOX_ENV_FLAG; use crate::utils::*; pub use macros::macro_execute; use crate::config::macros::Macro; +use crate::hooks; use crate::vault::{ GlobalVault, Vault, create_vault_password_file, interpolate_secrets, prompt_provider_choice, }; @@ -162,6 +165,7 @@ const AGENT_GRAPH_FILE_NAME: &str = "graph.yaml"; const ROLES_DIR_NAME: &str = "roles"; const SKILLS_DIR_NAME: &str = "skills"; const MACROS_DIR_NAME: &str = "macros"; +const HOOKS_DIR_NAME: &str = "hooks"; const ENV_FILE_NAME: &str = ".env"; const MESSAGES_FILE_NAME: &str = "messages.md"; const SESSIONS_DIR_NAME: &str = "sessions"; @@ -262,6 +266,9 @@ pub struct Config { pub enabled_mcp_servers: Option>, pub mcp_tools: Option>>, + #[serde(default)] + pub hooks: HooksMap, + pub auto_continue: bool, pub max_auto_continues: usize, pub inject_todo_instructions: bool, @@ -350,6 +357,8 @@ impl Default for Config { enabled_mcp_servers: None, mcp_tools: None, + hooks: Default::default(), + auto_continue: false, max_auto_continues: 10, inject_todo_instructions: true, @@ -412,6 +421,8 @@ pub fn install_builtins() -> Result<()> { Agent::install_builtin_agents(false)?; Macro::install_macros(false)?; Skill::install_builtin_skills(false)?; + hooks::install_builtin_hooks(false)?; + Role::install_builtin_role_hooks(false)?; Ok(()) } @@ -421,12 +432,20 @@ pub enum AssetCategory { Macros, Functions, Skills, + Hooks, #[value(name = "mcp-config", alias = "mcp_config")] McpConfig, } impl AssetCategory { - pub const NAMES: [&'static str; 5] = ["agents", "macros", "functions", "skills", "mcp-config"]; + pub const NAMES: [&'static str; 6] = [ + "agents", + "macros", + "functions", + "skills", + "hooks", + "mcp-config", + ]; pub fn parse(name: &str) -> Option { match name { @@ -434,6 +453,7 @@ impl AssetCategory { "macros" => Some(Self::Macros), "functions" => Some(Self::Functions), "skills" => Some(Self::Skills), + "hooks" => Some(Self::Hooks), "mcp-config" | "mcp_config" => Some(Self::McpConfig), _ => None, } @@ -453,17 +473,19 @@ pub enum InstallFilter { Skills, Macros, Functions, + Hooks, #[value(name = "mcp-config", alias = "mcp_config")] McpConfig, } impl InstallFilter { - pub const NAMES: [&'static str; 6] = [ + pub const NAMES: [&'static str; 7] = [ "agents", "roles", "skills", "macros", "functions", + "hooks", "mcp-config", ]; @@ -474,6 +496,7 @@ impl InstallFilter { "skills" => Some(Self::Skills), "macros" => Some(Self::Macros), "functions" => Some(Self::Functions), + "hooks" => Some(Self::Hooks), "mcp-config" | "mcp_config" => Some(Self::McpConfig), _ => None, } @@ -486,6 +509,7 @@ pub fn install_assets(category: AssetCategory) -> Result<()> { AssetCategory::Macros => ("macros", paths::macros_dir()), AssetCategory::Functions => ("functions", paths::functions_dir()), AssetCategory::Skills => ("skills", paths::skills_dir()), + AssetCategory::Hooks => ("hooks", paths::hooks_dir()), AssetCategory::McpConfig => ("MCP config", paths::mcp_config_file()), }; @@ -499,6 +523,7 @@ pub fn install_assets(category: AssetCategory) -> Result<()> { AssetCategory::Macros => Macro::install_macros(true)?, AssetCategory::Functions => Functions::install_builtin_global_tools(true)?, AssetCategory::Skills => Skill::install_builtin_skills(true)?, + AssetCategory::Hooks => hooks::install_builtin_hooks(true)?, AssetCategory::McpConfig => Functions::install_mcp_config()?, } @@ -1393,6 +1418,59 @@ clients: assert!(!CONFIG_TEMPLATE.contains("escalation_timeout")); } + #[test] + fn config_template_carries_hooks_block_without_global_hooks() { + // `global_hooks` is a per-agent whitelist; the global template must never grow it. + assert!(CONFIG_TEMPLATE.contains("\nhooks:")); + assert!(!CONFIG_TEMPLATE.contains("global_hooks")); + } + + #[test] + fn config_template_renders_with_empty_hooks() { + let clients = json!([{ "type": "openai", "api_key": "sk-test" }]); + + let rendered = render_config_template("openai:gpt-4o", None, &clients).unwrap(); + + assert!(rendered.contains("\nhooks:")); + assert!(!rendered.contains("global_hooks")); + + let cfg = Config::load_from_str(&rendered).unwrap(); + assert!(cfg.hooks.is_empty()); + } + + #[test] + fn config_parses_hooks_map_preserving_order() { + let yaml = "\ +hooks: + tool.started: + - name: notify + command: ./hooks/notify.sh + - name: audit + command: ./hooks/audit.sh + turn.completed: + - name: webhook + command: curl -s https://example.com/hook +"; + let cfg: Config = serde_yaml::from_str(yaml).unwrap(); + + assert_eq!( + cfg.hooks.keys().collect::>(), + vec!["tool.started", "turn.completed"] + ); + assert_eq!(cfg.hooks["tool.started"].len(), 2); + assert_eq!(cfg.hooks["tool.started"][0].name, "notify"); + assert_eq!(cfg.hooks["tool.started"][0].command, "./hooks/notify.sh"); + assert_eq!(cfg.hooks["turn.completed"][0].name, "webhook"); + } + + #[test] + fn config_without_hooks_parses_unchanged() { + let cfg: Config = serde_yaml::from_str("model: openai:gpt-4o").unwrap(); + + assert!(cfg.hooks.is_empty()); + assert_eq!(cfg.model_id, "openai:gpt-4o"); + } + #[test] fn config_enabled_macros_empty_string_is_some_empty() { let cfg: Config = serde_yaml::from_str("enabled_macros: \"\"").unwrap(); diff --git a/src/config/paths.rs b/src/config/paths.rs index a8252f6..8b9e125 100644 --- a/src/config/paths.rs +++ b/src/config/paths.rs @@ -2,7 +2,7 @@ use super::role::Role; use super::{ AGENT_GRAPH_FILE_NAME, AGENTS_DIR_NAME, BASH_PROMPT_UTILS_FILE_NAME, CONFIG_FILE_NAME, ENV_FILE_NAME, FUNCTIONS_BIN_DIR_NAME, FUNCTIONS_DIR_NAME, GLOBAL_TOOLS_DIR_NAME, - GLOBAL_TOOLS_UTILS_DIR_NAME, HIDDEN_MCP_FILE_NAME, INSTALLED_BUNDLES_FILE_NAME, + GLOBAL_TOOLS_UTILS_DIR_NAME, HIDDEN_MCP_FILE_NAME, HOOKS_DIR_NAME, INSTALLED_BUNDLES_FILE_NAME, MACROS_DIR_NAME, MCP_FILE_NAME, MEMORY_DIR_NAME, MEMORY_INDEX_FILE_NAME, ModelsOverride, RAGS_DIR_NAME, ROLES_DIR_NAME, SBX_KIT_DIR_NAME, SBX_KIT_HASH_FILE, SBX_MIXIN_FILE_NAME, SBX_MIXIN_KITS_DIR_NAME, SKILLS_DIR_NAME, WORKSPACE_COYOTE_DIR_NAME, @@ -184,6 +184,13 @@ pub fn role_file(name: &str) -> PathBuf { roles_dir().join(format!("{name}.md")) } +pub fn hooks_dir() -> PathBuf { + match env::var(get_env_name("hooks_dir")) { + Ok(value) => PathBuf::from(value), + Err(_) => local_dir(HOOKS_DIR_NAME), + } +} + pub fn skills_dir() -> PathBuf { match env::var(get_env_name("skills_dir")) { Ok(value) => PathBuf::from(value), @@ -947,6 +954,31 @@ mod tests { } } + #[test] + #[serial_test::serial] + fn hooks_dir_reflects_env_var_state() { + let env_name = get_env_name("hooks_dir"); + let prev = env::var_os(&env_name); + + unsafe { + env::remove_var(&env_name); + } + assert_eq!(hooks_dir(), config_dir().join("hooks")); + + let probe = PathBuf::from("/tmp/coyote-hooks-dir-probe"); + unsafe { + env::set_var(&env_name, &probe); + } + assert_eq!(hooks_dir(), probe); + + unsafe { + match prev { + Some(v) => env::set_var(&env_name, v), + None => env::remove_var(&env_name), + } + } + } + #[test] fn list_skills_skips_invalid_directory_names() { let unique = time::SystemTime::now() diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 921efa6..8874ed8 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -50,6 +50,7 @@ use super::memory::{ DEFAULT_MEMORY_CAP_WITH_TOOLS, DEFAULT_MEMORY_CAP_WITHOUT_TOOLS, MemoryStore, WorkspaceMemory, }; use crate::graph; +use crate::hooks::{self, HookEvent}; use anyhow::{Context, Error, Result, bail}; use colored::Colorize; use gman::providers::SupportedProvider; @@ -59,6 +60,7 @@ use inquire::{Confirm, MultiSelect, Text, list_option::ListOption, validator::Va use log::warn; use parking_lot::RwLock; use prompts::DEFAULT_SKILL_INSTRUCTIONS; +use rand::distr::{Alphanumeric, SampleString}; use std::collections::{BTreeSet, HashMap, HashSet}; use std::fs::{File, OpenOptions, read_dir, read_to_string, remove_dir_all, remove_file}; use std::io::Write; @@ -354,6 +356,13 @@ pub struct RequestContext { pub session_abort: Option, + /// Synthesized `COYOTE_AGENT_ID` for a top-level agent run (headless + /// `--agent` dispatch or an interactive agent context). Scheme: + /// `top--<8-char alphanumeric nonce>`; spawned sub-agents + /// carry supervisor-issued ids instead. `Some` exactly while the + /// top-level `agent.*` bracket is open. + pub top_level_agent_id: Option, + pub render_mode: RenderMode, } @@ -398,6 +407,7 @@ impl RequestContext { auto_continue_paused: None, pending_prefill: None, session_abort: None, + top_level_agent_id: None, render_mode: RenderMode::default(), } } @@ -469,6 +479,7 @@ impl RequestContext { auto_continue_paused: None, pending_prefill: None, session_abort: None, + top_level_agent_id: None, render_mode: RenderMode::default(), }; ctx.refresh_mcp_tool_filters(); @@ -529,6 +540,9 @@ impl RequestContext { auto_continue_paused: self.auto_continue_paused.clone(), pending_prefill: None, session_abort: self.session_abort.clone(), + // Branches never own the top-level `agent.*` bracket; only the + // root flow that opened it may fire its terminal event. + top_level_agent_id: None, render_mode: self.render_mode, } } @@ -585,6 +599,7 @@ impl RequestContext { auto_continue_paused: None, pending_prefill: None, session_abort: None, + top_level_agent_id: None, render_mode: parent.render_mode, } } @@ -1054,6 +1069,12 @@ impl RequestContext { } pub fn exit_session(&mut self) -> Result<()> { + // Fired while the session is still attached so the hook sees + // COYOTE_SESSION_ID and session-held role hooks still resolve; no + // session means no event. + if self.session.is_some() { + hooks::fire(HookEvent::SessionEnded, self, &[], None); + } if let Some(mut session) = self.session.take() { let sessions_dir = self.sessions_dir(); session.exit(&sessions_dir, self.working_mode.is_repl())?; @@ -4613,6 +4634,7 @@ impl RequestContext { ); } let mut session; + let mut created_new_session = false; match session_name { None | Some(TEMP_SESSION_NAME) => { let session_file = self.session_file(TEMP_SESSION_NAME); @@ -4622,11 +4644,13 @@ impl RequestContext { })?; } session = Some(Session::new_from_ctx(self, app, TEMP_SESSION_NAME)?); + created_new_session = true; } Some(name) => { let session_path = self.session_file(name); if !session_path.exists() { session = Some(Session::new_from_ctx(self, app, name)?); + created_new_session = true; } else { session = Some(Session::load_from_ctx(self, app, name, &session_path)?); } @@ -4709,6 +4733,9 @@ impl RequestContext { } self.refresh_mcp_tool_filters(); self.init_agent_session_variables(new_session)?; + if created_new_session { + hooks::fire(HookEvent::SessionStarted, self, &[], None); + } Ok(()) } @@ -4874,6 +4901,9 @@ impl RequestContext { tool_tracker, }; + // Interactive switch-away closes the top-level agent bracket while + // the agent is still attached, so agent-scoped hooks resolve. + self.top_level_agent_finished(None); if self.agent.take().is_some() { if let Some(supervisor) = self.supervisor.clone() { supervisor.read().cancel_recursive(); @@ -4901,6 +4931,50 @@ impl RequestContext { Ok(()) } + pub fn top_level_agent_started(&mut self) { + if self.top_level_agent_id.is_some() { + return; + } + + let Some(agent) = self.agent.as_ref() else { + return; + }; + let nonce = Alphanumeric.sample_string(&mut rand::rng(), 8); + let id = format!("top-{}-{nonce}", agent.name()); + self.top_level_agent_id = Some(id.clone()); + + hooks::fire( + HookEvent::AgentStarted, + self, + &[("COYOTE_AGENT_ID", id)], + None, + ); + } + + pub fn top_level_agent_finished(&mut self, error: Option<&Error>) { + let Some(id) = self.top_level_agent_id.take() else { + return; + }; + + match error { + None => hooks::fire( + HookEvent::AgentCompleted, + self, + &[("COYOTE_AGENT_ID", id)], + None, + ), + Some(err) => hooks::fire( + HookEvent::AgentFailed, + self, + &[ + ("COYOTE_AGENT_ID", id), + ("COYOTE_AGENT_ERROR", format!("{err:#}")), + ], + None, + ), + } + } + pub async fn edit_role(&mut self, app: &AppConfig, abort_signal: AbortSignal) -> Result<()> { let role_name; if let Some(session) = self.session.as_ref() { @@ -5421,6 +5495,7 @@ mod tests { use crate::config::tool_scope::test_fixtures::{FixtureServer, fixture_runtime}; use crate::function::jobs::RingBuf; use crate::function::{ToolCall, skill}; + use crate::hooks::{HookDef, HooksMap, test_sink}; use crate::mcp::{McpServer, McpServerFeatures, McpServersConfig, McpTransportType}; use crate::supervisor::{ AgentExitStatus, AgentHandle, AgentResult, JobHandle, JobResult, JobState, JobStatus, @@ -5428,6 +5503,7 @@ mod tests { use crate::utils; use crate::utils::get_env_name; use crate::vault::Vault; + use anyhow::anyhow; use rmcp::model::PromptArgument; use serde_json::json; use serial_test::serial; @@ -8230,6 +8306,349 @@ mod tests { assert!(ctx.session.is_none()); } + fn hooks_map_of(events: &[&str], marker: &str) -> HooksMap { + let mut map = HooksMap::default(); + for event in events { + map.insert( + event.to_string(), + vec![HookDef { + name: format!("{marker}-{event}"), + command: "true".to_string(), + }], + ); + } + map + } + + fn marker_captures(marker: &str) -> Vec { + test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name.starts_with(marker)) + .collect() + } + + /// Writes an agent whose config carries agent-scoped hooks for every + /// `agent.*` event, named `-`. + fn seed_agent_with_hooks(agent_name: &str, marker: &str) { + let agent_dir = paths::agent_data_dir(agent_name); + create_dir_all(&agent_dir).unwrap(); + let mut config = format!("name: {agent_name}\ninstructions: hi\nhooks:\n"); + for event in ["agent.started", "agent.completed", "agent.failed"] { + config.push_str(&format!( + " {event}:\n - name: {marker}-{event}\n command: 'true'\n" + )); + } + write(agent_dir.join("config.yaml"), config).unwrap(); + } + + fn unique_name(prefix: &str) -> String { + format!( + "{prefix}_{}", + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos() + ) + } + + #[test] + #[serial] + fn use_session_fires_session_started_for_new_sessions_only() { + let _guard = TestConfigDirGuard::new(); + let sessions_dir = paths::local_dir("sessions"); + create_dir_all(&sessions_dir).unwrap(); + let _sink = test_sink::install(); + let marker = "sess-started-q8n"; + + let mut ctx = create_test_ctx(); + ctx.update_app_config(|app| app.hooks = hooks_map_of(&["session.started"], marker)); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_session(&app, Some("hook-fresh"), abort.clone())).unwrap(); + + let captures = marker_captures(marker); + assert_eq!(captures.len(), 1, "a new session fires exactly one event"); + assert_eq!( + captures[0] + .envs + .get("COYOTE_SESSION_ID") + .map(String::as_str), + Some("hook-fresh") + ); + + // Resuming a persisted session fires nothing. + write_paused_todo_session(&ctx, "hook-resume"); + let mut resumed_ctx = create_test_ctx(); + resumed_ctx.update_app_config(|app| app.hooks = hooks_map_of(&["session.started"], marker)); + let resumed_app = resumed_ctx.app.config.clone(); + run_async(resumed_ctx.use_session(&resumed_app, Some("hook-resume"), abort)).unwrap(); + assert_eq!( + marker_captures(marker).len(), + 1, + "resume must not fire session.started" + ); + } + + #[test] + #[serial] + fn exit_session_fires_session_ended_only_with_a_session() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "sess-ended-w5c"; + + let mut ctx = create_test_ctx(); + ctx.update_app_config(|app| app.hooks = hooks_map_of(&["session.ended"], marker)); + + // Sessionless (headless) exit path: nothing fires. + ctx.exit_session().unwrap(); + assert!(marker_captures(marker).is_empty()); + + ctx.session = Some(Session::default()); + ctx.exit_session().unwrap(); + ctx.exit_session().unwrap(); + assert_eq!( + marker_captures(marker).len(), + 1, + "session.ended fires exactly once, only while a session is attached" + ); + } + + #[test] + #[serial] + fn chat_completion_seams_fire_no_turn_events_in_child_contexts() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "child-turn-b4h"; + + let mut ctx = create_test_ctx(); + ctx.update_app_config(|app| { + app.hooks = hooks_map_of( + &[ + "turn.started", + "turn.completed", + "turn.interrupted", + "turn.failed", + ], + marker, + ) + }); + let app = ctx.app.config.clone(); + + let input = Input::from_str(&ctx, "hello", None).unwrap(); + ctx.before_chat_completion(&input).unwrap(); + ctx.after_chat_completion(&app, &input, "output", &[]) + .unwrap(); + + assert!( + marker_captures(marker).is_empty(), + "per-round chat-completion seams must never fire turn events" + ); + } + + #[test] + #[serial] + fn top_level_agent_bracket_fires_started_and_completed_once() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "agent-top-ok-m2s"; + let agent_name = unique_name("hook_agent"); + seed_agent_with_hooks(&agent_name, marker); + + let mut ctx = create_test_ctx(); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_agent(&app, &agent_name, None, abort)).unwrap(); + + ctx.top_level_agent_started(); + // Per agent RUN, never per message: re-entering while open is a no-op. + ctx.top_level_agent_started(); + ctx.top_level_agent_finished(None); + ctx.top_level_agent_finished(None); + + let captures = marker_captures(marker); + let started: Vec<_> = captures + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-agent.started")) + .collect(); + let completed: Vec<_> = captures + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-agent.completed")) + .collect(); + assert_eq!(started.len(), 1); + assert_eq!(completed.len(), 1); + assert!( + captures + .iter() + .all(|capture| capture.hook_name != format!("{marker}-agent.failed")) + ); + let id = started[0] + .envs + .get("COYOTE_AGENT_ID") + .expect("agent id env"); + assert!( + id.starts_with(&format!("top-{agent_name}-")), + "synthesized id must follow the top-- scheme: {id}" + ); + assert_eq!(completed[0].envs.get("COYOTE_AGENT_ID"), Some(id)); + assert_eq!( + started[0].envs.get("COYOTE_AGENT_NAME").map(String::as_str), + Some(agent_name.as_str()) + ); + } + + #[test] + #[serial] + fn top_level_agent_failure_fires_agent_failed_with_error() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "agent-top-err-f7d"; + let agent_name = unique_name("hook_agent"); + seed_agent_with_hooks(&agent_name, marker); + + let mut ctx = create_test_ctx(); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_agent(&app, &agent_name, None, abort)).unwrap(); + + ctx.top_level_agent_started(); + let err = anyhow!("dispatch exploded"); + ctx.top_level_agent_finished(Some(&err)); + + let captures = marker_captures(marker); + let failed: Vec<_> = captures + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-agent.failed")) + .collect(); + assert_eq!(failed.len(), 1); + assert!( + captures + .iter() + .all(|capture| capture.hook_name != format!("{marker}-agent.completed")) + ); + assert_eq!( + failed[0].envs.get("COYOTE_AGENT_ERROR").map(String::as_str), + Some("dispatch exploded") + ); + assert!(failed[0].envs.contains_key("COYOTE_AGENT_ID")); + } + + #[test] + #[serial] + fn top_level_agent_helpers_noop_without_agent() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "agent-top-role-only-c9k"; + + let mut ctx = create_test_ctx(); + ctx.update_app_config(|app| { + app.hooks = hooks_map_of( + &["agent.started", "agent.completed", "agent.failed"], + marker, + ) + }); + ctx.role = Some(Role::new("plain", "prompt")); + + ctx.top_level_agent_started(); + ctx.top_level_agent_finished(None); + + assert!( + marker_captures(marker).is_empty(), + "a role-only context must fire no agent events" + ); + assert!(ctx.top_level_agent_id.is_none()); + } + + #[test] + #[serial] + fn exit_agent_closes_top_level_agent_bracket() { + let _guard = TestConfigDirGuard::new(); + let _sink = test_sink::install(); + let marker = "agent-top-exit-v3p"; + let agent_name = unique_name("hook_agent"); + seed_agent_with_hooks(&agent_name, marker); + + let mut ctx = create_test_ctx(); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_agent(&app, &agent_name, None, abort)).unwrap(); + ctx.top_level_agent_started(); + + ctx.exit_agent(&app).unwrap(); + // A later exit seam (REPL teardown) must not double-fire. + ctx.top_level_agent_finished(None); + + let captures = marker_captures(marker); + assert_eq!( + captures + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-agent.completed")) + .count(), + 1, + "switch-away fires the terminal event exactly once" + ); + } + + #[test] + #[serial] + fn resolved_hooks_resolves_role_hooks_held_by_fresh_session() { + let _guard = TestConfigDirGuard::new(); + let sessions_dir = paths::local_dir("sessions"); + create_dir_all(&sessions_dir).unwrap(); + + let mut ctx = create_test_ctx(); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_session(&app, Some("role-holder"), abort)).unwrap(); + + let role_content = "---\nhooks:\n turn.completed:\n - name: session-role-probe\n command: role-cmd\n---\nPrompt"; + ctx.use_role_obj(Role::new("hooked-role", role_content)) + .unwrap(); + assert!( + ctx.role.is_none(), + "use_role_obj must move the role into the active session" + ); + + let resolved = ctx.resolved_hooks(HookEvent::TurnCompleted); + assert_eq!(resolved.len(), 1); + assert_eq!(resolved[0].name, "session-role-probe"); + assert_eq!(resolved[0].command, "role-cmd"); + assert_eq!(resolved[0].cwd, paths::roles_dir()); + } + + #[test] + #[serial] + fn resolved_hooks_resolves_role_hooks_after_session_resume() { + let _guard = TestConfigDirGuard::new(); + let sessions_dir = paths::local_dir("sessions"); + create_dir_all(&sessions_dir).unwrap(); + let roles_dir = paths::roles_dir(); + create_dir_all(&roles_dir).unwrap(); + write( + roles_dir.join("resume-hooked.md"), + "---\nhooks:\n session.ended:\n - name: resume-role-probe\n command: role-cmd\n---\nYou are hooked.", + ) + .unwrap(); + + let seed_ctx = create_test_ctx(); + let session_path = seed_ctx.session_file("resume-role"); + ensure_parent_exists(&session_path).unwrap(); + write( + &session_path, + "model: test-seeded:test-chat\nrole_name: resume-hooked\nmessages: []\n", + ) + .unwrap(); + + let mut ctx = create_test_ctx(); + let app = ctx.app.config.clone(); + let abort = utils::create_abort_signal(); + run_async(ctx.use_session(&app, Some("resume-role"), abort)).unwrap(); + assert!(ctx.role.is_none()); + + let resolved = ctx.resolved_hooks(HookEvent::SessionEnded); + assert_eq!(resolved.len(), 1, "resumed session must restore role hooks"); + assert_eq!(resolved[0].name, "resume-role-probe"); + } + fn write_paused_todo_session(ctx: &RequestContext, name: &str) { let session_path = ctx.session_file(name); ensure_parent_exists(&session_path).unwrap(); @@ -8859,6 +9278,53 @@ mod tests { assert!(agents_dir.join("architect").join("config.yaml").exists()); } + #[test] + #[serial] + fn install_builtin_agents_reconciles_hooks_subdir_via_manifest() { + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; + + let _guard = TestConfigDirGuard::new(); + + Agent::install_builtin_agents(false).unwrap(); + + let agents_dir = paths::agents_data_dir(); + // Simulate an upgrade from an older install whose coder shipped a + // hook script that the current embed no longer contains: the manifest + // records it, so reconciliation removes it — while a user-created + // script in the same directory (absent from the manifest) survives. + let hooks_dir = agents_dir.join("coder").join("hooks"); + create_dir_all(&hooks_dir).unwrap(); + write(hooks_dir.join(BUILTIN_MANIFEST_FILE), "old-hook.sh\n").unwrap(); + write(hooks_dir.join("old-hook.sh"), "stale").unwrap(); + write(hooks_dir.join("user-hook.sh"), "user-owned").unwrap(); + // A custom agent outside the bundle is never reconciled, even with a + // manifest present. + let custom_hooks = agents_dir.join("myagent").join("hooks"); + create_dir_all(&custom_hooks).unwrap(); + write(custom_hooks.join(BUILTIN_MANIFEST_FILE), "custom.sh\n").unwrap(); + write(custom_hooks.join("custom.sh"), "keep").unwrap(); + + Agent::install_builtin_agents(false).unwrap(); + + assert!( + !hooks_dir.join("old-hook.sh").exists(), + "manifest-listed hook absent from the embed must be removed" + ); + assert!( + hooks_dir.join("user-hook.sh").exists(), + "user-created hook files must survive reconciliation" + ); + assert!( + !hooks_dir.join(BUILTIN_MANIFEST_FILE).exists(), + "an empty shipped set removes the manifest itself" + ); + assert!( + custom_hooks.join("custom.sh").exists(), + "custom agents outside the bundle must survive reconciliation" + ); + assert!(custom_hooks.join(BUILTIN_MANIFEST_FILE).exists()); + } + #[test] #[serial] fn install_builtin_skills_force_overwrites_only_with_force() { diff --git a/src/config/role.rs b/src/config/role.rs index 5485d07..eaa8123 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -1,12 +1,17 @@ use super::*; use crate::client::{Message, MessageContent, MessageRole, Model}; +use crate::config::builtin_manifest; +use crate::function::write_file_atomic; +use crate::hooks::HooksMap; use anyhow::Result; use fancy_regex::Regex; use rust_embed::Embed; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::collections::BTreeSet; +use std::path::Path; use std::sync::LazyLock; pub const SHELL_ROLE: &str = "shell"; @@ -99,6 +104,8 @@ pub struct Role { skill_instructions: Option, #[serde(skip_serializing_if = "Option::is_none")] memory: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + hooks: Option, #[serde(skip)] model: Model, @@ -161,6 +168,7 @@ impl Role { role.skill_instructions = value.as_str().map(|v| v.to_string()) } "memory" => role.memory = value.as_bool(), + "hooks" => role.hooks = parse_hooks_map(value), _ => (), } } @@ -181,6 +189,26 @@ impl Role { .collect() } + /// Installs builtin role hook scripts into `/hooks/` and + /// reconciles ones a release stopped shipping. This is the only builtin + /// install machinery roles have: general role installation deliberately + /// does not exist (builtin roles load straight from the embed). + pub fn install_builtin_role_hooks(force: bool) -> Result<()> { + let shipped: Vec<(String, String)> = RolesAsset::iter() + .filter_map(|file| { + let name = file.as_ref().strip_prefix("hooks/")?; + if name.is_empty() || name.contains('/') { + return None; + } + let embedded = RolesAsset::get(file.as_ref())?; + let content = unsafe { std::str::from_utf8_unchecked(&embedded.data) }; + Some((name.to_string(), content.to_string())) + }) + .collect(); + + install_and_reconcile_role_hooks(&paths::roles_dir().join("hooks"), &shipped, force) + } + pub fn has_args(&self) -> bool { self.name.contains('#') } @@ -248,6 +276,10 @@ impl Role { if let Some(memory) = self.memory { metadata.push(format!("memory: {memory}")); } + if let Some(hooks) = &self.hooks { + let inline = serde_json::to_string(hooks).unwrap_or_else(|_| "{}".to_string()); + metadata.push(format!("hooks: {inline}")); + } if metadata.is_empty() { format!("{}\n", self.prompt) } else if self.prompt.is_empty() { @@ -380,6 +412,10 @@ impl Role { self.memory } + pub fn hooks(&self) -> Option<&HooksMap> { + self.hooks.as_ref() + } + pub fn skills_enabled(&self) -> Option { self.skills_enabled } @@ -544,6 +580,16 @@ fn parse_mcp_tools_map(value: &Value) -> Option>> { Some(mcp_tools) } +fn parse_hooks_map(value: &Value) -> Option { + match serde_json::from_value(value.clone()) { + Ok(map) => Some(map), + Err(err) => { + debug!("Ignoring malformed hooks configuration: {err}"); + None + } + } +} + fn parse_structure_prompt(prompt: &str) -> (&str, Vec<(&str, &str)>) { let mut text = prompt; let mut search_input = true; @@ -592,6 +638,46 @@ fn parse_structure_prompt(prompt: &str) -> (&str, Vec<(&str, &str)>) { (prompt, vec![]) } +/// Only direct children of the hooks/ directory are supported: they are the +/// only shape the builtin manifest tracks, so nested asset paths are skipped +/// by the caller rather than installed unreconciled. +fn install_and_reconcile_role_hooks( + dir: &Path, + shipped: &[(String, String)], + force: bool, +) -> Result<()> { + if !shipped.is_empty() { + info!("Installing built-in role hooks in {}", dir.display()); + } + let mut written = BTreeSet::new(); + for (name, content) in shipped { + let path = dir.join(name); + if path.exists() && !force { + debug!( + "Role hook file already exists, skipping: {}", + path.display() + ); + continue; + } + ensure_parent_exists(&path)?; + info!("Creating role hook file: {}", path.display()); + write_file_atomic(&path, content, Some(0o755))?; + written.insert(name.clone()); + } + + let names: BTreeSet = shipped.iter().map(|(name, _)| name.clone()).collect(); + // Reconciliation is best-effort housekeeping: a failure here must not + // abort startup (install_builtins), matching the agent-side policy. + if let Err(err) = builtin_manifest::reconcile_builtin_dir(dir, &names, &written) { + warn!( + "Failed to reconcile builtin role hooks in {}: {err}", + dir.display() + ); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -641,6 +727,42 @@ mod tests { ); } + #[test] + fn role_hooks_round_trip_through_export() { + let content = "---\nhooks:\n turn.completed:\n - name: notify\n command: ./hooks/notify.sh\n---\nPrompt"; + + let role = Role::new("test", content); + + let hooks = role.hooks().expect("hooks should be parsed"); + assert_eq!(hooks["turn.completed"][0].name, "notify"); + assert_eq!(hooks["turn.completed"][0].command, "./hooks/notify.sh"); + + let exported = role.export(); + assert!(exported.contains("hooks:")); + + let reparsed = Role::new("test", &exported); + assert_eq!(reparsed.hooks(), role.hooks()); + assert_eq!(reparsed.prompt(), "Prompt"); + } + + #[test] + fn role_without_hooks_has_none_and_export_omits_key() { + let role = Role::new("test", "---\nmemory: true\n---\nPrompt"); + + assert!(role.hooks().is_none()); + assert!(!role.export().contains("hooks:")); + } + + #[test] + fn role_malformed_hooks_degrades_to_none() { + let content = "---\nhooks: [not, a, map]\n---\nPrompt"; + + let role = Role::new("test", content); + + assert!(role.hooks().is_none()); + assert_eq!(role.prompt(), "Prompt"); + } + #[test] fn role_new_no_metadata_has_none_fields() { let role = Role::new("test", "Just a prompt"); @@ -909,4 +1031,122 @@ Input 1 assert_eq!(parse_structure_prompt(prompt), (prompt, vec![])); } + + fn fixture(entries: &[(&str, &str)]) -> Vec<(String, String)> { + entries + .iter() + .map(|(name, content)| (name.to_string(), content.to_string())) + .collect() + } + + fn hooks_fixture_dir(label: &str) -> PathBuf { + let dir = temp_file(label, ""); + create_dir_all(&dir).unwrap(); + dir.join("hooks") + } + + #[test] + fn role_hooks_install_writes_scripts_and_manifest() { + let dir = hooks_fixture_dir("role-hooks-install-"); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("a.sh", "#!/bin/sh\n")]), false) + .unwrap(); + + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "#!/bin/sh\n"); + assert_eq!( + read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "a.sh\n" + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(dir.join("a.sh")).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o755, "role hook scripts install executable"); + let manifest_mode = fs::metadata(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)) + .unwrap() + .permissions() + .mode(); + assert_eq!( + manifest_mode & 0o111, + 0, + "the manifest must never be executable" + ); + } + let _ = fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_install_overwrites_only_with_force() { + let dir = hooks_fixture_dir("role-hooks-force-"); + let shipped = fixture(&[("a.sh", "new content\n")]); + create_dir_all(&dir).unwrap(); + fs::write(dir.join("a.sh"), "SENTINEL").unwrap(); + + install_and_reconcile_role_hooks(&dir, &shipped, false).unwrap(); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "SENTINEL"); + + install_and_reconcile_role_hooks(&dir, &shipped, true).unwrap(); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "new content\n"); + let _ = fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_reconcile_removes_dropped_hook_and_keeps_user_files() { + let dir = hooks_fixture_dir("role-hooks-reconcile-"); + + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("keep.sh", "#!/bin/sh\n"), ("drop.sh", "#!/bin/sh\n")]), + false, + ) + .unwrap(); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("keep.sh", "#!/bin/sh\n")]), false) + .unwrap(); + + assert!( + !dir.join("drop.sh").exists(), + "dropped builtin hook removed" + ); + assert!(dir.join("keep.sh").exists()); + assert!(dir.join("user.sh").exists(), "user file survives reconcile"); + assert_eq!( + read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "keep.sh\n" + ); + let _ = fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_never_claim_a_preexisting_user_file() { + let dir = hooks_fixture_dir("role-hooks-unclaimed-"); + create_dir_all(&dir).unwrap(); + fs::write(dir.join("a.sh"), "user-owned").unwrap(); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("a.sh", "#!/bin/sh\n")]), false) + .unwrap(); + assert!( + !dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE).exists(), + "a skipped pre-existing file must not be claimed by the manifest" + ); + + install_and_reconcile_role_hooks(&dir, &[], false).unwrap(); + assert_eq!( + read_to_string(dir.join("a.sh")).unwrap(), + "user-owned", + "the user file survives the hook being dropped from the embed" + ); + let _ = fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_empty_shipped_set_creates_nothing() { + let dir = hooks_fixture_dir("role-hooks-empty-"); + + install_and_reconcile_role_hooks(&dir, &[], false).unwrap(); + + assert!(!dir.exists(), "an empty shipped set must not create hooks/"); + let _ = fs::remove_dir_all(dir.parent().unwrap()); + } } diff --git a/src/config/session.rs b/src/config/session.rs index 6f541b9..abef564 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -3,6 +3,7 @@ use super::todo::TodoList; use super::*; use crate::client::{Message, MessageContent, MessageRole, TokenUsage}; +use crate::hooks::HooksMap; use crate::render::MarkdownRender; use anyhow::{Context, Result, bail}; @@ -113,6 +114,8 @@ pub struct Session { #[serde(skip)] role_prompt: String, #[serde(skip)] + role_hooks: Option, + #[serde(skip)] name: String, #[serde(skip)] path: Option, @@ -193,6 +196,7 @@ impl Session { && let Ok(role) = ctx.retrieve_role(app, role_name) { session.role_prompt = role.prompt().to_string(); + session.role_hooks = role.hooks().cloned(); } session.update_tokens(); @@ -254,6 +258,13 @@ impl Session { self.role_name.as_deref() } + /// Hooks of the role this session holds. `set_role` snapshots them + /// because only the frontmatter-stripped role prompt is persisted; + /// `load_from_ctx` restores them by re-reading the role file on resume. + pub fn role_hooks(&self) -> Option<&HooksMap> { + self.role_hooks.as_ref() + } + pub fn dirty(&self) -> bool { self.dirty } @@ -561,6 +572,7 @@ impl Session { self.model = role.model().clone(); self.role_name = convert_option_string(role.name()); self.role_prompt = role.prompt().to_string(); + self.role_hooks = role.hooks().cloned(); self.dirty = true; self.update_tokens(); } @@ -568,11 +580,13 @@ impl Session { pub fn clear_role(&mut self) { self.role_name = None; self.role_prompt.clear(); + self.role_hooks = None; } pub fn sync_agent(&mut self, agent: &Agent) { self.role_name = None; self.role_prompt = agent.interpolated_instructions(); + self.role_hooks = None; self.agent_variables = agent.variables().clone(); self.agent_instructions = self.role_prompt.clone(); if let Some(threshold) = agent.compression_threshold() { @@ -1111,6 +1125,25 @@ mod tests { assert!(!session.dirty()); } + #[test] + fn set_role_captures_and_clear_role_drops_role_hooks() { + let content = "---\nhooks:\n turn.completed:\n - name: role-probe\n command: role-cmd\n---\nPrompt"; + let mut session = Session::default(); + + session.set_role(Role::new("hooked", content)); + let hooks = session.role_hooks().expect("set_role must snapshot hooks"); + assert_eq!(hooks["turn.completed"][0].name, "role-probe"); + + session.clear_role(); + assert!(session.role_hooks().is_none()); + + session.set_role(Role::new("plain", "No frontmatter")); + assert!( + session.role_hooks().is_none(), + "a role without hooks must not leave a stale snapshot" + ); + } + fn push_interrupted_turn(session: &mut Session) { session.messages.push(Message::new( MessageRole::User, @@ -1233,6 +1266,37 @@ mod tests { assert!(!yaml.contains("token_usage")); } + #[test] + fn session_role_hooks_is_not_serialized() { + let content = "---\nhooks:\n turn.completed:\n - name: role-probe\n command: role-cmd\n---\nPrompt"; + let mut session = Session::default(); + session.set_role(Role::new("hooked", content)); + assert!(session.role_hooks().is_some()); + + let yaml = serde_yaml::to_string(&session).unwrap(); + assert!( + !yaml.contains("role_hooks"), + "the role-hooks snapshot must never reach session YAML" + ); + assert!( + !yaml.contains("role-cmd"), + "no hook command text may leak into session YAML under any key" + ); + } + + #[test] + fn session_deserialization_ignores_injected_role_hooks() { + // A session file is attacker-writable data: a `role_hooks:` key + // crafted into it must never deserialize into executable hook + // config. Hooks are only ever re-read from the role file itself. + let yaml = "model: ''\nmessages: []\nrole_hooks:\n session.started:\n - name: injected\n command: touch /tmp/pwned\n"; + let session: Session = serde_yaml::from_str(yaml).unwrap(); + assert!( + session.role_hooks().is_none(), + "injected role_hooks in session YAML must be ignored" + ); + } + #[test] fn session_cost_accumulates_and_survives_yaml_round_trip() { let mut session = Session::default(); diff --git a/src/function/agents.rs b/src/function/agents.rs index 3baafaa..4e05b38 100644 --- a/src/function/agents.rs +++ b/src/function/agents.rs @@ -4,8 +4,9 @@ use crate::client::{Model, ModelType, call_chat_completions}; use crate::config::{ Agent, AgentVariable, AgentVariables, AppState, Input, RequestContext, Role, RoleLike, default_max_agent_depth, effective_max_concurrent_jobs, jobs_enabled, - list_agents_with_descriptions, load_agent_variables, + list_agents_with_descriptions, load_agent_variables, paths, }; +use crate::hooks::{self, HookEvent, ResolvedHook}; use crate::supervisor::mailbox::{Envelope, EnvelopePayload, Inbox, PeerRegistry, graph_agent_id}; use crate::supervisor::notification::agent_notification; use crate::supervisor::{AgentExitStatus, AgentHandle, AgentResult, Supervisor, TaskKind}; @@ -14,7 +15,7 @@ use crate::utils::{AbortSignal, create_abort_signal, wait_abort_signal, wait_use use crate::graph; use crate::repl::DEFAULT_CONTINUATION_PROMPT; use anyhow::{Context, Result, anyhow, bail}; -use chrono::Utc; +use chrono::{SecondsFormat, Utc}; use indexmap::IndexMap; use log::{debug, warn}; use parking_lot::RwLock; @@ -996,6 +997,86 @@ fn sync_agent_functions_to_ctx(ctx: &mut RequestContext) -> Result<()> { Ok(()) } +/// Mirrors the base-env contract `hooks::fire` assembles from a live context, +/// for detached call sites that dispatch pre-resolved hook snapshots after +/// the context they were resolved from is gone. The names are captured by +/// the caller when it takes the snapshot; the timestamp is taken here, at +/// fire time, so it reflects when the event actually happened. +pub(crate) fn hook_base_envs( + event: HookEvent, + session_name: Option<&str>, + agent_name: Option<&str>, +) -> Vec<(String, String)> { + let mut envs = vec![ + ("COYOTE_EVENT".to_string(), event.as_str().to_string()), + ( + "COYOTE_CONFIG_DIR".to_string(), + paths::config_dir().display().to_string(), + ), + ( + "COYOTE_EVENT_TIMESTAMP".to_string(), + Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), + ), + ]; + if let Some(name) = session_name { + envs.push(("COYOTE_SESSION_ID".to_string(), name.to_string())); + } + if let Some(name) = agent_name { + envs.push(("COYOTE_AGENT_NAME".to_string(), name.to_string())); + } + envs +} + +/// Pre-resolved `agent.completed`/`agent.failed` snapshots for a spawned +/// agent's result arms, captured from the child context before the spawn +/// task takes ownership of it. Resolving against the child keeps global +/// hooks gated by the child's own whitelist, the same gate every other +/// event fired from the child's context goes through. +struct SpawnResultHooks { + completed: Vec, + failed: Vec, + session_name: Option, + agent_id: String, + agent_name: String, +} + +impl SpawnResultHooks { + fn resolve(child_ctx: &RequestContext, agent_id: &str, agent_name: &str) -> Self { + Self { + completed: child_ctx.resolved_hooks(HookEvent::AgentCompleted), + failed: child_ctx.resolved_hooks(HookEvent::AgentFailed), + session_name: child_ctx + .session + .as_ref() + .map(|session| session.name().to_string()), + agent_id: agent_id.to_string(), + agent_name: agent_name.to_string(), + } + } + + /// Fires `agent.completed`, or `agent.failed` when `error` is given. + fn fire(self, error: Option<&str>) { + let mut extras = vec![ + ("COYOTE_AGENT_ID", self.agent_id), + ("COYOTE_AGENT_NAME", self.agent_name.clone()), + ]; + let (event, resolved) = match error { + None => (HookEvent::AgentCompleted, self.completed), + Some(error) => { + extras.push(("COYOTE_AGENT_ERROR", error.to_string())); + (HookEvent::AgentFailed, self.failed) + } + }; + hooks::fire_resolved( + event, + resolved, + hook_base_envs(event, self.session_name.as_deref(), Some(&self.agent_name)), + &extras, + None, + ); + } +} + async fn handle_spawn(ctx: &mut RequestContext, args: &Value) -> Result { let agent_name = args .get("agent") @@ -1149,6 +1230,19 @@ async fn handle_spawn(ctx: &mut RequestContext, args: &Value) -> Result { debug!("Spawning child agent '{agent_name}' as '{agent_id}'"); + hooks::fire( + HookEvent::AgentStarted, + &child_ctx, + &[ + ("COYOTE_AGENT_ID", agent_id.clone()), + ("COYOTE_AGENT_NAME", agent_name.clone()), + ], + None, + ); + // Resolved before the spawn: the result arms outlive the child context + // the snapshots come from. + let result_hooks = SpawnResultHooks::resolve(&child_ctx, &agent_id, &agent_name); + let spawn_agent_id = agent_id.clone(); let spawn_agent_name = agent_name.clone(); let spawn_abort = child_abort.clone(); @@ -1173,6 +1267,10 @@ async fn handle_spawn(ctx: &mut RequestContext, args: &Value) -> Result { }, }; let success = agent_result.exit_status == AgentExitStatus::Completed; + match &agent_result.exit_status { + AgentExitStatus::Completed => result_hooks.fire(None), + AgentExitStatus::Failed(e) => result_hooks.fire(Some(e)), + } spawn_notifications.push(agent_notification( &agent_result.id, &agent_result.agent_name, @@ -2154,6 +2252,155 @@ mod tests { .unwrap(); } + /// A graph agent whose graph.yaml wires `agent.*` hooks at its top + /// level. With `failing`, the start node references a script that does + /// not exist, so the child run fails at runtime — after the spawn seam + /// has already fired `agent.started`. + fn write_graph_agent_with_hooks(agent_name: &str, marker: &str, failing: bool) { + let agent_dir = paths::agent_data_dir(agent_name); + create_dir_all(&agent_dir).unwrap(); + let (start, nodes) = if failing { + ( + "work", + " work:\n type: script\n script: missing.sh\n next: done\n done:\n type: end\n output: done\n", + ) + } else { + ("done", " done:\n type: end\n output: done\n") + }; + write( + agent_dir.join("graph.yaml"), + format!( + "name: {agent_name}\n\ + start: {start}\n\ + settings:\n\ + \x20 validate_before_run: false\n\ + hooks:\n\ + \x20 agent.started:\n\ + \x20 - name: {marker}_started\n\ + \x20 command: \"true\"\n\ + \x20 agent.completed:\n\ + \x20 - name: {marker}_completed\n\ + \x20 command: \"true\"\n\ + \x20 agent.failed:\n\ + \x20 - name: {marker}_failed\n\ + \x20 command: \"true\"\n\ + nodes:\n\ + {nodes}" + ), + ) + .unwrap(); + } + + fn hook_captures_named(name: &str) -> Vec { + hooks::test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name == name) + .collect() + } + + /// Spawns an agent through the supervisor seam and awaits its spawn + /// task, returning the spawn id. The result arms fire their hooks + /// before that task returns, so the sink is complete afterwards. + fn spawn_and_collect(agent_name: &str) -> String { + run_async(async { + let mut ctx = ctx_with_supervisor(4, 3); + let spawned = handle_spawn(&mut ctx, &json!({"agent": agent_name, "prompt": "hi"})) + .await + .unwrap(); + assert_eq!(spawned["status"], "ok", "{spawned}"); + let id = spawned["id"].as_str().unwrap().to_string(); + let handle = ctx + .supervisor + .as_ref() + .unwrap() + .write() + .take(&id) + .expect("spawned agent must be registered"); + handle + .join_handle + .await + .expect("spawn task must not panic") + .expect("spawn task must return an AgentResult"); + id + }) + } + + #[test] + #[serial] + fn spawned_agent_success_fires_started_and_completed_with_matching_id() { + let _guard = TestConfigDirGuard::new(); + let _sink = hooks::test_sink::install(); + let agent_name = unique_agent_name("test_hook_spawn_ok"); + write_graph_agent_with_hooks(&agent_name, "t037_spawn_ok", false); + + let id = spawn_and_collect(&agent_name); + + let started = hook_captures_named("t037_spawn_ok_started"); + assert_eq!(started.len(), 1, "{started:?}"); + assert_eq!(started[0].envs["COYOTE_EVENT"], "agent.started"); + assert_eq!(started[0].envs["COYOTE_AGENT_ID"], id); + assert_eq!(started[0].envs["COYOTE_AGENT_NAME"], agent_name); + + let completed = hook_captures_named("t037_spawn_ok_completed"); + assert_eq!(completed.len(), 1, "{completed:?}"); + assert_eq!(completed[0].envs["COYOTE_EVENT"], "agent.completed"); + assert_eq!(completed[0].envs["COYOTE_AGENT_ID"], id); + assert_eq!(completed[0].envs["COYOTE_AGENT_NAME"], agent_name); + + assert!(hook_captures_named("t037_spawn_ok_failed").is_empty()); + } + + #[test] + #[serial] + fn spawned_agent_failure_fires_started_and_failed_with_matching_id() { + let _guard = TestConfigDirGuard::new(); + let _sink = hooks::test_sink::install(); + let agent_name = unique_agent_name("test_hook_spawn_fail"); + write_graph_agent_with_hooks(&agent_name, "t037_spawn_fail", true); + + let id = spawn_and_collect(&agent_name); + + let started = hook_captures_named("t037_spawn_fail_started"); + assert_eq!(started.len(), 1, "{started:?}"); + assert_eq!(started[0].envs["COYOTE_AGENT_ID"], id); + + let failed = hook_captures_named("t037_spawn_fail_failed"); + assert_eq!(failed.len(), 1, "{failed:?}"); + assert_eq!(failed[0].envs["COYOTE_EVENT"], "agent.failed"); + assert_eq!(failed[0].envs["COYOTE_AGENT_ID"], id); + assert!( + !failed[0].envs["COYOTE_AGENT_ERROR"].is_empty(), + "a failed spawn must carry its error" + ); + + assert!(hook_captures_named("t037_spawn_fail_completed").is_empty()); + } + + #[test] + fn hook_base_envs_carries_optional_names_only_when_present() { + let envs = hook_base_envs(HookEvent::AgentCompleted, None, None); + let keys: Vec<&str> = envs.iter().map(|(key, _)| key.as_str()).collect(); + assert!(keys.contains(&"COYOTE_EVENT")); + assert!(keys.contains(&"COYOTE_CONFIG_DIR")); + assert!(keys.contains(&"COYOTE_EVENT_TIMESTAMP")); + assert!(!keys.contains(&"COYOTE_SESSION_ID")); + assert!(!keys.contains(&"COYOTE_AGENT_NAME")); + assert!( + envs.iter() + .any(|(key, value)| key == "COYOTE_EVENT" && value == "agent.completed") + ); + + let envs = hook_base_envs(HookEvent::AgentFailed, Some("sess"), Some("bot")); + assert!( + envs.iter() + .any(|(key, value)| key == "COYOTE_SESSION_ID" && value == "sess") + ); + assert!( + envs.iter() + .any(|(key, value)| key == "COYOTE_AGENT_NAME" && value == "bot") + ); + } + fn run_agent_for_graph_error( agent_name: &str, graph_inputs: Option>, diff --git a/src/function/jobs.rs b/src/function/jobs.rs index e8e490f..c9e9a3b 100644 --- a/src/function/jobs.rs +++ b/src/function/jobs.rs @@ -9,6 +9,7 @@ use crate::config::{ McpRuntime, RequestContext, effective_max_concurrent_jobs, jobs_enabled, paths, }; use crate::graph; +use crate::hooks::{self, HookEvent, ResolvedHook}; use crate::mcp::{ MCP_DESCRIBE_META_FUNCTION_NAME_PREFIX, MCP_INVOKE_META_FUNCTION_NAME_PREFIX, MCP_PROMPT_META_FUNCTION_NAME_PREFIX, MCP_READ_META_FUNCTION_NAME_PREFIX, @@ -53,6 +54,63 @@ pub fn is_agent_task(supervisor: Option<&Arc>>, id: &str) -> || supervisor.is_some_and(|sup| sup.read().has_agent(id)) } +struct JobHookSnapshot { + completed: Vec, + failed: Vec, + session_name: Option, + agent_name: Option, + job_id: String, + tool: String, +} + +impl JobHookSnapshot { + fn resolve(ctx: &RequestContext, job_id: &str, tool: &str) -> Self { + Self { + completed: ctx.resolved_hooks(HookEvent::JobCompleted), + failed: ctx.resolved_hooks(HookEvent::JobFailed), + session_name: ctx + .session + .as_ref() + .map(|session| session.name().to_string()), + agent_name: ctx.agent.as_ref().map(|agent| agent.name().to_string()), + job_id: job_id.to_string(), + tool: tool.to_string(), + } + } + + /// Fires `job.completed` or `job.failed`. `fire_resolved` callers supply + /// their own base envs; the event name and timestamp are computed here so + /// they reflect the actual outcome rather than the moment the snapshot + /// was taken. + fn fire_terminal(self, success: bool, error: Option) { + let (event, resolved) = if success { + (HookEvent::JobCompleted, self.completed) + } else { + (HookEvent::JobFailed, self.failed) + }; + + if resolved.is_empty() { + return; + } + + let base_envs = hooks::base_envs_parts( + event, + self.session_name.as_deref(), + self.agent_name.as_deref(), + ); + let mut extras = vec![ + ("COYOTE_JOB_ID", self.job_id), + ("COYOTE_TOOL_NAME", self.tool), + ]; + + if let Some(error) = error { + extras.push(("COYOTE_JOB_ERROR", error)); + } + + hooks::fire_resolved(event, resolved, base_envs, &extras, None); + } +} + pub struct RingBuf { buf: Vec, capacity: usize, @@ -470,15 +528,27 @@ async fn handle_start(ctx: &mut RequestContext, args: &Value) -> Result { let task_notifications = Arc::clone(&ctx.notification_queue); let notify_id = job_id.clone(); let notify_tool = tool.clone(); + let hook_snapshot = JobHookSnapshot::resolve(ctx, &job_id, &tool); + hooks::fire( + HookEvent::JobStarted, + ctx, + &[ + ("COYOTE_JOB_ID", job_id.clone()), + ("COYOTE_TOOL_NAME", tool.clone()), + ], + None, + ); tokio::spawn(async move { let result = run_mcp_job(job_ctx, server, inner_tool, inner_args).await; let success = result.is_ok(); + let error = result.as_ref().err().map(|err| err.to_string()); task_state.lock().status = if success { JobStatus::Completed } else { JobStatus::Failed }; task_notifications.push(job_notification(¬ify_id, ¬ify_tool, success)); + hook_snapshot.fire_terminal(success, error); result }) } else { @@ -488,9 +558,26 @@ async fn handle_start(ctx: &mut RequestContext, args: &Value) -> Result { let task_notifications = Arc::clone(&ctx.notification_queue); let notify_id = job_id.clone(); let notify_tool = tool.clone(); + let hook_snapshot = JobHookSnapshot::resolve(ctx, &job_id, &tool); + hooks::fire( + HookEvent::JobStarted, + ctx, + &[ + ("COYOTE_JOB_ID", job_id.clone()), + ("COYOTE_TOOL_NAME", tool.clone()), + ], + None, + ); tokio::spawn(async move { let result = run_process_job(snapshot, Arc::clone(&task_state), task_buf).await; let success = matches!(&result, Ok(job_result) if job_result.exit_code == Some(0)); + let error = (!success).then(|| match &result { + Err(err) => err.to_string(), + Ok(job_result) => match job_result.exit_code { + Some(code) => format!("exited with code {code}"), + None => "terminated without an exit code".to_string(), + }, + }); let mut job_state = task_state.lock(); job_state.pgid = None; job_state.status = if success { @@ -502,6 +589,7 @@ async fn handle_start(ctx: &mut RequestContext, args: &Value) -> Result { drop(job_state); task_notifications.push(job_notification(¬ify_id, ¬ify_tool, success)); + hook_snapshot.fire_terminal(success, error); result }) }; @@ -3119,4 +3207,190 @@ mod tests { ); assert!(ctx.supervisor.is_none(), "no job may be spawned"); } + + fn job_hooks_map(marker: &str) -> hooks::HooksMap { + ["job.started", "job.completed", "job.failed"] + .into_iter() + .map(|event| { + ( + event.to_string(), + vec![hooks::HookDef { + name: marker.to_string(), + command: "true".to_string(), + }], + ) + }) + .collect() + } + + fn job_hooked_ctx(marker: &str) -> RequestContext { + let app = app_state_with_config(|config| { + config.hooks = job_hooks_map(marker); + }); + RequestContext::new(app, WorkingMode::Cmd) + } + + fn job_captures(marker: &str) -> Vec { + hooks::test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name == marker) + .collect() + } + + #[cfg(unix)] + #[test] + #[serial_test::serial] + fn job_hooks_fire_started_and_completed() { + run_async(async { + let marker = "job-hooks-ok-x7x"; + let mut ctx = job_hooked_ctx(marker); + ctx.declared_function_names.insert("echo".into()); + let _guard = hooks::test_sink::install(); + + let started = handle_start(&mut ctx, &json!({"tool": "echo", "arguments": {}})) + .await + .unwrap(); + assert_eq!(started["status"], "ok"); + let job_id = started["job_id"].as_str().unwrap().to_string(); + + // `job.started` is dispatched before `handle_start` returns. + let captures = job_captures(marker); + assert_eq!(captures.len(), 1, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::JobStarted); + assert_eq!( + captures[0].envs.get("COYOTE_JOB_ID").map(String::as_str), + Some(job_id.as_str()) + ); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("echo") + ); + + let collected = handle_collect(&ctx, &json!({"id": job_id})).await.unwrap(); + assert_eq!(collected["status"], "completed"); + + let captures = job_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + let completed = &captures[1]; + assert_eq!(completed.event, HookEvent::JobCompleted); + assert_eq!( + completed.envs.get("COYOTE_EVENT").map(String::as_str), + Some("job.completed") + ); + assert_eq!( + completed.envs.get("COYOTE_JOB_ID").map(String::as_str), + Some(job_id.as_str()) + ); + assert_eq!( + completed.envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("echo") + ); + assert!(completed.envs.contains_key("COYOTE_CONFIG_DIR")); + assert!(completed.envs.contains_key("COYOTE_EVENT_TIMESTAMP")); + assert!(!completed.envs.contains_key("COYOTE_SESSION_ID")); + assert!(!completed.envs.contains_key("COYOTE_JOB_ERROR")); + }); + } + + #[cfg(unix)] + #[test] + #[serial_test::serial] + fn job_hooks_fire_failed_with_job_error() { + run_async(async { + let marker = "job-hooks-fail-x7x"; + let mut ctx = job_hooked_ctx(marker); + ctx.declared_function_names.insert("false".into()); + let _guard = hooks::test_sink::install(); + + let started = handle_start(&mut ctx, &json!({"tool": "false", "arguments": {}})) + .await + .unwrap(); + let job_id = started["job_id"].as_str().unwrap().to_string(); + + let collected = handle_collect(&ctx, &json!({"id": job_id})).await.unwrap(); + assert_eq!(collected["status"], "failed"); + + let captures = job_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::JobStarted); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::JobFailed); + assert_eq!( + failed.envs.get("COYOTE_EVENT").map(String::as_str), + Some("job.failed") + ); + assert_eq!( + failed.envs.get("COYOTE_JOB_ID").map(String::as_str), + Some(job_id.as_str()) + ); + assert_eq!( + failed.envs.get("COYOTE_JOB_ERROR").map(String::as_str), + Some("exited with code 1") + ); + }); + } + + #[test] + #[serial_test::serial] + fn job_hooks_fire_for_mcp_jobs() { + run_async(async { + let marker = "job-hooks-mcp-x7x"; + let mut ctx = job_hooked_ctx(marker); + ctx.declared_function_names + .insert("mcp_invoke_fixture".into()); + let fixture = FixtureServer::default(); + let (mut runtime, _server) = fixture_runtime(fixture).await; + let mut filter = ToolFilter::default(); + filter.push_layer(LayerSource::Global, &[]); + runtime.tool_filters.insert("fixture".to_string(), filter); + ctx.tool_scope.mcp_runtime = runtime; + let _guard = hooks::test_sink::install(); + + let started = handle_start( + &mut ctx, + &json!({"tool": "mcp_invoke_fixture", "arguments": {"tool": "dup"}}), + ) + .await + .unwrap(); + let job_id = started["job_id"].as_str().unwrap().to_string(); + + let collected = handle_collect(&ctx, &json!({"id": job_id})).await.unwrap(); + assert_eq!(collected["status"], "failed"); + + let captures = job_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::JobStarted); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("mcp_invoke_fixture") + ); + let failed = &captures[1]; + assert_eq!(failed.event, HookEvent::JobFailed); + assert!( + failed + .envs + .get("COYOTE_JOB_ERROR") + .unwrap() + .contains("dup not found"), + "{failed:?}" + ); + }); + } + + #[test] + #[serial_test::serial] + fn rejected_job_start_fires_no_hooks() { + let marker = "job-hooks-rejected-x7x"; + let mut ctx = job_hooked_ctx(marker); + let _guard = hooks::test_sink::install(); + + let result = run_async(handle_start( + &mut ctx, + &json!({"tool": "undeclared_tool", "arguments": {}}), + )) + .unwrap(); + + assert_eq!(result["status"], "error"); + assert!(job_captures(marker).is_empty()); + } } diff --git a/src/function/mod.rs b/src/function/mod.rs index c2b73dc..38b6579 100644 --- a/src/function/mod.rs +++ b/src/function/mod.rs @@ -17,6 +17,7 @@ use crate::{ use crate::config::ensure_parent_exists; use crate::config::paths; +use crate::hooks::{self, HookEvent}; use crate::mcp::{ MCP_DESCRIBE_META_FUNCTION_NAME_PREFIX, MCP_INVOKE_META_FUNCTION_NAME_PREFIX, MCP_META_FUNCTION_PREFIXES, MCP_PROMPT_META_FUNCTION_NAME_PREFIX, @@ -1539,6 +1540,22 @@ impl ToolCall { } pub async fn eval(&self, ctx: &mut RequestContext) -> Result { + let result = self.eval_inner(ctx).await; + if let Err(err) = &result { + hooks::fire( + HookEvent::ToolFailed, + ctx, + &[ + ("COYOTE_TOOL_NAME", self.name.clone()), + ("COYOTE_TOOL_ERROR", err.to_string()), + ], + None, + ); + } + result + } + + async fn eval_inner(&self, ctx: &mut RequestContext) -> Result { let agent = ctx.agent.clone(); let functions = ctx.tool_scope.functions.clone(); let current_depth = ctx.current_depth; @@ -1566,7 +1583,8 @@ impl ToolCall { ); }; - cmd_args.push(json_data.to_string()); + let args_json = json_data.to_string(); + cmd_args.push(args_json.clone()); if (*IS_STDOUT_TERMINAL || *SHOW_TOOL_CALLS) && current_depth == 0 @@ -1575,6 +1593,14 @@ impl ToolCall { println!("{}", format_call_log(&cmd_name, &cmd_args, &json_data)); } + hooks::fire( + HookEvent::ToolStarted, + ctx, + &[("COYOTE_TOOL_NAME", self.name.clone())], + Some(args_json.clone()), + ); + let tool_started_at = Instant::now(); + let output = match cmd_name.as_str() { _ if cmd_name.starts_with(MCP_SEARCH_META_FUNCTION_NAME_PREFIX) => { Self::search_mcp_tools(ctx, &cmd_name, &json_data) @@ -1699,6 +1725,38 @@ impl ToolCall { }, }; + match output.get("tool_call_error") { + Some(error) => { + let error = error + .as_str() + .map(str::to_string) + .unwrap_or_else(|| error.to_string()); + hooks::fire( + HookEvent::ToolFailed, + ctx, + &[ + ("COYOTE_TOOL_NAME", self.name.clone()), + ("COYOTE_TOOL_ERROR", error), + ], + Some(args_json), + ); + } + None => { + hooks::fire( + HookEvent::ToolCompleted, + ctx, + &[ + ("COYOTE_TOOL_NAME", self.name.clone()), + ( + "COYOTE_TOOL_DURATION_MS", + tool_started_at.elapsed().as_millis().to_string(), + ), + ], + Some(args_json), + ); + } + } + Ok(output) } @@ -5243,4 +5301,277 @@ mod tests { assert!(err.starts_with(expected), "{name}: {err}"); } } + + fn tool_hooks_map(marker: &str) -> hooks::HooksMap { + ["tool.started", "tool.completed", "tool.failed"] + .into_iter() + .map(|event| { + ( + event.to_string(), + vec![hooks::HookDef { + name: marker.to_string(), + command: "true".to_string(), + }], + ) + }) + .collect() + } + + fn ctx_with_tool_hooks(marker: &str, update: impl FnOnce(&mut AppConfig)) -> RequestContext { + let mut app = AppState::test_default(); + let mut config = AppConfig { + hooks: tool_hooks_map(marker), + ..Default::default() + }; + update(&mut config); + app.config = Arc::new(config); + RequestContext::new(Arc::new(app), WorkingMode::Cmd) + } + + fn tool_captures(marker: &str) -> Vec { + hooks::test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name == marker) + .collect() + } + + #[test] + #[serial] + fn eval_fires_started_and_completed_for_builtin_tools() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-builtin-ok-x7x"; + let mut ctx = ctx_with_tool_hooks(marker, |config| { + config.function_calling_support = true; + config.auto_continue = true; + }); + ctx.tool_scope.functions.append_todo_functions(); + let _guard = hooks::test_sink::install(); + + let output = + run_async(call_with_args("todo__init", json!({"goal": "g"})).eval(&mut ctx)).unwrap(); + + assert_eq!(output["status"], "ok"); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("todo__init") + ); + assert_eq!(captures[0].payload.as_deref(), Some(r#"{"goal":"g"}"#)); + assert_eq!(captures[0].cwd, paths::config_dir()); + assert_eq!(captures[1].event, HookEvent::ToolCompleted); + assert!(captures[1].envs.contains_key("COYOTE_TOOL_DURATION_MS")); + assert_eq!(captures[1].payload.as_deref(), Some(r#"{"goal":"g"}"#)); + } + + #[test] + #[serial] + fn eval_fires_started_and_completed_for_mcp_meta_tools() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-mcp-ok-x7x"; + run_async(async { + let mut result = CallToolResult::success(vec![ContentBlock::text("hi")]); + result.structured_content = Some(json!({"ok": true})); + let fixture = FixtureServer { + tool_result: Some(result), + ..Default::default() + }; + let (runtime, _server) = fixture_runtime(fixture).await; + let mut ctx = ctx_with_tool_hooks(marker, |_| {}); + ctx.tool_scope.mcp_runtime = runtime; + ctx.tool_scope + .functions + .append_mcp_meta_functions(vec![tools_only("fixture")]); + let _guard = hooks::test_sink::install(); + + let output = call_with_args("mcp_invoke_fixture", json!({"tool": "dup"})) + .eval(&mut ctx) + .await + .unwrap(); + + assert_eq!(output["isError"], false); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("mcp_invoke_fixture") + ); + assert_eq!(captures[0].payload.as_deref(), Some(r#"{"tool":"dup"}"#)); + assert_eq!(captures[1].event, HookEvent::ToolCompleted); + }); + } + + #[cfg(unix)] + #[test] + #[serial] + fn eval_fires_started_and_completed_for_external_command_tools() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-external-ok-x7x"; + let mut ctx = ctx_with_tool_hooks(marker, |_| {}); + ctx.tool_scope + .functions + .append_declaration(FunctionDeclaration { + name: "echo".to_string(), + description: String::new(), + parameters: JsonSchema::default(), + agent: false, + }); + let _guard = hooks::test_sink::install(); + + let output = run_async(call_with_args("echo", json!({"x": 1})).eval(&mut ctx)).unwrap(); + + assert_eq!(output, Value::Null); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("echo") + ); + assert_eq!(captures[1].event, HookEvent::ToolCompleted); + } + + #[cfg(unix)] + #[test] + #[serial] + fn eval_fires_failed_for_external_command_nonzero_exit() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-external-fail-x7x"; + let mut ctx = ctx_with_tool_hooks(marker, |_| {}); + ctx.tool_scope + .functions + .append_declaration(FunctionDeclaration { + name: "false".to_string(), + description: String::new(), + parameters: JsonSchema::default(), + agent: false, + }); + let _guard = hooks::test_sink::install(); + + let output = run_async(call_with_args("false", json!({})).eval(&mut ctx)).unwrap(); + + assert!(output["tool_call_error"].is_string()); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!(captures[1].event, HookEvent::ToolFailed); + assert!( + captures[1] + .envs + .get("COYOTE_TOOL_ERROR") + .unwrap() + .contains("exited with code 1") + ); + assert!(!captures[1].envs.contains_key("COYOTE_TOOL_DURATION_MS")); + } + + #[test] + #[serial] + fn eval_fires_failed_on_error_shaped_output_with_identical_result() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-error-shape-x7x"; + // Default config leaves todo tools disabled, so the handler bails + // and eval shapes the failure as a `tool_call_error` output. + let mut hooked = ctx_with_tool_hooks(marker, |_| {}); + hooked.tool_scope.functions.append_todo_functions(); + let mut plain = RequestContext::new(Arc::new(AppState::test_default()), WorkingMode::Cmd); + plain.tool_scope.functions.append_todo_functions(); + let call = call_with_args("todo__done", json!({})); + let _guard = hooks::test_sink::install(); + + let hooked_output = run_async(call.eval(&mut hooked)).unwrap(); + let plain_output = run_async(call.eval(&mut plain)).unwrap(); + + assert_eq!( + hooked_output.to_string(), + plain_output.to_string(), + "hooks must not change the tool result" + ); + let error = hooked_output["tool_call_error"].as_str().unwrap(); + assert!(error.starts_with("Todo tool failed:"), "{error}"); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!(captures[1].event, HookEvent::ToolFailed); + assert_eq!( + captures[1] + .envs + .get("COYOTE_TOOL_ERROR") + .map(String::as_str), + Some(error) + ); + } + + #[test] + #[serial] + fn eval_fires_hooks_with_exact_user_prefixed_tool_name() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-user-x7x"; + let mut ctx = ctx_with_tool_hooks(marker, |_| {}); + ctx.tool_scope.functions.append_user_interaction_functions(); + let _guard = hooks::test_sink::install(); + + let output = run_async(call_with_args("user__confirm", json!({})).eval(&mut ctx)).unwrap(); + + // Without a question the user handler errors before prompting, so the + // error-shaped output must fire started + failed with the exact name. + let error = output["tool_call_error"].as_str().unwrap(); + assert!(error.starts_with("User interaction failed"), "{error}"); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 2, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolStarted); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("user__confirm") + ); + assert_eq!(captures[1].event, HookEvent::ToolFailed); + assert_eq!( + captures[1].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("user__confirm") + ); + assert_eq!( + captures[1] + .envs + .get("COYOTE_TOOL_ERROR") + .map(String::as_str), + Some(error) + ); + } + + #[test] + #[serial] + fn eval_err_fires_failed_without_started_and_identical_error() { + use crate::hooks::HookEvent; + let marker = "tool-hooks-eval-err-x7x"; + let mut hooked = ctx_with_tool_hooks(marker, |_| {}); + let mut plain = RequestContext::new(Arc::new(AppState::test_default()), WorkingMode::Cmd); + let call = call_with_args("nonexistent_tool", json!({})); + let _guard = hooks::test_sink::install(); + + let hooked_err = run_async(call.eval(&mut hooked)).unwrap_err(); + let plain_err = run_async(call.eval(&mut plain)).unwrap_err(); + + assert_eq!( + hooked_err.to_string(), + plain_err.to_string(), + "hooks must not change the error" + ); + let captures = tool_captures(marker); + assert_eq!(captures.len(), 1, "{captures:?}"); + assert_eq!(captures[0].event, HookEvent::ToolFailed); + assert_eq!( + captures[0].envs.get("COYOTE_TOOL_NAME").map(String::as_str), + Some("nonexistent_tool") + ); + assert!( + captures[0] + .envs + .get("COYOTE_TOOL_ERROR") + .unwrap() + .contains("Unexpected call") + ); + assert!(captures[0].payload.is_none()); + } } diff --git a/src/graph/agent.rs b/src/graph/agent.rs index 86ded61..edce152 100644 --- a/src/graph/agent.rs +++ b/src/graph/agent.rs @@ -6,6 +6,7 @@ use super::types::AgentNode; use super::wall_clock; use crate::config::RequestContext; use crate::function::agents::run_agent_for_graph; +use crate::hooks::{self, HookEvent}; use crate::supervisor::mailbox::{Inbox, PeerRegistry}; use crate::utils::create_abort_signal; use anyhow::{Context, Error, Result, anyhow}; @@ -35,6 +36,11 @@ impl AgentNodeExecutor { parent_ctx: &mut RequestContext, retire_peer_on_return: bool, ) -> Result { + let agent_hook_id = parent_ctx + .peer_assignment + .as_ref() + .map(|(id, _)| id.clone()) + .unwrap_or_else(|| node_id.to_string()); let result = run( node_id, node, @@ -43,6 +49,18 @@ impl AgentNodeExecutor { retire_peer_on_return, ) .await; + if let Err(e) = &result { + hooks::fire( + HookEvent::AgentFailed, + parent_ctx, + &[ + ("COYOTE_AGENT_ID", agent_hook_id), + ("COYOTE_AGENT_NAME", node.agent.clone()), + ("COYOTE_AGENT_ERROR", format!("{e:#}")), + ], + None, + ); + } outcome_from(node_id, node, state_manager, result) } } diff --git a/src/graph/executor.rs b/src/graph/executor.rs index 71a87b5..686b74b 100644 --- a/src/graph/executor.rs +++ b/src/graph/executor.rs @@ -1,6 +1,6 @@ use super::agent::{AgentExecutionOutcome, AgentNodeExecutor}; use super::llm::{LlmExecutionOutcome, LlmNodeExecutor}; -use super::logging::{GraphLogger, narrate_node_complete, narrate_node_failed}; +use super::logging::{GraphLogger, narrate_node_complete, narrate_node_failed, node_type_label}; use super::map::MapNodeExecutor; use super::rag::RagNodeExecutor; use super::script::ScriptExecutor; @@ -11,6 +11,8 @@ use super::user_interaction::{ApprovalNodeExecutor, InputNodeExecutor}; use super::validator::{AgentValidationContext, GraphValidator}; use super::wall_clock; use crate::config::{AgentVariable, AgentVariables, RenderMode, RequestContext}; +use crate::function::agents::hook_base_envs; +use crate::hooks::{self, HookEvent, ResolvedHook}; use crate::supervisor::mailbox::{Inbox, PeerAssignment, PeerRegistry, graph_agent_id}; use crate::utils::{AbortSignal, wait_abort_signal, wait_user_interrupt}; use anyhow::{Context, Result, anyhow, bail}; @@ -234,6 +236,16 @@ impl GraphExecutor { })? .clone(); logger.node_start(&node, in_super_step); + let node_hooks = NodeHookEmitter::resolve(ctx, node_id, node_type_label(&node)); + hooks::fire( + HookEvent::GraphNodeStarted, + ctx, + &[ + ("COYOTE_NODE_ID", node_id.clone()), + ("COYOTE_NODE_TYPE", node_hooks.node_type.to_string()), + ], + None, + ); let branch_state = state.fork_for_branch_state(); let mut branch_ctx = ctx.fork_for_branch(); let mut peer_id: Option = None; @@ -281,6 +293,7 @@ impl GraphExecutor { "aborted", in_super_step, ); + node_hooks.failed(Duration::default(), "aborted"); return ( current.clone(), branch_state, @@ -315,6 +328,7 @@ impl GraphExecutor { route.as_deref(), in_super_step, ); + node_hooks.completed(elapsed, route.as_deref()); } Ok(StepResult::End(_)) => { narrate_node_complete( @@ -324,6 +338,7 @@ impl GraphExecutor { Some("END"), in_super_step, ); + node_hooks.completed(elapsed, Some("END")); } Err(e) => { narrate_node_failed( @@ -333,6 +348,7 @@ impl GraphExecutor { &e.to_string(), in_super_step, ); + node_hooks.failed(elapsed, &e.to_string()); } } (current, state, result, elapsed) @@ -498,6 +514,67 @@ fn provision_frontier_peers( (Some(registry), assignments) } +struct NodeHookEmitter { + completed: Vec, + failed: Vec, + session_name: Option, + agent_name: Option, + node_id: String, + node_type: &'static str, +} + +impl NodeHookEmitter { + fn resolve(ctx: &RequestContext, node_id: &str, node_type: &'static str) -> Self { + Self { + completed: ctx.resolved_hooks(HookEvent::GraphNodeCompleted), + failed: ctx.resolved_hooks(HookEvent::GraphNodeFailed), + session_name: ctx + .session + .as_ref() + .map(|session| session.name().to_string()), + agent_name: ctx.agent.as_ref().map(|agent| agent.name().to_string()), + node_id: node_id.to_string(), + node_type, + } + } + + fn extras(&self, elapsed: Duration) -> Vec<(&'static str, String)> { + vec![ + ("COYOTE_NODE_ID", self.node_id.clone()), + ("COYOTE_NODE_TYPE", self.node_type.to_string()), + ("COYOTE_NODE_DURATION_MS", elapsed.as_millis().to_string()), + ] + } + + fn dispatch(&self, event: HookEvent, resolved: &[ResolvedHook], extras: &[(&str, String)]) { + hooks::fire_resolved( + event, + resolved.to_vec(), + hook_base_envs( + event, + self.session_name.as_deref(), + self.agent_name.as_deref(), + ), + extras, + None, + ); + } + + fn completed(&self, elapsed: Duration, route: Option<&str>) { + let mut extras = self.extras(elapsed); + if let Some(route) = route { + extras.push(("COYOTE_NODE_ROUTE", route.to_string())); + } + self.dispatch(HookEvent::GraphNodeCompleted, &self.completed, &extras); + } + + fn failed(&self, elapsed: Duration, error: &str) { + let mut extras = self.extras(elapsed); + extras.push(("COYOTE_NODE_ERROR", error.to_string())); + self.dispatch(HookEvent::GraphNodeFailed, &self.failed, &extras); + } +} + pub(super) struct TaskCancelGuard(Vec); impl TaskCancelGuard { @@ -928,6 +1005,8 @@ mod tests { max_concurrent_agents: None, max_agent_depth: None, global_tools: Vec::new(), + hooks: Default::default(), + global_hooks: Vec::new(), mcp_servers: Vec::new(), mcp_tools: None, skills_enabled: None, @@ -1048,9 +1127,10 @@ mod tests { mod integration_tests { use super::*; use crate::config::paths; - use crate::config::{AppState, WorkingMode}; + use crate::config::{Agent, AgentConfig, AppConfig, AppState, WorkingMode}; #[cfg(unix)] use crate::function::jobs::RingBuf; + use crate::hooks::HookDef; #[cfg(unix)] use crate::supervisor::{JobHandle, JobResult, JobState, JobStatus, Supervisor, notification}; use crate::utils::{create_abort_signal, get_env_name, temp_file}; @@ -1066,6 +1146,33 @@ mod integration_tests { which::which(name).is_ok() } + fn hooks_config(entries: &[(&str, &str)]) -> hooks::HooksMap { + let mut map = hooks::HooksMap::new(); + for (event, name) in entries { + map.entry((*event).to_string()).or_default().push(HookDef { + name: (*name).to_string(), + command: "true".to_string(), + }); + } + map + } + + fn ctx_with_global_hooks(hooks: hooks::HooksMap) -> RequestContext { + let mut app = AppState::test_default(); + app.config = Arc::new(AppConfig { + hooks, + ..Default::default() + }); + RequestContext::new(Arc::new(app), WorkingMode::Cmd) + } + + fn hook_captures_named(name: &str) -> Vec { + hooks::test_sink::snapshot() + .into_iter() + .filter(|capture| capture.hook_name == name) + .collect() + } + struct TestWorkspace { dir: PathBuf, } @@ -1208,6 +1315,364 @@ nodes: assert_eq!(result, "held|held"); } + /// An agent node that fails but declares a fallback must surface the + /// failure to observers as `agent.failed`, fired at the point of + /// failure, even though the fallback then converts it into a normal + /// node completion that routes to the fallback target — so the same + /// node also yields `graph.node.completed`, never `graph.node.failed`. + #[tokio::test] + async fn agent_node_failure_with_fallback_fires_agent_failed_and_node_completed() { + let _sink = hooks::test_sink::install(); + let mut ctx = ctx_with_global_hooks(hooks_config(&[ + ("agent.failed", "t037_fb_agent_failed"), + ("graph.node.completed", "t037_fb_node_completed"), + ("graph.node.failed", "t037_fb_node_failed"), + ])); + // Forces the agent node to fail before it ever loads the (absent) + // agent from disk, keeping the test hermetic. + ctx.current_depth = crate::config::default_max_agent_depth(); + + let yaml = r#" +name: t +settings: + validate_before_run: false +start: worker +nodes: + worker: + type: agent + agent: missing_agent + prompt: p + fallback: recover + next: done + recover: + type: end + output: recovered + done: + type: end + output: done +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + let abort = create_abort_signal(); + let result = GraphExecutor::new(graph, ".") + .execute(&mut ctx, abort) + .await + .unwrap_or_else(|e| panic!("executor failed: {e:#}")); + assert_eq!(result, "recovered"); + + let agent_failed = hook_captures_named("t037_fb_agent_failed"); + assert_eq!(agent_failed.len(), 1, "{agent_failed:?}"); + assert_eq!(agent_failed[0].envs["COYOTE_EVENT"], "agent.failed"); + assert_eq!(agent_failed[0].envs["COYOTE_AGENT_ID"], "worker"); + assert_eq!(agent_failed[0].envs["COYOTE_AGENT_NAME"], "missing_agent"); + assert!( + agent_failed[0].envs["COYOTE_AGENT_ERROR"].contains("Max agent depth exceeded"), + "{:?}", + agent_failed[0].envs + ); + + let node_completed = hook_captures_named("t037_fb_node_completed"); + let worker_completed: Vec<_> = node_completed + .iter() + .filter(|capture| capture.envs["COYOTE_NODE_ID"] == "worker") + .collect(); + assert_eq!(worker_completed.len(), 1, "{node_completed:?}"); + assert_eq!(worker_completed[0].envs["COYOTE_NODE_TYPE"], "agent"); + assert_eq!(worker_completed[0].envs["COYOTE_NODE_ROUTE"], "recover"); + + let node_failed = hook_captures_named("t037_fb_node_failed"); + assert!( + node_failed + .iter() + .all(|capture| capture.envs["COYOTE_NODE_ID"] != "worker"), + "a node that continues via fallback must not fire graph.node.failed: {node_failed:?}" + ); + } + + /// Two branches running in the same super-step each carry their own + /// node metadata, captured task-locally before the branch was spawned; + /// the distinct routes prove one branch's metadata never leaks into a + /// sibling's events. + #[tokio::test] + async fn parallel_super_step_branches_fire_per_node_metadata() { + if !cmd_available("bash") { + eprintln!("skipping: bash not available"); + return; + } + let _sink = hooks::test_sink::install(); + let ws = TestWorkspace::new(); + for script in [ + "dispatcher.sh", + "worker_a.sh", + "worker_b.sh", + "sink_a.sh", + "sink_b.sh", + ] { + ws.write_script(script, "#!/bin/bash\necho '{}'\n"); + } + let mut ctx = ctx_with_global_hooks(hooks_config(&[ + ("graph.node.started", "t037_par_started"), + ("graph.node.completed", "t037_par_completed"), + ])); + + let yaml = r#" +name: t +start: dispatcher +nodes: + dispatcher: + type: script + script: dispatcher.sh + state_updates: {} + next: [worker_a, worker_b] + worker_a: + type: script + script: worker_a.sh + state_updates: {} + next: sink_a + worker_b: + type: script + script: worker_b.sh + state_updates: {} + next: sink_b + sink_a: + type: script + script: sink_a.sh + state_updates: {} + next: join + sink_b: + type: script + script: sink_b.sh + state_updates: {} + next: join + join: + type: end + output: done +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + let abort = create_abort_signal(); + let result = GraphExecutor::new(graph, &ws.dir) + .execute(&mut ctx, abort) + .await + .unwrap_or_else(|e| panic!("executor failed: {e:#}")); + assert_eq!(result, "done"); + + let started = hook_captures_named("t037_par_started"); + for node_id in [ + "dispatcher", + "worker_a", + "worker_b", + "sink_a", + "sink_b", + "join", + ] { + assert_eq!( + started + .iter() + .filter(|capture| capture.envs["COYOTE_NODE_ID"] == node_id) + .count(), + 1, + "exactly one graph.node.started for '{node_id}': {started:?}" + ); + } + + let completed = hook_captures_named("t037_par_completed"); + let completed_route = |node_id: &str| -> String { + let matches: Vec<_> = completed + .iter() + .filter(|capture| capture.envs["COYOTE_NODE_ID"] == node_id) + .collect(); + assert_eq!( + matches.len(), + 1, + "exactly one graph.node.completed for '{node_id}': {completed:?}" + ); + let expected_type = if node_id == "join" { "end" } else { "script" }; + assert_eq!(matches[0].envs["COYOTE_NODE_TYPE"], expected_type); + let duration = &matches[0].envs["COYOTE_NODE_DURATION_MS"]; + let _: u128 = duration + .parse() + .unwrap_or_else(|_| panic!("non-numeric duration for '{node_id}': {duration}")); + matches[0].envs["COYOTE_NODE_ROUTE"].clone() + }; + assert_eq!(completed_route("dispatcher"), "worker_a, worker_b"); + assert_eq!(completed_route("worker_a"), "sink_a"); + assert_eq!(completed_route("worker_b"), "sink_b"); + assert_eq!(completed_route("sink_a"), "join"); + assert_eq!(completed_route("sink_b"), "join"); + assert_eq!(completed_route("join"), "END"); + } + + /// A graph agent's hooks come from graph.yaml's top level: they resolve + /// through the agent scope end-to-end, while a global hook on the same + /// event stays gated off because the agent whitelisted nothing. + #[tokio::test] + async fn graph_agent_hooks_resolve_from_graph_yaml_top_level() { + let _sink = hooks::test_sink::install(); + let mut ctx = + ctx_with_global_hooks(hooks_config(&[("graph.node.completed", "t037_gy_global")])); + + let yaml = r#" +name: hooked_graph +settings: + validate_before_run: false +start: done +hooks: + graph.node.started: + - name: t037_gy_started + command: "true" + graph.node.completed: + - name: t037_gy_completed + command: "true" +nodes: + done: + type: end + output: ok +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + ctx.agent = Some(Agent::test_new(AgentConfig::from_graph( + "hooked_graph", + &graph, + ))); + + let abort = create_abort_signal(); + let result = GraphExecutor::new(graph, ".") + .execute(&mut ctx, abort) + .await + .unwrap_or_else(|e| panic!("executor failed: {e:#}")); + assert_eq!(result, "ok"); + + let started = hook_captures_named("t037_gy_started"); + assert_eq!(started.len(), 1, "{started:?}"); + assert_eq!(started[0].envs["COYOTE_EVENT"], "graph.node.started"); + assert_eq!(started[0].envs["COYOTE_NODE_ID"], "done"); + assert_eq!(started[0].envs["COYOTE_NODE_TYPE"], "end"); + assert_eq!(started[0].envs["COYOTE_AGENT_NAME"], "hooked_graph"); + + let completed = hook_captures_named("t037_gy_completed"); + assert_eq!(completed.len(), 1, "{completed:?}"); + assert_eq!(completed[0].envs["COYOTE_NODE_ID"], "done"); + assert_eq!(completed[0].envs["COYOTE_NODE_ROUTE"], "END"); + assert_eq!(completed[0].envs["COYOTE_AGENT_NAME"], "hooked_graph"); + + assert!( + hook_captures_named("t037_gy_global").is_empty(), + "an agent context must not run global hooks it did not whitelist" + ); + } + + /// A node that genuinely fails — no fallback to absorb the error — fires + /// `graph.node.failed` with the full payload observers rely on: the node's + /// id and type, a numeric duration, and the error text, even though the + /// failure then propagates out of the executor. + #[tokio::test] + async fn genuine_node_failure_fires_graph_node_failed_with_payload() { + let _sink = hooks::test_sink::install(); + let mut ctx = ctx_with_global_hooks(hooks_config(&[( + "graph.node.failed", + "t037_gf_node_failed", + )])); + // Forces the agent node to fail before it ever loads the (absent) + // agent from disk, keeping the test hermetic. + ctx.current_depth = crate::config::default_max_agent_depth(); + + let yaml = r#" +name: t +settings: + validate_before_run: false +start: worker +nodes: + worker: + type: agent + agent: missing_agent + prompt: p + next: done + done: + type: end + output: done +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + let abort = create_abort_signal(); + let result = GraphExecutor::new(graph, ".") + .execute(&mut ctx, abort) + .await; + let error = result.expect_err("a node failure without a fallback must propagate"); + + let failed = hook_captures_named("t037_gf_node_failed"); + assert_eq!(failed.len(), 1, "{failed:?}"); + let envs = &failed[0].envs; + assert_eq!(envs["COYOTE_EVENT"], "graph.node.failed"); + assert_eq!(envs["COYOTE_NODE_ID"], "worker"); + assert_eq!(envs["COYOTE_NODE_TYPE"], "agent"); + let duration = &envs["COYOTE_NODE_DURATION_MS"]; + let _: u128 = duration + .parse() + .unwrap_or_else(|_| panic!("non-numeric duration: {duration}")); + assert!(!envs["COYOTE_NODE_ERROR"].is_empty(), "{envs:?}"); + assert!( + format!("{error:#}").contains("Max agent depth exceeded"), + "unexpected executor error: {error:#}" + ); + } + + /// A malformed hook definition reaching the executor seam through + /// graph.yaml's top level — an entry whose command is blank — degrades + /// safely: the node still runs, the malformed hook is skipped with a + /// debug log, and a well-formed sibling on the same event still fires. + #[tokio::test] + async fn malformed_graph_yaml_hook_is_skipped_without_breaking_the_node() { + crate::testing::install_warn_collector(); + let _sink = hooks::test_sink::install(); + let mut ctx = make_ctx(); + + let yaml = r#" +name: hooked_graph +settings: + validate_before_run: false +start: done +hooks: + graph.node.completed: + - name: t037_mal_empty + command: " " + - name: t037_mal_valid + command: "true" +nodes: + done: + type: end + output: ok +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + ctx.agent = Some(Agent::test_new(AgentConfig::from_graph( + "hooked_graph", + &graph, + ))); + + let abort = create_abort_signal(); + let result = GraphExecutor::new(graph, ".") + .execute(&mut ctx, abort) + .await + .unwrap_or_else(|e| panic!("executor failed: {e:#}")); + assert_eq!(result, "ok"); + + assert!( + hook_captures_named("t037_mal_empty").is_empty(), + "a blank-command hook must be skipped, not dispatched" + ); + let valid = hook_captures_named("t037_mal_valid"); + assert_eq!(valid.len(), 1, "{valid:?}"); + assert_eq!(valid[0].envs["COYOTE_NODE_ID"], "done"); + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!( + debugs + .iter() + .any(|message| message.contains("t037_mal_empty") + && message.contains("empty command")), + "expected a debug log for the skipped hook: {debugs:?}" + ); + } + /// Declared variables are seeded into state at run start, so the agent /// node's strict prompt interpolation of `{{foo}}` succeeds instead of /// bailing with "not found in state". `ctx.agent` is None here, so each diff --git a/src/graph/map.rs b/src/graph/map.rs index 09fb027..514eea5 100644 --- a/src/graph/map.rs +++ b/src/graph/map.rs @@ -497,6 +497,8 @@ mod tests { max_concurrent_agents: None, max_agent_depth: None, global_tools: Vec::new(), + hooks: Default::default(), + global_hooks: Vec::new(), mcp_servers: Vec::new(), mcp_tools: None, skills_enabled: None, diff --git a/src/graph/types.rs b/src/graph/types.rs index 6a8c02f..0ac6c1c 100644 --- a/src/graph/types.rs +++ b/src/graph/types.rs @@ -1,4 +1,5 @@ use crate::config::AgentVariable; +use crate::hooks::HooksMap; use anyhow::Result; use indexmap::IndexMap; use serde::{Deserialize, Serialize}; @@ -43,6 +44,12 @@ pub struct Graph { #[serde(default)] pub global_tools: Vec, + #[serde(default)] + pub hooks: HooksMap, + + #[serde(default)] + pub global_hooks: Vec, + #[serde(default)] pub mcp_servers: Vec, @@ -611,6 +618,52 @@ nodes: } } + #[test] + fn graph_hooks_and_global_hooks_parse_and_round_trip() { + let yaml = r#" +name: g +start: e +hooks: + tool.started: + - name: notify + command: ./hooks/notify.sh +global_hooks: + - tool.started.notify +nodes: + e: + id: e + type: end + output: done +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + + assert_eq!(graph.hooks["tool.started"][0].name, "notify"); + assert_eq!(graph.hooks["tool.started"][0].command, "./hooks/notify.sh"); + assert_eq!(graph.global_hooks, vec!["tool.started.notify"]); + + let reparsed: Graph = + serde_yaml::from_str(&serde_yaml::to_string(&graph).unwrap()).unwrap(); + assert_eq!(reparsed.hooks, graph.hooks); + assert_eq!(reparsed.global_hooks, graph.global_hooks); + } + + #[test] + fn graph_without_hooks_keys_parses_empty() { + let yaml = r#" +name: g +start: e +nodes: + e: + id: e + type: end + output: done +"#; + let graph: Graph = serde_yaml::from_str(yaml).unwrap(); + + assert!(graph.hooks.is_empty()); + assert!(graph.global_hooks.is_empty()); + } + #[test] fn agent_node_teammates_parses_and_defaults_false() { let yaml = r#" diff --git a/src/graph/validator.rs b/src/graph/validator.rs index b82e983..2797325 100644 --- a/src/graph/validator.rs +++ b/src/graph/validator.rs @@ -1459,6 +1459,8 @@ mod tests { max_concurrent_agents: None, max_agent_depth: None, global_tools: Vec::new(), + hooks: Default::default(), + global_hooks: Vec::new(), mcp_servers: Vec::new(), mcp_tools: None, skills_enabled: None, diff --git a/src/hooks.rs b/src/hooks.rs new file mode 100644 index 0000000..4c97dee --- /dev/null +++ b/src/hooks.rs @@ -0,0 +1,1760 @@ +use crate::config::{RequestContext, ensure_parent_exists, paths}; +use crate::function::write_file_atomic; + +use anyhow::{Result, anyhow}; +use chrono::{SecondsFormat, Utc}; +use indexmap::IndexMap; +use rand::distr::{Alphanumeric, SampleString}; +use rust_embed::Embed; +use serde::{Deserialize, Serialize}; +use std::path::{Path, PathBuf}; +use std::process::Stdio; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; +use tokio::io::AsyncWriteExt; +use tokio::process::Command; +use tokio::sync::Notify; + +#[derive(Embed)] +#[folder = "assets/hooks/"] +struct HookAssets; + +pub fn install_builtin_hooks(force: bool) -> Result<()> { + info!( + "Installing built-in example hooks in {}", + paths::hooks_dir().display() + ); + + let mut wrote_any = false; + for file in HookAssets::iter() { + debug!("Processing hook file: {}", file.as_ref()); + + let embedded_file = HookAssets::get(&file) + .ok_or_else(|| anyhow!("Failed to load embedded hook file: {}", file.as_ref()))?; + let content = unsafe { std::str::from_utf8_unchecked(&embedded_file.data) }; + let file_path = paths::hooks_dir().join(file.as_ref()); + + if file_path.exists() && !force { + debug!( + "Hook file already exists, skipping: {}", + file_path.display() + ); + continue; + } + + ensure_parent_exists(&file_path)?; + info!("Creating hook file: {}", file_path.display()); + write_file_atomic(&file_path, content, Some(0o755))?; + wrote_any = true; + } + + if wrote_any && paths::hooks_dir() != paths::config_dir().join("hooks") { + warn!( + "{} overrides the hooks dir: example scripts install to {}, but relative \ + global-hook commands resolve against {}", + crate::utils::get_env_name("hooks_dir"), + paths::hooks_dir().display(), + paths::config_dir().display() + ); + } + + Ok(()) +} + +/// A single named hook: an external command to run when its event fires. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub struct HookDef { + pub name: String, + pub command: String, +} + +/// Ordered mapping of event name -> hook definitions, shared by every config scope. +pub type HooksMap = IndexMap>; + +/// Every event hooks can be attached to. `as_str` yields the dotted name used +/// as the key in `hooks:` maps and in `global_hooks` whitelist entries. +#[allow(dead_code)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum HookEvent { + TurnStarted, + TurnCompleted, + TurnInterrupted, + TurnFailed, + SessionStarted, + SessionEnded, + SessionCompressed, + ToolStarted, + ToolCompleted, + ToolFailed, + LlmRequestStarted, + LlmRequestCompleted, + LlmRequestFailed, + AgentStarted, + AgentCompleted, + AgentFailed, + GraphNodeStarted, + GraphNodeCompleted, + GraphNodeFailed, + JobStarted, + JobCompleted, + JobFailed, +} + +impl HookEvent { + pub fn as_str(self) -> &'static str { + match self { + HookEvent::TurnStarted => "turn.started", + HookEvent::TurnCompleted => "turn.completed", + HookEvent::TurnInterrupted => "turn.interrupted", + HookEvent::TurnFailed => "turn.failed", + HookEvent::SessionStarted => "session.started", + HookEvent::SessionEnded => "session.ended", + HookEvent::SessionCompressed => "session.compressed", + HookEvent::ToolStarted => "tool.started", + HookEvent::ToolCompleted => "tool.completed", + HookEvent::ToolFailed => "tool.failed", + HookEvent::LlmRequestStarted => "llm.request.started", + HookEvent::LlmRequestCompleted => "llm.request.completed", + HookEvent::LlmRequestFailed => "llm.request.failed", + HookEvent::AgentStarted => "agent.started", + HookEvent::AgentCompleted => "agent.completed", + HookEvent::AgentFailed => "agent.failed", + HookEvent::GraphNodeStarted => "graph.node.started", + HookEvent::GraphNodeCompleted => "graph.node.completed", + HookEvent::GraphNodeFailed => "graph.node.failed", + HookEvent::JobStarted => "job.started", + HookEvent::JobCompleted => "job.completed", + HookEvent::JobFailed => "job.failed", + } + } +} + +/// An owned snapshot of one hook to run. Detached dispatch tasks, job +/// snapshots, and forked graph-branch contexts carry it without borrowing the +/// context it was resolved from. +#[derive(Debug, Clone)] +pub struct ResolvedHook { + pub name: String, + /// Whitelist form `.`, e.g. `tool.started.notify`. + pub full_name: String, + pub command: String, + pub cwd: PathBuf, +} + +impl RequestContext { + /// Hooks that apply to this context for `event`, in firing order: global + /// (whitelisted through `global_hooks` when an agent is active), then + /// role, then agent. Duplicate names across scopes all fire. + /// + /// Role hooks resolve from the role held directly on the context or, + /// when the active role was moved into (or restored by) a session, from + /// the hooks snapshot the session captured via `Session::set_role` / + /// `Session::load_from_ctx`. + #[allow(dead_code)] + pub fn resolved_hooks(&self, event: HookEvent) -> Vec { + resolve_hooks( + event, + &self.app.config.hooks, + self.agent + .as_ref() + .map(|agent| (agent.global_hooks(), agent.name())), + self.role + .as_ref() + .and_then(|role| role.hooks()) + .or_else(|| { + self.session + .as_ref() + .and_then(|session| session.role_hooks()) + }), + self.agent + .as_ref() + .map(|agent| (agent.hooks(), agent.name())), + ) + } +} + +fn resolve_hooks( + event: HookEvent, + global_hooks: &HooksMap, + agent_gate: Option<(&[String], &str)>, + role_hooks: Option<&HooksMap>, + agent_hooks: Option<(&HooksMap, &str)>, +) -> Vec { + let event_name = event.as_str(); + let mut resolved = Vec::new(); + + if let Some(defs) = global_hooks.get(event_name) { + let cwd = paths::config_dir(); + for def in defs { + let full_name = format!("{event_name}.{}", def.name); + let admitted = match agent_gate { + None => true, + Some((gate, _)) => gate.contains(&full_name), + }; + if admitted { + resolved.push(ResolvedHook { + name: def.name.clone(), + full_name, + command: def.command.clone(), + cwd: cwd.clone(), + }); + } + } + } + if let Some((gate, agent_name)) = agent_gate + && !global_hooks.is_empty() + { + let prefix = format!("{event_name}."); + for entry in gate { + if let Some(name) = entry.strip_prefix(&prefix) { + let known = global_hooks + .get(event_name) + .is_some_and(|defs| defs.iter().any(|def| def.name == name)); + if !known { + debug!( + "Ignoring unknown global hook whitelist entry '{entry}' for agent '{agent_name}'" + ); + } + } + } + } + + if let Some(defs) = role_hooks.and_then(|hooks| hooks.get(event_name)) { + push_defs(event_name, defs, &paths::roles_dir(), &mut resolved); + } + + if let Some((hooks, agent_name)) = agent_hooks + && let Some(defs) = hooks.get(event_name) + { + push_defs( + event_name, + defs, + &paths::agent_data_dir(agent_name), + &mut resolved, + ); + } + + resolved +} + +fn push_defs(event_name: &str, defs: &[HookDef], cwd: &Path, out: &mut Vec) { + for def in defs { + out.push(ResolvedHook { + name: def.name.clone(), + full_name: format!("{event_name}.{}", def.name), + command: def.command.clone(), + cwd: cwd.to_path_buf(), + }); + } +} + +#[allow(dead_code)] +pub fn fire( + event: HookEvent, + ctx: &RequestContext, + extras: &[(&str, String)], + payload: Option, +) { + let resolved = ctx.resolved_hooks(event); + if resolved.is_empty() { + return; + } + + fire_resolved(event, resolved, base_envs(event, ctx), extras, payload); +} + +/// Dispatches already-resolved hooks, for call sites that outlive the context +/// borrow they resolved from. Same fire-and-forget semantics as [`fire`]. +#[allow(dead_code)] +pub fn fire_resolved( + event: HookEvent, + resolved: Vec, + base_envs: Vec<(String, String)>, + extras: &[(&str, String)], + payload: Option, +) { + let runtime = tokio::runtime::Handle::try_current(); + for hook in resolved { + if hook.command.trim().is_empty() { + debug!( + "Skipping hook '{}' (cwd '{}'): empty command", + hook.full_name, + hook.cwd.display() + ); + continue; + } + + let mut envs = base_envs.clone(); + envs.push(("COYOTE_HOOK_NAME".to_string(), hook.name.clone())); + envs.extend( + extras + .iter() + .map(|(key, value)| (key.to_string(), value.clone())), + ); + let envs: Vec<(String, String)> = envs + .into_iter() + .map(|(key, mut value)| { + // `end` is a char boundary by construction, so `truncate` cannot panic. + let end = truncate_env_value(&value).len(); + value.truncate(end); + (key, value) + }) + .collect(); + + #[cfg(test)] + if test_sink::record(event, &hook, &envs, payload.as_deref()) { + continue; + } + + let Ok(runtime) = &runtime else { + debug!( + "Skipping hook '{}': no Tokio runtime on this thread", + hook.full_name + ); + continue; + }; + + let payload = payload.clone(); + drop(runtime.spawn(run_hook( + event, + hook, + envs, + payload, + SpawnAckGuard::register(), + ))); + } +} + +static PENDING_SPAWNS: AtomicUsize = AtomicUsize::new(0); +static SPAWN_NOTIFY: Notify = Notify::const_new(); + +/// Marks one dispatch as pending until its spawn attempt resolves. The +/// decrement lives in `Drop` so it also fires on panic or when the dispatch +/// task is dropped before it was ever polled. +struct SpawnAckGuard; + +impl SpawnAckGuard { + fn register() -> Self { + PENDING_SPAWNS.fetch_add(1, Ordering::SeqCst); + SpawnAckGuard + } +} + +impl Drop for SpawnAckGuard { + fn drop(&mut self) { + PENDING_SPAWNS.fetch_sub(1, Ordering::SeqCst); + SPAWN_NOTIFY.notify_waiters(); + } +} + +/// Waits until every in-flight dispatch has finished its spawn attempt — +/// payload file written and `Command::spawn` returned, successfully or not — +/// or until `timeout` elapses. For exit-path call sites only (session end / +/// final turn): this waits for child spawn, never completion. Hook children +/// deliberately outlive coyote as orphans. +#[allow(dead_code)] +pub async fn drain_pending(timeout: Duration) { + let deadline = tokio::time::Instant::now() + timeout; + while PENDING_SPAWNS.load(Ordering::SeqCst) > 0 { + let mut notified = std::pin::pin!(SPAWN_NOTIFY.notified()); + notified.as_mut().enable(); + if PENDING_SPAWNS.load(Ordering::SeqCst) == 0 { + return; + } + if tokio::time::timeout_at(deadline, notified).await.is_err() { + debug!( + "Gave up waiting for {} pending hook dispatch(es) after {timeout:?}", + PENDING_SPAWNS.load(Ordering::SeqCst) + ); + return; + } + } +} + +fn base_envs(event: HookEvent, ctx: &RequestContext) -> Vec<(String, String)> { + base_envs_parts( + event, + ctx.session.as_ref().map(|session| session.name()), + ctx.agent.as_ref().map(|agent| agent.name()), + ) +} + +pub(crate) fn base_envs_parts( + event: HookEvent, + session_name: Option<&str>, + agent_name: Option<&str>, +) -> Vec<(String, String)> { + let mut envs = vec![ + ("COYOTE_EVENT".to_string(), event.as_str().to_string()), + ( + "COYOTE_CONFIG_DIR".to_string(), + paths::config_dir().display().to_string(), + ), + ( + "COYOTE_EVENT_TIMESTAMP".to_string(), + Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), + ), + ]; + if let Some(name) = session_name { + envs.push(("COYOTE_SESSION_ID".to_string(), name.to_string())); + } + if let Some(name) = agent_name { + envs.push(("COYOTE_AGENT_NAME".to_string(), name.to_string())); + } + envs +} + +async fn write_payload_file(path: &Path, json: &str) -> std::io::Result<()> { + let mut options = tokio::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(0o600); + let mut file = options.open(path).await?; + file.write_all(json.as_bytes()).await?; + file.flush().await +} + +#[cfg(test)] +static PAYLOAD_DIR_OVERRIDE: std::sync::Mutex> = std::sync::Mutex::new(None); + +fn payload_dir() -> PathBuf { + #[cfg(test)] + if let Some(dir) = PAYLOAD_DIR_OVERRIDE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone() + { + return dir; + } + std::env::temp_dir() +} + +async fn run_hook( + event: HookEvent, + hook: ResolvedHook, + mut envs: Vec<(String, String)>, + payload: Option, + ack: SpawnAckGuard, +) { + let payload_file = match payload { + Some(json) => { + let nonce = Alphanumeric.sample_string(&mut rand::rng(), 16); + let path = payload_dir().join(format!("coyote-hook-{}-{nonce}.json", event.as_str())); + match write_payload_file(&path, &json).await { + Ok(()) => Some(path), + Err(err) => { + debug!( + "Failed to write payload file for hook '{}': {err}", + hook.full_name + ); + None + } + } + } + None => None, + }; + if let Some(path) = &payload_file { + envs.push(( + "COYOTE_HOOK_PAYLOAD_FILE".to_string(), + path.display().to_string(), + )); + } + + #[cfg(not(windows))] + let mut command = { + let mut command = Command::new("sh"); + command.arg("-c").arg(&hook.command); + command + }; + // cmd.exe does not parse CommandLineToArgvW quoting, so the command line + // must be passed verbatim rather than through arg()'s quoting rules. + #[cfg(windows)] + let mut command = { + use std::os::windows::process::CommandExt; + let mut command = Command::new("cmd"); + command.arg("/C"); + command.as_std_mut().raw_arg(&hook.command); + command + }; + + command + .envs(envs) + .current_dir(&hook.cwd) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + // A fresh process group keeps a terminal Ctrl-C aimed at coyote from + // signalling hook children, which deliberately outlive coyote. + #[cfg(unix)] + command.process_group(0); + match command.spawn() { + Ok(mut child) => { + drop(ack); + let _ = child.wait().await; + } + Err(err) => { + debug!( + "Failed to spawn hook '{}' in '{}': {err}", + hook.full_name, + hook.cwd.display() + ); + drop(ack); + } + } + + if let Some(path) = payload_file { + let _ = tokio::fs::remove_file(path).await; + } +} + +/// Truncates an env value to 2048 bytes without splitting a UTF-8 character. +/// `fire_resolved` applies it to every env value it dispatches, with one +/// exemption: `COYOTE_HOOK_PAYLOAD_FILE` is pushed by `run_hook` after the +/// truncation pass, because that value is engine-generated, inherently short, +/// and a truncated path would be corrupt rather than merely trimmed. Exported +/// for call sites that need the bound before dispatch. +#[allow(dead_code)] +pub fn truncate_env_value(value: &str) -> &str { + const MAX_ENV_VALUE_BYTES: usize = 2048; + if value.len() <= MAX_ENV_VALUE_BYTES { + return value; + } + let mut end = MAX_ENV_VALUE_BYTES; + while !value.is_char_boundary(end) { + end -= 1; + } + &value[..end] +} + +#[cfg(test)] +pub(crate) mod test_sink { + use super::{HookEvent, ResolvedHook}; + use std::collections::HashMap; + use std::sync::Mutex; + use std::sync::atomic::{AtomicUsize, Ordering}; + + static INSTALLED: AtomicUsize = AtomicUsize::new(0); + // A test that panics while holding the buffer poisons its mutex; every + // lock site recovers via `into_inner` so later tests keep working. + pub(super) static CAPTURES: Mutex> = Mutex::new(Vec::new()); + + /// One dispatch the sink recorded instead of spawning. + #[derive(Debug, Clone)] + pub struct Capture { + pub event: HookEvent, + pub hook_name: String, + pub envs: HashMap, + /// The tool-arguments JSON as handed to dispatch; `None` for events + /// that carry no payload. + pub payload: Option, + /// The working directory the hook would have spawned in. + pub cwd: std::path::PathBuf, + } + + /// Suppresses process spawning and records every dispatch synchronously + /// until the returned guard drops. The capture buffer is process-global + /// and shared by all tests, so count assertions must either use hook + /// names unique to the test fixture and filter captures, or serialize + /// with `#[serial_test::serial]`. Tests that rely on real spawning must + /// also serialize against tests that install the sink. + #[must_use] + pub fn install() -> SinkGuard { + INSTALLED.fetch_add(1, Ordering::SeqCst); + SinkGuard + } + + pub struct SinkGuard; + + impl Drop for SinkGuard { + fn drop(&mut self) { + INSTALLED.fetch_sub(1, Ordering::SeqCst); + } + } + + pub(super) fn record( + event: HookEvent, + hook: &ResolvedHook, + envs: &[(String, String)], + payload: Option<&str>, + ) -> bool { + if INSTALLED.load(Ordering::SeqCst) == 0 { + return false; + } + CAPTURES + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .push(Capture { + event, + hook_name: hook.name.clone(), + envs: envs.iter().cloned().collect(), + payload: payload.map(str::to_string), + cwd: hook.cwd.clone(), + }); + true + } + + /// Removes and returns everything recorded so far. + pub fn drain() -> Vec { + std::mem::take( + &mut *CAPTURES + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()), + ) + } + + /// A copy of everything recorded so far, for tests that filter by a hook + /// name unique to their fixture. + #[allow(dead_code)] + pub fn snapshot() -> Vec { + CAPTURES + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::{AppConfig, AppState, Role, WorkingMode}; + use serial_test::serial; + use std::env; + use std::sync::Arc; + + fn hooks_map(event: &str, defs: &[(&str, &str)]) -> HooksMap { + let defs = defs + .iter() + .map(|(name, command)| HookDef { + name: name.to_string(), + command: command.to_string(), + }) + .collect(); + IndexMap::from([(event.to_string(), defs)]) + } + + fn names(resolved: &[ResolvedHook]) -> Vec<&str> { + resolved.iter().map(|hook| hook.name.as_str()).collect() + } + + fn ctx_with_global_hooks(hooks: HooksMap) -> RequestContext { + let mut app = AppState::test_default(); + app.config = Arc::new(AppConfig { + hooks, + ..Default::default() + }); + RequestContext::new(Arc::new(app), WorkingMode::Cmd) + } + + #[test] + #[serial] + fn install_builtin_hooks_installs_executable_scripts_and_honors_force() { + let env_name = crate::utils::get_env_name("hooks_dir"); + let prev = env::var_os(&env_name); + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let root = env::temp_dir().join(format!("coyote-hooks-install-{unique}")); + unsafe { + env::set_var(&env_name, &root); + } + + // Capture every outcome first and assert only after the env var is + // restored, so a failed assertion cannot leave the hooks-dir override + // pointing at a deleted temp dir for subsequent tests. + let notify = root.join("notify.sh"); + let log_events = root.join("log-events.sh"); + let fresh = install_builtin_hooks(false); + let installed = (notify.is_file(), log_events.is_file()); + #[cfg(unix)] + let modes: Vec> = { + use std::os::unix::fs::PermissionsExt; + [¬ify, &log_events] + .iter() + .map(|path| std::fs::metadata(path).map(|meta| meta.permissions().mode() & 0o777)) + .collect() + }; + let modified = std::fs::write(¬ify, "modified"); + let no_force = install_builtin_hooks(false); + let after_no_force = std::fs::read_to_string(¬ify); + let force = install_builtin_hooks(true); + let after_force = std::fs::read_to_string(¬ify); + + unsafe { + match prev { + Some(v) => env::set_var(&env_name, v), + None => env::remove_var(&env_name), + } + } + let _ = std::fs::remove_dir_all(&root); + + fresh.unwrap(); + assert!(installed.0, "notify.sh must be installed"); + assert!(installed.1, "log-events.sh must be installed"); + #[cfg(unix)] + for (path, mode) in [¬ify, &log_events].into_iter().zip(modes) { + assert_eq!( + mode.unwrap(), + 0o755, + "{} must be executable", + path.display() + ); + } + modified.unwrap(); + no_force.unwrap(); + assert_eq!(after_no_force.unwrap(), "modified"); + force.unwrap(); + assert!(after_force.unwrap().starts_with("#!/usr/bin/env bash")); + } + + /// The example hooks are plain scripts, not argc tools: they live outside + /// `assets/functions/tools/`, so the argc regeneration that runs during + /// tests must never have stamped them with an ARGC-BUILD block. + #[test] + fn builtin_hook_assets_are_not_argc_tools() { + let files: Vec = HookAssets::iter() + .map(|file| file.as_ref().to_string()) + .collect(); + assert!(files.contains(&"notify.sh".to_string()), "{files:?}"); + assert!(files.contains(&"log-events.sh".to_string()), "{files:?}"); + + for file in HookAssets::iter() { + let embedded = HookAssets::get(&file).unwrap(); + let content = std::str::from_utf8(&embedded.data).unwrap(); + assert!( + !content.contains("ARGC-BUILD"), + "{} must not contain an ARGC-BUILD block", + file.as_ref() + ); + assert!( + !content.contains("# @cmd"), + "{} must not use argc annotations", + file.as_ref() + ); + } + } + + #[test] + fn notify_script_prefers_notify_send_then_osascript_then_echo() { + let embedded = HookAssets::get("notify.sh").unwrap(); + let content = std::str::from_utf8(&embedded.data).unwrap(); + let notify_send = content.find("notify-send").expect("notify-send branch"); + let osascript = content.find("osascript").expect("osascript branch"); + let echo = content.find("echo \"[$title]").expect("echo fallback"); + assert!(notify_send < osascript); + assert!(osascript < echo); + } + + #[test] + fn log_events_script_defaults_to_tmp_log_behind_env_override() { + let embedded = HookAssets::get("log-events.sh").unwrap(); + let content = std::str::from_utf8(&embedded.data).unwrap(); + assert!(content.contains("COYOTE_HOOK_LOG")); + assert!(content.contains("/tmp/coyote-hooks.log")); + } + + #[cfg(unix)] + #[test] + fn log_events_script_appends_event_and_env_snapshot() { + let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("assets/hooks/log-events.sh"); + let log = + env::temp_dir().join(format!("coyote-log-events-test-{}.log", std::process::id())); + let _ = std::fs::remove_file(&log); + + let status = std::process::Command::new("bash") + .arg(&script) + .env("COYOTE_HOOK_LOG", &log) + .env("COYOTE_EVENT", "turn.completed") + .env("COYOTE_AGENT_NAME", "demo-agent") + .env("COYOTE_SECRET_DEMO", "hunter2") + .status() + .unwrap(); + + assert!(status.success()); + let logged = std::fs::read_to_string(&log).unwrap(); + let mode = { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(&log).unwrap().permissions().mode() + }; + let _ = std::fs::remove_file(&log); + assert!(logged.contains("turn.completed"), "{logged}"); + assert!(logged.contains("COYOTE_EVENT=turn.completed"), "{logged}"); + assert!(logged.contains("COYOTE_AGENT_NAME=demo-agent"), "{logged}"); + assert!(!logged.contains("COYOTE_SECRET_"), "{logged}"); + assert!(!logged.contains("hunter2"), "{logged}"); + assert_eq!(mode & 0o777, 0o600, "log file should be created 0600"); + } + + #[cfg(unix)] + #[test] + fn notify_script_falls_back_to_echo_without_notifiers() { + let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("assets/hooks/notify.sh"); + // An empty PATH hides notify-send and osascript; echo is a bash + // builtin, so only the fallback branch can produce output. + let mut cmd = std::process::Command::new("/bin/bash"); + cmd.arg(&script) + .env("PATH", "") + .env("COYOTE_EVENT", "turn.completed") + .env("COYOTE_TOOL_NAME", "demo"); + // Detach from any controlling terminal so the script cannot open + // /dev/tty and must fall back to captured stdout. + unsafe { + use std::os::unix::process::CommandExt; + cmd.pre_exec(|| { + libc::setsid(); + Ok(()) + }); + } + let output = cmd.output().unwrap(); + + assert!(output.status.success()); + let stdout = String::from_utf8(output.stdout).unwrap(); + assert!(stdout.contains("turn.completed"), "{stdout}"); + assert!(stdout.contains("tool=demo"), "{stdout}"); + } + + #[test] + fn hook_event_names_are_dotted() { + let cases = [ + (HookEvent::TurnStarted, "turn.started"), + (HookEvent::TurnCompleted, "turn.completed"), + (HookEvent::TurnInterrupted, "turn.interrupted"), + (HookEvent::TurnFailed, "turn.failed"), + (HookEvent::SessionStarted, "session.started"), + (HookEvent::SessionEnded, "session.ended"), + (HookEvent::SessionCompressed, "session.compressed"), + (HookEvent::ToolStarted, "tool.started"), + (HookEvent::ToolCompleted, "tool.completed"), + (HookEvent::ToolFailed, "tool.failed"), + (HookEvent::LlmRequestStarted, "llm.request.started"), + (HookEvent::LlmRequestCompleted, "llm.request.completed"), + (HookEvent::LlmRequestFailed, "llm.request.failed"), + (HookEvent::AgentStarted, "agent.started"), + (HookEvent::AgentCompleted, "agent.completed"), + (HookEvent::AgentFailed, "agent.failed"), + (HookEvent::GraphNodeStarted, "graph.node.started"), + (HookEvent::GraphNodeCompleted, "graph.node.completed"), + (HookEvent::GraphNodeFailed, "graph.node.failed"), + (HookEvent::JobStarted, "job.started"), + (HookEvent::JobCompleted, "job.completed"), + (HookEvent::JobFailed, "job.failed"), + ]; + for (event, name) in cases { + assert_eq!(event.as_str(), name); + } + } + + #[test] + fn root_context_gets_all_global_hooks() { + let mut global = hooks_map("turn.started", &[("a", "cmd-a"), ("b", "cmd-b")]); + global.extend(hooks_map("turn.completed", &[("other", "cmd-other")])); + + let resolved = resolve_hooks(HookEvent::TurnStarted, &global, None, None, None); + + assert_eq!(names(&resolved), ["a", "b"]); + assert_eq!(resolved[0].full_name, "turn.started.a"); + } + + #[test] + fn agent_without_whitelist_gets_no_global_hooks() { + let global = hooks_map("turn.started", &[("a", "cmd-a")]); + let agent = hooks_map("turn.started", &[("own", "agent-cmd")]); + + let resolved = resolve_hooks( + HookEvent::TurnStarted, + &global, + Some((&[], "plain-agent")), + None, + Some((&agent, "plain-agent")), + ); + + assert_eq!(names(&resolved), ["own"]); + } + + #[test] + fn whitelist_admits_only_exact_global_entries() { + let global = hooks_map( + "turn.started", + &[("a", "cmd-a"), ("b", "cmd-b"), ("c", "cmd-c")], + ); + let empty = HooksMap::default(); + let gate = vec![ + "turn.started.b".to_string(), + "turn.started.missing".to_string(), + "other.event.x".to_string(), + ]; + + let resolved = resolve_hooks( + HookEvent::TurnStarted, + &global, + Some((&gate, "gated-agent")), + None, + Some((&empty, "gated-agent")), + ); + + assert_eq!(names(&resolved), ["b"]); + } + + #[test] + fn unknown_whitelist_entries_log_debug_and_stay_inert() { + crate::testing::install_warn_collector(); + let global = hooks_map("turn.started", &[("a", "cmd-a")]); + let gate = vec![ + "turn.started.a".to_string(), + "turn.started.nonexistent-marker-xyz".to_string(), + ]; + + let resolved = resolve_hooks( + HookEvent::TurnStarted, + &global, + Some((&gate, "gate-agent-xyz")), + None, + None, + ); + + assert_eq!(names(&resolved), ["a"]); + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!(debugs.iter().any(|message| { + message.contains("turn.started.nonexistent-marker-xyz") + && message.contains("for agent 'gate-agent-xyz'") + })); + } + + #[test] + fn empty_global_map_short_circuits_whitelist_diagnostics() { + crate::testing::install_warn_collector(); + let global = HooksMap::default(); + let role = hooks_map("turn.started", &[("role-only", "role-cmd")]); + let gate = vec!["turn.started.x-shortcircuit-marker-p3q".to_string()]; + + let resolved = resolve_hooks( + HookEvent::TurnStarted, + &global, + Some((&gate, "short-circuit-agent")), + Some(&role), + None, + ); + + assert_eq!(names(&resolved), ["role-only"]); + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!( + debugs + .iter() + .all(|message| !message.contains("shortcircuit-marker-p3q")) + ); + } + + #[test] + fn role_hooks_are_always_additive() { + let global = HooksMap::default(); + let role = hooks_map("session.ended", &[("archive", "role-cmd")]); + let empty = HooksMap::default(); + + let gated = resolve_hooks( + HookEvent::SessionEnded, + &global, + Some((&[], "gated-agent")), + Some(&role), + Some((&empty, "gated-agent")), + ); + assert_eq!(names(&gated), ["archive"]); + + let root = resolve_hooks(HookEvent::SessionEnded, &global, None, Some(&role), None); + assert_eq!(names(&root), ["archive"]); + } + + #[test] + #[serial] + fn scopes_resolve_in_global_role_agent_order() { + let global = hooks_map("tool.completed", &[("notify", "global-cmd")]); + let role = hooks_map("tool.completed", &[("notify", "role-cmd")]); + let agent = hooks_map("tool.completed", &[("notify", "agent-cmd")]); + let gate = vec!["tool.completed.notify".to_string()]; + + let resolved = resolve_hooks( + HookEvent::ToolCompleted, + &global, + Some((&gate, "order-agent")), + Some(&role), + Some((&agent, "order-agent")), + ); + + let commands: Vec<&str> = resolved.iter().map(|hook| hook.command.as_str()).collect(); + assert_eq!(commands, ["global-cmd", "role-cmd", "agent-cmd"]); + assert!( + resolved + .iter() + .all(|hook| hook.full_name == "tool.completed.notify") + ); + assert_eq!(resolved[0].cwd, paths::config_dir()); + assert_eq!(resolved[1].cwd, paths::roles_dir()); + assert_eq!(resolved[2].cwd, paths::agent_data_dir("order-agent")); + } + + #[test] + fn resolved_hooks_reads_global_config_from_context() { + let ctx = ctx_with_global_hooks(hooks_map("session.started", &[("smoke", "true")])); + + let resolved = ctx.resolved_hooks(HookEvent::SessionStarted); + + assert_eq!(names(&resolved), ["smoke"]); + assert_eq!(resolved[0].full_name, "session.started.smoke"); + assert!(ctx.resolved_hooks(HookEvent::SessionEnded).is_empty()); + } + + #[test] + #[serial] + fn empty_command_is_skipped_without_error() { + let _guard = test_sink::install(); + let cwd = env::temp_dir(); + let hooks = vec![ + ResolvedHook { + name: "gap".to_string(), + full_name: "turn.failed.gap".to_string(), + command: " ".to_string(), + cwd: cwd.clone(), + }, + ResolvedHook { + name: "after-gap".to_string(), + full_name: "turn.failed.after-gap".to_string(), + command: "true".to_string(), + cwd, + }, + ]; + + fire_resolved(HookEvent::TurnFailed, hooks, Vec::new(), &[], None); + + let captures = test_sink::drain(); + assert!(captures.iter().all(|capture| capture.hook_name != "gap")); + assert!( + captures + .iter() + .any(|capture| capture.hook_name == "after-gap") + ); + } + + #[test] + #[serial] + fn sink_captures_synchronously_and_suppresses_spawn() { + let marker = + env::temp_dir().join(format!("coyote-hook-sink-marker-{}", std::process::id())); + let _ = std::fs::remove_file(&marker); + let touch = format!("touch \"{}\"", marker.display()); + let ctx = ctx_with_global_hooks(hooks_map("job.completed", &[("sink-only", &touch)])); + + let _guard = test_sink::install(); + fire( + HookEvent::JobCompleted, + &ctx, + &[("HOOK_EXTRA", "extra-value".to_string())], + None, + ); + + let captures = test_sink::drain(); + let capture = captures + .iter() + .find(|capture| capture.hook_name == "sink-only") + .expect("sink must record synchronously before fire returns"); + assert_eq!(capture.event, HookEvent::JobCompleted); + assert_eq!( + capture.envs.get("COYOTE_EVENT").map(String::as_str), + Some("job.completed") + ); + assert_eq!( + capture.envs.get("COYOTE_HOOK_NAME").map(String::as_str), + Some("sink-only") + ); + assert!(capture.envs.contains_key("COYOTE_CONFIG_DIR")); + assert!(capture.envs.contains_key("COYOTE_EVENT_TIMESTAMP")); + assert_eq!( + capture.envs.get("HOOK_EXTRA").map(String::as_str), + Some("extra-value") + ); + assert!( + !marker.exists(), + "no process may spawn while the sink is installed" + ); + } + + #[test] + #[serial] + fn fire_resolved_truncates_every_env_value() { + let _guard = test_sink::install(); + let oversized = "x".repeat(3000); + let hook = ResolvedHook { + name: "truncate-probe".to_string(), + full_name: "turn.failed.truncate-probe".to_string(), + command: "true".to_string(), + cwd: env::temp_dir(), + }; + + fire_resolved( + HookEvent::TurnFailed, + vec![hook], + vec![("COYOTE_BIG_BASE".to_string(), oversized.clone())], + &[("COYOTE_BIG_EXTRA", oversized)], + None, + ); + + let captures = test_sink::drain(); + let capture = captures + .iter() + .find(|capture| capture.hook_name == "truncate-probe") + .expect("sink must record the dispatch"); + assert_eq!(capture.envs.get("COYOTE_BIG_BASE").unwrap().len(), 2048); + assert_eq!(capture.envs.get("COYOTE_BIG_EXTRA").unwrap().len(), 2048); + } + + #[test] + #[serial] + fn sink_recovers_after_capture_buffer_poisoning() { + let _guard = test_sink::install(); + test_sink::drain(); + std::thread::spawn(|| { + let _held = test_sink::CAPTURES.lock().unwrap(); + panic!("poison the capture buffer"); + }) + .join() + .unwrap_err(); + + let hook = ResolvedHook { + name: "poison-recovery-marker-j8r".to_string(), + full_name: "turn.failed.poison-recovery-marker-j8r".to_string(), + command: "true".to_string(), + cwd: env::temp_dir(), + }; + assert!(test_sink::record(HookEvent::TurnFailed, &hook, &[], None)); + + let captures = test_sink::drain(); + assert!( + captures + .iter() + .any(|capture| capture.hook_name == "poison-recovery-marker-j8r") + ); + } + + #[test] + fn base_envs_parts_reflects_session_and_agent() { + let envs = base_envs_parts(HookEvent::TurnStarted, Some("sess-1"), Some("helper")); + let find = |key: &str| { + envs.iter() + .find(|(k, _)| k == key) + .map(|(_, value)| value.as_str()) + }; + assert_eq!(find("COYOTE_SESSION_ID"), Some("sess-1")); + assert_eq!(find("COYOTE_AGENT_NAME"), Some("helper")); + let timestamp = find("COYOTE_EVENT_TIMESTAMP").expect("timestamp env"); + let parsed = chrono::DateTime::parse_from_rfc3339(timestamp).unwrap(); + assert_eq!(parsed.offset().local_minus_utc(), 0); + + let envs = base_envs_parts(HookEvent::TurnStarted, None, None); + assert!( + envs.iter() + .all(|(key, _)| key != "COYOTE_SESSION_ID" && key != "COYOTE_AGENT_NAME") + ); + } + + #[tokio::test] + #[serial] + async fn drain_pending_returns_immediately_when_idle() { + let start = std::time::Instant::now(); + drain_pending(Duration::from_secs(5)).await; + assert!(start.elapsed() < Duration::from_secs(1)); + } + + #[tokio::test(start_paused = true)] + #[serial] + async fn drain_pending_logs_when_the_timeout_expires() { + crate::testing::install_warn_collector(); + let pending = SpawnAckGuard::register(); + + drain_pending(Duration::from_millis(50)).await; + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!( + debugs + .iter() + .any(|message| message.contains("Gave up waiting")) + ); + drop(pending); + } + + #[test] + fn truncate_env_value_keeps_short_values() { + assert_eq!(truncate_env_value("short"), "short"); + let exact = "a".repeat(2048); + assert_eq!(truncate_env_value(&exact), exact); + } + + #[test] + fn truncate_env_value_respects_char_boundaries() { + let value = format!("{}€", "a".repeat(2047)); + let truncated = truncate_env_value(&value); + assert_eq!(truncated.len(), 2047); + assert!(truncated.chars().all(|c| c == 'a')); + } + + #[test] + #[serial] + fn fire_resolved_without_runtime_skips_instead_of_panicking() { + crate::testing::install_warn_collector(); + let hook = ResolvedHook { + name: "no-runtime-marker-a7c".to_string(), + full_name: "turn.failed.no-runtime-marker-a7c".to_string(), + command: "true".to_string(), + cwd: env::temp_dir(), + }; + + fire_resolved(HookEvent::TurnFailed, vec![hook], Vec::new(), &[], None); + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!(debugs.iter().any(|message| { + message.contains("no-runtime-marker-a7c") + && message.contains("no Tokio runtime on this thread") + })); + } + + #[test] + #[serial] + fn fire_without_runtime_skips_instead_of_panicking() { + crate::testing::install_warn_collector(); + let ctx = ctx_with_global_hooks(hooks_map( + "turn.failed", + &[("fire-no-runtime-marker-b8d", "true")], + )); + + fire(HookEvent::TurnFailed, &ctx, &[], None); + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!(debugs.iter().any(|message| { + message.contains("fire-no-runtime-marker-b8d") + && message.contains("no Tokio runtime on this thread") + })); + } + + #[test] + fn debug_capture_excludes_non_hooks_targets() { + crate::testing::install_warn_collector(); + let hook = ResolvedHook { + name: "included-marker-k2v".to_string(), + full_name: "turn.failed.included-marker-k2v".to_string(), + command: " ".to_string(), + cwd: env::temp_dir(), + }; + + log::debug!( + target: concat!(env!("CARGO_CRATE_NAME"), "::not_hooks_marker_k2v"), + "excluded-marker-k2v" + ); + log::debug!( + target: concat!(env!("CARGO_CRATE_NAME"), "::hooksbogus_marker_k2v"), + "boundary-excluded-marker-k2v" + ); + fire_resolved(HookEvent::TurnFailed, vec![hook], Vec::new(), &[], None); + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!( + debugs + .iter() + .all(|message| !message.contains("excluded-marker-k2v")) + ); + assert!( + debugs + .iter() + .all(|message| !message.contains("boundary-excluded-marker-k2v")) + ); + assert!( + debugs + .iter() + .any(|message| message.contains("included-marker-k2v")) + ); + } + + #[test] + #[serial] + fn debug_capture_recovers_after_buffer_poisoning() { + crate::testing::install_warn_collector(); + std::thread::spawn(|| { + let _held = crate::testing::debug_messages().lock().unwrap(); + panic!("poison the debug buffer"); + }) + .join() + .unwrap_err(); + + let hook = ResolvedHook { + name: "poisoned-buffer-marker-w4t".to_string(), + full_name: "turn.failed.poisoned-buffer-marker-w4t".to_string(), + command: " ".to_string(), + cwd: env::temp_dir(), + }; + fire_resolved(HookEvent::TurnFailed, vec![hook], Vec::new(), &[], None); + + let mut debugs = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + assert!(debugs.iter().any(|message| { + message.contains("poisoned-buffer-marker-w4t") && message.contains("empty command") + })); + debugs.retain(|message| !message.contains("poisoned-buffer-marker-w4t")); + } + + #[test] + #[serial] + fn resolved_hooks_adds_role_hooks_through_context() { + let mut ctx = ctx_with_global_hooks(hooks_map( + "turn.completed", + &[("global-probe", "global-cmd")], + )); + let content = "---\nhooks:\n turn.completed:\n - name: role-probe\n command: role-cmd\n---\nPrompt"; + ctx.role = Some(Role::new("test", content)); + + let resolved = ctx.resolved_hooks(HookEvent::TurnCompleted); + + assert_eq!(names(&resolved), ["global-probe", "role-probe"]); + assert_eq!(resolved[1].command, "role-cmd"); + assert_eq!(resolved[1].cwd, paths::roles_dir()); + } + + #[cfg(unix)] + mod dispatch { + use super::*; + use crate::utils::get_env_name; + use std::fs::{create_dir_all, remove_dir_all}; + use std::path::PathBuf; + use std::time::{Duration, SystemTime, UNIX_EPOCH}; + + struct TestConfigDirGuard { + key: String, + previous: Option, + path: PathBuf, + } + + impl TestConfigDirGuard { + fn new() -> Self { + let key = get_env_name("config_dir"); + let previous = env::var_os(&key); + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let path = env::temp_dir().join(format!("coyote-hooks-tests-{unique}")); + create_dir_all(&path).unwrap(); + unsafe { + env::set_var(&key, &path); + } + Self { + key, + previous, + path, + } + } + } + + impl Drop for TestConfigDirGuard { + fn drop(&mut self) { + if let Some(previous) = &self.previous { + unsafe { + env::set_var(&self.key, previous); + } + } else { + unsafe { + env::remove_var(&self.key); + } + } + let _ = remove_dir_all(&self.path); + } + } + + /// Points `run_hook`'s payload directory at `path` for the guard's + /// lifetime, so payload writes can be aimed at a nonexistent + /// directory without touching the process-global `TMPDIR`. + struct PayloadDirOverrideGuard; + + impl PayloadDirOverrideGuard { + fn new(path: PathBuf) -> Self { + *PAYLOAD_DIR_OVERRIDE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = Some(path); + Self + } + } + + impl Drop for PayloadDirOverrideGuard { + fn drop(&mut self) { + *PAYLOAD_DIR_OVERRIDE + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = None; + } + } + + async fn wait_for(what: &str, cond: impl Fn() -> bool) { + for _ in 0..400 { + if cond() { + return; + } + tokio::time::sleep(Duration::from_millis(25)).await; + } + panic!("timed out waiting for {what}"); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn fire_spawns_detached_with_envs() { + let guard = TestConfigDirGuard::new(); + let out = guard.path.join("env-out"); + let gate = guard.path.join("gate"); + let done = guard.path.join("done"); + let command = r#"env > "$HOOK_OUT.tmp" && mv "$HOOK_OUT.tmp" "$HOOK_OUT"; until [ -e "$HOOK_GATE" ]; do sleep 0.05; done; : > "$HOOK_DONE""#; + let ctx = ctx_with_global_hooks(hooks_map("turn.completed", &[("envdump", command)])); + + fire( + HookEvent::TurnCompleted, + &ctx, + &[ + ("HOOK_OUT", out.display().to_string()), + ("HOOK_GATE", gate.display().to_string()), + ("HOOK_DONE", done.display().to_string()), + ], + None, + ); + + wait_for("hook env dump", || out.exists()).await; + assert!( + !done.exists(), + "dispatch must return before the hook completes" + ); + + let env_dump = std::fs::read_to_string(&out).unwrap(); + let lines: Vec<&str> = env_dump.lines().collect(); + assert!(lines.contains(&"COYOTE_EVENT=turn.completed")); + assert!(lines.contains(&"COYOTE_HOOK_NAME=envdump")); + let config_dir_line = format!("COYOTE_CONFIG_DIR={}", guard.path.display()); + assert!(lines.contains(&config_dir_line.as_str())); + assert!( + lines + .iter() + .any(|line| line.starts_with("COYOTE_EVENT_TIMESTAMP=")) + ); + let gate_line = format!("HOOK_GATE={}", gate.display()); + assert!(lines.contains(&gate_line.as_str())); + + std::fs::write(&gate, "").unwrap(); + wait_for("hook completion", || done.exists()).await; + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn hook_stdin_is_null_not_inherited() { + let guard = TestConfigDirGuard::new(); + let out = guard.path.join("stdin-out"); + let command = r#"if read -t 5 line; then echo open > "$HOOK_OUT.tmp"; else echo eof > "$HOOK_OUT.tmp"; fi; mv "$HOOK_OUT.tmp" "$HOOK_OUT""#; + let ctx = ctx_with_global_hooks(hooks_map("turn.started", &[("stdin-probe", command)])); + let start = std::time::Instant::now(); + + fire( + HookEvent::TurnStarted, + &ctx, + &[("HOOK_OUT", out.display().to_string())], + None, + ); + + wait_for("stdin probe output", || out.exists()).await; + assert_eq!(std::fs::read_to_string(&out).unwrap().trim(), "eof"); + assert!( + start.elapsed() < Duration::from_secs(5), + "read must hit EOF immediately, not time out" + ); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn payload_file_is_written_and_removed() { + let guard = TestConfigDirGuard::new(); + let out = guard.path.join("payload-out"); + let path_out = guard.path.join("payload-path"); + let command = r#"cp "$COYOTE_HOOK_PAYLOAD_FILE" "$HOOK_OUT.tmp" && mv "$HOOK_OUT.tmp" "$HOOK_OUT"; printf '%s' "$COYOTE_HOOK_PAYLOAD_FILE" > "$HOOK_PATH.tmp" && mv "$HOOK_PATH.tmp" "$HOOK_PATH""#; + let ctx = ctx_with_global_hooks(hooks_map("tool.started", &[("payload", command)])); + let payload = r#"{"path":"/tmp/target","recursive":true}"#; + + fire( + HookEvent::ToolStarted, + &ctx, + &[ + ("HOOK_OUT", out.display().to_string()), + ("HOOK_PATH", path_out.display().to_string()), + ], + Some(payload.to_string()), + ); + + wait_for("payload copy", || out.exists() && path_out.exists()).await; + assert_eq!(std::fs::read_to_string(&out).unwrap(), payload); + + let payload_file = PathBuf::from(std::fs::read_to_string(&path_out).unwrap()); + let file_name = payload_file.file_name().unwrap().to_string_lossy(); + assert!(file_name.starts_with("coyote-hook-tool.started-")); + wait_for("payload file removal", || !payload_file.exists()).await; + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn drain_pending_waits_for_spawn_not_completion() { + let guard = TestConfigDirGuard::new(); + let out = guard.path.join("spawned"); + let gate = guard.path.join("gate"); + let done = guard.path.join("done"); + let command = r#": > "$HOOK_OUT"; until [ -e "$HOOK_GATE" ]; do sleep 0.05; done; : > "$HOOK_DONE""#; + let ctx = + ctx_with_global_hooks(hooks_map("session.ended", &[("drain-probe", command)])); + + fire( + HookEvent::SessionEnded, + &ctx, + &[ + ("HOOK_OUT", out.display().to_string()), + ("HOOK_GATE", gate.display().to_string()), + ("HOOK_DONE", done.display().to_string()), + ], + None, + ); + + drain_pending(Duration::from_secs(10)).await; + + assert!(!done.exists(), "drain must not wait for hook completion"); + wait_for("hook start marker", || out.exists()).await; + assert!(!done.exists()); + + std::fs::write(&gate, "").unwrap(); + wait_for("hook completion", || done.exists()).await; + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn error_paths_log_debug_only() { + crate::testing::install_warn_collector(); + let empty_cwd = env::temp_dir(); + let hooks = vec![ + ResolvedHook { + name: "empty-marker-f5b".to_string(), + full_name: "turn.failed.empty-marker-f5b".to_string(), + command: " ".to_string(), + cwd: empty_cwd.clone(), + }, + ResolvedHook { + name: "badcwd-marker-f5b".to_string(), + full_name: "turn.failed.badcwd-marker-f5b".to_string(), + command: "true".to_string(), + cwd: PathBuf::from("/nonexistent/coyote-badcwd-marker-f5b"), + }, + ]; + + fire_resolved(HookEvent::TurnFailed, hooks, Vec::new(), &[], None); + drain_pending(Duration::from_secs(10)).await; + + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + let empty_cwd_display = empty_cwd.display().to_string(); + assert!(debugs.iter().any(|message| { + message.contains("empty-marker-f5b") + && message.contains("empty command") + && message.contains(empty_cwd_display.as_str()) + })); + assert!(debugs.iter().any(|message| { + message.contains("Failed to spawn hook 'turn.failed.badcwd-marker-f5b'") + && message.contains("/nonexistent/coyote-badcwd-marker-f5b") + })); + let warns: Vec = crate::testing::warn_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!(warns.iter().all(|message| !message.contains("marker-f5b"))); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn failing_hook_never_disturbs_the_engine() { + crate::testing::install_warn_collector(); + let guard = TestConfigDirGuard::new(); + let first = guard.path.join("first-out"); + let second = guard.path.join("second-out"); + let command = r#"echo err >&2; : > "$HOOK_OUT"; exit 1"#; + let ctx = ctx_with_global_hooks(hooks_map( + "tool.failed", + &[("stderr-exit1-marker", command)], + )); + + fire( + HookEvent::ToolFailed, + &ctx, + &[("HOOK_OUT", first.display().to_string())], + None, + ); + wait_for("first hook output", || first.exists()).await; + + fire( + HookEvent::ToolFailed, + &ctx, + &[("HOOK_OUT", second.display().to_string())], + None, + ); + wait_for("second hook output", || second.exists()).await; + + let warns: Vec = crate::testing::warn_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!( + warns + .iter() + .all(|message| !message.contains("stderr-exit1-marker")) + ); + } + + #[tokio::test] + #[serial] + async fn payload_file_is_created_with_owner_only_mode() { + use std::os::unix::fs::PermissionsExt; + let guard = TestConfigDirGuard::new(); + let path = guard.path.join("payload.json"); + + write_payload_file(&path, r#"{"probe":true}"#) + .await + .unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "payload file mode was {mode:o}"); + } + + #[tokio::test] + #[serial] + async fn payload_file_write_refuses_existing_paths() { + let guard = TestConfigDirGuard::new(); + + let plain = guard.path.join("existing.json"); + std::fs::write(&plain, "original").unwrap(); + assert!(write_payload_file(&plain, "{}").await.is_err()); + assert_eq!(std::fs::read_to_string(&plain).unwrap(), "original"); + + let target = guard.path.join("symlink-target"); + std::fs::write(&target, "original").unwrap(); + let link = guard.path.join("existing-link.json"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + assert!(write_payload_file(&link, "{}").await.is_err()); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "original"); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn payload_write_failure_still_runs_hook_without_payload_env() { + crate::testing::install_warn_collector(); + let guard = TestConfigDirGuard::new(); + let out = guard.path.join("env-out"); + let _payload_dir = PayloadDirOverrideGuard::new(guard.path.join("missing-payload-dir")); + let command = r#"env > "$HOOK_OUT.tmp" && mv "$HOOK_OUT.tmp" "$HOOK_OUT""#; + let ctx = ctx_with_global_hooks(hooks_map( + "tool.started", + &[("payload-fallback-marker", command)], + )); + + fire( + HookEvent::ToolStarted, + &ctx, + &[("HOOK_OUT", out.display().to_string())], + Some(r#"{"probe":true}"#.to_string()), + ); + + wait_for("hook env dump", || out.exists()).await; + let env_dump = std::fs::read_to_string(&out).unwrap(); + assert!( + env_dump + .lines() + .all(|line| !line.starts_with("COYOTE_HOOK_PAYLOAD_FILE=")) + ); + let debugs: Vec = crate::testing::debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + assert!(debugs.iter().any(|message| message.contains( + "Failed to write payload file for hook 'tool.started.payload-fallback-marker'" + ))); + } + + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn payload_paths_differ_across_dispatches() { + let guard = TestConfigDirGuard::new(); + let first = guard.path.join("first-path"); + let second = guard.path.join("second-path"); + let command = r#"printf '%s' "$COYOTE_HOOK_PAYLOAD_FILE" > "$HOOK_PATH.tmp" && mv "$HOOK_PATH.tmp" "$HOOK_PATH""#; + let ctx = ctx_with_global_hooks(hooks_map("tool.started", &[("nonce-probe", command)])); + + fire( + HookEvent::ToolStarted, + &ctx, + &[("HOOK_PATH", first.display().to_string())], + Some("{}".to_string()), + ); + wait_for("first payload path", || first.exists()).await; + + fire( + HookEvent::ToolStarted, + &ctx, + &[("HOOK_PATH", second.display().to_string())], + Some("{}".to_string()), + ); + wait_for("second payload path", || second.exists()).await; + + let first_path = std::fs::read_to_string(&first).unwrap(); + let second_path = std::fs::read_to_string(&second).unwrap(); + assert!(!first_path.is_empty()); + assert_ne!(first_path, second_path); + } + + #[tokio::test] + #[serial] + async fn resolved_hooks_gates_globals_and_adds_agent_hooks_through_context() { + let _guard = TestConfigDirGuard::new(); + let mut ctx = + ctx_with_global_hooks(hooks_map("tool.started", &[("notify", "global-cmd")])); + let app = ctx.app.config.clone(); + let agent_name = "hooks-gate-probe"; + let agent_dir = paths::agent_data_dir(agent_name); + create_dir_all(&agent_dir).unwrap(); + std::fs::write( + agent_dir.join("config.yaml"), + format!( + "name: {agent_name}\ninstructions: hi\nhooks:\n tool.started:\n - name: own\n command: agent-cmd\nglobal_hooks: []\n" + ), + ) + .unwrap(); + + ctx.use_agent(&app, agent_name, None, crate::utils::create_abort_signal()) + .await + .unwrap(); + + let resolved = ctx.resolved_hooks(HookEvent::ToolStarted); + assert_eq!(names(&resolved), ["own"]); + assert_eq!(resolved[0].command, "agent-cmd"); + assert_eq!(resolved[0].cwd, paths::agent_data_dir(agent_name)); + } + } +} diff --git a/src/main.rs b/src/main.rs index 31f81d8..52ff77a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,6 +4,7 @@ mod client; mod config; mod function; mod graph; +mod hooks; mod rag; mod render; mod repl; @@ -22,7 +23,8 @@ extern crate log; use crate::cli::Cli; use crate::client::{ - ModelType, call_chat_completions, call_chat_completions_streaming, catalog, list_models, oauth, + Client, ModelType, call_chat_completions, call_chat_completions_streaming, catalog, + list_models, oauth, }; use crate::config::instructions::WORKSPACE_INSTRUCTIONS_FILE_NAME; use crate::config::{ @@ -35,7 +37,7 @@ use crate::config::{memory, paths}; use crate::function::agents::{GuardrailAction, check_pending_tasks_guardrail}; use crate::mcp::McpServersConfig; use crate::render::{prompt_theme, render_error}; -use crate::repl::Repl; +use crate::repl::{EXIT_HOOK_DRAIN_TIMEOUT, Repl, TurnBracket}; use crate::utils::*; use crate::vault::{Vault, interpolate_secrets}; use anyhow::{Context, Result, anyhow, bail}; @@ -386,6 +388,12 @@ async fn run( let app = Arc::clone(&ctx.app.config); ctx.use_agent(app.as_ref(), agent, session, abort_signal.clone()) .await?; + // The top-level agent bracket does NOT open here: the inspection + // flags below (--list-sessions, --info, --macro, --rebuild-rag, + // --init-*) return early without running the agent, and an open + // bracket on those paths would leak `agent.started` with no + // terminal event. It opens at the dispatch sites further down, + // where an agent run is actually committed. } else { let app: Arc = Arc::clone(&ctx.app.config); if let Some(prompt) = &cli.temp_role { @@ -522,9 +530,17 @@ async fn run( return Ok(()); } if cli.execute && !is_repl { - let input = create_input(&ctx, text, &cli.file, abort_signal.clone()).await?; - shell_execute(&mut ctx, &SHELL, input, abort_signal.clone()).await?; - return Ok(()); + // Dispatch committed: open the top-level agent bracket only now, + // past every inspection-flag early return above. + ctx.top_level_agent_started(); + let result = async { + let input = create_input(&ctx, text, &cli.file, abort_signal.clone()).await?; + shell_execute(&mut ctx, &SHELL, input, abort_signal.clone()).await + } + .await; + ctx.top_level_agent_finished(result.as_ref().err()); + hooks::drain_pending(EXIT_HOOK_DRAIN_TIMEOUT).await; + return result; } { @@ -542,11 +558,23 @@ async fn run( return acp::run_acp_server(ctx, abort_signal).await; } + // Dispatch committed (headless directive or REPL): open the top-level + // agent bracket only now, past every inspection-flag early return + // above. The ACP server stays unbracketed deliberately. The headless + // arm closes the bracket below; the REPL closes it at teardown (or on + // an interactive agent switch). + ctx.top_level_agent_started(); match is_repl { false => { - let mut input = create_input(&ctx, text, &cli.file, abort_signal.clone()).await?; - input.use_embeddings(abort_signal.clone()).await?; - start_directive(&mut ctx, input, cli.code, abort_signal).await + let result = async { + let mut input = create_input(&ctx, text, &cli.file, abort_signal.clone()).await?; + input.use_embeddings(abort_signal.clone()).await?; + start_directive(&mut ctx, input, cli.code, abort_signal.clone()).await + } + .await; + ctx.top_level_agent_finished(result.as_ref().err()); + hooks::drain_pending(EXIT_HOOK_DRAIN_TIMEOUT).await; + result } true => { if !*IS_STDOUT_TERMINAL { @@ -563,6 +591,17 @@ async fn start_directive( input: Input, code_mode: bool, abort_signal: AbortSignal, +) -> Result<()> { + let bracket = TurnBracket::enter(ctx); + let result = start_directive_inner(ctx, input, code_mode, abort_signal.clone()).await; + bracket.finish(ctx, &abort_signal, result) +} + +async fn start_directive_inner( + ctx: &mut RequestContext, + input: Input, + code_mode: bool, + abort_signal: AbortSignal, ) -> Result<()> { let app: Arc = Arc::clone(&ctx.app.config); ctx.session_abort = Some(abort_signal.clone()); @@ -638,22 +677,10 @@ async fn shell_execute( abort_signal: AbortSignal, ) -> Result<()> { let app: Arc = Arc::clone(&ctx.app.config); - let client = input.create_client()?; - ctx.before_chat_completion(&input)?; - let (eval_str, _) = call_chat_completions( - &input, - false, - true, - client.as_ref(), - ctx, - abort_signal.clone(), - ) - .await?; - - ctx.after_chat_completion(app.as_ref(), &input, &eval_str, &[])?; - if eval_str.is_empty() { - bail!("No command generated"); - } + // Each revision round is its own turn: the recursive 'r' call re-enters. + let bracket = TurnBracket::enter(ctx); + let result = shell_execute_turn(ctx, &input, abort_signal.clone()).await; + let (client, eval_str) = bracket.finish(ctx, &abort_signal, result)?; if app.dry_run { app.print_markdown(&eval_str)?; return Ok(()); @@ -679,6 +706,8 @@ async fn shell_execute( if code == 0 && app.save_shell_history { let _ = append_to_shell_history(&shell.name, &eval_str, code); } + ctx.top_level_agent_finished(None); + hooks::drain_pending(EXIT_HOOK_DRAIN_TIMEOUT).await; process::exit(code); } 'r' => { @@ -726,6 +755,24 @@ async fn shell_execute( Ok(()) } +async fn shell_execute_turn( + ctx: &mut RequestContext, + input: &Input, + abort_signal: AbortSignal, +) -> Result<(Box, String)> { + let app: Arc = Arc::clone(&ctx.app.config); + let client = input.create_client()?; + ctx.before_chat_completion(input)?; + let (eval_str, _) = + call_chat_completions(input, false, true, client.as_ref(), ctx, abort_signal).await?; + + ctx.after_chat_completion(app.as_ref(), input, &eval_str, &[])?; + if eval_str.is_empty() { + bail!("No command generated"); + } + Ok((client, eval_str)) +} + async fn create_input( ctx: &RequestContext, text: Option, diff --git a/src/repl/mod.rs b/src/repl/mod.rs index f0b41a2..00e8edc 100644 --- a/src/repl/mod.rs +++ b/src/repl/mod.rs @@ -17,6 +17,7 @@ use crate::config::{ }; use crate::config::{AssetCategory, paths}; use crate::function::agents::{GuardrailAction, check_pending_tasks_guardrail}; +use crate::hooks::{self, HookEvent}; use crate::render::render_error; use crate::utils::{ AbortSignal, SHELL, abortable_run_with_spinner, create_abort_signal, dimmed_text, @@ -41,11 +42,58 @@ use reedline::{ use reedline::{MenuBuilder, Signal}; use std::collections::HashMap; use std::sync::LazyLock; +use std::time::Duration; use std::{env, process, sync::Arc}; use tokio::task; const MENU_NAME: &str = "completion_menu"; +/// Bounded wait applied at process-exit seams (REPL teardown, headless and +/// shell-execute exits) so already-fired exit hooks — `session.ended`, the +/// final `turn.*` / `agent.*` events — finish spawning before the process +/// tears down. Never used mid-session. +pub(crate) const EXIT_HOOK_DRAIN_TIMEOUT: Duration = Duration::from_secs(2); + +/// Brackets one user-visible turn at an outer boundary (`ask`, +/// `start_directive`, `shell_execute`): `enter` fires `turn.started`, and +/// `finish` fires exactly one of `turn.completed` / `turn.interrupted` / +/// `turn.failed`. The interrupted-vs-failed discrimination lives here and +/// nowhere else: a ctrl-c abort wins over the result, then an error means +/// failed (carrying `COYOTE_ERROR`), otherwise completed. Tool rounds, +/// auto-continue, and compression retries run inside one bracket and never +/// re-fire turn events. +pub(crate) struct TurnBracket(()); + +impl TurnBracket { + #[must_use] + pub(crate) fn enter(ctx: &RequestContext) -> Self { + hooks::fire(HookEvent::TurnStarted, ctx, &[], None); + Self(()) + } + + pub(crate) fn finish( + self, + ctx: &RequestContext, + abort_signal: &AbortSignal, + result: Result, + ) -> Result { + if abort_signal.aborted_ctrlc() { + hooks::fire(HookEvent::TurnInterrupted, ctx, &[], None); + } else { + match &result { + Ok(_) => hooks::fire(HookEvent::TurnCompleted, ctx, &[], None), + Err(err) => hooks::fire( + HookEvent::TurnFailed, + ctx, + &[("COYOTE_ERROR", format!("{err:#}"))], + None, + ), + } + } + result + } +} + pub const DEFAULT_CONTINUATION_PROMPT: &str = indoc! {" [SYSTEM REMINDER - TODO CONTINUATION] You have incomplete tasks. Rules: @@ -482,8 +530,12 @@ Type ".help" for additional help. supervisor.read().cancel_recursive(); } - self.ctx.write().exit_session()?; - Ok(()) + let exit_result = self.ctx.write().exit_session(); + self.ctx + .write() + .top_level_agent_finished(exit_result.as_ref().err()); + hooks::drain_pending(EXIT_HOOK_DRAIN_TIMEOUT).await; + exit_result } fn create_editor(ctx: Arc>, app: &AppConfig) -> Result { @@ -983,6 +1035,7 @@ pub async fn run_repl_command( let app = Arc::clone(&ctx.app.config); ctx.use_agent(app.as_ref(), agent_name, session_name, abort_signal.clone()) .await?; + ctx.top_level_agent_started(); if let Some(session) = &ctx.session { let (compressed, active) = replay::snapshot(session); replay::render(app.as_ref(), &compressed, &active)?; @@ -1090,6 +1143,7 @@ pub async fn run_repl_command( abort_signal.clone(), ) .await?; + hooks::fire(HookEvent::SessionCompressed, ctx, &[], None); println!("✓ Successfully compressed the session."); } _ => { @@ -1433,6 +1487,17 @@ async fn ask( abort_signal: AbortSignal, input: Input, with_embeddings: bool, +) -> Result<()> { + let bracket = TurnBracket::enter(ctx); + let result = ask_inner(ctx, abort_signal.clone(), input, with_embeddings).await; + bracket.finish(ctx, &abort_signal, result) +} + +async fn ask_inner( + ctx: &mut RequestContext, + abort_signal: AbortSignal, + input: Input, + with_embeddings: bool, ) -> Result<()> { let mut input = input; let mut with_embeddings = with_embeddings; @@ -1579,9 +1644,7 @@ async fn ask( }; eprintln!("\n📢 {}", color.italic().paint("Compressing the session."),); - if let Err(err) = ctx.compress_session().await { - warn!("Failed to compress the session: {err}"); - } + auto_compress_session(ctx).await; if let Some(session) = ctx.session.as_mut() { session.set_compressing(false); } @@ -1632,6 +1695,13 @@ fn print_turn_divider() { println!(); } +async fn auto_compress_session(ctx: &mut RequestContext) { + match ctx.compress_session().await { + Ok(()) => hooks::fire(HookEvent::SessionCompressed, ctx, &[], None), + Err(err) => warn!("Failed to compress the session: {err}"), + } +} + fn should_continue(ctx: &RequestContext) -> bool { let config = ctx.auto_continue_config(); ctx.app.config.function_calling_support @@ -1939,6 +2009,320 @@ pub fn split_args_text(line: &str, is_win: bool) -> (Vec, &str) { #[cfg(test)] mod tests { use super::*; + use crate::client::{ClientConfig, Model}; + use crate::config::{AppState, RoleLike, Session, WorkingMode}; + use crate::hooks::{HookDef, HooksMap, test_sink}; + use anyhow::anyhow; + use serial_test::serial; + use std::future::Future; + + const TURN_EVENTS: [&str; 4] = [ + "turn.started", + "turn.completed", + "turn.interrupted", + "turn.failed", + ]; + + fn ctx_with_hooks(events: &[&str], marker: &str) -> RequestContext { + let mut hooks_map = HooksMap::default(); + for event in events { + hooks_map.insert( + event.to_string(), + vec![HookDef { + name: format!("{marker}-{event}"), + command: "true".to_string(), + }], + ); + } + let mut app = AppState::test_default(); + app.config = Arc::new(AppConfig { + hooks: hooks_map, + ..Default::default() + }); + RequestContext::new(Arc::new(app), WorkingMode::Repl) + } + + /// Sink captures for `marker`, ordered (started, completed, interrupted, failed). + fn turn_counts(marker: &str) -> (usize, usize, usize, usize) { + let captures = test_sink::snapshot(); + let count = |event: &str| { + captures + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-{event}")) + .count() + }; + ( + count("turn.started"), + count("turn.completed"), + count("turn.interrupted"), + count("turn.failed"), + ) + } + + #[tokio::test] + #[serial] + async fn turn_bracket_ok_final_fires_started_and_completed() { + let _sink = test_sink::install(); + let marker = "tb-ok-x7q"; + let ctx = ctx_with_hooks(&TURN_EVENTS, marker); + let abort_signal = create_abort_signal(); + + let bracket = TurnBracket::enter(&ctx); + let result: Result<()> = async { Ok(()) }.await; + bracket.finish(&ctx, &abort_signal, result).unwrap(); + + assert_eq!(turn_counts(marker), (1, 1, 0, 0)); + } + + #[tokio::test] + #[serial] + async fn turn_bracket_tool_rounds_then_final_fire_one_turn() { + let _sink = test_sink::install(); + let marker = "tb-rounds-p2m"; + let ctx = ctx_with_hooks(&TURN_EVENTS, marker); + let abort_signal = create_abort_signal(); + + let bracket = TurnBracket::enter(&ctx); + let result: Result<()> = async { + // Simulates a turn that loops through tool rounds before the + // final round; rounds happen inside the bracket and never re-fire. + let mut rounds = 0; + loop { + rounds += 1; + if rounds == 3 { + break Ok(()); + } + } + } + .await; + bracket.finish(&ctx, &abort_signal, result).unwrap(); + + assert_eq!(turn_counts(marker), (1, 1, 0, 0)); + } + + #[tokio::test] + #[serial] + async fn turn_bracket_error_fires_failed_with_error_env() { + let _sink = test_sink::install(); + let marker = "tb-err-k4d"; + let ctx = ctx_with_hooks(&TURN_EVENTS, marker); + let abort_signal = create_abort_signal(); + + let bracket = TurnBracket::enter(&ctx); + let result: Result<()> = async { Err(anyhow!("turn exploded")) }.await; + let err = bracket.finish(&ctx, &abort_signal, result).unwrap_err(); + assert_eq!(err.to_string(), "turn exploded"); + + assert_eq!(turn_counts(marker), (1, 0, 0, 1)); + let captures = test_sink::snapshot(); + let failed = captures + .iter() + .find(|capture| capture.hook_name == format!("{marker}-turn.failed")) + .expect("turn.failed capture"); + assert_eq!( + failed.envs.get("COYOTE_ERROR").map(String::as_str), + Some("turn exploded") + ); + } + + #[tokio::test] + #[serial] + async fn turn_bracket_ctrlc_error_fires_interrupted_not_failed() { + let _sink = test_sink::install(); + let marker = "tb-ctrlc-err-j6w"; + let ctx = ctx_with_hooks(&TURN_EVENTS, marker); + let abort_signal = create_abort_signal(); + abort_signal.set_ctrlc(); + + let bracket = TurnBracket::enter(&ctx); + let result: Result<()> = async { Err(anyhow!("Aborted!")) }.await; + assert!(bracket.finish(&ctx, &abort_signal, result).is_err()); + + assert_eq!(turn_counts(marker), (1, 0, 1, 0)); + } + + #[tokio::test] + #[serial] + async fn turn_bracket_ctrlc_with_ok_exit_fires_interrupted() { + let _sink = test_sink::install(); + let marker = "tb-ctrlc-ok-r3v"; + let ctx = ctx_with_hooks(&TURN_EVENTS, marker); + let abort_signal = create_abort_signal(); + abort_signal.set_ctrlc(); + + let bracket = TurnBracket::enter(&ctx); + let result: Result<()> = async { Ok(()) }.await; + bracket.finish(&ctx, &abort_signal, result).unwrap(); + + assert_eq!(turn_counts(marker), (1, 0, 1, 0)); + } + + /// Drives a deep REPL future to completion on a thread with extra stack + /// headroom: nested `run_repl_command`/`compress_session` poll frames + /// are deep in debug builds and overflow the default test-thread stack + /// (Windows exhausts it first). + fn run_async(f: F) -> F::Output + where + F: Future + Send, + F::Output: Send, + { + std::thread::scope(|scope| { + std::thread::Builder::new() + .stack_size(8 * 1024 * 1024) + .spawn_scoped(scope, || { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap() + .block_on(f) + }) + .unwrap() + .join() + .unwrap() + }) + } + + #[test] + #[serial] + fn compress_command_failure_fires_no_session_compressed() { + let _sink = test_sink::install(); + let marker = "compress-fail-z9t"; + run_async(async { + let mut ctx = ctx_with_hooks(&["session.compressed"], marker); + ctx.session = Some(Session::default()); + let abort_signal = create_abort_signal(); + + // Boxed: `run_repl_command`'s state machine is far larger than a + // test thread's stack. + let result = Box::pin(run_repl_command( + &mut ctx, + abort_signal, + ".compress session", + )) + .await; + + assert!(result.is_err(), "compressing an empty session must fail"); + }); + assert_eq!( + test_sink::snapshot() + .iter() + .filter(|capture| capture.hook_name.starts_with(marker)) + .count(), + 0, + "a failed compression must fire no session.compressed hook" + ); + } + + /// A ctx whose compression can actually SUCCEED offline: `dry_run` + /// makes the summarization call echo instead of hitting the network, + /// the default openai client config makes the session model + /// resolvable, and the session already holds a user exchange so + /// `compress_session` has something to summarize. + fn compressible_ctx(marker: &str) -> RequestContext { + let mut hooks_map = HooksMap::default(); + hooks_map.insert( + "session.compressed".to_string(), + vec![HookDef { + name: format!("{marker}-session.compressed"), + command: "true".to_string(), + }], + ); + let mut app = AppState::test_default(); + app.config = Arc::new(AppConfig { + hooks: hooks_map, + dry_run: true, + clients: vec![ClientConfig::default()], + ..Default::default() + }); + let mut ctx = RequestContext::new(Arc::new(app), WorkingMode::Repl); + let mut session = Session::default(); + session.set_model(Model::new("openai", "test-compress-model")); + ctx.session = Some(session); + let input = Input::from_str(&ctx, "hello there", None).unwrap(); + ctx.session + .as_mut() + .unwrap() + .add_message(&input, "hi") + .unwrap(); + ctx + } + + fn session_compressed_count(marker: &str) -> usize { + test_sink::snapshot() + .iter() + .filter(|capture| capture.hook_name == format!("{marker}-session.compressed")) + .count() + } + + #[test] + #[serial] + fn compress_command_success_fires_session_compressed_once() { + let _sink = test_sink::install(); + let marker = "compress-ok-w2p"; + run_async(async { + let mut ctx = compressible_ctx(marker); + let abort_signal = create_abort_signal(); + + // Boxed: `run_repl_command`'s state machine is far larger than a + // test thread's stack. + let result = Box::pin(run_repl_command( + &mut ctx, + abort_signal, + ".compress session", + )) + .await; + + assert!( + result.is_ok(), + "dry-run compression must succeed: {result:?}" + ); + }); + assert_eq!( + session_compressed_count(marker), + 1, + "a successful `.compress session` must fire session.compressed exactly once" + ); + } + + #[test] + #[serial] + fn auto_compress_success_fires_session_compressed_once() { + // Covers the ask-loop call site through its extracted seam; + // driving `needs_compression` itself requires a full LLM turn and + // is exercised end-to-end by integration coverage, not here. + let _sink = test_sink::install(); + let marker = "auto-compress-ok-f8r"; + run_async(async { + let mut ctx = compressible_ctx(marker); + auto_compress_session(&mut ctx).await; + }); + + assert_eq!( + session_compressed_count(marker), + 1, + "a successful auto-compression must fire session.compressed exactly once" + ); + } + + #[test] + #[serial] + fn auto_compress_failure_fires_no_session_compressed_and_swallows_error() { + let _sink = test_sink::install(); + let marker = "auto-compress-fail-d3k"; + run_async(async { + let mut ctx = compressible_ctx(marker); + // An empty session makes compress_session fail; the seam must + // swallow the error (no panic, no propagation) and fire nothing. + ctx.session = Some(Session::default()); + auto_compress_session(&mut ctx).await; + }); + + assert_eq!( + session_compressed_count(marker), + 0, + "a failed auto-compression must fire no session.compressed hook" + ); + } #[test] fn test_process_command_line() { diff --git a/src/sandbox/mixins.rs b/src/sandbox/mixins.rs index 9d75e6f..7021d41 100644 --- a/src/sandbox/mixins.rs +++ b/src/sandbox/mixins.rs @@ -200,6 +200,12 @@ pub fn discover() -> Result> { for path in collect_mixins(&paths::rags_dir(), &[ScanMode::Flat]) { out.push(read_mixin(path)?); } + for path in collect_mixins(&paths::roles_dir(), &[ScanMode::Flat]) { + out.push(read_mixin(path)?); + } + for path in collect_mixins(&paths::roles_dir().join("hooks"), &[ScanMode::Flat]) { + out.push(read_mixin(path)?); + } if let Ok(cwd) = env::current_dir() && let Some(path) = paths::find_workspace_sbx_mixin(&cwd) @@ -940,6 +946,65 @@ network: let _ = fs::remove_dir_all(&root); } + #[test] + #[serial_test::serial] + fn discover_scans_roles_and_role_hooks_mixins() { + use crate::utils::get_env_name; + + let root = unique_root("discover-roles"); + let roles = root.join("roles"); + fs::create_dir_all(roles.join("hooks")).unwrap(); + let role_mixin = roles.join("review.sbx-mixin.yaml"); + let hook_mixin = roles.join("hooks").join("notify.sbx-mixin.yaml"); + fs::write(&role_mixin, "kind: mixin\n").unwrap(); + fs::write(&hook_mixin, "kind: mixin\n").unwrap(); + // The builtin installer's hidden manifest lives in the same dir but + // is not a mixin and must never be picked up by the flat scan. + let manifest = roles + .join("hooks") + .join(crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE); + fs::write(&manifest, "notify.sh\n").unwrap(); + + let config_env = get_env_name("config_dir"); + let roles_env = get_env_name("roles_dir"); + let prev_config = env::var_os(&config_env); + let prev_roles = env::var_os(&roles_env); + unsafe { + env::set_var(&config_env, &root); + env::set_var(&roles_env, &roles); + } + + let discovered = discover(); + + unsafe { + match prev_config { + Some(v) => env::set_var(&config_env, v), + None => env::remove_var(&config_env), + } + match prev_roles { + Some(v) => env::set_var(&roles_env, v), + None => env::remove_var(&roles_env), + } + } + + let found: Vec = discovered + .unwrap() + .into_iter() + .map(|mixin| mixin.path) + .collect(); + assert!(found.contains(&role_mixin), "missing role mixin: {found:?}"); + assert!( + found.contains(&hook_mixin), + "missing role hook mixin: {found:?}" + ); + assert!( + !found.contains(&manifest), + "builtin manifest must not be scanned as a mixin: {found:?}" + ); + + let _ = fs::remove_dir_all(&root); + } + /// `generate_rag_sbx_mixin` writes an agent-scoped RAG sidecar next to the /// rag yaml, at `//.sbx-mixin.yaml`. Before `SubdirFlat` /// existed, nothing scanned that shape and attaching a Qdrant RAG from diff --git a/src/testing.rs b/src/testing.rs index 6ff02ea..a41d69c 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -3,27 +3,64 @@ use log::{Level, LevelFilter, Log, Metadata, Record}; use std::sync::{Mutex, Once, OnceLock}; -struct WarnCollector; +struct TestLogCollector; static WARN_MESSAGES: OnceLock>> = OnceLock::new(); +static DEBUG_MESSAGES: OnceLock>> = OnceLock::new(); -/// Every warn-level message captured since the collector was installed. The -/// buffer is process-global and shared by all tests, so assert by filtering -/// for a marker unique to the test rather than on the whole buffer. +/// Debug capture is restricted to the hooks module so the buffer is not +/// flooded by debug output from the rest of the crate. Matching is +/// module-boundary-aware: only this exact module or its `::` submodules +/// qualify, so a sibling like `hooks_registry` would not. +const DEBUG_TARGET_PREFIX: &str = concat!(env!("CARGO_CRATE_NAME"), "::hooks"); + +fn captures_warn(metadata: &Metadata) -> bool { + metadata.level() <= Level::Warn +} + +fn captures_hooks_debug(metadata: &Metadata) -> bool { + if metadata.level() != Level::Debug { + return false; + } + let target = metadata.target(); + target == DEBUG_TARGET_PREFIX + || target + .strip_prefix(DEBUG_TARGET_PREFIX) + .is_some_and(|rest| rest.starts_with("::")) +} + +/// Every warn- or error-level message captured since the collector was +/// installed. The buffer is process-global and shared by all tests, so assert +/// by filtering for a marker unique to the test rather than on the whole +/// buffer. pub(crate) fn warn_messages() -> &'static Mutex> { WARN_MESSAGES.get_or_init(Mutex::default) } -impl Log for WarnCollector { +/// Every debug-level message from the hooks module captured since the +/// collector was installed. Same marker-filtering discipline as +/// [`warn_messages`]. +pub(crate) fn debug_messages() -> &'static Mutex> { + DEBUG_MESSAGES.get_or_init(Mutex::default) +} + +impl Log for TestLogCollector { fn enabled(&self, metadata: &Metadata) -> bool { - metadata.level() <= Level::Warn + captures_warn(metadata) || captures_hooks_debug(metadata) } + // A test that panics while holding a buffer poisons its mutex; the logger + // recovers via `into_inner` so every later test can still log. fn log(&self, record: &Record) { - if self.enabled(record.metadata()) { + if captures_warn(record.metadata()) { warn_messages() .lock() - .unwrap() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .push(record.args().to_string()); + } else if captures_hooks_debug(record.metadata()) { + debug_messages() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) .push(record.args().to_string()); } } @@ -31,13 +68,15 @@ impl Log for WarnCollector { fn flush(&self) {} } -/// Installs the process-wide warn collector. `log::set_logger` accepts one -/// logger per process, so every test that captures warns must install through -/// this shared entry point. +/// Installs the process-wide log collector. `log::set_logger` accepts one +/// logger per process, so every test that captures warns or hooks debug +/// output must install through this shared entry point. The max level is +/// Debug so the hooks debug capture sees its records; warn capture is +/// unaffected. pub(crate) fn install_warn_collector() { static INSTALL: Once = Once::new(); INSTALL.call_once(|| { - log::set_logger(&WarnCollector).expect("no other logger should be installed"); - log::set_max_level(LevelFilter::Warn); + log::set_logger(&TestLogCollector).expect("no other logger should be installed"); + log::set_max_level(LevelFilter::Debug); }); }