mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Validate /goal objective length in TUI (#20746)
## Why Long `/goal` definitions currently reach lower-level goal validation and can produce an opaque failure. This bug was reported by a user. Pasted instruction blocks are especially confusing because the composer can still contain a paste placeholder before expansion, which may otherwise fall into the generic prompt-size error path. There was also a related paste edge case where `/goal ` followed by a multiline block whose first pasted line was blank looked like a bare `/goal` command. That showed the goal usage/summary instead of setting the pasted objective. ## What Changed This adds TUI-side preflight validation for `/goal <objective>` using the shared `MAX_THREAD_GOAL_OBJECTIVE_CHARS` limit. Oversized typed, queued, and pasted goal objectives now fail locally with a goal-specific message that recommends putting longer instructions in a file and referencing that file from the goal. The TUI now also lets inline-argument slash commands consume later-line arguments before treating the first line as a bare command, so `/goal ` followed by blank lines and then objective text sets the goal instead of opening the bare `/goal` flow. ## Manual Testing 1. Start the TUI with goals enabled and an active session. 2. Submit `/goal ` followed by exactly 4,000 objective characters. It should continue through the normal goal-setting path. 3. Submit `/goal ` followed by 4,001 objective characters. It should not set a goal, and should show `Goal objective is too long: 4,001 characters. Limit: 4,000 characters.` followed by the guidance to put longer instructions in a file and reference that file from the goal. 4. Type `/goal `, paste a large block that becomes a `[Pasted Content ... chars]` placeholder, then submit. It should validate the expanded pasted text and show the goal-specific file guidance rather than the generic prompt-size error. 5. Type `/goal `, paste a multiline block whose first line is blank, then submit. It should set the objective from the non-blank pasted content instead of showing `Usage: /goal <objective>` or the bare goal summary. 6. While a turn is running, queue an oversized `/goal` command. When the queue drains, it should show the same goal-specific error and should not emit a goal-setting request.
This commit is contained in:
committed by
GitHub
Unverified
parent
91b7350187
commit
f09e1936e0
@@ -2810,24 +2810,27 @@ impl ChatComposer {
|
||||
if !self.slash_commands_enabled() || self.is_bash_mode {
|
||||
return None;
|
||||
}
|
||||
let first_line = self.textarea.text().lines().next().unwrap_or("");
|
||||
if let Some((name, rest, _rest_offset)) = parse_slash_name(first_line)
|
||||
&& rest.is_empty()
|
||||
&& let Some(cmd) =
|
||||
slash_commands::find_builtin_command(name, self.builtin_command_flags())
|
||||
{
|
||||
if self.reject_slash_command_if_unavailable(cmd) {
|
||||
self.stage_slash_command_history();
|
||||
self.record_pending_slash_command_history();
|
||||
return Some(InputResult::None);
|
||||
}
|
||||
self.stage_slash_command_history();
|
||||
self.textarea.set_text_clearing_elements("");
|
||||
self.is_bash_mode = false;
|
||||
Some(InputResult::Command(cmd))
|
||||
} else {
|
||||
None
|
||||
let text = self.textarea.text();
|
||||
let first_line = text.lines().next().unwrap_or("");
|
||||
let (name, rest, _rest_offset) = parse_slash_name(first_line)?;
|
||||
if !rest.is_empty() {
|
||||
return None;
|
||||
}
|
||||
let cmd = slash_commands::find_builtin_command(name, self.builtin_command_flags())?;
|
||||
if cmd.supports_inline_args()
|
||||
&& parse_slash_name(text).is_some_and(|(_, full_rest, _)| !full_rest.is_empty())
|
||||
{
|
||||
return None;
|
||||
}
|
||||
if self.reject_slash_command_if_unavailable(cmd) {
|
||||
self.stage_slash_command_history();
|
||||
self.record_pending_slash_command_history();
|
||||
return Some(InputResult::None);
|
||||
}
|
||||
self.stage_slash_command_history();
|
||||
self.textarea.set_text_clearing_elements("");
|
||||
self.is_bash_mode = false;
|
||||
Some(InputResult::Command(cmd))
|
||||
}
|
||||
|
||||
/// Check if the input is a slash command with args (e.g., /review args) and dispatch it.
|
||||
|
||||
@@ -320,6 +320,7 @@ use self::goal_status::GoalStatusState;
|
||||
#[cfg(test)]
|
||||
use self::goal_status::goal_status_indicator_from_app_goal;
|
||||
mod goal_menu;
|
||||
mod goal_validation;
|
||||
mod ide_context;
|
||||
use self::ide_context::IdeContextState;
|
||||
mod interrupts;
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
//! Validation helpers for `/goal` objective text.
|
||||
|
||||
use super::*;
|
||||
use crate::bottom_pane::ChatComposer;
|
||||
use codex_protocol::num_format::format_with_separators;
|
||||
use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS;
|
||||
|
||||
const GOAL_TOO_LONG_FILE_HINT: &str = "Put longer instructions in a file and refer to that file in the goal, for example: /goal follow the instructions in docs/goal.md.";
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub(super) enum GoalObjectiveValidationSource {
|
||||
Live,
|
||||
Queued,
|
||||
}
|
||||
|
||||
impl ChatWidget {
|
||||
pub(super) fn goal_objective_with_pending_pastes_is_allowed(
|
||||
&mut self,
|
||||
args: &str,
|
||||
text_elements: &[TextElement],
|
||||
) -> bool {
|
||||
let pending_pastes = self.bottom_pane.composer_pending_pastes();
|
||||
let objective_chars = if pending_pastes.is_empty() {
|
||||
args.trim().chars().count()
|
||||
} else {
|
||||
let (expanded, _) =
|
||||
ChatComposer::expand_pending_pastes(args, text_elements.to_vec(), &pending_pastes);
|
||||
expanded.trim().chars().count()
|
||||
};
|
||||
self.goal_objective_char_count_is_allowed(
|
||||
objective_chars,
|
||||
GoalObjectiveValidationSource::Live,
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn goal_objective_is_allowed(
|
||||
&mut self,
|
||||
objective: &str,
|
||||
source: GoalObjectiveValidationSource,
|
||||
) -> bool {
|
||||
self.goal_objective_char_count_is_allowed(objective.chars().count(), source)
|
||||
}
|
||||
|
||||
fn goal_objective_char_count_is_allowed(
|
||||
&mut self,
|
||||
actual_chars: usize,
|
||||
source: GoalObjectiveValidationSource,
|
||||
) -> bool {
|
||||
if actual_chars <= MAX_THREAD_GOAL_OBJECTIVE_CHARS {
|
||||
return true;
|
||||
}
|
||||
let actual_chars = format_with_separators(actual_chars as i64);
|
||||
let max_chars = format_with_separators(MAX_THREAD_GOAL_OBJECTIVE_CHARS as i64);
|
||||
self.add_error_message(format!(
|
||||
"Goal objective is too long: {actual_chars} characters. Limit: {max_chars} characters. {GOAL_TOO_LONG_FILE_HINT}"
|
||||
));
|
||||
if source == GoalObjectiveValidationSource::Live {
|
||||
self.bottom_pane
|
||||
.set_composer_text(String::new(), Vec::new(), Vec::new());
|
||||
self.bottom_pane.drain_pending_submission_state();
|
||||
}
|
||||
false
|
||||
}
|
||||
}
|
||||
@@ -5,6 +5,7 @@
|
||||
//! dispatch step and records the staged entry once the command has been handled, so
|
||||
//! slash-command recall follows the same submitted-input rule as ordinary text.
|
||||
|
||||
use super::goal_validation::GoalObjectiveValidationSource;
|
||||
use super::*;
|
||||
use crate::app_event::ThreadGoalSetMode;
|
||||
use crate::bottom_pane::prompt_args::parse_slash_name;
|
||||
@@ -475,6 +476,12 @@ impl ChatWidget {
|
||||
return;
|
||||
}
|
||||
|
||||
if cmd == SlashCommand::Goal
|
||||
&& !self.goal_objective_with_pending_pastes_is_allowed(&args, &text_elements)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
let Some((prepared_args, prepared_elements)) =
|
||||
self.prepare_live_inline_args(args, text_elements)
|
||||
else {
|
||||
@@ -672,6 +679,13 @@ impl ChatWidget {
|
||||
}
|
||||
return;
|
||||
}
|
||||
let validation_source = match source {
|
||||
SlashCommandDispatchSource::Live => GoalObjectiveValidationSource::Live,
|
||||
SlashCommandDispatchSource::Queued => GoalObjectiveValidationSource::Queued,
|
||||
};
|
||||
if !self.goal_objective_is_allowed(objective, validation_source) {
|
||||
return;
|
||||
}
|
||||
let Some(thread_id) = self.thread_id else {
|
||||
if source == SlashCommandDispatchSource::Live {
|
||||
self.queue_user_message_with_options(
|
||||
@@ -804,6 +818,11 @@ impl ChatWidget {
|
||||
rest_offset + leading_trimmed,
|
||||
&text_elements,
|
||||
);
|
||||
if cmd == SlashCommand::Goal
|
||||
&& !self.goal_objective_is_allowed(trimmed_rest, GoalObjectiveValidationSource::Queued)
|
||||
{
|
||||
return QueueDrain::Continue;
|
||||
}
|
||||
self.dispatch_prepared_command_with_args(
|
||||
cmd,
|
||||
PreparedSlashCommandArgs {
|
||||
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/goal_validation.rs
|
||||
expression: rendered
|
||||
---
|
||||
■ Goal objective is too long: 4,001 characters. Limit: 4,000 characters. Put longer instructions in a file and refer to that file in the goal, for example: /goal follow the instructions in docs/goal.md.
|
||||
@@ -226,6 +226,7 @@ mod approval_requests;
|
||||
mod composer_submission;
|
||||
mod exec_flow;
|
||||
mod goal_menu;
|
||||
mod goal_validation;
|
||||
mod guardian;
|
||||
mod helpers;
|
||||
mod history_replay;
|
||||
|
||||
@@ -0,0 +1,224 @@
|
||||
use super::*;
|
||||
use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS;
|
||||
use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn complete_turn_with_message(chat: &mut ChatWidget, turn_id: &str, message: Option<&str>) {
|
||||
if let Some(message) = message {
|
||||
complete_assistant_message(
|
||||
chat,
|
||||
&format!("{turn_id}-message"),
|
||||
message,
|
||||
Some(MessagePhase::FinalAnswer),
|
||||
);
|
||||
}
|
||||
handle_turn_completed(chat, turn_id, /*duration_ms*/ None);
|
||||
}
|
||||
|
||||
fn submit_composer_text(chat: &mut ChatWidget, text: &str) {
|
||||
chat.bottom_pane
|
||||
.set_composer_text(text.to_string(), Vec::new(), Vec::new());
|
||||
submit_current_composer(chat);
|
||||
}
|
||||
|
||||
fn submit_current_composer(chat: &mut ChatWidget) {
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
}
|
||||
|
||||
fn queue_composer_text_with_tab(chat: &mut ChatWidget, text: &str) {
|
||||
chat.bottom_pane
|
||||
.set_composer_text(text.to_string(), Vec::new(), Vec::new());
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE));
|
||||
}
|
||||
|
||||
fn drain_app_events(rx: &mut tokio::sync::mpsc::UnboundedReceiver<AppEvent>) -> Vec<AppEvent> {
|
||||
std::iter::from_fn(|| rx.try_recv().ok()).collect()
|
||||
}
|
||||
|
||||
fn rendered_insert_history(events: &[AppEvent]) -> String {
|
||||
events
|
||||
.iter()
|
||||
.filter_map(|event| match event {
|
||||
AppEvent::InsertHistoryCell(cell) => Some(
|
||||
cell.display_lines(/*width*/ 80)
|
||||
.into_iter()
|
||||
.map(|line| line.to_string())
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n"),
|
||||
),
|
||||
_ => None,
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
#[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;
|
||||
chat.set_feature_enabled(Feature::Goals, /*enabled*/ true);
|
||||
let thread_id = ThreadId::new();
|
||||
chat.thread_id = Some(thread_id);
|
||||
let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS);
|
||||
let command = format!("/goal {objective}");
|
||||
|
||||
submit_composer_text(&mut chat, &command);
|
||||
|
||||
let event = rx.try_recv().expect("expected goal objective event");
|
||||
let AppEvent::SetThreadGoalObjective {
|
||||
thread_id: actual_thread_id,
|
||||
objective: actual_objective,
|
||||
..
|
||||
} = event
|
||||
else {
|
||||
panic!("expected SetThreadGoalObjective, got {event:?}");
|
||||
};
|
||||
assert_eq!(actual_thread_id, thread_id);
|
||||
assert_eq!(actual_objective, objective);
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_slash_command_accepts_multiline_objective_after_blank_first_line() {
|
||||
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 objective = "follow these instructions\npreserve this detail";
|
||||
|
||||
submit_composer_text(&mut chat, &format!("/goal \n\n{objective}"));
|
||||
|
||||
let event = rx.try_recv().expect("expected goal objective event");
|
||||
let AppEvent::SetThreadGoalObjective {
|
||||
thread_id: actual_thread_id,
|
||||
objective: actual_objective,
|
||||
..
|
||||
} = event
|
||||
else {
|
||||
panic!("expected SetThreadGoalObjective, got {event:?}");
|
||||
};
|
||||
assert_eq!(actual_thread_id, thread_id);
|
||||
assert_eq!(actual_objective, objective);
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_slash_command_rejects_oversized_objective() {
|
||||
let (mut chat, mut 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 objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1);
|
||||
|
||||
submit_composer_text(&mut chat, &format!("/goal {objective}"));
|
||||
|
||||
let events = drain_app_events(&mut rx);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AppEvent::SetThreadGoalObjective { .. })),
|
||||
"oversized goal should not emit a SetThreadGoalObjective event: {events:?}"
|
||||
);
|
||||
let rendered = rendered_insert_history(&events);
|
||||
assert_chatwidget_snapshot!("goal_slash_command_oversized_objective_error", rendered);
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_slash_command_rejects_large_paste_using_expanded_length() {
|
||||
let (mut chat, mut 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());
|
||||
chat.bottom_pane
|
||||
.set_composer_text("/goal ".to_string(), Vec::new(), Vec::new());
|
||||
let objective = "x".repeat(MAX_THREAD_GOAL_OBJECTIVE_CHARS + 1);
|
||||
chat.handle_paste(objective);
|
||||
|
||||
assert!(
|
||||
chat.bottom_pane.composer_text().contains("[Pasted Content"),
|
||||
"expected large paste placeholder in composer"
|
||||
);
|
||||
submit_current_composer(&mut chat);
|
||||
|
||||
let events = drain_app_events(&mut rx);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AppEvent::SetThreadGoalObjective { .. })),
|
||||
"oversized pasted goal should not emit a SetThreadGoalObjective event: {events:?}"
|
||||
);
|
||||
let rendered = rendered_insert_history(&events);
|
||||
assert!(rendered.contains("Goal objective is too long"));
|
||||
assert!(rendered.contains("Put longer instructions in a file"));
|
||||
assert!(
|
||||
!rendered.contains("Message exceeds the maximum length"),
|
||||
"expected goal-specific length error, got {rendered:?}"
|
||||
);
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_slash_command_giant_paste_uses_goal_specific_error() {
|
||||
let (mut chat, mut 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());
|
||||
chat.bottom_pane
|
||||
.set_composer_text("/goal ".to_string(), Vec::new(), Vec::new());
|
||||
chat.handle_paste("x".repeat(MAX_USER_INPUT_TEXT_CHARS + 1));
|
||||
|
||||
submit_current_composer(&mut chat);
|
||||
|
||||
let events = drain_app_events(&mut rx);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AppEvent::SetThreadGoalObjective { .. })),
|
||||
"giant pasted goal should not emit a SetThreadGoalObjective event: {events:?}"
|
||||
);
|
||||
let rendered = rendered_insert_history(&events);
|
||||
assert!(rendered.contains("Goal objective is too long"));
|
||||
assert!(rendered.contains("Put longer instructions in a file"));
|
||||
assert!(
|
||||
!rendered.contains("Message exceeds the maximum length"),
|
||||
"expected goal-specific length error, got {rendered:?}"
|
||||
);
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn queued_goal_slash_command_rejects_oversized_objective_and_drains_next_input() {
|
||||
let (mut chat, mut 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());
|
||||
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");
|
||||
assert_eq!(chat.queued_user_messages.len(), 2);
|
||||
|
||||
complete_turn_with_message(&mut chat, "turn-1", Some("done"));
|
||||
|
||||
let events = drain_app_events(&mut rx);
|
||||
assert!(
|
||||
!events
|
||||
.iter()
|
||||
.any(|event| matches!(event, AppEvent::SetThreadGoalObjective { .. })),
|
||||
"oversized queued goal should not emit a SetThreadGoalObjective event: {events:?}"
|
||||
);
|
||||
let rendered = rendered_insert_history(&events);
|
||||
assert!(rendered.contains("Goal objective is too long"));
|
||||
assert!(rendered.contains("Put longer instructions in a file"));
|
||||
match next_submit_op(&mut op_rx) {
|
||||
Op::UserTurn { items, .. } => assert_eq!(
|
||||
items,
|
||||
vec![UserInput::Text {
|
||||
text: "continue".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}]
|
||||
),
|
||||
other => panic!("expected queued follow-up after oversized goal, got {other:?}"),
|
||||
}
|
||||
assert!(chat.queued_user_messages.is_empty());
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
}
|
||||
@@ -16,6 +16,10 @@ fn complete_turn_with_message(chat: &mut ChatWidget, turn_id: &str, message: Opt
|
||||
fn submit_composer_text(chat: &mut ChatWidget, text: &str) {
|
||||
chat.bottom_pane
|
||||
.set_composer_text(text.to_string(), Vec::new(), Vec::new());
|
||||
submit_current_composer(chat);
|
||||
}
|
||||
|
||||
fn submit_current_composer(chat: &mut ChatWidget) {
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
Reference in New Issue
Block a user