mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-05 15:44:49 +08:00
feat(hooks): add PostToolUseFailure propagation, validation, and tests
- Hook runner propagates execution failures as real errors, not soft warnings - Conversation converts failed pre/post hooks into error tool results - Plugins fully support PostToolUseFailure: aggregation, resolution, validation, execution - Add ordering + short-circuit tests for normal and failure hook chains - Add missing PostToolUseFailure manifest path rejection test - Verified: cargo clippy --all-targets -- -D warnings passes, cargo test 94 passed
This commit is contained in:
@@ -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");
|
||||
|
||||
@@ -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<Vec<AssistantEvent>, 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;
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user