mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Compact when comp_hash changes (#27520)
## Summary - snapshot `comp_hash` into `TurnContext` when the turn is created and use that snapshot as the downstream source of truth - persist the turn hash in rollout context and recover it into previous-turn settings during resume and fork replay - compact existing history with the previous model only when both adjacent turns provide hashes and the values differ - record `comp_hash_changed` as the compaction reason - cover ordinary transitions, resume, and missing-hash compatibility with end-to-end tests ## Why History produced under one compaction-compatible model configuration may not be safe to carry directly into another. Compacting at the turn boundary converts that history before context updates and the new user message are added. Persisting the turn snapshot in `TurnContextItem` makes the same protection work after resuming a rollout. A missing hash is not treated as evidence of incompatibility. `None → Some`, `Some → None`, and `None → None` do not trigger compaction; only `Some(previous) → Some(current)` with unequal values does. ## Stack - depends on #27532 - #27532 is based directly on `main` ## Testing - `just test -p codex-core pre_sampling_compact_` — 6 passed - `just test -p codex-core turn_context_item_uses_turn_context_comp_hash_snapshot` — passed - `just fix -p codex-core -p codex-protocol -p codex-analytics -p codex-models-manager`
This commit is contained in:
@@ -483,6 +483,7 @@ async fn process_compacted_history_reinjects_model_switch_message() {
|
||||
}];
|
||||
let previous_turn_settings = PreviousTurnSettings {
|
||||
model: "previous-regular-model".to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: None,
|
||||
};
|
||||
|
||||
|
||||
@@ -178,6 +178,7 @@ fn turn_context_item_filesystem_uses_workspace_roots_instead_of_cwd() {
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: "gpt-5".to_string(),
|
||||
comp_hash: None,
|
||||
personality: None,
|
||||
collaboration_mode: None,
|
||||
multi_agent_version: None,
|
||||
|
||||
@@ -130,6 +130,7 @@ fn reference_context_item() -> TurnContextItem {
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: "gpt-test".to_string(),
|
||||
comp_hash: None,
|
||||
personality: None,
|
||||
collaboration_mode: None,
|
||||
multi_agent_version: None,
|
||||
|
||||
@@ -276,10 +276,12 @@ impl SteerInputError {
|
||||
/// Conceptually this is the same role that `previous_model` used to fill, but
|
||||
/// it can carry other prior-turn settings that matter when constructing
|
||||
/// sensible state-change diffs or full-context reinjection, such as model
|
||||
/// switches or detecting a prior `realtime_active -> false` transition.
|
||||
/// switches, compaction compatibility, or detecting a prior
|
||||
/// `realtime_active -> false` transition.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) struct PreviousTurnSettings {
|
||||
pub(crate) model: String,
|
||||
pub(crate) comp_hash: Option<String>,
|
||||
pub(crate) realtime_active: Option<bool>,
|
||||
}
|
||||
|
||||
|
||||
@@ -114,6 +114,7 @@ pub(super) async fn spawn_review_thread(
|
||||
config: per_turn_config,
|
||||
auth_manager: auth_manager_for_context,
|
||||
model_info: model_info.clone(),
|
||||
comp_hash: model_info.comp_hash.clone(),
|
||||
tool_mode,
|
||||
session_telemetry: session_telemetry_for_context,
|
||||
provider: provider_for_context,
|
||||
|
||||
@@ -184,6 +184,7 @@ impl Session {
|
||||
) {
|
||||
active_segment.previous_turn_settings = Some(PreviousTurnSettings {
|
||||
model: ctx.model.clone(),
|
||||
comp_hash: ctx.comp_hash.clone(),
|
||||
realtime_active: ctx.realtime_active,
|
||||
});
|
||||
if matches!(
|
||||
|
||||
@@ -70,6 +70,7 @@ async fn record_initial_history_resumed_bare_turn_context_does_not_hydrate_previ
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -109,6 +110,7 @@ async fn record_initial_history_resumed_hydrates_previous_turn_settings_from_lif
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: Some("comp-hash-a".to_string()),
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -166,6 +168,7 @@ async fn record_initial_history_resumed_hydrates_previous_turn_settings_from_lif
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: Some("comp-hash-a".to_string()),
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -271,6 +274,7 @@ async fn reconstruct_history_rollback_keeps_history_and_metadata_in_sync_for_com
|
||||
reconstructed.previous_turn_settings,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -364,6 +368,7 @@ async fn reconstruct_history_rollback_keeps_history_and_metadata_in_sync_for_inc
|
||||
reconstructed.previous_turn_settings,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -489,6 +494,7 @@ async fn reconstruct_history_rollback_skips_non_user_turns_for_history_and_metad
|
||||
reconstructed.previous_turn_settings,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -589,6 +595,7 @@ async fn reconstruct_history_rollback_counts_inter_agent_assistant_turns() {
|
||||
reconstructed.previous_turn_settings,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -810,6 +817,7 @@ async fn record_initial_history_resumed_rollback_drops_incomplete_user_turn_comp
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -963,6 +971,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1024,6 +1033,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -1043,6 +1053,7 @@ async fn record_initial_history_resumed_turn_context_after_compaction_reestablis
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1072,6 +1083,7 @@ async fn record_initial_history_resumed_aborted_turn_without_id_clears_active_tu
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1161,6 +1173,7 @@ async fn record_initial_history_resumed_aborted_turn_without_id_clears_active_tu
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -1192,6 +1205,7 @@ async fn record_initial_history_resumed_unmatched_abort_preserves_active_turn_fo
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: current_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1281,6 +1295,7 @@ async fn record_initial_history_resumed_unmatched_abort_preserves_active_turn_fo
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: current_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -1310,6 +1325,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_compaction_clea
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1391,6 +1407,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_compaction_clea
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -1441,6 +1458,7 @@ async fn record_initial_history_resumed_trailing_incomplete_turn_preserves_turn_
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -1470,6 +1488,7 @@ async fn record_initial_history_resumed_replaced_incomplete_compacted_turn_clear
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -1563,6 +1582,7 @@ async fn record_initial_history_resumed_replaced_incomplete_compacted_turn_clear
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
|
||||
@@ -2669,6 +2669,7 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
personality: turn_context.personality,
|
||||
collaboration_mode: Some(turn_context.collaboration_mode.clone()),
|
||||
multi_agent_version: None,
|
||||
@@ -2721,6 +2722,7 @@ async fn record_initial_history_forked_hydrates_previous_turn_settings() {
|
||||
session.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: previous_model.to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -2763,6 +2765,7 @@ async fn thread_rollback_drops_last_turn_from_history() {
|
||||
sess.persist_rollout_items(&rollout_items).await;
|
||||
sess.set_previous_turn_settings(Some(PreviousTurnSettings {
|
||||
model: "stale-model".to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(tc.realtime_active),
|
||||
}))
|
||||
.await;
|
||||
@@ -2945,6 +2948,7 @@ async fn thread_rollback_recomputes_previous_turn_settings_and_reference_context
|
||||
.await;
|
||||
sess.set_previous_turn_settings(Some(PreviousTurnSettings {
|
||||
model: "stale-model".to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: None,
|
||||
}))
|
||||
.await;
|
||||
@@ -2961,6 +2965,7 @@ async fn thread_rollback_recomputes_previous_turn_settings_and_reference_context
|
||||
sess.previous_turn_settings().await,
|
||||
Some(PreviousTurnSettings {
|
||||
model: tc.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(tc.realtime_active),
|
||||
})
|
||||
);
|
||||
@@ -3061,6 +3066,7 @@ async fn thread_rollback_restores_cleared_reference_context_item_after_compactio
|
||||
RolloutItem::TurnContext(TurnContextItem {
|
||||
turn_id: Some(rolled_back_turn_id.clone()),
|
||||
model: "rolled-back-model".to_string(),
|
||||
comp_hash: None,
|
||||
..first_context_item.clone()
|
||||
}),
|
||||
RolloutItem::ResponseItem(user_message("turn 2 user")),
|
||||
@@ -7457,6 +7463,7 @@ async fn build_settings_update_items_uses_previous_turn_settings_for_realtime_en
|
||||
previous_context_item.realtime_active = None;
|
||||
let previous_turn_settings = PreviousTurnSettings {
|
||||
model: previous_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(true),
|
||||
};
|
||||
let mut current_context = previous_context
|
||||
@@ -8050,6 +8057,7 @@ async fn build_initial_context_uses_previous_turn_settings_for_realtime_end() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let previous_turn_settings = PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(true),
|
||||
};
|
||||
|
||||
@@ -8072,6 +8080,7 @@ async fn build_initial_context_restates_realtime_start_when_reference_context_is
|
||||
turn_context.realtime_active = true;
|
||||
let previous_turn_settings = PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(true),
|
||||
};
|
||||
|
||||
@@ -8105,6 +8114,18 @@ fn file_system_policy_with_unreadable_glob(turn_context: &TurnContext) -> FileSy
|
||||
policy
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_context_item_uses_turn_context_comp_hash_snapshot() {
|
||||
let (_session, mut turn_context) = make_session_and_context().await;
|
||||
turn_context.comp_hash = Some("turn-context-hash".to_string());
|
||||
turn_context.model_info.comp_hash = Some("model-info-hash".to_string());
|
||||
|
||||
assert_eq!(
|
||||
turn_context.to_turn_context_item().comp_hash.as_deref(),
|
||||
Some("turn-context-hash")
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn turn_context_item_omits_legacy_equivalent_file_system_sandbox_policy() {
|
||||
let (_session, turn_context) = make_session_and_context().await;
|
||||
@@ -8296,6 +8317,7 @@ async fn build_initial_context_prepends_model_switch_message() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let previous_turn_settings = PreviousTurnSettings {
|
||||
model: "previous-regular-model".to_string(),
|
||||
comp_hash: None,
|
||||
realtime_active: None,
|
||||
};
|
||||
|
||||
@@ -8348,6 +8370,7 @@ async fn record_context_updates_and_set_reference_context_item_persists_full_rei
|
||||
session
|
||||
.set_previous_turn_settings(Some(PreviousTurnSettings {
|
||||
model: previous_context.model_info.slug.clone(),
|
||||
comp_hash: None,
|
||||
realtime_active: Some(previous_context.realtime_active),
|
||||
}))
|
||||
.await;
|
||||
|
||||
@@ -172,6 +172,7 @@ pub(crate) async fn run_turn(
|
||||
.await;
|
||||
sess.set_previous_turn_settings(Some(PreviousTurnSettings {
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
comp_hash: turn_context.comp_hash.clone(),
|
||||
realtime_active: Some(turn_context.realtime_active),
|
||||
}))
|
||||
.await;
|
||||
@@ -798,8 +799,16 @@ async fn run_pre_sampling_compact(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Runs pre-sampling compaction against the previous model when switching to a smaller
|
||||
/// context-window model.
|
||||
/// Returns true only when both turns declare compaction compatibility hashes and they differ.
|
||||
/// A missing hash does not provide enough information to trigger compaction.
|
||||
fn comp_hash_changed(previous: Option<&str>, current: Option<&str>) -> bool {
|
||||
previous
|
||||
.zip(current)
|
||||
.is_some_and(|(previous, current)| previous != current)
|
||||
}
|
||||
|
||||
/// Runs pre-sampling compaction against the previous model when its compaction compatibility
|
||||
/// hash changed or when switching to a smaller context-window model.
|
||||
///
|
||||
/// Returns `Err(_)` only when compaction was attempted and failed.
|
||||
async fn maybe_run_previous_model_inline_compact(
|
||||
@@ -810,12 +819,29 @@ async fn maybe_run_previous_model_inline_compact(
|
||||
let Some(previous_turn_settings) = sess.previous_turn_settings().await else {
|
||||
return Ok(());
|
||||
};
|
||||
let should_compact_for_comp_hash_change = comp_hash_changed(
|
||||
previous_turn_settings.comp_hash.as_deref(),
|
||||
turn_context.comp_hash.as_deref(),
|
||||
);
|
||||
let previous_model_turn_context = Arc::new(
|
||||
turn_context
|
||||
.with_model(previous_turn_settings.model, &sess.services.models_manager)
|
||||
.await,
|
||||
);
|
||||
|
||||
if should_compact_for_comp_hash_change {
|
||||
run_auto_compact(
|
||||
sess,
|
||||
&previous_model_turn_context,
|
||||
client_session,
|
||||
InitialContextInjection::DoNotInject,
|
||||
CompactionReason::CompHashChanged,
|
||||
CompactionPhase::PreTurn,
|
||||
)
|
||||
.await?;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let Some(old_context_window) = previous_model_turn_context.model_context_window() else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
@@ -61,6 +61,7 @@ pub struct TurnContext {
|
||||
pub config: Arc<Config>,
|
||||
pub(crate) auth_manager: Option<Arc<AuthManager>>,
|
||||
pub(crate) model_info: ModelInfo,
|
||||
pub(crate) comp_hash: Option<String>,
|
||||
pub(crate) tool_mode: ToolMode,
|
||||
pub(crate) session_telemetry: SessionTelemetry,
|
||||
pub(crate) provider: SharedModelProvider,
|
||||
@@ -229,6 +230,7 @@ impl TurnContext {
|
||||
config: Arc::new(config),
|
||||
auth_manager: self.auth_manager.clone(),
|
||||
model_info: model_info.clone(),
|
||||
comp_hash: model_info.comp_hash.clone(),
|
||||
tool_mode,
|
||||
session_telemetry: self
|
||||
.session_telemetry
|
||||
@@ -355,6 +357,7 @@ impl TurnContext {
|
||||
network: self.turn_context_network_item(),
|
||||
file_system_sandbox_policy: self.non_legacy_file_system_sandbox_policy(),
|
||||
model: self.model_info.slug.clone(),
|
||||
comp_hash: self.comp_hash.clone(),
|
||||
personality: self.personality,
|
||||
collaboration_mode: Some(self.collaboration_mode.clone()),
|
||||
multi_agent_version: Some(self.multi_agent_version),
|
||||
@@ -531,6 +534,7 @@ impl Session {
|
||||
config: per_turn_config.clone(),
|
||||
auth_manager: auth_manager_for_context,
|
||||
model_info: model_info.clone(),
|
||||
comp_hash: model_info.comp_hash.clone(),
|
||||
tool_mode,
|
||||
session_telemetry: session_telemetry_for_context,
|
||||
provider: provider_for_context,
|
||||
|
||||
Reference in New Issue
Block a user