Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions codex-rs/app-server-protocol/src/protocol/item_builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -90,11 +90,19 @@ 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);
let cwd = payload
.cwd
.as_str()
.get(..7)
.filter(|scheme| scheme.eq_ignore_ascii_case("file://"))
.and_then(|_| payload.cwd.to_inferred_path_uri())
.map(LegacyAppPathString::from)
.unwrap_or_else(|| payload.cwd.clone());
ThreadItem::CommandExecution {
id: payload.call_id.clone(),
command: shlex_join(&payload.command),
cwd: payload.cwd.clone().into(),
cwd,
process_id: payload.process_id.clone(),
source: payload.source.into(),
status: CommandExecutionStatus::InProgress,
Expand All @@ -112,12 +120,20 @@ 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);
let cwd = payload
.cwd
.as_str()
.get(..7)
.filter(|scheme| scheme.eq_ignore_ascii_case("file://"))
.and_then(|_| payload.cwd.to_inferred_path_uri())
.map(LegacyAppPathString::from)
.unwrap_or_else(|| payload.cwd.clone());

ThreadItem::CommandExecution {
id: payload.call_id.clone(),
command: shlex_join(&payload.command),
cwd: payload.cwd.clone().into(),
cwd,
process_id: payload.process_id.clone(),
source: payload.source.into(),
status: (&payload.status).into(),
Expand All @@ -128,14 +144,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()
Expand Down
55 changes: 54 additions & 1 deletion codex-rs/app-server-protocol/src/protocol/item_builders_tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use super::*;
use codex_protocol::protocol::ExecCommandSource;
use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;

#[test]
Expand All @@ -25,7 +27,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(),
Expand All @@ -39,3 +41,54 @@ 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 cwd_uri = PathUri::parse(raw_uri).expect("raw file URI should parse");
let expected_path = cwd_uri
.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"),
}];
let payload = ExecCommandBeginEvent {
call_id: "call-1".to_string(),
process_id: None,
turn_id: "turn-1".to_string(),
started_at_ms: 0,
command: vec!["cat".to_string(), "file.txt".to_string()],
cwd,
parsed_cmd,
source: ExecCommandSource::Agent,
interaction_input: None,
};

assert_eq!(
build_command_execution_begin_item(&payload),
ThreadItem::CommandExecution {
id: "call-1".to_string(),
command: "cat file.txt".to_string(),
cwd: LegacyAppPathString::from(cwd_uri),
process_id: None,
source: CommandExecutionSource::Agent,
status: CommandExecutionStatus::InProgress,
command_actions: vec![CommandAction::Read {
command: "cat file.txt".to_string(),
name: "file.txt".to_string(),
path: expected_path,
}],
aggregated_output: None,
exit_code: None,
duration_ms: None,
}
);
}
4 changes: 2 additions & 2 deletions codex-rs/core/src/tools/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
9 changes: 6 additions & 3 deletions codex-rs/core/tests/suite/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(_))
Expand Down Expand Up @@ -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",
);

Expand Down Expand Up @@ -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"
);

Expand Down
5 changes: 3 additions & 2 deletions codex-rs/protocol/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand All @@ -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)]
Expand Down
23 changes: 18 additions & 5 deletions codex-rs/rollout-trace/src/protocol_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -182,7 +182,14 @@ 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()
.get(..7)
.filter(|scheme| scheme.eq_ignore_ascii_case("file://"))
.and_then(|_| cwd.to_inferred_path_uri())
.map(Into::into)
.unwrap_or_else(|| cwd.clone())
.into_string(),
parsed_cmd,
source: *source,
interaction_input: interaction_input.as_deref(),
Expand All @@ -192,8 +199,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,
Expand Down Expand Up @@ -242,7 +248,14 @@ 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()
.get(..7)
.filter(|scheme| scheme.eq_ignore_ascii_case("file://"))
.and_then(|_| cwd.to_inferred_path_uri())
.map(Into::into)
.unwrap_or_else(|| cwd.clone())
.into_string(),
parsed_cmd,
source: *source,
interaction_input: interaction_input.as_deref(),
Expand Down
4 changes: 2 additions & 2 deletions codex-rs/rollout-trace/src/protocol_event_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!("file:///C:/windows"))?,
parsed_cmd: Vec::new(),
source: ExecCommandSource::Agent,
interaction_input: None,
Expand All @@ -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!("file:///workspace/project"))?,
parsed_cmd: Vec::new(),
source: ExecCommandSource::UnifiedExecInteraction,
interaction_input: Some("input".to_string()),
Expand Down
20 changes: 19 additions & 1 deletion codex-rs/utils/path-uri/src/api_path_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ impl LegacyAppPathString {
})
}

/// Parses this API string as an absolute path using the convention inferred from its spelling.
fn has_serialized_file_uri_prefix(&self) -> bool {
self.0
.get(..7)
.is_some_and(|scheme| scheme.eq_ignore_ascii_case("file://"))
}

/// 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()
}
Expand Down Expand Up @@ -137,6 +143,18 @@ impl TryFrom<LegacyAppPathString> for PathUri {
type Error = LegacyAppPathStringError;

fn try_from(path: LegacyAppPathString) -> Result<Self, Self::Error> {
// Check the original spelling before parsing because URL parsing
// canonicalizes relative aliases such as `file:repo`, losing the
// distinction from serialized absolute file URI spellings.
if path.has_serialized_file_uri_prefix() {
return PathUri::parse(path.as_str()).map_err(|_| {
LegacyAppPathStringError::InvalidNativePath {
path: path.0,
convention: None,
}
});
}

let Some(convention) = path.infer_absolute_path_convention() else {
return Err(LegacyAppPathStringError::InvalidNativePath {
path: path.0,
Expand Down
34 changes: 34 additions & 0 deletions codex-rs/utils/path-uri/src/api_path_string_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,40 @@ fn converts_absolute_api_paths_using_the_inferred_convention() {
}
}

#[test]
fn converts_raw_file_uris_to_inferred_path_uris() {
for (raw_uri, expected_path) in [
("file:///workspace/file.rs", "/workspace/file.rs"),
("file:///C:/workspace/file.rs", r"C:\workspace\file.rs"),
("file://server/share/file.rs", r"\\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");
let expected_path =
serde_json::from_value::<LegacyAppPathString>(serde_json::json!(expected_path))
.expect("expected native path should deserialize as API text");
let expected_uri = PathUri::parse(raw_uri).expect("raw file URI should parse");

assert_eq!(
path.to_inferred_path_uri(),
Some(expected_uri.clone()),
"round-tripping {raw_uri:?}",
);
assert_eq!(PathUri::try_from(path), Ok(expected_uri.clone()));
assert_eq!(LegacyAppPathString::from(expected_uri), expected_path);
}
}

#[test]
fn inferred_path_uri_rejects_relative_file_uri_aliases() {
for raw_uri in ["file:repo", "file:../repo"] {
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(), None, "parsing {raw_uri:?}");
}
}

#[test]
fn converts_native_api_path_to_inferred_absolute_path() {
#[cfg(windows)]
Expand Down
Loading