From b568babc8f0237b9aaebe9cc428a1bd77f671e07 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 6 Apr 2026 22:27:09 -0700 Subject: [PATCH] Default loop alarms to idle recurrence --- codex-rs/app-server/README.md | 2 +- codex-rs/core/src/alarm_trigger.rs | 58 ++++++++++++++++------ codex-rs/core/src/alarms.rs | 77 ++++++++++++++++++++++++++++- codex-rs/tools/src/alarm_tool.rs | 5 +- codex-rs/tui/src/alarm_scheduler.rs | 19 +++++-- 5 files changed, 141 insertions(+), 20 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 585a087b5c..aa1c7258d2 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -145,7 +145,7 @@ Example with notification opt-out: - `thread/unsubscribe` — unsubscribe this connection from thread turn/item events. If this was the last subscriber, the server shuts down and unloads the thread, then emits `thread/closed`. - `thread/name/set` — set or update a thread’s user-facing name for either a loaded thread or a persisted rollout; returns `{}` on success and emits `thread/name/updated` to initialized, opted-in clients. Thread names are not required to be unique; name lookups resolve to the most recently updated thread. - `thread/unarchive` — move an archived rollout file back into the sessions directory; returns the restored `thread` on success and emits `thread/unarchived`. -- `thread/alarm/create` — create a runtime alarm on a loaded thread; accepts a structured `trigger`, `prompt`, and `delivery`, returns the created alarm, and requires the under-development feature flag `alarm_tool` to be enabled for that thread. Delay triggers use `seconds` plus optional `repeat`; schedule triggers use optional `dtstart` and/or `rrule`. +- `thread/alarm/create` — create a runtime alarm on a loaded thread; accepts a structured `trigger`, `prompt`, and `delivery`, returns the created alarm, and requires the under-development feature flag `alarm_tool` to be enabled for that thread. Delay triggers use `seconds` plus optional `repeat`; `seconds: 0, repeat: true` means run whenever the thread is idle. Schedule triggers use optional `dtstart` and/or `rrule`. - `thread/alarm/delete` — delete one runtime alarm from a loaded thread by id; returns `{ deleted }` and requires `alarm_tool`. - `thread/alarm/list` — list runtime alarms for a loaded thread; returns `{ data }` and requires `alarm_tool`. - `thread/alarm/updated` — notification emitted when a loaded thread’s alarm list changes, including one-shot alarms deleting themselves after execution or alarms restored from disk. diff --git a/codex-rs/core/src/alarm_trigger.rs b/codex-rs/core/src/alarm_trigger.rs index 34ea9f9e7b..76da82797f 100644 --- a/codex-rs/core/src/alarm_trigger.rs +++ b/codex-rs/core/src/alarm_trigger.rs @@ -48,6 +48,16 @@ impl AlarmTrigger { } } + pub(crate) fn is_idle_recurring(&self) -> bool { + matches!( + self, + Self::Delay { + seconds: 0, + repeat: Some(true), + } + ) + } + pub(crate) fn display(&self) -> String { match self { Self::Delay { seconds, repeat } => { @@ -115,7 +125,7 @@ fn timing_for_new_trigger_with_timezone( AlarmTrigger::Delay { seconds, repeat } => { let repeat = repeat.unwrap_or(false); if repeat && *seconds == 0 { - return Err("delay.repeat requires seconds to be greater than 0".to_string()); + return Ok(timing(normalized, /*pending_run*/ true, None, now)); } let next_run_at = checked_add_seconds(created_at, *seconds)?; let pending_run = next_run_at <= now; @@ -170,7 +180,7 @@ fn timing_for_restored_trigger_with_timezone( AlarmTrigger::Delay { seconds, repeat } => { let repeat = repeat.unwrap_or(false); if repeat && *seconds == 0 { - return Err("delay.repeat requires seconds to be greater than 0".to_string()); + return Ok(timing(normalized, /*pending_run*/ true, None, now)); } let next_run_at = persisted_next_run_at .or_else(|| next_delay_run_at(created_at, *seconds)) @@ -221,6 +231,9 @@ fn next_run_after_due_with_timezone( match trigger { AlarmTrigger::Delay { seconds, repeat } => { if repeat.unwrap_or(false) { + if *seconds == 0 { + return Ok(None); + } next_delay_recurring_run_at_from_timestamp(created_at, *seconds, now) } else { Ok(None) @@ -239,12 +252,7 @@ fn normalize_trigger( timezone: Tz, ) -> Result { match trigger { - AlarmTrigger::Delay { seconds, repeat } => { - if repeat.unwrap_or(false) && seconds == 0 { - return Err("delay.repeat requires seconds to be greater than 0".to_string()); - } - Ok(AlarmTrigger::Delay { seconds, repeat }) - } + AlarmTrigger::Delay { seconds, repeat } => Ok(AlarmTrigger::Delay { seconds, repeat }), AlarmTrigger::Schedule { dtstart, rrule } => { let dtstart = normalize_optional_string(dtstart); let rrule = normalize_optional_string(rrule); @@ -480,19 +488,41 @@ mod tests { } #[test] - fn delay_repeat_rejects_zero_seconds() { + fn delay_repeat_zero_is_idle_recurring() { + let timing = timing_for_new_trigger_with_timezone( + AlarmTrigger::Delay { + seconds: 0, + repeat: Some(true), + }, + utc(TS_100), + utc(TS_100), + Tz::UTC, + ) + .expect("zero repeat should be a valid idle-recurring trigger"); assert_eq!( - timing_for_new_trigger_with_timezone( - AlarmTrigger::Delay { + timing, + TriggerTiming { + trigger: AlarmTrigger::Delay { seconds: 0, repeat: Some(true), }, - utc(TS_100), + pending_run: true, + next_run_at: None, + timer_delay: None, + } + ); + assert_eq!( + next_run_after_due_with_timezone( + &AlarmTrigger::Delay { + seconds: 0, + repeat: Some(true), + }, + /*created_at*/ 100, utc(TS_100), Tz::UTC, ) - .expect_err("zero repeat should be invalid"), - "delay.repeat requires seconds to be greater than 0" + .expect("zero repeat should remain timer-free"), + None ); } diff --git a/codex-rs/core/src/alarms.rs b/codex-rs/core/src/alarms.rs index 34d38ba760..cde9f79878 100644 --- a/codex-rs/core/src/alarms.rs +++ b/codex-rs/core/src/alarms.rs @@ -291,6 +291,12 @@ impl AlarmsState { .values() .filter(|runtime| runtime.pending_run) .filter_map(|runtime| { + if runtime.alarm.trigger.is_idle_recurring() { + if can_after_turn { + return Some((runtime, AlarmDelivery::AfterTurn)); + } + return None; + } let actual_delivery = match runtime.alarm.delivery { AlarmDelivery::AfterTurn if can_after_turn => AlarmDelivery::AfterTurn, AlarmDelivery::AfterTurn => return None, @@ -326,11 +332,12 @@ impl AlarmsState { } } else { alarm.last_run_at = Some(now.timestamp()); + let pending_run = alarm.trigger.is_idle_recurring(); self.alarms.insert( alarm.id.clone(), AlarmRuntime { alarm: alarm.clone(), - pending_run: false, + pending_run, timer_cancel, }, ); @@ -486,6 +493,7 @@ mod tests { use super::CreateAlarm; use super::MAX_ACTIVE_ALARMS_PER_THREAD; use super::PersistedAlarm; + use super::ThreadAlarm; use super::alarm_prompt_input_item; use super::alarm_sidecar_path_for_rollout; use super::load_alarm_sidecar; @@ -588,6 +596,73 @@ mod tests { assert_eq!(second.context.current_alarm_id, "alarm-2"); } + #[test] + fn idle_recurring_alarm_remains_pending_after_claim() { + let now = Utc.timestamp_opt(100, 0).single().expect("valid timestamp"); + let mut alarms = AlarmsState::default(); + let (alarm, timer_spec) = alarms + .create_alarm( + CreateAlarm { + id: "alarm-1".to_string(), + trigger: delay(ZERO_SECONDS, Some(true)), + prompt: "keep going".to_string(), + delivery: AlarmDelivery::AfterTurn, + now, + }, + /*timer_cancel*/ None, + ) + .expect("alarm should be created"); + assert_eq!(timer_spec, None); + + let claimed = alarms + .claim_next_alarm( + now, /*can_after_turn*/ true, /*can_steer_current_turn*/ true, + ) + .expect("alarm should be claimed"); + assert!(!claimed.deleted_one_shot_alarm); + assert_eq!( + alarms.persisted_alarms(), + vec![PersistedAlarm { + alarm: ThreadAlarm { + last_run_at: Some(100), + ..alarm + }, + pending_run: true, + }] + ); + } + + #[test] + fn idle_recurring_alarm_waits_for_idle_even_if_delivery_requests_steer() { + let now = Utc.timestamp_opt(100, 0).single().expect("valid timestamp"); + let mut alarms = AlarmsState::default(); + alarms + .create_alarm( + CreateAlarm { + id: "alarm-1".to_string(), + trigger: delay(ZERO_SECONDS, Some(true)), + prompt: "keep going".to_string(), + delivery: AlarmDelivery::SteerCurrentTurn, + now, + }, + /*timer_cancel*/ None, + ) + .expect("alarm should be created"); + + assert_eq!( + alarms.claim_next_alarm( + now, /*can_after_turn*/ false, /*can_steer_current_turn*/ true, + ), + None + ); + let claimed = alarms + .claim_next_alarm( + now, /*can_after_turn*/ true, /*can_steer_current_turn*/ false, + ) + .expect("alarm should be claimed when idle"); + assert_eq!(claimed.context.delivery, AlarmDelivery::AfterTurn); + } + #[test] fn create_alarm_rejects_more_than_maximum_active_alarms() { let now = Utc.timestamp_opt(100, 0).single().expect("valid timestamp"); diff --git a/codex-rs/tools/src/alarm_tool.rs b/codex-rs/tools/src/alarm_tool.rs index 4dd24a5b68..c01a9df0f1 100644 --- a/codex-rs/tools/src/alarm_tool.rs +++ b/codex-rs/tools/src/alarm_tool.rs @@ -26,7 +26,10 @@ pub fn create_alarm_create_tool() -> ToolSpec { ( "repeat".to_string(), JsonSchema::Boolean { - description: Some("Delay trigger recurrence flag.".to_string()), + description: Some( + "Delay trigger recurrence flag. With seconds 0, repeat means run whenever the thread is idle." + .to_string(), + ), }, ), ( diff --git a/codex-rs/tui/src/alarm_scheduler.rs b/codex-rs/tui/src/alarm_scheduler.rs index 783858a4a5..3c6a5caae9 100644 --- a/codex-rs/tui/src/alarm_scheduler.rs +++ b/codex-rs/tui/src/alarm_scheduler.rs @@ -275,12 +275,13 @@ Return only the JSON object requested by the response schema. Rules: - Extract the alarm prompt by removing the scheduling phrase but preserving the user's requested task. - Use delivery "after-turn" unless the user clearly asks for same-turn/current-turn steering; then use "steer-current-turn". -- For "now", "immediately", or specs with no explicit timing, use { "kind": "delay", "seconds": 0, "repeat": false }. +- Treat `/loop` as recurring by default. Only create a one-shot alarm when the user clearly asks for a single run, one-time reminder, or other one-off action. +- For "now", "immediately", or specs with no explicit timing, use { "kind": "delay", "seconds": 0, "repeat": true } unless the user clearly asked for one-shot behavior. This means the alarm fires whenever the thread is idle. - For delay triggers, set dtstart and rrule to null. - For schedule triggers, set seconds and repeat to null. -- For relative one-shot timing like "in 30 seconds", use a delay trigger with seconds set to the relative delay and repeat false. +- For relative timing like "in 30 seconds", use a delay trigger with seconds set to the relative delay and repeat true unless the user clearly asked for one-shot behavior. - For interval timing like "every 5 minutes", use a delay trigger with seconds set to the interval and repeat true. -- For one-shot wall-clock timing like "at 9pm", use a schedule trigger with dtstart set to the next matching local datetime in YYYY-MM-DDTHH:MM:SS and rrule null. +- For one-shot wall-clock timing like "at 9pm once" or "one time at 9pm", use a schedule trigger with dtstart set to the next matching local datetime in YYYY-MM-DDTHH:MM:SS and rrule null. - For recurring calendar timing, use a schedule trigger with rrule set to an RFC 5545 RRULE string and dtstart set when the user supplies a start datetime; otherwise null. - For schedule triggers, use floating local wall-clock datetimes without timezone suffixes. "#; @@ -316,6 +317,18 @@ mod tests { ); } + #[test] + fn parser_prompt_defaults_ambiguous_loop_to_idle_recurring() { + assert!( + PARSE_ALARM_SYSTEM_PROMPT + .contains(r#"{ "kind": "delay", "seconds": 0, "repeat": true }"#) + ); + assert!( + PARSE_ALARM_SYSTEM_PROMPT + .contains("Only create a one-shot alarm when the user clearly asks") + ); + } + #[test] fn parsed_alarm_spec_accepts_permissive_delay_trigger_shape() { let parsed: ParsedAlarmSpec = serde_json::from_value(json!({