mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
[codex] Defer fork context injection until first turn (#15699)
## Summary - remove the fork-startup `build_initial_context` injection - keep the reconstructed `reference_context_item` as the fork baseline until the first real turn - update fork-history tests and the request snapshot, and add a `TODO(ccunningham)` for remaining nondiffable initial-context inputs ## Why Fork startup was appending current-session initial context immediately after reconstructing the parent rollout, then the first real turn could emit context updates again. That duplicated model-visible context in the child rollout. ## Impact Forked sessions now behave like resume for context seeding: startup reconstructs history and preserves the prior baseline, and the first real turn handles any current-session context emission. --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
Charley Cunningham
committed by
GitHub
GitHub
parent
2e03d8b4d2
commit
d72fa2a209
@@ -2215,15 +2215,6 @@ impl Session {
|
||||
self.persist_rollout_items(&rollout_items).await;
|
||||
}
|
||||
|
||||
// Append the current session's initial context after the reconstructed history.
|
||||
let initial_context = self.build_initial_context(&turn_context).await;
|
||||
self.record_conversation_items(&turn_context, &initial_context)
|
||||
.await;
|
||||
{
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_reference_context_item(Some(turn_context.to_turn_context_item()));
|
||||
}
|
||||
|
||||
// Forked threads should remain file-backed immediately after startup.
|
||||
self.ensure_rollout_materialized().await;
|
||||
|
||||
|
||||
@@ -1115,18 +1115,12 @@ async fn recompute_token_usage_updates_model_context_window() {
|
||||
#[tokio::test]
|
||||
async fn record_initial_history_reconstructs_forked_transcript() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let (rollout_items, mut expected) = sample_rollout(&session, &turn_context).await;
|
||||
let (rollout_items, expected) = sample_rollout(&session, &turn_context).await;
|
||||
|
||||
session
|
||||
.record_initial_history(InitialHistory::Forked(rollout_items))
|
||||
.await;
|
||||
|
||||
let reconstruction_turn = session.new_default_turn().await;
|
||||
expected.extend(
|
||||
session
|
||||
.build_initial_context(reconstruction_turn.as_ref())
|
||||
.await,
|
||||
);
|
||||
let history = session.state.lock().await.clone_history();
|
||||
assert_eq!(expected, history.raw_items());
|
||||
}
|
||||
@@ -1244,7 +1238,7 @@ async fn fork_startup_context_then_first_turn_diff_snapshot() -> anyhow::Result<
|
||||
|
||||
let request = first_forked_request.single_request();
|
||||
let snapshot = context_snapshot::format_labeled_requests_snapshot(
|
||||
"First request after fork when fork startup changes approval policy and the first forked turn changes approval policy again and enters plan mode.",
|
||||
"First request after fork when startup preserves the parent baseline, the fork changes approval policy, and the first forked turn enters plan mode.",
|
||||
&[("First Forked Turn Request", &request)],
|
||||
&ContextSnapshotOptions::default()
|
||||
.render_mode(ContextSnapshotRenderMode::KindWithTextPrefix { max_chars: 96 })
|
||||
@@ -1309,7 +1303,7 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
text_elements: Vec::new(),
|
||||
},
|
||||
)),
|
||||
RolloutItem::TurnContext(previous_context_item),
|
||||
RolloutItem::TurnContext(previous_context_item.clone()),
|
||||
RolloutItem::EventMsg(EventMsg::TurnComplete(
|
||||
codex_protocol::protocol::TurnCompleteEvent {
|
||||
turn_id,
|
||||
@@ -1322,6 +1316,7 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
.record_initial_history(InitialHistory::Forked(rollout_items))
|
||||
.await;
|
||||
|
||||
let history = session.clone_history().await;
|
||||
assert_eq!(
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
@@ -1329,6 +1324,13 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
assert_eq!(history.raw_items(), &[]);
|
||||
assert_eq!(
|
||||
serde_json::to_value(session.reference_context_item().await)
|
||||
.expect("serialize fork reference context item"),
|
||||
serde_json::to_value(Some(previous_context_item))
|
||||
.expect("serialize expected reference context item")
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -193,6 +193,10 @@ pub(crate) fn build_settings_update_items(
|
||||
exec_policy: &Policy,
|
||||
personality_feature_enabled: bool,
|
||||
) -> Vec<ResponseItem> {
|
||||
// TODO(ccunningham): build_settings_update_items still does not cover every
|
||||
// model-visible item emitted by build_initial_context. Persist the remaining
|
||||
// inputs or add explicit replay events so fork/resume can diff everything
|
||||
// deterministically.
|
||||
let contextual_user_message = build_environment_update_item(previous, next, shell);
|
||||
let developer_update_sections = [
|
||||
// Keep model-switch instructions first so model-specific guidance is read before
|
||||
|
||||
@@ -1,17 +1,15 @@
|
||||
---
|
||||
source: core/src/codex_tests.rs
|
||||
assertion_line: 1282
|
||||
assertion_line: 1254
|
||||
expression: snapshot
|
||||
---
|
||||
Scenario: First request after fork when fork startup changes approval policy and the first forked turn changes approval policy again and enters plan mode.
|
||||
Scenario: First request after fork when startup preserves the parent baseline, the fork changes approval policy, and the first forked turn enters plan mode.
|
||||
|
||||
## First Forked Turn Request
|
||||
00:message/developer:<PERMISSIONS_INSTRUCTIONS>
|
||||
01:message/user:<ENVIRONMENT_CONTEXT:cwd=<CWD>>
|
||||
02:message/user:fork seed
|
||||
03:message/developer:<PERMISSIONS_INSTRUCTIONS>
|
||||
04:message/user:<ENVIRONMENT_CONTEXT:cwd=<CWD>>
|
||||
05:message/developer[2]:
|
||||
03:message/developer[2]:
|
||||
[01] <PERMISSIONS_INSTRUCTIONS>
|
||||
[02] <collaboration_mode>Fork turn collaboration instructions.</collaboration_mode>
|
||||
06:message/user:after fork
|
||||
04:message/user:after fork
|
||||
|
||||
@@ -125,9 +125,8 @@ async fn fork_thread_twice_drops_to_first_message() {
|
||||
|
||||
// GetHistory on fork1 flushed; the file is ready.
|
||||
let fork1_items = read_items(&fork1_path);
|
||||
assert!(fork1_items.len() > expected_after_first.len());
|
||||
pretty_assertions::assert_eq!(
|
||||
serde_json::to_value(&fork1_items[..expected_after_first.len()]).unwrap(),
|
||||
serde_json::to_value(&fork1_items).unwrap(),
|
||||
serde_json::to_value(&expected_after_first).unwrap()
|
||||
);
|
||||
|
||||
@@ -156,9 +155,8 @@ async fn fork_thread_twice_drops_to_first_message() {
|
||||
.unwrap_or(0);
|
||||
let expected_after_second: Vec<RolloutItem> = fork1_items[..cut_last_on_fork1].to_vec();
|
||||
let fork2_items = read_items(&fork2_path);
|
||||
assert!(fork2_items.len() > expected_after_second.len());
|
||||
pretty_assertions::assert_eq!(
|
||||
serde_json::to_value(&fork2_items[..expected_after_second.len()]).unwrap(),
|
||||
serde_json::to_value(&fork2_items).unwrap(),
|
||||
serde_json::to_value(&expected_after_second).unwrap()
|
||||
);
|
||||
}
|
||||
|
||||
@@ -443,14 +443,14 @@ async fn resume_and_fork_append_permissions_messages() -> Result<()> {
|
||||
let body4 = req4.single_request().body_json();
|
||||
let input4 = body4["input"].as_array().expect("input array");
|
||||
let permissions_fork = permissions_texts(input4);
|
||||
assert_eq!(permissions_fork.len(), permissions_base.len() + 2);
|
||||
assert_eq!(permissions_fork.len(), permissions_base.len() + 1);
|
||||
assert_eq!(
|
||||
&permissions_fork[..permissions_base.len()],
|
||||
permissions_base.as_slice()
|
||||
);
|
||||
let new_permissions = &permissions_fork[permissions_base.len()..];
|
||||
assert_eq!(new_permissions.len(), 2);
|
||||
assert_eq!(new_permissions[0], new_permissions[1]);
|
||||
assert_eq!(new_permissions.len(), 1);
|
||||
assert_eq!(permissions_fork, permissions_resume);
|
||||
assert!(!permissions_base.contains(&new_permissions[0]));
|
||||
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user