feat(TASK-039): wire hooks into bundle installs and reconcile builtin hook dirs
Bundles: RemoteLayout.hooks + scan/is_empty/plan routing into hooks_dir, a functional --filter hooks arm replacing the inert interim arm, TopCategory::Hooks provenance rows, and per-file 'bundle defines hook script' summary lines (visibility only, no approval prompt). Reconciliation: new builtin_manifest module — a hidden per-directory .builtin-manifest records the files the builtin installer actually owns (written this run, previously owned, or pending failed deletions); deletion is strictly manifest-minus-embed so user-created files in the same directory are never touched. Wired into install_builtin_agents for agents/<name>/hooks/ and a greenfield minimal Role::install_builtin_role_hooks for roles/hooks/. Hardening: bundles are barred from shipping any path component that aliases .builtin-manifest (Unicode case fold + trailing dot/space, directories included); manifest entries reuse the bundle store's is_safe_component rules; malformed manifests fail safe toward keeping files; role-side reconcile failures degrade to a warning like the agent side.
This commit is contained in:
7 files changed
+893
-8
No files matched your search
@@ -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<String, String>;
|
||||
#[folder = "assets/agents/"]
|
||||
struct AgentAssets;
|
||||
|
||||
/// Splits an embedded asset path of the form `<agent>/hooks/<file>` 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<String, BTreeSet<String>> = 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<String> = 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<String>
|
||||
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#"
|
||||
|
||||
@@ -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<String>,
|
||||
written: &BTreeSet<String>,
|
||||
) -> Result<()> {
|
||||
let previous = read_manifest(dir);
|
||||
let mut retained: BTreeSet<String> = 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<String> {
|
||||
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<String>) -> 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::<Vec<_>>().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<String> {
|
||||
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"));
|
||||
}
|
||||
}
|
||||
@@ -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<InstallFilter>, 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<PathBuf>,
|
||||
skills: Option<PathBuf>,
|
||||
macros: Option<PathBuf>,
|
||||
hooks: Option<PathBuf>,
|
||||
functions_tools: Option<PathBuf>,
|
||||
mcp_json: Option<PathBuf>,
|
||||
manifest: Option<BundleManifest>,
|
||||
@@ -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<RemoteLayout> {
|
||||
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<InstallFilter>) -> 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<InstallPlan> {
|
||||
&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<String> {
|
||||
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/<name>/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,
|
||||
}],
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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 `<roles_dir>/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<String> = 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());
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user