mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Collect plugin metrics from unified exec commands (#38253)
## What changed - Create a metrics sidecar for attributed local plugin commands launched through unified exec, and grant the sandbox access to its output file. - Publish valid measurements when the initial command exits, while discarding the sidecar when the process remains live for later polling. - Strip inherited or configured `CODEX_PLUGIN_METRICS_OUTPUT` values so each command can only use its own sidecar. GitOrigin-RevId: 43e669e4c77171aec8a798de0c12ca13bfb8adee
This commit is contained in:
@@ -38,6 +38,7 @@ use crate::unified_exec::NoopSpawnLifecycle;
|
||||
use crate::unified_exec::UnifiedExecError;
|
||||
use crate::unified_exec::UnifiedExecProcess;
|
||||
use crate::unified_exec::UnifiedExecProcessManager;
|
||||
use codex_core_plugins::PluginMetricsSidecar;
|
||||
use codex_network_proxy::ManagedNetworkSandboxContext;
|
||||
use codex_network_proxy::NetworkProxy;
|
||||
use codex_protocol::error::CodexErr;
|
||||
@@ -45,6 +46,7 @@ use codex_protocol::error::SandboxErr;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_sandboxing::SandboxCommand;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use codex_sandboxing::policy_transforms::merge_permission_profiles;
|
||||
use codex_shell_command::powershell::prefix_powershell_script_with_utf8;
|
||||
use codex_tools::UnifiedExecShellMode;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -99,6 +101,11 @@ pub struct UnifiedExecRuntime<'a> {
|
||||
shell_mode: UnifiedExecShellMode,
|
||||
}
|
||||
|
||||
pub(crate) struct UnifiedExecAttempt {
|
||||
pub(crate) process: UnifiedExecProcess,
|
||||
pub(crate) metrics_sidecar: Option<PluginMetricsSidecar>,
|
||||
}
|
||||
|
||||
fn unified_exec_options(
|
||||
network_denial_cancellation_token: Option<CancellationToken>,
|
||||
) -> ExecOptions {
|
||||
@@ -187,7 +194,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
}
|
||||
}
|
||||
|
||||
impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRuntime<'a> {
|
||||
impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRuntime<'a> {
|
||||
fn turn_environment<'b>(&self, req: &'b UnifiedExecRequest) -> &'b TurnEnvironment {
|
||||
&req.turn_environment
|
||||
}
|
||||
@@ -239,7 +246,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
req: &UnifiedExecRequest,
|
||||
attempt: &SandboxAttempt<'_>,
|
||||
ctx: &ToolCtx,
|
||||
) -> Result<UnifiedExecProcess, ToolError> {
|
||||
) -> Result<UnifiedExecAttempt, ToolError> {
|
||||
let base_command = &req.command;
|
||||
let windows_sandbox_proxy_settings_mode = ctx.session.windows_sandbox_proxy_settings_mode;
|
||||
let session_shell = ctx.session.user_shell();
|
||||
@@ -269,7 +276,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
launch_sandbox_permissions,
|
||||
));
|
||||
let env = exec_env_for_sandbox_permissions(&req.env, launch_sandbox_permissions);
|
||||
let (env, managed_network_context, network_proxy_launch) = match managed_network {
|
||||
let (mut env, managed_network_context, network_proxy_launch) = match managed_network {
|
||||
Some(network) if environment_is_remote => {
|
||||
let mut launch = network.remote_launch_config().await.map_err(|err| {
|
||||
ToolError::Codex(CodexErr::Io(io::Error::other(err.to_string())))
|
||||
@@ -330,8 +337,19 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
None => (env, None, None),
|
||||
};
|
||||
let explicit_env_overrides = req.explicit_env_overrides.clone();
|
||||
#[cfg(unix)]
|
||||
let mut env = env;
|
||||
let metrics_sidecar = (!environment_is_remote
|
||||
&& ctx.session.services.analytics_events_client.is_enabled())
|
||||
.then(|| {
|
||||
let cwd = req.cwd.to_abs_path().ok()?;
|
||||
ctx.step_context
|
||||
.turn
|
||||
.plugin_metrics_operation_for_command(&req.command, &cwd)
|
||||
})
|
||||
.flatten()
|
||||
.and_then(PluginMetricsSidecar::create);
|
||||
if let Some(sidecar) = metrics_sidecar.as_ref() {
|
||||
sidecar.install_output_env(&mut env);
|
||||
}
|
||||
#[cfg(unix)]
|
||||
let runtime_path_prepends = {
|
||||
let mut runtime_path_prepends = RuntimePathPrepends::default();
|
||||
@@ -375,6 +393,13 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
} else {
|
||||
command
|
||||
};
|
||||
let sidecar_permissions = metrics_sidecar
|
||||
.as_ref()
|
||||
.map(PluginMetricsSidecar::additional_permissions);
|
||||
let additional_permissions = merge_permission_profiles(
|
||||
req.additional_permissions.as_ref(),
|
||||
sidecar_permissions.as_ref(),
|
||||
);
|
||||
|
||||
if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode {
|
||||
let command = build_unified_exec_sandbox_command(
|
||||
@@ -382,7 +407,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
&req.cwd,
|
||||
&env,
|
||||
managed_network_context.clone(),
|
||||
req.additional_permissions.clone(),
|
||||
additional_permissions.clone(),
|
||||
)
|
||||
.map_err(|error| match error {
|
||||
ToolError::Rejected(_) => {
|
||||
@@ -416,7 +441,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
return self
|
||||
let process = self
|
||||
.manager
|
||||
.open_session_with_prepared_exec_env(
|
||||
req.process_id,
|
||||
@@ -436,7 +461,11 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
}))
|
||||
}
|
||||
other => ToolError::Rejected(other.to_string()),
|
||||
});
|
||||
})?;
|
||||
return Ok(UnifiedExecAttempt {
|
||||
process,
|
||||
metrics_sidecar,
|
||||
});
|
||||
}
|
||||
None => {
|
||||
tracing::warn!(
|
||||
@@ -450,7 +479,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
&req.cwd,
|
||||
&env,
|
||||
managed_network_context,
|
||||
req.additional_permissions.clone(),
|
||||
additional_permissions,
|
||||
)
|
||||
.map_err(|error| match error {
|
||||
ToolError::Rejected(_) => {
|
||||
@@ -459,7 +488,8 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
error @ ToolError::Codex(_) => error,
|
||||
})?;
|
||||
let options = unified_exec_options(attempt.network_denial_cancellation_token.clone());
|
||||
self.manager
|
||||
let process = self
|
||||
.manager
|
||||
.open_session_with_exec_env(
|
||||
req.process_id,
|
||||
command,
|
||||
@@ -474,7 +504,11 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
Box::new(NoopSpawnLifecycle),
|
||||
req.turn_environment.environment.as_ref(),
|
||||
)
|
||||
.await
|
||||
.await?;
|
||||
Ok(UnifiedExecAttempt {
|
||||
process,
|
||||
metrics_sidecar,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -491,6 +525,7 @@ mod tests {
|
||||
use codex_tools::ZshForkConfig;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -20,6 +20,7 @@ use crate::exec_env::inject_apply_patch_env;
|
||||
use crate::exec_env::inject_permission_profile_env;
|
||||
use crate::exec_env::inject_session_id_env;
|
||||
use crate::exec_policy::ExecApprovalRequest;
|
||||
use crate::plugins::metrics::finish_and_track_measurements;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
use crate::sandboxing::ExecServerEnvConfig;
|
||||
@@ -31,6 +32,7 @@ use crate::tools::network_approval::DeferredNetworkApproval;
|
||||
use crate::tools::network_approval::finish_deferred_network_approval;
|
||||
use crate::tools::orchestrator::ToolOrchestrator;
|
||||
use crate::tools::runtimes::is_managed_proxy_env_var;
|
||||
use crate::tools::runtimes::unified_exec::UnifiedExecAttempt;
|
||||
use crate::tools::runtimes::unified_exec::UnifiedExecRequest as UnifiedExecToolRequest;
|
||||
use crate::tools::runtimes::unified_exec::UnifiedExecRuntime;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
@@ -58,7 +60,10 @@ use crate::unified_exec::head_tail_buffer::HeadTailBuffer;
|
||||
use crate::unified_exec::process::OutputHandles;
|
||||
use crate::unified_exec::process::SpawnLifecycleHandle;
|
||||
use crate::unified_exec::process::UnifiedExecProcess;
|
||||
use codex_core_plugins::PLUGIN_METRICS_OUTPUT_ENV_VAR;
|
||||
use codex_core_plugins::PluginCommandAttribution;
|
||||
use codex_core_plugins::PluginMetricsSidecar;
|
||||
use codex_core_plugins::strip_output_env;
|
||||
use codex_network_proxy::NetworkPolicyDecider;
|
||||
use codex_network_proxy::NetworkProxy;
|
||||
use codex_protocol::config_types::ShellEnvironmentPolicy;
|
||||
@@ -127,12 +132,14 @@ fn exec_env_policy_from_shell_policy(
|
||||
exclude.extend([
|
||||
CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(),
|
||||
codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(),
|
||||
PLUGIN_METRICS_OUTPUT_ENV_VAR.to_string(),
|
||||
]);
|
||||
let mut r#set = policy.r#set.clone();
|
||||
r#set.retain(|key, _| {
|
||||
![
|
||||
CODEX_PERMISSION_PROFILE_ENV_VAR,
|
||||
codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR,
|
||||
PLUGIN_METRICS_OUTPUT_ENV_VAR,
|
||||
]
|
||||
.iter()
|
||||
.any(|runtime_key| key.eq_ignore_ascii_case(runtime_key))
|
||||
@@ -238,12 +245,27 @@ struct PreparedProcessHandles {
|
||||
}
|
||||
|
||||
struct InitialExecCommandGuard {
|
||||
active: Arc<AtomicBool>,
|
||||
active: Option<Arc<AtomicBool>>,
|
||||
metrics_sidecar: Option<PluginMetricsSidecar>,
|
||||
}
|
||||
|
||||
impl InitialExecCommandGuard {
|
||||
fn finish_plugin_metrics(&mut self, context: &UnifiedExecContext, exit_code: i32) {
|
||||
finish_and_track_measurements(
|
||||
self.metrics_sidecar.take(),
|
||||
exit_code,
|
||||
&context.session,
|
||||
&context.step_context.turn,
|
||||
&context.call_id,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for InitialExecCommandGuard {
|
||||
fn drop(&mut self) {
|
||||
self.active.store(false, Ordering::Release);
|
||||
if let Some(active) = self.active.as_ref() {
|
||||
active.store(false, Ordering::Release);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -442,15 +464,18 @@ impl UnifiedExecProcessManager {
|
||||
.open_session_with_sandbox(&request, cwd.clone(), context)
|
||||
.await;
|
||||
|
||||
let (process, mut deferred_network_approval) = match process {
|
||||
Ok((process, deferred_network_approval)) => {
|
||||
(Arc::new(process), deferred_network_approval)
|
||||
}
|
||||
let (attempt, mut deferred_network_approval) = match process {
|
||||
Ok((attempt, deferred_network_approval)) => (attempt, deferred_network_approval),
|
||||
Err(err) => {
|
||||
self.release_process_id(request.process_id).await;
|
||||
return Err(err);
|
||||
}
|
||||
};
|
||||
let UnifiedExecAttempt {
|
||||
process,
|
||||
metrics_sidecar,
|
||||
} = attempt;
|
||||
let process = Arc::new(process);
|
||||
let network_denial_monitor = deferred_network_approval.as_ref().map(|deferred| {
|
||||
terminate_process_on_network_denial(
|
||||
Arc::clone(&process),
|
||||
@@ -499,7 +524,7 @@ impl UnifiedExecProcessManager {
|
||||
// Persist live sessions before the initial yield wait so interrupting the
|
||||
// turn cannot drop the last Arc and terminate the background process.
|
||||
let process_started_alive = !process.has_exited() && process.exit_code().is_none();
|
||||
let _initial_exec_command_guard = if process_started_alive {
|
||||
let mut initial_exec_command_guard = if process_started_alive {
|
||||
let initial_exec_command_active = Arc::new(AtomicBool::new(true));
|
||||
self.store_process(
|
||||
Arc::clone(&process),
|
||||
@@ -517,11 +542,15 @@ impl UnifiedExecProcessManager {
|
||||
Arc::clone(&initial_exec_command_active),
|
||||
)
|
||||
.await;
|
||||
Some(InitialExecCommandGuard {
|
||||
active: initial_exec_command_active,
|
||||
})
|
||||
InitialExecCommandGuard {
|
||||
active: Some(initial_exec_command_active),
|
||||
metrics_sidecar,
|
||||
}
|
||||
} else {
|
||||
None
|
||||
InitialExecCommandGuard {
|
||||
active: None,
|
||||
metrics_sidecar,
|
||||
}
|
||||
};
|
||||
|
||||
let yield_time_ms = clamp_yield_time(request.yield_time_ms);
|
||||
@@ -600,7 +629,10 @@ impl UnifiedExecProcessManager {
|
||||
exit_code,
|
||||
process_id,
|
||||
..
|
||||
} => (Some(process_id), exit_code),
|
||||
} => {
|
||||
drop(initial_exec_command_guard.metrics_sidecar.take());
|
||||
(Some(process_id), exit_code)
|
||||
}
|
||||
ProcessStatus::Exited { exit_code, entry } => {
|
||||
if let Err(message) =
|
||||
finish_deferred_network_approval_after_process_exit_for_session(
|
||||
@@ -620,6 +652,8 @@ impl UnifiedExecProcessManager {
|
||||
output_omitted_bytes,
|
||||
)
|
||||
})?;
|
||||
initial_exec_command_guard
|
||||
.finish_plugin_metrics(context, exit_code.unwrap_or(-1));
|
||||
(None, exit_code)
|
||||
}
|
||||
ProcessStatus::Unknown => {
|
||||
@@ -652,6 +686,7 @@ impl UnifiedExecProcessManager {
|
||||
}
|
||||
let exit_code = process.exit_code();
|
||||
let exit = exit_code.unwrap_or(-1);
|
||||
initial_exec_command_guard.finish_plugin_metrics(context, exit);
|
||||
emit_exec_end_for_unified_exec(
|
||||
Arc::clone(&context.session),
|
||||
Arc::clone(&context.step_context.turn),
|
||||
@@ -1163,7 +1198,7 @@ impl UnifiedExecProcessManager {
|
||||
request: &ExecCommandRequest,
|
||||
cwd: PathUri,
|
||||
context: &UnifiedExecContext,
|
||||
) -> Result<(UnifiedExecProcess, Option<DeferredNetworkApproval>), UnifiedExecError> {
|
||||
) -> Result<(UnifiedExecAttempt, Option<DeferredNetworkApproval>), UnifiedExecError> {
|
||||
let turn = &context.step_context.turn;
|
||||
let local_policy_env = create_env(
|
||||
&turn.config.permissions.shell_environment_policy,
|
||||
@@ -1178,7 +1213,15 @@ impl UnifiedExecProcessManager {
|
||||
inject_apply_patch_env(&mut env, &turn.config.features);
|
||||
let active_permission_profile = request.turn_environment.active_permission_profile();
|
||||
inject_permission_profile_env(&mut env, active_permission_profile.as_ref());
|
||||
let env = apply_unified_exec_env(env);
|
||||
let mut env = apply_unified_exec_env(env);
|
||||
strip_output_env(&mut env);
|
||||
let mut explicit_env_overrides = turn
|
||||
.config
|
||||
.permissions
|
||||
.shell_environment_policy
|
||||
.r#set
|
||||
.clone();
|
||||
strip_output_env(&mut explicit_env_overrides);
|
||||
let exec_server_env_config = ExecServerEnvConfig {
|
||||
policy: exec_env_policy_from_shell_policy(
|
||||
&turn.config.permissions.shell_environment_policy,
|
||||
@@ -1215,12 +1258,7 @@ impl UnifiedExecProcessManager {
|
||||
turn_environment: request.turn_environment.clone(),
|
||||
env,
|
||||
exec_server_env_config: Some(exec_server_env_config),
|
||||
explicit_env_overrides: turn
|
||||
.config
|
||||
.permissions
|
||||
.shell_environment_policy
|
||||
.r#set
|
||||
.clone(),
|
||||
explicit_env_overrides,
|
||||
network: request.network.clone(),
|
||||
tty: request.tty,
|
||||
sandbox_permissions: request.sandbox_permissions,
|
||||
|
||||
@@ -109,6 +109,10 @@ fn exec_env_policy_excludes_non_inheritable_and_runtime_variables() {
|
||||
"codex_apply_patch_preserve_line_endings".to_string(),
|
||||
"1".to_string(),
|
||||
),
|
||||
(
|
||||
"codex_plugin_metrics_output".to_string(),
|
||||
"/stale/sidecar".to_string(),
|
||||
),
|
||||
("KEEP".to_string(), "value".to_string()),
|
||||
]),
|
||||
..Default::default()
|
||||
@@ -122,6 +126,7 @@ fn exec_env_policy_excludes_non_inheritable_and_runtime_variables() {
|
||||
exclude: vec![
|
||||
CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(),
|
||||
codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(),
|
||||
PLUGIN_METRICS_OUTPUT_ENV_VAR.to_string(),
|
||||
],
|
||||
r#set: HashMap::from([("KEEP".to_string(), "value".to_string())]),
|
||||
include_only: Vec::new(),
|
||||
|
||||
Reference in New Issue
Block a user