diff --git a/src/config/agent.rs b/src/config/agent.rs index f82e272..4a818e6 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -13,6 +13,7 @@ use crate::{ }; use super::rag_cache::RagKey; +use crate::config::builtin_manifest; use crate::config::paths; use crate::config::prompts::{ DEFAULT_JOB_INSTRUCTIONS, DEFAULT_SPAWN_INSTRUCTIONS, DEFAULT_TEAMMATE_INSTRUCTIONS, @@ -28,6 +29,7 @@ use fancy_regex::Captures; use inquire::{Text, validator::Validation}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; +use std::collections::BTreeSet; use std::{env, ffi::OsStr, path::Path}; const DEFAULT_AGENT_NAME: &str = "rag"; @@ -47,6 +49,20 @@ pub type AgentVariables = IndexMap; #[folder = "assets/agents/"] struct AgentAssets; +/// Splits an embedded asset path of the form `/hooks/` into +/// its agent and hook-file names. Only direct children of `hooks/` count: +/// they are the only shape the builtin manifest tracks. +fn parse_direct_hook_path(path: &str) -> Option<(&str, &str)> { + let (agent, rest) = path.split_once('/')?; + let hook = direct_hook_name(rest)?; + Some((agent, hook)) +} + +fn direct_hook_name(rest: &str) -> Option<&str> { + let name = rest.strip_prefix("hooks/")?; + (!name.is_empty() && !name.contains('/')).then_some(name) +} + #[derive(Debug, Clone)] pub struct Agent { name: String, @@ -71,6 +87,7 @@ impl Agent { paths::agents_data_dir().display() ); + let mut written_hooks: HashMap> = HashMap::new(); for file in AgentAssets::iter() { debug!("Processing agent file: {}", file.as_ref()); @@ -97,6 +114,12 @@ impl Agent { info!("Creating agent file: {}", file_path.display()); let mut agent_file = File::create(&file_path)?; agent_file.write_all(content.as_bytes())?; + if let Some((agent, hook)) = parse_direct_hook_path(file.as_ref()) { + written_hooks + .entry(agent.to_string()) + .or_default() + .insert(hook.to_string()); + } #[cfg(unix)] if is_script { @@ -135,6 +158,27 @@ impl Agent { } } + // Hook filenames are open-ended (unlike AGENT_DEFINITION_FILES), so + // each bundled agent's hooks/ directory reconciles through its + // builtin manifest: only files the installer previously shipped are + // removal candidates, and user-created files in the same directory + // are never touched. Only direct children of hooks/ are tracked; + // nested shipped files install but are not reconciled. + for (agent, files) in &bundled_files { + let shipped: BTreeSet = files + .iter() + .filter_map(|rest| direct_hook_name(rest)) + .map(str::to_string) + .collect(); + let written = written_hooks.remove(agent.as_str()).unwrap_or_default(); + let hooks_dir = paths::agents_data_dir().join(agent).join("hooks"); + if let Err(err) = + builtin_manifest::reconcile_builtin_dir(&hooks_dir, &shipped, &written) + { + warn!("Failed to reconcile builtin hooks for agent '{agent}': {err}"); + } + } + Ok(()) } @@ -1318,6 +1362,16 @@ pub fn complete_agent_variables(agent_name: &str) -> Vec<(String, Option mod tests { use super::*; + #[test] + fn parse_direct_hook_path_accepts_only_direct_hook_children() { + assert_eq!(parse_direct_hook_path("x/hooks/a.sh"), Some(("x", "a.sh"))); + assert_eq!(parse_direct_hook_path("x/hooks/nested/a.sh"), None); + assert_eq!(parse_direct_hook_path("hooks/a.sh"), None); + assert_eq!(parse_direct_hook_path("x/hooks/"), None); + assert_eq!(parse_direct_hook_path("x/config.yaml"), None); + assert_eq!(parse_direct_hook_path("config.yaml"), None); + } + #[test] fn agent_config_parses_from_yaml() { let yaml = r#" diff --git a/src/config/builtin_manifest.rs b/src/config/builtin_manifest.rs new file mode 100644 index 0000000..8fc8577 --- /dev/null +++ b/src/config/builtin_manifest.rs @@ -0,0 +1,300 @@ +//! Per-directory tracking of files the builtin installer shipped, so a later +//! release that stops shipping an asset can remove the stale copy without +//! ever touching user-created files that live in the same directory. +//! +//! Unlike agent definition files, hook filenames are open-ended: no closed +//! candidate list can distinguish "previously bundled" from "user-authored". +//! The manifest records exactly what the installer shipped; anything absent +//! from it is user-owned and untouchable. + +use crate::config::ensure_parent_exists; +use crate::function::write_file_atomic; + +use anyhow::{Context, Result}; +use std::collections::BTreeSet; +use std::fs; +use std::path::Path; + +/// Hidden per-directory record of the files the builtin installer shipped +/// there. Format: one plain filename per line (LF-separated, sorted); no +/// path separators, and never the manifest itself. Written non-executable +/// through the same atomic-write path as other installed files. +pub(crate) const BUILTIN_MANIFEST_FILE: &str = ".builtin-manifest"; + +/// True when `name` aliases the manifest file on some real-world filesystem: +/// the comparison ignores trailing dots/spaces (Windows strips them at +/// creation) and folds Unicode case (case-insensitive APFS/NTFS volumes fold +/// beyond ASCII, e.g. U+017F ſ → s). +pub(crate) fn is_builtin_manifest_name(name: &str) -> bool { + name.trim_end_matches(['.', ' ']) + .to_uppercase() + .eq_ignore_ascii_case(BUILTIN_MANIFEST_FILE) +} + +/// Reconciles `dir` against the currently shipped set of builtin filenames: +/// a file is removed ONLY if it appears in the previous manifest AND is +/// absent from `shipped`. The manifest is then rewritten to the names this +/// installer actually owns: files written this run (`written`), plus shipped +/// names it already owned, plus stale files whose removal failed (kept so a +/// later run retries). A shipped name the installer declined to write — a +/// pre-existing user file with a colliding name — is never claimed. +/// +/// Failure modes degrade toward keeping files: a missing or unreadable +/// manifest yields no deletions, and entries that are not plain filenames +/// are ignored. +pub(crate) fn reconcile_builtin_dir( + dir: &Path, + shipped: &BTreeSet, + written: &BTreeSet, +) -> Result<()> { + let previous = read_manifest(dir); + let mut retained: BTreeSet = written.clone(); + retained.extend(shipped.intersection(&previous).cloned()); + for name in previous.difference(shipped) { + let path = dir.join(name); + if !path.is_file() { + continue; + } + info!( + "Removing stale builtin file no longer shipped: {}", + path.display() + ); + if let Err(err) = fs::remove_file(&path) { + warn!( + "Failed to remove stale builtin file {}: {err}", + path.display() + ); + retained.insert(name.clone()); + } + } + + write_manifest(dir, &retained) +} + +fn read_manifest(dir: &Path) -> BTreeSet { + let path = dir.join(BUILTIN_MANIFEST_FILE); + let content = match fs::read_to_string(&path) { + Ok(content) => content, + Err(err) => { + debug!( + "No readable builtin manifest at {} ({err}); skipping stale-file removal", + path.display() + ); + return BTreeSet::new(); + } + }; + + content + .lines() + .map(str::trim) + .filter(|line| { + if line.is_empty() || is_builtin_manifest_name(line) { + return false; + } + if !is_plain_file_name(line) { + debug!( + "Ignoring suspicious builtin manifest entry in {}: {line:?}", + path.display() + ); + return false; + } + true + }) + .map(str::to_string) + .collect() +} + +/// Manifest entries name direct children of the manifest's directory and +/// nothing else; anything path-like is rejected so a corrupted or crafted +/// manifest can never direct a deletion outside that directory. The same +/// component rules the bundle store applies to recorded paths are reused so +/// Windows drive prefixes (`C:x`), ADS colons, trailing dots/spaces, and +/// reserved device names are rejected too. +fn is_plain_file_name(name: &str) -> bool { + !name.contains(['/', '\\']) + && name != "." + && name != ".." + && super::install_remote::is_safe_component(name) +} + +fn write_manifest(dir: &Path, names: &BTreeSet) -> Result<()> { + let path = dir.join(BUILTIN_MANIFEST_FILE); + if names.is_empty() { + if path.is_file() { + fs::remove_file(&path) + .with_context(|| format!("failed to remove builtin manifest {}", path.display()))?; + } + return Ok(()); + } + + let mut content = names.iter().cloned().collect::>().join("\n"); + content.push('\n'); + ensure_parent_exists(&path)?; + write_file_atomic(&path, &content, None) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::utils; + + fn fresh_dir(label: &str) -> std::path::PathBuf { + let dir = utils::temp_file(label, ""); + fs::create_dir_all(&dir).unwrap(); + dir + } + + fn shipped(names: &[&str]) -> BTreeSet { + names.iter().map(|n| n.to_string()).collect() + } + + #[test] + fn removes_only_manifest_listed_files_absent_from_shipped() { + let dir = fresh_dir("builtin-manifest-stale-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), "old.sh\nkept.sh\n").unwrap(); + fs::write(dir.join("old.sh"), "stale").unwrap(); + fs::write(dir.join("kept.sh"), "current").unwrap(); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["kept.sh"]), &shipped(&[])).unwrap(); + + assert!(!dir.join("old.sh").exists(), "stale bundled file removed"); + assert!(dir.join("kept.sh").exists(), "still-shipped file kept"); + assert!(dir.join("user.sh").exists(), "unlisted user file untouched"); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "kept.sh\n" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn missing_manifest_deletes_nothing_and_writes_shipped_set() { + let dir = fresh_dir("builtin-manifest-missing-"); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["new.sh"]), &shipped(&["new.sh"])).unwrap(); + + assert!(dir.join("user.sh").exists()); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "new.sh\n" + ); + let _ = fs::remove_dir_all(&dir); + } + + /// A shipped name the installer skipped (pre-existing user file with a + /// colliding name) is never claimed by the manifest, so a later release + /// dropping that hook can never delete the user's file. + #[test] + fn skipped_preexisting_user_file_is_never_claimed() { + let dir = fresh_dir("builtin-manifest-unclaimed-"); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["user.sh"]), &shipped(&[])).unwrap(); + assert!( + !dir.join(BUILTIN_MANIFEST_FILE).exists(), + "nothing was written this run and nothing was owned before" + ); + + reconcile_builtin_dir(&dir, &shipped(&[]), &shipped(&[])).unwrap(); + assert!( + dir.join("user.sh").exists(), + "the unclaimed user file survives the hook being dropped" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn suspicious_manifest_entries_never_direct_deletions() { + let dir = fresh_dir("builtin-manifest-suspicious-"); + let outside = fresh_dir("builtin-manifest-outside-"); + let escape = format!( + "../{}/victim.sh", + outside.file_name().unwrap().to_string_lossy() + ); + fs::write( + dir.join(BUILTIN_MANIFEST_FILE), + format!("{escape}\nsub/nested.sh\nC:victim.sh\ntrailing.\n..\n.\n\n"), + ) + .unwrap(); + fs::write(outside.join("victim.sh"), "keep").unwrap(); + fs::create_dir_all(dir.join("sub")).unwrap(); + fs::write(dir.join("sub/nested.sh"), "keep").unwrap(); + + reconcile_builtin_dir(&dir, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + + assert!(outside.join("victim.sh").exists()); + assert!(dir.join("sub/nested.sh").exists()); + let _ = fs::remove_dir_all(&dir); + let _ = fs::remove_dir_all(&outside); + } + + #[test] + fn unreadable_manifest_deletes_nothing() { + let dir = fresh_dir("builtin-manifest-unreadable-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), [0xff, 0xfe, 0x00]).unwrap(); + fs::write(dir.join("old.sh"), "keep").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["new.sh"]), &shipped(&["new.sh"])).unwrap(); + + assert!( + dir.join("old.sh").exists(), + "a bad manifest must fail safe toward keeping files" + ); + assert_eq!( + fs::read_to_string(dir.join(BUILTIN_MANIFEST_FILE)).unwrap(), + "new.sh\n", + "manifest self-heals to the current shipped set" + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn empty_shipped_set_removes_manifest_and_creates_nothing() { + let dir = fresh_dir("builtin-manifest-empty-"); + fs::write(dir.join(BUILTIN_MANIFEST_FILE), "old.sh\n").unwrap(); + fs::write(dir.join("old.sh"), "stale").unwrap(); + fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + reconcile_builtin_dir(&dir, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + + assert!(!dir.join("old.sh").exists()); + assert!(dir.join("user.sh").exists()); + assert!(!dir.join(BUILTIN_MANIFEST_FILE).exists()); + + let absent = dir.join("never-created"); + reconcile_builtin_dir(&absent, &BTreeSet::new(), &BTreeSet::new()).unwrap(); + assert!(!absent.exists(), "no directory is created for an empty set"); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn manifest_listing_itself_never_deletes_it() { + let dir = fresh_dir("builtin-manifest-self-"); + fs::write( + dir.join(BUILTIN_MANIFEST_FILE), + format!("{BUILTIN_MANIFEST_FILE}\n.BUILTIN-MANIFEST\nkept.sh\n"), + ) + .unwrap(); + fs::write(dir.join("kept.sh"), "current").unwrap(); + + reconcile_builtin_dir(&dir, &shipped(&["kept.sh"]), &shipped(&[])).unwrap(); + + assert!(dir.join(BUILTIN_MANIFEST_FILE).exists()); + assert!(dir.join("kept.sh").exists()); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn manifest_name_matcher_covers_filesystem_aliases() { + assert!(is_builtin_manifest_name(".builtin-manifest")); + assert!(is_builtin_manifest_name(".BUILTIN-MANIFEST")); + assert!(is_builtin_manifest_name(".Builtin-Manifest.")); + assert!(is_builtin_manifest_name(".builtin-manifest ")); + assert!(is_builtin_manifest_name(".builtin-manifeſt")); + assert!(!is_builtin_manifest_name("builtin-manifest")); + assert!(!is_builtin_manifest_name(".builtin-manifesto")); + assert!(!is_builtin_manifest_name("notify.sh")); + } +} diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 02a5b99..c85aa75 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -2,6 +2,7 @@ use super::bundles::{ BundleStore, FileAction, FileRecord, InstallMetadata, McpAction, McpServerRecord, hash_bytes, hash_file, }; +use crate::config::builtin_manifest::is_builtin_manifest_name; use crate::config::{AssetCategory, BUNDLE_MANIFEST_FILE, InstallFilter, paths}; #[cfg(not(windows))] use crate::function::Language; @@ -32,7 +33,7 @@ pub fn install_remote(git_url: &str, filter: Option, force: bool) if layout.is_empty() { println!( "No recognized assets found in {git_url}. Expected one or more of: \ - agents/, roles/, skills/, macros/, functions/tools/, mcp.json" + agents/, roles/, skills/, macros/, functions/tools/, hooks/, mcp.json" ); return Ok(()); } @@ -805,7 +806,7 @@ fn is_safe_relative_path(path: &str) -> bool { /// Rejects names Windows refuses or silently rewrites (alternate data stream /// colons, reserved device names, trailing dots or spaces) so a recorded path /// denotes the same regular file on every platform. -fn is_safe_component(name: &str) -> bool { +pub(crate) fn is_safe_component(name: &str) -> bool { !name.contains(':') && !name.ends_with('.') && !name.ends_with(' ') @@ -1270,6 +1271,7 @@ struct RemoteLayout { roles: Option, skills: Option, macros: Option, + hooks: Option, functions_tools: Option, mcp_json: Option, manifest: Option, @@ -1282,6 +1284,7 @@ impl RemoteLayout { && self.roles.is_none() && self.skills.is_none() && self.macros.is_none() + && self.hooks.is_none() && self.functions_tools.is_none() && self.mcp_json.is_none() } @@ -1313,6 +1316,11 @@ fn scan_remote_layout(root: &Path) -> Result { layout.macros = Some(macros); } + let hooks = root.join("hooks"); + if hooks.is_dir() { + layout.hooks = Some(hooks); + } + let root_mcp = root.join("mcp.json"); if root_mcp.is_file() { layout.mcp_json = Some(root_mcp); @@ -1540,10 +1548,10 @@ fn apply_filter(mut layout: RemoteLayout, filter: Option) -> Remo functions_tools: layout.functions_tools.take(), ..base }, - // Bundles cannot ship hooks yet: RemoteLayout has no hooks field, so - // this filter always yields an empty layout. Bundle-side hooks support - // (layout field, scan, is_empty) lands together with TopCategory::Hooks. - InstallFilter::Hooks => base, + InstallFilter::Hooks => RemoteLayout { + hooks: layout.hooks.take(), + ..base + }, InstallFilter::McpConfig => RemoteLayout { mcp_json: layout.mcp_json.take(), ..base @@ -1595,6 +1603,7 @@ enum TopCategory { Roles, Skills, Macros, + Hooks, FunctionsTools, } @@ -1605,6 +1614,7 @@ impl TopCategory { TopCategory::Roles => "roles", TopCategory::Skills => "skills", TopCategory::Macros => "macros", + TopCategory::Hooks => "hooks", TopCategory::FunctionsTools => "functions/tools", } } @@ -1623,6 +1633,8 @@ enum PlannedKind { struct PlannedFile { src: PathBuf, dst: PathBuf, + /// Path relative to the category's source directory in the bundle. + rel: PathBuf, kind: PlannedKind, top_category: TopCategory, } @@ -1664,6 +1676,9 @@ fn plan_changes(layout: &RemoteLayout) -> Result { &mut files, )?; } + if let Some(src_dir) = &layout.hooks { + plan_dir_into(src_dir, &paths::hooks_dir(), TopCategory::Hooks, &mut files)?; + } if let Some(src_dir) = &layout.functions_tools { plan_dir_into( src_dir, @@ -1690,7 +1705,28 @@ fn plan_dir_into( for src in walk_files(src_dir)? { let rel = src .strip_prefix(src_dir) - .expect("walk_files only returns paths under src_dir"); + .expect("walk_files only returns paths under src_dir") + .to_path_buf(); + + // The builtin installer's per-directory manifests drive stale-file + // deletion; letting a bundle install one would hand it control over + // which local files the next builtin reconcile removes. Every path + // component is checked (a bundle-shipped DIRECTORY of that name would + // permanently occupy the manifest path and disable reconciliation), + // under the filesystem-alias-aware match of is_builtin_manifest_name. + let is_builtin_manifest = rel.components().any(|c| match c.as_os_str().to_str() { + Some(name) => is_builtin_manifest_name(name), + // A non-UTF-8 name can never equal the ASCII manifest constant. + None => false, + }); + if is_builtin_manifest { + log::warn!( + "Ignoring bundle file {} ({}): the name is reserved for the builtin installer", + rel.display(), + category.label() + ); + continue; + } if category == TopCategory::Skills { let skill_name = rel @@ -1711,11 +1747,12 @@ fn plan_dir_into( })?; } - let dst = dst_dir.join(rel); + let dst = dst_dir.join(&rel); let kind = classify_file(&src, &dst)?; out.push(PlannedFile { src, dst, + rel, kind, top_category: category, }); @@ -1794,6 +1831,7 @@ fn print_plan_summary(plan: &InstallPlan) { TopCategory::Roles, TopCategory::Skills, TopCategory::Macros, + TopCategory::Hooks, TopCategory::FunctionsTools, ] { let new_ = count_kind(plan, cat, PlannedKind::New); @@ -1812,6 +1850,33 @@ fn print_plan_summary(plan: &InstallPlan) { println!("{line}"); } } + + for line in hook_script_lines(plan) { + println!("{line}"); + } +} + +/// Bundle-shipped hook scripts execute on the user's machine once wired into +/// config, so every planned file landing under a hooks/ directory is called +/// out individually. Visibility only: installing them needs no extra approval. +fn hook_script_lines(plan: &InstallPlan) -> Vec { + plan.files + .iter() + .filter(|planned| { + planned.top_category == TopCategory::Hooks + || planned + .rel + .parent() + .is_some_and(|parent| parent.components().any(|c| c.as_os_str() == "hooks")) + }) + .map(|planned| { + let rel = planned.rel.to_string_lossy().replace('\\', "/"); + format!( + "bundle defines hook script: {}/{rel}", + planned.top_category.label() + ) + }) + .collect() } fn count_kind(plan: &InstallPlan, cat: TopCategory, kind: PlannedKind) -> usize { @@ -2353,6 +2418,7 @@ fn print_secret_summary(added: &[String], deferred: &[String]) { #[cfg(test)] mod tests { use super::*; + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; use crate::sandbox::SANDBOX_ENV_FLAG; use crate::utils::get_env_name; use serial_test::serial; @@ -2555,6 +2621,7 @@ mod tests { roles: Some(PathBuf::from("r")), skills: Some(PathBuf::from("s")), macros: Some(PathBuf::from("m")), + hooks: Some(PathBuf::from("h")), functions_tools: Some(PathBuf::from("f")), mcp_json: Some(PathBuf::from("j")), ..RemoteLayout::default() @@ -2564,6 +2631,7 @@ mod tests { assert!(out.agents.is_some() && out.roles.is_some() && out.skills.is_some()); assert!(out.macros.is_some() && out.functions_tools.is_some() && out.mcp_json.is_some()); + assert!(out.hooks.is_some()); } #[test] @@ -2642,6 +2710,39 @@ mod tests { assert!(out.functions_tools.is_none() && out.mcp_json.is_none()); } + /// `--filter hooks` used to be an inert arm that always produced an empty + /// layout; it now selects the bundle's top-level hooks directory. + #[test] + fn apply_filter_hooks_keeps_only_hooks() { + let l = RemoteLayout { + agents: Some(PathBuf::from("a")), + roles: Some(PathBuf::from("r")), + skills: Some(PathBuf::from("s")), + macros: Some(PathBuf::from("m")), + hooks: Some(PathBuf::from("h")), + functions_tools: Some(PathBuf::from("f")), + mcp_json: Some(PathBuf::from("j")), + ..RemoteLayout::default() + }; + + let out = apply_filter(l, Some(InstallFilter::Hooks)); + + assert_eq!(out.hooks, Some(PathBuf::from("h"))); + assert!(!out.is_empty()); + assert!(out.agents.is_none() && out.roles.is_none() && out.skills.is_none()); + assert!(out.macros.is_none() && out.functions_tools.is_none() && out.mcp_json.is_none()); + } + + #[test] + fn remote_layout_with_only_hooks_is_not_empty() { + let l = RemoteLayout { + hooks: Some(PathBuf::from("h")), + ..RemoteLayout::default() + }; + + assert!(!l.is_empty()); + } + #[test] fn walk_files_skips_dot_git_and_collects_regular_files() { let root = fresh_temp_dir("walk-test-"); @@ -2688,6 +2789,7 @@ mod tests { fs::create_dir_all(root.join("roles")).unwrap(); fs::create_dir_all(root.join("skills")).unwrap(); fs::create_dir_all(root.join("macros")).unwrap(); + fs::create_dir_all(root.join("hooks")).unwrap(); fs::create_dir_all(root.join("functions/tools")).unwrap(); touch(&root.join("functions/mcp.json")); touch(&root.join("README.md")); @@ -2697,11 +2799,29 @@ mod tests { assert!(layout.roles.is_some()); assert!(layout.skills.is_some()); assert!(layout.macros.is_some()); + assert!(layout.hooks.is_some()); assert!(layout.functions_tools.is_some()); assert!(layout.mcp_json.is_some()); let _ = fs::remove_dir_all(&root); } + #[test] + fn scan_remote_layout_finds_hooks_only() { + let root = fresh_temp_dir("scan-hooks-only-"); + fs::create_dir_all(root.join("hooks")).unwrap(); + touch(&root.join("hooks/notify.sh")); + + let layout = scan_remote_layout(&root).unwrap(); + + assert_eq!(layout.hooks, Some(root.join("hooks"))); + assert!(layout.agents.is_none()); + assert!(layout.roles.is_none()); + assert!(layout.macros.is_none()); + assert!(layout.functions_tools.is_none()); + assert!(layout.mcp_json.is_none()); + let _ = fs::remove_dir_all(&root); + } + #[test] fn scan_remote_layout_finds_root_mcp_json() { let root = fresh_temp_dir("scan-root-mcp-test-"); @@ -2756,6 +2876,71 @@ mod tests { let _ = fs::remove_dir_all(&root); } + #[test] + fn plan_dir_into_skips_bundle_shipped_builtin_manifests() { + let root = fresh_temp_dir("plan-manifest-skip-"); + let src_dir = root.join("hooks"); + touch(&src_dir.join("notify.sh")); + touch(&src_dir.join(BUILTIN_MANIFEST_FILE)); + touch(&src_dir.join(format!("sub/{BUILTIN_MANIFEST_FILE}"))); + // Case and trailing-dot variants alias the real manifest path on + // case-insensitive filesystems; they live in separate subdirs so this + // fixture also builds on such filesystems without name collisions. + touch(&src_dir.join("sub2/.BUILTIN-MANIFEST")); + touch(&src_dir.join("sub3/.Builtin-Manifest.")); + // A DIRECTORY of the manifest name would occupy the manifest path and + // disable reconciliation; a Unicode case-fold alias (U+017F ſ → s) + // aliases it on case-insensitive APFS/NTFS volumes. + touch(&src_dir.join("sub4/.builtin-manifest/payload.sh")); + touch(&src_dir.join("sub5/.builtin-manifeſt")); + let dst_dir = root.join("dst"); + + let mut files = Vec::new(); + plan_dir_into(&src_dir, &dst_dir, TopCategory::Hooks, &mut files).unwrap(); + + let rels: Vec<&Path> = files.iter().map(|p| p.rel.as_path()).collect(); + assert_eq!( + rels, + vec![Path::new("notify.sh")], + "a bundle must never install a builtin manifest: it would hand the \ + bundle control over builtin stale-file deletion" + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn hook_script_lines_cover_every_hooks_dir_and_nothing_else() { + let file = |rel: &str, cat: TopCategory| PlannedFile { + src: PathBuf::from("src").join(rel), + dst: PathBuf::from("dst").join(rel), + rel: PathBuf::from(rel), + kind: PlannedKind::New, + top_category: cat, + }; + let plan = InstallPlan { + files: vec![ + file("notify.sh", TopCategory::Hooks), + file("x/hooks/pre.sh", TopCategory::Agents), + file("hooks/role-hook.sh", TopCategory::Roles), + file("x/config.yaml", TopCategory::Agents), + file("reviewer.md", TopCategory::Roles), + file("hello.yaml", TopCategory::Macros), + ], + mcp_json: None, + }; + + let lines = hook_script_lines(&plan); + + assert_eq!( + lines, + vec![ + "bundle defines hook script: hooks/notify.sh", + "bundle defines hook script: agents/x/hooks/pre.sh", + "bundle defines hook script: roles/hooks/role-hook.sh", + ] + ); + } + #[test] fn classify_file_new_when_dst_missing() { let dir = fresh_temp_dir("classify-new-"); @@ -3424,6 +3609,90 @@ mod tests { let _ = fs::remove_dir_all(&src_root); } + fn hooks_fixture_bundle(label: &str) -> (PathBuf, PathBuf) { + let src_root = fresh_temp_dir(label); + let repo = src_root.join("hooky"); + write_src(&repo, "hooks/notify-me.sh", "#!/bin/sh\necho hi\n"); + write_src(&repo, "roles/hooks/role-hook.sh", "#!/bin/sh\n"); + write_src(&repo, "agents/x/hooks/pre.sh", "#!/bin/sh\n"); + write_src(&repo, "macros/hello.yaml", "name: hello\n"); + init_bundle_repo(&repo); + (src_root, repo) + } + + #[cfg(unix)] + fn assert_executable(path: &Path) { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(path).unwrap().permissions().mode(); + assert!( + mode & 0o111 != 0, + "{} must carry the executable bit", + path.display() + ); + } + + #[test] + #[serial] + fn install_remote_installs_all_hooks_dirs_and_records_hooks_category() { + let _guard = TestVaultConfigGuard::new("prov-hooks-dirs"); + let (src_root, repo) = hooks_fixture_bundle("prov-hooks-dirs-src-"); + + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + + let top = paths::hooks_dir().join("notify-me.sh"); + let role = paths::roles_dir().join("hooks/role-hook.sh"); + let agent = paths::agents_data_dir().join("x/hooks/pre.sh"); + assert!(top.is_file(), "top-level hooks/ installs to hooks_dir"); + assert!(role.is_file(), "roles/hooks/ installs via the roles walk"); + assert!( + agent.is_file(), + "agents//hooks/ installs via the agents walk" + ); + #[cfg(unix)] + { + assert_executable(&top); + assert_executable(&role); + assert_executable(&agent); + } + + let store = BundleStore::load().unwrap(); + let record = store.get("hooky").unwrap(); + let category_of = |path: &str| { + record + .files + .iter() + .find(|f| f.path == path) + .unwrap_or_else(|| panic!("no provenance row for {path}")) + .category + .clone() + }; + assert_eq!(category_of("hooks/notify-me.sh"), "hooks"); + assert_eq!(category_of("roles/hooks/role-hook.sh"), "roles"); + assert_eq!(category_of("agents/x/hooks/pre.sh"), "agents"); + let _ = fs::remove_dir_all(&src_root); + } + + #[test] + #[serial] + fn install_remote_filter_hooks_restricts_to_top_level_hooks_dir() { + let _guard = TestVaultConfigGuard::new("prov-hooks-filter"); + let (src_root, repo) = hooks_fixture_bundle("prov-hooks-filter-src-"); + + install_remote(repo.to_str().unwrap(), Some(InstallFilter::Hooks), false).unwrap(); + + assert!(paths::hooks_dir().join("notify-me.sh").is_file()); + assert!(!paths::roles_dir().join("hooks/role-hook.sh").exists()); + assert!(!paths::agents_data_dir().join("x/hooks/pre.sh").exists()); + assert!(!paths::macros_dir().join("hello.yaml").exists()); + + let store = BundleStore::load().unwrap(); + let record = store.get("hooky").unwrap(); + assert_eq!(record.files.len(), 1); + assert_eq!(record.files[0].path, "hooks/notify-me.sh"); + assert_eq!(record.files[0].category, "hooks"); + let _ = fs::remove_dir_all(&src_root); + } + #[test] #[serial] fn install_remote_without_manifest_uses_repo_slug_and_short_sha_version() { @@ -3471,12 +3740,14 @@ mod tests { PlannedFile { src: src_new, dst: dst_new.clone(), + rel: PathBuf::from("new.yaml"), kind: PlannedKind::New, top_category: TopCategory::Macros, }, PlannedFile { src: src_conflict, dst: dst_conflict.clone(), + rel: PathBuf::from("conflict.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }, @@ -3529,6 +3800,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3840,6 +4112,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3886,6 +4159,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("owned.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -3965,6 +4239,7 @@ mod tests { files: vec![PlannedFile { src, dst: dst.clone(), + rel: PathBuf::from("shared.yaml"), kind: PlannedKind::Conflict, top_category: TopCategory::Macros, }], @@ -4237,6 +4512,7 @@ mod tests { files: vec![PlannedFile { src: dir.join("bundle-src/agents/foo/graph.yaml"), dst: dir.join("agents/foo/graph.yaml"), + rel: PathBuf::from("foo/graph.yaml"), kind: PlannedKind::New, top_category: TopCategory::Agents, }], @@ -4279,6 +4555,7 @@ mod tests { files: vec![PlannedFile { src: dir.join("bundle-src/agents/foo/graph.yaml"), dst: dir.join("agents/foo/graph.yaml"), + rel: PathBuf::from("foo/graph.yaml"), kind: PlannedKind::New, top_category: TopCategory::Agents, }], diff --git a/src/config/mod.rs b/src/config/mod.rs index 73ecdc2..2badeec 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,6 +1,7 @@ mod agent; mod app_config; mod app_state; +pub(crate) mod builtin_manifest; mod bundles; mod input; mod install_remote; @@ -420,6 +421,7 @@ pub fn install_builtins() -> Result<()> { Macro::install_macros(false)?; Skill::install_builtin_skills(false)?; crate::hooks::install_builtin_hooks(false)?; + Role::install_builtin_role_hooks(false)?; Ok(()) } diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 16daa82..05e23cc 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -9290,6 +9290,53 @@ mod tests { assert!(agents_dir.join("architect").join("config.yaml").exists()); } + #[test] + #[serial] + fn install_builtin_agents_reconciles_hooks_subdir_via_manifest() { + use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE; + + let _guard = TestConfigDirGuard::new(); + + Agent::install_builtin_agents(false).unwrap(); + + let agents_dir = paths::agents_data_dir(); + // Simulate an upgrade from an older install whose coder shipped a + // hook script that the current embed no longer contains: the manifest + // records it, so reconciliation removes it — while a user-created + // script in the same directory (absent from the manifest) survives. + let hooks_dir = agents_dir.join("coder").join("hooks"); + create_dir_all(&hooks_dir).unwrap(); + write(hooks_dir.join(BUILTIN_MANIFEST_FILE), "old-hook.sh\n").unwrap(); + write(hooks_dir.join("old-hook.sh"), "stale").unwrap(); + write(hooks_dir.join("user-hook.sh"), "user-owned").unwrap(); + // A custom agent outside the bundle is never reconciled, even with a + // manifest present. + let custom_hooks = agents_dir.join("myagent").join("hooks"); + create_dir_all(&custom_hooks).unwrap(); + write(custom_hooks.join(BUILTIN_MANIFEST_FILE), "custom.sh\n").unwrap(); + write(custom_hooks.join("custom.sh"), "keep").unwrap(); + + Agent::install_builtin_agents(false).unwrap(); + + assert!( + !hooks_dir.join("old-hook.sh").exists(), + "manifest-listed hook absent from the embed must be removed" + ); + assert!( + hooks_dir.join("user-hook.sh").exists(), + "user-created hook files must survive reconciliation" + ); + assert!( + !hooks_dir.join(BUILTIN_MANIFEST_FILE).exists(), + "an empty shipped set removes the manifest itself" + ); + assert!( + custom_hooks.join("custom.sh").exists(), + "custom agents outside the bundle must survive reconciliation" + ); + assert!(custom_hooks.join(BUILTIN_MANIFEST_FILE).exists()); + } + #[test] #[serial] fn install_builtin_skills_force_overwrites_only_with_force() { diff --git a/src/config/role.rs b/src/config/role.rs index 18d9220..7297b9b 100644 --- a/src/config/role.rs +++ b/src/config/role.rs @@ -1,6 +1,8 @@ use super::*; use crate::client::{Message, MessageContent, MessageRole, Model}; +use crate::config::builtin_manifest; +use crate::function::write_file_atomic; use crate::hooks::HooksMap; use anyhow::Result; @@ -8,6 +10,8 @@ use fancy_regex::Regex; use rust_embed::Embed; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::collections::BTreeSet; +use std::path::Path; use std::sync::LazyLock; pub const SHELL_ROLE: &str = "shell"; @@ -185,6 +189,26 @@ impl Role { .collect() } + /// Installs builtin role hook scripts into `/hooks/` and + /// reconciles ones a release stopped shipping. This is the only builtin + /// install machinery roles have: general role installation deliberately + /// does not exist (builtin roles load straight from the embed). + pub fn install_builtin_role_hooks(force: bool) -> Result<()> { + let shipped: Vec<(String, String)> = RolesAsset::iter() + .filter_map(|file| { + let name = file.as_ref().strip_prefix("hooks/")?; + if name.is_empty() || name.contains('/') { + return None; + } + let embedded = RolesAsset::get(file.as_ref())?; + let content = unsafe { std::str::from_utf8_unchecked(&embedded.data) }; + Some((name.to_string(), content.to_string())) + }) + .collect(); + + install_and_reconcile_role_hooks(&paths::roles_dir().join("hooks"), &shipped, force) + } + pub fn has_args(&self) -> bool { self.name.contains('#') } @@ -614,6 +638,46 @@ fn parse_structure_prompt(prompt: &str) -> (&str, Vec<(&str, &str)>) { (prompt, vec![]) } +/// Only direct children of the hooks/ directory are supported: they are the +/// only shape the builtin manifest tracks, so nested asset paths are skipped +/// by the caller rather than installed unreconciled. +fn install_and_reconcile_role_hooks( + dir: &Path, + shipped: &[(String, String)], + force: bool, +) -> Result<()> { + if !shipped.is_empty() { + info!("Installing built-in role hooks in {}", dir.display()); + } + let mut written = BTreeSet::new(); + for (name, content) in shipped { + let path = dir.join(name); + if path.exists() && !force { + debug!( + "Role hook file already exists, skipping: {}", + path.display() + ); + continue; + } + ensure_parent_exists(&path)?; + info!("Creating role hook file: {}", path.display()); + write_file_atomic(&path, content, Some(0o755))?; + written.insert(name.clone()); + } + + let names: BTreeSet = shipped.iter().map(|(name, _)| name.clone()).collect(); + // Reconciliation is best-effort housekeeping: a failure here must not + // abort startup (install_builtins), matching the agent-side policy. + if let Err(err) = builtin_manifest::reconcile_builtin_dir(dir, &names, &written) { + warn!( + "Failed to reconcile builtin role hooks in {}: {err}", + dir.display() + ); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -967,4 +1031,135 @@ Input 1 assert_eq!(parse_structure_prompt(prompt), (prompt, vec![])); } + + fn fixture(entries: &[(&str, &str)]) -> Vec<(String, String)> { + entries + .iter() + .map(|(name, content)| (name.to_string(), content.to_string())) + .collect() + } + + fn hooks_fixture_dir(label: &str) -> std::path::PathBuf { + let dir = crate::utils::temp_file(label, ""); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("hooks") + } + + #[test] + fn role_hooks_install_writes_scripts_and_manifest() { + let dir = hooks_fixture_dir("role-hooks-install-"); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("a.sh", "#!/bin/sh\n")]), false) + .unwrap(); + + assert_eq!( + std::fs::read_to_string(dir.join("a.sh")).unwrap(), + "#!/bin/sh\n" + ); + assert_eq!( + std::fs::read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "a.sh\n" + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(dir.join("a.sh")) + .unwrap() + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o755, "role hook scripts install executable"); + let manifest_mode = + std::fs::metadata(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)) + .unwrap() + .permissions() + .mode(); + assert_eq!( + manifest_mode & 0o111, + 0, + "the manifest must never be executable" + ); + } + let _ = std::fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_install_overwrites_only_with_force() { + let dir = hooks_fixture_dir("role-hooks-force-"); + let shipped = fixture(&[("a.sh", "new content\n")]); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("a.sh"), "SENTINEL").unwrap(); + + install_and_reconcile_role_hooks(&dir, &shipped, false).unwrap(); + assert_eq!( + std::fs::read_to_string(dir.join("a.sh")).unwrap(), + "SENTINEL" + ); + + install_and_reconcile_role_hooks(&dir, &shipped, true).unwrap(); + assert_eq!( + std::fs::read_to_string(dir.join("a.sh")).unwrap(), + "new content\n" + ); + let _ = std::fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_reconcile_removes_dropped_hook_and_keeps_user_files() { + let dir = hooks_fixture_dir("role-hooks-reconcile-"); + + install_and_reconcile_role_hooks( + &dir, + &fixture(&[("keep.sh", "#!/bin/sh\n"), ("drop.sh", "#!/bin/sh\n")]), + false, + ) + .unwrap(); + std::fs::write(dir.join("user.sh"), "user-owned").unwrap(); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("keep.sh", "#!/bin/sh\n")]), false) + .unwrap(); + + assert!( + !dir.join("drop.sh").exists(), + "dropped builtin hook removed" + ); + assert!(dir.join("keep.sh").exists()); + assert!(dir.join("user.sh").exists(), "user file survives reconcile"); + assert_eq!( + std::fs::read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(), + "keep.sh\n" + ); + let _ = std::fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_never_claim_a_preexisting_user_file() { + let dir = hooks_fixture_dir("role-hooks-unclaimed-"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("a.sh"), "user-owned").unwrap(); + + install_and_reconcile_role_hooks(&dir, &fixture(&[("a.sh", "#!/bin/sh\n")]), false) + .unwrap(); + assert!( + !dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE).exists(), + "a skipped pre-existing file must not be claimed by the manifest" + ); + + install_and_reconcile_role_hooks(&dir, &[], false).unwrap(); + assert_eq!( + std::fs::read_to_string(dir.join("a.sh")).unwrap(), + "user-owned", + "the user file survives the hook being dropped from the embed" + ); + let _ = std::fs::remove_dir_all(dir.parent().unwrap()); + } + + #[test] + fn role_hooks_empty_shipped_set_creates_nothing() { + let dir = hooks_fixture_dir("role-hooks-empty-"); + + install_and_reconcile_role_hooks(&dir, &[], false).unwrap(); + + assert!(!dir.exists(), "an empty shipped set must not create hooks/"); + let _ = std::fs::remove_dir_all(dir.parent().unwrap()); + } } diff --git a/src/sandbox/mixins.rs b/src/sandbox/mixins.rs index 100b06b..7294be8 100644 --- a/src/sandbox/mixins.rs +++ b/src/sandbox/mixins.rs @@ -965,6 +965,12 @@ network: let hook_mixin = roles.join("hooks").join("notify.sbx-mixin.yaml"); fs::write(&role_mixin, "kind: mixin\n").unwrap(); fs::write(&hook_mixin, "kind: mixin\n").unwrap(); + // The builtin installer's hidden manifest lives in the same dir but + // is not a mixin and must never be picked up by the flat scan. + let manifest = roles + .join("hooks") + .join(crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE); + fs::write(&manifest, "notify.sh\n").unwrap(); let config_env = get_env_name("config_dir"); let roles_env = get_env_name("roles_dir"); @@ -998,6 +1004,10 @@ network: found.contains(&hook_mixin), "missing role hook mixin: {found:?}" ); + assert!( + !found.contains(&manifest), + "builtin manifest must not be scanned as a mixin: {found:?}" + ); let _ = fs::remove_dir_all(&root); }