mirror of
https://github.com/instructkr/claw-code.git
synced 2026-06-05 22:17:10 +08:00
fix: validate hook config entries partially
Hook config now supports the Claude Code structured hook format with partial validation. Invalid hook entries are recorded in invalid_hooks while valid siblings are retained, following the same pattern as MCP partial validation (#440). Key changes: - RuntimeInvalidHookConfig now includes typed kind field (invalid_hooks_config or unknown_hook_event) for machine-readable error classification - Hook parsing collects all invalid entries instead of halting at first error - Unknown hook event names recorded as invalid without rejecting valid hooks - Legacy bare-string hooks still load with deprecation warnings - Claude Code documented format loads without error (matcher + nested hooks) - config/status/doctor JSON surfaces hook_validation metadata - classify_error_kind maps hook errors to invalid_hooks_config Generated with https://github.com/Yeachan-Heo/gajae-code Co-authored-by: Gajae Code <dev@gajae-code.com>
This commit is contained in:
@@ -182,6 +182,7 @@ pub struct RuntimeHookConfig {
|
||||
pre_tool_use: Vec<RuntimeHookCommand>,
|
||||
post_tool_use: Vec<RuntimeHookCommand>,
|
||||
post_tool_use_failure: Vec<RuntimeHookCommand>,
|
||||
invalid_hooks: Vec<RuntimeInvalidHookConfig>,
|
||||
}
|
||||
|
||||
/// A hook command plus optional tool matcher from object-style hook config.
|
||||
@@ -191,6 +192,16 @@ pub struct RuntimeHookCommand {
|
||||
matcher: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct RuntimeInvalidHookConfig {
|
||||
pub event: String,
|
||||
pub index: Option<usize>,
|
||||
pub hook_index: Option<usize>,
|
||||
pub kind: String,
|
||||
pub error_field: String,
|
||||
pub reason: String,
|
||||
}
|
||||
|
||||
/// Raw permission rule lists grouped by allow, deny, and ask behavior.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Default)]
|
||||
pub struct RuntimePermissionRuleConfig {
|
||||
@@ -1198,6 +1209,7 @@ impl RuntimeHookConfig {
|
||||
pre_tool_use,
|
||||
post_tool_use,
|
||||
post_tool_use_failure,
|
||||
invalid_hooks: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1235,6 +1247,8 @@ impl RuntimeHookConfig {
|
||||
&mut self.post_tool_use_failure,
|
||||
other.post_tool_use_failure_entries(),
|
||||
);
|
||||
self.invalid_hooks
|
||||
.extend(other.invalid_hooks.iter().cloned());
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
@@ -1246,6 +1260,25 @@ impl RuntimeHookConfig {
|
||||
pub fn post_tool_use_failure_entries(&self) -> &[RuntimeHookCommand] {
|
||||
&self.post_tool_use_failure
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn invalid_hooks(&self) -> &[RuntimeInvalidHookConfig] {
|
||||
&self.invalid_hooks
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn invalid_count(&self) -> usize {
|
||||
self.invalid_hooks.len()
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn has_invalid_hooks(&self) -> bool {
|
||||
!self.invalid_hooks.is_empty()
|
||||
}
|
||||
|
||||
pub fn push_invalid_hook(&mut self, invalid: RuntimeInvalidHookConfig) {
|
||||
self.invalid_hooks.push(invalid);
|
||||
}
|
||||
}
|
||||
|
||||
fn hook_commands(commands: &[RuntimeHookCommand]) -> Vec<String> {
|
||||
@@ -1634,14 +1667,217 @@ fn parse_optional_hooks_config_object(
|
||||
return Ok(RuntimeHookConfig::default());
|
||||
};
|
||||
let hooks = expect_object(hooks_value, context)?;
|
||||
Ok(RuntimeHookConfig {
|
||||
pre_tool_use: optional_hook_command_array(hooks, "PreToolUse", context)?
|
||||
.unwrap_or_default(),
|
||||
post_tool_use: optional_hook_command_array(hooks, "PostToolUse", context)?
|
||||
.unwrap_or_default(),
|
||||
post_tool_use_failure: optional_hook_command_array(hooks, "PostToolUseFailure", context)?
|
||||
.unwrap_or_default(),
|
||||
})
|
||||
Ok(parse_hooks_object_partial(hooks, context))
|
||||
}
|
||||
|
||||
fn parse_hooks_object_partial(
|
||||
hooks: &BTreeMap<String, JsonValue>,
|
||||
context: &str,
|
||||
) -> RuntimeHookConfig {
|
||||
let mut config = RuntimeHookConfig::default();
|
||||
parse_hook_event_partial(
|
||||
&mut config,
|
||||
hooks,
|
||||
"PreToolUse",
|
||||
context,
|
||||
|config, command| {
|
||||
config.pre_tool_use.push(command);
|
||||
},
|
||||
);
|
||||
parse_hook_event_partial(
|
||||
&mut config,
|
||||
hooks,
|
||||
"PostToolUse",
|
||||
context,
|
||||
|config, command| {
|
||||
config.post_tool_use.push(command);
|
||||
},
|
||||
);
|
||||
parse_hook_event_partial(
|
||||
&mut config,
|
||||
hooks,
|
||||
"PostToolUseFailure",
|
||||
context,
|
||||
|config, command| {
|
||||
config.post_tool_use_failure.push(command);
|
||||
},
|
||||
);
|
||||
for event in hooks.keys().filter(|event| !is_supported_hook_event(event)) {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.clone(),
|
||||
index: None,
|
||||
hook_index: None,
|
||||
kind: "unknown_hook_event".to_string(),
|
||||
error_field: event.clone(),
|
||||
reason: format!("{context}: unknown hook event {event}"),
|
||||
});
|
||||
}
|
||||
config
|
||||
}
|
||||
|
||||
fn is_supported_hook_event(event: &str) -> bool {
|
||||
matches!(event, "PreToolUse" | "PostToolUse" | "PostToolUseFailure")
|
||||
}
|
||||
|
||||
fn parse_hook_event_partial(
|
||||
config: &mut RuntimeHookConfig,
|
||||
hooks: &BTreeMap<String, JsonValue>,
|
||||
event: &str,
|
||||
context: &str,
|
||||
mut push_command: impl FnMut(&mut RuntimeHookConfig, RuntimeHookCommand),
|
||||
) {
|
||||
let Some(value) = hooks.get(event) else {
|
||||
return;
|
||||
};
|
||||
let Some(array) = value.as_array() else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: None,
|
||||
hook_index: None,
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: event.to_string(),
|
||||
reason: format!("{context}: field {event} must be an array"),
|
||||
});
|
||||
return;
|
||||
};
|
||||
|
||||
for (index, item) in array.iter().enumerate() {
|
||||
if let Some(command) = item.as_str() {
|
||||
if command.trim().is_empty() {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: None,
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "command".to_string(),
|
||||
reason: format!("{context}: field {event}[{index}] must be a non-empty string"),
|
||||
});
|
||||
} else {
|
||||
push_command(config, RuntimeHookCommand::new(command.to_string()));
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
let Some(entry) = item.as_object() else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: None,
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: event.to_string(),
|
||||
reason: format!(
|
||||
"{context}: field {event}[{index}] must be a string or hook object"
|
||||
),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
|
||||
let matcher = match optional_hook_matcher(entry, context, event, index) {
|
||||
Ok(matcher) => matcher,
|
||||
Err(error) => {
|
||||
config.push_invalid_hook(runtime_invalid_hook(
|
||||
event,
|
||||
Some(index),
|
||||
None,
|
||||
"matcher",
|
||||
error,
|
||||
));
|
||||
continue;
|
||||
}
|
||||
};
|
||||
let Some(hook_array) = entry.get("hooks").and_then(JsonValue::as_array) else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: None,
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "hooks".to_string(),
|
||||
reason: format!("{context}: field {event}[{index}].hooks must be an array"),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
for (hook_index, hook) in hook_array.iter().enumerate() {
|
||||
let Some(hook_object) = hook.as_object() else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: Some(hook_index),
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "hooks".to_string(),
|
||||
reason: format!(
|
||||
"{context}: field {event}[{index}].hooks[{hook_index}] must be an object"
|
||||
),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
if let Some(hook_type) = hook_object.get("type") {
|
||||
let Some(hook_type) = hook_type.as_str() else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: Some(hook_index),
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "type".to_string(),
|
||||
reason: format!(
|
||||
"{context}: field {event}[{index}].hooks[{hook_index}].type must be a string"
|
||||
),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
if hook_type != "command" {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: Some(hook_index),
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "type".to_string(),
|
||||
reason: format!(
|
||||
"{context}: field {event}[{index}].hooks[{hook_index}].type must be \"command\""
|
||||
),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
}
|
||||
let Some(command) = hook_object
|
||||
.get("command")
|
||||
.and_then(JsonValue::as_str)
|
||||
.filter(|command| !command.trim().is_empty())
|
||||
else {
|
||||
config.push_invalid_hook(RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index: Some(index),
|
||||
hook_index: Some(hook_index),
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: "command".to_string(),
|
||||
reason: format!(
|
||||
"{context}: field {event}[{index}].hooks[{hook_index}].command must be a non-empty string"
|
||||
),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
push_command(
|
||||
config,
|
||||
RuntimeHookCommand::with_matcher(command.to_string(), matcher.clone()),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn runtime_invalid_hook(
|
||||
event: &str,
|
||||
index: Option<usize>,
|
||||
hook_index: Option<usize>,
|
||||
error_field: &str,
|
||||
error: ConfigError,
|
||||
) -> RuntimeInvalidHookConfig {
|
||||
RuntimeInvalidHookConfig {
|
||||
event: event.to_string(),
|
||||
index,
|
||||
hook_index,
|
||||
kind: "invalid_hooks_config".to_string(),
|
||||
error_field: error_field.to_string(),
|
||||
reason: config_error_detail(&error),
|
||||
}
|
||||
}
|
||||
|
||||
fn validate_optional_hooks_config(
|
||||
@@ -2108,77 +2344,6 @@ fn optional_string_array(
|
||||
}
|
||||
}
|
||||
|
||||
fn optional_hook_command_array(
|
||||
object: &BTreeMap<String, JsonValue>,
|
||||
key: &str,
|
||||
context: &str,
|
||||
) -> Result<Option<Vec<RuntimeHookCommand>>, ConfigError> {
|
||||
let Some(value) = object.get(key) else {
|
||||
return Ok(None);
|
||||
};
|
||||
let Some(array) = value.as_array() else {
|
||||
return Err(ConfigError::Parse(format!(
|
||||
"{context}: field {key} must be an array"
|
||||
)));
|
||||
};
|
||||
|
||||
let mut commands = Vec::new();
|
||||
for (index, item) in array.iter().enumerate() {
|
||||
if let Some(command) = item.as_str() {
|
||||
commands.push(RuntimeHookCommand::new(command.to_string()));
|
||||
continue;
|
||||
}
|
||||
|
||||
let Some(entry) = item.as_object() else {
|
||||
return Err(ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}] must be a string or hook object"
|
||||
)));
|
||||
};
|
||||
let matcher = optional_hook_matcher(entry, context, key, index)?;
|
||||
let hooks = entry
|
||||
.get("hooks")
|
||||
.and_then(JsonValue::as_array)
|
||||
.ok_or_else(|| {
|
||||
ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}].hooks must be an array"
|
||||
))
|
||||
})?;
|
||||
for (hook_index, hook) in hooks.iter().enumerate() {
|
||||
let Some(hook_object) = hook.as_object() else {
|
||||
return Err(ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}].hooks[{hook_index}] must be an object"
|
||||
)));
|
||||
};
|
||||
if let Some(hook_type) = hook_object.get("type") {
|
||||
let Some(hook_type) = hook_type.as_str() else {
|
||||
return Err(ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}].hooks[{hook_index}].type must be a string"
|
||||
)));
|
||||
};
|
||||
if hook_type != "command" {
|
||||
return Err(ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}].hooks[{hook_index}].type must be \"command\""
|
||||
)));
|
||||
}
|
||||
}
|
||||
let command = hook_object
|
||||
.get("command")
|
||||
.and_then(JsonValue::as_str)
|
||||
.filter(|command| !command.trim().is_empty())
|
||||
.ok_or_else(|| {
|
||||
ConfigError::Parse(format!(
|
||||
"{context}: field {key}[{index}].hooks[{hook_index}].command must be a non-empty string"
|
||||
))
|
||||
})?;
|
||||
commands.push(RuntimeHookCommand::with_matcher(
|
||||
command.to_string(),
|
||||
matcher.clone(),
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(Some(commands))
|
||||
}
|
||||
|
||||
fn optional_hook_matcher(
|
||||
entry: &BTreeMap<String, JsonValue>,
|
||||
context: &str,
|
||||
@@ -2428,7 +2593,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_object_style_hook_entries_without_command() {
|
||||
fn records_object_style_hook_entries_without_command_441() {
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
let home = root.join("home").join(".claw");
|
||||
@@ -2440,12 +2605,20 @@ mod tests {
|
||||
)
|
||||
.expect("write settings");
|
||||
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should reject malformed hook entry");
|
||||
.expect("config should load valid siblings and record malformed hook entry");
|
||||
|
||||
assert!(error
|
||||
.to_string()
|
||||
assert!(loaded.hooks().pre_tool_use().is_empty());
|
||||
assert_eq!(loaded.hooks().invalid_count(), 1);
|
||||
assert_eq!(
|
||||
loaded.hooks().invalid_hooks()[0].kind,
|
||||
"invalid_hooks_config"
|
||||
);
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].event, "PreToolUse");
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].error_field, "command");
|
||||
assert!(loaded.hooks().invalid_hooks()[0]
|
||||
.reason
|
||||
.contains("command must be a non-empty string"));
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
@@ -3188,7 +3361,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_invalid_hook_entries_before_merge() {
|
||||
fn loads_valid_hook_entries_and_records_invalid_siblings_441() {
|
||||
// given
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
@@ -3208,19 +3381,21 @@ mod tests {
|
||||
)
|
||||
.expect("write invalid project settings");
|
||||
|
||||
// when
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should fail");
|
||||
.expect("config should load valid hook entries and record invalid siblings");
|
||||
|
||||
// then — config validation now catches the mixed array before the hooks parser
|
||||
let rendered = error.to_string();
|
||||
assert!(
|
||||
rendered.contains("hooks.PreToolUse")
|
||||
&& rendered.contains("must be an array of strings"),
|
||||
"expected validation error for hooks.PreToolUse, got: {rendered}"
|
||||
assert_eq!(loaded.hooks().pre_tool_use(), &["project".to_string()]);
|
||||
assert_eq!(loaded.hooks().invalid_count(), 1);
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].event, "PreToolUse");
|
||||
assert_eq!(
|
||||
loaded.hooks().invalid_hooks()[0].kind,
|
||||
"invalid_hooks_config"
|
||||
);
|
||||
assert!(!rendered.contains("merged settings.hooks"));
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].index, Some(1));
|
||||
assert!(loaded.hooks().invalid_hooks()[0]
|
||||
.reason
|
||||
.contains("must be a string or hook object"));
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
@@ -3363,7 +3538,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validates_wrong_type_for_known_field_with_field_path() {
|
||||
fn hook_event_wrong_type_is_recorded_without_config_failure_441() {
|
||||
// given
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
@@ -3377,29 +3552,145 @@ mod tests {
|
||||
)
|
||||
.expect("write user settings");
|
||||
|
||||
// when
|
||||
let error = ConfigLoader::new(&cwd, &home)
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect_err("config should fail");
|
||||
.expect("config should record malformed hook event without failing");
|
||||
|
||||
// then
|
||||
let rendered = error.to_string();
|
||||
assert!(
|
||||
rendered.contains(&user_settings.display().to_string()),
|
||||
"error should include file path, got: {rendered}"
|
||||
assert!(loaded.hooks().pre_tool_use().is_empty());
|
||||
assert_eq!(loaded.hooks().invalid_count(), 1);
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].event, "PreToolUse");
|
||||
assert_eq!(
|
||||
loaded.hooks().invalid_hooks()[0].kind,
|
||||
"invalid_hooks_config"
|
||||
);
|
||||
assert_eq!(loaded.hooks().invalid_hooks()[0].index, None);
|
||||
assert!(loaded.hooks().invalid_hooks()[0]
|
||||
.reason
|
||||
.contains("field PreToolUse must be an array"));
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collects_all_invalid_hook_siblings_instead_of_halting_at_first_441() {
|
||||
// ROADMAP #441 finding (c): first-error-only halting means users must fix
|
||||
// one hook at a time. After #441 partial fix, all invalid entries in the
|
||||
// same config are collected.
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
let home = root.join("home").join(".claw");
|
||||
fs::create_dir_all(&home).expect("home config dir");
|
||||
fs::create_dir_all(&cwd).expect("project dir");
|
||||
fs::write(
|
||||
home.join("settings.json"),
|
||||
r#"{"hooks":{"PreToolUse":[42],"PostToolUse":"not-an-array","InvalidEvent":["cmd"]}}"#,
|
||||
)
|
||||
.expect("write settings");
|
||||
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect("config should collect all invalid hooks without halting at first");
|
||||
|
||||
assert!(loaded.hooks().pre_tool_use().is_empty());
|
||||
assert!(loaded.hooks().post_tool_use().is_empty());
|
||||
// Three distinct invalid entries: 42, wrong type, unknown event
|
||||
assert_eq!(loaded.hooks().invalid_count(), 3);
|
||||
|
||||
let invalid = loaded.hooks().invalid_hooks();
|
||||
// PreToolUse[0]=42
|
||||
assert_eq!(invalid[0].event, "PreToolUse");
|
||||
assert_eq!(invalid[0].index, Some(0));
|
||||
assert_eq!(invalid[0].kind, "invalid_hooks_config");
|
||||
// PostToolUse wrong type
|
||||
assert_eq!(invalid[1].event, "PostToolUse");
|
||||
assert_eq!(invalid[1].index, None);
|
||||
assert_eq!(invalid[1].kind, "invalid_hooks_config");
|
||||
// Unknown event
|
||||
assert_eq!(invalid[2].event, "InvalidEvent");
|
||||
assert_eq!(invalid[2].index, None);
|
||||
assert_eq!(invalid[2].kind, "unknown_hook_event");
|
||||
assert!(invalid[2]
|
||||
.reason
|
||||
.contains("unknown hook event InvalidEvent"));
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unknown_hook_events_recorded_with_correct_kind_441() {
|
||||
// ROADMAP #441 finding (a): unknown event names like Stop/Notification
|
||||
// should not reject entire hooks config; they are recorded as invalid.
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
let home = root.join("home").join(".claw");
|
||||
fs::create_dir_all(&home).expect("home config dir");
|
||||
fs::create_dir_all(&cwd).expect("project dir");
|
||||
fs::write(
|
||||
home.join("settings.json"),
|
||||
r#"{"hooks":{"PreToolUse":["valid-cmd"],"Stop":"not-an-array","Notification":[{}]}}"#,
|
||||
)
|
||||
.expect("write settings");
|
||||
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect("config should load valid hooks and record unknown event siblings");
|
||||
|
||||
// Valid PreToolUse hook should load
|
||||
assert_eq!(loaded.hooks().pre_tool_use(), &["valid-cmd".to_string()]);
|
||||
// Stop and Notification are unknown events; each gets one invalid entry
|
||||
// Notification:[{}] also has an empty-object entry issue but since we
|
||||
// don't parse unknown events, only the unknown-event invalid is recorded
|
||||
let invalid = loaded.hooks().invalid_hooks();
|
||||
assert!(
|
||||
rendered.contains("hooks"),
|
||||
"error should include field path component 'hooks', got: {rendered}"
|
||||
invalid.len() >= 2,
|
||||
"expected at least 2 invalid hooks, got {}",
|
||||
invalid.len()
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("PreToolUse"),
|
||||
"error should describe the type mismatch, got: {rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("array"),
|
||||
"error should describe the expected type, got: {rendered}"
|
||||
|
||||
let stop = invalid
|
||||
.iter()
|
||||
.find(|h| h.event == "Stop")
|
||||
.expect("Stop invalid hook");
|
||||
assert_eq!(stop.kind, "unknown_hook_event");
|
||||
assert_eq!(stop.index, None);
|
||||
assert!(stop.reason.contains("unknown hook event Stop"));
|
||||
|
||||
let notif = invalid
|
||||
.iter()
|
||||
.find(|h| h.event == "Notification")
|
||||
.expect("Notification invalid hook");
|
||||
assert_eq!(notif.kind, "unknown_hook_event");
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn documented_claude_code_hook_format_loads_without_error_441() {
|
||||
// ROADMAP #441: the Claude Code documented hook format
|
||||
// {"hooks":{"PreToolUse":[{"matcher":"Read","hooks":[{"type":"command","command":"..."}]}]}}
|
||||
// must load without config_load_error.
|
||||
let root = temp_dir();
|
||||
let cwd = root.join("project");
|
||||
let home = root.join("home").join(".claw");
|
||||
fs::create_dir_all(&home).expect("home config dir");
|
||||
fs::create_dir_all(&cwd).expect("project dir");
|
||||
fs::write(
|
||||
home.join("settings.json"),
|
||||
r#"{"hooks":{"PreToolUse":[{"matcher":"Read","hooks":[{"type":"command","command":"/bin/echo pretool"}]}]}}"#,
|
||||
)
|
||||
.expect("write settings");
|
||||
|
||||
let loaded = ConfigLoader::new(&cwd, &home)
|
||||
.load()
|
||||
.expect("Claude Code documented hook format must load without error");
|
||||
|
||||
assert_eq!(
|
||||
loaded.hooks().pre_tool_use(),
|
||||
&["/bin/echo pretool".to_string()]
|
||||
);
|
||||
assert_eq!(loaded.hooks().invalid_count(), 0);
|
||||
let entries = loaded.hooks().pre_tool_use_entries();
|
||||
assert_eq!(entries[0].matcher(), Some("Read"));
|
||||
|
||||
fs::remove_dir_all(root).expect("cleanup temp dir");
|
||||
}
|
||||
|
||||
@@ -118,10 +118,7 @@ impl FieldType {
|
||||
Self::StringArray => value
|
||||
.as_array()
|
||||
.is_some_and(|arr| arr.iter().all(|v| v.as_str().is_some())),
|
||||
Self::HookArray => value.as_array().is_some_and(|arr| {
|
||||
arr.iter()
|
||||
.all(|entry| entry.as_str().is_some() || entry.as_object().is_some())
|
||||
}),
|
||||
Self::HookArray => true,
|
||||
Self::RulesImport => {
|
||||
value.as_str().is_some()
|
||||
|| value
|
||||
@@ -439,6 +436,43 @@ fn validate_object_keys(
|
||||
result
|
||||
}
|
||||
|
||||
/// Emit deprecation warnings for bare string hook entries in the hooks object.
|
||||
/// Legacy `["command-string"]` arrays still load but suggest migration to the
|
||||
/// structured `{matcher, hooks:[{type, command}]}` form.
|
||||
fn validate_hook_entry_format(
|
||||
hooks: &BTreeMap<String, JsonValue>,
|
||||
source: &str,
|
||||
path_display: &str,
|
||||
) -> ValidationResult {
|
||||
let mut result = ValidationResult {
|
||||
errors: Vec::new(),
|
||||
warnings: Vec::new(),
|
||||
};
|
||||
for spec in HOOKS_FIELDS {
|
||||
let Some(value) = hooks.get(spec.name) else {
|
||||
continue;
|
||||
};
|
||||
let Some(array) = value.as_array() else {
|
||||
continue;
|
||||
};
|
||||
for item in array {
|
||||
if item.as_str().is_some() {
|
||||
result.warnings.push(ConfigDiagnostic {
|
||||
path: path_display.to_string(),
|
||||
field: format!("hooks.{}", spec.name),
|
||||
line: find_key_line(source, spec.name),
|
||||
kind: DiagnosticKind::Deprecated {
|
||||
replacement: "object-style hook entries with hooks:[{type:\"command\",command:\"...\"}]",
|
||||
},
|
||||
});
|
||||
// One deprecation warning per event is enough
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
fn suggest_field(input: &str, candidates: &[&str]) -> Option<String> {
|
||||
let input_lower = input.to_ascii_lowercase();
|
||||
candidates
|
||||
@@ -510,6 +544,7 @@ pub fn validate_config_file(
|
||||
source,
|
||||
&path_display,
|
||||
));
|
||||
result.merge(validate_hook_entry_format(hooks, source, &path_display));
|
||||
}
|
||||
if let Some(permissions) = object.get("permissions").and_then(JsonValue::as_object) {
|
||||
result.merge(validate_object_keys(
|
||||
@@ -714,7 +749,7 @@ mod tests {
|
||||
#[test]
|
||||
fn validates_nested_hooks_keys() {
|
||||
// given
|
||||
let source = r#"{"hooks": {"PreToolUse": ["cmd"], "BadHook": ["x"]}}"#;
|
||||
let source = r#"{"hooks": {"PreToolUse": [{"hooks":[{"type":"command","command":"cmd"}]}], "BadHook": ["x"]}}"#;
|
||||
let parsed = JsonValue::parse(source).expect("valid json");
|
||||
let object = parsed.as_object().expect("object");
|
||||
|
||||
@@ -723,7 +758,12 @@ mod tests {
|
||||
|
||||
// then
|
||||
assert!(result.errors.is_empty());
|
||||
assert_eq!(result.warnings.len(), 1);
|
||||
assert_eq!(
|
||||
result.warnings.len(),
|
||||
1,
|
||||
"expected only the unknown key warning, got {:?}",
|
||||
result.warnings
|
||||
);
|
||||
assert_eq!(result.warnings[0].field, "hooks.BadHook");
|
||||
}
|
||||
|
||||
@@ -739,15 +779,14 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_wrong_hook_entry_types() {
|
||||
fn allows_wrong_hook_entry_types_for_partial_runtime_validation_441() {
|
||||
let source = r#"{"hooks":{"PreToolUse":[42]}}"#;
|
||||
let parsed = JsonValue::parse(source).expect("valid json");
|
||||
let object = parsed.as_object().expect("object");
|
||||
|
||||
let result = validate_config_file(object, source, &test_path());
|
||||
|
||||
assert_eq!(result.errors.len(), 1);
|
||||
assert_eq!(result.errors[0].field, "hooks.PreToolUse");
|
||||
assert!(result.errors.is_empty(), "{:?}", result.errors);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -847,7 +886,7 @@ mod tests {
|
||||
// given
|
||||
let source = r#"{
|
||||
"model": "opus",
|
||||
"hooks": {"PreToolUse": ["guard"]},
|
||||
"hooks": {"PreToolUse": [{"hooks":[{"type":"command","command":"guard"}]}]},
|
||||
"permissions": {"defaultMode": "plan", "allow": ["Read"]},
|
||||
"mcpServers": {},
|
||||
"sandbox": {"enabled": false}
|
||||
|
||||
@@ -71,8 +71,8 @@ pub use config::{
|
||||
McpSdkServerConfig, McpServerConfig, McpStdioServerConfig, McpTransport,
|
||||
McpWebSocketServerConfig, OAuthConfig, ProviderFallbackConfig, ResolvedPermissionMode,
|
||||
RulesImportConfig, RuntimeConfig, RuntimeFeatureConfig, RuntimeHookCommand, RuntimeHookConfig,
|
||||
RuntimePermissionRuleConfig, RuntimePluginConfig, ScopedMcpServerConfig,
|
||||
CLAW_SETTINGS_SCHEMA_NAME,
|
||||
RuntimeInvalidHookConfig, RuntimePermissionRuleConfig, RuntimePluginConfig,
|
||||
ScopedMcpServerConfig, CLAW_SETTINGS_SCHEMA_NAME,
|
||||
};
|
||||
pub use config_validate::{
|
||||
check_unsupported_format, format_diagnostics, validate_config_file, ConfigDiagnostic,
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
#![recursion_limit = "256"]
|
||||
#![allow(
|
||||
dead_code,
|
||||
unused_imports,
|
||||
@@ -59,7 +60,7 @@ use runtime::{
|
||||
ConversationMessage, ConversationRuntime, McpConfigCollection, McpInvalidServerConfig,
|
||||
McpServer, McpServerManager, McpServerSpec, McpTool, MessageRole, ModelPricing, PermissionMode,
|
||||
PermissionPolicy, ProjectContext, PromptCacheEvent, ResolvedPermissionMode, RuntimeError,
|
||||
Session, TokenUsage, ToolError, ToolExecutor, UsageTracker,
|
||||
RuntimeInvalidHookConfig, Session, TokenUsage, ToolError, ToolExecutor, UsageTracker,
|
||||
};
|
||||
use serde::Deserialize;
|
||||
use serde_json::{json, Map, Value};
|
||||
@@ -3503,6 +3504,11 @@ fn render_doctor_report(
|
||||
.ok()
|
||||
.map(|runtime_config| McpValidationSummary::from_collection(runtime_config.mcp()))
|
||||
.unwrap_or_default();
|
||||
let hook_validation = config
|
||||
.as_ref()
|
||||
.ok()
|
||||
.map(HookValidationSummary::from_config)
|
||||
.unwrap_or_default();
|
||||
let context = StatusContext {
|
||||
cwd: cwd.clone(),
|
||||
session_path: None,
|
||||
@@ -3532,12 +3538,14 @@ fn render_doctor_report(
|
||||
config_load_error: config.as_ref().err().map(ToString::to_string),
|
||||
config_load_error_kind: None,
|
||||
mcp_validation: mcp_validation.clone(),
|
||||
hook_validation: hook_validation.clone(),
|
||||
};
|
||||
Ok(DoctorReport {
|
||||
checks: vec![
|
||||
check_auth_health(),
|
||||
check_config_health(&config_loader, config.as_ref()),
|
||||
check_mcp_validation_health(&mcp_validation),
|
||||
check_hook_validation_health(&hook_validation),
|
||||
check_install_source_health(),
|
||||
check_workspace_health(&context),
|
||||
check_memory_health(&context),
|
||||
@@ -3838,6 +3846,10 @@ fn check_config_health(
|
||||
"mcp_invalid_servers".to_string(),
|
||||
json!(runtime_config.mcp().invalid_count()),
|
||||
),
|
||||
(
|
||||
"hook_invalid_entries".to_string(),
|
||||
json!(runtime_config.hooks().invalid_count()),
|
||||
),
|
||||
]))
|
||||
}
|
||||
Err(error) => DiagnosticCheck::new(
|
||||
@@ -3918,6 +3930,51 @@ fn check_mcp_validation_health(summary: &McpValidationSummary) -> DiagnosticChec
|
||||
]))
|
||||
}
|
||||
|
||||
fn check_hook_validation_health(summary: &HookValidationSummary) -> DiagnosticCheck {
|
||||
let mut details = vec![
|
||||
format!("Valid entries {}", summary.valid_count),
|
||||
format!("Invalid entries {}", summary.invalid_count()),
|
||||
];
|
||||
details.extend(
|
||||
summary
|
||||
.invalid_hooks
|
||||
.iter()
|
||||
.map(|hook| format!("Invalid hook {} ({})", hook.event, hook.reason)),
|
||||
);
|
||||
|
||||
DiagnosticCheck::new(
|
||||
"Hook validation",
|
||||
if summary.has_invalid_hooks() {
|
||||
DiagnosticLevel::Warn
|
||||
} else {
|
||||
DiagnosticLevel::Ok
|
||||
},
|
||||
if summary.has_invalid_hooks() {
|
||||
format!(
|
||||
"{} hook entries are invalid; {} valid entries remain loaded",
|
||||
summary.invalid_count(),
|
||||
summary.valid_count
|
||||
)
|
||||
} else {
|
||||
format!("{} hook entries validated", summary.valid_count)
|
||||
},
|
||||
)
|
||||
.with_hint(if summary.has_invalid_hooks() {
|
||||
"Inspect `claw status --output-format json` hook_validation.invalid_hooks and fix each rejected hooks entry."
|
||||
} else {
|
||||
""
|
||||
})
|
||||
.with_details(details)
|
||||
.with_data(Map::from_iter([
|
||||
("valid_count".to_string(), json!(summary.valid_count)),
|
||||
("invalid_count".to_string(), json!(summary.invalid_count())),
|
||||
(
|
||||
"invalid_hooks".to_string(),
|
||||
Value::Array(invalid_hooks_json(&summary.invalid_hooks)),
|
||||
),
|
||||
]))
|
||||
}
|
||||
|
||||
fn check_permission_health(permission_mode: PermissionModeProvenance) -> DiagnosticCheck {
|
||||
let mode = permission_mode.mode.as_str();
|
||||
let source = permission_mode.source.as_str();
|
||||
@@ -4897,6 +4954,57 @@ impl McpValidationSummary {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
struct HookValidationSummary {
|
||||
valid_count: usize,
|
||||
invalid_hooks: Vec<RuntimeInvalidHookConfig>,
|
||||
}
|
||||
|
||||
impl HookValidationSummary {
|
||||
fn from_config(config: &runtime::RuntimeConfig) -> Self {
|
||||
let hooks = config.hooks();
|
||||
Self {
|
||||
valid_count: hooks.pre_tool_use_entries().len()
|
||||
+ hooks.post_tool_use_entries().len()
|
||||
+ hooks.post_tool_use_failure_entries().len(),
|
||||
invalid_hooks: hooks.invalid_hooks().to_vec(),
|
||||
}
|
||||
}
|
||||
|
||||
fn invalid_count(&self) -> usize {
|
||||
self.invalid_hooks.len()
|
||||
}
|
||||
|
||||
fn has_invalid_hooks(&self) -> bool {
|
||||
!self.invalid_hooks.is_empty()
|
||||
}
|
||||
|
||||
fn json_value(&self) -> serde_json::Value {
|
||||
json!({
|
||||
"valid_count": self.valid_count,
|
||||
"invalid_count": self.invalid_count(),
|
||||
"invalid_hooks": invalid_hooks_json(&self.invalid_hooks),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn invalid_hooks_json(invalid_hooks: &[RuntimeInvalidHookConfig]) -> Vec<serde_json::Value> {
|
||||
invalid_hooks
|
||||
.iter()
|
||||
.map(|hook| {
|
||||
json!({
|
||||
"event": &hook.event,
|
||||
"index": hook.index,
|
||||
"hook_index": hook.hook_index,
|
||||
"kind": &hook.kind,
|
||||
"error_field": &hook.error_field,
|
||||
"reason": &hook.reason,
|
||||
"valid": false,
|
||||
})
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn invalid_mcp_servers_json(invalid_servers: &[McpInvalidServerConfig]) -> Vec<serde_json::Value> {
|
||||
invalid_servers
|
||||
.iter()
|
||||
@@ -5060,6 +5168,7 @@ struct StatusContext {
|
||||
/// instead of regex-scraping the prose.
|
||||
config_load_error_kind: Option<&'static str>,
|
||||
mcp_validation: McpValidationSummary,
|
||||
hook_validation: HookValidationSummary,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -8972,10 +9081,11 @@ fn status_json_value(
|
||||
json!({
|
||||
"kind": "status",
|
||||
"action": "show",
|
||||
"status": if degraded || context.mcp_validation.has_invalid_servers() { "degraded" } else { "ok" },
|
||||
"status": if degraded || context.mcp_validation.has_invalid_servers() || context.hook_validation.has_invalid_hooks() { "degraded" } else { "ok" },
|
||||
"config_load_error": context.config_load_error,
|
||||
"config_load_error_kind": context.config_load_error_kind,
|
||||
"mcp_validation": context.mcp_validation.json_value(),
|
||||
"hook_validation": context.hook_validation.json_value(),
|
||||
"model": model,
|
||||
"model_source": model_source,
|
||||
"model_raw": model_raw,
|
||||
@@ -9043,6 +9153,7 @@ fn status_json_value(
|
||||
"memory_files": memory_files_json(&context.memory_files),
|
||||
"unloaded_memory_files": context.unloaded_memory_files,
|
||||
"mcp_validation": context.mcp_validation.json_value(),
|
||||
"hook_validation": context.hook_validation.json_value(),
|
||||
},
|
||||
"sandbox": {
|
||||
"enabled": context.sandbox_status.enabled,
|
||||
@@ -9121,6 +9232,11 @@ fn status_context(
|
||||
.ok()
|
||||
.map(|runtime_config| McpValidationSummary::from_collection(runtime_config.mcp()))
|
||||
.unwrap_or_default();
|
||||
let hook_validation = runtime_config
|
||||
.as_ref()
|
||||
.ok()
|
||||
.map(HookValidationSummary::from_config)
|
||||
.unwrap_or_default();
|
||||
Ok(StatusContext {
|
||||
cwd: cwd.clone(),
|
||||
session_path: session_path.map(Path::to_path_buf),
|
||||
@@ -9145,6 +9261,7 @@ fn status_context(
|
||||
config_load_error,
|
||||
config_load_error_kind,
|
||||
mcp_validation,
|
||||
hook_validation,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -9727,7 +9844,7 @@ fn render_doctor_help_json() -> serde_json::Value {
|
||||
"requires_session_resume": false,
|
||||
"mutates_workspace": false,
|
||||
"output_fields": ["kind", "action", "status", "message", "report", "has_failures", "summary", "checks", "allowed_tools"],
|
||||
"check_names": ["auth", "config", "mcp validation", "install source", "workspace", "memory", "boot preflight", "sandbox", "permissions", "system"],
|
||||
"check_names": ["auth", "config", "mcp validation", "hook validation", "install source", "workspace", "memory", "boot preflight", "sandbox", "permissions", "system"],
|
||||
"status_values": ["ok", "warn", "fail"],
|
||||
"options": [
|
||||
{
|
||||
@@ -9981,10 +10098,19 @@ fn render_config_json(
|
||||
.map(|w| serde_json::Value::String(w.clone()))
|
||||
.collect();
|
||||
|
||||
let hook_validation = HookValidationSummary::from_config(&runtime_config);
|
||||
let has_hook_issues = hook_validation.has_invalid_hooks();
|
||||
let status_value = if inspection.load_error.is_some() {
|
||||
"error"
|
||||
} else if has_hook_issues {
|
||||
"degraded"
|
||||
} else {
|
||||
"ok"
|
||||
};
|
||||
let base = serde_json::json!({
|
||||
"kind": "config",
|
||||
"action": if section.is_some() { "show" } else { "list" },
|
||||
"status": if inspection.load_error.is_some() { "error" } else { "ok" },
|
||||
"status": status_value,
|
||||
"cwd": cwd.display().to_string(),
|
||||
"loaded_files": loaded_files,
|
||||
"merged_keys": merged_keys,
|
||||
@@ -9993,6 +10119,7 @@ fn render_config_json(
|
||||
"files": files,
|
||||
"warnings": warnings_json,
|
||||
"load_error": inspection.load_error.clone(),
|
||||
"hook_validation": hook_validation.json_value(),
|
||||
});
|
||||
|
||||
if let Some(section) = section {
|
||||
@@ -16736,6 +16863,7 @@ mod tests {
|
||||
config_load_error: None,
|
||||
config_load_error_kind: None,
|
||||
mcp_validation: super::McpValidationSummary::default(),
|
||||
hook_validation: super::HookValidationSummary::default(),
|
||||
},
|
||||
None, // #148
|
||||
None,
|
||||
@@ -16887,6 +17015,7 @@ mod tests {
|
||||
config_load_error: None,
|
||||
config_load_error_kind: None,
|
||||
mcp_validation: super::McpValidationSummary::default(),
|
||||
hook_validation: super::HookValidationSummary::default(),
|
||||
};
|
||||
|
||||
let check = super::check_workspace_health(&context);
|
||||
@@ -16937,6 +17066,7 @@ mod tests {
|
||||
config_load_error: None,
|
||||
config_load_error_kind: None,
|
||||
mcp_validation: super::McpValidationSummary::default(),
|
||||
hook_validation: super::HookValidationSummary::default(),
|
||||
};
|
||||
|
||||
let check = super::check_memory_health(&context);
|
||||
@@ -16979,6 +17109,7 @@ mod tests {
|
||||
config_load_error: None,
|
||||
config_load_error_kind: None,
|
||||
mcp_validation: super::McpValidationSummary::default(),
|
||||
hook_validation: super::HookValidationSummary::default(),
|
||||
};
|
||||
|
||||
let value = status_json_value(
|
||||
|
||||
@@ -112,6 +112,7 @@ fn assert_doctor_help_json_contract(parsed: &Value) {
|
||||
assert!(checks.iter().any(|check| check == "boot preflight"));
|
||||
assert!(checks.iter().any(|check| check == "memory"));
|
||||
assert!(checks.iter().any(|check| check == "mcp validation"));
|
||||
assert!(checks.iter().any(|check| check == "hook validation"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1459,7 +1460,7 @@ fn doctor_and_resume_status_emit_json_when_requested() {
|
||||
.is_some_and(|available| available.iter().any(|name| name == "web_fetch")));
|
||||
|
||||
let checks = doctor["checks"].as_array().expect("doctor checks");
|
||||
assert_eq!(checks.len(), 10);
|
||||
assert_eq!(checks.len(), 11);
|
||||
let check_names = checks
|
||||
.iter()
|
||||
.map(|check| {
|
||||
@@ -1481,6 +1482,7 @@ fn doctor_and_resume_status_emit_json_when_requested() {
|
||||
"auth",
|
||||
"config",
|
||||
"mcp validation",
|
||||
"hook validation",
|
||||
"install source",
|
||||
"workspace",
|
||||
"memory",
|
||||
|
||||
Reference in New Issue
Block a user