mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Add presentation hint for quiet managed hooks
This commit is contained in:
@@ -69,6 +69,7 @@ mod tests {
|
||||
timeout_sec: None,
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: Default::default(),
|
||||
},
|
||||
],
|
||||
}],
|
||||
|
||||
@@ -29,6 +29,7 @@ use crate::events::common::validate_matcher_pattern;
|
||||
use codex_protocol::protocol::HookHandlerType;
|
||||
use codex_protocol::protocol::HookSource;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use codex_protocol::protocol::HookVisibilityHint;
|
||||
|
||||
pub(crate) struct DiscoveryResult {
|
||||
pub handlers: Vec<ConfiguredHandler>,
|
||||
@@ -434,7 +435,19 @@ fn append_matcher_groups(
|
||||
timeout_sec,
|
||||
r#async,
|
||||
status_message,
|
||||
visibility_hint,
|
||||
} => {
|
||||
let visibility_hint = if source.is_managed {
|
||||
visibility_hint
|
||||
} else if visibility_hint == HookVisibilityHint::Hidden {
|
||||
warnings.push(format!(
|
||||
"ignoring visibilityHint in {}: hidden hooks are only supported for managed hook sources",
|
||||
source.path.display()
|
||||
));
|
||||
HookVisibilityHint::Default
|
||||
} else {
|
||||
HookVisibilityHint::Default
|
||||
};
|
||||
let command = if cfg!(windows) {
|
||||
command_windows.unwrap_or(command)
|
||||
} else {
|
||||
@@ -461,6 +474,7 @@ fn append_matcher_groups(
|
||||
timeout_sec: Some(timeout_sec),
|
||||
r#async,
|
||||
status_message: status_message.clone(),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
};
|
||||
let current_hash =
|
||||
command_hook_hash(event_name, matcher, &group, normalized_handler);
|
||||
@@ -509,6 +523,7 @@ fn append_matcher_groups(
|
||||
source: source.source,
|
||||
display_order: *display_order,
|
||||
env: source.env.clone(),
|
||||
visibility_hint,
|
||||
});
|
||||
}
|
||||
*display_order += 1;
|
||||
@@ -630,6 +645,7 @@ mod tests {
|
||||
use codex_config::MatcherGroup;
|
||||
use codex_config::TomlValue;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use codex_protocol::protocol::HookVisibilityHint;
|
||||
|
||||
fn source_path() -> AbsolutePathBuf {
|
||||
test_path_buf("/tmp/hooks.json").abs()
|
||||
@@ -681,6 +697,7 @@ mod tests {
|
||||
timeout_sec: None,
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}
|
||||
}
|
||||
@@ -716,6 +733,7 @@ mod tests {
|
||||
source: hook_source(),
|
||||
display_order: 0,
|
||||
env: std::collections::HashMap::new(),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}]
|
||||
);
|
||||
}
|
||||
@@ -751,6 +769,7 @@ mod tests {
|
||||
source: hook_source(),
|
||||
display_order: 0,
|
||||
env: std::collections::HashMap::new(),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}]
|
||||
);
|
||||
}
|
||||
@@ -868,6 +887,103 @@ mod tests {
|
||||
assert_eq!(handlers[0].matcher.as_deref(), Some("Edit|Write"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn visibility_hint_does_not_change_command_trust_hash() {
|
||||
let source_path = source_path();
|
||||
let hook_states = std::collections::HashMap::new();
|
||||
let mut default_entries = Vec::new();
|
||||
let mut hidden_entries = Vec::new();
|
||||
let mut default_handlers = Vec::new();
|
||||
let mut hidden_handlers = Vec::new();
|
||||
let mut default_warnings = Vec::new();
|
||||
let mut hidden_warnings = Vec::new();
|
||||
let mut default_display_order = 0;
|
||||
let mut hidden_display_order = 0;
|
||||
let default_group = command_group(Some("^Bash$"));
|
||||
let mut hidden_group = command_group(Some("^Bash$"));
|
||||
let HookHandlerConfig::Command {
|
||||
visibility_hint, ..
|
||||
} = &mut hidden_group.hooks[0]
|
||||
else {
|
||||
unreachable!("command_group should create a command hook");
|
||||
};
|
||||
*visibility_hint = HookVisibilityHint::Hidden;
|
||||
|
||||
append_matcher_groups(
|
||||
&mut default_handlers,
|
||||
&mut default_entries,
|
||||
&mut default_warnings,
|
||||
&mut default_display_order,
|
||||
&hook_handler_source(&source_path, &hook_states),
|
||||
HookEventName::PreToolUse,
|
||||
vec![default_group],
|
||||
);
|
||||
append_matcher_groups(
|
||||
&mut hidden_handlers,
|
||||
&mut hidden_entries,
|
||||
&mut hidden_warnings,
|
||||
&mut hidden_display_order,
|
||||
&hook_handler_source(&source_path, &hook_states),
|
||||
HookEventName::PreToolUse,
|
||||
vec![hidden_group],
|
||||
);
|
||||
|
||||
assert_eq!(default_warnings, Vec::<String>::new());
|
||||
assert_eq!(hidden_warnings, Vec::<String>::new());
|
||||
assert_eq!(
|
||||
default_entries[0].current_hash,
|
||||
hidden_entries[0].current_hash
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unmanaged_visibility_hint_is_ignored_with_warning() {
|
||||
let source_path = source_path();
|
||||
let hook_states = std::collections::HashMap::new();
|
||||
let mut handlers = Vec::new();
|
||||
let mut entries = Vec::new();
|
||||
let mut warnings = Vec::new();
|
||||
let mut display_order = 0;
|
||||
let mut hidden_group = command_group(Some("^Bash$"));
|
||||
let HookHandlerConfig::Command {
|
||||
visibility_hint, ..
|
||||
} = &mut hidden_group.hooks[0]
|
||||
else {
|
||||
unreachable!("command_group should create a command hook");
|
||||
};
|
||||
*visibility_hint = HookVisibilityHint::Hidden;
|
||||
let unmanaged_source = super::HookHandlerSource {
|
||||
path: &source_path,
|
||||
key_source: source_path.display().to_string(),
|
||||
source: HookSource::User,
|
||||
is_managed: false,
|
||||
bypass_hook_trust: false,
|
||||
hook_states: &hook_states,
|
||||
env: std::collections::HashMap::new(),
|
||||
plugin_id: None,
|
||||
};
|
||||
|
||||
append_matcher_groups(
|
||||
&mut handlers,
|
||||
&mut entries,
|
||||
&mut warnings,
|
||||
&mut display_order,
|
||||
&unmanaged_source,
|
||||
HookEventName::PreToolUse,
|
||||
vec![hidden_group],
|
||||
);
|
||||
|
||||
assert_eq!(handlers, Vec::<ConfiguredHandler>::new());
|
||||
assert_eq!(entries.len(), 1);
|
||||
assert_eq!(
|
||||
warnings,
|
||||
vec![format!(
|
||||
"ignoring visibilityHint in {}: hidden hooks are only supported for managed hook sources",
|
||||
source_path.display()
|
||||
)]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn toml_hook_discovery_ignores_malformed_state_entries() {
|
||||
let layer = ConfigLayerEntry::new(
|
||||
@@ -893,6 +1009,7 @@ mod tests {
|
||||
timeout_sec: None,
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -923,6 +1040,7 @@ mod tests {
|
||||
timeout_sec: None,
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
);
|
||||
|
||||
@@ -77,6 +77,7 @@ pub(crate) fn running_summary(handler: &ConfiguredHandler) -> HookRunSummary {
|
||||
display_order: handler.display_order,
|
||||
status: HookRunStatus::Running,
|
||||
status_message: handler.status_message.clone(),
|
||||
visibility_hint: handler.visibility_hint,
|
||||
started_at: chrono::Utc::now().timestamp(),
|
||||
completed_at: None,
|
||||
duration_ms: None,
|
||||
@@ -130,6 +131,7 @@ pub(crate) fn completed_summary(
|
||||
display_order: handler.display_order,
|
||||
status,
|
||||
status_message: handler.status_message.clone(),
|
||||
visibility_hint: handler.visibility_hint,
|
||||
started_at: run_result.started_at,
|
||||
completed_at: Some(run_result.completed_at),
|
||||
duration_ms: Some(run_result.duration_ms),
|
||||
@@ -177,6 +179,7 @@ mod tests {
|
||||
source: HookSource::User,
|
||||
display_order,
|
||||
env: std::collections::HashMap::new(),
|
||||
visibility_hint: Default::default(),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -31,6 +31,7 @@ use codex_protocol::protocol::HookHandlerType;
|
||||
use codex_protocol::protocol::HookRunSummary;
|
||||
use codex_protocol::protocol::HookSource;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use codex_protocol::protocol::HookVisibilityHint;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
@@ -46,6 +47,7 @@ pub(crate) struct ConfiguredHandler {
|
||||
pub command: String,
|
||||
pub timeout_sec: u64,
|
||||
pub status_message: Option<String>,
|
||||
pub visibility_hint: HookVisibilityHint,
|
||||
pub source_path: AbsolutePathBuf,
|
||||
pub source: HookSource,
|
||||
pub display_order: i64,
|
||||
|
||||
@@ -25,6 +25,7 @@ use codex_protocol::protocol::HookOutputEntryKind;
|
||||
use codex_protocol::protocol::HookRunStatus;
|
||||
use codex_protocol::protocol::HookSource;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use codex_protocol::protocol::HookVisibilityHint;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -66,6 +67,7 @@ fn pre_tool_use_hook_events(command: impl Into<String>) -> HookEventsToml {
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -173,6 +175,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Hidden,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -235,6 +238,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
});
|
||||
assert_eq!(preview.len(), 1);
|
||||
assert_eq!(preview[0].source_path, managed_dir);
|
||||
assert_eq!(preview[0].visibility_hint, HookVisibilityHint::Hidden);
|
||||
|
||||
let outcome = engine
|
||||
.run_pre_tool_use(PreToolUseRequest {
|
||||
@@ -252,6 +256,10 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
.await;
|
||||
|
||||
assert!(!outcome.should_block);
|
||||
assert_eq!(
|
||||
outcome.hook_events[0].run.visibility_hint,
|
||||
HookVisibilityHint::Hidden
|
||||
);
|
||||
let log_contents = fs::read_to_string(log_path).expect("read managed hook log");
|
||||
assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\""));
|
||||
}
|
||||
@@ -274,6 +282,7 @@ async fn requirements_managed_hooks_execute_windows_command_override() {
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -352,6 +361,7 @@ fn unknown_requirement_source_hooks_stay_managed() {
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -420,6 +430,7 @@ fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() {
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -651,6 +662,7 @@ fn requirements_managed_hooks_load_when_managed_dir_is_missing() {
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -1146,6 +1158,7 @@ print(json.dumps({
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -1258,6 +1271,7 @@ fn plugin_hook_sources_expand_plugin_placeholders() {
|
||||
timeout_sec: Some(5),
|
||||
r#async: false,
|
||||
status_message: None,
|
||||
visibility_hint: HookVisibilityHint::Default,
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
|
||||
@@ -589,6 +589,7 @@ mod tests {
|
||||
command: "python3 compact_hook.py".to_string(),
|
||||
timeout_sec: 5,
|
||||
status_message: Some("running compact hook".to_string()),
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
@@ -548,6 +548,7 @@ mod tests {
|
||||
command: "python3 post_tool_use_hook.py".to_string(),
|
||||
timeout_sec: 5,
|
||||
status_message: Some("running post tool use hook".to_string()),
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
@@ -740,6 +740,7 @@ mod tests {
|
||||
command: "echo hook".to_string(),
|
||||
timeout_sec: 5,
|
||||
status_message: None,
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
@@ -361,6 +361,7 @@ mod tests {
|
||||
command: "echo hook".to_string(),
|
||||
timeout_sec: 600,
|
||||
status_message: None,
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
@@ -528,6 +528,7 @@ mod tests {
|
||||
command: "echo hook".to_string(),
|
||||
timeout_sec: 600,
|
||||
status_message: None,
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
@@ -419,6 +419,7 @@ mod tests {
|
||||
command: "echo hook".to_string(),
|
||||
timeout_sec: 5,
|
||||
status_message: None,
|
||||
visibility_hint: Default::default(),
|
||||
source_path: test_path_buf("/tmp/hooks.json").abs(),
|
||||
source: codex_protocol::protocol::HookSource::User,
|
||||
display_order: 0,
|
||||
|
||||
Reference in New Issue
Block a user