From df8b48267896bdcba4ed401c095a279cb2b4c39b Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 6 Apr 2026 16:12:42 -0700 Subject: [PATCH] codex: harden /loop parsing for PR #16953 --- codex-rs/tui/src/alarm_scheduler.rs | 255 ++++++++++++++----------- codex-rs/tui/src/app_server_session.rs | 16 -- 2 files changed, 142 insertions(+), 129 deletions(-) diff --git a/codex-rs/tui/src/alarm_scheduler.rs b/codex-rs/tui/src/alarm_scheduler.rs index 22a60382d6..42c7a58d9b 100644 --- a/codex-rs/tui/src/alarm_scheduler.rs +++ b/codex-rs/tui/src/alarm_scheduler.rs @@ -1,14 +1,8 @@ -use crate::AppServerTarget; -use crate::start_app_server_for_picker; -use codex_app_server_client::AppServerEvent; use codex_app_server_protocol::AlarmDelivery; -use codex_app_server_protocol::ServerNotification; -use codex_app_server_protocol::ThreadItem; use codex_core::config::Config; -use codex_protocol::user_input::UserInput; +use regex_lite::Regex; use serde::Deserialize; -use serde_json::json; -use tracing::warn; +use std::sync::LazyLock; #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct ParsedAlarmSpec { @@ -46,117 +40,97 @@ impl ParsedAlarmSpecResponse { } } +static EVERY_PATTERN: LazyLock = LazyLock::new(|| { + Regex::new(r"(?i)\bevery\s+(\d+)\s*(seconds?|secs?|s|minutes?|mins?|m|hours?|hrs?|h|days?|d)\b") + .unwrap_or_else(|err| panic!("valid recurring alarm parser regex: {err}")) +}); +static RUN_ONCE_PATTERN: LazyLock = LazyLock::new(|| { + Regex::new(r"(?i)\b(run once|once|one-shot|one shot|single-run|single run|just once)\b") + .unwrap_or_else(|err| panic!("valid one-shot alarm parser regex: {err}")) +}); +static STEER_CURRENT_TURN_PATTERN: LazyLock = LazyLock::new(|| { + Regex::new( + r"(?i)\b(as a steer|steer (?:the )?current turn|steer this turn|during (?:the )?current turn|during this turn|in (?:the )?current turn)\b", + ) + .unwrap_or_else(|err| panic!("valid steer alarm parser regex: {err}")) +}); + pub(crate) async fn parse_alarm_spec( - config: Config, - target: AppServerTarget, + _config: Config, + _target: crate::AppServerTarget, spec: String, ) -> std::result::Result { - let mut app_server = start_app_server_for_picker(&config, &target) - .await - .map_err(|err| format!("Failed to start parser session: {err}"))?; - let result = async { - let thread = app_server - .start_ephemeral_thread(&config) - .await - .map_err(|err| format!("Failed to start parser thread: {err}"))?; + parse_alarm_spec_text(&spec) +} - let output_schema = json!({ - "type": "object", - "properties": { - "cronExpression": { "type": "string" }, - "prompt": { "type": "string" }, - "runOnce": { "type": ["boolean", "null"] }, - "delivery": { "type": "string" } - }, - "required": ["cronExpression", "prompt", "runOnce", "delivery"], - "additionalProperties": false - }); - - let parser_prompt = format!( - "{}\n{}", - concat!( - "Convert the user's /loop spec into JSON with keys cronExpression, prompt, runOnce, and delivery.\n", - "Only emit valid JSON matching the schema.\n", - "Supported cronExpression values for this client are:\n", - "- @after-turn\n", - "- @every s\n", - "- @every m\n", - "- @every h\n", - "- @every d\n", - "Use @after-turn when no explicit interval is present.\n", - "Set runOnce to true only when the user clearly asked for a one-shot run.\n", - "Set delivery to `steer-current-turn` only when the user clearly asked for the alarm to be delivered during the current turn or as a steer.\n", - "Otherwise set delivery to `after-turn`.\n", - "Keep stop conditions and alarm intent inside prompt.\n", - "If the schedule request is unsupported by the allowed cronExpression formats, return an empty cronExpression.\n", - "User spec:" - ), - spec - ); - - app_server - .turn_start( - codex_protocol::ThreadId::from_string(&thread.id) - .map_err(|err| format!("Parser thread id was invalid: {err}"))?, - vec![UserInput::Text { - text: parser_prompt, - text_elements: Vec::new(), - }], - config.cwd.to_path_buf(), - config.permissions.approval_policy.value(), - config.approvals_reviewer, - config.permissions.sandbox_policy.get().clone(), - config.model.clone().unwrap_or_default(), - /*effort*/ None, - /*summary*/ None, - config.service_tier.map(Some), - /*collaboration_mode*/ None, - config.personality, - Some(output_schema), - ) - .await - .map_err(|err| format!("Failed to run /loop parser prompt: {err}"))?; - - let mut final_message = None; - while let Some(event) = app_server.next_event().await { - match event { - AppServerEvent::ServerNotification(ServerNotification::ItemCompleted( - notification, - )) => { - if let ThreadItem::AgentMessage { text, .. } = notification.item { - final_message = Some(text); - } - } - AppServerEvent::ServerNotification(ServerNotification::TurnCompleted( - notification, - )) => { - if notification.turn.error.is_some() { - return Err("The /loop parser turn failed.".to_string()); - } - break; - } - AppServerEvent::Disconnected { message } => { - return Err(format!("The /loop parser session disconnected: {message}")); - } - AppServerEvent::Lagged { .. } - | AppServerEvent::ServerNotification(_) - | AppServerEvent::ServerRequest(_) => {} - } - } - - let response = - final_message.ok_or_else(|| "The /loop parser returned no output.".to_string())?; - let parsed: ParsedAlarmSpecResponse = serde_json::from_str(&response) - .map_err(|err| format!("Invalid /loop parser JSON: {err}"))?; - parsed.into_parsed() - } - .await; - - if let Err(err) = app_server.shutdown().await { - warn!(%err, "failed to shut down temporary /loop parser session"); +fn parse_alarm_spec_text(spec: &str) -> std::result::Result { + let trimmed = spec.trim(); + if trimmed.is_empty() { + return Err("Could not determine a prompt from /loop input.".to_string()); } - result + let delivery = if STEER_CURRENT_TURN_PATTERN.is_match(trimmed) { + AlarmDelivery::SteerCurrentTurn + } else { + AlarmDelivery::AfterTurn + }; + let run_once = RUN_ONCE_PATTERN.is_match(trimmed).then_some(true); + + let (cron_expression, prompt) = if let Some(captures) = EVERY_PATTERN.captures(trimmed) { + let quantity = captures.get(1).map(|m| m.as_str()).ok_or_else(|| { + "Could not determine a supported schedule from /loop input.".to_string() + })?; + let unit = captures.get(2).map(|m| m.as_str()).ok_or_else(|| { + "Could not determine a supported schedule from /loop input.".to_string() + })?; + let schedule = captures.get(0).ok_or_else(|| { + "Could not determine a supported schedule from /loop input.".to_string() + })?; + let prompt = remove_match(trimmed, schedule.start(), schedule.end()); + ( + format!("@every {quantity}{}", normalized_unit_suffix(unit)?), + clean_prompt_text(&prompt), + ) + } else { + ("@after-turn".to_string(), trimmed.to_string()) + }; + + ParsedAlarmSpecResponse { + cron_expression, + prompt, + run_once, + delivery, + } + .into_parsed() +} + +fn normalized_unit_suffix(unit: &str) -> std::result::Result { + match unit.to_ascii_lowercase().as_str() { + "s" | "second" | "seconds" | "sec" | "secs" => Ok('s'), + "m" | "minute" | "minutes" | "min" | "mins" => Ok('m'), + "h" | "hour" | "hours" | "hr" | "hrs" => Ok('h'), + "d" | "day" | "days" => Ok('d'), + _ => Err("Could not determine a supported schedule from /loop input.".to_string()), + } +} + +fn remove_match(text: &str, start: usize, end: usize) -> String { + let prefix = text[..start].trim_end(); + let suffix = text[end..].trim_start_matches([' ', ',', ':', ';', '-']); + match (prefix.is_empty(), suffix.is_empty()) { + (true, true) => String::new(), + (true, false) => suffix.to_string(), + (false, true) => prefix.to_string(), + (false, false) => format!("{prefix} {suffix}"), + } +} + +fn clean_prompt_text(prompt: &str) -> String { + prompt + .trim() + .trim_start_matches([',', ':', ';', '-']) + .trim() + .to_string() } pub(crate) fn format_alarm_summary( @@ -178,3 +152,58 @@ fn delivery_str(delivery: AlarmDelivery) -> &'static str { AlarmDelivery::SteerCurrentTurn => "steer-current-turn", } } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn parse_alarm_spec_defaults_to_after_turn_without_interval() { + assert_eq!( + parse_alarm_spec_text("give me a random animal name").expect("parse succeeds"), + ParsedAlarmSpec { + cron_expression: "@after-turn".to_string(), + prompt: "give me a random animal name".to_string(), + run_once: None, + delivery: AlarmDelivery::AfterTurn, + } + ); + } + + #[test] + fn parse_alarm_spec_extracts_supported_recurring_interval() { + assert_eq!( + parse_alarm_spec_text("every 10 seconds give me an inspirational affirmation") + .expect("parse succeeds"), + ParsedAlarmSpec { + cron_expression: "@every 10s".to_string(), + prompt: "give me an inspirational affirmation".to_string(), + run_once: None, + delivery: AlarmDelivery::AfterTurn, + } + ); + } + + #[test] + fn parse_alarm_spec_detects_run_once_and_steer_delivery() { + assert_eq!( + parse_alarm_spec_text("once during this turn remind me to rebase") + .expect("parse succeeds"), + ParsedAlarmSpec { + cron_expression: "@after-turn".to_string(), + prompt: "once during this turn remind me to rebase".to_string(), + run_once: Some(true), + delivery: AlarmDelivery::SteerCurrentTurn, + } + ); + } + + #[test] + fn parse_alarm_spec_rejects_missing_prompt_after_schedule() { + assert_eq!( + parse_alarm_spec_text("every 10 seconds").expect_err("parse should fail"), + "Could not determine a prompt from /loop input.".to_string() + ); + } +} diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index babaf8e651..7d1ca1f1c5 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -320,22 +320,6 @@ impl AppServerSession { started_thread_from_start_response(response, config).await } - pub(crate) async fn start_ephemeral_thread(&mut self, config: &Config) -> Result { - let request_id = self.next_request_id(); - let mut params = thread_start_params_from_config( - config, - self.thread_params_mode(), - self.remote_cwd_override.as_deref(), - ); - params.ephemeral = Some(true); - let response: ThreadStartResponse = self - .client - .request_typed(ClientRequest::ThreadStart { request_id, params }) - .await - .wrap_err("thread/start failed for ephemeral TUI session")?; - Ok(response.thread) - } - pub(crate) async fn resume_thread( &mut self, config: Config,