mirror of
https://github.com/openai/codex.git
synced 2026-11-12 14:41:42 +00:00
fix(core) Preserve base_instructions in SessionMeta (#9427)
## Summary This PR consolidates base_instructions onto SessionMeta / SessionConfiguration, so we ensure `base_instructions` is set once per session and should be (mostly) immutable, unless: - overridden by config on resume / fork - sub-agent tasks, like review or collab In a future PR, we should convert all references to `base_instructions` to consistently used the typed struct, so it's less likely that we put other strings there. See #9423. However, this PR is already quite complex, so I'm deferring that to a follow-up. ## Testing - [x] Added a resume test to assert that instructions are preserved. In particular, `resume_switches_models_preserves_base_instructions` fails against main. Existing test coverage thats assert base instructions are preserved across multiple requests in a session: - Manual compact keeps baseline instructions: core/tests/suite/compact.rs:199 - Auto-compact keeps baseline instructions: core/tests/suite/compact.rs:1142 - Prompt caching reuses the same instructions across two requests: core/tests/suite/prompt_caching.rs:150 and core/tests/suite/prompt_caching.rs:157 - Prompt caching with explicit expected string across two requests: core/tests/suite/prompt_caching.rs:213 and core/tests/suite/prompt_caching.rs:222 - Resume with model switch keeps original instructions: core/tests/suite/resume.rs:136 - Compact/resume/fork uses request 0 instructions for later expected payloads: core/tests/suite/compact_resume_fork.rs:215
This commit is contained in:
1 parent
65d3b9e145
commit
675f165c56
17 files changed
+700
-116
No files matched your search
@@ -9,6 +9,7 @@ use std::path::PathBuf;
|
||||
use std::time::SystemTime;
|
||||
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::BaseInstructions;
|
||||
use serde_json::Value;
|
||||
use time::OffsetDateTime;
|
||||
use time::format_description::FormatItem;
|
||||
@@ -59,6 +60,7 @@ pub enum RolloutRecorderParams {
|
||||
conversation_id: ThreadId,
|
||||
forked_from_id: Option<ThreadId>,
|
||||
source: SessionSource,
|
||||
base_instructions: BaseInstructions,
|
||||
},
|
||||
Resume {
|
||||
path: PathBuf,
|
||||
@@ -81,11 +83,13 @@ impl RolloutRecorderParams {
|
||||
conversation_id: ThreadId,
|
||||
forked_from_id: Option<ThreadId>,
|
||||
source: SessionSource,
|
||||
base_instructions: BaseInstructions,
|
||||
) -> Self {
|
||||
Self::Create {
|
||||
conversation_id,
|
||||
forked_from_id,
|
||||
source,
|
||||
base_instructions,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -160,6 +164,7 @@ impl RolloutRecorder {
|
||||
conversation_id,
|
||||
forked_from_id,
|
||||
source,
|
||||
base_instructions,
|
||||
} => {
|
||||
let LogFileInfo {
|
||||
file,
|
||||
@@ -188,6 +193,7 @@ impl RolloutRecorder {
|
||||
cli_version: env!("CARGO_PKG_VERSION").to_string(),
|
||||
source,
|
||||
model_provider: Some(config.model_provider_id.clone()),
|
||||
base_instructions: Some(base_instructions),
|
||||
}),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ use std::fs::{self};
|
||||
use std::io::Write;
|
||||
use std::path::Path;
|
||||
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use time::Duration;
|
||||
use time::OffsetDateTime;
|
||||
@@ -89,6 +90,7 @@ fn write_session_file_with_provider(
|
||||
"cwd": ".",
|
||||
"originator": "test_originator",
|
||||
"cli_version": "test_version",
|
||||
"base_instructions": null,
|
||||
});
|
||||
|
||||
if let Some(source) = source {
|
||||
@@ -129,6 +131,48 @@ fn write_session_file_with_provider(
|
||||
Ok((dt, uuid))
|
||||
}
|
||||
|
||||
fn write_session_file_with_meta_payload(
|
||||
root: &Path,
|
||||
ts_str: &str,
|
||||
uuid: Uuid,
|
||||
payload: serde_json::Value,
|
||||
) -> std::io::Result<()> {
|
||||
let format: &[FormatItem] =
|
||||
format_description!("[year]-[month]-[day]T[hour]-[minute]-[second]");
|
||||
let dt = PrimitiveDateTime::parse(ts_str, format)
|
||||
.unwrap()
|
||||
.assume_utc();
|
||||
let dir = root
|
||||
.join("sessions")
|
||||
.join(format!("{:04}", dt.year()))
|
||||
.join(format!("{:02}", u8::from(dt.month())))
|
||||
.join(format!("{:02}", dt.day()));
|
||||
fs::create_dir_all(&dir)?;
|
||||
|
||||
let filename = format!("rollout-{ts_str}-{uuid}.jsonl");
|
||||
let file_path = dir.join(filename);
|
||||
let mut file = File::create(file_path)?;
|
||||
|
||||
let meta = serde_json::json!({
|
||||
"timestamp": ts_str,
|
||||
"type": "session_meta",
|
||||
"payload": payload,
|
||||
});
|
||||
writeln!(file, "{meta}")?;
|
||||
|
||||
let user_event = serde_json::json!({
|
||||
"timestamp": ts_str,
|
||||
"type": "event_msg",
|
||||
"payload": {"type": "user_message", "message": "Hello from user", "kind": "plain"}
|
||||
});
|
||||
writeln!(file, "{user_event}")?;
|
||||
|
||||
let times = FileTimes::new().set_modified(dt.into());
|
||||
file.set_times(times)?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_list_conversations_latest_first() {
|
||||
let temp = TempDir::new().unwrap();
|
||||
@@ -206,6 +250,7 @@ async fn test_list_conversations_latest_first() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let head_2 = vec![serde_json::json!({
|
||||
"id": u2,
|
||||
@@ -215,6 +260,7 @@ async fn test_list_conversations_latest_first() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let head_1 = vec![serde_json::json!({
|
||||
"id": u1,
|
||||
@@ -224,6 +270,7 @@ async fn test_list_conversations_latest_first() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
|
||||
let updated_times: Vec<Option<String>> =
|
||||
@@ -344,6 +391,7 @@ async fn test_pagination_cursor() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let head_4 = vec![serde_json::json!({
|
||||
"id": u4,
|
||||
@@ -353,6 +401,7 @@ async fn test_pagination_cursor() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let updated_page1: Vec<Option<String>> =
|
||||
page1.items.iter().map(|i| i.updated_at.clone()).collect();
|
||||
@@ -410,6 +459,7 @@ async fn test_pagination_cursor() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let head_2 = vec![serde_json::json!({
|
||||
"id": u2,
|
||||
@@ -419,6 +469,7 @@ async fn test_pagination_cursor() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let updated_page2: Vec<Option<String>> =
|
||||
page2.items.iter().map(|i| i.updated_at.clone()).collect();
|
||||
@@ -470,6 +521,7 @@ async fn test_pagination_cursor() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let updated_page3: Vec<Option<String>> =
|
||||
page3.items.iter().map(|i| i.updated_at.clone()).collect();
|
||||
@@ -527,6 +579,7 @@ async fn test_get_thread_contents() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})];
|
||||
let expected_page = ThreadsPage {
|
||||
items: vec![ThreadItem {
|
||||
@@ -551,6 +604,7 @@ async fn test_get_thread_contents() {
|
||||
"cwd": ".",
|
||||
"originator": "test_originator",
|
||||
"cli_version": "test_version",
|
||||
"base_instructions": null,
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
}
|
||||
@@ -566,6 +620,93 @@ async fn test_get_thread_contents() {
|
||||
assert_eq!(content, expected_content);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_base_instructions_missing_in_meta_defaults_to_null() {
|
||||
let temp = TempDir::new().unwrap();
|
||||
let home = temp.path();
|
||||
|
||||
let ts = "2025-04-02T10-30-00";
|
||||
let uuid = Uuid::from_u128(101);
|
||||
let payload = serde_json::json!({
|
||||
"id": uuid,
|
||||
"timestamp": ts,
|
||||
"cwd": ".",
|
||||
"originator": "test_originator",
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
});
|
||||
write_session_file_with_meta_payload(home, ts, uuid, payload).unwrap();
|
||||
|
||||
let provider_filter = provider_vec(&[TEST_PROVIDER]);
|
||||
let page = get_threads(
|
||||
home,
|
||||
1,
|
||||
None,
|
||||
ThreadSortKey::CreatedAt,
|
||||
INTERACTIVE_SESSION_SOURCES,
|
||||
Some(provider_filter.as_slice()),
|
||||
TEST_PROVIDER,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let head = page
|
||||
.items
|
||||
.first()
|
||||
.and_then(|item| item.head.first())
|
||||
.expect("session meta head");
|
||||
assert_eq!(
|
||||
head.get("base_instructions"),
|
||||
Some(&serde_json::Value::Null)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_base_instructions_present_in_meta_is_preserved() {
|
||||
let temp = TempDir::new().unwrap();
|
||||
let home = temp.path();
|
||||
|
||||
let ts = "2025-04-03T10-30-00";
|
||||
let uuid = Uuid::from_u128(102);
|
||||
let base_text = "Custom base instructions";
|
||||
let payload = serde_json::json!({
|
||||
"id": uuid,
|
||||
"timestamp": ts,
|
||||
"cwd": ".",
|
||||
"originator": "test_originator",
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": {"text": base_text},
|
||||
});
|
||||
write_session_file_with_meta_payload(home, ts, uuid, payload).unwrap();
|
||||
|
||||
let provider_filter = provider_vec(&[TEST_PROVIDER]);
|
||||
let page = get_threads(
|
||||
home,
|
||||
1,
|
||||
None,
|
||||
ThreadSortKey::CreatedAt,
|
||||
INTERACTIVE_SESSION_SOURCES,
|
||||
Some(provider_filter.as_slice()),
|
||||
TEST_PROVIDER,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let head = page
|
||||
.items
|
||||
.first()
|
||||
.and_then(|item| item.head.first())
|
||||
.expect("session meta head");
|
||||
let base = head
|
||||
.get("base_instructions")
|
||||
.and_then(|value| value.get("text"))
|
||||
.and_then(serde_json::Value::as_str);
|
||||
assert_eq!(base, Some(base_text));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_created_at_sort_uses_file_mtime_for_updated_at() -> Result<()> {
|
||||
let temp = TempDir::new().unwrap();
|
||||
@@ -637,6 +778,7 @@ async fn test_updated_at_uses_file_mtime() -> Result<()> {
|
||||
cli_version: "test_version".into(),
|
||||
source: SessionSource::VSCode,
|
||||
model_provider: Some("test-provider".into()),
|
||||
base_instructions: None,
|
||||
},
|
||||
git: None,
|
||||
}),
|
||||
@@ -744,6 +886,7 @@ async fn test_stable_ordering_same_second_pagination() {
|
||||
"cli_version": "test_version",
|
||||
"source": "vscode",
|
||||
"model_provider": "test-provider",
|
||||
"base_instructions": null,
|
||||
})]
|
||||
};
|
||||
let updated_page1: Vec<Option<String>> =
|
||||
|
||||
Reference in new issue
Block a user