diff --git a/.gitattributes b/.gitattributes index e05c06b..07bd69a 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,5 @@ assets/config-template.yaml text eol=lf +assets/hooks/*.sh text eol=lf +assets/functions/tools/* text eol=lf +assets/functions/scripts/* text eol=lf +assets/functions/utils/*.sh text eol=lf diff --git a/assets/hooks/log-events.sh b/assets/hooks/log-events.sh index a5563d6..0b402b6 100755 --- a/assets/hooks/log-events.sh +++ b/assets/hooks/log-events.sh @@ -4,7 +4,8 @@ # each one carries. # # Env vars read: -# COYOTE_HOOK_LOG log file to append to (default: /tmp/coyote-hooks.log) +# COYOTE_HOOK_LOG log file to append to +# (default: ${XDG_STATE_HOME:-$HOME/.local/state}/coyote/hooks.log) # COYOTE_* everything coyote sets for the event is dumped verbatim # # Secrets (COYOTE_SECRET_*) are excluded from the snapshot; widening the grep @@ -26,15 +27,15 @@ set -u -log_file="${COYOTE_HOOK_LOG:-/tmp/coyote-hooks.log}" +log_file="${COYOTE_HOOK_LOG:-${XDG_STATE_HOME:-$HOME/.local/state}/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. +# COYOTE_HOOK_LOG may point into a shared directory: refuse to append through +# a symlink someone else planted there. [ -L "$log_file" ] && exit 0 umask 077 +mkdir -p "$(dirname "$log_file")" 2> /dev/null || true { - echo "=== $(date '+%Y-%m-%dT%H:%M:%S%z') ${COYOTE_EVENT:-unknown}" + echo "=== $(date -u '+%Y-%m-%dT%H:%M:%SZ') ${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 index 1c010a3..1a7e5ef 100755 --- a/assets/hooks/notify.sh +++ b/assets/hooks/notify.sh @@ -38,5 +38,8 @@ elif command -v osascript > /dev/null 2>&1; then 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" + # $body carries model-influenced values: strip control bytes (including + # ESC, so ANSI/OSC sequences cannot drive the terminal) before echoing. + safe_line="$(printf '%s' "[$title] $body" | tr -d '\000-\010\013\014\016-\037\177')" + { echo "$safe_line" > /dev/tty; } 2> /dev/null || echo "$safe_line" fi diff --git a/scripts/usage-probe-hooks.sh b/scripts/usage-probe-hooks.sh index 6246bb5..aab18ac 100755 --- a/scripts/usage-probe-hooks.sh +++ b/scripts/usage-probe-hooks.sh @@ -52,6 +52,21 @@ fail() { exit 1 } +# Bounded poll: retries the given command every 0.1s until it succeeds or the +# ~10s deadline expires, then fails loudly. Hooks are fire-and-forget child +# processes, so every wait on their side effects goes through this helper — +# never a bare sleep. +wait_for() { + local what="$1" + shift + local tries=0 + until "$@"; do + tries=$((tries + 1)) + [ "$tries" -ge 100 ] && fail "timed out waiting for $what" + sleep 0.1 + done +} + write_dryrun_config() { # $1 = config dir, $2..$n = extra YAML fragment lines appended verbatim local dir="$1" @@ -95,7 +110,8 @@ write_dryrun_config "$CFG_A" "hooks:" \ " 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 +scenario_a_logs_ready() { [ -s "$TS_LOG" ] && [ -s "$TC_LOG" ] && [ -s "$LC_LOG" ]; } +wait_for "turn/llm hook logs" scenario_a_logs_ready [ "$(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)" @@ -120,12 +136,12 @@ 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=$? +rc_b=0 +COYOTE_CONFIG_DIR="$CFG_B" "$BIN" --no-stream "say exactly: hi" > "$WORKDIR/out_b.txt" 2> "$WORKDIR/err_b.txt" || rc_b=$? +rc_base=0 +COYOTE_CONFIG_DIR="$CFG_BASE" "$BIN" --no-stream "say exactly: hi" > "$WORKDIR/out_base.txt" 2> "$WORKDIR/err_base.txt" || rc_base=$? -[ "$exit_b" = "$exit_base" ] || fail "exit code differs: with-hooks=$exit_b baseline=$exit_base" +[ "$rc_b" = "$rc_base" ] || fail "exit code differs: with-hooks=$rc_b baseline=$rc_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" @@ -155,6 +171,12 @@ fi # reinstalled from the packaged content. echo "# LOCALLY MODIFIED" > "$CFG_C/hooks/notify.sh" rm "$CFG_C/hooks/log-events.sh" +# The refresh also touches the roles and per-agent hook locations: user files +# living there must survive it untouched. +AGENT_C="$(ls "$CFG_C/agents" | head -1)" +mkdir -p "$CFG_C/roles/hooks" "$CFG_C/agents/$AGENT_C/hooks" +echo "mine-role" > "$CFG_C/roles/hooks/user-role.sh" +echo "mine-agent" > "$CFG_C/agents/$AGENT_C/hooks/user-agent.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 "non-interactive reinstall overwrote the locally modified notify.sh" @@ -162,7 +184,15 @@ grep -q "LOCALLY MODIFIED" "$CFG_C/hooks/notify.sh" \ || fail "reinstall did not restore the deleted log-events.sh" grep -q '^#!/usr/bin/env bash' "$CFG_C/hooks/log-events.sh" \ || fail "log-events.sh was not restored to the packaged script" -echo "PASS: --install-builtins hooks installs both scripts 0755; re-runs keep modified files and restore missing ones" +if command -v stat > /dev/null 2>&1; then + mode_restored="$(stat -c '%a' "$CFG_C/hooks/log-events.sh" 2>/dev/null || stat -f '%Lp' "$CFG_C/hooks/log-events.sh")" + [ "$mode_restored" = "755" ] || fail "restored log-events.sh mode is $mode_restored, expected 755" +fi +[ "$(cat "$CFG_C/roles/hooks/user-role.sh")" = "mine-role" ] \ + || fail "user file in roles/hooks was touched by the refresh" +[ "$(cat "$CFG_C/agents/$AGENT_C/hooks/user-agent.sh")" = "mine-agent" ] \ + || fail "user file in the agent hooks dir was touched by the refresh" +echo "PASS: --install-builtins hooks installs both scripts 0755; re-runs keep modified files (all hook locations) and restore missing ones 0755" echo "== Scenario C2: cold-start auto-bootstrap fills the hooks dir but never forces ==" CFG_C2="$WORKDIR/c2" @@ -198,7 +228,7 @@ write_dryrun_config "$CFG_C3" "hooks:" \ 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 +wait_for "installed log-events.sh output" test -s "$HOOK_LOG" [ -f "$HOOK_LOG" ] || fail "installed log-events.sh did not write to COYOTE_HOOK_LOG via relative-path wiring" [ "$(grep -c '^=== ' "$HOOK_LOG")" = "1" ] \ @@ -219,6 +249,7 @@ echo "== Scenario D: --agent --info fires zero agent.* events ==" CFG_D="$WORKDIR/d" mkdir -p "$CFG_D/agents" AGENT_LOG="$WORKDIR/agent-events.log" +SENTINEL_LOG="$WORKDIR/d-sentinel.log" write_dryrun_config "$CFG_D" "hooks:" \ " agent.started:" \ " - name: probe-agent" \ @@ -228,13 +259,20 @@ write_dryrun_config "$CFG_D" "hooks:" \ " command: \"echo AGENT_DONE >> $AGENT_LOG\"" \ " agent.failed:" \ " - name: probe-agent" \ - " command: \"echo AGENT_FAILED >> $AGENT_LOG\"" + " command: \"echo AGENT_FAILED >> $AGENT_LOG\"" \ + " turn.completed:" \ + " - name: sentinel" \ + " command: \"echo SENTINEL >> $SENTINEL_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 +# Proving absence needs a positive signal to wait on: a follow-up plain run +# fires turn.completed, and its sentinel landing bounds how long the --info +# run's (nonexistent) agent.* children could have taken to write. +COYOTE_CONFIG_DIR="$CFG_D" "$BIN" --no-stream "say exactly: hi" > /dev/null +wait_for "scenario D sentinel" test -s "$SENTINEL_LOG" [ -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" @@ -290,10 +328,10 @@ grep -q 'old-hook.sh' "$CFG_F/hooks/.builtin-manifest" \ # not fail the command. printf '\xff\xfe\x00' > "$CFG_F/hooks/.builtin-manifest" echo "keep" > "$CFG_F/hooks/orphan.sh" -exit_f=0 -COYOTE_CONFIG_DIR="$CFG_F" "$BIN" --install-builtins hooks > "$WORKDIR/install_f.out" 2>&1 || exit_f=$? -[ "$exit_f" = "0" ] \ - || fail "reinstall failed on a malformed manifest (exit $exit_f): $(cat "$WORKDIR/install_f.out")" +rc_f=0 +COYOTE_CONFIG_DIR="$CFG_F" "$BIN" --install-builtins hooks > "$WORKDIR/install_f.out" 2>&1 || rc_f=$? +[ "$rc_f" = "0" ] \ + || fail "reinstall failed on a malformed manifest (exit $rc_f): $(cat "$WORKDIR/install_f.out")" [ -f "$CFG_F/hooks/orphan.sh" ] \ || fail "a malformed manifest caused a deletion (fail-safe broken)" [ -f "$CFG_F/hooks/notify.sh" ] && [ -f "$CFG_F/hooks/log-events.sh" ] \ diff --git a/src/config/agent.rs b/src/config/agent.rs index 58e208c..156bd5e 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -32,7 +32,7 @@ use inquire::{Text, validator::Validation}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, BTreeSet}; -use std::{env, ffi::OsStr, path::Path}; +use std::{env, path::Path}; const DEFAULT_AGENT_NAME: &str = "rag"; @@ -89,7 +89,8 @@ pub(crate) fn install_and_reconcile_agent_hooks( } ensure_parent_exists(&path)?; info!("Creating agent hook file: {}", path.display()); - write_file_atomic(&path, content, Some(0o755))?; + write_file_atomic(&path, content, None)?; + set_executable_bit_if_script(&path)?; written.insert(name.clone()); } @@ -127,6 +128,15 @@ fn sweep_removed_agent_hooks(bundled_files: &HashMap>) { if bundled_files.contains_key(name) { continue; } + // A symlinked agent dir is user-arranged: never follow it into a + // hooks tree that lives somewhere else. + if !entry + .path() + .symlink_metadata() + .is_ok_and(|metadata| metadata.is_dir()) + { + continue; + } let hooks_dir = entry.path().join("hooks"); if !hooks_dir .join(builtin_manifest::BUILTIN_MANIFEST_FILE) @@ -184,14 +194,9 @@ impl Agent { let embedded_file = AgentAssets::get(&file) .ok_or_else(|| anyhow!("Failed to load embedded agent file: {}", file.as_ref()))?; - let content = unsafe { std::str::from_utf8_unchecked(&embedded_file.data) }; + let content = std::str::from_utf8(&embedded_file.data) + .expect("bundled agent asset is not valid UTF-8"); let file_path = paths::agents_data_dir().join(file.as_ref()); - let file_extension = file_path - .extension() - .and_then(OsStr::to_str) - .map(|s| s.to_lowercase()); - #[cfg_attr(not(unix), expect(unused))] - let is_script = matches!(file_extension.as_deref(), Some("sh") | Some("py")); if file_path.exists() && !conflict::should_replace_existing( @@ -213,18 +218,13 @@ impl Agent { info!("Creating agent file: {}", file_path.display()); let mut agent_file = File::create(&file_path)?; agent_file.write_all(content.as_bytes())?; + set_executable_bit_if_script(&file_path)?; 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 { - use std::{fs, os::unix::fs::PermissionsExt}; - fs::set_permissions(&file_path, fs::Permissions::from_mode(0o755))?; - } } let mut bundled_files: HashMap> = HashMap::new(); @@ -303,7 +303,8 @@ impl Agent { }; let embedded = AgentAssets::get(&file) .ok_or_else(|| anyhow!("Failed to load embedded agent file: {}", file.as_ref()))?; - let content = unsafe { std::str::from_utf8_unchecked(&embedded.data) }; + let content = std::str::from_utf8(&embedded.data) + .expect("bundled agent hook asset is not valid UTF-8"); hooks.push((hook.to_string(), content.to_string())); } @@ -2058,36 +2059,7 @@ nodes: {} assert_eq!(output, expected); } - /// Points the config dir at a fresh temp directory for the guard's - /// lifetime and removes it on drop. Tests using it must serialize. - struct TestConfigDirGuard { - _env: crate::testing::EnvVarGuard, - root: PathBuf, - } - - impl TestConfigDirGuard { - fn new(label: &str) -> Self { - let unique = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos(); - let root = env::temp_dir().join(format!("coyote-{label}-{unique}")); - std::fs::create_dir_all(&root).unwrap(); - Self { - _env: crate::testing::EnvVarGuard::set( - crate::utils::get_env_name("config_dir"), - &root, - ), - root, - } - } - } - - impl Drop for TestConfigDirGuard { - fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.root); - } - } + use crate::testing::TestConfigDirGuard; fn fixture(entries: &[(&str, &str)]) -> Vec<(String, String)> { entries @@ -2220,4 +2192,30 @@ nodes: {} ); assert_eq!(read_to_string(dir.join("user.sh")).unwrap(), "user-owned"); } + + #[cfg(unix)] + #[test] + #[serial_test::serial] + fn sweep_removed_agent_hooks_never_follows_symlinked_agent_dirs() { + let guard = TestConfigDirGuard::new("agent-sweep-symlink"); + let real = guard.path.join("agent-elsewhere"); + let hooks = real.join("hooks"); + std::fs::create_dir_all(&hooks).unwrap(); + std::fs::write( + hooks.join(builtin_manifest::BUILTIN_MANIFEST_FILE), + "shipped.sh\n", + ) + .unwrap(); + std::fs::write(hooks.join("shipped.sh"), "stale").unwrap(); + std::fs::create_dir_all(paths::agents_data_dir()).unwrap(); + std::os::unix::fs::symlink(&real, paths::agents_data_dir().join("linked")).unwrap(); + + sweep_removed_agent_hooks(&HashMap::new()); + + assert!( + hooks.join("shipped.sh").exists(), + "the sweep must never follow a symlinked agent dir" + ); + assert!(hooks.join(builtin_manifest::BUILTIN_MANIFEST_FILE).exists()); + } } diff --git a/src/config/builtin_manifest.rs b/src/config/builtin_manifest.rs index 7e3bc31..ce0da9c 100644 --- a/src/config/builtin_manifest.rs +++ b/src/config/builtin_manifest.rs @@ -23,6 +23,22 @@ pub(crate) fn is_builtin_manifest_name(name: &str) -> bool { .eq_ignore_ascii_case(BUILTIN_MANIFEST_FILE) } +/// NTFS 8.3 short names (`BUILTI~1`, `builti~1.sh`) can alias any long +/// filename on the same volume — including the manifest itself — so an entry +/// shaped like one is never accepted: it could direct a deletion at a file +/// the manifest never named. +fn is_ntfs_short_name_alias(name: &str) -> bool { + let stem = name.split('.').next().unwrap_or(name); + let Some((base, digits)) = stem.rsplit_once('~') else { + return false; + }; + !base.is_empty() + && base.len() <= 8 + && base.chars().all(|c| c.is_ascii_alphanumeric() || c == '~') + && !digits.is_empty() + && digits.chars().all(|c| c.is_ascii_digit()) +} + /// 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 @@ -83,7 +99,7 @@ fn read_manifest(dir: &Path) -> BTreeSet { if line.is_empty() || is_builtin_manifest_name(line) { return false; } - if !is_plain_file_name(line) { + if !is_plain_file_name(line) || is_ntfs_short_name_alias(line) { debug!( "Ignoring suspicious builtin manifest entry in {}: {line:?}", path.display() @@ -290,4 +306,33 @@ mod tests { assert!(!is_builtin_manifest_name(".builtin-manifesto")); assert!(!is_builtin_manifest_name("notify.sh")); } + + #[test] + fn ntfs_short_name_aliases_are_detected() { + assert!(is_ntfs_short_name_alias("BUILTI~1")); + assert!(is_ntfs_short_name_alias("builti~1.sh")); + assert!(is_ntfs_short_name_alias("NOTIFY~12")); + assert!(is_ntfs_short_name_alias("A~1")); + assert!(!is_ntfs_short_name_alias("notify.sh")); + assert!(!is_ntfs_short_name_alias("my~hook.sh")); + assert!(!is_ntfs_short_name_alias("~1")); + assert!(!is_ntfs_short_name_alias("way-too-long~1")); + assert!(!is_ntfs_short_name_alias(".builtin-manifest")); + } + + #[test] + fn ntfs_short_name_entries_never_direct_deletions() { + let dir = fresh_dir("builtin-manifest-shortname-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), "BUILTI~1\nbuilti~1.sh\n").unwrap(); + fs::write(dir.join("BUILTI~1"), "keep").unwrap(); + fs::write(dir.join("builti~1.sh"), "keep").unwrap(); + + reconcile_builtin_dir(&dir, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + + assert!( + dir.join("BUILTI~1").exists() && dir.join("builti~1.sh").exists(), + "8.3-alias-shaped manifest entries must never be removal candidates" + ); + let _ = fs::remove_dir_all(&dir); + } } diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 4ebbee1..d776fd0 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -4,9 +4,9 @@ use super::bundles::{ }; use crate::config::builtin_manifest::is_builtin_manifest_name; use crate::config::conflict::{ConflictAction, NonInteractive, StickyMode, resolve_conflict}; -use crate::config::{AssetCategory, BUNDLE_MANIFEST_FILE, InstallFilter, paths}; -#[cfg(not(windows))] -use crate::function::Language; +use crate::config::{ + AssetCategory, BUNDLE_MANIFEST_FILE, InstallFilter, paths, set_executable_bit_if_script, +}; use crate::mcp::{McpServer, McpServersConfig}; use crate::utils; use crate::utils::IS_STDOUT_TERMINAL; @@ -1858,18 +1858,18 @@ fn print_plan_summary(plan: &InstallPlan) { } /// 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. +/// config, so every planned file landing in a hook location is called out +/// individually. Visibility only: installing them needs no extra approval. +/// +/// Only the locations the engine actually resolves hooks from are announced: +/// the top-level hooks category, direct children of `agents//hooks/`, +/// and direct children of `roles/hooks/`. A `hooks/` directory anywhere else +/// (e.g. inside a skill or macro subtree) is inert data and announcing it +/// would be noise. 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")) - }) + .filter(|planned| is_hook_location(planned)) .map(|planned| { let rel = planned.rel.to_string_lossy().replace('\\', "/"); format!( @@ -1880,6 +1880,16 @@ fn hook_script_lines(plan: &InstallPlan) -> Vec { .collect() } +fn is_hook_location(planned: &PlannedFile) -> bool { + let components: Vec<_> = planned.rel.components().collect(); + match planned.top_category { + TopCategory::Hooks => true, + TopCategory::Agents => components.len() == 3 && components[1].as_os_str() == "hooks", + TopCategory::Roles => components.len() == 2 && components[0].as_os_str() == "hooks", + _ => false, + } +} + fn count_kind(plan: &InstallPlan, cat: TopCategory, kind: PlannedKind) -> usize { plan.files .iter() @@ -2042,26 +2052,6 @@ fn write_file(src: &Path, dst: &Path) -> Result<()> { Ok(()) } -#[cfg(unix)] -fn set_executable_bit_if_script(path: &Path) -> Result<()> { - use std::os::unix::fs::PermissionsExt; - - let Some(ext) = path.extension().and_then(OsStr::to_str) else { - return Ok(()); - }; - if Language::from_extension(ext) == Language::Unsupported { - return Ok(()); - } - fs::set_permissions(path, fs::Permissions::from_mode(0o755)) - .with_context(|| format!("chmod {}", path.display()))?; - Ok(()) -} - -#[cfg(not(unix))] -fn set_executable_bit_if_script(_path: &Path) -> Result<()> { - Ok(()) -} - #[derive(Debug)] struct McpMergeReport { added: Vec, @@ -2874,6 +2864,14 @@ mod tests { file("x/config.yaml", TopCategory::Agents), file("reviewer.md", TopCategory::Roles), file("hello.yaml", TopCategory::Macros), + // Only the locations the engine resolves hooks from announce: + // a hooks/ directory nested deeper or under another category + // is inert data. + file("x/hooks/nested/deep.sh", TopCategory::Agents), + file("x/sub/hooks/pre.sh", TopCategory::Agents), + file("sub/hooks/extra.sh", TopCategory::Roles), + file("my-skill/hooks/tool.sh", TopCategory::Skills), + file("my-macro/hooks/tool.sh", TopCategory::Macros), ], mcp_json: None, }; diff --git a/src/config/macros.rs b/src/config/macros.rs index 2193afe..1da3b9d 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -167,7 +167,8 @@ impl Macro { debug!("Processing macro file: {}", file.as_ref()); let embedded_file = MacroAssets::get(&file) .ok_or_else(|| anyhow!("Failed to load embedded macro file: {}", file.as_ref()))?; - let content = unsafe { std::str::from_utf8_unchecked(&embedded_file.data) }; + let content = std::str::from_utf8(&embedded_file.data) + .expect("bundled macro asset is not valid UTF-8"); let file_path = paths::macros_dir().join(file.as_ref()); if file_path.exists() && !force { @@ -350,56 +351,14 @@ fn default_true() -> bool { mod tests { use super::*; use crate::config::{AppState, Session, WorkingMode}; + use crate::testing::TestConfigDirGuard; use crate::utils::{create_abort_signal, get_env_name}; use serial_test::serial; use std::fs::{create_dir_all, remove_dir_all, write}; use std::future::Future; - use std::path::PathBuf; use std::time::{SystemTime, UNIX_EPOCH}; use std::{env, str}; - 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-macros-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); - } - } - fn test_ctx() -> RequestContext { RequestContext::new(Arc::new(AppState::test_default()), WorkingMode::Cmd) } @@ -974,7 +933,7 @@ variables: #[test] #[serial] fn non_isolated_steps_run_on_live_ctx_and_mutations_persist() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file( "live-macro", "isolated: false\nsteps:\n - \".set temperature 0.42\"\n", @@ -1006,7 +965,7 @@ variables: #[test] #[serial] fn non_isolated_step_failure_aborts_and_restores_flag_and_mode() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file( "fail-macro", "isolated: false\nsteps:\n - \".set temperature 0.9\"\n - \".update\"\n - \".set temperature 0.1\"\n", @@ -1057,7 +1016,7 @@ variables: #[test] #[serial] fn non_isolated_macro_step_invoking_macro_is_rejected() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file( "outer-macro", "isolated: false\nsteps:\n - \".inner-macro\"\n", @@ -1093,7 +1052,7 @@ variables: #[test] #[serial] fn isolated_macro_step_runs_non_isolated_macro_inline_on_fork() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file("iso-outer-macro", "steps:\n - \".inner-macro\"\n"); write_macro_file( "inner-macro", @@ -1122,7 +1081,7 @@ variables: #[test] #[serial] fn isolated_macro_still_forks_and_leaves_live_ctx_untouched() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file("iso-macro", "steps:\n - \".set temperature 0.77\"\n"); let mut ctx = test_ctx(); ctx.session = Some(Session::default()); @@ -1152,7 +1111,7 @@ variables: #[test] #[serial] fn guard_restores_prior_flag_values_after_inline_run() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("macros-tests"); write_macro_file( "inner-macro", "isolated: false\nsteps:\n - \".set temperature 0.11\"\n", diff --git a/src/config/mod.rs b/src/config/mod.rs index e5d5885..c87a6b0 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -419,6 +419,10 @@ impl Default for Config { } pub fn install_builtins() -> Result<()> { + // Best-effort housekeeping riding along with the startup installers: + // payload files orphaned by crashed processes accumulate in a shared + // temp dir and nothing else ever revisits them. + hooks::sweep_stale_payload_files(); Functions::install_builtin_global_tools(false)?; Agent::install_builtin_agents(InstallMode::Skip)?; Macro::install_macros(false)?; @@ -1152,6 +1156,32 @@ pub(crate) fn ensure_parent_exists(path: &Path) -> Result<()> { Ok(()) } +/// The one exec-bit policy for every hook/agent installer: a file is +/// executable iff `Language::from_extension` recognizes its extension. +/// Users may keep non-executable support files in hooks directories, so +/// installers must write plain and then apply this predicate — never +/// blanket-0755. +#[cfg(unix)] +pub(crate) fn set_executable_bit_if_script(path: &Path) -> Result<()> { + use crate::function::Language; + use std::os::unix::fs::PermissionsExt; + + let Some(ext) = path.extension().and_then(std::ffi::OsStr::to_str) else { + return Ok(()); + }; + if Language::from_extension(ext) == Language::Unsupported { + return Ok(()); + } + fs::set_permissions(path, fs::Permissions::from_mode(0o755)) + .with_context(|| format!("chmod {}", path.display()))?; + Ok(()) +} + +#[cfg(not(unix))] +pub(crate) fn set_executable_bit_if_script(_path: &Path) -> Result<()> { + Ok(()) +} + fn read_env_value(key: &str) -> Option> where T: std::str::FromStr, @@ -1824,33 +1854,7 @@ hooks: } } - /// Points the config dir at a fresh temp directory for the guard's - /// lifetime and removes it on drop. Tests using it must serialize. - struct TestConfigDirGuard { - _env: crate::testing::EnvVarGuard, - root: PathBuf, - } - - impl TestConfigDirGuard { - fn new(label: &str) -> Self { - let unique = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos(); - let root = env::temp_dir().join(format!("coyote-{label}-{unique}")); - fs::create_dir_all(&root).unwrap(); - Self { - _env: crate::testing::EnvVarGuard::set(get_env_name("config_dir"), &root), - root, - } - } - } - - impl Drop for TestConfigDirGuard { - fn drop(&mut self) { - let _ = fs::remove_dir_all(&self.root); - } - } + use crate::testing::TestConfigDirGuard; #[test] #[serial_test::serial] diff --git a/src/config/role.rs b/src/config/role.rs index 55b2c9c..a2fa11c 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -180,7 +180,8 @@ impl Role { pub fn builtin(name: &str) -> Result { let content = RolesAsset::get(&format!("{name}.md")) .ok_or_else(|| anyhow!("Unknown role `{name}`"))?; - let content = unsafe { std::str::from_utf8_unchecked(&content.data) }; + let content = + std::str::from_utf8(&content.data).expect("bundled role asset is not valid UTF-8"); Ok(Role::new(name, content)) } @@ -202,7 +203,8 @@ impl Role { return None; } let embedded = RolesAsset::get(file.as_ref())?; - let content = unsafe { std::str::from_utf8_unchecked(&embedded.data) }; + let content = std::str::from_utf8(&embedded.data) + .expect("bundled role hook asset is not valid UTF-8"); Some((name.to_string(), content.to_string())) }) .collect(); @@ -665,7 +667,8 @@ pub(crate) fn install_and_reconcile_role_hooks( } ensure_parent_exists(&path)?; info!("Creating role hook file: {}", path.display()); - write_file_atomic(&path, content, Some(0o755))?; + write_file_atomic(&path, content, None)?; + set_executable_bit_if_script(&path)?; written.insert(name.clone()); } @@ -1084,6 +1087,41 @@ Input 1 let _ = fs::remove_dir_all(dir.parent().unwrap()); } + /// The installer writes plain and then applies the shared exec-bit + /// predicate: users may keep non-executable support files in hooks + /// directories, so only recognized script extensions get 0755. + #[cfg(unix)] + #[test] + fn role_hooks_install_leaves_non_script_files_non_executable() { + use std::os::unix::fs::PermissionsExt; + + let dir = hooks_fixture_dir("role-hooks-nonscript-"); + + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("README.md", "docs\n"), ("run.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .unwrap(); + + let readme_mode = fs::metadata(dir.join("README.md")) + .unwrap() + .permissions() + .mode(); + assert_eq!( + readme_mode & 0o111, + 0, + "a non-script file in a hooks dir must stay non-executable" + ); + let script_mode = fs::metadata(dir.join("run.sh")) + .unwrap() + .permissions() + .mode(); + assert_eq!(script_mode & 0o777, 0o755); + 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-"); diff --git a/src/config/skill.rs b/src/config/skill.rs index c705a11..b4b7a7d 100644 --- a/src/config/skill.rs +++ b/src/config/skill.rs @@ -100,7 +100,8 @@ impl Skill { let embedded_file = SkillsAsset::get(&file) .ok_or_else(|| anyhow!("Failed to load embedded skill file: {}", file.as_ref()))?; - let content = unsafe { std::str::from_utf8_unchecked(&embedded_file.data) }; + let content = std::str::from_utf8(&embedded_file.data) + .expect("bundled skill asset is not valid UTF-8"); let file_path = paths::skills_dir().join(file.as_ref()); if file_path.exists() && !force { diff --git a/src/graph/executor.rs b/src/graph/executor.rs index 9f6b6e2..a840280 100644 --- a/src/graph/executor.rs +++ b/src/graph/executor.rs @@ -1132,14 +1132,12 @@ mod integration_tests { 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}; + use crate::utils::{create_abort_signal, temp_file}; use parking_lot::Mutex; use serial_test::serial; - use std::env; use std::fs; #[cfg(unix)] use std::mem; - use std::time::{SystemTime, UNIX_EPOCH}; fn cmd_available(name: &str) -> bool { which::which(name).is_ok() @@ -1267,7 +1265,7 @@ nodes: eprintln!("skipping: bash or python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-executor-tests"); materialize_probe_agent(0.0); let ws = TestWorkspace::new(); ws.write_script("dispatcher.sh", "#!/bin/bash\necho '{}'\n"); @@ -1681,7 +1679,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-executor-tests"); materialize_probe_agent(0.0); let ws = TestWorkspace::new(); @@ -2364,47 +2362,7 @@ nodes: assert_eq!(events[0].event, "job_completed"); } - 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-graph-executor-tests-{unique}")); - fs::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 _ = fs::remove_dir_all(&self.path); - } - } + use crate::testing::TestConfigDirGuard; const PROBE_AGENT: &str = "timeout-probe"; @@ -2654,7 +2612,7 @@ nodes: eprintln!("skipping: bash or python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-executor-tests"); materialize_probe_agent(5.0); let ws = TestWorkspace::new(); // The abort must land after the worker tasks exist (their retirement diff --git a/src/graph/map.rs b/src/graph/map.rs index 514eea5..8f2b821 100644 --- a/src/graph/map.rs +++ b/src/graph/map.rs @@ -746,14 +746,12 @@ mod chain_tests { use super::*; use crate::config::paths; use crate::config::{AppState, Role, WorkingMode}; - use crate::utils::{AbortSignal, create_abort_signal, get_env_name, temp_file}; + use crate::utils::{AbortSignal, create_abort_signal, temp_file}; use indexmap::IndexMap; use serde_json::json; use serial_test::serial; - use std::env; use std::fs; use std::path::PathBuf; - use std::time::{SystemTime, UNIX_EPOCH}; fn cmd_available(name: &str) -> bool { which::which(name).is_ok() @@ -1739,47 +1737,7 @@ nodes: } } - 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-graph-map-tests-{unique}")); - fs::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 _ = fs::remove_dir_all(&self.path); - } - } + use crate::testing::TestConfigDirGuard; const PROBE_AGENT: &str = "timeout-probe"; @@ -2031,7 +1989,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_probe_agent(0.0); let h = Harness::new(&flagged_probe_graph(None)); let peers = manual_peer("worker"); @@ -2057,7 +2015,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_probe_agent(0.0); let ws = TestWorkspace::new(); let yaml = format!( @@ -2107,7 +2065,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_probe_agent(5.0); let h = Harness::new(&flagged_probe_graph(Some(1))); let (registry, assignments) = h.provision("worker", 2); @@ -2144,7 +2102,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_probe_agent(1.5); let h = Harness::new(&flagged_probe_graph(Some(0))); let peers = manual_peer("worker"); @@ -2167,7 +2125,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_probe_agent(0.0); let h = Harness::new(&format!( r#" @@ -2272,7 +2230,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_echo_inputs_agent(); let parent = echo_inputs_parent_graph(" inputs:\n width: \"{{n}}\"\n"); @@ -2291,7 +2249,7 @@ nodes: eprintln!("skipping: python3 not available"); return; } - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("graph-map-tests"); materialize_echo_inputs_agent(); let parent = echo_inputs_parent_graph(""); diff --git a/src/hooks.rs b/src/hooks.rs index e3e1d4b..2ee36fa 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -1,5 +1,7 @@ use crate::config::conflict::{self, InstallMode, StickyMode}; -use crate::config::{RequestContext, builtin_manifest, ensure_parent_exists, paths}; +use crate::config::{ + RequestContext, builtin_manifest, ensure_parent_exists, paths, set_executable_bit_if_script, +}; use crate::function::write_file_atomic; use anyhow::{Result, anyhow}; @@ -34,7 +36,8 @@ pub fn install_builtin_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Resu 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 content = std::str::from_utf8(&embedded_file.data) + .expect("bundled hook asset is not valid UTF-8"); let file_path = paths::hooks_dir().join(file.as_ref()); if file_path.exists() @@ -49,7 +52,8 @@ pub fn install_builtin_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Resu ensure_parent_exists(&file_path)?; info!("Creating hook file: {}", file_path.display()); - write_file_atomic(&file_path, content, Some(0o755))?; + write_file_atomic(&file_path, content, None)?; + set_executable_bit_if_script(&file_path)?; wrote_any = true; if !file.as_ref().contains('/') { written.insert(file.as_ref().to_string()); @@ -75,7 +79,11 @@ pub fn install_builtin_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Resu ); } - if wrote_any && paths::hooks_dir() != paths::config_dir().join("hooks") { + // Both sides canonicalize so an override that merely spells the default + // path differently (symlinks, `..`, trailing components) does not warn. + let hooks_dir = canonical_or_original(&paths::hooks_dir()); + let default_hooks_dir = canonical_or_original(&paths::config_dir().join("hooks")); + if wrote_any && hooks_dir != default_hooks_dir { warn!( "{} overrides the hooks dir: example scripts install to {}, but relative \ global-hook commands resolve against {}", @@ -88,6 +96,10 @@ pub fn install_builtin_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Resu Ok(()) } +fn canonical_or_original(path: &Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) +} + /// A single named hook: an external command to run when its event fires. #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] pub struct HookDef { @@ -437,6 +449,21 @@ async fn write_payload_file(path: &Path, json: &str) -> std::io::Result<()> { #[cfg(unix)] options.mode(0o600); let mut file = options.open(path).await?; + let written = write_payload_contents(&mut file, json).await; + if written.is_err() { + // `create_new` succeeded, so the file is ours: never leave a partial + // payload orphaned in the shared directory. + drop(file); + let _ = tokio::fs::remove_file(path).await; + } + written +} + +async fn write_payload_contents(file: &mut tokio::fs::File, json: &str) -> std::io::Result<()> { + #[cfg(test)] + if PAYLOAD_WRITE_FAILURE.load(Ordering::SeqCst) { + return Err(std::io::Error::other("injected payload write failure")); + } file.write_all(json.as_bytes()).await?; file.flush().await } @@ -444,6 +471,10 @@ async fn write_payload_file(path: &Path, json: &str) -> std::io::Result<()> { #[cfg(test)] static PAYLOAD_DIR_OVERRIDE: std::sync::Mutex> = std::sync::Mutex::new(None); +#[cfg(test)] +static PAYLOAD_WRITE_FAILURE: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + fn payload_dir() -> PathBuf { #[cfg(test)] if let Some(dir) = PAYLOAD_DIR_OVERRIDE @@ -456,6 +487,65 @@ fn payload_dir() -> PathBuf { std::env::temp_dir() } +const STALE_PAYLOAD_MAX_AGE: Duration = Duration::from_secs(24 * 60 * 60); + +/// Removes payload files left in the shared payload directory by crashed +/// processes — `run_hook` deletes its own file after the hook exits, so +/// anything older than [`STALE_PAYLOAD_MAX_AGE`] is an orphan. Runs once per +/// startup from `install_builtins`. Only exact `coyote-hook-*.json` names are +/// candidates; the directory is shared, so nothing else may ever be touched. +/// Best-effort: failures log at debug and never abort startup. +pub fn sweep_stale_payload_files() { + sweep_payload_files_older_than(STALE_PAYLOAD_MAX_AGE); +} + +fn sweep_payload_files_older_than(max_age: Duration) { + let dir = payload_dir(); + let entries = match std::fs::read_dir(&dir) { + Ok(entries) => entries, + Err(err) => { + debug!( + "Skipping stale hook-payload sweep in {}: {err}", + dir.display() + ); + return; + } + }; + let now = std::time::SystemTime::now(); + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { + continue; + }; + if !name.starts_with("coyote-hook-") || !name.ends_with(".json") { + continue; + } + // DirEntry::metadata does not traverse symlinks, so a planted link + // is skipped rather than followed. + let Ok(metadata) = entry.metadata() else { + continue; + }; + if !metadata.is_file() { + continue; + } + let stale = metadata + .modified() + .ok() + .and_then(|mtime| now.duration_since(mtime).ok()) + .is_some_and(|age| age >= max_age); + if !stale { + continue; + } + match std::fs::remove_file(entry.path()) { + Ok(()) => debug!("Removed stale hook payload file {}", entry.path().display()), + Err(err) => debug!( + "Failed to remove stale hook payload file {}: {err}", + entry.path().display() + ), + } + } +} + async fn run_hook( event: HookEvent, hook: ResolvedHook, @@ -701,6 +791,131 @@ mod tests { } } + /// Points `run_hook`'s payload directory at `path` for the guard's + /// lifetime, so payload writes can be aimed at a controlled 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; + } + } + + /// Makes `write_payload_file` fail after the `create_new` open succeeds, + /// simulating a write error on the already-created file. + struct PayloadWriteFailureGuard; + + impl PayloadWriteFailureGuard { + fn install() -> Self { + PAYLOAD_WRITE_FAILURE.store(true, Ordering::SeqCst); + Self + } + } + + impl Drop for PayloadWriteFailureGuard { + fn drop(&mut self) { + PAYLOAD_WRITE_FAILURE.store(false, Ordering::SeqCst); + } + } + + fn fresh_payload_dir(label: &str) -> PathBuf { + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let dir = env::temp_dir().join(format!("coyote-{label}-{unique}")); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[tokio::test] + #[serial] + async fn payload_write_failure_after_creation_unlinks_the_orphan() { + let dir = fresh_payload_dir("payload-orphan"); + let path = dir.join("coyote-hook-tool.started-orphan.json"); + let _fail = PayloadWriteFailureGuard::install(); + + let result = write_payload_file(&path, r#"{"probe":true}"#).await; + + assert!(result.is_err()); + assert!( + !path.exists(), + "a payload file created before the write failed must be unlinked" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[serial] + fn stale_payload_sweep_removes_only_matching_stale_files() { + let dir = fresh_payload_dir("payload-sweep"); + let _payload_dir = PayloadDirOverrideGuard::new(dir.clone()); + let stale = dir.join("coyote-hook-tool.started-abc123.json"); + std::fs::write(&stale, "{}").unwrap(); + // Everything outside the exact `coyote-hook-*.json` pattern survives, + // whatever its age: the payload dir is shared. + let bystanders = [ + dir.join("coyote-hook-tool.started-abc123.json.bak"), + dir.join("coyote-hookless.json"), + dir.join("other.json"), + dir.join("coyote-hook-note.txt"), + ]; + for path in &bystanders { + std::fs::write(path, "keep").unwrap(); + } + std::fs::create_dir_all(dir.join("coyote-hook-decoy-dir.json")).unwrap(); + + // Zero max age marks every matching file stale without depending on + // the wall clock. + sweep_payload_files_older_than(Duration::ZERO); + + assert!(!stale.exists(), "a stale payload file must be removed"); + for path in &bystanders { + assert!(path.exists(), "{} must survive the sweep", path.display()); + } + assert!(dir.join("coyote-hook-decoy-dir.json").is_dir()); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[serial] + fn stale_payload_sweep_keeps_files_younger_than_the_threshold() { + let dir = fresh_payload_dir("payload-sweep-young"); + let _payload_dir = PayloadDirOverrideGuard::new(dir.clone()); + let young = dir.join("coyote-hook-tool.started-young.json"); + std::fs::write(&young, "{}").unwrap(); + + sweep_payload_files_older_than(STALE_PAYLOAD_MAX_AGE); + + assert!( + young.exists(), + "a freshly written payload file must never be swept" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[serial] + fn stale_payload_sweep_tolerates_a_missing_directory() { + let dir = env::temp_dir().join("coyote-payload-sweep-missing-dir"); + let _ = std::fs::remove_dir_all(&dir); + let _payload_dir = PayloadDirOverrideGuard::new(dir); + + sweep_stale_payload_files(); + } + #[test] #[serial] fn install_builtin_hooks_installs_executable_scripts_and_honors_force() { @@ -930,17 +1145,17 @@ mod tests { 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"); + let echo = content.find("echo \"$safe_line\"").expect("echo fallback"); assert!(notify_send < osascript); assert!(osascript < echo); } #[test] - fn log_events_script_defaults_to_tmp_log_behind_env_override() { + fn log_events_script_defaults_to_xdg_state_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")); + assert!(content.contains("${XDG_STATE_HOME:-$HOME/.local/state}/coyote/hooks.log")); } #[cfg(unix)] @@ -979,13 +1194,24 @@ mod tests { #[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. + // A PATH exposing only tr (needed by the fallback's sanitizer) hides + // notify-send and osascript; echo is a bash builtin, so only the + // fallback branch can produce output. + let tr = ["/usr/bin/tr", "/bin/tr"] + .into_iter() + .map(Path::new) + .find(|path| path.exists()) + .expect("tr binary"); + let bin = + env::temp_dir().join(format!("coyote-notify-fallback-bin-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&bin); + std::fs::create_dir_all(&bin).unwrap(); + std::os::unix::fs::symlink(tr, bin.join("tr")).unwrap(); let mut cmd = std::process::Command::new("/bin/bash"); cmd.arg(&script) - .env("PATH", "") + .env("PATH", &bin) .env("COYOTE_EVENT", "turn.completed") - .env("COYOTE_TOOL_NAME", "demo"); + .env("COYOTE_TOOL_NAME", "demo\u{1b}]0;evil\u{7}"); // Detach from any controlling terminal so the script cannot open // /dev/tty and must fall back to captured stdout. unsafe { @@ -996,11 +1222,16 @@ mod tests { }); } let output = cmd.output().unwrap(); + let _ = std::fs::remove_dir_all(&bin); assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); assert!(stdout.contains("turn.completed"), "{stdout}"); assert!(stdout.contains("tool=demo"), "{stdout}"); + assert!( + !stdout.contains('\u{1b}') && !stdout.contains('\u{7}'), + "the fallback must strip control bytes before echoing: {stdout:?}" + ); } #[test] @@ -1508,59 +1739,10 @@ mod tests { #[cfg(unix)] mod dispatch { use super::*; - use crate::utils::get_env_name; - use std::fs::{create_dir_all, remove_dir_all}; + use crate::testing::TestConfigDirGuard; + use std::fs::create_dir_all; use std::path::PathBuf; - use std::time::{Duration, SystemTime, UNIX_EPOCH}; - - struct TestConfigDirGuard { - _env: crate::testing::EnvVarGuard, - path: PathBuf, - } - - impl TestConfigDirGuard { - fn new() -> Self { - let key = get_env_name("config_dir"); - 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(); - Self { - _env: crate::testing::EnvVarGuard::set(key, &path), - path, - } - } - } - - impl Drop for TestConfigDirGuard { - fn drop(&mut self) { - 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; - } - } + use std::time::Duration; async fn wait_for(what: &str, cond: impl Fn() -> bool) { for _ in 0..400 { @@ -1575,7 +1757,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn fire_spawns_detached_with_envs() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let out = guard.path.join("env-out"); let gate = guard.path.join("gate"); let done = guard.path.join("done"); @@ -1620,7 +1802,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn hook_stdin_is_null_not_inherited() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); 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)])); @@ -1644,7 +1826,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn payload_file_is_written_and_removed() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); 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""#; @@ -1673,7 +1855,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn drain_pending_waits_for_spawn_not_completion() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let out = guard.path.join("spawned"); let gate = guard.path.join("gate"); let done = guard.path.join("done"); @@ -1744,7 +1926,7 @@ mod tests { #[serial] async fn failing_hook_never_disturbs_the_engine() { crate::testing::install_log_collector(); - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let first = guard.path.join("first-out"); let second = guard.path.join("second-out"); let command = r#"echo err >&2; : > "$HOOK_OUT"; exit 1"#; @@ -1781,7 +1963,7 @@ mod tests { #[serial] async fn payload_file_is_created_with_owner_only_mode() { use std::os::unix::fs::PermissionsExt; - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let path = guard.path.join("payload.json"); write_payload_file(&path, r#"{"probe":true}"#) @@ -1795,7 +1977,7 @@ mod tests { #[tokio::test] #[serial] async fn payload_file_write_refuses_existing_paths() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let plain = guard.path.join("existing.json"); std::fs::write(&plain, "original").unwrap(); @@ -1814,7 +1996,7 @@ mod tests { #[serial] async fn payload_write_failure_still_runs_hook_without_payload_env() { crate::testing::install_log_collector(); - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); 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""#; @@ -1846,7 +2028,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn payload_paths_differ_across_dispatches() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); 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""#; @@ -1877,7 +2059,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn payload_paths_differ_within_one_fire() { - let guard = TestConfigDirGuard::new(); + let guard = TestConfigDirGuard::new("hooks-dispatch"); let first = guard.path.join("first-path"); let second = guard.path.join("second-path"); let command_first = r#"printf '%s' "$COYOTE_HOOK_PAYLOAD_FILE" > "$HOOK_PATH_FIRST.tmp" && mv "$HOOK_PATH_FIRST.tmp" "$HOOK_PATH_FIRST""#; @@ -1914,7 +2096,7 @@ mod tests { #[tokio::test] #[serial] async fn resolved_hooks_gates_globals_and_adds_agent_hooks_through_context() { - let _guard = TestConfigDirGuard::new(); + let _guard = TestConfigDirGuard::new("hooks-dispatch"); let mut ctx = ctx_with_global_hooks(hooks_map("tool.started", &[("notify", "global-cmd")])); let app = ctx.app.config.clone(); diff --git a/src/sandbox/mixins.rs b/src/sandbox/mixins.rs index 7021d41..015b2e9 100644 --- a/src/sandbox/mixins.rs +++ b/src/sandbox/mixins.rs @@ -197,6 +197,16 @@ pub fn discover() -> Result> { ) { out.push(read_mixin(path)?); } + // Hooks directories are deliberately scanned: hook scripts run inside the + // sandbox too, so a sidecar mixin next to one can declare its needs. The + // agents scan above never descends into `agents//hooks/`, and the + // `.builtin-manifest` living in these dirs is not matched by the + // `*.sbx-mixin.yaml` suffix filter. + for agent_dir in subdirs_of(&paths::agents_data_dir()) { + for path in collect_mixins(&agent_dir.join("hooks"), &[ScanMode::Flat]) { + out.push(read_mixin(path)?); + } + } for path in collect_mixins(&paths::rags_dir(), &[ScanMode::Flat]) { out.push(read_mixin(path)?); } @@ -206,6 +216,9 @@ pub fn discover() -> Result> { for path in collect_mixins(&paths::roles_dir().join("hooks"), &[ScanMode::Flat]) { out.push(read_mixin(path)?); } + for path in collect_mixins(&paths::hooks_dir(), &[ScanMode::Flat]) { + out.push(read_mixin(path)?); + } if let Ok(cwd) = env::current_dir() && let Some(path) = paths::find_workspace_sbx_mixin(&cwd) @@ -1005,6 +1018,51 @@ network: let _ = fs::remove_dir_all(&root); } + #[test] + #[serial_test::serial] + fn discover_scans_global_and_agent_hooks_mixins_but_never_the_manifest() { + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; + use crate::utils::get_env_name; + + let root = unique_root("discover-hooks-dirs"); + let hooks = root.join("hooks"); + let agent_hooks = root.join("agents").join("researcher").join("hooks"); + fs::create_dir_all(&hooks).unwrap(); + fs::create_dir_all(&agent_hooks).unwrap(); + let global_mixin = hooks.join("notify.sbx-mixin.yaml"); + let agent_mixin = agent_hooks.join("guard.sbx-mixin.yaml"); + fs::write(&global_mixin, "kind: mixin\n").unwrap(); + fs::write(&agent_mixin, "kind: mixin\n").unwrap(); + // The builtin installer's hidden manifest lives in both hooks dirs + // but is not a mixin and must never be picked up. + fs::write(hooks.join(BUILTIN_MANIFEST_FILE), "notify.sh\n").unwrap(); + fs::write(agent_hooks.join(BUILTIN_MANIFEST_FILE), "guard.sh\n").unwrap(); + + let _config = crate::testing::EnvVarGuard::set(get_env_name("config_dir"), &root); + let found: Vec = discover() + .unwrap() + .into_iter() + .map(|mixin| mixin.path) + .collect(); + + assert!( + found.contains(&global_mixin), + "missing global hooks mixin: {found:?}" + ); + assert!( + found.contains(&agent_mixin), + "missing agent hooks mixin: {found:?}" + ); + assert!( + found.iter().all(|path| path + .file_name() + .is_none_or(|name| name != BUILTIN_MANIFEST_FILE)), + "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 380e7cd..00957d6 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -146,3 +146,32 @@ impl Drop for EnvVarGuard { } } } + +/// Points the config dir at a fresh temp directory for the guard's lifetime +/// and removes it on drop, including on panic. Tests using it must serialize +/// (`#[serial]`) — the config-dir env var is process-global. +pub(crate) struct TestConfigDirGuard { + _env: EnvVarGuard, + pub(crate) path: std::path::PathBuf, +} + +impl TestConfigDirGuard { + pub(crate) fn new(label: &str) -> Self { + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let path = std::env::temp_dir().join(format!("coyote-{label}-{unique}")); + std::fs::create_dir_all(&path).unwrap(); + Self { + _env: EnvVarGuard::set(crate::utils::get_env_name("config_dir"), &path), + path, + } + } +} + +impl Drop for TestConfigDirGuard { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +}