style: applied improved styling to the whole of the hooks integration
This commit is contained in:
17 files changed
+92
-176
No files matched your search
+1
-1
@@ -1489,7 +1489,7 @@ nodes:
|
||||
type: end
|
||||
output: done
|
||||
";
|
||||
let graph: graph::Graph = serde_yaml::from_str(yaml).unwrap();
|
||||
let graph: Graph = serde_yaml::from_str(yaml).unwrap();
|
||||
|
||||
let config = AgentConfig::from_graph("g", &graph);
|
||||
|
||||
|
||||
@@ -807,7 +807,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn from_config_copies_hooks() {
|
||||
let mut hooks = crate::hooks::HooksMap::new();
|
||||
let mut hooks = HooksMap::new();
|
||||
hooks.insert(
|
||||
"turn.completed".to_string(),
|
||||
vec![crate::hooks::HookDef {
|
||||
@@ -845,7 +845,7 @@ mod tests {
|
||||
let env_name = get_env_name("hooks");
|
||||
let prev = env::var_os(&env_name);
|
||||
|
||||
let mut hooks = crate::hooks::HooksMap::new();
|
||||
let mut hooks = HooksMap::new();
|
||||
hooks.insert(
|
||||
"turn.completed".to_string(),
|
||||
vec![crate::hooks::HookDef {
|
||||
|
||||
@@ -15,16 +15,8 @@ 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()
|
||||
@@ -36,8 +28,8 @@ pub(crate) fn is_builtin_manifest_name(name: &str) -> bool {
|
||||
/// 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.
|
||||
/// later run retries). A shipped name the installer declined to write (e.g., a
|
||||
/// pre-existing user file with a colliding name) is never claimed.
|
||||
///
|
||||
/// Failure modes degrade toward keeping files: a missing or unreadable
|
||||
/// manifest yields no deletions, and entries that are not plain filenames
|
||||
@@ -137,8 +129,9 @@ fn write_manifest(dir: &Path, names: &BTreeSet<String>) -> Result<()> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::utils;
|
||||
use std::path::PathBuf;
|
||||
|
||||
fn fresh_dir(label: &str) -> std::path::PathBuf {
|
||||
fn fresh_dir(label: &str) -> PathBuf {
|
||||
let dir = utils::temp_file(label, "");
|
||||
fs::create_dir_all(&dir).unwrap();
|
||||
dir
|
||||
|
||||
@@ -2418,6 +2418,7 @@ fn print_secret_summary(added: &[String], deferred: &[String]) {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::config;
|
||||
use crate::config::builtin_manifest::BUILTIN_MANIFEST_FILE;
|
||||
use crate::sandbox::SANDBOX_ENV_FLAG;
|
||||
use crate::utils::get_env_name;
|
||||
@@ -2710,8 +2711,6 @@ 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 {
|
||||
@@ -3624,8 +3623,9 @@ mod tests {
|
||||
fn assert_executable(path: &Path) {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
let mode = fs::metadata(path).unwrap().permissions().mode();
|
||||
assert!(
|
||||
mode & 0o111 != 0,
|
||||
assert_ne!(
|
||||
mode & 0o111,
|
||||
0,
|
||||
"{} must carry the executable bit",
|
||||
path.display()
|
||||
);
|
||||
@@ -3932,7 +3932,7 @@ mod tests {
|
||||
}
|
||||
let _guard = TestVaultConfigGuard::new("prov-builtin");
|
||||
|
||||
crate::config::install_assets(crate::config::AssetCategory::Macros).unwrap();
|
||||
config::install_assets(config::AssetCategory::Macros).unwrap();
|
||||
|
||||
assert!(fs::read_dir(paths::macros_dir()).unwrap().next().is_some());
|
||||
assert!(!paths::installed_bundles_file().exists());
|
||||
@@ -3957,7 +3957,7 @@ mod tests {
|
||||
|
||||
// Capture outcomes before restoring the env var so a failed assertion
|
||||
// cannot leave the hooks-dir override pointing at a deleted temp dir.
|
||||
let result = crate::config::install_assets(crate::config::AssetCategory::Hooks);
|
||||
let result = config::install_assets(AssetCategory::Hooks);
|
||||
let installed = (
|
||||
hooks_dir.join("notify.sh").is_file(),
|
||||
hooks_dir.join("log-events.sh").is_file(),
|
||||
|
||||
+3
-2
@@ -83,6 +83,7 @@ use crate::utils::*;
|
||||
pub use macros::macro_execute;
|
||||
|
||||
use crate::config::macros::Macro;
|
||||
use crate::hooks;
|
||||
use crate::vault::{
|
||||
GlobalVault, Vault, create_vault_password_file, interpolate_secrets, prompt_provider_choice,
|
||||
};
|
||||
@@ -420,7 +421,7 @@ pub fn install_builtins() -> Result<()> {
|
||||
Agent::install_builtin_agents(false)?;
|
||||
Macro::install_macros(false)?;
|
||||
Skill::install_builtin_skills(false)?;
|
||||
crate::hooks::install_builtin_hooks(false)?;
|
||||
hooks::install_builtin_hooks(false)?;
|
||||
Role::install_builtin_role_hooks(false)?;
|
||||
Ok(())
|
||||
}
|
||||
@@ -522,7 +523,7 @@ pub fn install_assets(category: AssetCategory) -> Result<()> {
|
||||
AssetCategory::Macros => Macro::install_macros(true)?,
|
||||
AssetCategory::Functions => Functions::install_builtin_global_tools(true)?,
|
||||
AssetCategory::Skills => Skill::install_builtin_skills(true)?,
|
||||
AssetCategory::Hooks => crate::hooks::install_builtin_hooks(true)?,
|
||||
AssetCategory::Hooks => hooks::install_builtin_hooks(true)?,
|
||||
AssetCategory::McpConfig => Functions::install_mcp_config()?,
|
||||
}
|
||||
|
||||
|
||||
@@ -4733,8 +4733,6 @@ impl RequestContext {
|
||||
}
|
||||
self.refresh_mcp_tool_filters();
|
||||
self.init_agent_session_variables(new_session)?;
|
||||
// NEW sessions only, and only once the session is fully engaged on
|
||||
// the context; resuming an existing session fires nothing.
|
||||
if created_new_session {
|
||||
hooks::fire(HookEvent::SessionStarted, self, &[], None);
|
||||
}
|
||||
@@ -4933,24 +4931,18 @@ impl RequestContext {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Opens the top-level `agent.*` bracket: fires `agent.started` once per
|
||||
/// agent-context lifetime when the ROOT context runs with a loaded agent
|
||||
/// (headless `--agent` dispatch, or entering an agent interactively).
|
||||
/// Spawned sub-agents are bracketed at their spawn seam instead, never
|
||||
/// here. Synthesizes and stores `COYOTE_AGENT_ID` under the
|
||||
/// `top-<agent-name>-<8-char alphanumeric nonce>` scheme; calling this
|
||||
/// again while the bracket is open, or without an agent, is a no-op —
|
||||
/// per-message granularity belongs to `turn.*`, not `agent.*`.
|
||||
pub fn top_level_agent_started(&mut self) {
|
||||
if self.top_level_agent_id.is_some() {
|
||||
return;
|
||||
}
|
||||
|
||||
let Some(agent) = self.agent.as_ref() else {
|
||||
return;
|
||||
};
|
||||
let nonce = Alphanumeric.sample_string(&mut rand::rng(), 8);
|
||||
let id = format!("top-{}-{nonce}", agent.name());
|
||||
self.top_level_agent_id = Some(id.clone());
|
||||
|
||||
hooks::fire(
|
||||
HookEvent::AgentStarted,
|
||||
self,
|
||||
@@ -4959,15 +4951,11 @@ impl RequestContext {
|
||||
);
|
||||
}
|
||||
|
||||
/// Closes the top-level `agent.*` bracket: fires `agent.completed`, or
|
||||
/// `agent.failed` carrying `COYOTE_AGENT_ERROR` when the run ended in an
|
||||
/// error. No-op unless [`Self::top_level_agent_started`] opened the
|
||||
/// bracket; the stored id is consumed so exactly one terminal event
|
||||
/// fires per lifetime.
|
||||
pub fn top_level_agent_finished(&mut self, error: Option<&Error>) {
|
||||
let Some(id) = self.top_level_agent_id.take() else {
|
||||
return;
|
||||
};
|
||||
|
||||
match error {
|
||||
None => hooks::fire(
|
||||
HookEvent::AgentCompleted,
|
||||
|
||||
+22
-35
@@ -1039,9 +1039,9 @@ Input 1
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn hooks_fixture_dir(label: &str) -> std::path::PathBuf {
|
||||
let dir = crate::utils::temp_file(label, "");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
fn hooks_fixture_dir(label: &str) -> PathBuf {
|
||||
let dir = temp_file(label, "");
|
||||
create_dir_all(&dir).unwrap();
|
||||
dir.join("hooks")
|
||||
}
|
||||
|
||||
@@ -1052,55 +1052,42 @@ Input 1
|
||||
install_and_reconcile_role_hooks(&dir, &fixture(&[("a.sh", "#!/bin/sh\n")]), false)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "#!/bin/sh\n");
|
||||
assert_eq!(
|
||||
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(),
|
||||
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"))
|
||||
let mode = fs::metadata(dir.join("a.sh")).unwrap().permissions().mode();
|
||||
assert_eq!(mode & 0o777, 0o755, "role hook scripts install executable");
|
||||
let manifest_mode = fs::metadata(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE))
|
||||
.unwrap()
|
||||
.permissions()
|
||||
.mode();
|
||||
assert_eq!(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());
|
||||
let _ = fs::remove_dir_all(dir.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn role_hooks_install_overwrites_only_with_force() {
|
||||
let dir = hooks_fixture_dir("role-hooks-force-");
|
||||
let shipped = fixture(&[("a.sh", "new content\n")]);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
std::fs::write(dir.join("a.sh"), "SENTINEL").unwrap();
|
||||
create_dir_all(&dir).unwrap();
|
||||
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"
|
||||
);
|
||||
assert_eq!(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());
|
||||
assert_eq!(read_to_string(dir.join("a.sh")).unwrap(), "new content\n");
|
||||
let _ = fs::remove_dir_all(dir.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1113,7 +1100,7 @@ Input 1
|
||||
false,
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(dir.join("user.sh"), "user-owned").unwrap();
|
||||
fs::write(dir.join("user.sh"), "user-owned").unwrap();
|
||||
|
||||
install_and_reconcile_role_hooks(&dir, &fixture(&[("keep.sh", "#!/bin/sh\n")]), false)
|
||||
.unwrap();
|
||||
@@ -1125,17 +1112,17 @@ Input 1
|
||||
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(),
|
||||
read_to_string(dir.join(builtin_manifest::BUILTIN_MANIFEST_FILE)).unwrap(),
|
||||
"keep.sh\n"
|
||||
);
|
||||
let _ = std::fs::remove_dir_all(dir.parent().unwrap());
|
||||
let _ = fs::remove_dir_all(dir.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn role_hooks_never_claim_a_preexisting_user_file() {
|
||||
let dir = hooks_fixture_dir("role-hooks-unclaimed-");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
std::fs::write(dir.join("a.sh"), "user-owned").unwrap();
|
||||
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();
|
||||
@@ -1146,11 +1133,11 @@ Input 1
|
||||
|
||||
install_and_reconcile_role_hooks(&dir, &[], false).unwrap();
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(dir.join("a.sh")).unwrap(),
|
||||
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());
|
||||
let _ = fs::remove_dir_all(dir.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1160,6 +1147,6 @@ Input 1
|
||||
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());
|
||||
let _ = fs::remove_dir_all(dir.parent().unwrap());
|
||||
}
|
||||
}
|
||||
@@ -1288,7 +1288,7 @@ mod tests {
|
||||
fn session_deserialization_ignores_injected_role_hooks() {
|
||||
// A session file is attacker-writable data: a `role_hooks:` key
|
||||
// crafted into it must never deserialize into executable hook
|
||||
// config — hooks are only ever re-read from the role file itself.
|
||||
// config. Hooks are only ever re-read from the role file itself.
|
||||
let yaml = "model: ''\nmessages: []\nrole_hooks:\n session.started:\n - name: injected\n command: touch /tmp/pwned\n";
|
||||
let session: Session = serde_yaml::from_str(yaml).unwrap();
|
||||
assert!(
|
||||
|
||||
Reference in new issue
Block a user