mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(app-server): experimental flag to persist extended history (#11227)
This PR adds an experimental `persist_extended_history` bool flag to app-server thread APIs so rollout logs can retain a richer set of EventMsgs for non-lossy Thread > Turn > ThreadItems reconstruction (i.e. on `thread/resume`). ### Motivation Today, our rollout recorder only persists a small subset (e.g. user message, reasoning, assistant message) of `EventMsg` types, dropping a good number (like command exec, file change, etc.) that are important for reconstructing full item history for `thread/resume`, `thread/read`, and `thread/fork`. Some clients want to be able to resume a thread without lossiness. This lossiness is primarily a UI thing, since what the model sees are `ResponseItem` and not `EventMsg`. ### Approach This change introduces an opt-in `persist_full_history` flag to preserve those events when you start/resume/fork a thread (defaults to `false`). This is done by adding an `EventPersistenceMode` to the rollout recorder: - `Limited` (existing behavior, default) - `Extended` (new opt-in behavior) In `Extended` mode, persist additional `EventMsg` variants needed for non-lossy app-server `ThreadItem` reconstruction. We now store the following ThreadItems that we didn't before: - web search - command execution - patch/file changes - MCP tool calls - image view calls - collab tool outcomes - context compaction - review mode enter/exit For **command executions** in particular, we truncate the output using the existing `truncate_text` from core to store an upper bound of 10,000 bytes, which is also the default value for truncating tool outputs shown to the model. This keeps the size of the rollout file and command execution items returned over the wire reasonable. And we also persist `EventMsg::Error` which we can now map back to the Turn's status and populates the Turn's error metadata. #### Updates to EventMsgs To truly make `thread/resume` non-lossy, we also needed to persist the `status` on `EventMsg::CommandExecutionEndEvent` and `EventMsg::PatchApplyEndEvent`. Previously it was not obvious whether a command failed or was declined (similar for apply_patch). These EventMsgs were never persisted before so I made it a required field.
This commit is contained in:
committed by
GitHub
Unverified
parent
22fa283511
commit
efc8d45750
@@ -50,7 +50,7 @@ impl AgentControl {
|
||||
let new_thread = match session_source {
|
||||
Some(session_source) => {
|
||||
state
|
||||
.spawn_new_thread_with_source(config, self.clone(), session_source)
|
||||
.spawn_new_thread_with_source(config, self.clone(), session_source, false)
|
||||
.await?
|
||||
}
|
||||
None => state.spawn_new_thread(config, self.clone()).await?,
|
||||
|
||||
@@ -196,6 +196,7 @@ use crate::rollout::RolloutRecorder;
|
||||
use crate::rollout::RolloutRecorderParams;
|
||||
use crate::rollout::map_session_init_error;
|
||||
use crate::rollout::metadata;
|
||||
use crate::rollout::policy::EventPersistenceMode;
|
||||
use crate::shell;
|
||||
use crate::shell_snapshot::ShellSnapshot;
|
||||
use crate::skills::SkillError;
|
||||
@@ -284,6 +285,7 @@ impl Codex {
|
||||
session_source: SessionSource,
|
||||
agent_control: AgentControl,
|
||||
dynamic_tools: Vec<DynamicToolSpec>,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<CodexSpawnOk> {
|
||||
let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
let (tx_event, rx_event) = async_channel::unbounded();
|
||||
@@ -396,6 +398,7 @@ impl Codex {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source,
|
||||
dynamic_tools,
|
||||
persist_extended_history,
|
||||
};
|
||||
|
||||
// Generate a unique ID for the lifetime of this Codex session.
|
||||
@@ -733,6 +736,7 @@ pub(crate) struct SessionConfiguration {
|
||||
/// Source of the session (cli, vscode, exec, mcp, ...)
|
||||
session_source: SessionSource,
|
||||
dynamic_tools: Vec<DynamicToolSpec>,
|
||||
persist_extended_history: bool,
|
||||
}
|
||||
|
||||
impl SessionConfiguration {
|
||||
@@ -984,12 +988,24 @@ impl Session {
|
||||
text: session_configuration.base_instructions.clone(),
|
||||
},
|
||||
session_configuration.dynamic_tools.clone(),
|
||||
if session_configuration.persist_extended_history {
|
||||
EventPersistenceMode::Extended
|
||||
} else {
|
||||
EventPersistenceMode::Limited
|
||||
},
|
||||
),
|
||||
)
|
||||
}
|
||||
InitialHistory::Resumed(resumed_history) => (
|
||||
resumed_history.conversation_id,
|
||||
RolloutRecorderParams::resume(resumed_history.rollout_path.clone()),
|
||||
RolloutRecorderParams::resume(
|
||||
resumed_history.rollout_path.clone(),
|
||||
if session_configuration.persist_extended_history {
|
||||
EventPersistenceMode::Extended
|
||||
} else {
|
||||
EventPersistenceMode::Limited
|
||||
},
|
||||
),
|
||||
),
|
||||
};
|
||||
let state_builder = match &initial_history {
|
||||
@@ -6297,6 +6313,7 @@ mod tests {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
};
|
||||
|
||||
let mut state = SessionState::new(session_configuration);
|
||||
@@ -6387,6 +6404,7 @@ mod tests {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
};
|
||||
|
||||
let mut state = SessionState::new(session_configuration);
|
||||
@@ -6696,6 +6714,7 @@ mod tests {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6748,6 +6767,7 @@ mod tests {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
};
|
||||
let per_turn_config = Session::build_per_turn_config(&session_configuration);
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(
|
||||
@@ -6893,6 +6913,7 @@ mod tests {
|
||||
original_config_do_not_use: Arc::clone(&config),
|
||||
session_source: SessionSource::Exec,
|
||||
dynamic_tools: Vec::new(),
|
||||
persist_extended_history: false,
|
||||
};
|
||||
let per_turn_config = Session::build_per_turn_config(&session_configuration);
|
||||
let model_info = ModelsManager::construct_model_info_offline_for_tests(
|
||||
|
||||
@@ -58,6 +58,7 @@ pub(crate) async fn run_codex_thread_interactive(
|
||||
SessionSource::SubAgent(SubAgentSource::Review),
|
||||
parent_session.services.agent_control.clone(),
|
||||
Vec::new(),
|
||||
false,
|
||||
)
|
||||
.await?;
|
||||
let codex = Arc::new(codex);
|
||||
|
||||
@@ -124,6 +124,7 @@ pub use rollout::list::ThreadsPage;
|
||||
pub use rollout::list::parse_cursor;
|
||||
pub use rollout::list::read_head_for_summary;
|
||||
pub use rollout::list::read_session_meta_line;
|
||||
pub use rollout::policy::EventPersistenceMode;
|
||||
pub use rollout::rollout_date_parts;
|
||||
pub use rollout::session_index::find_thread_names_by_ids;
|
||||
mod function_tool;
|
||||
|
||||
@@ -2,12 +2,20 @@ use crate::protocol::EventMsg;
|
||||
use crate::protocol::RolloutItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
|
||||
/// Whether a rollout `item` should be persisted in rollout files.
|
||||
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
|
||||
pub enum EventPersistenceMode {
|
||||
#[default]
|
||||
Limited,
|
||||
Extended,
|
||||
}
|
||||
|
||||
/// Whether a rollout `item` should be persisted in rollout files for the
|
||||
/// provided persistence `mode`.
|
||||
#[inline]
|
||||
pub(crate) fn is_persisted_response_item(item: &RolloutItem) -> bool {
|
||||
pub(crate) fn is_persisted_response_item(item: &RolloutItem, mode: EventPersistenceMode) -> bool {
|
||||
match item {
|
||||
RolloutItem::ResponseItem(item) => should_persist_response_item(item),
|
||||
RolloutItem::EventMsg(ev) => should_persist_event_msg(ev),
|
||||
RolloutItem::EventMsg(ev) => should_persist_event_msg(ev, mode),
|
||||
// Persist Codex executive markers so we can analyze flows (e.g., compaction, API turns).
|
||||
RolloutItem::Compacted(_) | RolloutItem::TurnContext(_) | RolloutItem::SessionMeta(_) => {
|
||||
true
|
||||
@@ -51,9 +59,33 @@ pub(crate) fn should_persist_response_item_for_memories(item: &ResponseItem) ->
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether an `EventMsg` should be persisted in rollout files.
|
||||
/// Whether an `EventMsg` should be persisted in rollout files for the
|
||||
/// provided persistence `mode`.
|
||||
#[inline]
|
||||
pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
pub(crate) fn should_persist_event_msg(ev: &EventMsg, mode: EventPersistenceMode) -> bool {
|
||||
match mode {
|
||||
EventPersistenceMode::Limited => should_persist_event_msg_limited(ev),
|
||||
EventPersistenceMode::Extended => should_persist_event_msg_extended(ev),
|
||||
}
|
||||
}
|
||||
|
||||
fn should_persist_event_msg_limited(ev: &EventMsg) -> bool {
|
||||
matches!(
|
||||
event_msg_persistence_mode(ev),
|
||||
Some(EventPersistenceMode::Limited)
|
||||
)
|
||||
}
|
||||
|
||||
fn should_persist_event_msg_extended(ev: &EventMsg) -> bool {
|
||||
matches!(
|
||||
event_msg_persistence_mode(ev),
|
||||
Some(EventPersistenceMode::Limited) | Some(EventPersistenceMode::Extended)
|
||||
)
|
||||
}
|
||||
|
||||
/// Returns the minimum persistence mode that includes this event.
|
||||
/// `None` means the event should never be persisted.
|
||||
fn event_msg_persistence_mode(ev: &EventMsg) -> Option<EventPersistenceMode> {
|
||||
match ev {
|
||||
EventMsg::UserMessage(_)
|
||||
| EventMsg::AgentMessage(_)
|
||||
@@ -67,15 +99,29 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::UndoCompleted(_)
|
||||
| EventMsg::TurnAborted(_)
|
||||
| EventMsg::TurnStarted(_)
|
||||
| EventMsg::TurnComplete(_) => true,
|
||||
| EventMsg::TurnComplete(_) => Some(EventPersistenceMode::Limited),
|
||||
EventMsg::ItemCompleted(event) => {
|
||||
// Plan items are derived from streaming tags and are not part of the
|
||||
// raw ResponseItem history, so we persist their completion to replay
|
||||
// them on resume without bloating rollouts with every item lifecycle.
|
||||
matches!(event.item, codex_protocol::items::TurnItem::Plan(_))
|
||||
if matches!(event.item, codex_protocol::items::TurnItem::Plan(_)) {
|
||||
Some(EventPersistenceMode::Limited)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
EventMsg::Error(_)
|
||||
| EventMsg::Warning(_)
|
||||
| EventMsg::WebSearchEnd(_)
|
||||
| EventMsg::ExecCommandEnd(_)
|
||||
| EventMsg::PatchApplyEnd(_)
|
||||
| EventMsg::McpToolCallEnd(_)
|
||||
| EventMsg::ViewImageToolCall(_)
|
||||
| EventMsg::CollabAgentSpawnEnd(_)
|
||||
| EventMsg::CollabAgentInteractionEnd(_)
|
||||
| EventMsg::CollabWaitingEnd(_)
|
||||
| EventMsg::CollabCloseEnd(_)
|
||||
| EventMsg::CollabResumeEnd(_) => Some(EventPersistenceMode::Extended),
|
||||
EventMsg::Warning(_)
|
||||
| EventMsg::AgentMessageDelta(_)
|
||||
| EventMsg::AgentReasoningDelta(_)
|
||||
| EventMsg::AgentReasoningRawContentDelta(_)
|
||||
@@ -84,13 +130,10 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::SessionConfigured(_)
|
||||
| EventMsg::ThreadNameUpdated(_)
|
||||
| EventMsg::McpToolCallBegin(_)
|
||||
| EventMsg::McpToolCallEnd(_)
|
||||
| EventMsg::WebSearchBegin(_)
|
||||
| EventMsg::WebSearchEnd(_)
|
||||
| EventMsg::ExecCommandBegin(_)
|
||||
| EventMsg::TerminalInteraction(_)
|
||||
| EventMsg::ExecCommandOutputDelta(_)
|
||||
| EventMsg::ExecCommandEnd(_)
|
||||
| EventMsg::ExecApprovalRequest(_)
|
||||
| EventMsg::RequestUserInput(_)
|
||||
| EventMsg::DynamicToolCallRequest(_)
|
||||
@@ -99,7 +142,6 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::BackgroundEvent(_)
|
||||
| EventMsg::StreamError(_)
|
||||
| EventMsg::PatchApplyBegin(_)
|
||||
| EventMsg::PatchApplyEnd(_)
|
||||
| EventMsg::TurnDiff(_)
|
||||
| EventMsg::GetHistoryEntryResponse(_)
|
||||
| EventMsg::UndoStarted(_)
|
||||
@@ -112,7 +154,6 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::RemoteSkillDownloaded(_)
|
||||
| EventMsg::PlanUpdate(_)
|
||||
| EventMsg::ShutdownComplete
|
||||
| EventMsg::ViewImageToolCall(_)
|
||||
| EventMsg::DeprecationNotice(_)
|
||||
| EventMsg::ItemStarted(_)
|
||||
| EventMsg::AgentMessageContentDelta(_)
|
||||
@@ -121,14 +162,9 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool {
|
||||
| EventMsg::ReasoningRawContentDelta(_)
|
||||
| EventMsg::SkillsUpdateAvailable
|
||||
| EventMsg::CollabAgentSpawnBegin(_)
|
||||
| EventMsg::CollabAgentSpawnEnd(_)
|
||||
| EventMsg::CollabAgentInteractionBegin(_)
|
||||
| EventMsg::CollabAgentInteractionEnd(_)
|
||||
| EventMsg::CollabWaitingBegin(_)
|
||||
| EventMsg::CollabWaitingEnd(_)
|
||||
| EventMsg::CollabCloseBegin(_)
|
||||
| EventMsg::CollabCloseEnd(_)
|
||||
| EventMsg::CollabResumeBegin(_)
|
||||
| EventMsg::CollabResumeEnd(_) => false,
|
||||
| EventMsg::CollabResumeBegin(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,6 +36,7 @@ use super::list::get_threads_in_root;
|
||||
use super::list::parse_cursor;
|
||||
use super::list::parse_timestamp_uuid_from_filename;
|
||||
use super::metadata;
|
||||
use super::policy::EventPersistenceMode;
|
||||
use super::policy::is_persisted_response_item;
|
||||
use crate::config::Config;
|
||||
use crate::default_client::originator;
|
||||
@@ -43,6 +44,9 @@ use crate::git_info::collect_git_info;
|
||||
use crate::path_utils;
|
||||
use crate::state_db;
|
||||
use crate::state_db::StateDbHandle;
|
||||
use crate::truncate::TruncationPolicy;
|
||||
use crate::truncate::truncate_text;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::InitialHistory;
|
||||
use codex_protocol::protocol::ResumedHistory;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
@@ -67,6 +71,7 @@ pub struct RolloutRecorder {
|
||||
tx: Sender<RolloutCmd>,
|
||||
pub(crate) rollout_path: PathBuf,
|
||||
state_db: Option<StateDbHandle>,
|
||||
event_persistence_mode: EventPersistenceMode,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
@@ -77,9 +82,11 @@ pub enum RolloutRecorderParams {
|
||||
source: SessionSource,
|
||||
base_instructions: BaseInstructions,
|
||||
dynamic_tools: Vec<DynamicToolSpec>,
|
||||
event_persistence_mode: EventPersistenceMode,
|
||||
},
|
||||
Resume {
|
||||
path: PathBuf,
|
||||
event_persistence_mode: EventPersistenceMode,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -104,6 +111,7 @@ impl RolloutRecorderParams {
|
||||
source: SessionSource,
|
||||
base_instructions: BaseInstructions,
|
||||
dynamic_tools: Vec<DynamicToolSpec>,
|
||||
event_persistence_mode: EventPersistenceMode,
|
||||
) -> Self {
|
||||
Self::Create {
|
||||
conversation_id,
|
||||
@@ -111,11 +119,42 @@ impl RolloutRecorderParams {
|
||||
source,
|
||||
base_instructions,
|
||||
dynamic_tools,
|
||||
event_persistence_mode,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn resume(path: PathBuf) -> Self {
|
||||
Self::Resume { path }
|
||||
pub fn resume(path: PathBuf, event_persistence_mode: EventPersistenceMode) -> Self {
|
||||
Self::Resume {
|
||||
path,
|
||||
event_persistence_mode,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const PERSISTED_EXEC_AGGREGATED_OUTPUT_MAX_BYTES: usize = 10_000;
|
||||
|
||||
fn sanitize_rollout_item_for_persistence(
|
||||
item: RolloutItem,
|
||||
mode: EventPersistenceMode,
|
||||
) -> RolloutItem {
|
||||
if mode != EventPersistenceMode::Extended {
|
||||
return item;
|
||||
}
|
||||
|
||||
match item {
|
||||
RolloutItem::EventMsg(EventMsg::ExecCommandEnd(mut event)) => {
|
||||
// Persist only a bounded aggregated summary of command output.
|
||||
event.aggregated_output = truncate_text(
|
||||
&event.aggregated_output,
|
||||
TruncationPolicy::Bytes(PERSISTED_EXEC_AGGREGATED_OUTPUT_MAX_BYTES),
|
||||
);
|
||||
// Drop unnecessary fields from rollout storage since aggregated_output is all we need.
|
||||
event.stdout.clear();
|
||||
event.stderr.clear();
|
||||
event.formatted_output.clear();
|
||||
RolloutItem::EventMsg(EventMsg::ExecCommandEnd(event))
|
||||
}
|
||||
_ => item,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -322,58 +361,70 @@ impl RolloutRecorder {
|
||||
state_db_ctx: Option<StateDbHandle>,
|
||||
state_builder: Option<ThreadMetadataBuilder>,
|
||||
) -> std::io::Result<Self> {
|
||||
let (file, deferred_log_file_info, rollout_path, meta) = match params {
|
||||
RolloutRecorderParams::Create {
|
||||
conversation_id,
|
||||
forked_from_id,
|
||||
source,
|
||||
base_instructions,
|
||||
dynamic_tools,
|
||||
} => {
|
||||
let log_file_info = precompute_log_file_info(config, conversation_id)?;
|
||||
let path = log_file_info.path.clone();
|
||||
let session_id = log_file_info.conversation_id;
|
||||
let started_at = log_file_info.timestamp;
|
||||
|
||||
let timestamp_format: &[FormatItem] = format_description!(
|
||||
"[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:3]Z"
|
||||
);
|
||||
let timestamp = started_at
|
||||
.to_offset(time::UtcOffset::UTC)
|
||||
.format(timestamp_format)
|
||||
.map_err(|e| IoError::other(format!("failed to format timestamp: {e}")))?;
|
||||
|
||||
let session_meta = SessionMeta {
|
||||
id: session_id,
|
||||
let (file, deferred_log_file_info, rollout_path, meta, event_persistence_mode) =
|
||||
match params {
|
||||
RolloutRecorderParams::Create {
|
||||
conversation_id,
|
||||
forked_from_id,
|
||||
timestamp,
|
||||
cwd: config.cwd.clone(),
|
||||
originator: originator().value,
|
||||
cli_version: env!("CARGO_PKG_VERSION").to_string(),
|
||||
source,
|
||||
model_provider: Some(config.model_provider_id.clone()),
|
||||
base_instructions: Some(base_instructions),
|
||||
dynamic_tools: if dynamic_tools.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(dynamic_tools)
|
||||
},
|
||||
};
|
||||
base_instructions,
|
||||
dynamic_tools,
|
||||
event_persistence_mode,
|
||||
} => {
|
||||
let log_file_info = precompute_log_file_info(config, conversation_id)?;
|
||||
let path = log_file_info.path.clone();
|
||||
let session_id = log_file_info.conversation_id;
|
||||
let started_at = log_file_info.timestamp;
|
||||
|
||||
(None, Some(log_file_info), path, Some(session_meta))
|
||||
}
|
||||
RolloutRecorderParams::Resume { path } => (
|
||||
Some(
|
||||
tokio::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(&path)
|
||||
.await?,
|
||||
let timestamp_format: &[FormatItem] = format_description!(
|
||||
"[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:3]Z"
|
||||
);
|
||||
let timestamp = started_at
|
||||
.to_offset(time::UtcOffset::UTC)
|
||||
.format(timestamp_format)
|
||||
.map_err(|e| IoError::other(format!("failed to format timestamp: {e}")))?;
|
||||
|
||||
let session_meta = SessionMeta {
|
||||
id: session_id,
|
||||
forked_from_id,
|
||||
timestamp,
|
||||
cwd: config.cwd.clone(),
|
||||
originator: originator().value,
|
||||
cli_version: env!("CARGO_PKG_VERSION").to_string(),
|
||||
source,
|
||||
model_provider: Some(config.model_provider_id.clone()),
|
||||
base_instructions: Some(base_instructions),
|
||||
dynamic_tools: if dynamic_tools.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(dynamic_tools)
|
||||
},
|
||||
};
|
||||
|
||||
(
|
||||
None,
|
||||
Some(log_file_info),
|
||||
path,
|
||||
Some(session_meta),
|
||||
event_persistence_mode,
|
||||
)
|
||||
}
|
||||
RolloutRecorderParams::Resume {
|
||||
path,
|
||||
event_persistence_mode,
|
||||
} => (
|
||||
Some(
|
||||
tokio::fs::OpenOptions::new()
|
||||
.append(true)
|
||||
.open(&path)
|
||||
.await?,
|
||||
),
|
||||
None,
|
||||
path,
|
||||
None,
|
||||
event_persistence_mode,
|
||||
),
|
||||
None,
|
||||
path,
|
||||
None,
|
||||
),
|
||||
};
|
||||
};
|
||||
|
||||
// Clone the cwd for the spawned task to collect git info asynchronously
|
||||
let cwd = config.cwd.clone();
|
||||
@@ -402,6 +453,7 @@ impl RolloutRecorder {
|
||||
tx,
|
||||
rollout_path,
|
||||
state_db: state_db_ctx,
|
||||
event_persistence_mode,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -419,8 +471,11 @@ impl RolloutRecorder {
|
||||
// Note that function calls may look a bit strange if they are
|
||||
// "fully qualified MCP tool calls," so we could consider
|
||||
// reformatting them in that case.
|
||||
if is_persisted_response_item(item) {
|
||||
filtered.push(item.clone());
|
||||
if is_persisted_response_item(item, self.event_persistence_mode) {
|
||||
filtered.push(sanitize_rollout_item_for_persistence(
|
||||
item.clone(),
|
||||
self.event_persistence_mode,
|
||||
));
|
||||
}
|
||||
}
|
||||
if filtered.is_empty() {
|
||||
@@ -673,9 +728,7 @@ async fn rollout_writer(
|
||||
RolloutCmd::AddItems(items) => {
|
||||
let mut persisted_items = Vec::new();
|
||||
for item in items {
|
||||
if is_persisted_response_item(&item) {
|
||||
persisted_items.push(item);
|
||||
}
|
||||
persisted_items.push(item);
|
||||
}
|
||||
if persisted_items.is_empty() {
|
||||
continue;
|
||||
@@ -1003,6 +1056,7 @@ mod tests {
|
||||
SessionSource::Exec,
|
||||
BaseInstructions::default(),
|
||||
Vec::new(),
|
||||
EventPersistenceMode::Limited,
|
||||
),
|
||||
None,
|
||||
None,
|
||||
|
||||
@@ -21,6 +21,7 @@ use crate::protocol::EventMsg;
|
||||
use crate::protocol::ExecCommandBeginEvent;
|
||||
use crate::protocol::ExecCommandEndEvent;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
use crate::protocol::ExecCommandStatus;
|
||||
use crate::protocol::SandboxPolicy;
|
||||
use crate::protocol::TurnStartedEvent;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
@@ -207,6 +208,7 @@ pub(crate) async fn execute_user_shell_command(
|
||||
exit_code: -1,
|
||||
duration: Duration::ZERO,
|
||||
formatted_output: aborted_message,
|
||||
status: ExecCommandStatus::Failed,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
@@ -233,6 +235,11 @@ pub(crate) async fn execute_user_shell_command(
|
||||
&output,
|
||||
turn_context.truncation_policy,
|
||||
),
|
||||
status: if output.exit_code == 0 {
|
||||
ExecCommandStatus::Completed
|
||||
} else {
|
||||
ExecCommandStatus::Failed
|
||||
},
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
@@ -272,6 +279,7 @@ pub(crate) async fn execute_user_shell_command(
|
||||
&exec_output,
|
||||
turn_context.truncation_policy,
|
||||
),
|
||||
status: ExecCommandStatus::Failed,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -277,13 +277,15 @@ impl ThreadManager {
|
||||
}
|
||||
|
||||
pub async fn start_thread(&self, config: Config) -> CodexResult<NewThread> {
|
||||
self.start_thread_with_tools(config, Vec::new()).await
|
||||
self.start_thread_with_tools(config, Vec::new(), false)
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn start_thread_with_tools(
|
||||
&self,
|
||||
config: Config,
|
||||
dynamic_tools: Vec<codex_protocol::dynamic_tools::DynamicToolSpec>,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.state
|
||||
.spawn_thread(
|
||||
@@ -292,6 +294,7 @@ impl ThreadManager {
|
||||
Arc::clone(&self.state.auth_manager),
|
||||
self.agent_control(),
|
||||
dynamic_tools,
|
||||
persist_extended_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -303,7 +306,7 @@ impl ThreadManager {
|
||||
auth_manager: Arc<AuthManager>,
|
||||
) -> CodexResult<NewThread> {
|
||||
let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?;
|
||||
self.resume_thread_with_history(config, initial_history, auth_manager)
|
||||
self.resume_thread_with_history(config, initial_history, auth_manager, false)
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -312,6 +315,7 @@ impl ThreadManager {
|
||||
config: Config,
|
||||
initial_history: InitialHistory,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.state
|
||||
.spawn_thread(
|
||||
@@ -320,6 +324,7 @@ impl ThreadManager {
|
||||
auth_manager,
|
||||
self.agent_control(),
|
||||
Vec::new(),
|
||||
persist_extended_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -349,6 +354,7 @@ impl ThreadManager {
|
||||
nth_user_message: usize,
|
||||
config: Config,
|
||||
path: PathBuf,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
let history = RolloutRecorder::get_rollout_history(&path).await?;
|
||||
let history = truncate_before_nth_user_message(history, nth_user_message);
|
||||
@@ -359,6 +365,7 @@ impl ThreadManager {
|
||||
Arc::clone(&self.state.auth_manager),
|
||||
self.agent_control(),
|
||||
Vec::new(),
|
||||
persist_extended_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -409,7 +416,7 @@ impl ThreadManagerState {
|
||||
config: Config,
|
||||
agent_control: AgentControl,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.spawn_new_thread_with_source(config, agent_control, self.session_source.clone())
|
||||
self.spawn_new_thread_with_source(config, agent_control, self.session_source.clone(), false)
|
||||
.await
|
||||
}
|
||||
|
||||
@@ -418,6 +425,7 @@ impl ThreadManagerState {
|
||||
config: Config,
|
||||
agent_control: AgentControl,
|
||||
session_source: SessionSource,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.spawn_thread_with_source(
|
||||
config,
|
||||
@@ -426,6 +434,7 @@ impl ThreadManagerState {
|
||||
agent_control,
|
||||
session_source,
|
||||
Vec::new(),
|
||||
persist_extended_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -445,6 +454,7 @@ impl ThreadManagerState {
|
||||
agent_control,
|
||||
session_source,
|
||||
Vec::new(),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -457,6 +467,7 @@ impl ThreadManagerState {
|
||||
auth_manager: Arc<AuthManager>,
|
||||
agent_control: AgentControl,
|
||||
dynamic_tools: Vec<codex_protocol::dynamic_tools::DynamicToolSpec>,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.spawn_thread_with_source(
|
||||
config,
|
||||
@@ -465,10 +476,12 @@ impl ThreadManagerState {
|
||||
agent_control,
|
||||
self.session_source.clone(),
|
||||
dynamic_tools,
|
||||
persist_extended_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) async fn spawn_thread_with_source(
|
||||
&self,
|
||||
config: Config,
|
||||
@@ -477,6 +490,7 @@ impl ThreadManagerState {
|
||||
agent_control: AgentControl,
|
||||
session_source: SessionSource,
|
||||
dynamic_tools: Vec<codex_protocol::dynamic_tools::DynamicToolSpec>,
|
||||
persist_extended_history: bool,
|
||||
) -> CodexResult<NewThread> {
|
||||
self.file_watcher.register_config(&config);
|
||||
let CodexSpawnOk {
|
||||
@@ -491,6 +505,7 @@ impl ThreadManagerState {
|
||||
session_source,
|
||||
agent_control,
|
||||
dynamic_tools,
|
||||
persist_extended_history,
|
||||
)
|
||||
.await?;
|
||||
self.finalize_thread_spawn(codex, thread_id).await
|
||||
|
||||
@@ -9,9 +9,11 @@ use crate::protocol::EventMsg;
|
||||
use crate::protocol::ExecCommandBeginEvent;
|
||||
use crate::protocol::ExecCommandEndEvent;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
use crate::protocol::ExecCommandStatus;
|
||||
use crate::protocol::FileChange;
|
||||
use crate::protocol::PatchApplyBeginEvent;
|
||||
use crate::protocol::PatchApplyEndEvent;
|
||||
use crate::protocol::PatchApplyStatus;
|
||||
use crate::protocol::TurnDiffEvent;
|
||||
use crate::tools::context::SharedTurnDiffTracker;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
@@ -56,6 +58,7 @@ pub(crate) enum ToolEventStage {
|
||||
pub(crate) enum ToolEventFailure {
|
||||
Output(ExecToolCallOutput),
|
||||
Message(String),
|
||||
Rejected(String),
|
||||
}
|
||||
|
||||
pub(crate) async fn emit_exec_command_begin(
|
||||
@@ -195,6 +198,11 @@ impl ToolEmitter {
|
||||
output.stdout.text.clone(),
|
||||
output.stderr.text.clone(),
|
||||
output.exit_code == 0,
|
||||
if output.exit_code == 0 {
|
||||
PatchApplyStatus::Completed
|
||||
} else {
|
||||
PatchApplyStatus::Failed
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
@@ -208,6 +216,11 @@ impl ToolEmitter {
|
||||
output.stdout.text.clone(),
|
||||
output.stderr.text.clone(),
|
||||
output.exit_code == 0,
|
||||
if output.exit_code == 0 {
|
||||
PatchApplyStatus::Completed
|
||||
} else {
|
||||
PatchApplyStatus::Failed
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
@@ -221,6 +234,21 @@ impl ToolEmitter {
|
||||
String::new(),
|
||||
(*message).to_string(),
|
||||
false,
|
||||
PatchApplyStatus::Failed,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
(
|
||||
Self::ApplyPatch { changes, .. },
|
||||
ToolEventStage::Failure(ToolEventFailure::Rejected(message)),
|
||||
) => {
|
||||
emit_patch_end(
|
||||
ctx,
|
||||
changes.clone(),
|
||||
String::new(),
|
||||
(*message).to_string(),
|
||||
false,
|
||||
PatchApplyStatus::Declined,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
@@ -301,6 +329,13 @@ impl ToolEmitter {
|
||||
Err(ToolError::Rejected(msg)) => {
|
||||
// Normalize common rejection messages for exec tools so tests and
|
||||
// users see a clear, consistent phrase.
|
||||
//
|
||||
// NOTE: ToolError::Rejected is currently used for both user-declined approvals
|
||||
// and some operational/runtime rejection paths (for example setup failures).
|
||||
// We intentionally map all of them through the "rejected" event path for now,
|
||||
// which means a subset of non-user failures may be reported as Declined.
|
||||
//
|
||||
// TODO: We should add a new ToolError variant for user-declined approvals.
|
||||
let normalized = if msg == "rejected by user" {
|
||||
match self {
|
||||
Self::Shell { .. } | Self::UnifiedExec { .. } => {
|
||||
@@ -311,7 +346,7 @@ impl ToolEmitter {
|
||||
} else {
|
||||
msg
|
||||
};
|
||||
let event = ToolEventStage::Failure(ToolEventFailure::Message(normalized.clone()));
|
||||
let event = ToolEventStage::Failure(ToolEventFailure::Rejected(normalized.clone()));
|
||||
let result = Err(FunctionCallError::RespondToModel(normalized));
|
||||
(event, result)
|
||||
}
|
||||
@@ -357,6 +392,7 @@ struct ExecCommandResult {
|
||||
exit_code: i32,
|
||||
duration: Duration,
|
||||
formatted_output: String,
|
||||
status: ExecCommandStatus,
|
||||
}
|
||||
|
||||
async fn emit_exec_stage(
|
||||
@@ -386,6 +422,11 @@ async fn emit_exec_stage(
|
||||
exit_code: output.exit_code,
|
||||
duration: output.duration,
|
||||
formatted_output: format_exec_output_str(&output, ctx.turn.truncation_policy),
|
||||
status: if output.exit_code == 0 {
|
||||
ExecCommandStatus::Completed
|
||||
} else {
|
||||
ExecCommandStatus::Failed
|
||||
},
|
||||
};
|
||||
emit_exec_end(ctx, exec_input, exec_result).await;
|
||||
}
|
||||
@@ -398,6 +439,20 @@ async fn emit_exec_stage(
|
||||
exit_code: -1,
|
||||
duration: Duration::ZERO,
|
||||
formatted_output: text,
|
||||
status: ExecCommandStatus::Failed,
|
||||
};
|
||||
emit_exec_end(ctx, exec_input, exec_result).await;
|
||||
}
|
||||
ToolEventStage::Failure(ToolEventFailure::Rejected(message)) => {
|
||||
let text = message.to_string();
|
||||
let exec_result = ExecCommandResult {
|
||||
stdout: String::new(),
|
||||
stderr: text.clone(),
|
||||
aggregated_output: text.clone(),
|
||||
exit_code: -1,
|
||||
duration: Duration::ZERO,
|
||||
formatted_output: text,
|
||||
status: ExecCommandStatus::Declined,
|
||||
};
|
||||
emit_exec_end(ctx, exec_input, exec_result).await;
|
||||
}
|
||||
@@ -427,6 +482,7 @@ async fn emit_exec_end(
|
||||
exit_code: exec_result.exit_code,
|
||||
duration: exec_result.duration,
|
||||
formatted_output: exec_result.formatted_output,
|
||||
status: exec_result.status,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
@@ -438,6 +494,7 @@ async fn emit_patch_end(
|
||||
stdout: String,
|
||||
stderr: String,
|
||||
success: bool,
|
||||
status: PatchApplyStatus,
|
||||
) {
|
||||
ctx.session
|
||||
.send_event(
|
||||
@@ -449,6 +506,7 @@ async fn emit_patch_end(
|
||||
stderr,
|
||||
success,
|
||||
changes,
|
||||
status,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -1301,6 +1301,7 @@ mod tests {
|
||||
phase: None,
|
||||
})]),
|
||||
AuthManager::from_auth_for_testing(CodexAuth::from_api_key("dummy")),
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.expect("start thread");
|
||||
|
||||
Reference in New Issue
Block a user