diff --git a/rust/crates/plugins/src/lib.rs b/rust/crates/plugins/src/lib.rs index 321070a..ed1eb76 100644 --- a/rust/crates/plugins/src/lib.rs +++ b/rust/crates/plugins/src/lib.rs @@ -1456,6 +1456,12 @@ fn build_plugin_manifest( let permissions = build_manifest_permissions(&raw.permissions, &mut errors); validate_command_entries(root, raw.hooks.pre_tool_use.iter(), "hook", &mut errors); validate_command_entries(root, raw.hooks.post_tool_use.iter(), "hook", &mut errors); + validate_command_entries( + root, + raw.hooks.post_tool_use_failure.iter(), + "hook", + &mut errors, + ); validate_command_entries( root, raw.lifecycle.init.iter(), @@ -2111,6 +2117,16 @@ mod tests { ); } + fn write_broken_failure_hook_plugin(root: &Path, name: &str) { + write_file( + root.join(MANIFEST_RELATIVE_PATH).as_path(), + format!( + "{{\n \"name\": \"{name}\",\n \"version\": \"1.0.0\",\n \"description\": \"broken plugin\",\n \"hooks\": {{\n \"PostToolUseFailure\": [\"./hooks/missing-failure.sh\"]\n }}\n}}" + ) + .as_str(), + ); + } + fn write_lifecycle_plugin(root: &Path, name: &str, version: &str) -> PathBuf { let log_path = root.join("lifecycle.log"); write_file( @@ -2825,14 +2841,19 @@ mod tests { #[test] fn rejects_plugin_sources_with_missing_hook_paths() { + // given let config_home = temp_dir("broken-home"); let source_root = temp_dir("broken-source"); write_broken_plugin(&source_root, "broken"); let manager = PluginManager::new(PluginManagerConfig::new(&config_home)); + + // when let error = manager .validate_plugin_source(source_root.to_str().expect("utf8 path")) .expect_err("missing hook file should fail validation"); + + // then assert!(error.to_string().contains("does not exist")); let mut manager = PluginManager::new(PluginManagerConfig::new(&config_home)); @@ -2845,6 +2866,33 @@ mod tests { let _ = fs::remove_dir_all(source_root); } + #[test] + fn rejects_plugin_sources_with_missing_failure_hook_paths() { + // given + let config_home = temp_dir("broken-failure-home"); + let source_root = temp_dir("broken-failure-source"); + write_broken_failure_hook_plugin(&source_root, "broken-failure"); + + let manager = PluginManager::new(PluginManagerConfig::new(&config_home)); + + // when + let error = manager + .validate_plugin_source(source_root.to_str().expect("utf8 path")) + .expect_err("missing failure hook file should fail validation"); + + // then + assert!(error.to_string().contains("does not exist")); + + let mut manager = PluginManager::new(PluginManagerConfig::new(&config_home)); + let install_error = manager + .install(source_root.to_str().expect("utf8 path")) + .expect_err("install should reject invalid failure hook paths"); + assert!(install_error.to_string().contains("does not exist")); + + let _ = fs::remove_dir_all(config_home); + let _ = fs::remove_dir_all(source_root); + } + #[test] fn plugin_registry_runs_initialize_and_shutdown_for_enabled_plugins() { let config_home = temp_dir("lifecycle-home"); diff --git a/rust/crates/runtime/src/conversation.rs b/rust/crates/runtime/src/conversation.rs index c93e217..1218975 100644 --- a/rust/crates/runtime/src/conversation.rs +++ b/rust/crates/runtime/src/conversation.rs @@ -785,6 +785,7 @@ mod tests { use crate::prompt::{ProjectContext, SystemPromptBuilder}; use crate::session::{ContentBlock, MessageRole, Session}; use crate::usage::TokenUsage; + use crate::ToolError; use std::fs; use std::path::PathBuf; use std::sync::Arc; @@ -1202,6 +1203,85 @@ mod tests { ); } + #[test] + fn appends_post_tool_use_failure_hook_feedback_to_tool_result() { + struct TwoCallApiClient { + calls: usize, + } + + impl ApiClient for TwoCallApiClient { + fn stream(&mut self, request: ApiRequest) -> Result, RuntimeError> { + self.calls += 1; + match self.calls { + 1 => Ok(vec![ + AssistantEvent::ToolUse { + id: "tool-1".to_string(), + name: "fail".to_string(), + input: r#"{"path":"README.md"}"#.to_string(), + }, + AssistantEvent::MessageStop, + ]), + 2 => { + assert!(request + .messages + .iter() + .any(|message| message.role == MessageRole::Tool)); + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + AssistantEvent::MessageStop, + ]) + } + _ => Err(RuntimeError::new("unexpected extra API call")), + } + } + } + + // given + let mut runtime = ConversationRuntime::new_with_features( + Session::new(), + TwoCallApiClient { calls: 0 }, + StaticToolExecutor::new() + .register("fail", |_input| Err(ToolError::new("tool exploded"))), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + &RuntimeFeatureConfig::default().with_hooks(RuntimeHookConfig::new( + Vec::new(), + vec![shell_snippet("printf 'post hook should not run'")], + vec![shell_snippet("printf 'failure hook ran'")], + )), + ); + + // when + let summary = runtime + .run_turn("use fail", None) + .expect("tool loop succeeds"); + + // then + assert_eq!(summary.tool_results.len(), 1); + let ContentBlock::ToolResult { + is_error, output, .. + } = &summary.tool_results[0].blocks[0] + else { + panic!("expected tool result block"); + }; + assert!( + *is_error, + "failure hook path should preserve error result: {output:?}" + ); + assert!( + output.contains("tool exploded"), + "tool output missing failure reason: {output:?}" + ); + assert!( + output.contains("failure hook ran"), + "tool output missing failure hook feedback: {output:?}" + ); + assert!( + !output.contains("post hook should not run"), + "normal post hook should not run on tool failure: {output:?}" + ); + } + #[test] fn reconstructs_usage_tracker_from_restored_session() { struct SimpleApi; diff --git a/rust/crates/runtime/src/hooks.rs b/rust/crates/runtime/src/hooks.rs index f0a32fe..c6970c0 100644 --- a/rust/crates/runtime/src/hooks.rs +++ b/rust/crates/runtime/src/hooks.rs @@ -806,19 +806,50 @@ mod tests { #[test] fn runs_post_tool_use_failure_hooks() { + // given let runner = HookRunner::new(RuntimeHookConfig::new( Vec::new(), Vec::new(), vec![shell_snippet("printf 'failure hook ran'")], )); + // when let result = runner.run_post_tool_use_failure("bash", r#"{"command":"false"}"#, "command failed"); + // then assert!(!result.is_denied()); assert_eq!(result.messages(), &["failure hook ran".to_string()]); } + #[test] + fn stops_running_failure_hooks_after_failure() { + // given + let runner = HookRunner::new(RuntimeHookConfig::new( + Vec::new(), + Vec::new(), + vec![ + shell_snippet("printf 'broken failure hook'; exit 1"), + shell_snippet("printf 'later failure hook'"), + ], + )); + + // when + let result = + runner.run_post_tool_use_failure("bash", r#"{"command":"false"}"#, "command failed"); + + // then + assert!(result.is_failed()); + assert!(result + .messages() + .iter() + .any(|message| message.contains("broken failure hook"))); + assert!(!result + .messages() + .iter() + .any(|message| message == "later failure hook")); + } + #[test] fn executes_hooks_in_configured_order() { // given