openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 17:38:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address PR review feedback (#27337)

Browse Source
This commit is contained in:
Eric Traut
2026-06-10 00:43:20 -07:00
parent d7f7844ed0
commit 9bd8079935
5 changed files with 119 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/chatwidget/slash_dispatch.rs
+33 -13
View File
@@ -30,6 +30,12 @@ struct PreparedSlashCommandArgs {
source: SlashCommandDispatchSource,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum PreparedSlashCommandOutcome {
UseDefaultQueuedDrain,
ContinueQueuedDrain,
}
const SIDE_STARTING_CONTEXT_LABEL: &str = "Side starting...";
const SIDE_SLASH_COMMAND_UNAVAILABLE_HINT: &str =
"Press Ctrl+C to return to the main thread first.";
@@ -620,7 +626,7 @@ impl ChatWidget {
&mut self,
cmd: SlashCommand,
prepared: PreparedSlashCommandArgs,
) {
) -> PreparedSlashCommandOutcome {
let PreparedSlashCommandArgs {
args,
text_elements,
@@ -665,19 +671,19 @@ impl ChatWidget {
},
SlashCommand::Rename if !trimmed.is_empty() => {
if !self.ensure_thread_rename_allowed() {
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
}
self.session_telemetry
.counter("codex.thread.rename", /*inc*/ 1, &[]);
let Some(name) = normalize_thread_name(&args) else {
self.add_error_message("Thread name cannot be empty.".to_string());
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
};
self.app_event_tx.set_thread_name(name);
}
SlashCommand::Plan if !trimmed.is_empty() => {
if !self.apply_plan_slash_command() {
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
}
let user_message = self.prepared_inline_user_message(
args,
@@ -701,7 +707,7 @@ impl ChatWidget {
if source == SlashCommandDispatchSource::Live {
self.clear_live_goal_submission();
}
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
}
enum GoalControlCommand {
Clear,
@@ -716,7 +722,7 @@ impl ChatWidget {
if source == SlashCommandDispatchSource::Live {
self.clear_live_goal_submission();
}
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
}
"pause" => Some(GoalControlCommand::SetStatus(AppThreadGoalStatus::Paused)),
"resume" => Some(GoalControlCommand::SetStatus(AppThreadGoalStatus::Active)),
@@ -733,7 +739,7 @@ impl ChatWidget {
if source == SlashCommandDispatchSource::Live {
self.clear_live_goal_submission();
}
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
};
match command {
GoalControlCommand::Clear => {
@@ -749,7 +755,7 @@ impl ChatWidget {
if source == SlashCommandDispatchSource::Live {
self.clear_live_goal_submission();
}
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
}
let pending_pastes = if source == SlashCommandDispatchSource::Live {
self.bottom_pane.composer_pending_pastes()
@@ -760,6 +766,7 @@ impl ChatWidget {
self.config.codex_home.as_path(),
GoalDraft {
objective: args,
text_elements,
pending_pastes,
local_images,
remote_image_urls,
@@ -768,7 +775,14 @@ impl ChatWidget {
Ok(objective) => objective,
Err(err) => {
self.add_error_message(err.to_string());
return;
return match source {
SlashCommandDispatchSource::Live => {
PreparedSlashCommandOutcome::UseDefaultQueuedDrain
}
SlashCommandDispatchSource::Queued => {
PreparedSlashCommandOutcome::ContinueQueuedDrain
}
};
}
};
let Some(thread_id) = self.thread_id else {
@@ -790,7 +804,7 @@ impl ChatWidget {
Some("The session must start before you can set a goal.".to_string()),
);
}
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
};
self.app_event_tx.send(AppEvent::SetThreadGoalObjective {
thread_id,
@@ -808,7 +822,7 @@ impl ChatWidget {
self.add_error_message(format!(
"'/{command}' is unavailable before the session starts."
));
return;
return PreparedSlashCommandOutcome::UseDefaultQueuedDrain;
};
let user_message = self.prepared_inline_user_message(
args,
@@ -849,6 +863,7 @@ impl ChatWidget {
if source == SlashCommandDispatchSource::Live && cmd != SlashCommand::Goal {
self.bottom_pane.drain_pending_submission_state();
}
PreparedSlashCommandOutcome::UseDefaultQueuedDrain
}
pub(super) fn submit_queued_slash_prompt(&mut self, user_message: UserMessage) -> QueueDrain {
@@ -936,7 +951,7 @@ impl ChatWidget {
rest_offset + leading_trimmed,
&text_elements,
);
self.dispatch_prepared_command_with_args(
let outcome = self.dispatch_prepared_command_with_args(
cmd,
PreparedSlashCommandArgs {
args: trimmed_rest.to_string(),
@@ -947,7 +962,12 @@ impl ChatWidget {
source: SlashCommandDispatchSource::Queued,
},
);
self.queued_command_drain_result(cmd)
match outcome {
PreparedSlashCommandOutcome::UseDefaultQueuedDrain => {
self.queued_command_drain_result(cmd)
}
PreparedSlashCommandOutcome::ContinueQueuedDrain => QueueDrain::Continue,
}
}
fn builtin_command_flags(&self) -> BuiltinCommandFlags {
codex-rs/tui/src/chatwidget/tests/goal_menu.rs
+2 -2
View File
@@ -81,7 +81,7 @@ async fn goal_menu_managed_file_snapshot() {
/*token_budget*/ Some(80_000),
);
goal.objective = format!(
"Goal objective file: {}\nRead that file before continuing.",
"Codex goal objective file: {}\nRead that file before continuing.",
test_path_display("/tmp/project/goal.md")
);
@@ -178,7 +178,7 @@ async fn goal_edit_prompt_hydrates_and_materializes_oversized_objective_file() {
/*token_budget*/ Some(80_000),
);
goal.objective = format!(
"Goal objective file: {}\nRead that file before continuing.",
"Codex goal objective file: {}\nRead that file before continuing.",
path.display()
);
codex-rs/tui/src/chatwidget/tests/goal_validation.rs
+69
View File
@@ -63,6 +63,14 @@ fn next_goal_objective(
objective
}
#[test]
fn sentinel_like_objective_is_plain_text() {
let objective =
"Goal objective file: /tmp/not-managed-by-codex\nRead that file before continuing.";
assert_eq!(crate::goal_files::objective_file_path(objective), None);
}
#[tokio::test]
async fn goal_slash_command_accepts_objective_at_limit() {
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
@@ -111,6 +119,38 @@ async fn goal_slash_command_materializes_oversized_objective() {
assert_no_submit_op(&mut op_rx);
}
#[tokio::test]
async fn goal_slash_command_materializes_only_paste_text_element() {
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.set_feature_enabled(Feature::Goals, /*enabled*/ true);
let thread_id = ThreadId::new();
chat.thread_id = Some(thread_id);
let paste = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1);
let placeholder = format!("[Pasted Content {} chars]", paste.chars().count());
chat.bottom_pane.set_composer_text(
format!("/goal keep literal {placeholder} and "),
Vec::new(),
Vec::new(),
);
chat.handle_paste(paste.clone());
submit_current_composer(&mut chat);
let actual_objective = next_goal_objective(&mut rx, thread_id);
assert!(
actual_objective.contains(&format!(
"keep literal {placeholder} and pasted text file: "
)),
"expected only the inserted paste element to be materialized, got {actual_objective:?}"
);
let path = path_after_prefix(&actual_objective, "pasted text file: ");
assert_eq!(
std::fs::read_to_string(path).expect("read paste file"),
paste
);
assert_no_submit_op(&mut op_rx);
}
#[tokio::test]
async fn goal_slash_command_materializes_large_paste() {
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
@@ -137,6 +177,35 @@ async fn goal_slash_command_materializes_large_paste() {
assert_no_submit_op(&mut op_rx);
}
#[tokio::test]
async fn queued_goal_materialization_error_drains_next_input() {
let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
chat.set_feature_enabled(Feature::Goals, /*enabled*/ true);
chat.thread_id = Some(ThreadId::new());
let codex_home_file = chat.config.codex_home.join("not-a-directory");
std::fs::write(&codex_home_file, b"not a directory").expect("write codex home file");
chat.config.codex_home = codex_home_file;
handle_turn_started(&mut chat, "turn-1");
let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1);
queue_composer_text_with_tab(&mut chat, &format!("/goal {objective}"));
queue_composer_text_with_tab(&mut chat, "continue after failed goal");
complete_turn_with_message(&mut chat, "turn-1", Some("done"));
match next_submit_op(&mut op_rx) {
Op::UserTurn { items, .. } => assert_eq!(
items,
vec![UserInput::Text {
text: "continue after failed goal".to_string(),
text_elements: Vec::new(),
}]
),
other => panic!("expected queued message after failed goal, got {other:?}"),
}
assert!(chat.input_queue.queued_user_messages.is_empty());
}
#[tokio::test]
async fn queued_goal_slash_command_materializes_oversized_objective_and_stops_queue() {
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
codex-rs/tui/src/goal_display.rs
+1 -1
View File
@@ -118,7 +118,7 @@ mod tests {
fn goal_usage_summary_formats_managed_file_objective() {
let mut goal = test_thread_goal(/*token_budget*/ None, /*tokens_used*/ 0);
goal.objective =
"Goal objective file: /tmp/project/goal.md\nRead that file before continuing."
"Codex goal objective file: /tmp/project/goal.md\nRead that file before continuing."
.to_string();
assert_eq!(
codex-rs/tui/src/goal_files.rs
+14 -5
View File
@@ -8,42 +8,51 @@ use std::fs;
use std::path::Path;
use std::path::PathBuf;
use crate::bottom_pane::ChatComposer;
use crate::bottom_pane::LocalImageAttachment;
use anyhow::Context;
use anyhow::Result;
use anyhow::bail;
use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS;
use codex_protocol::user_input::TextElement;
use uuid::Uuid;
const GOAL_ATTACHMENT_DIR: &str = "attachments";
const GOAL_FILE_PREFIX: &str = "Goal objective file: ";
const GOAL_FILE_PREFIX: &str = "Codex goal objective file: ";
const GOAL_FILE_NAME: &str = "goal-objective.md";
#[derive(Debug, Default)]
pub(crate) struct GoalDraft {
pub(crate) objective: String,
pub(crate) text_elements: Vec<TextElement>,
pub(crate) pending_pastes: Vec<(String, String)>,
pub(crate) local_images: Vec<LocalImageAttachment>,
pub(crate) remote_image_urls: Vec<String>,
}
pub(crate) fn materialize_goal_draft(codex_home: &Path, draft: GoalDraft) -> Result<String> {
let mut objective = draft.objective.trim().to_string();
if objective.is_empty() {
let mut objective = draft.objective;
if objective.trim().is_empty() {
bail!("Goal objective must not be empty.");
}
let mut output_dir = None;
let mut materialized_pastes = Vec::new();
for (idx, (placeholder, text)) in draft.pending_pastes.iter().enumerate() {
let path = ensure_output_dir(codex_home, &mut output_dir)?
.join(format!("pasted-text-{}.txt", idx + 1));
write_file(&path, text)?;
if !placeholder.is_empty() {
let replacement = format!("pasted text file: {}", path.display());
objective = objective.replace(placeholder, &replacement);
materialized_pastes.push((
placeholder.clone(),
format!("pasted text file: {}", path.display()),
));
}
}
let (expanded_objective, _) =
ChatComposer::expand_pending_pastes(&objective, draft.text_elements, &materialized_pastes);
objective = expanded_objective.trim().to_string();
let mut image_lines = Vec::new();
for (idx, image) in draft.local_images.iter().enumerate() {