agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

core: bundle settings diff updates into one dev/user envelope (#12417)

Browse Source 
## Summary
- bundle contextual prompt injection into at most one developer message
plus one contextual user message in both:
  - per-turn settings updates
  - initial context insertion
- preserve `<model_switch>` across compaction by rebuilding it through
canonical initial-context injection, instead of relying on
strip/reattach hacks
- centralize contextual user fragment detection in one shared definition
table and reuse it for parsing/compaction logic
- keep `AGENTS.md` in its natural serialized format:
  - `# AGENTS.md instructions for {dirname}`
  - `<INSTRUCTIONS>...</INSTRUCTIONS>`
- simplify related tests/helpers and accept the expected snapshot/layout
updates from bundled multi-part messages

## Why
The goal is to converge toward a simpler, more intentional prompt shape
where contextual updates are consistently represented as one developer
envelope plus one contextual user envelope, while keeping parsing and
compaction behavior aligned with that representation.

## Notable details
- the temporary `SettingsUpdateEnvelope` wrapper was removed; these
paths now return `Vec<ResponseItem>` directly
- local/remote compaction no longer rely on model-switch strip/restore
helpers
- contextual user detection is now driven by shared fragment definitions
instead of ad hoc matcher assembly
- AGENTS/user instructions are still the same logical context; only the
synthetic `<user_instructions>` wrapper was replaced by the natural
AGENTS text format

## Testing
- `just fmt`
- `cargo test -p codex-app-server
codex_message_processor::tests::extract_conversation_summary_prefers_plain_user_messages
-- --exact`
- `cargo test -p codex-core
compact::tests::collect_user_messages_filters_session_prefix_entries
--lib -- --exact`
- `cargo test -p codex-core --test all
'suite::compact::snapshot_request_shape_pre_turn_compaction_strips_incoming_model_switch'
-- --exact`
- `cargo test -p codex-core --test all
'suite::compact_remote::snapshot_request_shape_remote_pre_turn_compaction_strips_incoming_model_switch'
-- --exact`
- `cargo test -p codex-core --test all
'suite::client::includes_apps_guidance_as_developer_message_when_enabled'
-- --exact`
- `cargo test -p codex-core --test all
'suite::client::includes_developer_instructions_message_in_request' --
--exact`
- `cargo test -p codex-core --test all
'suite::client::includes_user_instructions_message_in_request' --
--exact`
- `cargo test -p codex-core --test all
'suite::client::resume_includes_initial_messages_and_sends_prior_items'
-- --exact`
- `cargo test -p codex-core --test all
'suite::review::review_input_isolated_from_parent_history' -- --exact`
- `cargo test -p codex-exec --test all
'suite::resume::exec_resume_last_respects_cwd_filter_and_all_flag' --
--exact`
- `cargo test -p core_test_support
context_snapshot::tests::full_text_mode_preserves_unredacted_text --
--exact`

## Notes
- I also ran several targeted `compact`, `compact_remote`,
`prompt_caching`, `model_visible_layout`, and `event_mapping` tests
while iterating on prompt-shape changes.
- I have not claimed a clean full-workspace `cargo test` from this
environment because local sandbox/resource conditions have previously
produced unrelated failures in large workspace runs.
This commit is contained in:
Charley Cunningham
2026-02-26 00:12:08 -08:00
committed by GitHub
Unverified
parent 28bfbb8f2b
commit 07aefffb1f
47 changed files with 966 additions and 813 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/codex.rs
+92 -52
View File
@@ -1669,7 +1669,10 @@ impl Session {
match conversation_history {
InitialHistory::New => {
// Build and record initial items (user instructions + environment context)
let items = self.build_initial_context(&turn_context).await;
// TODO(ccunningham): Defer initial context insertion until the first real turn
// starts so it reflects the actual first-turn settings (permissions, etc.) and
// we do not emit model-visible "diff" updates before the first user message.
let items = self.build_initial_context(&turn_context, None).await;
self.record_conversation_items(&turn_context, &items).await;
{
let mut state = self.state.lock().await;
@@ -1773,7 +1776,7 @@ impl Session {
}
// Append the current session's initial context after the reconstructed history.
let initial_context = self.build_initial_context(&turn_context).await;
let initial_context = self.build_initial_context(&turn_context, None).await;
self.record_conversation_items(&turn_context, &initial_context)
.await;
{
@@ -2862,7 +2865,7 @@ impl Session {
} else {
let user_messages = collect_user_messages(history.raw_items());
let rebuilt = compact::build_compacted_history(
self.build_initial_context(turn_context).await,
self.build_initial_context(turn_context, None).await,
&user_messages,
&compacted.message,
);
@@ -2990,10 +2993,20 @@ impl Session {
pub(crate) async fn build_initial_context(
&self,
turn_context: &TurnContext,
previous_user_turn_model: Option<&str>,
) -> Vec<ResponseItem> {
let mut items = Vec::<ResponseItem>::with_capacity(4);
let mut developer_sections = Vec::<String>::with_capacity(8);
let mut contextual_user_sections = Vec::<String>::with_capacity(2);
let shell = self.user_shell();
items.push(
if let Some(model_switch_message) =
crate::context_manager::updates::build_model_instructions_update_item(
previous_user_turn_model,
turn_context,
)
{
developer_sections.push(model_switch_message.into_text());
}
developer_sections.push(
DeveloperInstructions::from_policy(
turn_context.sandbox_policy.get(),
turn_context.approval_policy.value(),
@@ -3001,17 +3014,17 @@ impl Session {
&turn_context.cwd,
turn_context.features.enabled(Feature::RequestPermissions),
)
.into(),
.into_text(),
);
if let Some(developer_instructions) = turn_context.developer_instructions.as_deref() {
items.push(DeveloperInstructions::new(developer_instructions.to_string()).into());
developer_sections.push(developer_instructions.to_string());
}
// Add developer instructions for memories.
if let Some(memory_prompt) =
build_memory_tool_developer_instructions(&turn_context.config.codex_home).await
&& turn_context.features.enabled(Feature::MemoryTool)
{
items.push(DeveloperInstructions::new(memory_prompt).into());
developer_sections.push(memory_prompt);
}
// Add developer instructions from collaboration_mode if they exist and are non-empty
let (collaboration_mode, base_instructions) = {
@@ -3024,7 +3037,7 @@ impl Session {
if let Some(collab_instructions) =
DeveloperInstructions::from_collaboration_mode(&collaboration_mode)
{
items.push(collab_instructions.into());
developer_sections.push(collab_instructions.into_text());
}
if self.features.enabled(Feature::Personality)
&& let Some(personality) = turn_context.personality
@@ -3039,34 +3052,46 @@ impl Session {
personality,
)
{
items.push(
DeveloperInstructions::personality_spec_message(personality_message).into(),
developer_sections.push(
DeveloperInstructions::personality_spec_message(personality_message)
.into_text(),
);
}
}
if turn_context.features.enabled(Feature::Apps) {
items.push(DeveloperInstructions::new(render_apps_section()).into());
developer_sections.push(render_apps_section());
}
if turn_context.features.enabled(Feature::CodexGitCommit)
&& let Some(commit_message_instruction) = commit_message_trailer_instruction(
turn_context.config.commit_attribution.as_deref(),
)
{
items.push(DeveloperInstructions::new(commit_message_instruction).into());
developer_sections.push(commit_message_instruction);
}
if let Some(user_instructions) = turn_context.user_instructions.as_deref() {
items.push(
contextual_user_sections.push(
UserInstructions {
text: user_instructions.to_string(),
directory: turn_context.cwd.to_string_lossy().into_owned(),
}
.into(),
.serialize_to_text(),
);
}
items.push(ResponseItem::from(EnvironmentContext::from_turn_context(
turn_context,
shell.as_ref(),
)));
contextual_user_sections.push(
EnvironmentContext::from_turn_context(turn_context, shell.as_ref()).serialize_to_xml(),
);
let mut items = Vec::with_capacity(2);
if let Some(developer_message) =
crate::context_manager::updates::build_developer_update_item(developer_sections)
{
items.push(developer_message);
}
if let Some(contextual_user_message) =
crate::context_manager::updates::build_contextual_user_message(contextual_user_sections)
{
items.push(contextual_user_message);
}
items
}
@@ -3111,22 +3136,8 @@ impl Session {
let reference_context_item = self.reference_context_item().await;
let should_inject_full_context = reference_context_item.is_none();
let context_items = if should_inject_full_context {
let mut initial_context = self.build_initial_context(turn_context).await;
// Full reinjection bypasses the settings-diff path, so add the model-switch
// instruction explicitly when needed. Keep it before the rest of full context so
// model-specific guidance is read first.
if let Some(model_switch_item) =
crate::context_manager::updates::build_model_instructions_update_item(
previous_user_turn_model,
turn_context,
)
{
// TODO(ccunningham): When a model switch changes the effective personality
// instructions, inject the updated personality spec alongside <model_switch>
// here so resume/model-switch paths can avoid forcing full reinjection.
initial_context.insert(0, model_switch_item);
}
initial_context
self.build_initial_context(turn_context, previous_user_turn_model)
.await
} else {
// Steady-state path: append only context diffs to minimize token overhead.
self.build_settings_update_items(
@@ -3516,9 +3527,9 @@ impl Session {
use_linux_sandbox_bwrap: turn_context.features.enabled(Feature::UseLinuxSandboxBwrap),
};
{
let mut cancel_guard = self.services.mcp_startup_cancellation_token.lock().await;
cancel_guard.cancel();
*cancel_guard = CancellationToken::new();
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
guard.cancel();
*guard = CancellationToken::new();
}
let (refreshed_manager, cancel_token) = McpConnectionManager::new(
&mcp_servers,
@@ -3532,11 +3543,11 @@ impl Session {
)
.await;
{
let mut cancel_guard = self.services.mcp_startup_cancellation_token.lock().await;
if cancel_guard.is_cancelled() {
let mut guard = self.services.mcp_startup_cancellation_token.lock().await;
if guard.is_cancelled() {
cancel_token.cancel();
}
*cancel_guard = cancel_token;
*guard = cancel_token;
}
let mut manager = self.services.mcp_connection_manager.write().await;
@@ -5011,6 +5022,7 @@ pub(crate) async fn run_turn(
&sess,
&turn_context,
InitialContextInjection::BeforeLastUserMessage,
previous_model.as_deref(),
)
.await
.is_err()
@@ -5134,7 +5146,13 @@ async fn run_pre_sampling_compact(
.unwrap_or(i64::MAX);
// Compact if the total usage tokens are greater than the auto compact limit
if total_usage_tokens >= auto_compact_limit {
run_auto_compact(sess, turn_context, InitialContextInjection::DoNotInject).await?;
run_auto_compact(
sess,
turn_context,
InitialContextInjection::DoNotInject,
None,
)
.await?;
}
Ok(())
}
@@ -5177,6 +5195,7 @@ async fn maybe_run_previous_model_inline_compact(
sess,
&previous_model_turn_context,
InitialContextInjection::DoNotInject,
None,
)
.await?;
return Ok(true);
@@ -5188,12 +5207,14 @@ async fn run_auto_compact(
sess: &Arc<Session>,
turn_context: &Arc<TurnContext>,
initial_context_injection: InitialContextInjection,
previous_user_turn_model: Option<&str>,
) -> CodexResult<()> {
if should_use_remote_compact_task(&turn_context.provider) {
run_inline_remote_auto_compact_task(
Arc::clone(sess),
Arc::clone(turn_context),
initial_context_injection,
previous_user_turn_model,
)
.await?;
} else {
@@ -5201,6 +5222,7 @@ async fn run_auto_compact(
Arc::clone(sess),
Arc::clone(turn_context),
initial_context_injection,
previous_user_turn_model,
)
.await?;
}
@@ -7438,7 +7460,7 @@ mod tests {
session
.record_context_updates_and_set_reference_context_item(&turn_context, None)
.await;
expected.extend(session.build_initial_context(&turn_context).await);
expected.extend(session.build_initial_context(&turn_context, None).await);
let history_after_seed = session.clone_history().await;
assert_eq!(expected, history_after_seed.raw_items());
@@ -7600,7 +7622,7 @@ mod tests {
let reconstruction_turn = session.new_default_turn().await;
expected.extend(
session
.build_initial_context(reconstruction_turn.as_ref())
.build_initial_context(reconstruction_turn.as_ref(), None)
.await,
);
let history = session.state.lock().await.clone_history();
@@ -7643,7 +7665,7 @@ mod tests {
async fn thread_rollback_drops_last_turn_from_history() {
let (sess, tc, rx) = make_session_and_context_with_rx().await;
let initial_context = sess.build_initial_context(tc.as_ref()).await;
let initial_context = sess.build_initial_context(tc.as_ref(), None).await;
sess.record_into_history(&initial_context, tc.as_ref())
.await;
@@ -7714,7 +7736,7 @@ mod tests {
async fn thread_rollback_clears_history_when_num_turns_exceeds_existing_turns() {
let (sess, tc, rx) = make_session_and_context_with_rx().await;
let initial_context = sess.build_initial_context(tc.as_ref()).await;
let initial_context = sess.build_initial_context(tc.as_ref(), None).await;
sess.record_into_history(&initial_context, tc.as_ref())
.await;
@@ -7742,7 +7764,7 @@ mod tests {
async fn thread_rollback_fails_when_turn_in_progress() {
let (sess, tc, rx) = make_session_and_context_with_rx().await;
let initial_context = sess.build_initial_context(tc.as_ref()).await;
let initial_context = sess.build_initial_context(tc.as_ref(), None).await;
sess.record_into_history(&initial_context, tc.as_ref())
.await;
@@ -7763,7 +7785,7 @@ mod tests {
async fn thread_rollback_fails_when_num_turns_is_zero() {
let (sess, tc, rx) = make_session_and_context_with_rx().await;
let initial_context = sess.build_initial_context(tc.as_ref()).await;
let initial_context = sess.build_initial_context(tc.as_ref(), None).await;
sess.record_into_history(&initial_context, tc.as_ref())
.await;
@@ -8787,7 +8809,7 @@ mod tests {
.record_context_updates_and_set_reference_context_item(&turn_context, None)
.await;
let history = session.clone_history().await;
let initial_context = session.build_initial_context(&turn_context).await;
let initial_context = session.build_initial_context(&turn_context, None).await;
assert_eq!(history.raw_items().to_vec(), initial_context);
let current_context = session.reference_context_item().await;
@@ -8831,10 +8853,28 @@ mod tests {
let history = session.clone_history().await;
let mut expected_history = vec![compacted_summary];
expected_history.extend(session.build_initial_context(&turn_context).await);
expected_history.extend(session.build_initial_context(&turn_context, None).await);
assert_eq!(history.raw_items().to_vec(), expected_history);
}
#[tokio::test]
async fn build_initial_context_prepends_model_switch_message() {
let (session, turn_context) = make_session_and_context().await;
let initial_context = session
.build_initial_context(&turn_context, Some("previous-regular-model"))
.await;
let ResponseItem::Message { role, content, .. } = &initial_context[0] else {
panic!("expected developer message");
};
assert_eq!(role, "developer");
let [ContentItem::InputText { text }, ..] = content.as_slice() else {
panic!("expected developer text");
};
assert!(text.contains("<model_switch>"));
}
#[tokio::test]
async fn run_user_shell_command_does_not_set_reference_context_item() {
let (session, _turn_context, rx) = make_session_and_context_with_rx().await;
@@ -9156,7 +9196,7 @@ mod tests {
let ContentItem::InputText { text } = content_item else {
return false;
};
text.contains(crate::session_prefix::TURN_ABORTED_OPEN_TAG)
text.contains(crate::contextual_user_message::TURN_ABORTED_OPEN_TAG)
})
}),
"expected a model-visible turn aborted marker in history after interrupt"
@@ -9230,7 +9270,7 @@ mod tests {
// personality_spec) matches reconstruction.
let reconstruction_turn = session.new_default_turn().await;
let mut initial_context = session
.build_initial_context(reconstruction_turn.as_ref())
.build_initial_context(reconstruction_turn.as_ref(), None)
.await;
// Ensure personality_spec is present when Personality is enabled, so expected matches
// what reconstruction produces (build_initial_context may omit it when baked into model).