Skip to content

Commit 50f53e7

Browse files
bolinfestbolinfest
authored
feat: add path field to ParsedCommand::Read variant (#5275)
`ParsedCommand::Read` has a `name` field that attempts to identify the name of the file being read, but the file may not be in the `cwd` in which the command is invoked as demonstrated by this existing unit test: https://github.com/openai/codex/blob/0139f6780c850d87bb37bbb3a11e763d5dc3b50d/codex-rs/core/src/parse_command.rs#L250-L260 As you can see, `tui/Cargo.toml` is the relative path to the file being read. This PR introduces a new `path: PathBuf` field to `ParsedCommand::Read` that attempts to capture this information. When possible, this is an absolute path, though when relative, it should be resolved against the `cwd` that will be used to run the command to derive the absolute path. This should make it easier for clients to provide UI for a "read file" event that corresponds to the command execution.
1 parent 40fba1b commit 50f53e7

File tree

4 files changed

+145
-17
lines changed

4 files changed

+145
-17
lines changed

‎codex-rs/core/src/parse_command.rs‎

Lines changed: 130 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use crate::bash::try_parse_word_only_commands_sequence;
33
use codex_protocol::parse_command::ParsedCommand;
44
use shlex::split as shlex_split;
55
use shlex::try_join as shlex_try_join;
6+
use std::path::PathBuf;
67

78
fn shlex_join(tokens: &[String]) -> String {
89
shlex_try_join(tokens.iter().map(String::as_str))
@@ -37,6 +38,7 @@ pub fn parse_command(command: &[String]) -> Vec<ParsedCommand> {
3738
/// Tests are at the top to encourage using TDD + Codex to fix the implementation.
3839
mod tests {
3940
use super::*;
41+
use std::path::PathBuf;
4042
use std::string::ToString;
4143

4244
fn shlex_split_safe(s: &str) -> Vec<String> {
@@ -186,6 +188,7 @@ mod tests {
186188
vec![ParsedCommand::Read {
187189
cmd: inner.to_string(),
188190
name: "README.md".to_string(),
191+
path: PathBuf::from("webview/README.md"),
189192
}],
190193
);
191194
}
@@ -197,6 +200,7 @@ mod tests {
197200
vec![ParsedCommand::Read {
198201
cmd: "cat foo.txt".to_string(),
199202
name: "foo.txt".to_string(),
203+
path: PathBuf::from("foo/foo.txt"),
200204
}],
201205
);
202206
}
@@ -219,6 +223,7 @@ mod tests {
219223
vec![ParsedCommand::Read {
220224
cmd: "cat foo.txt".to_string(),
221225
name: "foo.txt".to_string(),
226+
path: PathBuf::from("foo/foo.txt"),
222227
}],
223228
);
224229
}
@@ -243,6 +248,7 @@ mod tests {
243248
vec![ParsedCommand::Read {
244249
cmd: inner.to_string(),
245250
name: "Cargo.toml".to_string(),
251+
path: PathBuf::from("Cargo.toml"),
246252
}],
247253
);
248254
}
@@ -255,6 +261,7 @@ mod tests {
255261
vec![ParsedCommand::Read {
256262
cmd: inner.to_string(),
257263
name: "Cargo.toml".to_string(),
264+
path: PathBuf::from("tui/Cargo.toml"),
258265
}],
259266
);
260267
}
@@ -267,6 +274,7 @@ mod tests {
267274
vec![ParsedCommand::Read {
268275
cmd: inner.to_string(),
269276
name: "README.md".to_string(),
277+
path: PathBuf::from("README.md"),
270278
}],
271279
);
272280
}
@@ -280,6 +288,7 @@ mod tests {
280288
vec![ParsedCommand::Read {
281289
cmd: inner.to_string(),
282290
name: "README.md".to_string(),
291+
path: PathBuf::from("README.md"),
283292
}]
284293
);
285294
}
@@ -449,6 +458,7 @@ mod tests {
449458
vec![ParsedCommand::Read {
450459
cmd: inner.to_string(),
451460
name: "parse_command.rs".to_string(),
461+
path: PathBuf::from("core/src/parse_command.rs"),
452462
}],
453463
);
454464
}
@@ -461,6 +471,7 @@ mod tests {
461471
vec![ParsedCommand::Read {
462472
cmd: inner.to_string(),
463473
name: "history_cell.rs".to_string(),
474+
path: PathBuf::from("tui/src/history_cell.rs"),
464475
}],
465476
);
466477
}
@@ -474,6 +485,7 @@ mod tests {
474485
vec![ParsedCommand::Read {
475486
cmd: "cat -- ansi-escape/Cargo.toml".to_string(),
476487
name: "Cargo.toml".to_string(),
488+
path: PathBuf::from("ansi-escape/Cargo.toml"),
477489
}],
478490
);
479491
}
@@ -503,6 +515,7 @@ mod tests {
503515
vec![ParsedCommand::Read {
504516
cmd: "sed -n '260,640p' exec/src/event_processor_with_human_output.rs".to_string(),
505517
name: "event_processor_with_human_output.rs".to_string(),
518+
path: PathBuf::from("exec/src/event_processor_with_human_output.rs"),
506519
}],
507520
);
508521
}
@@ -662,6 +675,7 @@ mod tests {
662675
vec![ParsedCommand::Read {
663676
cmd: r#"cat "pkg\\src\\main.rs""#.to_string(),
664677
name: "main.rs".to_string(),
678+
path: PathBuf::from(r#"pkg\src\main.rs"#),
665679
}],
666680
);
667681
}
@@ -673,6 +687,7 @@ mod tests {
673687
vec![ParsedCommand::Read {
674688
cmd: "head -n50 Cargo.toml".to_string(),
675689
name: "Cargo.toml".to_string(),
690+
path: PathBuf::from("Cargo.toml"),
676691
}],
677692
);
678693
}
@@ -703,6 +718,7 @@ mod tests {
703718
vec![ParsedCommand::Read {
704719
cmd: "tail -n+10 README.md".to_string(),
705720
name: "README.md".to_string(),
721+
path: PathBuf::from("README.md"),
706722
}],
707723
);
708724
}
@@ -739,6 +755,7 @@ mod tests {
739755
vec![ParsedCommand::Read {
740756
cmd: "cat -- ./-strange-file-name".to_string(),
741757
name: "-strange-file-name".to_string(),
758+
path: PathBuf::from("./-strange-file-name"),
742759
}],
743760
);
744761

