mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
app-server: preserve legacy cwd strings in exec events
This commit is contained in:
@@ -35,8 +35,8 @@ use codex_protocol::protocol::PatchApplyBeginEvent;
|
||||
use codex_protocol::protocol::PatchApplyEndEvent;
|
||||
use codex_shell_command::parse_command::parse_command;
|
||||
use codex_shell_command::parse_command::shlex_join;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathConvention;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use tracing::warn;
|
||||
@@ -90,11 +90,11 @@ pub fn build_command_execution_approval_request_item(
|
||||
}
|
||||
|
||||
pub fn build_command_execution_begin_item(payload: &ExecCommandBeginEvent) -> ThreadItem {
|
||||
let command_actions = command_actions_for_path_uri(&payload.parsed_cmd, &payload.cwd);
|
||||
let command_actions = command_actions_for_legacy_cwd(&payload.parsed_cmd, &payload.cwd);
|
||||
ThreadItem::CommandExecution {
|
||||
id: payload.call_id.clone(),
|
||||
command: shlex_join(&payload.command),
|
||||
cwd: payload.cwd.clone().into(),
|
||||
cwd: payload.cwd.clone(),
|
||||
process_id: payload.process_id.clone(),
|
||||
source: payload.source.into(),
|
||||
status: CommandExecutionStatus::InProgress,
|
||||
@@ -112,12 +112,12 @@ pub fn build_command_execution_end_item(payload: &ExecCommandEndEvent) -> Thread
|
||||
Some(payload.aggregated_output.clone())
|
||||
};
|
||||
let duration_ms = i64::try_from(payload.duration.as_millis()).unwrap_or(i64::MAX);
|
||||
let command_actions = command_actions_for_path_uri(&payload.parsed_cmd, &payload.cwd);
|
||||
let command_actions = command_actions_for_legacy_cwd(&payload.parsed_cmd, &payload.cwd);
|
||||
|
||||
ThreadItem::CommandExecution {
|
||||
id: payload.call_id.clone(),
|
||||
command: shlex_join(&payload.command),
|
||||
cwd: payload.cwd.clone().into(),
|
||||
cwd: payload.cwd.clone(),
|
||||
process_id: payload.process_id.clone(),
|
||||
source: payload.source.into(),
|
||||
status: (&payload.status).into(),
|
||||
@@ -128,14 +128,19 @@ pub fn build_command_execution_end_item(payload: &ExecCommandEndEvent) -> Thread
|
||||
}
|
||||
}
|
||||
|
||||
fn command_actions_for_path_uri(parsed_cmd: &[ParsedCommand], cwd: &PathUri) -> Vec<CommandAction> {
|
||||
fn command_actions_for_legacy_cwd(
|
||||
parsed_cmd: &[ParsedCommand],
|
||||
cwd: &LegacyAppPathString,
|
||||
) -> Vec<CommandAction> {
|
||||
// TODO(anp): Carry PathUri into CommandAction so foreign Read actions retain resolved paths.
|
||||
// Until then, omit those actions rather than project a foreign cwd onto the host.
|
||||
let native_cwd = if cwd.infer_path_convention() == Some(PathConvention::native()) {
|
||||
cwd.to_abs_path().ok()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let native_cwd = cwd.to_inferred_path_uri().and_then(|cwd| {
|
||||
if cwd.infer_path_convention() == Some(PathConvention::native()) {
|
||||
cwd.to_abs_path().ok()
|
||||
} else {
|
||||
None
|
||||
}
|
||||
});
|
||||
|
||||
parsed_cmd
|
||||
.iter()
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use super::*;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
@@ -25,7 +26,7 @@ fn foreign_read_is_omitted_without_dropping_other_command_actions() {
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
command_actions_for_path_uri(&parsed_cmd, &cwd),
|
||||
command_actions_for_legacy_cwd(&parsed_cmd, &cwd.into()),
|
||||
vec![
|
||||
CommandAction::ListFiles {
|
||||
command: "ls".to_string(),
|
||||
@@ -39,3 +40,32 @@ fn foreign_read_is_omitted_without_dropping_other_command_actions() {
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn raw_file_uri_cwd_is_converted_for_command_actions() {
|
||||
#[cfg(windows)]
|
||||
let raw_uri = "file:///C:/src";
|
||||
#[cfg(not(windows))]
|
||||
let raw_uri = "file:///usr/local/src";
|
||||
let expected_path = PathUri::parse(raw_uri)
|
||||
.expect("raw file URI should parse")
|
||||
.to_abs_path()
|
||||
.expect("raw file URI should be native")
|
||||
.join("file.txt");
|
||||
let cwd = serde_json::from_value::<LegacyAppPathString>(serde_json::json!(raw_uri))
|
||||
.expect("raw file URI should deserialize as a legacy API path");
|
||||
let parsed_cmd = vec![ParsedCommand::Read {
|
||||
cmd: "cat file.txt".to_string(),
|
||||
name: "file.txt".to_string(),
|
||||
path: PathBuf::from("file.txt"),
|
||||
}];
|
||||
|
||||
assert_eq!(
|
||||
command_actions_for_legacy_cwd(&parsed_cmd, &cwd),
|
||||
vec![CommandAction::Read {
|
||||
command: "cat file.txt".to_string(),
|
||||
name: "file.txt".to_string(),
|
||||
path: expected_path,
|
||||
}],
|
||||
);
|
||||
}
|
||||
|
||||
@@ -111,7 +111,7 @@ pub(crate) async fn emit_exec_command_begin(
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
started_at_ms: now_unix_timestamp_ms(),
|
||||
command: command.to_vec(),
|
||||
cwd: cwd.clone(),
|
||||
cwd: cwd.clone().into(),
|
||||
parsed_cmd: parsed_cmd.to_vec(),
|
||||
source,
|
||||
interaction_input,
|
||||
@@ -551,7 +551,7 @@ async fn emit_exec_end(
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
completed_at_ms: now_unix_timestamp_ms(),
|
||||
command: exec_input.command.to_vec(),
|
||||
cwd: exec_input.cwd.clone(),
|
||||
cwd: exec_input.cwd.clone().into(),
|
||||
parsed_cmd: exec_input.parsed_cmd.to_vec(),
|
||||
source: exec_input.source,
|
||||
interaction_input: exec_input.interaction_input.map(str::to_owned),
|
||||
|
||||
@@ -194,7 +194,8 @@ async fn windows_exec_server_runs_with_native_shell_and_cwd() -> Result<()> {
|
||||
assert_eq!(&begin.command[1..], ["-NoProfile", "-Command", COMMAND]);
|
||||
|
||||
let end = end.context("exec_command should emit an end event")?;
|
||||
let expected_cwd = PathUri::parse("file:///C:/windows")?;
|
||||
let expected_cwd: LegacyAppPathString =
|
||||
PathUri::parse("file:///C:/windows")?.into();
|
||||
assert_eq!((&begin.cwd, &end.cwd), (&expected_cwd, &expected_cwd));
|
||||
assert_eq!((end.exit_code, end.status), (0, ExecCommandStatus::Completed));
|
||||
|
||||
|
||||
@@ -437,7 +437,10 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> {
|
||||
|
||||
assert_command(&begin_event.command, "-lc", "/bin/echo hello unified exec");
|
||||
|
||||
assert_eq!(begin_event.cwd, PathUri::from_host_native_path(&cwd)?);
|
||||
assert_eq!(
|
||||
begin_event.cwd,
|
||||
PathUri::from_host_native_path(&cwd)?.into()
|
||||
);
|
||||
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
@@ -508,7 +511,7 @@ async fn unified_exec_resolves_relative_workdir() -> Result<()> {
|
||||
|
||||
assert_eq!(
|
||||
begin_event.cwd,
|
||||
PathUri::from_host_native_path(&workdir)?,
|
||||
PathUri::from_host_native_path(&workdir)?.into(),
|
||||
"exec_command cwd should resolve relative workdir against turn cwd",
|
||||
);
|
||||
|
||||
@@ -570,7 +573,7 @@ async fn unified_exec_respects_workdir_override() -> Result<()> {
|
||||
|
||||
assert_eq!(
|
||||
begin_event.cwd,
|
||||
PathUri::from_host_native_path(&workdir)?,
|
||||
PathUri::from_host_native_path(&workdir)?.into(),
|
||||
"exec_command cwd should reflect the requested workdir override"
|
||||
);
|
||||
|
||||
|
||||
@@ -54,6 +54,7 @@ use crate::request_permissions::RequestPermissionsResponse;
|
||||
use crate::request_user_input::RequestUserInputResponse;
|
||||
use crate::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use schemars::JsonSchema;
|
||||
use serde::Deserialize;
|
||||
@@ -3345,7 +3346,7 @@ pub struct ExecCommandBeginEvent {
|
||||
/// The command to be executed.
|
||||
pub command: Vec<String>,
|
||||
/// The command's working directory if not the default cwd for the agent.
|
||||
pub cwd: PathUri,
|
||||
pub cwd: LegacyAppPathString,
|
||||
pub parsed_cmd: Vec<ParsedCommand>,
|
||||
/// Where the command originated. Defaults to Agent for backward compatibility.
|
||||
#[serde(default)]
|
||||
@@ -3371,7 +3372,7 @@ pub struct ExecCommandEndEvent {
|
||||
/// The command that was executed.
|
||||
pub command: Vec<String>,
|
||||
/// The command's working directory if not the default cwd for the agent.
|
||||
pub cwd: PathUri,
|
||||
pub cwd: LegacyAppPathString,
|
||||
pub parsed_cmd: Vec<ParsedCommand>,
|
||||
/// Where the command originated. Defaults to Agent for backward compatibility.
|
||||
#[serde(default)]
|
||||
|
||||
@@ -147,7 +147,7 @@ impl Serialize for ToolRuntimePayload<'_> {
|
||||
/// Rollout-trace representation of an exec begin event.
|
||||
///
|
||||
/// Rollout traces share the rollout compatibility requirement that paths remain path-flavored
|
||||
/// strings on disk, even though live events carry `PathUri` internally.
|
||||
/// strings on disk.
|
||||
#[derive(Serialize)]
|
||||
struct ExecCommandBeginTracePayload<'a> {
|
||||
call_id: &'a str,
|
||||
@@ -156,7 +156,7 @@ struct ExecCommandBeginTracePayload<'a> {
|
||||
turn_id: &'a str,
|
||||
started_at_ms: i64,
|
||||
command: &'a [String],
|
||||
cwd: String,
|
||||
cwd: &'a str,
|
||||
parsed_cmd: &'a [codex_protocol::parse_command::ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -182,7 +182,7 @@ impl<'a> From<&'a ExecCommandBeginEvent> for ExecCommandBeginTracePayload<'a> {
|
||||
turn_id,
|
||||
started_at_ms: *started_at_ms,
|
||||
command,
|
||||
cwd: cwd.inferred_native_path_string(),
|
||||
cwd: cwd.as_str(),
|
||||
parsed_cmd,
|
||||
source: *source,
|
||||
interaction_input: interaction_input.as_deref(),
|
||||
@@ -192,8 +192,7 @@ impl<'a> From<&'a ExecCommandBeginEvent> for ExecCommandBeginTracePayload<'a> {
|
||||
|
||||
/// Rollout-trace representation of an exec end event.
|
||||
///
|
||||
/// Like [`ExecCommandBeginTracePayload`], this renders `cwd` as an inferred native path to preserve
|
||||
/// the on-disk format rather than serializing the internal `PathUri`.
|
||||
/// Like [`ExecCommandBeginTracePayload`], this preserves the native-path `cwd` spelling.
|
||||
#[derive(Serialize)]
|
||||
struct ExecCommandEndTracePayload<'a> {
|
||||
call_id: &'a str,
|
||||
@@ -202,7 +201,7 @@ struct ExecCommandEndTracePayload<'a> {
|
||||
turn_id: &'a str,
|
||||
completed_at_ms: i64,
|
||||
command: &'a [String],
|
||||
cwd: String,
|
||||
cwd: &'a str,
|
||||
parsed_cmd: &'a [codex_protocol::parse_command::ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -242,7 +241,7 @@ impl<'a> From<&'a ExecCommandEndEvent> for ExecCommandEndTracePayload<'a> {
|
||||
turn_id,
|
||||
completed_at_ms: *completed_at_ms,
|
||||
command,
|
||||
cwd: cwd.inferred_native_path_string(),
|
||||
cwd: cwd.as_str(),
|
||||
parsed_cmd,
|
||||
source: *source,
|
||||
interaction_input: interaction_input.as_deref(),
|
||||
|
||||
@@ -60,7 +60,7 @@ fn exec_command_trace_payloads_use_inferred_native_cwd() -> anyhow::Result<()> {
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 1234,
|
||||
command: vec!["pwd".to_string()],
|
||||
cwd: "file:///C:/windows".parse()?,
|
||||
cwd: serde_json::from_value(json!(r"C:\windows"))?,
|
||||
parsed_cmd: Vec::new(),
|
||||
source: ExecCommandSource::Agent,
|
||||
interaction_input: None,
|
||||
@@ -71,7 +71,7 @@ fn exec_command_trace_payloads_use_inferred_native_cwd() -> anyhow::Result<()> {
|
||||
turn_id: "turn-1".to_string(),
|
||||
completed_at_ms: 2345,
|
||||
command: vec!["pwd".to_string()],
|
||||
cwd: "file:///workspace/project".parse()?,
|
||||
cwd: serde_json::from_value(json!("/workspace/project"))?,
|
||||
parsed_cmd: Vec::new(),
|
||||
source: ExecCommandSource::UnifiedExecInteraction,
|
||||
interaction_input: Some("input".to_string()),
|
||||
|
||||
@@ -81,9 +81,11 @@ impl LegacyAppPathString {
|
||||
})
|
||||
}
|
||||
|
||||
/// Parses this API string as an absolute path using the convention inferred from its spelling.
|
||||
/// Parses this API string as a file URI or as an absolute path using its inferred convention.
|
||||
pub fn to_inferred_path_uri(&self) -> Option<PathUri> {
|
||||
PathUri::try_from(self.clone()).ok()
|
||||
PathUri::parse(&self.0)
|
||||
.ok()
|
||||
.or_else(|| PathUri::try_from(self.clone()).ok())
|
||||
}
|
||||
|
||||
/// Parses this API string as a host-native absolute path.
|
||||
|
||||
@@ -465,6 +465,24 @@ fn converts_absolute_api_paths_using_the_inferred_convention() {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn converts_raw_file_uris_to_inferred_path_uris() {
|
||||
for raw_uri in [
|
||||
"file:///workspace/file.rs",
|
||||
"file:///C:/workspace/file.rs",
|
||||
"file://server/share/file.rs",
|
||||
] {
|
||||
let path = serde_json::from_value::<LegacyAppPathString>(serde_json::json!(raw_uri))
|
||||
.expect("raw file URI should deserialize as API text");
|
||||
|
||||
assert_eq!(
|
||||
path.to_inferred_path_uri(),
|
||||
Some(PathUri::parse(raw_uri).expect("raw file URI should parse")),
|
||||
"round-tripping {raw_uri:?}",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn converts_native_api_path_to_inferred_absolute_path() {
|
||||
#[cfg(windows)]
|
||||
|
||||
Reference in New Issue
Block a user