mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Enable clock tools for persistent reasoning turns (#40942)
## What changed - Default the current-time reminder and interruptible `clock.sleep` tool on for turns using persistent reasoning effort. - Preserve explicit `features.current_time_reminder` settings and managed requirements, and gate reminder recording on the resolved feature state. ## Testing - Cover configured, persistent, ordinary-effort, explicitly disabled, and managed-disabled clock setups. - Verify review turns do not expose clock tools or current-time reminders. GitOrigin-RevId: 4ff8805807ceeef374abd962b1d9e7c37f1fbd83
This commit is contained in:
1 parent
e0c727de04
commit
0e9a2bae52
4 files changed
+114
-10
No files matched your search
@@ -1,15 +1,41 @@
|
|||||||
use chrono::DateTime;
|
use chrono::DateTime;
|
||||||
use chrono::Utc;
|
use chrono::Utc;
|
||||||
use codex_features::CurrentTimeReminderDeliveryMode;
|
use codex_features::CurrentTimeReminderDeliveryMode;
|
||||||
|
use codex_features::Feature;
|
||||||
use codex_protocol::error::CodexErr;
|
use codex_protocol::error::CodexErr;
|
||||||
use codex_protocol::error::Result as CodexResult;
|
use codex_protocol::error::Result as CodexResult;
|
||||||
use codex_protocol::models::ResponseItem;
|
use codex_protocol::models::ResponseItem;
|
||||||
|
|
||||||
use super::session::Session;
|
use super::session::Session;
|
||||||
use super::turn_context::TurnContext;
|
use super::turn_context::TurnContext;
|
||||||
|
use crate::config::Config;
|
||||||
|
use crate::config::CurrentTimeReminderConfig;
|
||||||
use crate::context::ContextualUserFragment;
|
use crate::context::ContextualUserFragment;
|
||||||
use crate::context_manager::is_user_turn_boundary;
|
use crate::context_manager::is_user_turn_boundary;
|
||||||
|
|
||||||
|
pub(super) fn apply_persistent_defaults(config: &mut Config) {
|
||||||
|
if config.current_time_reminder.is_some()
|
||||||
|
|| config
|
||||||
|
.config_layer_stack
|
||||||
|
.effective_config()
|
||||||
|
.get("features")
|
||||||
|
.and_then(|features| features.get("current_time_reminder"))
|
||||||
|
.is_some()
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Apply defaults only to this turn; explicit settings and managed policy win.
|
||||||
|
if config.features.enable(Feature::CurrentTimeReminder).is_ok()
|
||||||
|
&& config.features.enabled(Feature::CurrentTimeReminder)
|
||||||
|
{
|
||||||
|
config.current_time_reminder = Some(CurrentTimeReminderConfig {
|
||||||
|
sleep_tool: true,
|
||||||
|
..CurrentTimeReminderConfig::default()
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
pub(crate) struct CurrentTimeReminderState {
|
pub(crate) struct CurrentTimeReminderState {
|
||||||
last_delivery_time: Option<DateTime<Utc>>,
|
last_delivery_time: Option<DateTime<Utc>>,
|
||||||
@@ -73,6 +99,13 @@ pub(super) async fn maybe_record_current_time_reminder(
|
|||||||
turn_context: &TurnContext,
|
turn_context: &TurnContext,
|
||||||
window_id: &str,
|
window_id: &str,
|
||||||
) -> CodexResult<()> {
|
) -> CodexResult<()> {
|
||||||
|
if !turn_context
|
||||||
|
.config
|
||||||
|
.features
|
||||||
|
.enabled(Feature::CurrentTimeReminder)
|
||||||
|
{
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
let Some(config) = turn_context.config.current_time_reminder else {
|
let Some(config) = turn_context.config.current_time_reminder else {
|
||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ use codex_protocol::config_types::ShellEnvironmentPolicy;
|
|||||||
use codex_protocol::models::AdditionalPermissionProfile;
|
use codex_protocol::models::AdditionalPermissionProfile;
|
||||||
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
|
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
|
||||||
use codex_protocol::openai_models::ModelInfo;
|
use codex_protocol::openai_models::ModelInfo;
|
||||||
|
use codex_protocol::openai_models::ReasoningEffort;
|
||||||
use codex_protocol::permissions::RawFileSystemSandboxPolicy;
|
use codex_protocol::permissions::RawFileSystemSandboxPolicy;
|
||||||
use codex_protocol::protocol::EnvironmentConfig;
|
use codex_protocol::protocol::EnvironmentConfig;
|
||||||
use codex_protocol::protocol::EnvironmentConfigState;
|
use codex_protocol::protocol::EnvironmentConfigState;
|
||||||
@@ -725,6 +726,9 @@ impl Session {
|
|||||||
|
|
||||||
let mut per_turn_config = per_turn_config;
|
let mut per_turn_config = per_turn_config;
|
||||||
super::token_budget::apply_model_defaults(&mut per_turn_config, model_info);
|
super::token_budget::apply_model_defaults(&mut per_turn_config, model_info);
|
||||||
|
if step_settings.reasoning_effort() == Some(&ReasoningEffort::Persistent) {
|
||||||
|
super::time_reminder::apply_persistent_defaults(&mut per_turn_config);
|
||||||
|
}
|
||||||
per_turn_config.service_tier = step_settings.service_tier.clone();
|
per_turn_config.service_tier = step_settings.service_tier.clone();
|
||||||
let permission_profile = environments.permission_profile_or_else(|| {
|
let permission_profile = environments.permission_profile_or_else(|| {
|
||||||
per_turn_config.permissions.effective_permission_profile()
|
per_turn_config.permissions.effective_permission_profile()
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ use anyhow::anyhow;
|
|||||||
use chrono::DateTime;
|
use chrono::DateTime;
|
||||||
use chrono::Local;
|
use chrono::Local;
|
||||||
use chrono::Utc;
|
use chrono::Utc;
|
||||||
|
use codex_config::test_support::CloudConfigBundleFixture;
|
||||||
use codex_core::SleepFuture;
|
use codex_core::SleepFuture;
|
||||||
use codex_core::TimeFuture;
|
use codex_core::TimeFuture;
|
||||||
use codex_core::TimeProvider;
|
use codex_core::TimeProvider;
|
||||||
@@ -20,6 +21,7 @@ use codex_features::Feature;
|
|||||||
use codex_model_provider_info::built_in_model_providers;
|
use codex_model_provider_info::built_in_model_providers;
|
||||||
use codex_protocol::ThreadId;
|
use codex_protocol::ThreadId;
|
||||||
use codex_protocol::models::PermissionProfile;
|
use codex_protocol::models::PermissionProfile;
|
||||||
|
use codex_protocol::openai_models::ReasoningEffort;
|
||||||
use codex_protocol::protocol::CodexErrorInfo;
|
use codex_protocol::protocol::CodexErrorInfo;
|
||||||
use codex_protocol::protocol::EventMsg;
|
use codex_protocol::protocol::EventMsg;
|
||||||
use codex_protocol::protocol::Op;
|
use codex_protocol::protocol::Op;
|
||||||
@@ -40,6 +42,7 @@ use core_test_support::test_codex::test_codex;
|
|||||||
use core_test_support::wait_for_event;
|
use core_test_support::wait_for_event;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use serde_json::json;
|
use serde_json::json;
|
||||||
|
use test_case::test_case;
|
||||||
|
|
||||||
const FIRST_REMINDER: &str =
|
const FIRST_REMINDER: &str =
|
||||||
"<current_time_reminder>It is 2026-06-17 17:34:15 UTC.</current_time_reminder>";
|
"<current_time_reminder>It is 2026-06-17 17:34:15 UTC.</current_time_reminder>";
|
||||||
@@ -51,6 +54,15 @@ const THIRD_REMINDER: &str =
|
|||||||
"<current_time_reminder>It is 2026-06-17 17:36:15 UTC.</current_time_reminder>";
|
"<current_time_reminder>It is 2026-06-17 17:36:15 UTC.</current_time_reminder>";
|
||||||
const FIRST_TIME_UNIX_SECONDS: i64 = 1_781_717_655;
|
const FIRST_TIME_UNIX_SECONDS: i64 = 1_781_717_655;
|
||||||
|
|
||||||
|
#[derive(Clone, Copy)]
|
||||||
|
enum ClockSetup {
|
||||||
|
Configured,
|
||||||
|
Persistent,
|
||||||
|
OrdinaryEffort,
|
||||||
|
ExplicitlyDisabled,
|
||||||
|
RequiredOff,
|
||||||
|
}
|
||||||
|
|
||||||
struct TestTimeProvider {
|
struct TestTimeProvider {
|
||||||
current_time: AtomicI64,
|
current_time: AtomicI64,
|
||||||
sleep_seconds: AtomicU64,
|
sleep_seconds: AtomicU64,
|
||||||
@@ -323,8 +335,13 @@ async fn current_time_reminders_can_follow_only_user_or_tool_outputs() -> Result
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test_case(ClockSetup::Configured; "configured")]
|
||||||
|
#[test_case(ClockSetup::Persistent; "persistent")]
|
||||||
|
#[test_case(ClockSetup::OrdinaryEffort; "ordinary_effort")]
|
||||||
|
#[test_case(ClockSetup::ExplicitlyDisabled; "explicitly_disabled")]
|
||||||
|
#[test_case(ClockSetup::RequiredOff; "required_off")]
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn system_time_source_adds_current_time_reminder() -> Result<()> {
|
async fn system_time_source_adds_current_time_reminder(clock_setup: ClockSetup) -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
let server = start_mock_server().await;
|
let server = start_mock_server().await;
|
||||||
@@ -333,16 +350,61 @@ async fn system_time_source_adds_current_time_reminder() -> Result<()> {
|
|||||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
let test = test_codex()
|
let mut builder = test_codex()
|
||||||
.with_config(|config| {
|
.with_pre_build_hook(move |home| {
|
||||||
enable_current_time_reminder(config, /*interval*/ 1, CurrentTimeSource::System)
|
let config = match clock_setup {
|
||||||
|
ClockSetup::Configured => Some("[features]\ncurrent_time_reminder = true\n"),
|
||||||
|
ClockSetup::ExplicitlyDisabled => {
|
||||||
|
Some("[features]\ncurrent_time_reminder = false\n")
|
||||||
|
}
|
||||||
|
ClockSetup::Persistent | ClockSetup::OrdinaryEffort | ClockSetup::RequiredOff => {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
};
|
||||||
|
if let Some(config) = config {
|
||||||
|
std::fs::write(home.join("config.toml"), config)
|
||||||
|
.expect("clock configuration should be written");
|
||||||
|
}
|
||||||
})
|
})
|
||||||
.build(&server)
|
.with_config(move |config| {
|
||||||
.await?;
|
config.include_environment_context = false;
|
||||||
|
config.model_reasoning_effort = Some(match clock_setup {
|
||||||
|
ClockSetup::OrdinaryEffort => ReasoningEffort::High,
|
||||||
|
ClockSetup::Configured
|
||||||
|
| ClockSetup::Persistent
|
||||||
|
| ClockSetup::ExplicitlyDisabled
|
||||||
|
| ClockSetup::RequiredOff => ReasoningEffort::Persistent,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
if matches!(clock_setup, ClockSetup::RequiredOff) {
|
||||||
|
builder = builder.with_cloud_config_bundle(
|
||||||
|
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||||
|
"[features]\ncurrent_time_reminder = false\n",
|
||||||
|
),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
let test = builder.build_with_auto_env(&server).await?;
|
||||||
|
|
||||||
test.submit_turn("what time is it?").await?;
|
test.submit_text_turn("what time is it?").await?;
|
||||||
|
|
||||||
let request = responses.single_request();
|
let request = responses.single_request();
|
||||||
|
assert_eq!(
|
||||||
|
["curr_time", "sleep"].map(|name| request.tool_by_name("clock", name).is_some()),
|
||||||
|
match clock_setup {
|
||||||
|
ClockSetup::Configured => [true, false],
|
||||||
|
ClockSetup::Persistent => [true, true],
|
||||||
|
ClockSetup::OrdinaryEffort
|
||||||
|
| ClockSetup::ExplicitlyDisabled
|
||||||
|
| ClockSetup::RequiredOff => [false, false],
|
||||||
|
}
|
||||||
|
);
|
||||||
|
if matches!(
|
||||||
|
clock_setup,
|
||||||
|
ClockSetup::OrdinaryEffort | ClockSetup::ExplicitlyDisabled | ClockSetup::RequiredOff
|
||||||
|
) {
|
||||||
|
assert!(current_time_reminders(&request).is_empty());
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
assert!(request.has_content_kinds(&["current_time.reminder"]));
|
assert!(request.has_content_kinds(&["current_time.reminder"]));
|
||||||
let reminders = current_time_reminders(&request);
|
let reminders = current_time_reminders(&request);
|
||||||
assert_eq!(reminders.len(), 1);
|
assert_eq!(reminders.len(), 1);
|
||||||
|
|||||||
@@ -1012,7 +1012,7 @@ async fn review_uses_custom_review_model_from_config() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Ensure that when `review_model` is not set in the config, the review request
|
/// Ensure that when `review_model` is not set in the config, the review request
|
||||||
/// uses the session model.
|
/// uses the session model without exposing disabled clock tools or reminders.
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn review_uses_session_model_when_review_model_unset() {
|
async fn review_uses_session_model_when_review_model_unset() {
|
||||||
skip_if_no_network!();
|
skip_if_no_network!();
|
||||||
@@ -1025,7 +1025,7 @@ async fn review_uses_session_model_when_review_model_unset() {
|
|||||||
.with_config(|config| {
|
.with_config(|config| {
|
||||||
config.model = Some("gpt-5.4".to_string());
|
config.model = Some("gpt-5.4".to_string());
|
||||||
config.review_model = None;
|
config.review_model = None;
|
||||||
config.model_reasoning_effort = Some(ReasoningEffort::Max);
|
config.model_reasoning_effort = Some(ReasoningEffort::Persistent);
|
||||||
})
|
})
|
||||||
.build_with_auto_env(&server)
|
.build_with_auto_env(&server)
|
||||||
.await
|
.await
|
||||||
@@ -1061,7 +1061,12 @@ async fn review_uses_session_model_when_review_model_unset() {
|
|||||||
assert_eq!(request.path(), "/v1/responses");
|
assert_eq!(request.path(), "/v1/responses");
|
||||||
let body = request.body_json();
|
let body = request.body_json();
|
||||||
assert_eq!(body["model"].as_str().unwrap(), "gpt-5.4");
|
assert_eq!(body["model"].as_str().unwrap(), "gpt-5.4");
|
||||||
assert_eq!(body["reasoning"]["effort"].as_str(), Some("max"));
|
assert_eq!(body["reasoning"]["effort"].as_str(), Some("disabled"));
|
||||||
|
assert_eq!(
|
||||||
|
["curr_time", "sleep"].map(|name| request.tool_by_name("clock", name).is_some()),
|
||||||
|
[false, false]
|
||||||
|
);
|
||||||
|
assert!(!request.has_content_kinds(&["current_time.reminder"]));
|
||||||
|
|
||||||
let _codex_home_guard = codex_home;
|
let _codex_home_guard = codex_home;
|
||||||
server.verify().await;
|
server.verify().await;
|
||||||
|
|||||||
Reference in new issue
Block a user