mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Collect metrics from plugin shell commands (#38252)
## What changed - Provide matching local plugin commands with a sandbox-writable temporary output file through `CODEX_PLUGIN_METRICS_OUTPUT` when analytics is enabled. - Validate successful command output against the plugin's `analytics.yaml` declaration, including measurement names, enum dimensions, finite values, duplicate rows, and size limits, before publishing analytics events. - Keep the output path reserved from user overrides and clean up the temporary file after execution. ## Testing - Cover output validation, limits, cleanup, sandbox permissions, environment handling, and path replacement. - Verify measurement collection through both classic and zsh-fork shell runtimes. GitOrigin-RevId: 88af0f87dc2f207fcbcca6af498f5c940d79349d
This commit is contained in:
@@ -53,6 +53,7 @@ tokio = { workspace = true, features = ["fs", "macros", "rt", "time"] }
|
||||
toml = { workspace = true }
|
||||
tracing = { workspace = true }
|
||||
url = { workspace = true }
|
||||
uuid = { workspace = true, features = ["v4"] }
|
||||
zip = { workspace = true }
|
||||
|
||||
[target.'cfg(target_os = "macos")'.dependencies]
|
||||
|
||||
@@ -16,6 +16,7 @@ pub mod marketplace_upgrade;
|
||||
mod npm_source;
|
||||
mod plugin_bundle_archive;
|
||||
mod plugin_metrics;
|
||||
mod plugin_metrics_sidecar;
|
||||
mod provider;
|
||||
pub mod remote;
|
||||
pub mod remote_bundle;
|
||||
@@ -78,6 +79,10 @@ pub use marketplace_upgrade::ConfiguredMarketplaceUpgradeOutcome as PluginMarket
|
||||
pub use plugin_metrics::PluginMeasurementDefinition;
|
||||
pub use plugin_metrics::PluginMetricsOperation;
|
||||
pub use plugin_metrics::ResolvedPluginMetricsOperation;
|
||||
pub use plugin_metrics_sidecar::PLUGIN_METRICS_OUTPUT_ENV_VAR;
|
||||
pub use plugin_metrics_sidecar::PluginMeasurementBatch;
|
||||
pub use plugin_metrics_sidecar::PluginMetricsSidecar;
|
||||
pub use plugin_metrics_sidecar::strip_output_env;
|
||||
pub use provider::ExecutorPluginProvider;
|
||||
pub use provider::ExecutorPluginProviderError;
|
||||
pub use provider::ResolvedExecutorPlugin;
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
use crate::ResolvedPluginMetricsOperation;
|
||||
use codex_analytics::PluginMeasurementRow;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use serde::Deserialize;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::BTreeSet;
|
||||
use std::collections::HashMap;
|
||||
use std::io::Read;
|
||||
use std::io::Seek;
|
||||
use std::io::SeekFrom;
|
||||
use tempfile::NamedTempFile;
|
||||
use uuid::Uuid;
|
||||
|
||||
pub const PLUGIN_METRICS_OUTPUT_ENV_VAR: &str = "CODEX_PLUGIN_METRICS_OUTPUT";
|
||||
const MAX_OUTPUT_BYTES: u64 = 64 * 1024;
|
||||
const MAX_OUTPUT_ROWS: usize = 100;
|
||||
|
||||
#[derive(Debug, PartialEq)]
|
||||
pub struct PluginMeasurementBatch {
|
||||
pub plugin_id: String,
|
||||
pub execution_id: String,
|
||||
pub operation: String,
|
||||
pub rows: Vec<PluginMeasurementRow>,
|
||||
}
|
||||
|
||||
pub struct PluginMetricsSidecar {
|
||||
output_file: NamedTempFile,
|
||||
_output_dir: tempfile::TempDir,
|
||||
absolute_output_dir: AbsolutePathBuf,
|
||||
output_env_value: String,
|
||||
resolved: ResolvedPluginMetricsOperation,
|
||||
execution_id: String,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
struct OutputEnvelope {
|
||||
version: u32,
|
||||
measurements: Vec<serde_json::Value>,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
struct OutputMeasurement {
|
||||
name: String,
|
||||
value: f64,
|
||||
#[serde(default)]
|
||||
dimensions: BTreeMap<String, String>,
|
||||
}
|
||||
|
||||
impl PluginMetricsSidecar {
|
||||
pub fn create(resolved: ResolvedPluginMetricsOperation) -> Option<Self> {
|
||||
let sidecar_dir = tempfile::Builder::new()
|
||||
.prefix("codex-plugin-metrics-")
|
||||
.tempdir()
|
||||
.ok()?;
|
||||
let output_file = tempfile::Builder::new()
|
||||
.prefix("measurements-")
|
||||
.suffix(".json")
|
||||
.tempfile_in(sidecar_dir.path())
|
||||
.ok()?;
|
||||
let absolute_output_dir = AbsolutePathBuf::from_absolute_path(sidecar_dir.path()).ok()?;
|
||||
let absolute_output_path = AbsolutePathBuf::from_absolute_path(output_file.path()).ok()?;
|
||||
let output_env_value = absolute_output_path.as_path().to_str()?.to_string();
|
||||
Some(Self {
|
||||
output_file,
|
||||
_output_dir: sidecar_dir,
|
||||
absolute_output_dir,
|
||||
output_env_value,
|
||||
resolved,
|
||||
execution_id: Uuid::new_v4().to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
pub fn install_output_env(&self, env: &mut HashMap<String, String>) {
|
||||
env.insert(
|
||||
PLUGIN_METRICS_OUTPUT_ENV_VAR.to_string(),
|
||||
self.output_env_value.clone(),
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
fn absolute_output_path(&self) -> AbsolutePathBuf {
|
||||
AbsolutePathBuf::from_absolute_path(self.output_file.path()).expect("absolute output path")
|
||||
}
|
||||
|
||||
pub fn additional_permissions(&self) -> AdditionalPermissionProfile {
|
||||
AdditionalPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
/*read*/ None,
|
||||
/*write*/ Some(vec![self.absolute_output_dir.clone()]),
|
||||
)),
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
pub fn finish(mut self, exit_code: i32) -> Option<PluginMeasurementBatch> {
|
||||
if exit_code != 0 {
|
||||
return None;
|
||||
}
|
||||
let rows = parse_output(self.output_file.as_file_mut(), &self.resolved)?;
|
||||
(!rows.is_empty()).then(|| PluginMeasurementBatch {
|
||||
plugin_id: self.resolved.plugin_id.as_key(),
|
||||
execution_id: self.execution_id,
|
||||
operation: self.resolved.operation.operation_name,
|
||||
rows,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
pub fn strip_output_env(env: &mut HashMap<String, String>) {
|
||||
if cfg!(windows) {
|
||||
env.retain(|key, _| !key.eq_ignore_ascii_case(PLUGIN_METRICS_OUTPUT_ENV_VAR));
|
||||
} else {
|
||||
env.remove(PLUGIN_METRICS_OUTPUT_ENV_VAR);
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_output(
|
||||
output_file: &mut std::fs::File,
|
||||
resolved: &ResolvedPluginMetricsOperation,
|
||||
) -> Option<Vec<PluginMeasurementRow>> {
|
||||
let mut contents = Vec::new();
|
||||
output_file.seek(SeekFrom::Start(0)).ok()?;
|
||||
output_file
|
||||
.take(MAX_OUTPUT_BYTES + 1)
|
||||
.read_to_end(&mut contents)
|
||||
.ok()?;
|
||||
if contents.len() as u64 > MAX_OUTPUT_BYTES {
|
||||
return None;
|
||||
}
|
||||
let output: OutputEnvelope = serde_json::from_slice(&contents).ok()?;
|
||||
if output.version != 1 || output.measurements.len() > MAX_OUTPUT_ROWS {
|
||||
return None;
|
||||
}
|
||||
|
||||
let mut seen = BTreeSet::new();
|
||||
let mut rows = Vec::new();
|
||||
for value in output.measurements {
|
||||
let Ok(measurement) = serde_json::from_value::<OutputMeasurement>(value) else {
|
||||
continue;
|
||||
};
|
||||
let Some(definition) = resolved.operation.measurements.get(&measurement.name) else {
|
||||
continue;
|
||||
};
|
||||
if !measurement.value.is_finite()
|
||||
|| measurement.dimensions.len() != definition.enum_dimensions.len()
|
||||
|| !definition.enum_dimensions.iter().all(|(name, allowed)| {
|
||||
measurement
|
||||
.dimensions
|
||||
.get(name)
|
||||
.is_some_and(|value| allowed.contains(value))
|
||||
})
|
||||
|| !seen.insert((measurement.name.clone(), measurement.dimensions.clone()))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
rows.push(PluginMeasurementRow {
|
||||
measurement_name: measurement.name,
|
||||
number_value: measurement.value,
|
||||
dimensions: measurement.dimensions,
|
||||
});
|
||||
}
|
||||
Some(rows)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "plugin_metrics_sidecar_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,192 @@
|
||||
use super::*;
|
||||
use crate::PluginMeasurementDefinition;
|
||||
use crate::PluginMetricsOperation;
|
||||
use codex_plugin::PluginId;
|
||||
use codex_protocol::models::LegacyReadWriteRoots;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
|
||||
fn create_sidecar() -> PluginMetricsSidecar {
|
||||
PluginMetricsSidecar::create(resolved_operation()).expect("create sidecar")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sidecar_is_created_in_system_temp_with_private_permissions() {
|
||||
let output_dir =
|
||||
AbsolutePathBuf::from_absolute_path(std::env::temp_dir()).expect("absolute temp directory");
|
||||
let sidecar = PluginMetricsSidecar::create(resolved_operation()).expect("create sidecar");
|
||||
|
||||
assert_eq!(
|
||||
sidecar.absolute_output_path().parent(),
|
||||
Some(sidecar.absolute_output_dir.clone())
|
||||
);
|
||||
assert_eq!(sidecar.absolute_output_dir.parent(), Some(output_dir));
|
||||
assert!(sidecar.absolute_output_dir.as_path().is_dir());
|
||||
let roots = sidecar
|
||||
.additional_permissions()
|
||||
.file_system
|
||||
.expect("file system permissions")
|
||||
.legacy_read_write_roots()
|
||||
.expect("legacy roots");
|
||||
assert_eq!(
|
||||
roots,
|
||||
LegacyReadWriteRoots {
|
||||
read: None,
|
||||
write: Some(vec![sidecar.absolute_output_dir]),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
fn resolved_operation() -> ResolvedPluginMetricsOperation {
|
||||
ResolvedPluginMetricsOperation {
|
||||
plugin_id: PluginId::parse("security@openai-curated").expect("valid plugin id"),
|
||||
operation: PluginMetricsOperation {
|
||||
operation_name: "security_scan".to_string(),
|
||||
measurements: BTreeMap::from([
|
||||
(
|
||||
"finding_count".to_string(),
|
||||
PluginMeasurementDefinition {
|
||||
enum_dimensions: BTreeMap::from([(
|
||||
"severity".to_string(),
|
||||
BTreeSet::from(["high".to_string(), "low".to_string()]),
|
||||
)]),
|
||||
},
|
||||
),
|
||||
(
|
||||
"files_scanned".to_string(),
|
||||
PluginMeasurementDefinition {
|
||||
enum_dimensions: BTreeMap::new(),
|
||||
},
|
||||
),
|
||||
]),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sidecar_keeps_valid_rows_and_first_duplicate_then_cleans_up() {
|
||||
let sidecar = create_sidecar();
|
||||
let path = sidecar.absolute_output_path();
|
||||
std::fs::write(
|
||||
path.as_path(),
|
||||
json!({
|
||||
"version": 1,
|
||||
"measurements": [
|
||||
{"name": "finding_count", "value": 3, "dimensions": {"severity": "high"}},
|
||||
{"name": "unknown", "value": 1},
|
||||
{"name": "finding_count", "value": 4},
|
||||
{"name": "finding_count", "value": 5, "dimensions": {"severity": "critical"}},
|
||||
{"name": "finding_count", "value": 6, "dimensions": {"severity": "high", "extra": "x"}},
|
||||
{"name": "finding_count", "value": 99, "dimensions": {"severity": "high"}},
|
||||
{"name": "files_scanned", "value": 17},
|
||||
{"name": "files_scanned", "value": "not-a-number"},
|
||||
{"name": "files_scanned", "value": 18, "unknown": true}
|
||||
]
|
||||
})
|
||||
.to_string(),
|
||||
)
|
||||
.expect("write output");
|
||||
|
||||
let batch = sidecar.finish(/*exit_code*/ 0).expect("valid measurements");
|
||||
let execution_id = batch.execution_id.clone();
|
||||
assert_eq!(
|
||||
batch,
|
||||
PluginMeasurementBatch {
|
||||
plugin_id: "security@openai-curated".to_string(),
|
||||
execution_id: execution_id.clone(),
|
||||
operation: "security_scan".to_string(),
|
||||
rows: vec![
|
||||
PluginMeasurementRow {
|
||||
measurement_name: "finding_count".to_string(),
|
||||
number_value: 3.0,
|
||||
dimensions: BTreeMap::from([("severity".to_string(), "high".to_string(),)]),
|
||||
},
|
||||
PluginMeasurementRow {
|
||||
measurement_name: "files_scanned".to_string(),
|
||||
number_value: 17.0,
|
||||
dimensions: BTreeMap::new(),
|
||||
},
|
||||
],
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
Uuid::parse_str(&execution_id)
|
||||
.expect("execution id UUID")
|
||||
.get_version(),
|
||||
Some(uuid::Version::Random)
|
||||
);
|
||||
assert!(!path.exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn malformed_oversized_and_nonzero_outputs_are_ignored_and_cleaned_up() {
|
||||
for output in [
|
||||
r#"{"version":2,"measurements":[]}"#.as_bytes().to_vec(),
|
||||
r#"{"version":1,"measurements":[],"unknown":true}"#.as_bytes().to_vec(),
|
||||
json!({
|
||||
"version": 1,
|
||||
"measurements": vec![json!({"name": "files_scanned", "value": 1}); MAX_OUTPUT_ROWS + 1]
|
||||
})
|
||||
.to_string()
|
||||
.into_bytes(),
|
||||
vec![b' '; MAX_OUTPUT_BYTES as usize + 1],
|
||||
] {
|
||||
let sidecar = create_sidecar();
|
||||
let path = sidecar.absolute_output_path();
|
||||
std::fs::write(path.as_path(), output).expect("write output");
|
||||
assert_eq!(sidecar.finish(/*exit_code*/ 0), None);
|
||||
assert!(!path.exists());
|
||||
}
|
||||
|
||||
let sidecar = create_sidecar();
|
||||
let path = sidecar.absolute_output_path();
|
||||
std::fs::write(
|
||||
path.as_path(),
|
||||
r#"{"version":1,"measurements":[{"name":"files_scanned","value":1}]}"#,
|
||||
)
|
||||
.expect("write output");
|
||||
assert_eq!(sidecar.finish(/*exit_code*/ 1), None);
|
||||
assert!(!path.exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reserved_output_env_is_absent_without_sidecar_and_cannot_be_overridden() {
|
||||
let mut env = HashMap::from([
|
||||
(
|
||||
PLUGIN_METRICS_OUTPUT_ENV_VAR.to_string(),
|
||||
"/user/path".to_string(),
|
||||
),
|
||||
("KEEP".to_string(), "value".to_string()),
|
||||
]);
|
||||
strip_output_env(&mut env);
|
||||
assert_eq!(
|
||||
env,
|
||||
HashMap::from([("KEEP".to_string(), "value".to_string())])
|
||||
);
|
||||
|
||||
let sidecar = create_sidecar();
|
||||
let path = sidecar.absolute_output_path();
|
||||
sidecar.install_output_env(&mut env);
|
||||
assert_eq!(
|
||||
env.get(PLUGIN_METRICS_OUTPUT_ENV_VAR).map(String::as_str),
|
||||
path.as_path().to_str()
|
||||
);
|
||||
drop(sidecar);
|
||||
assert!(!path.exists());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn sidecar_reads_the_original_file_after_path_replacement() {
|
||||
let sidecar = create_sidecar();
|
||||
let path = sidecar.absolute_output_path();
|
||||
std::fs::remove_file(path.as_path()).expect("remove original output path");
|
||||
std::fs::write(
|
||||
path.as_path(),
|
||||
r#"{"version":1,"measurements":[{"name":"files_scanned","value":99}]}"#,
|
||||
)
|
||||
.expect("write replacement output");
|
||||
|
||||
assert_eq!(sidecar.finish(/*exit_code*/ 0), None);
|
||||
assert!(!path.exists());
|
||||
}
|
||||
Reference in New Issue
Block a user