mirror of
https://github.com/openai/codex.git
synced 2026-11-13 21:30:47 +00:00
codex: address PR review feedback (#29472)
This commit is contained in:
@@ -129,7 +129,7 @@ pub fn build_command_execution_end_item(payload: &ExecCommandEndEvent) -> Thread
|
||||
}
|
||||
|
||||
fn normalized_app_server_cwd(cwd: &LegacyAppPathString) -> LegacyAppPathString {
|
||||
cwd.to_inferred_path_uri()
|
||||
cwd.to_file_uri()
|
||||
.map(LegacyAppPathString::from)
|
||||
.unwrap_or_else(|| cwd.clone())
|
||||
}
|
||||
|
||||
@@ -21,6 +21,8 @@ use codex_protocol::protocol::PatchApplyStatus;
|
||||
use codex_protocol::protocol::TurnDiffEvent;
|
||||
use codex_shell_command::parse_command::parse_command;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
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;
|
||||
@@ -111,7 +113,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().into(),
|
||||
cwd: legacy_cwd_for_event(cwd),
|
||||
parsed_cmd: parsed_cmd.to_vec(),
|
||||
source,
|
||||
interaction_input,
|
||||
@@ -551,7 +553,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().into(),
|
||||
cwd: legacy_cwd_for_event(exec_input.cwd),
|
||||
parsed_cmd: exec_input.parsed_cmd.to_vec(),
|
||||
source: exec_input.source,
|
||||
interaction_input: exec_input.interaction_input.map(str::to_owned),
|
||||
@@ -567,6 +569,11 @@ async fn emit_exec_end(
|
||||
.await;
|
||||
}
|
||||
|
||||
fn legacy_cwd_for_event(cwd: &PathUri) -> LegacyAppPathString {
|
||||
LegacyAppPathString::from_path_uri(cwd, PathConvention::native())
|
||||
.unwrap_or_else(|_| cwd.clone().into())
|
||||
}
|
||||
|
||||
async fn emit_patch_end(
|
||||
ctx: ToolEventCtx<'_>,
|
||||
changes: HashMap<PathBuf, FileChange>,
|
||||
@@ -816,4 +823,12 @@ mod tests {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn event_cwd_preserves_drive_shaped_posix_path() {
|
||||
let cwd = PathUri::parse("file:///C:/repo").expect("absolute POSIX path URI");
|
||||
|
||||
assert_eq!(legacy_cwd_for_event(&cwd).as_str(), "/C:/repo");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -156,7 +156,7 @@ struct ExecCommandBeginTracePayload<'a> {
|
||||
turn_id: &'a str,
|
||||
started_at_ms: i64,
|
||||
command: &'a [String],
|
||||
cwd: &'a str,
|
||||
cwd: String,
|
||||
parsed_cmd: &'a [codex_protocol::parse_command::ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -182,7 +182,10 @@ impl<'a> From<&'a ExecCommandBeginEvent> for ExecCommandBeginTracePayload<'a> {
|
||||
turn_id,
|
||||
started_at_ms: *started_at_ms,
|
||||
command,
|
||||
cwd: cwd.as_str(),
|
||||
cwd: cwd
|
||||
.to_file_uri()
|
||||
.map(|cwd| cwd.inferred_native_path_string())
|
||||
.unwrap_or_else(|| cwd.as_str().to_string()),
|
||||
parsed_cmd,
|
||||
source: *source,
|
||||
interaction_input: interaction_input.as_deref(),
|
||||
@@ -201,7 +204,7 @@ struct ExecCommandEndTracePayload<'a> {
|
||||
turn_id: &'a str,
|
||||
completed_at_ms: i64,
|
||||
command: &'a [String],
|
||||
cwd: &'a str,
|
||||
cwd: String,
|
||||
parsed_cmd: &'a [codex_protocol::parse_command::ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -241,7 +244,10 @@ impl<'a> From<&'a ExecCommandEndEvent> for ExecCommandEndTracePayload<'a> {
|
||||
turn_id,
|
||||
completed_at_ms: *completed_at_ms,
|
||||
command,
|
||||
cwd: cwd.as_str(),
|
||||
cwd: cwd
|
||||
.to_file_uri()
|
||||
.map(|cwd| cwd.inferred_native_path_string())
|
||||
.unwrap_or_else(|| cwd.as_str().to_string()),
|
||||
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: serde_json::from_value(json!(r"C:\windows"))?,
|
||||
cwd: serde_json::from_value(json!("file:///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: serde_json::from_value(json!("/workspace/project"))?,
|
||||
cwd: serde_json::from_value(json!("file:///workspace/project"))?,
|
||||
parsed_cmd: Vec::new(),
|
||||
source: ExecCommandSource::UnifiedExecInteraction,
|
||||
interaction_input: Some("input".to_string()),
|
||||
|
||||
@@ -81,10 +81,18 @@ impl LegacyAppPathString {
|
||||
})
|
||||
}
|
||||
|
||||
/// Parses this API string as a serialized file URI.
|
||||
pub fn to_file_uri(&self) -> Option<PathUri> {
|
||||
let scheme = self.0.get(..7)?;
|
||||
if !scheme.eq_ignore_ascii_case("file://") {
|
||||
return None;
|
||||
}
|
||||
PathUri::parse(&self.0).ok()
|
||||
}
|
||||
|
||||
/// 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::parse(&self.0)
|
||||
.ok()
|
||||
self.to_file_uri()
|
||||
.or_else(|| PathUri::try_from(self.clone()).ok())
|
||||
}
|
||||
|
||||
|
||||
@@ -480,6 +480,17 @@ fn converts_raw_file_uris_to_inferred_path_uris() {
|
||||
Some(PathUri::parse(raw_uri).expect("raw file URI should parse")),
|
||||
"round-tripping {raw_uri:?}",
|
||||
);
|
||||
assert_eq!(path.to_file_uri(), path.to_inferred_path_uri());
|
||||
}
|
||||
}
|
||||
|
||||
#[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:?}");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user