From 0615194a36758dd91872f391558eb910786a1029 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Thu, 30 Apr 2026 16:37:05 -0700 Subject: [PATCH] support skill-scoped hooks --- codex-rs/analytics/src/events.rs | 1 + .../schema/json/ServerNotification.json | 1 + .../codex_app_server_protocol.schemas.json | 1 + .../codex_app_server_protocol.v2.schemas.json | 1 + .../json/v2/HookCompletedNotification.json | 1 + .../json/v2/HookStartedNotification.json | 1 + .../schema/json/v2/HooksListResponse.json | 1 + .../schema/typescript/v2/HookSource.ts | 2 +- .../app-server-protocol/src/protocol/v2.rs | 1 + codex-rs/core-skills/src/loader.rs | 38 +++-- codex-rs/core-skills/src/loader_tests.rs | 40 +++++ codex-rs/core-skills/src/model.rs | 9 + codex-rs/core/src/hook_runtime.rs | 9 +- codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/session/turn.rs | 16 +- codex-rs/core/src/session/turn_context.rs | 23 +++ codex-rs/core/src/skills.rs | 38 +++++ codex-rs/core/tests/suite/hooks.rs | 161 ++++++++++++++++++ codex-rs/hooks/src/engine/discovery.rs | 39 +++++ codex-rs/hooks/src/engine/mod.rs | 33 ++++ codex-rs/hooks/src/lib.rs | 1 + codex-rs/hooks/src/registry.rs | 17 ++ codex-rs/protocol/src/protocol.rs | 2 + .../tui/src/bottom_pane/hooks_browser_view.rs | 7 + 24 files changed, 426 insertions(+), 18 deletions(-) diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 7120960b62..1919343906 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -685,6 +685,7 @@ fn analytics_hook_source(source: HookSource) -> &'static str { HookSource::Mdm => "mdm", HookSource::SessionFlags => "session_flags", HookSource::Plugin => "plugin", + HookSource::Skill => "skill", HookSource::CloudRequirements => "cloud_requirements", HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", HookSource::LegacyManagedConfigMdm => "legacy_managed_config_mdm", diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 82914f3a6f..c2c5db8cd7 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1902,6 +1902,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index b0687e7089..f6bd60e1c3 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -9986,6 +9986,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index c00abca266..4485f29b6b 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -6595,6 +6595,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json index d55c059a73..5ef458bd84 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookCompletedNotification.json @@ -161,6 +161,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json index 03d2998ca5..1884d1aa66 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HookStartedNotification.json @@ -161,6 +161,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json index 5190b22711..6db64088be 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/HooksListResponse.json @@ -117,6 +117,7 @@ "mdm", "sessionFlags", "plugin", + "skill", "cloudRequirements", "legacyManagedConfigFile", "legacyManagedConfigMdm", diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts b/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts index 98bbe1e412..6ce13ff34f 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/HookSource.ts @@ -2,4 +2,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. -export type HookSource = "system" | "user" | "project" | "mdm" | "sessionFlags" | "plugin" | "cloudRequirements" | "legacyManagedConfigFile" | "legacyManagedConfigMdm" | "unknown"; +export type HookSource = "system" | "user" | "project" | "mdm" | "sessionFlags" | "plugin" | "skill" | "cloudRequirements" | "legacyManagedConfigFile" | "legacyManagedConfigMdm" | "unknown"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 068a9c5f5e..f56b654140 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -472,6 +472,7 @@ v2_enum_from_core!( Mdm, SessionFlags, Plugin, + Skill, CloudRequirements, LegacyManagedConfigFile, LegacyManagedConfigMdm, diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index d7a69e8a25..5474968910 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -10,6 +10,7 @@ use crate::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; +use codex_config::HookEventsToml; use codex_config::default_project_root_markers; use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; @@ -42,6 +43,8 @@ struct SkillFrontmatter { description: Option, #[serde(default)] metadata: SkillFrontmatterMetadata, + #[serde(default)] + hooks: HookEventsToml, } #[derive(Debug, Default, Deserialize)] @@ -554,8 +557,11 @@ async fn discover_skills_under_root( if metadata.is_file && file_name == SKILLS_FILENAME { match parse_skill_file(fs, &path, scope).await { - Ok(skill) => { - outcome.skills.push(skill); + Ok(parsed) => { + outcome + .hooks_by_skill_path + .insert(parsed.metadata.path_to_skills_md.clone(), parsed.hooks); + outcome.skills.push(parsed.metadata); } Err(err) => { if scope != SkillScope::System { @@ -583,7 +589,7 @@ async fn parse_skill_file( fs: &dyn ExecutorFileSystem, path: &AbsolutePathBuf, scope: SkillScope, -) -> Result { +) -> Result { let contents = fs .read_file_text(path, /*sandbox*/ None) .await @@ -630,18 +636,26 @@ async fn parse_skill_file( let resolved_path = canonicalize_for_skill_identity(path); - Ok(SkillMetadata { - name, - description, - short_description, - interface, - dependencies, - policy, - path_to_skills_md: resolved_path, - scope, + Ok(ParsedSkill { + metadata: SkillMetadata { + name, + description, + short_description, + interface, + dependencies, + policy, + path_to_skills_md: resolved_path, + scope, + }, + hooks: parsed.hooks, }) } +struct ParsedSkill { + metadata: SkillMetadata, + hooks: HookEventsToml, +} + fn default_skill_name(path: &AbsolutePathBuf) -> String { path.parent() .and_then(|parent| { diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index a12f09f80f..d82b8cfe4f 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -328,6 +328,46 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<() Ok(()) } +#[tokio::test] +async fn parses_hooks_from_skill_frontmatter() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let skill_dir = codex_home.path().join("skills").join("secure-operations"); + fs::create_dir_all(&skill_dir)?; + fs::write( + skill_dir.join(SKILLS_FILENAME), + r#"--- +name: secure-operations +description: security checks +hooks: + PreToolUse: + - matcher: "Bash" + hooks: + - type: command + command: "./scripts/security-check.sh" +--- + +# Body +"#, + )?; + + let outcome = load_skills_from_roots(vec![SkillRoot { + path: codex_home.path().join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + }]) + .await; + + let skill = outcome.skills.first().expect("skill should load"); + let hooks = outcome + .hooks_for_skill(skill) + .expect("skill hooks should be retained"); + assert_eq!(hooks.pre_tool_use.len(), 1); + assert_eq!(hooks.pre_tool_use[0].matcher.as_deref(), Some("Bash")); + assert_eq!(hooks.handler_count(), 1); + + Ok(()) +} + fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { write_skill_at(&codex_home.path().join("skills"), dir, name, description) } diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 0a72c24fe8..65e230c5ee 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use std::fmt; use std::sync::Arc; +use codex_config::HookEventsToml; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -92,6 +93,7 @@ pub struct SkillLoadOutcome { pub(crate) skill_roots: Vec, pub(crate) skill_root_by_path: Arc>, pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, + pub(crate) hooks_by_skill_path: HashMap, pub(crate) implicit_skills_by_scripts_dir: Arc>, pub(crate) implicit_skills_by_doc_path: Arc>, } @@ -126,6 +128,10 @@ impl SkillLoadOutcome { self.file_systems_by_skill_path .get(&skill.path_to_skills_md) } + + pub fn hooks_for_skill(&self, skill: &SkillMetadata) -> Option<&HookEventsToml> { + self.hooks_by_skill_path.get(&skill.path_to_skills_md) + } } #[derive(Clone, Default)] @@ -178,6 +184,9 @@ pub fn filter_skill_load_outcome_for_product( outcome .file_systems_by_skill_path .retain_paths(&retained_paths); + outcome + .hooks_by_skill_path + .retain(|path, _| retained_paths.contains(path)); outcome.skill_root_by_path = Arc::new( outcome .skill_root_by_path diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 9a92854515..1175c9637b 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -116,7 +116,7 @@ pub(crate) async fn run_pending_session_start_hooks( permission_mode: hook_permission_mode(turn_context), source: session_start_source, }; - let hooks = sess.hooks(); + let hooks = turn_context.turn_skills.hooks_for_turn(sess.hooks()); let preview_runs = hooks.preview_session_start(&request); run_context_injecting_hook( sess, @@ -153,7 +153,7 @@ pub(crate) async fn run_pre_tool_use_hooks( tool_use_id, tool_input: tool_input.clone(), }; - let hooks = sess.hooks(); + let hooks = turn_context.turn_skills.hooks_for_turn(sess.hooks()); let preview_runs = hooks.preview_pre_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -203,7 +203,7 @@ pub(crate) async fn run_permission_request_hooks( run_id_suffix: run_id_suffix.to_string(), tool_input: payload.tool_input, }; - let hooks = sess.hooks(); + let hooks = turn_context.turn_skills.hooks_for_turn(sess.hooks()); let preview_runs = hooks.preview_permission_request(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -244,7 +244,7 @@ pub(crate) async fn run_post_tool_use_hooks( tool_input, tool_response, }; - let hooks = sess.hooks(); + let hooks = turn_context.turn_skills.hooks_for_turn(sess.hooks()); let preview_runs = hooks.preview_post_tool_use(&request); emit_hook_started_events(sess, turn_context, preview_runs).await; @@ -478,6 +478,7 @@ fn hook_run_metric_tags(run: &HookRunSummary) -> [(&'static str, &'static str); HookSource::Mdm => "mdm", HookSource::SessionFlags => "session_flags", HookSource::Plugin => "plugin", + HookSource::Skill => "skill", HookSource::CloudRequirements => "cloud_requirements", HookSource::LegacyManagedConfigFile => "legacy_managed_config_file", HookSource::LegacyManagedConfigMdm => "legacy_managed_config_mdm", diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 6a61079a3b..d9e158d105 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -91,6 +91,7 @@ pub(crate) use skills::SkillLoadOutcome; pub(crate) use skills::SkillMetadata; pub(crate) use skills::SkillsLoadInput; pub(crate) use skills::SkillsManager; +pub(crate) use skills::active_skill_hook_sources; pub(crate) use skills::build_available_skills; pub(crate) use skills::build_skill_injections; pub(crate) use skills::build_skill_name_counts; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 286aa2d687..481c8d9a06 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -5,6 +5,7 @@ use std::sync::atomic::Ordering; use crate::SkillInjections; use crate::SkillLoadOutcome; +use crate::active_skill_hook_sources; use crate::build_skill_injections; use crate::client::ModelClientSession; use crate::client_common::Prompt; @@ -222,6 +223,19 @@ pub(crate) async fn run_turn( &connector_slug_counts, ) }); + if turn_context.features.enabled(Feature::CodexHooks) + && let Some(outcome) = skills_outcome + { + let (skill_hook_sources, skill_hook_warnings) = + active_skill_hook_sources(outcome, &mentioned_skills); + turn_context + .turn_skills + .set_active_hook_sources(skill_hook_sources); + for message in skill_hook_warnings { + sess.send_event(&turn_context, EventMsg::Warning(WarningEvent { message })) + .await; + } + } let config = turn_context.config.clone(); if config .features @@ -520,7 +534,7 @@ pub(crate) async fn run_turn( stop_hook_active, last_assistant_message: last_agent_message.clone(), }; - let hooks = sess.hooks(); + let hooks = turn_context.turn_skills.hooks_for_turn(sess.hooks()); for run in hooks.preview_stop(&stop_request) { sess.send_event( &turn_context, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 410e16703a..44c1447957 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -18,6 +18,7 @@ pub(super) fn image_generation_tool_auth_allowed(auth_manager: Option<&AuthManag pub(crate) struct TurnSkillsContext { pub(crate) outcome: Arc, pub(crate) implicit_invocation_seen_skills: Arc>>, + active_hook_sources: Arc>>, } impl TurnSkillsContext { @@ -25,6 +26,28 @@ impl TurnSkillsContext { Self { outcome, implicit_invocation_seen_skills: Arc::new(Mutex::new(HashSet::new())), + active_hook_sources: Arc::new(std::sync::RwLock::new(Vec::new())), + } + } + + pub(crate) fn set_active_hook_sources(&self, sources: Vec) { + let mut active_hook_sources = self + .active_hook_sources + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + *active_hook_sources = sources; + } + + pub(crate) fn hooks_for_turn(&self, base_hooks: Arc) -> Arc { + let active_hook_sources = self + .active_hook_sources + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone(); + if active_hook_sources.is_empty() { + base_hooks + } else { + Arc::new(base_hooks.with_skill_hook_sources(active_hook_sources)) } } } diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 0681d09624..30ae827eb0 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -9,6 +9,7 @@ use crate::session::turn_context::TurnContext; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::build_track_events_context; +use codex_hooks::SkillHookSource; use codex_protocol::protocol::SkillScope; use codex_protocol::request_user_input::RequestUserInputArgs; use codex_protocol::request_user_input::RequestUserInputQuestion; @@ -230,3 +231,40 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( vec![invocation], ); } + +pub(crate) fn active_skill_hook_sources( + outcome: &SkillLoadOutcome, + skills: &[SkillMetadata], +) -> (Vec, Vec) { + let mut sources = Vec::new(); + let mut warnings = Vec::new(); + + for skill in skills { + let Some(hooks) = outcome.hooks_for_skill(skill) else { + continue; + }; + if hooks.is_empty() { + continue; + } + + let mut hooks = hooks.clone(); + if !hooks.session_start.is_empty() { + warnings.push(format!( + "Skipping SessionStart hooks from skill {} because skills activate after thread start.", + skill.name + )); + hooks.session_start.clear(); + } + if hooks.is_empty() { + continue; + } + + sources.push(SkillHookSource { + skill_name: skill.name.clone(), + source_path: skill.path_to_skills_md.clone(), + hooks, + }); + } + + (sources, warnings) +} diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index daf3053adc..402ec1a5ba 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -1975,6 +1975,167 @@ print(json.dumps({{ Ok(()) } +#[tokio::test] +async fn skill_pre_tool_use_blocks_only_while_skill_is_active() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let first_call_id = "skill-pretooluse-first-shell-command"; + let second_call_id = "skill-pretooluse-second-shell-command"; + let first_marker = std::env::temp_dir().join("skill-pretooluse-first-shell-command-marker"); + let second_marker = std::env::temp_dir().join("skill-pretooluse-second-shell-command-marker"); + let first_command = format!("printf blocked > {}", first_marker.display()); + let second_command = format!("printf allowed > {}", second_marker.display()); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + first_call_id, + "shell_command", + &serde_json::to_string(&serde_json::json!({ "command": first_command }))?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "skill hook blocked it"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + core_test_support::responses::ev_function_call( + second_call_id, + "shell_command", + &serde_json::to_string(&serde_json::json!({ "command": second_command }))?, + ), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_assistant_message("msg-2", "skill hook is inactive now"), + ev_completed("resp-4"), + ]), + ], + ) + .await; + + let home = Arc::new(TempDir::new()?); + let skill_dir = home.path().join("skills/secure-operations"); + fs::create_dir_all(&skill_dir).context("create skill dir")?; + let script_path = skill_dir.join("pre_tool_use_hook.py"); + let log_path = skill_dir.join("pre_tool_use_hook_log.jsonl"); + fs::write( + &script_path, + format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": "blocked by skill hook" + }} +}})) +"#, + log_path = log_path.display(), + ), + ) + .context("write skill pre tool use hook script")?; + fs::write( + skill_dir.join("SKILL.md"), + format!( + r#"--- +name: secure-operations +description: Perform operations with security checks +hooks: + PreToolUse: + - matcher: "^Bash$" + hooks: + - type: command + command: "python3 {script_path}" +--- + +# Body +"#, + script_path = script_path.display(), + ), + ) + .context("write skill")?; + + let mut builder = test_codex() + .with_home(Arc::clone(&home)) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + for marker in [&first_marker, &second_marker] { + if marker.exists() { + fs::remove_file(marker).context("remove leftover skill pre tool use marker")?; + } + } + + test.submit_turn_with_policy( + "use $secure-operations and run the shell command", + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .await?; + test.submit_turn_with_policy( + "run the shell command without the skill", + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 4); + let first_output_item = requests[1].function_call_output(first_call_id); + let first_output = first_output_item + .get("output") + .and_then(Value::as_str) + .expect("first shell command output string"); + assert!( + first_output.contains("Command blocked by PreToolUse hook: blocked by skill hook"), + "active skill hook should block the shell command", + ); + let second_output_item = requests[3].function_call_output(second_call_id); + let second_output = second_output_item + .get("output") + .and_then(Value::as_str) + .expect("second shell command output string"); + assert!( + !second_output.contains("Command blocked by PreToolUse hook"), + "inactive skill hook should not block later turns", + ); + assert!( + !first_marker.exists(), + "skill hook should block the first command" + ); + assert!( + second_marker.exists(), + "skill hook should be cleaned up before the next turn", + ); + + let hook_inputs = read_hook_inputs_from_log(&log_path)?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!(hook_inputs[0]["hook_event_name"], "PreToolUse"); + assert_eq!(hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(hook_inputs[0]["tool_use_id"], first_call_id); + assert_eq!(hook_inputs[0]["tool_input"]["command"], first_command); + + Ok(()) +} + #[tokio::test] async fn pre_tool_use_blocks_shell_when_defined_in_config_toml() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/engine/discovery.rs b/codex-rs/hooks/src/engine/discovery.rs index 8c520b7497..8185314068 100644 --- a/codex-rs/hooks/src/engine/discovery.rs +++ b/codex-rs/hooks/src/engine/discovery.rs @@ -23,6 +23,7 @@ use super::HookListEntry; use crate::config_rules::disabled_hook_keys_from_stack; use crate::events::common::matcher_pattern_for_event; use crate::events::common::validate_matcher_pattern; +use crate::registry::SkillHookSource; use codex_protocol::protocol::HookHandlerType; use codex_protocol::protocol::HookSource; @@ -197,6 +198,44 @@ fn append_plugin_hook_sources( } } +pub(crate) fn append_skill_hook_sources( + handlers: &mut Vec, + warnings: &mut Vec, + mut display_order: i64, + skill_hook_sources: Vec, +) { + let disabled_hook_keys = HashSet::new(); + let mut hook_entries = Vec::new(); + for source in skill_hook_sources { + let SkillHookSource { + skill_name, + source_path, + mut hooks, + } = source; + if !hooks.session_start.is_empty() { + warnings.push(format!( + "skipping SessionStart hooks from skill {skill_name}: skills activate after thread start" + )); + hooks.session_start.clear(); + } + append_hook_events( + handlers, + &mut hook_entries, + warnings, + &mut display_order, + HookHandlerSource { + path: &source_path, + key_source: format!("skill:{}", source_path.display()), + source: HookSource::Skill, + disabled_hook_keys: &disabled_hook_keys, + env: HashMap::new(), + plugin_id: None, + }, + hooks, + ); + } +} + fn managed_hooks_source_path( managed_hooks: &ManagedHooksRequirementsToml, requirement_source: Option<&RequirementSource>, diff --git a/codex-rs/hooks/src/engine/mod.rs b/codex-rs/hooks/src/engine/mod.rs index c06c9fabd7..299435117c 100644 --- a/codex-rs/hooks/src/engine/mod.rs +++ b/codex-rs/hooks/src/engine/mod.rs @@ -6,6 +6,7 @@ pub(crate) mod schema_loader; use std::collections::HashMap; +use crate::registry::SkillHookSource; use codex_config::ConfigLayerStack; use codex_plugin::PluginHookSource; use codex_protocol::protocol::HookEventName; @@ -87,6 +88,7 @@ pub struct HookListEntry { #[derive(Clone)] pub(crate) struct ClaudeHooksEngine { + enabled: bool, handlers: Vec, warnings: Vec, shell: CommandShell, @@ -102,6 +104,7 @@ impl ClaudeHooksEngine { ) -> Self { if !enabled { return Self { + enabled, handlers: Vec::new(), warnings: Vec::new(), shell, @@ -115,6 +118,7 @@ impl ClaudeHooksEngine { plugin_hook_load_warnings, ); Self { + enabled, handlers: discovered.handlers, warnings: discovered.warnings, shell, @@ -125,6 +129,35 @@ impl ClaudeHooksEngine { &self.warnings } + pub(crate) fn with_skill_hook_sources(&self, skill_hook_sources: Vec) -> Self { + if skill_hook_sources.is_empty() { + return self.clone(); + } + if !self.enabled { + return self.clone(); + } + + let mut handlers = self.handlers.clone(); + let mut warnings = self.warnings.clone(); + let next_display_order = handlers + .iter() + .map(|handler| handler.display_order) + .max() + .map_or(0, |display_order| display_order + 1); + discovery::append_skill_hook_sources( + &mut handlers, + &mut warnings, + next_display_order, + skill_hook_sources, + ); + Self { + enabled: self.enabled, + handlers, + warnings, + shell: self.shell.clone(), + } + } + pub(crate) fn preview_session_start( &self, request: &SessionStartRequest, diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index 4e16969a58..d65222fbb3 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -47,6 +47,7 @@ pub use legacy_notify::notify_hook; pub use registry::HookListOutcome; pub use registry::Hooks; pub use registry::HooksConfig; +pub use registry::SkillHookSource; pub use registry::command_from_argv; pub use registry::list_hooks; pub use schema::write_schema_fixtures; diff --git a/codex-rs/hooks/src/registry.rs b/codex-rs/hooks/src/registry.rs index ae80015729..a53790c3ff 100644 --- a/codex-rs/hooks/src/registry.rs +++ b/codex-rs/hooks/src/registry.rs @@ -1,5 +1,7 @@ use codex_config::ConfigLayerStack; +use codex_config::HookEventsToml; use codex_plugin::PluginHookSource; +use codex_utils_absolute_path::AbsolutePathBuf; use tokio::process::Command; use crate::engine::ClaudeHooksEngine; @@ -33,6 +35,13 @@ pub struct HooksConfig { pub shell_args: Vec, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillHookSource { + pub skill_name: String, + pub source_path: AbsolutePathBuf, + pub hooks: HookEventsToml, +} + #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct HookListOutcome { pub hooks: Vec, @@ -81,6 +90,14 @@ impl Hooks { self.engine.warnings() } + pub fn with_skill_hook_sources(&self, skill_hook_sources: Vec) -> Self { + Self { + after_agent: self.after_agent.clone(), + after_tool_use: self.after_tool_use.clone(), + engine: self.engine.with_skill_hook_sources(skill_hook_sources), + } + } + fn hooks_for_event(&self, hook_event: &HookEvent) -> &[Hook] { match hook_event { HookEvent::AfterAgent { .. } => &self.after_agent, diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 9445c1994e..42c38d7387 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1573,6 +1573,7 @@ pub enum HookSource { Mdm, SessionFlags, Plugin, + Skill, CloudRequirements, LegacyManagedConfigFile, LegacyManagedConfigMdm, @@ -4020,6 +4021,7 @@ mod tests { assert_eq!(HookSource::Project.is_managed(), false); assert_eq!(HookSource::SessionFlags.is_managed(), false); assert_eq!(HookSource::Plugin.is_managed(), false); + assert_eq!(HookSource::Skill.is_managed(), false); assert_eq!(HookSource::Unknown.is_managed(), false); } diff --git a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs index 2f4c6a8a0d..f814890c7b 100644 --- a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs +++ b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs @@ -564,6 +564,7 @@ fn hook_source_summary(hook: &HookMetadata) -> String { .as_deref() .map(|plugin_id| format!("Plugin - {plugin_id}")) .unwrap_or_else(|| "Plugin".to_string()), + HookSource::Skill => "Skill".to_string(), _ => config_source_label(hook.source).to_string(), } } @@ -571,6 +572,11 @@ fn hook_source_summary(hook: &HookMetadata) -> String { fn detail_source_value(hook: &HookMetadata) -> String { match hook.source { HookSource::Plugin => hook_source_summary(hook), + HookSource::Skill => format!( + "{} - {}", + config_source_label(hook.source), + format_directory_display(&hook.source_path, /*max_width*/ None) + ), HookSource::System | HookSource::Mdm | HookSource::CloudRequirements @@ -592,6 +598,7 @@ fn config_source_label(source: HookSource) -> &'static str { HookSource::Mdm => "Admin config", HookSource::SessionFlags => "Session flags", HookSource::Plugin => unreachable!("plugin hooks are handled by summary_source"), + HookSource::Skill => "Skill", HookSource::CloudRequirements => "Admin config", HookSource::LegacyManagedConfigFile => "Admin config", HookSource::LegacyManagedConfigMdm => "Admin config",