mirror of
https://github.com/openai/codex.git
synced 2026-11-05 16:29:31 +00:00
[core] debounce current-time reminders by elapsed time (#29659)
## Summary - rename `reminder_interval_model_requests` to `reminder_interval_seconds` - read the configured time provider before every model request and inject a reminder only after the configured number of seconds has elapsed - preserve immediate first delivery and forced delivery after compaction changes the context window ## Tests - `just test -p codex-core current_time_reminder`
This commit is contained in:
1 parent
02c326f646
commit
9fe689783d
9 files changed
+60
-38
No files matched your search
@@ -115,7 +115,7 @@ model_provider = "mock_provider"
|
|||||||
|
|
||||||
[features.current_time_reminder]
|
[features.current_time_reminder]
|
||||||
enabled = true
|
enabled = true
|
||||||
reminder_interval_model_requests = 1
|
reminder_interval_seconds = 1
|
||||||
clock_source = "external"
|
clock_source = "external"
|
||||||
|
|
||||||
[model_providers.mock_provider]
|
[model_providers.mock_provider]
|
||||||
|
|||||||
@@ -826,7 +826,7 @@
|
|||||||
"enabled": {
|
"enabled": {
|
||||||
"type": "boolean"
|
"type": "boolean"
|
||||||
},
|
},
|
||||||
"reminder_interval_model_requests": {
|
"reminder_interval_seconds": {
|
||||||
"format": "uint64",
|
"format": "uint64",
|
||||||
"minimum": 1.0,
|
"minimum": 1.0,
|
||||||
"type": "integer"
|
"type": "integer"
|
||||||
|
|||||||
@@ -620,11 +620,11 @@ current_time_reminder = true
|
|||||||
r#"
|
r#"
|
||||||
[features.current_time_reminder]
|
[features.current_time_reminder]
|
||||||
enabled = true
|
enabled = true
|
||||||
reminder_interval_model_requests = 4
|
reminder_interval_seconds = 4
|
||||||
clock_source = "external"
|
clock_source = "external"
|
||||||
"#,
|
"#,
|
||||||
CurrentTimeReminderConfig {
|
CurrentTimeReminderConfig {
|
||||||
reminder_interval_model_requests: 4,
|
reminder_interval_seconds: 4,
|
||||||
clock_source: CurrentTimeSource::External,
|
clock_source: CurrentTimeSource::External,
|
||||||
},
|
},
|
||||||
),
|
),
|
||||||
@@ -642,7 +642,7 @@ async fn load_config_rejects_zero_current_time_reminder_interval() -> std::io::R
|
|||||||
r#"
|
r#"
|
||||||
[features.current_time_reminder]
|
[features.current_time_reminder]
|
||||||
enabled = true
|
enabled = true
|
||||||
reminder_interval_model_requests = 0
|
reminder_interval_seconds = 0
|
||||||
"#,
|
"#,
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
@@ -651,7 +651,7 @@ reminder_interval_model_requests = 0
|
|||||||
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
|
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
error.to_string(),
|
error.to_string(),
|
||||||
"features.current_time_reminder.reminder_interval_model_requests must be positive"
|
"features.current_time_reminder.reminder_interval_seconds must be positive"
|
||||||
);
|
);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1115,14 +1115,14 @@ pub struct RolloutBudgetConfig {
|
|||||||
|
|
||||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
|
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
|
||||||
pub struct CurrentTimeReminderConfig {
|
pub struct CurrentTimeReminderConfig {
|
||||||
pub reminder_interval_model_requests: u64,
|
pub reminder_interval_seconds: u64,
|
||||||
pub clock_source: CurrentTimeSource,
|
pub clock_source: CurrentTimeSource,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Default for CurrentTimeReminderConfig {
|
impl Default for CurrentTimeReminderConfig {
|
||||||
fn default() -> Self {
|
fn default() -> Self {
|
||||||
Self {
|
Self {
|
||||||
reminder_interval_model_requests: 1,
|
reminder_interval_seconds: 1,
|
||||||
clock_source: CurrentTimeSource::System,
|
clock_source: CurrentTimeSource::System,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -2657,18 +2657,18 @@ fn resolve_current_time_reminder_config(
|
|||||||
|
|
||||||
let base = current_time_reminder_toml_config(config_toml.features.as_ref());
|
let base = current_time_reminder_toml_config(config_toml.features.as_ref());
|
||||||
let default = CurrentTimeReminderConfig::default();
|
let default = CurrentTimeReminderConfig::default();
|
||||||
let reminder_interval_model_requests = base
|
let reminder_interval_seconds = base
|
||||||
.and_then(|config| config.reminder_interval_model_requests)
|
.and_then(|config| config.reminder_interval_seconds)
|
||||||
.unwrap_or(default.reminder_interval_model_requests);
|
.unwrap_or(default.reminder_interval_seconds);
|
||||||
if reminder_interval_model_requests == 0 {
|
if reminder_interval_seconds == 0 {
|
||||||
return Err(std::io::Error::new(
|
return Err(std::io::Error::new(
|
||||||
std::io::ErrorKind::InvalidInput,
|
std::io::ErrorKind::InvalidInput,
|
||||||
"features.current_time_reminder.reminder_interval_model_requests must be positive",
|
"features.current_time_reminder.reminder_interval_seconds must be positive",
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(Some(CurrentTimeReminderConfig {
|
Ok(Some(CurrentTimeReminderConfig {
|
||||||
reminder_interval_model_requests,
|
reminder_interval_seconds,
|
||||||
clock_source: base
|
clock_source: base
|
||||||
.and_then(|config| config.clock_source)
|
.and_then(|config| config.clock_source)
|
||||||
.unwrap_or(default.clock_source),
|
.unwrap_or(default.clock_source),
|
||||||
|
|||||||
@@ -357,7 +357,7 @@ mod tests {
|
|||||||
features.current_time_reminder,
|
features.current_time_reminder,
|
||||||
Some(FeatureToml::Config(CurrentTimeReminderConfigToml {
|
Some(FeatureToml::Config(CurrentTimeReminderConfigToml {
|
||||||
enabled: Some(true),
|
enabled: Some(true),
|
||||||
reminder_interval_model_requests: Some(1),
|
reminder_interval_seconds: Some(1),
|
||||||
clock_source: Some(codex_features::CurrentTimeSource::System),
|
clock_source: Some(codex_features::CurrentTimeSource::System),
|
||||||
}))
|
}))
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -1,3 +1,5 @@
|
|||||||
|
use chrono::DateTime;
|
||||||
|
use chrono::Utc;
|
||||||
use codex_protocol::error::CodexErr;
|
use codex_protocol::error::CodexErr;
|
||||||
use codex_protocol::error::Result as CodexResult;
|
use codex_protocol::error::Result as CodexResult;
|
||||||
|
|
||||||
@@ -7,18 +9,27 @@ use crate::context::ContextualUserFragment;
|
|||||||
|
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
pub(crate) struct CurrentTimeReminderState {
|
pub(crate) struct CurrentTimeReminderState {
|
||||||
model_requests_since_delivery: u64,
|
last_delivery_time: Option<DateTime<Utc>>,
|
||||||
last_window_id: Option<String>,
|
last_window_id: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl CurrentTimeReminderState {
|
impl CurrentTimeReminderState {
|
||||||
fn take_reminder_due(&mut self, window_id: &str, interval: u64) -> bool {
|
fn take_reminder_due(
|
||||||
self.model_requests_since_delivery = self.model_requests_since_delivery.saturating_add(1);
|
&mut self,
|
||||||
|
window_id: &str,
|
||||||
|
current_time: DateTime<Utc>,
|
||||||
|
interval_seconds: u64,
|
||||||
|
) -> bool {
|
||||||
let reminder_is_due = self.last_window_id.as_deref() != Some(window_id)
|
let reminder_is_due = self.last_window_id.as_deref() != Some(window_id)
|
||||||
|| self.model_requests_since_delivery >= interval;
|
|| self.last_delivery_time.is_none_or(|last_delivery_time| {
|
||||||
|
current_time
|
||||||
|
.signed_duration_since(last_delivery_time)
|
||||||
|
.num_seconds()
|
||||||
|
>= i64::try_from(interval_seconds).unwrap_or(i64::MAX)
|
||||||
|
});
|
||||||
|
|
||||||
if reminder_is_due {
|
if reminder_is_due {
|
||||||
self.model_requests_since_delivery = 0;
|
self.last_delivery_time = Some(current_time);
|
||||||
self.last_window_id = Some(window_id.to_string());
|
self.last_window_id = Some(window_id.to_string());
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -35,16 +46,6 @@ pub(super) async fn maybe_record_current_time_reminder(
|
|||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|
||||||
let reminder_is_due = {
|
|
||||||
let mut state = sess.state.lock().await;
|
|
||||||
state
|
|
||||||
.current_time_reminder
|
|
||||||
.take_reminder_due(window_id, config.reminder_interval_model_requests)
|
|
||||||
};
|
|
||||||
if !reminder_is_due {
|
|
||||||
return Ok(());
|
|
||||||
}
|
|
||||||
|
|
||||||
let current_time = sess
|
let current_time = sess
|
||||||
.services
|
.services
|
||||||
.time_provider
|
.time_provider
|
||||||
@@ -52,6 +53,18 @@ pub(super) async fn maybe_record_current_time_reminder(
|
|||||||
.await
|
.await
|
||||||
.map_err(|err| CodexErr::Fatal(format!("failed to read current time: {err:#}")))?;
|
.map_err(|err| CodexErr::Fatal(format!("failed to read current time: {err:#}")))?;
|
||||||
|
|
||||||
|
let reminder_is_due = {
|
||||||
|
let mut state = sess.state.lock().await;
|
||||||
|
state.current_time_reminder.take_reminder_due(
|
||||||
|
window_id,
|
||||||
|
current_time,
|
||||||
|
config.reminder_interval_seconds,
|
||||||
|
)
|
||||||
|
};
|
||||||
|
if !reminder_is_due {
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
|
||||||
let response_item =
|
let response_item =
|
||||||
ContextualUserFragment::into(crate::context::CurrentTimeReminder::new(current_time));
|
ContextualUserFragment::into(crate::context::CurrentTimeReminder::new(current_time));
|
||||||
sess.record_conversation_items(turn_context, std::slice::from_ref(&response_item))
|
sess.record_conversation_items(turn_context, std::slice::from_ref(&response_item))
|
||||||
|
|||||||
@@ -881,7 +881,7 @@ text(JSON.stringify(result));
|
|||||||
.enable(Feature::CurrentTimeReminder)
|
.enable(Feature::CurrentTimeReminder)
|
||||||
.expect("test config should allow current-time reminders");
|
.expect("test config should allow current-time reminders");
|
||||||
config.current_time_reminder = Some(CurrentTimeReminderConfig {
|
config.current_time_reminder = Some(CurrentTimeReminderConfig {
|
||||||
reminder_interval_model_requests: 50,
|
reminder_interval_seconds: 3_000,
|
||||||
clock_source: CurrentTimeSource::System,
|
clock_source: CurrentTimeSource::System,
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ use serde_json::json;
|
|||||||
|
|
||||||
const FIRST_REMINDER: &str = "It is 2026-06-17 17:34:15 UTC.";
|
const FIRST_REMINDER: &str = "It is 2026-06-17 17:34:15 UTC.";
|
||||||
const SECOND_REMINDER: &str = "It is 2026-06-17 17:35:15 UTC.";
|
const SECOND_REMINDER: &str = "It is 2026-06-17 17:35:15 UTC.";
|
||||||
|
const THIRD_REMINDER: &str = "It is 2026-06-17 17:36:15 UTC.";
|
||||||
const FIRST_TIME_UNIX_SECONDS: i64 = 1_781_717_655;
|
const FIRST_TIME_UNIX_SECONDS: i64 = 1_781_717_655;
|
||||||
|
|
||||||
struct TestTimeProvider(AtomicI64);
|
struct TestTimeProvider(AtomicI64);
|
||||||
@@ -83,13 +84,13 @@ fn enable_current_time_reminder(
|
|||||||
.enable(Feature::CurrentTimeReminder)
|
.enable(Feature::CurrentTimeReminder)
|
||||||
.expect("test config should allow current-time reminders");
|
.expect("test config should allow current-time reminders");
|
||||||
config.current_time_reminder = Some(CurrentTimeReminderConfig {
|
config.current_time_reminder = Some(CurrentTimeReminderConfig {
|
||||||
reminder_interval_model_requests: interval,
|
reminder_interval_seconds: interval,
|
||||||
clock_source,
|
clock_source,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn current_time_reminders_follow_request_interval_and_persist_in_history() -> Result<()> {
|
async fn current_time_reminders_follow_time_interval_and_persist_in_history() -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
let server = start_mock_server().await;
|
let server = start_mock_server().await;
|
||||||
@@ -120,7 +121,7 @@ async fn current_time_reminders_follow_request_interval_and_persist_in_history()
|
|||||||
.await;
|
.await;
|
||||||
let test = test_codex()
|
let test = test_codex()
|
||||||
.with_config(|config| {
|
.with_config(|config| {
|
||||||
enable_current_time_reminder(config, /*interval*/ 2, CurrentTimeSource::External)
|
enable_current_time_reminder(config, /*interval*/ 120, CurrentTimeSource::External)
|
||||||
})
|
})
|
||||||
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
||||||
.build(&server)
|
.build(&server)
|
||||||
@@ -136,7 +137,7 @@ async fn current_time_reminders_follow_request_interval_and_persist_in_history()
|
|||||||
assert_eq!(current_time_reminders(&requests[1]), vec![FIRST_REMINDER]);
|
assert_eq!(current_time_reminders(&requests[1]), vec![FIRST_REMINDER]);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
current_time_reminders(&requests[2]),
|
current_time_reminders(&requests[2]),
|
||||||
vec![FIRST_REMINDER, SECOND_REMINDER]
|
vec![FIRST_REMINDER, THIRD_REMINDER]
|
||||||
);
|
);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
@@ -195,7 +196,11 @@ async fn current_time_reminder_is_refreshed_after_compaction() -> Result<()> {
|
|||||||
let test = test_codex()
|
let test = test_codex()
|
||||||
.with_config(move |config| {
|
.with_config(move |config| {
|
||||||
config.model_provider = model_provider;
|
config.model_provider = model_provider;
|
||||||
enable_current_time_reminder(config, /*interval*/ 50, CurrentTimeSource::External);
|
enable_current_time_reminder(
|
||||||
|
config,
|
||||||
|
/*interval*/ 3_000,
|
||||||
|
CurrentTimeSource::External,
|
||||||
|
);
|
||||||
})
|
})
|
||||||
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
||||||
.build(&server)
|
.build(&server)
|
||||||
@@ -295,7 +300,11 @@ async fn current_time_tool_returns_the_latest_time() -> Result<()> {
|
|||||||
.await;
|
.await;
|
||||||
let test = test_codex()
|
let test = test_codex()
|
||||||
.with_config(|config| {
|
.with_config(|config| {
|
||||||
enable_current_time_reminder(config, /*interval*/ 50, CurrentTimeSource::External)
|
enable_current_time_reminder(
|
||||||
|
config,
|
||||||
|
/*interval*/ 3_000,
|
||||||
|
CurrentTimeSource::External,
|
||||||
|
)
|
||||||
})
|
})
|
||||||
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
.with_external_time_provider(Arc::new(TestTimeProvider::default()))
|
||||||
.build(&server)
|
.build(&server)
|
||||||
|
|||||||
@@ -144,7 +144,7 @@ pub struct CurrentTimeReminderConfigToml {
|
|||||||
pub enabled: Option<bool>,
|
pub enabled: Option<bool>,
|
||||||
#[serde(skip_serializing_if = "Option::is_none")]
|
#[serde(skip_serializing_if = "Option::is_none")]
|
||||||
#[schemars(range(min = 1))]
|
#[schemars(range(min = 1))]
|
||||||
pub reminder_interval_model_requests: Option<u64>,
|
pub reminder_interval_seconds: Option<u64>,
|
||||||
#[serde(skip_serializing_if = "Option::is_none")]
|
#[serde(skip_serializing_if = "Option::is_none")]
|
||||||
pub clock_source: Option<CurrentTimeSource>,
|
pub clock_source: Option<CurrentTimeSource>,
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user