mirror of
https://github.com/openai/codex.git
synced 2026-11-10 08:43:33 +00:00
Move message history out of core (#21278)
## Why Message history was implemented inside `codex-core` and surfaced through core protocol ops and `SessionConfiguredEvent` fields even though the current consumer is TUI-local prompt recall. That made core own UI history persistence and exposed `history_log_id` / `history_entry_count` through surfaces that app-server and other clients do not need. This change moves message history persistence out of core and keeps the recall plumbing local to the TUI. ## What changed - Added a new `codex-message-history` crate for appending, looking up, trimming, and reading metadata from `history.jsonl`. - Removed core protocol history ops/events: `AddToHistory`, `GetHistoryEntryRequest`, and `GetHistoryEntryResponse`. - Removed `history_log_id` and `history_entry_count` from `SessionConfiguredEvent` and updated exec/MCP/test fixtures accordingly. - Updated the TUI to dispatch local app events for message-history append/lookup and keep its persistent-history metadata in TUI session state. ## Validation - `cargo test -p codex-message-history -p codex-protocol` - `cargo test -p codex-exec event_processor_with_json_output` - `cargo test -p codex-mcp-server outgoing_message` - `cargo test -p codex-tui` - `just fix -p codex-message-history -p codex-protocol -p codex-core -p codex-tui -p codex-exec -p codex-mcp-server`
This commit is contained in:
1 parent
be1d3cff93
commit
2004173cd7
45 files changed
+375
-492
No files matched your search
@@ -648,8 +648,7 @@ mod tests {
|
||||
cwd: next_cwd.clone().abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
});
|
||||
|
||||
@@ -326,6 +326,17 @@ impl App {
|
||||
AppEvent::CodexOp(op) => {
|
||||
self.submit_active_thread_op(app_server, op).await?;
|
||||
}
|
||||
AppEvent::AppendMessageHistoryEntry { thread_id, text } => {
|
||||
self.append_message_history_entry(thread_id, text);
|
||||
}
|
||||
AppEvent::LookupMessageHistoryEntry {
|
||||
thread_id,
|
||||
offset,
|
||||
log_id,
|
||||
} => {
|
||||
self.lookup_message_history_entry(thread_id, offset, log_id)
|
||||
.await?;
|
||||
}
|
||||
AppEvent::ApproveRecentAutoReviewDenial { thread_id, id } => {
|
||||
self.chat_widget
|
||||
.approve_recent_auto_review_denial(thread_id, id);
|
||||
|
||||
@@ -550,18 +550,9 @@ async fn history_lookup_response_is_routed_to_requesting_thread() -> Result<()>
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
let handled = app
|
||||
.try_handle_local_history_op(
|
||||
thread_id,
|
||||
&Op::GetHistoryEntryRequest {
|
||||
offset: 0,
|
||||
log_id: 1,
|
||||
},
|
||||
)
|
||||
app.lookup_message_history_entry(thread_id, /*offset*/ 0, /*log_id*/ 1)
|
||||
.await?;
|
||||
|
||||
assert!(handled);
|
||||
|
||||
let app_event = tokio::time::timeout(Duration::from_secs(1), app_event_rx.recv())
|
||||
.await
|
||||
.expect("history lookup should emit an app event")
|
||||
@@ -3751,8 +3742,7 @@ async fn render_clear_ui_header_after_long_transcript_for_snapshot() -> String {
|
||||
cwd: test_path_buf("/tmp/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: Some(ReasoningEffortConfig::High),
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
};
|
||||
@@ -3997,8 +3987,7 @@ fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
}
|
||||
@@ -4543,8 +4532,7 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() {
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
};
|
||||
@@ -4607,8 +4595,7 @@ async fn backtrack_selection_with_duplicate_history_targets_unique_turn() {
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
});
|
||||
@@ -4700,8 +4687,7 @@ async fn backtrack_resubmit_preserves_data_image_urls_in_user_turn() {
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
});
|
||||
@@ -5099,8 +5085,7 @@ async fn new_session_requests_shutdown_for_previous_conversation() {
|
||||
cwd: test_path_buf("/home/user/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
};
|
||||
@@ -5221,8 +5206,7 @@ async fn clear_only_ui_reset_preserves_chat_session_state() {
|
||||
cwd: test_path_buf("/tmp/project").abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
});
|
||||
|
||||
@@ -354,8 +354,7 @@ mod tests {
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
}
|
||||
|
||||
@@ -413,10 +413,6 @@ impl App {
|
||||
) -> Result<()> {
|
||||
crate::session_log::log_outbound_op(&op);
|
||||
|
||||
if self.try_handle_local_history_op(thread_id, &op).await? {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if self
|
||||
.try_resolve_app_server_request(app_server, thread_id, &op)
|
||||
.await?
|
||||
@@ -441,70 +437,57 @@ impl App {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Spawn a background task that fetches MCP server status from the app-server
|
||||
/// via paginated RPCs, then delivers the result back through
|
||||
/// `AppEvent::McpInventoryLoaded`.
|
||||
///
|
||||
/// The spawned task is fire-and-forget: no `JoinHandle` is stored, so a stale
|
||||
/// result may arrive after the user has moved on. We currently accept that
|
||||
/// tradeoff because the effect is limited to stale inventory output in history,
|
||||
/// while request-token invalidation would add cross-cutting async state for a
|
||||
/// low-severity path.
|
||||
pub(super) async fn try_handle_local_history_op(
|
||||
/// Persist prompt text in the local cross-session message history.
|
||||
pub(super) fn append_message_history_entry(&self, thread_id: ThreadId, text: String) {
|
||||
let history_config = codex_message_history::HistoryConfig::new(
|
||||
self.chat_widget.config_ref().codex_home.clone(),
|
||||
&self.chat_widget.config_ref().history,
|
||||
);
|
||||
tokio::spawn(async move {
|
||||
if let Err(err) =
|
||||
codex_message_history::append_entry(&text, thread_id, &history_config).await
|
||||
{
|
||||
tracing::warn!(
|
||||
thread_id = %thread_id,
|
||||
error = %err,
|
||||
"failed to append to message history"
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/// Fetch one local cross-session message history entry for the requesting thread.
|
||||
pub(super) async fn lookup_message_history_entry(
|
||||
&mut self,
|
||||
thread_id: ThreadId,
|
||||
op: &AppCommand,
|
||||
) -> Result<bool> {
|
||||
match op {
|
||||
AppCommand::AddToHistory { text } => {
|
||||
let text = text.to_string();
|
||||
let config = self.chat_widget.config_ref().clone();
|
||||
tokio::spawn(async move {
|
||||
if let Err(err) = append_message_history_entry(&text, &thread_id, &config).await
|
||||
{
|
||||
tracing::warn!(
|
||||
thread_id = %thread_id,
|
||||
error = %err,
|
||||
"failed to append to message history"
|
||||
);
|
||||
}
|
||||
});
|
||||
Ok(true)
|
||||
}
|
||||
AppCommand::GetHistoryEntryRequest { offset, log_id } => {
|
||||
let config = self.chat_widget.config_ref().clone();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
let offset = *offset;
|
||||
let log_id = *log_id;
|
||||
tokio::spawn(async move {
|
||||
let entry_opt = tokio::task::spawn_blocking(move || {
|
||||
lookup_message_history_entry(log_id, offset, &config)
|
||||
})
|
||||
.await
|
||||
.unwrap_or_else(|err| {
|
||||
tracing::warn!(error = %err, "history lookup task failed");
|
||||
None
|
||||
});
|
||||
offset: usize,
|
||||
log_id: u64,
|
||||
) -> Result<()> {
|
||||
let history_config = codex_message_history::HistoryConfig::new(
|
||||
self.chat_widget.config_ref().codex_home.clone(),
|
||||
&self.chat_widget.config_ref().history,
|
||||
);
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
tokio::spawn(async move {
|
||||
let entry_opt = tokio::task::spawn_blocking(move || {
|
||||
codex_message_history::lookup(log_id, offset, &history_config)
|
||||
})
|
||||
.await
|
||||
.unwrap_or_else(|err| {
|
||||
tracing::warn!(error = %err, "history lookup task failed");
|
||||
None
|
||||
});
|
||||
|
||||
app_event_tx.send(AppEvent::ThreadHistoryEntryResponse {
|
||||
thread_id,
|
||||
event: HistoryLookupResponse {
|
||||
offset,
|
||||
log_id,
|
||||
entry: entry_opt.map(|entry| {
|
||||
codex_protocol::message_history::HistoryEntry {
|
||||
conversation_id: entry.session_id,
|
||||
ts: entry.ts,
|
||||
text: entry.text,
|
||||
}
|
||||
}),
|
||||
},
|
||||
});
|
||||
});
|
||||
Ok(true)
|
||||
}
|
||||
_ => Ok(false),
|
||||
}
|
||||
app_event_tx.send(AppEvent::ThreadHistoryEntryResponse {
|
||||
thread_id,
|
||||
event: HistoryLookupResponse {
|
||||
offset,
|
||||
log_id,
|
||||
entry: entry_opt.map(|entry| entry.text),
|
||||
},
|
||||
});
|
||||
});
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(super) async fn try_submit_active_thread_op_via_app_server(
|
||||
@@ -941,8 +924,7 @@ impl App {
|
||||
} else if rollout_path.is_some() {
|
||||
session.model.clear();
|
||||
}
|
||||
session.history_log_id = 0;
|
||||
session.history_entry_count = 0;
|
||||
session.message_history = None;
|
||||
session.rollout_path = rollout_path;
|
||||
self.upsert_agent_picker_thread(
|
||||
thread_id,
|
||||
|
||||
@@ -76,8 +76,7 @@ impl App {
|
||||
cwd: thread.cwd.clone(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: self.chat_widget.current_reasoning_effort(),
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: thread.path.clone(),
|
||||
});
|
||||
@@ -96,8 +95,7 @@ impl App {
|
||||
} else if thread.path.is_some() {
|
||||
session.model.clear();
|
||||
}
|
||||
session.history_log_id = 0;
|
||||
session.history_entry_count = 0;
|
||||
session.message_history = None;
|
||||
session
|
||||
}
|
||||
|
||||
@@ -153,8 +151,7 @@ mod tests {
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
message_history: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user