mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
codex: harden /loop parsing for PR #16953
This commit is contained in:
Eric Traut
+142
-113
@@ -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<Regex> = 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<Regex> = 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<Regex> = 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<ParsedAlarmSpec, String> {
|
||||
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 <N>s\n",
|
||||
"- @every <N>m\n",
|
||||
"- @every <N>h\n",
|
||||
"- @every <N>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<ParsedAlarmSpec, String> {
|
||||
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<char, String> {
|
||||
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()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<Thread> {
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user