diff --git a/scripts/usage-probe-hooks.sh b/scripts/usage-probe-hooks.sh index 7096535..6246bb5 100755 --- a/scripts/usage-probe-hooks.sh +++ b/scripts/usage-probe-hooks.sh @@ -9,10 +9,16 @@ # 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. +# 0755 into /hooks via the real CLI; a non-interactive re-run +# keeps locally modified scripts while still installing missing ones. # D) `coyote --agent --info` (an inspection flag) fires ZERO # agent.* hook events. +# E) `coyote --install-builtins hooks` reconciles manifest-owned leftovers +# in roles/hooks and each bundled agent's hooks dir, sparing user files. +# F) top-level hooks-dir manifest reconcile: a stale shipped hook is removed +# on reinstall; a malformed manifest deletes NOTHING (fail-safe). +# G) a plain startup sweeps manifest-owned hooks of removed builtin agents +# while leaving user agent dirs and user files untouched. # # 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 @@ -143,16 +149,20 @@ if command -v stat > /dev/null 2>&1; then [ "$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. +# Modify one script locally and delete the other, then re-run +# --install-builtins hooks: without a terminal the conflict resolution keeps +# local files, so the edit must survive while the missing script is +# reinstalled from the packaged content. echo "# LOCALLY MODIFIED" > "$CFG_C/hooks/notify.sh" +rm "$CFG_C/hooks/log-events.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" + || fail "non-interactive reinstall overwrote the locally modified notify.sh" +[ -f "$CFG_C/hooks/log-events.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" echo "== Scenario C2: cold-start auto-bootstrap fills the hooks dir but never forces ==" CFG_C2="$WORKDIR/c2" @@ -228,5 +238,102 @@ 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 "== Scenario E: --install-builtins hooks reconciles roles/hooks and agent hooks dirs ==" +CFG_E="$WORKDIR/e" +AGENT_E="$(ls "$REPO_ROOT/assets/agents" | head -1)" +mkdir -p "$CFG_E/roles/hooks" "$CFG_E/agents/$AGENT_E/hooks" +# Manifest-owned leftovers a previous release shipped, plus user files that +# must survive the refresh in both locations. +printf 'stale-role.sh\n' > "$CFG_E/roles/hooks/.builtin-manifest" +echo "stale" > "$CFG_E/roles/hooks/stale-role.sh" +echo "mine" > "$CFG_E/roles/hooks/user-role.sh" +printf 'stale-agent.sh\n' > "$CFG_E/agents/$AGENT_E/hooks/.builtin-manifest" +echo "stale" > "$CFG_E/agents/$AGENT_E/hooks/stale-agent.sh" +echo "mine" > "$CFG_E/agents/$AGENT_E/hooks/user-agent.sh" + +COYOTE_CONFIG_DIR="$CFG_E" "$BIN" --install-builtins hooks > "$WORKDIR/install_e.out" 2>&1 \ + || fail "--install-builtins hooks failed: $(cat "$WORKDIR/install_e.out")" + +[ -f "$CFG_E/hooks/notify.sh" ] || fail "global hooks were not installed" +[ ! -f "$CFG_E/roles/hooks/stale-role.sh" ] \ + || fail "manifest-owned stale role hook survived the refresh" +[ "$(cat "$CFG_E/roles/hooks/user-role.sh")" = "mine" ] \ + || fail "user file in roles/hooks was touched by the refresh" +[ ! -f "$CFG_E/agents/$AGENT_E/hooks/stale-agent.sh" ] \ + || fail "manifest-owned stale agent hook survived the refresh" +[ "$(cat "$CFG_E/agents/$AGENT_E/hooks/user-agent.sh")" = "mine" ] \ + || fail "user file in the agent hooks dir was touched by the refresh" +echo "PASS: refresh reconciles roles/hooks and bundled-agent hooks, sparing user files" + +echo "== Scenario F: top-level manifest reconcile removes stale hooks; malformed manifest deletes nothing ==" +CFG_F="$WORKDIR/f" +mkdir -p "$CFG_F" +COYOTE_CONFIG_DIR="$CFG_F" "$BIN" --install-builtins hooks > /dev/null 2>&1 +[ -f "$CFG_F/hooks/.builtin-manifest" ] \ + || fail "install did not write a .builtin-manifest to the top-level hooks dir" + +# Simulate an upgrade from a release that also shipped old-hook.sh: the +# manifest records it, so reinstall must remove it — while a user script +# absent from the manifest survives. +printf 'log-events.sh\nnotify.sh\nold-hook.sh\n' > "$CFG_F/hooks/.builtin-manifest" +echo "stale" > "$CFG_F/hooks/old-hook.sh" +echo "mine" > "$CFG_F/hooks/user-hook.sh" +COYOTE_CONFIG_DIR="$CFG_F" "$BIN" --install-builtins hooks > /dev/null 2>&1 +[ ! -f "$CFG_F/hooks/old-hook.sh" ] \ + || fail "stale shipped hook recorded in the manifest was not removed on reinstall" +[ "$(cat "$CFG_F/hooks/user-hook.sh")" = "mine" ] \ + || fail "user hook script was deleted or modified by manifest reconcile" +grep -q 'old-hook.sh' "$CFG_F/hooks/.builtin-manifest" \ + && fail "manifest still lists the removed stale hook" + +# Fail-safe: a malformed (non-UTF8) manifest must delete NOTHING and must +# 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")" +[ -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" ] \ + || fail "shipped hooks missing after malformed-manifest reinstall" +echo "PASS: stale manifest-owned hook removed; user files survive; malformed manifest deletes nothing" + +echo "== Scenario G: plain startup sweeps removed-builtin-agent hooks, spares user agents ==" +CFG_G="$WORKDIR/g" +mkdir -p "$CFG_G" +write_dryrun_config "$CFG_G" +# ghost: a removed builtin agent (has a manifest, absent from the embed) +# with a user file beside the manifest-owned hook -> hooks dir survives +# with only the user file. +mkdir -p "$CFG_G/agents/ghost/hooks" +printf 'shipped.sh\n' > "$CFG_G/agents/ghost/hooks/.builtin-manifest" +echo "stale" > "$CFG_G/agents/ghost/hooks/shipped.sh" +echo "mine" > "$CFG_G/agents/ghost/hooks/user.sh" +# ghost2: same, but nothing user-owned -> the emptied hooks dir is removed. +mkdir -p "$CFG_G/agents/ghost2/hooks" +printf 'shipped.sh\n' > "$CFG_G/agents/ghost2/hooks/.builtin-manifest" +echo "stale" > "$CFG_G/agents/ghost2/hooks/shipped.sh" +# mine: a user agent with no manifest -> never touched. +mkdir -p "$CFG_G/agents/mine/hooks" +echo "mine" > "$CFG_G/agents/mine/hooks/mine.sh" + +COYOTE_CONFIG_DIR="$CFG_G" "$BIN" --no-stream "say exactly: hi" > /dev/null + +[ ! -f "$CFG_G/agents/ghost/hooks/shipped.sh" ] \ + || fail "removed-agent sweep left the manifest-owned hook behind" +[ ! -f "$CFG_G/agents/ghost/hooks/.builtin-manifest" ] \ + || fail "removed-agent sweep left the manifest behind" +[ "$(cat "$CFG_G/agents/ghost/hooks/user.sh")" = "mine" ] \ + || fail "removed-agent sweep deleted a user file" +[ ! -d "$CFG_G/agents/ghost2/hooks" ] \ + || fail "emptied hooks dir of a removed agent was not removed" +[ -d "$CFG_G/agents/ghost2" ] \ + || fail "sweep removed more than the hooks dir of a removed agent" +[ "$(cat "$CFG_G/agents/mine/hooks/mine.sh")" = "mine" ] \ + || fail "sweep touched a user agent dir without a manifest" +echo "PASS: startup sweep removes only manifest-owned hooks of removed agents" + echo echo "ALL SCENARIOS PASSED" diff --git a/src/config/agent.rs b/src/config/agent.rs index 00ed166..58e208c 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -14,11 +14,13 @@ use crate::{ use super::rag_cache::RagKey; use crate::config::builtin_manifest; +use crate::config::conflict::{self, InstallMode, StickyMode}; use crate::config::paths; use crate::config::prompts::{ DEFAULT_JOB_INSTRUCTIONS, DEFAULT_SPAWN_INSTRUCTIONS, DEFAULT_TEAMMATE_INSTRUCTIONS, DEFAULT_TODO_INSTRUCTIONS, DEFAULT_USER_INTERACTION_INSTRUCTIONS, }; +use crate::function::write_file_atomic; use crate::graph::types::RagNode; use crate::graph::{Graph, GraphParser, NodeType}; use crate::mcp::McpServerFeatures; @@ -29,7 +31,7 @@ use fancy_regex::Captures; use inquire::{Text, validator::Validation}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::{env, ffi::OsStr, path::Path}; const DEFAULT_AGENT_NAME: &str = "rag"; @@ -63,6 +65,91 @@ fn direct_hook_name(rest: &str) -> Option<&str> { (!name.is_empty() && !name.contains('/')).then_some(name) } +/// Installs one bundled agent's hook scripts under `/hooks/` and +/// reconciles that directory through its builtin manifest, mirroring the +/// role-side hook installer. +pub(crate) fn install_and_reconcile_agent_hooks( + agent: &str, + shipped: &[(String, String)], + mode: InstallMode, + sticky: &mut StickyMode, +) -> Result<()> { + let dir = paths::agents_data_dir().join(agent).join("hooks"); + let mut written = BTreeSet::new(); + for (name, content) in shipped { + let path = dir.join(name); + if path.exists() + && !conflict::should_replace_existing(&path, content, "agents", mode, sticky)? + { + debug!( + "Agent hook file already exists, skipping: {}", + path.display() + ); + continue; + } + ensure_parent_exists(&path)?; + info!("Creating agent 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 the install, matching the role-side policy. + if let Err(err) = builtin_manifest::reconcile_builtin_dir(&dir, &names, &written) { + warn!("Failed to reconcile builtin hooks for agent '{agent}': {err}"); + } + Ok(()) +} + +/// An agent the embed stopped bundling is never visited by the per-agent +/// reconcile loop, so its installed hooks and manifest would orphan forever. +/// Any agent directory that carries a builtin hooks manifest but is absent +/// from the current embed gets its manifest-owned hooks removed, each +/// deletion logged individually by the reconcile. Only the installer writes +/// manifests, but a manual copy of a builtin agent directory carries the +/// hidden manifest along: files it lists are treated as installer-owned and +/// swept, while files absent from the manifest always survive. +/// Best-effort: a failure here must not abort the install. +fn sweep_removed_agent_hooks(bundled_files: &HashMap>) { + let entries = match std::fs::read_dir(paths::agents_data_dir()) { + Ok(entries) => entries, + Err(err) => { + warn!("Failed to scan agents dir for removed-agent hooks: {err}"); + return; + } + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { + continue; + }; + if bundled_files.contains_key(name) { + continue; + } + let hooks_dir = entry.path().join("hooks"); + if !hooks_dir + .join(builtin_manifest::BUILTIN_MANIFEST_FILE) + .is_file() + { + continue; + } + info!( + "Removing hooks shipped by removed built-in agent '{name}' in {}", + hooks_dir.display() + ); + if let Err(err) = + builtin_manifest::reconcile_builtin_dir(&hooks_dir, &BTreeSet::new(), &BTreeSet::new()) + { + warn!("Failed to reconcile builtin hooks for removed agent '{name}': {err}"); + continue; + } + // remove_dir only succeeds on an empty directory, so a user file + // left behind keeps the directory in place. + let _ = std::fs::remove_dir(&hooks_dir); + } +} + #[derive(Debug, Clone)] pub struct Agent { name: String, @@ -81,12 +168,16 @@ pub struct Agent { } impl Agent { - pub fn install_builtin_agents(force: bool) -> Result<()> { + pub fn install_builtin_agents(mode: InstallMode) -> Result<()> { info!( "Installing built-in agents in {}", paths::agents_data_dir().display() ); + // Whole-agent installs run Skip (startup) or Force (reinstall); a + // Prompt caller would still get coherent per-file conflict handling, + // scoped to this run. + let mut sticky = StickyMode::None; let mut written_hooks: HashMap> = HashMap::new(); for file in AgentAssets::iter() { debug!("Processing agent file: {}", file.as_ref()); @@ -102,7 +193,15 @@ impl Agent { #[cfg_attr(not(unix), expect(unused))] let is_script = matches!(file_extension.as_deref(), Some("sh") | Some("py")); - if file_path.exists() && !force { + if file_path.exists() + && !conflict::should_replace_existing( + &file_path, + content, + "agents", + mode, + &mut sticky, + )? + { debug!( "Agent file already exists, skipping: {}", file_path.display() @@ -179,6 +278,38 @@ impl Agent { } } + sweep_removed_agent_hooks(&bundled_files); + + Ok(()) + } + + /// Refreshes only the bundled agents' hook scripts, leaving every other + /// agent file alone: the hooks asset category must be able to update all + /// hook locations without reinstalling whole agents. + /// Every bundled agent's hooks/ directory reconciles, even when the + /// current embed ships no hooks for it. + pub fn install_builtin_agent_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Result<()> { + // Every bundled agent gets an entry, hooks or not: an agent whose + // hook assets were all dropped still needs its hooks/ directory + // reconciled so manifest-owned leftovers are removed. + let mut shipped: BTreeMap> = BTreeMap::new(); + for file in AgentAssets::iter() { + let Some((agent, rest)) = file.as_ref().split_once('/') else { + continue; + }; + let hooks = shipped.entry(agent.to_string()).or_default(); + let Some(hook) = direct_hook_name(rest) else { + continue; + }; + 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) }; + hooks.push((hook.to_string(), content.to_string())); + } + + for (agent, files) in &shipped { + install_and_reconcile_agent_hooks(agent, files, mode, sticky)?; + } Ok(()) } @@ -1926,4 +2057,167 @@ 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); + } + } + + fn fixture(entries: &[(&str, &str)]) -> Vec<(String, String)> { + entries + .iter() + .map(|(name, content)| (name.to_string(), content.to_string())) + .collect() + } + + #[test] + #[serial_test::serial] + fn agent_hooks_prompt_mode_asks_per_conflict_and_honors_sticky() { + use crate::config::conflict::prompt_script; + + let _guard = TestConfigDirGuard::new("agent-hooks-prompt"); + let shipped = fixture(&[("a.sh", "new content\n"), ("b.sh", "new content\n")]); + let dir = paths::agents_data_dir().join("probe").join("hooks"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("a.sh"), "local a").unwrap(); + std::fs::write(dir.join("b.sh"), "local b").unwrap(); + + // First conflict answered per-file, second by the sticky replace-all. + let script = prompt_script::install(&["replace-all"]); + let mut sticky = StickyMode::None; + install_and_reconcile_agent_hooks("probe", &shipped, InstallMode::Prompt, &mut sticky) + .unwrap(); + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "new content\n"); + assert_eq!(read_to_string(dir.join("b.sh")).unwrap(), "new content\n"); + assert_eq!(sticky, StickyMode::ReplaceAll); + assert_eq!( + read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "a.sh\nb.sh\n" + ); + drop(script); + + // A sticky mode carried in from an earlier location suppresses all + // prompting: `--install-builtins hooks` shares one sticky scope + // across the global, role, and agent hook locations. + std::fs::write(dir.join("a.sh"), "local again").unwrap(); + let script = prompt_script::install(&[]); + let mut sticky = StickyMode::KeepAll; + install_and_reconcile_agent_hooks("probe", &shipped, InstallMode::Prompt, &mut sticky) + .unwrap(); + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "local again"); + drop(script); + } + + #[test] + #[serial_test::serial] + fn agent_hooks_reconcile_adds_updates_and_drops_within_a_shipped_agent() { + let _guard = TestConfigDirGuard::new("agent-hooks-reconcile"); + let dir = paths::agents_data_dir().join("probe").join("hooks"); + + install_and_reconcile_agent_hooks( + "probe", + &fixture(&[("keep.sh", "v1\n"), ("drop.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .unwrap(); + std::fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + install_and_reconcile_agent_hooks( + "probe", + &fixture(&[("keep.sh", "v2\n"), ("added.sh", "#!/bin/sh\n")]), + InstallMode::Force, + &mut StickyMode::None, + ) + .unwrap(); + + assert_eq!( + read_to_string(dir.join("keep.sh")).unwrap(), + "v2\n", + "a still-shipped hook updates under Force" + ); + assert_eq!( + read_to_string(dir.join("added.sh")).unwrap(), + "#!/bin/sh\n", + "a newly shipped hook is added" + ); + assert!( + !dir.join("drop.sh").exists(), + "a dropped hook reconciles away" + ); + assert_eq!( + read_to_string(dir.join("user.sh")).unwrap(), + "user-owned", + "user files in the same dir survive" + ); + assert_eq!( + read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "added.sh\nkeep.sh\n" + ); + } + + #[test] + #[serial_test::serial] + fn install_builtin_agent_hooks_reconciles_hookless_bundled_agents() { + let _guard = TestConfigDirGuard::new("agent-hooks-hookless"); + let agent = AgentAssets::iter() + .filter_map(|file| { + file.as_ref() + .split_once('/') + .map(|(agent, _)| agent.to_string()) + }) + .find(|agent| { + !AgentAssets::iter().any(|file| { + parse_direct_hook_path(file.as_ref()) + .is_some_and(|(hook_agent, _)| hook_agent == agent) + }) + }) + .expect("at least one bundled agent ships no hooks"); + let dir = paths::agents_data_dir().join(&agent).join("hooks"); + // A manifest-owned hook left over from a release that shipped it. + install_and_reconcile_agent_hooks( + &agent, + &fixture(&[("stale.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .unwrap(); + std::fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + Agent::install_builtin_agent_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + + assert!( + !dir.join("stale.sh").exists(), + "a still-bundled agent with no hook assets must reconcile its hooks dir" + ); + assert_eq!(read_to_string(dir.join("user.sh")).unwrap(), "user-owned"); + } } diff --git a/src/config/conflict.rs b/src/config/conflict.rs new file mode 100644 index 0000000..2ff7f1e --- /dev/null +++ b/src/config/conflict.rs @@ -0,0 +1,379 @@ +//! Shared keep/replace conflict resolution for installers that must not +//! silently clobber locally modified files. The bundle installer and the +//! builtin hook installers all funnel per-file conflicts through +//! [`resolve_conflict`], so the prompt wording and the sticky +//! keep-all/replace-all semantics are identical everywhere — including when +//! one sticky scope spans several install locations in a single run. + +use crate::utils::IS_STDOUT_TERMINAL; + +use anyhow::{Context, Result, bail}; +use inquire::Select; +use std::fs; +use std::path::Path; + +/// How an installer treats a destination file that already exists. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum InstallMode { + /// Leave existing files untouched. The only mode startup ever uses: + /// startup runs on every launch — including the REPL, which is a + /// terminal — so it must never prompt. + Skip, + /// Overwrite existing files unconditionally. + Force, + /// Write missing files, silently skip identical ones, and ask per + /// differing file. + Prompt, +} + +/// A `keep-all`/`replace-all` answer outlives the file it was given for: +/// the caller threads one value through every subsequent conflict in the +/// same run. +#[derive(Debug, Clone, Copy, PartialEq)] +pub(crate) enum StickyMode { + None, + KeepAll, + ReplaceAll, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConflictAction { + Keep, + Replace, +} + +/// What [`resolve_conflict`] does without a terminal: bundle installs bail +/// (an unattended install must not half-apply a plan silently), builtin hook +/// refreshes keep the local file (a scripted refresh must neither clobber +/// local changes nor fail). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum NonInteractive { + Bail, + Keep, +} + +/// Decides whether an installer should overwrite the existing file at `dst` +/// with `content` under `mode`. Only called for files that already exist; +/// missing files are always written without prompting. In `Prompt` mode an +/// unreadable local file counts as differing, so the user decides. +pub(crate) fn should_replace_existing( + dst: &Path, + content: &str, + category: &str, + mode: InstallMode, + sticky: &mut StickyMode, +) -> Result { + match mode { + InstallMode::Skip => Ok(false), + InstallMode::Force => Ok(true), + InstallMode::Prompt => { + if fs::read(dst).is_ok_and(|local| local == content.as_bytes()) { + return Ok(false); + } + match resolve_conflict(dst, category, sticky, NonInteractive::Keep)? { + ConflictAction::Keep => Ok(false), + ConflictAction::Replace => Ok(true), + } + } + } +} + +pub(crate) fn resolve_conflict( + dst: &Path, + category: &str, + sticky: &mut StickyMode, + non_interactive: NonInteractive, +) -> Result { + match *sticky { + StickyMode::KeepAll => return Ok(ConflictAction::Keep), + StickyMode::ReplaceAll => return Ok(ConflictAction::Replace), + StickyMode::None => {} + } + + if !is_interactive() { + match non_interactive { + NonInteractive::Keep => return Ok(ConflictAction::Keep), + NonInteractive::Bail => bail!( + "Refusing to overwrite local file {} non-interactively. \ + Re-run in a terminal, with --install-force (installs), \ + or with --yes (updates).", + dst.display() + ), + } + } + + let prompt = format!("Conflict at {} (category: {category})", dst.display()); + let choice = select_choice(&prompt)?; + + match choice.as_str() { + "keep" => Ok(ConflictAction::Keep), + "replace" => Ok(ConflictAction::Replace), + "keep-all" => { + *sticky = StickyMode::KeepAll; + Ok(ConflictAction::Keep) + } + "replace-all" => { + *sticky = StickyMode::ReplaceAll; + Ok(ConflictAction::Replace) + } + "abort" => bail!("Install aborted by user at conflict resolution."), + _ => unreachable!("inquire::Select returned an unexpected option"), + } +} + +fn is_interactive() -> bool { + #[cfg(test)] + if let Some(forced) = prompt_script::forced_tty() { + return forced; + } + *IS_STDOUT_TERMINAL +} + +fn select_choice(prompt: &str) -> Result { + #[cfg(test)] + if prompt_script::active() { + return Ok(prompt_script::next_answer(prompt)); + } + Select::new( + prompt, + vec!["keep", "replace", "keep-all", "replace-all", "abort"], + ) + .prompt() + .map(str::to_string) + .with_context(|| "failed to read conflict choice") +} + +#[cfg(test)] +pub(crate) mod prompt_script { + //! Scripted stand-in for the interactive conflict prompt. + //! `inquire::Select` cannot run under the test harness, so while a guard + //! is installed the resolver takes its terminal answer from the guard and + //! serves each prompt from a queue of scripted answers, counting every + //! prompt asked. A forced-terminal guard with an empty queue pins the + //! "never prompts" invariant: any prompt panics and the counter stays 0. + + use std::sync::Mutex; + use std::sync::atomic::{AtomicUsize, Ordering}; + + const INACTIVE: usize = 0; + const TTY: usize = 1; + const NON_TTY: usize = 2; + + static STATE: AtomicUsize = AtomicUsize::new(INACTIVE); + static ASKED: AtomicUsize = AtomicUsize::new(0); + // A test that panics while holding the queue poisons its mutex; every + // lock site recovers via `into_inner` so later tests keep working. + static ANSWERS: Mutex> = Mutex::new(Vec::new()); + + /// Installs the script with a forced terminal until the guard drops. + /// The script is process-global, so tests using it must serialize + /// (`#[serial]`). Answers are consumed front to back; a prompt beyond + /// the scripted answers panics. + #[must_use] + pub fn install(answers: &[&str]) -> ScriptGuard { + install_with_state(TTY, answers) + } + + /// Installs the script with the terminal forced absent, for pinning the + /// non-interactive behavior regardless of where the tests actually run. + #[must_use] + pub fn install_non_interactive() -> ScriptGuard { + install_with_state(NON_TTY, &[]) + } + + fn install_with_state(state: usize, answers: &[&str]) -> ScriptGuard { + *ANSWERS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) = + answers.iter().map(|answer| answer.to_string()).collect(); + ASKED.store(0, Ordering::SeqCst); + STATE.store(state, Ordering::SeqCst); + ScriptGuard + } + + pub struct ScriptGuard; + + impl Drop for ScriptGuard { + fn drop(&mut self) { + STATE.store(INACTIVE, Ordering::SeqCst); + ANSWERS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clear(); + } + } + + /// Number of prompts asked since the script was installed. + pub fn prompts_asked() -> usize { + ASKED.load(Ordering::SeqCst) + } + + pub(super) fn active() -> bool { + STATE.load(Ordering::SeqCst) != INACTIVE + } + + pub(super) fn forced_tty() -> Option { + match STATE.load(Ordering::SeqCst) { + TTY => Some(true), + NON_TTY => Some(false), + _ => None, + } + } + + pub(super) fn next_answer(prompt: &str) -> String { + ASKED.fetch_add(1, Ordering::SeqCst); + let mut answers = ANSWERS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + assert!( + !answers.is_empty(), + "conflict prompt asked with no scripted answer left: {prompt}" + ); + answers.remove(0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serial_test::serial; + use std::path::PathBuf; + + fn dst() -> PathBuf { + PathBuf::from("/tmp/coyote-conflict-tests/example.yaml") + } + + #[test] + fn sticky_modes_short_circuit_without_prompting() { + let mut sticky = StickyMode::KeepAll; + assert!(matches!( + resolve_conflict(&dst(), "macros", &mut sticky, NonInteractive::Bail).unwrap(), + ConflictAction::Keep + )); + + let mut sticky = StickyMode::ReplaceAll; + assert!(matches!( + resolve_conflict(&dst(), "macros", &mut sticky, NonInteractive::Bail).unwrap(), + ConflictAction::Replace + )); + } + + #[test] + #[serial] + fn non_interactive_bail_keeps_the_bundle_error_shape() { + let _script = prompt_script::install_non_interactive(); + let mut sticky = StickyMode::None; + + let err = + resolve_conflict(&dst(), "macros", &mut sticky, NonInteractive::Bail).unwrap_err(); + + assert!( + err.to_string().contains("Refusing to overwrite local file"), + "got: {err}" + ); + assert!(err.to_string().contains("example.yaml"), "got: {err}"); + assert_eq!(prompt_script::prompts_asked(), 0); + } + + #[test] + #[serial] + fn non_interactive_keep_resolves_without_error_or_prompt() { + let _script = prompt_script::install_non_interactive(); + let mut sticky = StickyMode::None; + + let action = resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap(); + + assert!(matches!(action, ConflictAction::Keep)); + assert_eq!(sticky, StickyMode::None); + assert_eq!(prompt_script::prompts_asked(), 0); + } + + #[test] + #[serial] + fn scripted_answers_drive_actions_and_sticky_transitions() { + let _script = prompt_script::install(&["keep", "replace", "replace-all"]); + let mut sticky = StickyMode::None; + + assert!(matches!( + resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap(), + ConflictAction::Keep + )); + assert_eq!(sticky, StickyMode::None); + assert!(matches!( + resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap(), + ConflictAction::Replace + )); + assert_eq!(sticky, StickyMode::None); + assert!(matches!( + resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap(), + ConflictAction::Replace + )); + assert_eq!(sticky, StickyMode::ReplaceAll); + // The sticky answer now short-circuits: no further prompt is asked. + assert!(matches!( + resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap(), + ConflictAction::Replace + )); + assert_eq!(prompt_script::prompts_asked(), 3); + } + + #[test] + #[serial] + fn abort_answer_bails_with_the_bundle_abort_message() { + let _script = prompt_script::install(&["abort"]); + let mut sticky = StickyMode::None; + + let err = resolve_conflict(&dst(), "hooks", &mut sticky, NonInteractive::Keep).unwrap_err(); + + assert!( + err.to_string() + .contains("Install aborted by user at conflict resolution."), + "got: {err}" + ); + } + + #[test] + #[serial] + fn should_replace_existing_skips_identical_content_without_prompting() { + let _script = prompt_script::install(&[]); + let dir = crate::utils::temp_file("conflict-identical-", ""); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("hook.sh"); + std::fs::write(&path, "same content").unwrap(); + + let mut sticky = StickyMode::None; + let replace = should_replace_existing( + &path, + "same content", + "hooks", + InstallMode::Prompt, + &mut sticky, + ) + .unwrap(); + + let _ = std::fs::remove_dir_all(&dir); + assert!(!replace); + assert_eq!(prompt_script::prompts_asked(), 0); + } + + #[test] + #[serial] + fn should_replace_existing_never_prompts_in_skip_or_force_mode() { + let _script = prompt_script::install(&[]); + let dir = crate::utils::temp_file("conflict-modes-", ""); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("hook.sh"); + std::fs::write(&path, "local edit").unwrap(); + + let mut sticky = StickyMode::None; + let skip = + should_replace_existing(&path, "shipped", "hooks", InstallMode::Skip, &mut sticky); + let force = + should_replace_existing(&path, "shipped", "hooks", InstallMode::Force, &mut sticky); + + let _ = std::fs::remove_dir_all(&dir); + assert!(!skip.unwrap()); + assert!(force.unwrap()); + assert_eq!(prompt_script::prompts_asked(), 0); + } +} diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 01d9424..4ebbee1 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -3,6 +3,7 @@ use super::bundles::{ hash_file, }; 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; @@ -1886,18 +1887,6 @@ fn count_kind(plan: &InstallPlan, cat: TopCategory, kind: PlannedKind) -> usize .count() } -#[derive(Debug, Clone, Copy, PartialEq)] -enum StickyMode { - None, - KeepAll, - ReplaceAll, -} - -enum ConflictAction { - Keep, - Replace, -} - #[derive(Debug)] struct ApplyReport { new_count: usize, @@ -1937,7 +1926,12 @@ fn apply_plan( record_written_file(store, bundle, planned, FileAction::Replaced)?; report.refreshed_count += 1; } - PlannedKind::Conflict => match resolve_conflict(planned, &mut sticky)? { + PlannedKind::Conflict => match resolve_conflict( + &planned.dst, + planned.top_category.label(), + &mut sticky, + NonInteractive::Bail, + )? { ConflictAction::Keep => report.kept_count += 1, ConflictAction::Replace => { write_file(&planned.src, &planned.dst)?; @@ -2037,50 +2031,6 @@ fn record_mcp_merge(store: &mut BundleStore, bundle: &str, report: &McpMergeRepo store.record_mcp_servers(bundle, entries) } -fn resolve_conflict(planned: &PlannedFile, sticky: &mut StickyMode) -> Result { - match *sticky { - StickyMode::KeepAll => return Ok(ConflictAction::Keep), - StickyMode::ReplaceAll => return Ok(ConflictAction::Replace), - StickyMode::None => {} - } - - if !*IS_STDOUT_TERMINAL { - bail!( - "Refusing to overwrite local file {} non-interactively. \ - Re-run in a terminal, with --install-force (installs), \ - or with --yes (updates).", - planned.dst.display() - ); - } - - let prompt = format!( - "Conflict at {} (category: {})", - planned.dst.display(), - planned.top_category.label() - ); - let choice = Select::new( - &prompt, - vec!["keep", "replace", "keep-all", "replace-all", "abort"], - ) - .prompt() - .with_context(|| "failed to read conflict choice")?; - - match choice { - "keep" => Ok(ConflictAction::Keep), - "replace" => Ok(ConflictAction::Replace), - "keep-all" => { - *sticky = StickyMode::KeepAll; - Ok(ConflictAction::Keep) - } - "replace-all" => { - *sticky = StickyMode::ReplaceAll; - Ok(ConflictAction::Replace) - } - "abort" => bail!("Install aborted by user at conflict resolution."), - _ => unreachable!("inquire::Select returned an unexpected option"), - } -} - fn write_file(src: &Path, dst: &Path) -> Result<()> { if let Some(parent) = dst.parent() { fs::create_dir_all(parent) diff --git a/src/config/mod.rs b/src/config/mod.rs index 497c679..e5d5885 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -3,6 +3,7 @@ mod app_config; mod app_state; pub(crate) mod builtin_manifest; mod bundles; +pub(crate) mod conflict; mod input; mod install_remote; pub(crate) mod instructions; @@ -37,6 +38,7 @@ pub use self::app_config::AppConfig; pub use self::app_state::AppState; pub(crate) use self::bundles::installed_bundle_names; pub use self::bundles::list_installed_bundles; +use self::conflict::{InstallMode, StickyMode}; pub use self::input::Input; pub use self::install_remote::{ DEFAULT_GIT_HOST, install_or_update, install_or_update_from_repl_args, uninstall_bundle, @@ -418,11 +420,11 @@ impl Default for Config { pub fn install_builtins() -> Result<()> { Functions::install_builtin_global_tools(false)?; - Agent::install_builtin_agents(false)?; + Agent::install_builtin_agents(InstallMode::Skip)?; Macro::install_macros(false)?; Skill::install_builtin_skills(false)?; - hooks::install_builtin_hooks(false)?; - Role::install_builtin_role_hooks(false)?; + hooks::install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None)?; + Role::install_builtin_role_hooks(InstallMode::Skip, &mut StickyMode::None)?; Ok(()) } @@ -519,11 +521,19 @@ pub fn install_assets(category: AssetCategory) -> Result<()> { } match category { - AssetCategory::Agents => Agent::install_builtin_agents(true)?, + AssetCategory::Agents => Agent::install_builtin_agents(InstallMode::Force)?, 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::Hooks => { + // One sticky scope spans all three hook locations, so a + // keep-all/replace-all answer carries through the whole refresh, + // matching bundle-install semantics. + let mut sticky = StickyMode::None; + hooks::install_builtin_hooks(InstallMode::Prompt, &mut sticky)?; + Role::install_builtin_role_hooks(InstallMode::Prompt, &mut sticky)?; + Agent::install_builtin_agent_hooks(InstallMode::Prompt, &mut sticky)?; + } AssetCategory::McpConfig => Functions::install_mcp_config()?, } @@ -544,6 +554,14 @@ fn confirm_asset_overwrite(category: AssetCategory, label: &str, target: &Path) references) are left untouched.", target.display() ), + AssetCategory::Hooks => format!( + "Refreshing bundled hooks installs any missing bundled hook \ + scripts in {}, the roles hooks directory, and each bundled \ + agent's hooks directory. Where a local copy differs from the \ + bundled version you are asked per file whether to keep or \ + replace it; non-interactive runs keep local files.", + target.display() + ), _ => format!( "Reinstalling bundled {label} overwrites every bundled {label} in \ {} with this build's packaged versions. Local changes to bundled \ @@ -1805,4 +1823,143 @@ hooks: std::panic::resume_unwind(panic); } } + + /// 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); + } + } + + #[test] + #[serial_test::serial] + fn install_assets_hooks_shares_one_sticky_scope_across_all_three_locations() { + use crate::config::conflict::prompt_script; + + let _guard = TestConfigDirGuard::new("hooks-three-locations"); + let notify = paths::hooks_dir().join("notify.sh"); + let role_dir = paths::roles_dir().join("hooks"); + let agent_dir = paths::agents_data_dir().join("probe").join("hooks"); + let shipped = vec![("hook.sh".to_string(), "shipped\n".to_string())]; + + // Drives the exact three installers `install_assets(Hooks)` runs, + // threading one sticky scope through all of them. + let refresh = |sticky: &mut StickyMode| { + hooks::install_builtin_hooks(InstallMode::Prompt, sticky).unwrap(); + role::install_and_reconcile_role_hooks( + &role_dir, + &shipped, + InstallMode::Prompt, + sticky, + ) + .unwrap(); + agent::install_and_reconcile_agent_hooks( + "probe", + &shipped, + InstallMode::Prompt, + sticky, + ) + .unwrap(); + }; + let modify_all = || { + fs::write(¬ify, "local global").unwrap(); + fs::write(role_dir.join("hook.sh"), "local role").unwrap(); + fs::write(agent_dir.join("hook.sh"), "local agent").unwrap(); + }; + + let script = prompt_script::install(&[]); + refresh(&mut StickyMode::None); + assert_eq!( + prompt_script::prompts_asked(), + 0, + "a clean install asks nothing" + ); + drop(script); + + // One replace-all at the first (global) conflict must silently + // replace the role and agent conflicts too. + modify_all(); + let script = prompt_script::install(&["replace-all"]); + let mut sticky = StickyMode::None; + refresh(&mut sticky); + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(sticky, StickyMode::ReplaceAll); + assert!( + fs::read_to_string(¬ify) + .unwrap() + .starts_with("#!/usr/bin/env bash") + ); + assert_eq!( + fs::read_to_string(role_dir.join("hook.sh")).unwrap(), + "shipped\n" + ); + assert_eq!( + fs::read_to_string(agent_dir.join("hook.sh")).unwrap(), + "shipped\n" + ); + drop(script); + + // And one keep-all must keep every location's local edit. + modify_all(); + let script = prompt_script::install(&["keep-all"]); + let mut sticky = StickyMode::None; + refresh(&mut sticky); + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(sticky, StickyMode::KeepAll); + assert_eq!(fs::read_to_string(¬ify).unwrap(), "local global"); + assert_eq!( + fs::read_to_string(role_dir.join("hook.sh")).unwrap(), + "local role" + ); + assert_eq!( + fs::read_to_string(agent_dir.join("hook.sh")).unwrap(), + "local agent" + ); + drop(script); + } + + #[test] + #[serial_test::serial] + fn install_builtins_never_prompts_and_keeps_local_hook_edits() { + use crate::config::conflict::prompt_script; + + let _guard = TestConfigDirGuard::new("install-builtins-startup"); + install_builtins().unwrap(); + let notify = paths::hooks_dir().join("notify.sh"); + fs::write(¬ify, "# local edit").unwrap(); + + // A forced terminal with no scripted answers: any prompt would panic + // and the counter would move. + let script = prompt_script::install(&[]); + install_builtins().unwrap(); + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(fs::read_to_string(¬ify).unwrap(), "# local edit"); + drop(script); + + let _script = prompt_script::install_non_interactive(); + install_builtins().unwrap(); + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(fs::read_to_string(¬ify).unwrap(), "# local edit"); + } } diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 3e3e412..30e8486 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -5497,6 +5497,7 @@ mod tests { use crate::config::AppState; use crate::config::agent::AgentConfig; use crate::config::bundles::BundleStore; + use crate::config::conflict::InstallMode; use crate::config::mcp_tool_policy::LayerSource; use crate::config::tool_scope::test_fixtures::{FixtureServer, fixture_runtime}; use crate::function::jobs::RingBuf; @@ -9184,19 +9185,19 @@ mod tests { fn install_builtin_agents_force_overwrites_only_with_force() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); let file = first_file(&paths::agents_data_dir()).expect("bundled agents should be installed"); write(&file, "SENTINEL").unwrap(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); assert_eq!( read_to_string(&file).unwrap(), "SENTINEL", "non-force install must not overwrite an existing file" ); - Agent::install_builtin_agents(true).unwrap(); + Agent::install_builtin_agents(InstallMode::Force).unwrap(); assert_ne!( read_to_string(&file).unwrap(), "SENTINEL", @@ -9209,7 +9210,7 @@ mod tests { fn install_builtin_agents_removes_stale_definition_files() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); let agents_dir = paths::agents_data_dir(); // Simulate an upgrade from an older install: coder used to ship @@ -9230,7 +9231,7 @@ mod tests { create_dir_all(agents_dir.join("myagent")).unwrap(); write(agents_dir.join("myagent").join("config.yaml"), "keep").unwrap(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); assert!( !agents_dir.join("coder").join("config.yaml").exists(), @@ -9270,12 +9271,12 @@ mod tests { fn install_builtin_agents_removes_stale_graph_for_config_agent() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); let agents_dir = paths::agents_data_dir(); write(agents_dir.join("architect").join("graph.yaml"), "stale").unwrap(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); assert!( !agents_dir.join("architect").join("graph.yaml").exists(), @@ -9291,7 +9292,7 @@ mod tests { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); let agents_dir = paths::agents_data_dir(); // Simulate an upgrade from an older install whose coder shipped a @@ -9303,14 +9304,20 @@ mod tests { 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(); + // An agent directory carrying a builtin manifest but absent from the + // embed is a removed builtin agent: the sweep deletes exactly the + // manifest-owned files, sparing anything user-created alongside. + let removed_hooks = agents_dir.join("myagent").join("hooks"); + create_dir_all(&removed_hooks).unwrap(); + write(removed_hooks.join(BUILTIN_MANIFEST_FILE), "shipped.sh\n").unwrap(); + write(removed_hooks.join("shipped.sh"), "stale").unwrap(); + write(removed_hooks.join("user-note.sh"), "keep").unwrap(); + // A user agent never has a manifest, so the sweep must not touch it. + let user_hooks = agents_dir.join("useragent").join("hooks"); + create_dir_all(&user_hooks).unwrap(); + write(user_hooks.join("mine.sh"), "keep").unwrap(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); assert!( !hooks_dir.join("old-hook.sh").exists(), @@ -9325,10 +9332,18 @@ mod tests { "an empty shipped set removes the manifest itself" ); assert!( - custom_hooks.join("custom.sh").exists(), - "custom agents outside the bundle must survive reconciliation" + !removed_hooks.join("shipped.sh").exists(), + "manifest-owned hooks of a removed builtin agent must be swept" + ); + assert!(!removed_hooks.join(BUILTIN_MANIFEST_FILE).exists()); + assert!( + removed_hooks.join("user-note.sh").exists(), + "user files beside a removed agent's manifest must survive the sweep" + ); + assert!( + user_hooks.join("mine.sh").exists(), + "agent dirs without a manifest must never be touched" ); - assert!(custom_hooks.join(BUILTIN_MANIFEST_FILE).exists()); } #[test] @@ -9372,7 +9387,7 @@ mod tests { #[serial] fn bundled_assets_pin_task_queue_guidance() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); // 1. Architect config: the parallel-mode "Task-queue mirroring" // section with all five HARD RULES. @@ -9441,7 +9456,7 @@ mod tests { #[serial] fn bundled_assets_pin_review_incomplete_escalation() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); // Every anchor is a single-line prefix so YAML re-wrapping cannot // break the pin; clauses that wrap differently per config are split @@ -9605,7 +9620,7 @@ mod tests { #[serial] fn bundled_assets_pin_verification_commands_as_declared_variable() { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); const VARIABLES_FORM: &str = "--variables {\"verification_commands\": \"[\\\"cargo test --all\\\""; @@ -9796,7 +9811,7 @@ mod tests { } let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); for (name, expected) in [ ("architect", 10), @@ -9915,7 +9930,7 @@ mod tests { let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); Skill::install_builtin_skills(false).unwrap(); let mut checked = Vec::new(); @@ -10142,7 +10157,7 @@ mod tests { let engine_seeded = ["initial_prompt", "output", "choice", "input"]; let _guard = TestConfigDirGuard::new(); - Agent::install_builtin_agents(false).unwrap(); + Agent::install_builtin_agents(InstallMode::Skip).unwrap(); let mut checked = Vec::new(); for entry in std::fs::read_dir(paths::agents_data_dir()).unwrap() { diff --git a/src/config/role.rs b/src/config/role.rs index eaa8123..55b2c9c 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -2,6 +2,7 @@ use super::*; use crate::client::{Message, MessageContent, MessageRole, Model}; use crate::config::builtin_manifest; +use crate::config::conflict::{self, InstallMode, StickyMode}; use crate::function::write_file_atomic; use crate::hooks::HooksMap; @@ -193,7 +194,7 @@ impl Role { /// 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<()> { + pub fn install_builtin_role_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Result<()> { let shipped: Vec<(String, String)> = RolesAsset::iter() .filter_map(|file| { let name = file.as_ref().strip_prefix("hooks/")?; @@ -206,7 +207,7 @@ impl Role { }) .collect(); - install_and_reconcile_role_hooks(&paths::roles_dir().join("hooks"), &shipped, force) + install_and_reconcile_role_hooks(&paths::roles_dir().join("hooks"), &shipped, mode, sticky) } pub fn has_args(&self) -> bool { @@ -641,10 +642,11 @@ fn parse_structure_prompt(prompt: &str) -> (&str, Vec<(&str, &str)>) { /// 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( +pub(crate) fn install_and_reconcile_role_hooks( dir: &Path, shipped: &[(String, String)], - force: bool, + mode: InstallMode, + sticky: &mut StickyMode, ) -> Result<()> { if !shipped.is_empty() { info!("Installing built-in role hooks in {}", dir.display()); @@ -652,7 +654,9 @@ fn install_and_reconcile_role_hooks( let mut written = BTreeSet::new(); for (name, content) in shipped { let path = dir.join(name); - if path.exists() && !force { + if path.exists() + && !conflict::should_replace_existing(&path, content, "roles", mode, sticky)? + { debug!( "Role hook file already exists, skipping: {}", path.display() @@ -1049,8 +1053,13 @@ Input 1 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(); + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("a.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .unwrap(); assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "#!/bin/sh\n"); assert_eq!( @@ -1082,14 +1091,50 @@ Input 1 create_dir_all(&dir).unwrap(); fs::write(dir.join("a.sh"), "SENTINEL").unwrap(); - install_and_reconcile_role_hooks(&dir, &shipped, false).unwrap(); + install_and_reconcile_role_hooks(&dir, &shipped, InstallMode::Skip, &mut StickyMode::None) + .unwrap(); assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "SENTINEL"); - install_and_reconcile_role_hooks(&dir, &shipped, true).unwrap(); + install_and_reconcile_role_hooks(&dir, &shipped, InstallMode::Force, &mut StickyMode::None) + .unwrap(); assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "new content\n"); let _ = fs::remove_dir_all(dir.parent().unwrap()); } + #[test] + #[serial_test::serial] + fn role_hooks_prompt_mode_asks_per_conflict_and_honors_sticky() { + use crate::config::conflict::prompt_script; + + let dir = hooks_fixture_dir("role-hooks-prompt-"); + let shipped = fixture(&[("a.sh", "new content\n"), ("b.sh", "new content\n")]); + create_dir_all(&dir).unwrap(); + fs::write(dir.join("a.sh"), "local a").unwrap(); + fs::write(dir.join("b.sh"), "local b").unwrap(); + + // First conflict answered per-file, second by the sticky replace-all. + let script = prompt_script::install(&["replace-all"]); + let mut sticky = StickyMode::None; + install_and_reconcile_role_hooks(&dir, &shipped, InstallMode::Prompt, &mut sticky).unwrap(); + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "new content\n"); + assert_eq!(read_to_string(dir.join("b.sh")).unwrap(), "new content\n"); + assert_eq!(sticky, StickyMode::ReplaceAll); + drop(script); + + // A sticky mode carried in from an earlier location suppresses all + // prompting: `--install-builtins hooks` shares one sticky scope + // across the global, role, and agent hook locations. + fs::write(dir.join("a.sh"), "local again").unwrap(); + let script = prompt_script::install(&[]); + let mut sticky = StickyMode::KeepAll; + install_and_reconcile_role_hooks(&dir, &shipped, InstallMode::Prompt, &mut sticky).unwrap(); + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "local again"); + drop(script); + 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-"); @@ -1097,13 +1142,19 @@ Input 1 install_and_reconcile_role_hooks( &dir, &fixture(&[("keep.sh", "#!/bin/sh\n"), ("drop.sh", "#!/bin/sh\n")]), - false, + InstallMode::Skip, + &mut StickyMode::None, ) .unwrap(); fs::write(dir.join("user.sh"), "user-owned").unwrap(); - install_and_reconcile_role_hooks(&dir, &fixture(&[("keep.sh", "#!/bin/sh\n")]), false) - .unwrap(); + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("keep.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .unwrap(); assert!( !dir.join("drop.sh").exists(), @@ -1124,14 +1175,20 @@ Input 1 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(); + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("a.sh", "#!/bin/sh\n")]), + InstallMode::Skip, + &mut StickyMode::None, + ) + .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(); + install_and_reconcile_role_hooks(&dir, &[], InstallMode::Skip, &mut StickyMode::None) + .unwrap(); assert_eq!( read_to_string(dir.join("a.sh")).unwrap(), "user-owned", @@ -1144,7 +1201,8 @@ Input 1 fn role_hooks_empty_shipped_set_creates_nothing() { let dir = hooks_fixture_dir("role-hooks-empty-"); - install_and_reconcile_role_hooks(&dir, &[], false).unwrap(); + install_and_reconcile_role_hooks(&dir, &[], InstallMode::Skip, &mut StickyMode::None) + .unwrap(); assert!(!dir.exists(), "an empty shipped set must not create hooks/"); let _ = fs::remove_dir_all(dir.parent().unwrap()); diff --git a/src/hooks.rs b/src/hooks.rs index 78bf511..e3e1d4b 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -1,4 +1,5 @@ -use crate::config::{RequestContext, ensure_parent_exists, paths}; +use crate::config::conflict::{self, InstallMode, StickyMode}; +use crate::config::{RequestContext, builtin_manifest, ensure_parent_exists, paths}; use crate::function::write_file_atomic; use anyhow::{Result, anyhow}; @@ -7,6 +8,7 @@ use indexmap::IndexMap; use rand::distr::{Alphanumeric, SampleString}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; +use std::collections::BTreeSet; use std::path::{Path, PathBuf}; use std::process::Stdio; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -19,13 +21,14 @@ use tokio::sync::Notify; #[folder = "assets/hooks/"] struct HookAssets; -pub fn install_builtin_hooks(force: bool) -> Result<()> { +pub fn install_builtin_hooks(mode: InstallMode, sticky: &mut StickyMode) -> Result<()> { info!( "Installing built-in example hooks in {}", paths::hooks_dir().display() ); let mut wrote_any = false; + let mut written = BTreeSet::new(); for file in HookAssets::iter() { debug!("Processing hook file: {}", file.as_ref()); @@ -34,7 +37,9 @@ pub fn install_builtin_hooks(force: bool) -> Result<()> { 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 { + if file_path.exists() + && !conflict::should_replace_existing(&file_path, content, "hooks", mode, sticky)? + { debug!( "Hook file already exists, skipping: {}", file_path.display() @@ -46,6 +51,28 @@ pub fn install_builtin_hooks(force: bool) -> Result<()> { info!("Creating hook file: {}", file_path.display()); write_file_atomic(&file_path, content, Some(0o755))?; wrote_any = true; + if !file.as_ref().contains('/') { + written.insert(file.as_ref().to_string()); + } + } + + // Only direct children of the hooks dir reconcile through the builtin + // manifest — the only shape it tracks — so a hook a release stops + // shipping is removed while user scripts in the same directory are + // never touched. + let shipped: BTreeSet = HookAssets::iter() + .map(|file| file.as_ref().to_string()) + .filter(|name| !name.contains('/')) + .collect(); + // Reconciliation is best-effort housekeeping: a failure here must not + // abort startup (install_builtins), matching the role/agent-side policy. + if let Err(err) = + builtin_manifest::reconcile_builtin_dir(&paths::hooks_dir(), &shipped, &written) + { + warn!( + "Failed to reconcile builtin hooks in {}: {err}", + paths::hooks_dir().display() + ); } if wrote_any && paths::hooks_dir() != paths::config_dir().join("hooks") { @@ -614,6 +641,7 @@ pub(crate) mod test_sink { #[cfg(test)] mod tests { use super::*; + use crate::config::conflict::prompt_script; use crate::config::{AppConfig, AppState, Role, WorkingMode}; use serial_test::serial; use std::env; @@ -643,6 +671,36 @@ mod tests { RequestContext::new(Arc::new(app), WorkingMode::Cmd) } + /// Points the hooks dir at a fresh temp directory for the guard's + /// lifetime and removes it on drop. Tests using it must serialize. + struct HooksDirGuard { + _env: crate::testing::EnvVarGuard, + root: PathBuf, + } + + impl HooksDirGuard { + 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}")); + Self { + _env: crate::testing::EnvVarGuard::set( + crate::utils::get_env_name("hooks_dir"), + &root, + ), + root, + } + } + } + + impl Drop for HooksDirGuard { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.root); + } + } + #[test] #[serial] fn install_builtin_hooks_installs_executable_scripts_and_honors_force() { @@ -659,7 +717,7 @@ mod tests { // below still has to run before any assertion can bail out. let notify = root.join("notify.sh"); let log_events = root.join("log-events.sh"); - let fresh = install_builtin_hooks(false); + let fresh = install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None); let installed = (notify.is_file(), log_events.is_file()); #[cfg(unix)] let modes: Vec> = { @@ -670,9 +728,9 @@ mod tests { .collect() }; let modified = std::fs::write(¬ify, "modified"); - let no_force = install_builtin_hooks(false); + let no_force = install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None); let after_no_force = std::fs::read_to_string(¬ify); - let force = install_builtin_hooks(true); + let force = install_builtin_hooks(InstallMode::Force, &mut StickyMode::None); let after_force = std::fs::read_to_string(¬ify); drop(env_guard); @@ -697,6 +755,148 @@ mod tests { assert!(after_force.unwrap().starts_with("#!/usr/bin/env bash")); } + #[test] + #[serial] + fn install_builtin_hooks_skip_mode_never_prompts_even_on_a_terminal() { + let guard = HooksDirGuard::new("hooks-skip-no-prompt"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + let notify = guard.root.join("notify.sh"); + std::fs::write(¬ify, "modified").unwrap(); + + // A forced terminal with no scripted answers: any prompt would panic + // and the counter would move. + let _script = prompt_script::install(&[]); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(std::fs::read_to_string(¬ify).unwrap(), "modified"); + } + + #[test] + #[serial] + fn install_builtin_hooks_prompt_mode_asks_only_for_differing_files() { + let guard = HooksDirGuard::new("hooks-prompt-conflict"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + let notify = guard.root.join("notify.sh"); + std::fs::write(¬ify, "modified").unwrap(); + + // log-events.sh is identical to the embed, so only notify.sh asks. + let script = prompt_script::install(&["keep"]); + install_builtin_hooks(InstallMode::Prompt, &mut StickyMode::None).unwrap(); + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(std::fs::read_to_string(¬ify).unwrap(), "modified"); + drop(script); + + let script = prompt_script::install(&["replace"]); + install_builtin_hooks(InstallMode::Prompt, &mut StickyMode::None).unwrap(); + assert_eq!(prompt_script::prompts_asked(), 1); + assert!( + std::fs::read_to_string(¬ify) + .unwrap() + .starts_with("#!/usr/bin/env bash") + ); + drop(script); + } + + #[test] + #[serial] + fn install_builtin_hooks_prompt_mode_writes_missing_files_without_asking() { + let guard = HooksDirGuard::new("hooks-prompt-missing"); + let _script = prompt_script::install(&[]); + + install_builtin_hooks(InstallMode::Prompt, &mut StickyMode::None).unwrap(); + + assert_eq!(prompt_script::prompts_asked(), 0); + assert!(guard.root.join("notify.sh").is_file()); + assert!(guard.root.join("log-events.sh").is_file()); + } + + #[test] + #[serial] + fn install_builtin_hooks_prompt_mode_keeps_local_files_without_a_terminal() { + let guard = HooksDirGuard::new("hooks-prompt-non-tty"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + let notify = guard.root.join("notify.sh"); + std::fs::write(¬ify, "modified").unwrap(); + + let _script = prompt_script::install_non_interactive(); + install_builtin_hooks(InstallMode::Prompt, &mut StickyMode::None).unwrap(); + + assert_eq!(prompt_script::prompts_asked(), 0); + assert_eq!(std::fs::read_to_string(¬ify).unwrap(), "modified"); + } + + #[test] + #[serial] + fn install_builtin_hooks_keep_all_covers_every_remaining_conflict() { + let guard = HooksDirGuard::new("hooks-prompt-keep-all"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + let notify = guard.root.join("notify.sh"); + let log_events = guard.root.join("log-events.sh"); + std::fs::write(¬ify, "modified notify").unwrap(); + std::fs::write(&log_events, "modified log-events").unwrap(); + + let _script = prompt_script::install(&["keep-all"]); + install_builtin_hooks(InstallMode::Prompt, &mut StickyMode::None).unwrap(); + + assert_eq!(prompt_script::prompts_asked(), 1); + assert_eq!(std::fs::read_to_string(¬ify).unwrap(), "modified notify"); + assert_eq!( + std::fs::read_to_string(&log_events).unwrap(), + "modified log-events" + ); + } + + #[test] + #[serial] + fn install_builtin_hooks_removes_stale_shipped_hooks_via_manifest() { + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; + + let guard = HooksDirGuard::new("hooks-manifest-stale"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + + // Simulate an upgrade from a release that also shipped old-hook.sh: + // the manifest records it, so reinstall removes it — while a user + // script absent from the manifest survives. + std::fs::write( + guard.root.join(BUILTIN_MANIFEST_FILE), + "log-events.sh\nnotify.sh\nold-hook.sh\n", + ) + .unwrap(); + std::fs::write(guard.root.join("old-hook.sh"), "stale").unwrap(); + std::fs::write(guard.root.join("user-hook.sh"), "user-owned").unwrap(); + + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + + assert!(!guard.root.join("old-hook.sh").exists()); + assert_eq!( + std::fs::read_to_string(guard.root.join("user-hook.sh")).unwrap(), + "user-owned" + ); + assert_eq!( + std::fs::read_to_string(guard.root.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "log-events.sh\nnotify.sh\n" + ); + } + + #[test] + #[serial] + fn install_builtin_hooks_malformed_manifest_deletes_nothing() { + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; + + let guard = HooksDirGuard::new("hooks-manifest-malformed"); + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + std::fs::write(guard.root.join(BUILTIN_MANIFEST_FILE), [0xff, 0xfe, 0x00]).unwrap(); + std::fs::write(guard.root.join("old-hook.sh"), "keep").unwrap(); + + install_builtin_hooks(InstallMode::Skip, &mut StickyMode::None).unwrap(); + + assert!( + guard.root.join("old-hook.sh").exists(), + "an unreadable manifest must fail safe toward keeping files" + ); + } + /// 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.