@@ -748,6 +765,7 @@ mod tests {
748765
vec![ParsedCommand::Read {
749766
cmd: "sed -n '12,20p' Cargo.toml".to_string(),
750767
name: "Cargo.toml".to_string(),
768+
path: PathBuf::from("Cargo.toml"),
751769
}],
752770
);
753771
}
@@ -840,11 +858,39 @@ pub fn parse_command_impl(command: &[String]) -> Vec<ParsedCommand> {
840858
// Preserve left-to-right execution order for all commands, including bash -c/-lc
841859
// so summaries reflect the order they will run.
842860

843-
// Map each pipeline segment to its parsed summary.
844-
let mut commands: Vec<ParsedCommand> = parts
845-
.iter()
846-
.map(|tokens| summarize_main_tokens(tokens))
847-
.collect();
861+
// Map each pipeline segment to its parsed summary, tracking `cd` to compute paths.
862+
let mut commands: Vec<ParsedCommand> = Vec::new();
863+
let mut cwd: Option<String> = None;
864+
for tokens in &parts {
865+
if let Some((head, tail)) = tokens.split_first()
866+
&& head == "cd"
867+
{
868+
if let Some(dir) = tail.first() {
869+
cwd = Some(match &cwd {
870+
Some(base) => join_paths(base, dir),
871+
None => dir.clone(),
872+
});
873+
}
874+
continue;
875+
}
876+
let parsed = summarize_main_tokens(tokens);
877+
let parsed = match parsed {
878+
ParsedCommand::Read { cmd, name, path } => {
879+
if let Some(base) = &cwd {
880+
let full = join_paths(base, &path.to_string_lossy());
881+
ParsedCommand::Read {
882+
cmd,
883+
name,
884+
path: PathBuf::from(full),
885+
}
886+
} else {
887+
ParsedCommand::Read { cmd, name, path }
888+
}
889+
}
890+
other => other,
891+
};
892+
commands.push(parsed);
893+
}
848894

849895
while let Some(next) = simplify_once(&commands) {
850896
commands = next;
@@ -1129,10 +1175,39 @@ fn parse_bash_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
11291175
cmd: script.clone(),
11301176
}]);
11311177
}
1132-
let mut commands: Vec<ParsedCommand> = filtered_commands
1133-
.into_iter()
1134-
.map(|tokens| summarize_main_tokens(&tokens))
1135-
.collect();
1178+
// Build parsed commands, tracking `cd` segments to compute effective file paths.
1179+
let mut commands: Vec<ParsedCommand> = Vec::new();
1180+
let mut cwd: Option<String> = None;
1181+
for tokens in filtered_commands.into_iter() {
1182+
if let Some((head, tail)) = tokens.split_first()
1183+
&& head == "cd"
1184+
{
1185+
if let Some(dir) = tail.first() {
1186+
cwd = Some(match &cwd {
1187+
Some(base) => join_paths(base, dir),
1188+
None => dir.clone(),
1189+
});
1190+
}
1191+
continue;
1192+
}
1193+
let parsed = summarize_main_tokens(&tokens);
1194+
let parsed = match parsed {
1195+
ParsedCommand::Read { cmd, name, path } => {
1196+
if let Some(base) = &cwd {
1197+
let full = join_paths(base, &path.to_string_lossy());
1198+
ParsedCommand::Read {
1199+
cmd,
1200+
name,
1201+
path: PathBuf::from(full),
1202+
}
1203+
} else {
1204+
ParsedCommand::Read { cmd, name, path }
1205+
}
1206+
}
1207+
other => other,
1208+
};
1209+
commands.push(parsed);
1210+
}
11361211
if commands.len() > 1 {
11371212
commands.retain(|pc| !matches!(pc, ParsedCommand::Unknown { cmd } if cmd == "true"));
11381213
// Apply the same simplifications used for non-bash parsing, e.g., drop leading `cd`.
@@ -1152,7 +1227,7 @@ fn parse_bash_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
11521227
commands = commands
11531228
.into_iter()
11541229
.map(|pc| match pc {
1155-
ParsedCommand::Read { name, cmd, .. } => {
1230+
ParsedCommand::Read { name, cmd, path } => {
11561231
if had_connectors {
11571232
let has_pipe = script_tokens.iter().any(|t| t == "|");
11581233
let has_sed_n = script_tokens.windows(2).any(|w| {
@@ -1163,14 +1238,16 @@ fn parse_bash_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
11631238
ParsedCommand::Read {
11641239
cmd: script.clone(),
11651240
name,
1241+
path,
11661242
}
11671243
} else {
1168-
ParsedCommand::Read { cmd, name }
1244+
ParsedCommand::Read { cmd, name, path }
11691245
}
11701246
} else {
11711247
ParsedCommand::Read {
11721248
cmd: shlex_join(&script_tokens),
11731249
name,
1250+
path,
11741251
}
11751252
}
11761253
}
@@ -1335,10 +1412,12 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
13351412
tail
13361413
};
13371414
if effective_tail.len() == 1 {
1338-
let name = short_display_path(&effective_tail[0]);
1415+
let path = effective_tail[0].clone();
1416+
let name = short_display_path(&path);
13391417
ParsedCommand::Read {
13401418
cmd: shlex_join(main_cmd),
13411419
name,
1420+
path: PathBuf::from(path),
13421421
}
13431422
} else {
13441423
ParsedCommand::Unknown {
@@ -1373,10 +1452,12 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
13731452
i += 1;
13741453
}
13751454
if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) {
1376-
let name = short_display_path(p);
1455+
let path = p.clone();
1456+
let name = short_display_path(&path);
13771457
return ParsedCommand::Read {
13781458
cmd: shlex_join(main_cmd),
13791459
name,
1460+
path: PathBuf::from(path),
13801461
};
13811462
}
13821463
}
@@ -1415,10 +1496,12 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
14151496
i += 1;
14161497
}
14171498
if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) {
1418-
let name = short_display_path(p);
1499+
let path = p.clone();
1500+
let name = short_display_path(&path);
14191501
return ParsedCommand::Read {
14201502
cmd: shlex_join(main_cmd),
14211503
name,
1504+
path: PathBuf::from(path),
14221505
};
14231506
}
14241507
}
@@ -1430,10 +1513,12 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
14301513
// Avoid treating option values as paths (e.g., nl -s " ").
14311514
let candidates = skip_flag_values(tail, &["-s", "-w", "-v", "-i", "-b"]);
14321515
if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) {
1433-
let name = short_display_path(p);
1516+
let path = p.clone();
1517+
let name = short_display_path(&path);
14341518
ParsedCommand::Read {
14351519
cmd: shlex_join(main_cmd),
14361520
name,
1521+
path: PathBuf::from(path),
14371522
}
14381523
} else {
14391524
ParsedCommand::Unknown {
@@ -1448,10 +1533,12 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
14481533
&& is_valid_sed_n_arg(tail.get(1).map(String::as_str)) =>
14491534
{
14501535
if let Some(path) = tail.get(2) {
1451-
let name = short_display_path(path);
1536+
let path = path.clone();
1537+
let name = short_display_path(&path);
14521538
ParsedCommand::Read {
14531539
cmd: shlex_join(main_cmd),
14541540
name,
1541+
path: PathBuf::from(path),
14551542
}
14561543
} else {
14571544
ParsedCommand::Unknown {
@@ -1465,3 +1552,30 @@ fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand {
14651552
},
14661553
}
14671554
}
1555+
1556+
fn is_abs_like(path: &str) -> bool {
1557+
if std::path::Path::new(path).is_absolute() {
1558+
return true;
1559+
}
1560+
let mut chars = path.chars();
1561+
match (chars.next(), chars.next(), chars.next()) {
1562+
// Windows drive path like C:\
1563+
(Some(d), Some(':'), Some('\\')) if d.is_ascii_alphabetic() => return true,
1564+
// UNC path like \\server\share
1565+
(Some('\\'), Some('\\'), _) => return true,
1566+
_ => {}
1567+
}
1568+
false
1569+
}
1570+
1571+
fn join_paths(base: &str, rel: &str) -> String {
1572+
if is_abs_like(rel) {
1573+
return rel.to_string();
1574+
}
1575+
if base.is_empty() {
1576+
return rel.to_string();
1577+
}
1578+
let mut buf = PathBuf::from(base);
1579+
buf.push(rel);
1580+
buf.to_string_lossy().to_string()
1581+
}

‎codex-rs/protocol/src/parse_command.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use serde::Deserialize;
22
use serde::Serialize;
3+
use std::path::PathBuf;
34
use ts_rs::TS;
45

56
#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, TS)]
@@ -8,6 +9,11 @@ pub enum ParsedCommand {
89
Read {
910
cmd: String,
1011
name: String,
12+
/// (Best effort) Path to the file being read by the command. When
13+
/// possible, this is an absolute path, though when relative, it should
14+
/// be resolved against the `cwd`` that will be used to run the command
15+
/// to derive the absolute path.
16+
path: PathBuf,
1117
},
1218
ListFiles {
1319
cmd: String,

‎codex-rs/tui/src/chatwidget/tests.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1631,7 +1631,7 @@ fn status_widget_and_approval_modal_snapshot() {
16311631
let ev = ExecApprovalRequestEvent {
16321632
call_id: "call-approve-exec".into(),
16331633
command: vec!["echo".into(), "hello world".into()],
1634-
cwd: std::path::PathBuf::from("/tmp"),
1634+
cwd: PathBuf::from("/tmp"),
16351635
reason: Some(
16361636
"this is a test reason such as one that would be produced by the model".into(),
16371637
),
@@ -2310,6 +2310,7 @@ fn chatwidget_exec_and_status_layout_vt100_snapshot() {
23102310
ParsedCommand::Read {
23112311
name: "diff_render.rs".into(),
23122312
cmd: "cat diff_render.rs".into(),
2313+
path: "diff_render.rs".into(),
23132314
},
23142315
],
23152316
}),

0 commit comments

Comments
 (0)