mirror of
https://github.com/openai/codex.git
synced 2026-11-03 00:21:31 +00:00
[codex] Avoid duplicate hooks.json discovery with profiles (#26418)
## Summary V2 profiles add both `config.toml` and `<profile>.config.toml` to the config stack. Because both user layers resolve hook discovery to the same Codex home, Codex loaded the same `hooks.json` twice. This duplicated hook rows and caused each matching command to run twice. Deduplicate JSON hook discovery by absolute config folder within each effective config stack. TOML hooks remain layer-specific, and multi-cwd `hooks/list` results remain independently resolved per cwd. ## Reproduction 1. Add `config.toml` and `work.config.toml` under `$CODEX_HOME`. 2. Add one command hook to `$CODEX_HOME/hooks.json`. 3. Run Codex with `--profile work`. 4. Trigger the hook. Before this change, one declaration creates two handlers. Afterward, it creates one. Fixes #25645 and addresses the single-cwd duplication in #25437. ## Validation - `cargo nextest run -p codex-hooks` - `just fix -p codex-hooks` - `just fmt` - `just argument-comment-lint -p codex-hooks`
This commit is contained in:
1 parent
7516eb5c70
commit
0d8dee9427
2 files changed
+99
-1
No files matched your search
@@ -1,4 +1,5 @@
|
|||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
use std::collections::HashSet;
|
||||||
use std::fs;
|
use std::fs;
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
|
|
||||||
@@ -69,6 +70,7 @@ pub(crate) fn discover_handlers(
|
|||||||
let mut hook_entries = Vec::new();
|
let mut hook_entries = Vec::new();
|
||||||
let mut warnings = plugin_hook_load_warnings;
|
let mut warnings = plugin_hook_load_warnings;
|
||||||
let mut display_order = 0_i64;
|
let mut display_order = 0_i64;
|
||||||
|
let mut visited_json_hook_folders = HashSet::new();
|
||||||
let hook_states = hook_states_from_stack(config_layer_stack);
|
let hook_states = hook_states_from_stack(config_layer_stack);
|
||||||
let policy = HookDiscoveryPolicy {
|
let policy = HookDiscoveryPolicy {
|
||||||
allow_managed_hooks_only: config_layer_stack.is_some_and(|config_layer_stack| {
|
allow_managed_hooks_only: config_layer_stack.is_some_and(|config_layer_stack| {
|
||||||
@@ -111,7 +113,12 @@ pub(crate) fn discover_handlers(
|
|||||||
if !policy.allows(&policy_source) {
|
if !policy.allows(&policy_source) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
let json_hooks = load_hooks_json(layer.hooks_config_folder().as_deref(), &mut warnings);
|
let json_hooks = match layer.hooks_config_folder() {
|
||||||
|
Some(config_folder) if visited_json_hook_folders.insert(config_folder.clone()) => {
|
||||||
|
load_hooks_json(Some(config_folder.as_path()), &mut warnings)
|
||||||
|
}
|
||||||
|
_ => None,
|
||||||
|
};
|
||||||
let toml_hooks = load_toml_hooks_from_layer(layer, &mut warnings);
|
let toml_hooks = load_toml_hooks_from_layer(layer, &mut warnings);
|
||||||
|
|
||||||
if let (Some((json_source_path, json_events)), Some((toml_source_path, toml_events))) =
|
if let (Some((json_source_path, json_events)), Some((toml_source_path, toml_events))) =
|
||||||
|
|||||||
@@ -1134,6 +1134,97 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() {
|
|||||||
assert_eq!(preview[1].source_path, config_path);
|
assert_eq!(preview[1].source_path, config_path);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn profile_user_layers_load_shared_hooks_json_once() {
|
||||||
|
let temp = tempdir().expect("create temp dir");
|
||||||
|
let config_path =
|
||||||
|
AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path");
|
||||||
|
let profile_path = AbsolutePathBuf::try_from(temp.path().join("work.config.toml"))
|
||||||
|
.expect("absolute profile path");
|
||||||
|
let hooks_json_path =
|
||||||
|
AbsolutePathBuf::try_from(temp.path().join("hooks.json")).expect("absolute hooks path");
|
||||||
|
fs::write(
|
||||||
|
hooks_json_path.as_path(),
|
||||||
|
r#"{
|
||||||
|
"hooks": {
|
||||||
|
"PreToolUse": [
|
||||||
|
{
|
||||||
|
"matcher": "^Bash$",
|
||||||
|
"hooks": [
|
||||||
|
{
|
||||||
|
"type": "command",
|
||||||
|
"command": "python3 /tmp/json-hook.py"
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
}"#,
|
||||||
|
)
|
||||||
|
.expect("write hooks.json");
|
||||||
|
let config_layer_stack = ConfigLayerStack::new(
|
||||||
|
vec![
|
||||||
|
ConfigLayerEntry::new(
|
||||||
|
ConfigLayerSource::User {
|
||||||
|
file: config_path,
|
||||||
|
profile: None,
|
||||||
|
},
|
||||||
|
TomlValue::Table(Default::default()),
|
||||||
|
),
|
||||||
|
ConfigLayerEntry::new(
|
||||||
|
ConfigLayerSource::User {
|
||||||
|
file: profile_path,
|
||||||
|
profile: Some("work".to_string()),
|
||||||
|
},
|
||||||
|
TomlValue::Table(Default::default()),
|
||||||
|
),
|
||||||
|
],
|
||||||
|
ConfigRequirements::default(),
|
||||||
|
ConfigRequirementsToml::default(),
|
||||||
|
)
|
||||||
|
.expect("config layer stack");
|
||||||
|
|
||||||
|
let engine = ClaudeHooksEngine::new(
|
||||||
|
/*enabled*/ true,
|
||||||
|
/*bypass_hook_trust*/ true,
|
||||||
|
Some(&config_layer_stack),
|
||||||
|
Vec::new(),
|
||||||
|
Vec::new(),
|
||||||
|
CommandShell {
|
||||||
|
program: String::new(),
|
||||||
|
args: Vec::new(),
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
assert!(engine.warnings().is_empty());
|
||||||
|
assert_eq!(engine.handlers.len(), 1);
|
||||||
|
let preview = engine.preview_pre_tool_use(&PreToolUseRequest {
|
||||||
|
session_id: ThreadId::new(),
|
||||||
|
turn_id: "turn-1".to_string(),
|
||||||
|
subagent: None,
|
||||||
|
cwd: cwd(),
|
||||||
|
transcript_path: None,
|
||||||
|
model: "gpt-test".to_string(),
|
||||||
|
permission_mode: "default".to_string(),
|
||||||
|
tool_name: "Bash".to_string(),
|
||||||
|
matcher_aliases: Vec::new(),
|
||||||
|
tool_use_id: "tool-1".to_string(),
|
||||||
|
tool_input: serde_json::json!({ "command": "echo hello" }),
|
||||||
|
});
|
||||||
|
assert_eq!(preview.len(), 1);
|
||||||
|
assert_eq!(preview[0].source_path, hooks_json_path);
|
||||||
|
|
||||||
|
let listed = crate::list_hooks(crate::HooksConfig {
|
||||||
|
feature_enabled: true,
|
||||||
|
bypass_hook_trust: true,
|
||||||
|
config_layer_stack: Some(config_layer_stack),
|
||||||
|
..Default::default()
|
||||||
|
});
|
||||||
|
assert!(listed.warnings.is_empty());
|
||||||
|
assert_eq!(listed.hooks.len(), 1);
|
||||||
|
assert_eq!(listed.hooks[0].source_path, hooks_json_path);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn plugin_hook_sources_run_with_plugin_env_and_plugin_source() {
|
async fn plugin_hook_sources_run_with_plugin_env_and_plugin_source() {
|
||||||
let temp = tempdir().expect("create temp dir");
|
let temp = tempdir().expect("create temp dir");
|
||||||
|
|||||||
Reference in new issue
Block a user