mirror of
https://github.com/openai/codex.git
synced 2026-11-07 20:30:33 +00:00
Preserve selected reasoning effort for synchronous Guardian reviews (#46292)
## Why Synchronous Guardian reviewers need to use their selected request-level reasoning effort even when managed requirements enable `reasoning_effort_override` and the parent history contains effort updates. ## What changed Disable reasoning-effort overrides for synchronous Guardian sessions and use the same decision for request filtering, effort pinning, and update injection. Reviewers omit inherited configuration updates from requests without changing saved history or appending new effort updates. ## Testing Add regression coverage for both current and legacy Guardian session sources with managed overrides enabled. Verify that the parent retains its pinned effort, reviewer requests follow selected `low` and `high` efforts, and inherited updates remain in saved history. GitOrigin-RevId: 3a635dbdc613ea72f7a96fc96bb386e4ac7994c3
This commit is contained in:
@@ -494,6 +494,10 @@ impl ModelClient {
|
||||
let auth_env_telemetry =
|
||||
collect_auth_env_telemetry(model_provider.info(), codex_api_key_env_enabled);
|
||||
let include_attestation = model_provider.supports_attestation();
|
||||
// Reviewers use their own request-level effort even when managed requirements
|
||||
// pin the parent's feature on. Share this decision with update injection and pinning.
|
||||
let reasoning_effort_override_enabled = reasoning_effort_override_enabled
|
||||
&& !crate::guardian::is_basic_session_source(&session_source);
|
||||
Self {
|
||||
state: Arc::new(ModelClientState {
|
||||
thread_id,
|
||||
@@ -524,6 +528,10 @@ impl ModelClient {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn reasoning_effort_override_enabled(&self) -> bool {
|
||||
self.state.reasoning_effort_override_enabled
|
||||
}
|
||||
|
||||
pub(crate) fn with_restored_history(mut self, restored_history: bool) -> Self {
|
||||
self.restored_history = restored_history;
|
||||
self
|
||||
|
||||
@@ -3,12 +3,12 @@
|
||||
//! Only trusted harness items establish overrides. Replay preserves startup prewarm's
|
||||
//! baseline while it is retained.
|
||||
//! Successful compaction retires the overrides and allows a fresh request baseline.
|
||||
//! Synchronous Guardian reviewers always use their selected request-level effort.
|
||||
|
||||
use super::session::Session;
|
||||
use super::step_context::StepContext;
|
||||
use super::step_settings::ResolvedStepSettings;
|
||||
use crate::state::ReasoningEffortPin;
|
||||
use codex_features::Feature;
|
||||
use codex_history::CodexHarnessMetadata;
|
||||
use codex_history::ResponseItemEnvelope;
|
||||
use codex_protocol::models::ConfigurationReasoning;
|
||||
@@ -96,7 +96,11 @@ impl Session {
|
||||
usage: RequestEffortUsage,
|
||||
) -> Option<ReasoningEffort> {
|
||||
let selected_effort = settings.reasoning_effort().cloned();
|
||||
if !self.enabled(Feature::ReasoningEffortOverride) {
|
||||
if !self
|
||||
.services
|
||||
.model_client
|
||||
.reasoning_effort_override_enabled()
|
||||
{
|
||||
return selected_effort;
|
||||
}
|
||||
if usage == RequestEffortUsage::Compaction
|
||||
@@ -130,7 +134,10 @@ impl Session {
|
||||
&self,
|
||||
settings: &ResolvedStepSettings,
|
||||
) -> Option<ReasoningEffort> {
|
||||
if !self.enabled(Feature::ReasoningEffortOverride)
|
||||
if !self
|
||||
.services
|
||||
.model_client
|
||||
.reasoning_effort_override_enabled()
|
||||
|| !settings.model_info.use_responses_lite
|
||||
|| !self.provider().await.is_openai()
|
||||
{
|
||||
|
||||
@@ -2,10 +2,11 @@
|
||||
|
||||
use super::RequestEffortUsage;
|
||||
use crate::session::step_settings::ResolvedStepSettings;
|
||||
use crate::session::tests::make_session_and_context;
|
||||
use crate::session::tests::make_session_and_context_with_auth_and_config_and_rx;
|
||||
use codex_features::Feature;
|
||||
use codex_history::InitialHistory;
|
||||
use codex_history::ResumedHistory;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -20,11 +21,17 @@ use test_case::test_case;
|
||||
}); "resume")]
|
||||
#[tokio::test]
|
||||
async fn initial_replay_preserves_prewarmed_effort(history: InitialHistory) {
|
||||
let (mut session, turn_context) = make_session_and_context().await;
|
||||
session
|
||||
.features
|
||||
.enable(Feature::ReasoningEffortOverride)
|
||||
.unwrap();
|
||||
let (session, turn_context, _events) = make_session_and_context_with_auth_and_config_and_rx(
|
||||
CodexAuth::from_api_key("Test API Key"),
|
||||
Vec::new(),
|
||||
|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ReasoningEffortOverride)
|
||||
.unwrap();
|
||||
},
|
||||
)
|
||||
.await;
|
||||
let mut model_info = Arc::clone(&turn_context.initial_settings.model_info);
|
||||
Arc::make_mut(&mut model_info).use_responses_lite = true;
|
||||
let mut selected = turn_context.initial_settings.selected().clone();
|
||||
@@ -59,11 +66,17 @@ async fn initial_replay_preserves_prewarmed_effort(history: InitialHistory) {
|
||||
|
||||
#[tokio::test]
|
||||
async fn compaction_effort_lookup_preserves_pin_for_fallback_models() {
|
||||
let (mut session, turn_context) = make_session_and_context().await;
|
||||
session
|
||||
.features
|
||||
.enable(Feature::ReasoningEffortOverride)
|
||||
.unwrap();
|
||||
let (session, turn_context, _events) = make_session_and_context_with_auth_and_config_and_rx(
|
||||
CodexAuth::from_api_key("Test API Key"),
|
||||
Vec::new(),
|
||||
|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ReasoningEffortOverride)
|
||||
.unwrap();
|
||||
},
|
||||
)
|
||||
.await;
|
||||
session
|
||||
.state
|
||||
.lock()
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
//! Trusted reasoning-effort updates follow surviving history and the next turn's selected settings.
|
||||
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_core::ForkSnapshot;
|
||||
use codex_core::RecoverTurnRequest;
|
||||
use codex_core::StartIfIdleSubmission;
|
||||
use codex_core::StartThreadOptions;
|
||||
use codex_core::SuspendTurnOutcome;
|
||||
use codex_core::TurnInput;
|
||||
use codex_core::TurnInputRequest;
|
||||
@@ -16,8 +18,12 @@ use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::openai_models::ReasoningEffortPreset;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::InternalSessionSource;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_protocol::protocol::ThreadSource;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::responses::ResponsesRequest;
|
||||
@@ -70,6 +76,126 @@ fn message(role: &str, text: &str) -> Value {
|
||||
})
|
||||
}
|
||||
|
||||
#[test_case(SessionSource::Internal(InternalSessionSource::Guardian); "internal guardian")]
|
||||
#[test_case(SessionSource::SubAgent(SubAgentSource::Other("guardian".to_string())); "legacy guardian")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn sync_guardian_ignores_managed_reasoning_override(
|
||||
session_source: SessionSource,
|
||||
) -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
let server = responses::start_mock_server().await;
|
||||
let mut mocks = Vec::new();
|
||||
for id in [
|
||||
"parent-medium",
|
||||
"parent-high",
|
||||
"reviewer-low",
|
||||
"reviewer-high",
|
||||
] {
|
||||
mocks.push(
|
||||
responses::mount_sse_once(&server, responses::sse(vec![responses::ev_completed(id)]))
|
||||
.await,
|
||||
);
|
||||
}
|
||||
let mut test = override_builder()
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
"[features]\nreasoning_effort_override = true\n",
|
||||
),
|
||||
)
|
||||
.with_config(|config| config.model_provider.supports_websockets = false)
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
test.submit_text_turn("first parent turn").await?;
|
||||
submit_thread_settings(
|
||||
&test.codex,
|
||||
ThreadSettingsOverrides {
|
||||
effort: Some(Some(ReasoningEffort::High)),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
test.submit_text_turn("second parent turn").await?;
|
||||
let parent = Arc::clone(&test.codex);
|
||||
parent.shutdown_and_wait().await?;
|
||||
|
||||
// Fork real parent history, including its trusted effort updates, into a reviewer.
|
||||
let config = test.config.clone();
|
||||
assert!(config.features.enabled(Feature::ReasoningEffortOverride));
|
||||
let mut options = StartThreadOptions::new(config);
|
||||
options.session_source = Some(session_source);
|
||||
options.thread_source = Some(ThreadSource::GuardianReview);
|
||||
let forked = test
|
||||
.thread_manager
|
||||
.fork_thread(
|
||||
ForkSnapshot::Interrupted,
|
||||
options,
|
||||
parent.rollout_path().expect("parent rollout path"),
|
||||
)
|
||||
.await?;
|
||||
test.codex = forked.thread;
|
||||
test.session_configured = forked.session_configured;
|
||||
for effort in [ReasoningEffort::Low, ReasoningEffort::High] {
|
||||
submit_thread_settings(
|
||||
&test.codex,
|
||||
ThreadSettingsOverrides {
|
||||
effort: Some(Some(effort)),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
test.submit_text_turn("review an action").await?;
|
||||
}
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
|
||||
let requests = mocks
|
||||
.iter()
|
||||
.map(responses::ResponseMock::single_request)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
requests
|
||||
.iter()
|
||||
.map(|request| request.body_json()["reasoning"]["effort"].clone())
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
Value::from("medium"),
|
||||
Value::from("medium"),
|
||||
Value::from("low"),
|
||||
Value::from("high"),
|
||||
],
|
||||
);
|
||||
let inherited_updates = vec![
|
||||
effort_update(ReasoningEffort::Medium),
|
||||
effort_update(ReasoningEffort::High),
|
||||
];
|
||||
assert_eq!(
|
||||
requests.iter().map(effort_updates).collect::<Vec<_>>(),
|
||||
vec![
|
||||
vec![effort_update(ReasoningEffort::Medium)],
|
||||
inherited_updates.clone(),
|
||||
vec![],
|
||||
vec![],
|
||||
],
|
||||
);
|
||||
// Reviewers neither append new updates nor erase inherited durable history.
|
||||
for thread in [&parent, &test.codex] {
|
||||
let saved_updates = thread
|
||||
.load_history(/*include_archived*/ false)
|
||||
.await?
|
||||
.items
|
||||
.into_iter()
|
||||
.filter_map(|item| match item {
|
||||
RolloutItem::ResponseItem(ResponseItemEnvelope {
|
||||
item: item @ ResponseItem::ConfigurationUpdate { .. },
|
||||
..
|
||||
}) => Some(serde_json::to_value(item).expect("serialize saved update")),
|
||||
_ => None,
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(saved_updates, inherited_updates);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn reasoning_effort_override_recovery_reuses_trusted_tail_update() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
Reference in New Issue
Block a user