mirror of
https://github.com/openai/codex.git
synced 2026-11-01 19:33:35 +00:00
Include environment hooks in thread hook listings (#56553)
## Why Listing hooks by working directory alone omits hooks supplied by a thread's selected execution environments, including hooks that can block a turn. ## What changed - Add `threadId` to `hooks/list` to return a loaded thread's current hook inventory, including selected, ready environments. Reject requests that also specify `cwds`. - Expose `environmentId` in hook metadata and have the TUI hooks browser query the current thread and label environment-owned hooks. Keep working-directory lookup for startup review. - Represent environment hook source paths with `PathUri` and use `LegacyAppPathString` in hook run summaries to preserve native path conventions. ## Testing Add coverage for listing a blocking environment hook and removing it after environment failure, delivering prompt input and reporting exit status in PowerShell and Bash, and running `PreToolUse` hooks only on the command's owning executor. Extend remote network tests to verify that environment hooks use the command's network policy while retaining their filesystem access. GitOrigin-RevId: 97229e22793df51f4bd502c1f9e19f0c52fb2305
This commit is contained in:
@@ -589,7 +589,7 @@ fn append_environment_hooks(
|
||||
handler.source_path = super::HandlerSourcePath::Environment {
|
||||
environment_id: source.environment_id.clone(),
|
||||
cwd: source.cwd.clone(),
|
||||
source_path: path.clone(),
|
||||
source_path: path.clone().into(),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -79,10 +79,10 @@ pub(crate) fn select_handlers_for_matcher_inputs(
|
||||
|
||||
pub(crate) fn running_summary(handler: &ConfiguredHandler) -> HookRunSummary {
|
||||
let source_path = match &handler.source_path {
|
||||
HandlerSourcePath::Local(path)
|
||||
| HandlerSourcePath::Environment {
|
||||
HandlerSourcePath::Local(path) => path.clone().into(),
|
||||
HandlerSourcePath::Environment {
|
||||
source_path: path, ..
|
||||
} => path,
|
||||
} => path.clone().into(),
|
||||
HandlerSourcePath::ExecutorScoped { .. } => {
|
||||
unreachable!("executor-scoped hooks do not produce public hook summaries")
|
||||
}
|
||||
@@ -94,7 +94,7 @@ pub(crate) fn running_summary(handler: &ConfiguredHandler) -> HookRunSummary {
|
||||
handler_type: handler.handler_type(),
|
||||
execution_mode: handler.execution_mode(),
|
||||
scope: scope_for_event(handler.event_name),
|
||||
source_path: source_path.clone(),
|
||||
source_path,
|
||||
source: handler.source,
|
||||
display_order: handler.display_order,
|
||||
status: HookRunStatus::Running,
|
||||
@@ -247,10 +247,10 @@ pub(crate) fn completed_summary(
|
||||
entries: Vec<codex_protocol::protocol::HookOutputEntry>,
|
||||
) -> HookRunSummary {
|
||||
let source_path = match &handler.source_path {
|
||||
HandlerSourcePath::Local(path)
|
||||
| HandlerSourcePath::Environment {
|
||||
HandlerSourcePath::Local(path) => path.clone().into(),
|
||||
HandlerSourcePath::Environment {
|
||||
source_path: path, ..
|
||||
} => path,
|
||||
} => path.clone().into(),
|
||||
HandlerSourcePath::ExecutorScoped { .. } => {
|
||||
unreachable!("executor-scoped hooks do not produce public hook summaries")
|
||||
}
|
||||
@@ -262,7 +262,7 @@ pub(crate) fn completed_summary(
|
||||
handler_type: handler.handler_type(),
|
||||
execution_mode: handler.execution_mode(),
|
||||
scope: scope_for_event(handler.event_name),
|
||||
source_path: source_path.clone(),
|
||||
source_path,
|
||||
source: handler.source,
|
||||
display_order: handler.display_order,
|
||||
status,
|
||||
|
||||
@@ -83,7 +83,7 @@ pub(crate) enum HandlerSourcePath {
|
||||
Environment {
|
||||
environment_id: String,
|
||||
cwd: PathUri,
|
||||
source_path: AbsolutePathBuf,
|
||||
source_path: PathUri,
|
||||
},
|
||||
/// Executor-scoped handlers are currently excluded from user-visible hook reporting
|
||||
/// (events, summary, telemetry). Their handlers are always executed async.
|
||||
@@ -129,7 +129,9 @@ impl std::fmt::Display for HandlerSourcePath {
|
||||
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
match self {
|
||||
Self::Local(path) => write!(formatter, "{}", path.display()),
|
||||
Self::Environment { source_path, .. } => write!(formatter, "{}", source_path.display()),
|
||||
Self::Environment { source_path, .. } => {
|
||||
write!(formatter, "{}", source_path.inferred_native_path_string())
|
||||
}
|
||||
Self::ExecutorScoped {
|
||||
environment_id,
|
||||
manifest_path,
|
||||
|
||||
@@ -32,6 +32,7 @@ use codex_protocol::protocol::HookOutputEntryKind;
|
||||
use codex_protocol::protocol::HookRunStatus;
|
||||
use codex_protocol::protocol::HookSource;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use futures::FutureExt;
|
||||
use futures::future::BoxFuture;
|
||||
@@ -654,7 +655,10 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
tool_input: serde_json::json!({ "command": "echo hello" }),
|
||||
});
|
||||
assert_eq!(preview.len(), 1);
|
||||
assert_eq!(preview[0].source_path, managed_dir);
|
||||
assert_eq!(
|
||||
preview[0].source_path,
|
||||
LegacyAppPathString::from_abs_path(&managed_dir)
|
||||
);
|
||||
|
||||
let outcome = engine
|
||||
.run_pre_tool_use(PreToolUseRequest {
|
||||
@@ -1588,8 +1592,14 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() {
|
||||
.collect::<Vec<_>>(),
|
||||
vec![HookSource::System, HookSource::System]
|
||||
);
|
||||
assert_eq!(preview[0].source_path, hooks_json_path);
|
||||
assert_eq!(preview[1].source_path, config_path);
|
||||
assert_eq!(
|
||||
preview[0].source_path,
|
||||
LegacyAppPathString::from_abs_path(&hooks_json_path)
|
||||
);
|
||||
assert_eq!(
|
||||
preview[1].source_path,
|
||||
LegacyAppPathString::from_abs_path(&config_path)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1671,7 +1681,10 @@ fn profile_user_layers_load_shared_hooks_json_once() {
|
||||
tool_input: serde_json::json!({ "command": "echo hello" }),
|
||||
});
|
||||
assert_eq!(preview.len(), 1);
|
||||
assert_eq!(preview[0].source_path, hooks_json_path);
|
||||
assert_eq!(
|
||||
preview[0].source_path,
|
||||
LegacyAppPathString::from_abs_path(&hooks_json_path)
|
||||
);
|
||||
|
||||
let listed = crate::list_hooks(crate::HooksConfig {
|
||||
feature_enabled: true,
|
||||
@@ -2109,7 +2122,10 @@ print(json.dumps({
|
||||
});
|
||||
assert_eq!(preview.len(), 1);
|
||||
assert_eq!(preview[0].source, HookSource::Plugin);
|
||||
assert_eq!(preview[0].source_path, source_path);
|
||||
assert_eq!(
|
||||
preview[0].source_path,
|
||||
LegacyAppPathString::from_abs_path(&source_path)
|
||||
);
|
||||
let listed = crate::list_hooks(crate::HooksConfig {
|
||||
legacy_notify_argv: None,
|
||||
feature_enabled: true,
|
||||
@@ -2457,7 +2473,8 @@ async fn subagent_stop_paths_are_scoped_to_each_hook() {
|
||||
source_path: HandlerSourcePath::Environment {
|
||||
environment_id: "remote".to_string(),
|
||||
cwd: PathUri::parse("file:///C:/team%20workspace").expect("Windows working directory"),
|
||||
source_path,
|
||||
source_path: PathUri::parse("file:///C:/team%20workspace/requirements.toml")
|
||||
.expect("Windows source path"),
|
||||
},
|
||||
source: HookSource::System,
|
||||
..local.clone()
|
||||
|
||||
Reference in New Issue
Block a user