mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Move apply-patch file changes into turn items (#20540)
## Why Apply-patch file changes are now part of the core turn item stream, so v2 clients can consume the same first-class item lifecycle path used by other turn items instead of relying on app-server-specific remapping from legacy patch events. ## What changed - Added a core `TurnItem::FileChange` carrying apply-patch changes and completion metadata. - Updated the apply-patch tool emitter to send `ItemStarted` / `ItemCompleted` with the new `FileChange` item while preserving legacy `PatchApplyBegin` / `PatchApplyEnd` fan-out. - Updated app-server v2 conversion to render the new core item directly and stopped `event_mapping` from remapping old patch begin/end events into item notifications. - Kept thread history reconstruction based on the existing old apply-patch events for rollout compatibility. ## Verification - `cargo test -p codex-protocol -p codex-app-server-protocol` - `cargo test -p codex-core --test all apply_patch_tool_executes_and_emits_patch_events` - `cargo test -p codex-app-server bespoke_event_handling`
This commit is contained in:
committed by
GitHub
Unverified
parent
0b04d1b3cc
commit
f476338f93
@@ -1,7 +1,6 @@
|
||||
use crate::protocol::common::ServerNotification;
|
||||
use crate::protocol::item_builders::build_command_execution_begin_item;
|
||||
use crate::protocol::item_builders::build_command_execution_end_item;
|
||||
use crate::protocol::item_builders::build_file_change_begin_item;
|
||||
use crate::protocol::item_builders::convert_patch_changes;
|
||||
use crate::protocol::v2::AgentMessageDeltaNotification;
|
||||
use crate::protocol::v2::CollabAgentState;
|
||||
@@ -450,13 +449,6 @@ pub fn item_event_to_server_notification(
|
||||
item: item_completed_event.item.into(),
|
||||
})
|
||||
}
|
||||
EventMsg::PatchApplyBegin(patch_begin_event) => {
|
||||
ServerNotification::ItemStarted(ItemStartedNotification {
|
||||
thread_id,
|
||||
turn_id,
|
||||
item: build_file_change_begin_item(&patch_begin_event),
|
||||
})
|
||||
}
|
||||
EventMsg::PatchApplyUpdated(event) => {
|
||||
ServerNotification::FileChangePatchUpdated(FileChangePatchUpdatedNotification {
|
||||
thread_id,
|
||||
|
||||
@@ -1,9 +1,8 @@
|
||||
//! Shared builders for synthetic [`ThreadItem`] values emitted by the app-server layer.
|
||||
//! Shared builders for app-server [`ThreadItem`] values derived from compatibility events.
|
||||
//!
|
||||
//! These items do not come from first-class core `ItemStarted` / `ItemCompleted` events.
|
||||
//! Instead, the app-server synthesizes them so clients can render a coherent lifecycle for
|
||||
//! approvals and other pre-execution flows before the underlying tool has started or when the
|
||||
//! tool never starts at all.
|
||||
//! Most live tool items now come from first-class core `ItemStarted` / `ItemCompleted` events.
|
||||
//! These builders remain for approval flows, rebuilt legacy history, and other pre-execution
|
||||
//! paths where the underlying tool has not started or never starts at all.
|
||||
//!
|
||||
//! Keeping these builders in one place is useful for two reasons:
|
||||
//! - Live notifications and rebuilt `thread/read` history both need to construct the same
|
||||
|
||||
@@ -357,6 +357,7 @@ impl ThreadHistoryBuilder {
|
||||
| codex_protocol::items::TurnItem::Reasoning(_)
|
||||
| codex_protocol::items::TurnItem::WebSearch(_)
|
||||
| codex_protocol::items::TurnItem::ImageGeneration(_)
|
||||
| codex_protocol::items::TurnItem::FileChange(_)
|
||||
| codex_protocol::items::TurnItem::ContextCompaction(_) => {}
|
||||
}
|
||||
}
|
||||
@@ -378,6 +379,7 @@ impl ThreadHistoryBuilder {
|
||||
| codex_protocol::items::TurnItem::Reasoning(_)
|
||||
| codex_protocol::items::TurnItem::WebSearch(_)
|
||||
| codex_protocol::items::TurnItem::ImageGeneration(_)
|
||||
| codex_protocol::items::TurnItem::FileChange(_)
|
||||
| codex_protocol::items::TurnItem::ContextCompaction(_) => {}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::path::PathBuf;
|
||||
|
||||
use crate::RequestId;
|
||||
use crate::protocol::common::AuthMode;
|
||||
use crate::protocol::item_builders::convert_patch_changes;
|
||||
use codex_experimental_api_macros::ExperimentalApi;
|
||||
use codex_protocol::account::PlanType;
|
||||
use codex_protocol::account::ProviderAccount;
|
||||
@@ -6469,6 +6470,15 @@ impl From<CoreTurnItem> for ThreadItem {
|
||||
result: image.result,
|
||||
saved_path: image.saved_path,
|
||||
},
|
||||
CoreTurnItem::FileChange(file_change) => ThreadItem::FileChange {
|
||||
id: file_change.id,
|
||||
changes: convert_patch_changes(&file_change.changes),
|
||||
status: file_change
|
||||
.status
|
||||
.as_ref()
|
||||
.map(PatchApplyStatus::from)
|
||||
.unwrap_or(PatchApplyStatus::InProgress),
|
||||
},
|
||||
CoreTurnItem::ContextCompaction(compaction) => {
|
||||
ThreadItem::ContextCompaction { id: compaction.id }
|
||||
}
|
||||
@@ -8078,6 +8088,7 @@ mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::items::AgentMessageContent;
|
||||
use codex_protocol::items::AgentMessageItem;
|
||||
use codex_protocol::items::FileChangeItem;
|
||||
use codex_protocol::items::ReasoningItem;
|
||||
use codex_protocol::items::TurnItem;
|
||||
use codex_protocol::items::UserMessageItem;
|
||||
@@ -10358,6 +10369,35 @@ mod tests {
|
||||
}),
|
||||
}
|
||||
);
|
||||
|
||||
let file_change_item = TurnItem::FileChange(FileChangeItem {
|
||||
id: "patch-1".to_string(),
|
||||
changes: [(
|
||||
PathBuf::from("README.md"),
|
||||
codex_protocol::protocol::FileChange::Add {
|
||||
content: "hello\n".to_string(),
|
||||
},
|
||||
)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
status: Some(codex_protocol::protocol::PatchApplyStatus::Completed),
|
||||
auto_approved: None,
|
||||
stdout: Some("Done!".to_string()),
|
||||
stderr: Some(String::new()),
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
ThreadItem::from(file_change_item),
|
||||
ThreadItem::FileChange {
|
||||
id: "patch-1".to_string(),
|
||||
changes: vec![FileUpdateChange {
|
||||
path: "README.md".to_string(),
|
||||
kind: PatchChangeKind::Add,
|
||||
diff: "hello\n".to_string(),
|
||||
}],
|
||||
status: PatchApplyStatus::Completed,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -29,7 +29,6 @@ use codex_app_server_protocol::ExecPolicyAmendment as V2ExecPolicyAmendment;
|
||||
use codex_app_server_protocol::FileChangeApprovalDecision;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalParams;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalResponse;
|
||||
use codex_app_server_protocol::FileUpdateChange;
|
||||
use codex_app_server_protocol::GrantedPermissionProfile as V2GrantedPermissionProfile;
|
||||
use codex_app_server_protocol::GuardianWarningNotification;
|
||||
use codex_app_server_protocol::HookCompletedNotification;
|
||||
@@ -46,7 +45,6 @@ use codex_app_server_protocol::ModelVerificationNotification;
|
||||
use codex_app_server_protocol::NetworkApprovalContext as V2NetworkApprovalContext;
|
||||
use codex_app_server_protocol::NetworkPolicyAmendment as V2NetworkPolicyAmendment;
|
||||
use codex_app_server_protocol::NetworkPolicyRuleAction as V2NetworkPolicyRuleAction;
|
||||
use codex_app_server_protocol::PatchApplyStatus;
|
||||
use codex_app_server_protocol::PermissionsRequestApprovalParams;
|
||||
use codex_app_server_protocol::PermissionsRequestApprovalResponse;
|
||||
use codex_app_server_protocol::RawResponseItemCompletedNotification;
|
||||
@@ -82,11 +80,8 @@ use codex_app_server_protocol::TurnPlanUpdatedNotification;
|
||||
use codex_app_server_protocol::TurnStartedNotification;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
use codex_app_server_protocol::WarningNotification;
|
||||
use codex_app_server_protocol::build_file_change_approval_request_item;
|
||||
use codex_app_server_protocol::build_file_change_end_item;
|
||||
use codex_app_server_protocol::build_item_from_guardian_event;
|
||||
use codex_app_server_protocol::build_turns_from_rollout_items;
|
||||
use codex_app_server_protocol::convert_patch_changes;
|
||||
use codex_app_server_protocol::guardian_auto_approval_review_notification;
|
||||
use codex_app_server_protocol::item_event_to_server_notification;
|
||||
use codex_core::CodexThread;
|
||||
@@ -524,28 +519,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
let permission_guard = thread_watch_manager
|
||||
.note_permission_requested(&conversation_id.to_string())
|
||||
.await;
|
||||
// Until we migrate the core to be aware of a first class FileChangeItem
|
||||
// and emit the corresponding EventMsg, we repurpose the call_id as the item_id.
|
||||
let item_id = event.call_id.clone();
|
||||
let patch_changes = convert_patch_changes(&event.changes);
|
||||
let first_start = {
|
||||
let mut state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.file_change_started
|
||||
.insert(item_id.clone())
|
||||
};
|
||||
if first_start {
|
||||
let item = build_file_change_approval_request_item(&event);
|
||||
let notification = ItemStartedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemStarted(notification))
|
||||
.await;
|
||||
}
|
||||
|
||||
let params = FileChangeRequestApprovalParams {
|
||||
thread_id: conversation_id.to_string(),
|
||||
@@ -559,14 +533,10 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
tokio::spawn(async move {
|
||||
on_file_change_request_approval_response(
|
||||
event_turn_id,
|
||||
conversation_id,
|
||||
item_id,
|
||||
patch_changes,
|
||||
pending_request_id,
|
||||
rx,
|
||||
conversation,
|
||||
outgoing,
|
||||
thread_state.clone(),
|
||||
permission_guard,
|
||||
)
|
||||
@@ -1104,40 +1074,9 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
)
|
||||
.await;
|
||||
}
|
||||
EventMsg::PatchApplyBegin(patch_begin_event) => {
|
||||
// Until we migrate the core to be aware of a first class FileChangeItem
|
||||
// and emit the corresponding EventMsg, we repurpose the call_id as the item_id.
|
||||
let item_id = patch_begin_event.call_id.clone();
|
||||
|
||||
let first_start = {
|
||||
let mut state = thread_state.lock().await;
|
||||
state
|
||||
.turn_summary
|
||||
.file_change_started
|
||||
.insert(item_id.clone())
|
||||
};
|
||||
if first_start {
|
||||
let notification = item_event_to_server_notification(
|
||||
EventMsg::PatchApplyBegin(patch_begin_event),
|
||||
&conversation_id.to_string(),
|
||||
&event_turn_id,
|
||||
);
|
||||
outgoing.send_server_notification(notification).await;
|
||||
}
|
||||
}
|
||||
EventMsg::PatchApplyEnd(patch_end_event) => {
|
||||
// Until we migrate the core to be aware of a first class FileChangeItem
|
||||
// and emit the corresponding EventMsg, we repurpose the call_id as the item_id.
|
||||
let item_id = patch_end_event.call_id.clone();
|
||||
complete_file_change_item(
|
||||
conversation_id,
|
||||
item_id,
|
||||
build_file_change_end_item(&patch_end_event),
|
||||
event_turn_id.clone(),
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
EventMsg::PatchApplyBegin(_) | EventMsg::PatchApplyEnd(_) => {
|
||||
// Core still fans out these deprecated events for legacy clients;
|
||||
// v2 clients receive the canonical FileChange item instead.
|
||||
}
|
||||
EventMsg::ExecCommandBegin(exec_command_begin_event) => {
|
||||
if matches!(
|
||||
@@ -1425,31 +1364,6 @@ async fn emit_turn_completed_with_status(
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn complete_file_change_item(
|
||||
conversation_id: ThreadId,
|
||||
item_id: String,
|
||||
item: ThreadItem,
|
||||
turn_id: String,
|
||||
outgoing: &ThreadScopedOutgoingMessageSender,
|
||||
thread_state: &Arc<Mutex<ThreadState>>,
|
||||
) {
|
||||
thread_state
|
||||
.lock()
|
||||
.await
|
||||
.turn_summary
|
||||
.file_change_started
|
||||
.remove(&item_id);
|
||||
|
||||
let notification = ItemCompletedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id,
|
||||
item,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemCompleted(notification))
|
||||
.await;
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
async fn start_command_execution_item(
|
||||
conversation_id: &ThreadId,
|
||||
@@ -2002,38 +1916,28 @@ fn render_review_output_text(output: &ReviewOutputEvent) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
fn map_file_change_approval_decision(
|
||||
decision: FileChangeApprovalDecision,
|
||||
) -> (ReviewDecision, Option<PatchApplyStatus>) {
|
||||
fn map_file_change_approval_decision(decision: FileChangeApprovalDecision) -> ReviewDecision {
|
||||
match decision {
|
||||
FileChangeApprovalDecision::Accept => (ReviewDecision::Approved, None),
|
||||
FileChangeApprovalDecision::AcceptForSession => (ReviewDecision::ApprovedForSession, None),
|
||||
FileChangeApprovalDecision::Decline => {
|
||||
(ReviewDecision::Denied, Some(PatchApplyStatus::Declined))
|
||||
}
|
||||
FileChangeApprovalDecision::Cancel => {
|
||||
(ReviewDecision::Abort, Some(PatchApplyStatus::Declined))
|
||||
}
|
||||
FileChangeApprovalDecision::Accept => ReviewDecision::Approved,
|
||||
FileChangeApprovalDecision::AcceptForSession => ReviewDecision::ApprovedForSession,
|
||||
FileChangeApprovalDecision::Decline => ReviewDecision::Denied,
|
||||
FileChangeApprovalDecision::Cancel => ReviewDecision::Abort,
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
async fn on_file_change_request_approval_response(
|
||||
event_turn_id: String,
|
||||
conversation_id: ThreadId,
|
||||
item_id: String,
|
||||
changes: Vec<FileUpdateChange>,
|
||||
pending_request_id: RequestId,
|
||||
receiver: oneshot::Receiver<ClientRequestResult>,
|
||||
codex: Arc<CodexThread>,
|
||||
outgoing: ThreadScopedOutgoingMessageSender,
|
||||
thread_state: Arc<Mutex<ThreadState>>,
|
||||
permission_guard: ThreadWatchActiveGuard,
|
||||
) {
|
||||
let response = receiver.await;
|
||||
resolve_server_request_on_thread_listener(&thread_state, pending_request_id).await;
|
||||
drop(permission_guard);
|
||||
let (decision, completion_status) = match response {
|
||||
let decision = match response {
|
||||
Ok(Ok(value)) => {
|
||||
let response = serde_json::from_value::<FileChangeRequestApprovalResponse>(value)
|
||||
.unwrap_or_else(|err| {
|
||||
@@ -2043,39 +1947,19 @@ async fn on_file_change_request_approval_response(
|
||||
}
|
||||
});
|
||||
|
||||
let (decision, completion_status) =
|
||||
map_file_change_approval_decision(response.decision);
|
||||
// Allow EventMsg::PatchApplyEnd to emit ItemCompleted for accepted patches.
|
||||
// Only short-circuit on declines/cancels/failures.
|
||||
(decision, completion_status)
|
||||
map_file_change_approval_decision(response.decision)
|
||||
}
|
||||
Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return,
|
||||
Ok(Err(err)) => {
|
||||
error!("request failed with client error: {err:?}");
|
||||
(ReviewDecision::Denied, Some(PatchApplyStatus::Failed))
|
||||
ReviewDecision::Denied
|
||||
}
|
||||
Err(err) => {
|
||||
error!("request failed: {err:?}");
|
||||
(ReviewDecision::Denied, Some(PatchApplyStatus::Failed))
|
||||
ReviewDecision::Denied
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(status) = completion_status {
|
||||
complete_file_change_item(
|
||||
conversation_id,
|
||||
item_id.clone(),
|
||||
ThreadItem::FileChange {
|
||||
id: item_id.clone(),
|
||||
changes,
|
||||
status,
|
||||
},
|
||||
event_turn_id.clone(),
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
if let Err(err) = codex
|
||||
.submit(Op::PatchApproval {
|
||||
id: item_id,
|
||||
@@ -2886,10 +2770,9 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn file_change_accept_for_session_maps_to_approved_for_session() {
|
||||
let (decision, completion_status) =
|
||||
let decision =
|
||||
map_file_change_approval_decision(FileChangeApprovalDecision::AcceptForSession);
|
||||
assert_eq!(decision, ReviewDecision::ApprovedForSession);
|
||||
assert_eq!(completion_status, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -61,7 +61,6 @@ pub(crate) enum ThreadListenerCommand {
|
||||
#[derive(Default, Clone)]
|
||||
pub(crate) struct TurnSummary {
|
||||
pub(crate) started_at: Option<i64>,
|
||||
pub(crate) file_change_started: HashSet<String>,
|
||||
pub(crate) command_execution_started: HashSet<String>,
|
||||
pub(crate) last_error: Option<TurnError>,
|
||||
}
|
||||
|
||||
@@ -6,6 +6,8 @@ use crate::tools::sandboxing::ToolError;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::SandboxErr;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
use codex_protocol::items::FileChangeItem;
|
||||
use codex_protocol::items::TurnItem;
|
||||
use codex_protocol::parse_command::ParsedCommand;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecCommandBeginEvent;
|
||||
@@ -13,8 +15,6 @@ use codex_protocol::protocol::ExecCommandEndEvent;
|
||||
use codex_protocol::protocol::ExecCommandSource;
|
||||
use codex_protocol::protocol::ExecCommandStatus;
|
||||
use codex_protocol::protocol::FileChange;
|
||||
use codex_protocol::protocol::PatchApplyBeginEvent;
|
||||
use codex_protocol::protocol::PatchApplyEndEvent;
|
||||
use codex_protocol::protocol::PatchApplyStatus;
|
||||
use codex_protocol::protocol::TurnDiffEvent;
|
||||
use codex_shell_command::parse_command::parse_command;
|
||||
@@ -183,13 +183,15 @@ impl ToolEmitter {
|
||||
guard.on_patch_begin(changes);
|
||||
}
|
||||
ctx.session
|
||||
.send_event(
|
||||
.emit_turn_item_started(
|
||||
ctx.turn,
|
||||
EventMsg::PatchApplyBegin(PatchApplyBeginEvent {
|
||||
call_id: ctx.call_id.to_string(),
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
auto_approved: *auto_approved,
|
||||
&TurnItem::FileChange(FileChangeItem {
|
||||
id: ctx.call_id.to_string(),
|
||||
changes: changes.clone(),
|
||||
status: None,
|
||||
auto_approved: Some(*auto_approved),
|
||||
stdout: None,
|
||||
stderr: None,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
@@ -200,7 +202,6 @@ impl ToolEmitter {
|
||||
changes.clone(),
|
||||
output.stdout.text.clone(),
|
||||
output.stderr.text.clone(),
|
||||
output.exit_code == 0,
|
||||
if output.exit_code == 0 {
|
||||
PatchApplyStatus::Completed
|
||||
} else {
|
||||
@@ -218,7 +219,6 @@ impl ToolEmitter {
|
||||
changes.clone(),
|
||||
output.stdout.text.clone(),
|
||||
output.stderr.text.clone(),
|
||||
output.exit_code == 0,
|
||||
if output.exit_code == 0 {
|
||||
PatchApplyStatus::Completed
|
||||
} else {
|
||||
@@ -236,7 +236,6 @@ impl ToolEmitter {
|
||||
changes.clone(),
|
||||
String::new(),
|
||||
(*message).to_string(),
|
||||
/*success*/ false,
|
||||
PatchApplyStatus::Failed,
|
||||
)
|
||||
.await;
|
||||
@@ -250,7 +249,6 @@ impl ToolEmitter {
|
||||
changes.clone(),
|
||||
String::new(),
|
||||
(*message).to_string(),
|
||||
/*success*/ false,
|
||||
PatchApplyStatus::Declined,
|
||||
)
|
||||
.await;
|
||||
@@ -496,20 +494,18 @@ async fn emit_patch_end(
|
||||
changes: HashMap<PathBuf, FileChange>,
|
||||
stdout: String,
|
||||
stderr: String,
|
||||
success: bool,
|
||||
status: PatchApplyStatus,
|
||||
) {
|
||||
ctx.session
|
||||
.send_event(
|
||||
.emit_turn_item_completed(
|
||||
ctx.turn,
|
||||
EventMsg::PatchApplyEnd(PatchApplyEndEvent {
|
||||
call_id: ctx.call_id.to_string(),
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
stdout,
|
||||
stderr,
|
||||
success,
|
||||
TurnItem::FileChange(FileChangeItem {
|
||||
id: ctx.call_id.to_string(),
|
||||
changes,
|
||||
status,
|
||||
status: Some(status),
|
||||
auto_approved: None,
|
||||
stdout: Some(stdout),
|
||||
stderr: Some(stderr),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -4,6 +4,7 @@ use std::fs;
|
||||
|
||||
use assert_matches::assert_matches;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::items::TurnItem;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::plan_tool::StepStatus;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
@@ -365,9 +366,30 @@ async fn apply_patch_tool_executes_and_emits_patch_events() -> anyhow::Result<()
|
||||
})
|
||||
.await?;
|
||||
|
||||
let mut saw_file_change_started = false;
|
||||
let mut saw_file_change_completed = false;
|
||||
let mut saw_patch_begin = false;
|
||||
let mut patch_end_success = None;
|
||||
wait_for_event(&codex, |event| match event {
|
||||
EventMsg::ItemStarted(started) => {
|
||||
if let TurnItem::FileChange(item) = &started.item {
|
||||
saw_file_change_started = true;
|
||||
assert_eq!(item.id, call_id);
|
||||
assert_eq!(item.status, None);
|
||||
}
|
||||
false
|
||||
}
|
||||
EventMsg::ItemCompleted(completed) => {
|
||||
if let TurnItem::FileChange(item) = &completed.item {
|
||||
saw_file_change_completed = true;
|
||||
assert_eq!(item.id, call_id);
|
||||
assert_eq!(
|
||||
item.status,
|
||||
Some(codex_protocol::protocol::PatchApplyStatus::Completed)
|
||||
);
|
||||
}
|
||||
false
|
||||
}
|
||||
EventMsg::PatchApplyBegin(begin) => {
|
||||
saw_patch_begin = true;
|
||||
assert_eq!(begin.call_id, call_id);
|
||||
@@ -383,6 +405,14 @@ async fn apply_patch_tool_executes_and_emits_patch_events() -> anyhow::Result<()
|
||||
})
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
saw_file_change_started,
|
||||
"expected ItemStarted for TurnItem::FileChange"
|
||||
);
|
||||
assert!(
|
||||
saw_file_change_completed,
|
||||
"expected ItemCompleted for TurnItem::FileChange"
|
||||
);
|
||||
assert!(saw_patch_begin, "expected PatchApplyBegin event");
|
||||
let patch_end_success =
|
||||
patch_end_success.expect("expected PatchApplyEnd event to capture success flag");
|
||||
|
||||
@@ -8,7 +8,11 @@ use crate::protocol::AgentReasoningEvent;
|
||||
use crate::protocol::AgentReasoningRawContentEvent;
|
||||
use crate::protocol::ContextCompactedEvent;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::protocol::FileChange;
|
||||
use crate::protocol::ImageGenerationEndEvent;
|
||||
use crate::protocol::PatchApplyBeginEvent;
|
||||
use crate::protocol::PatchApplyEndEvent;
|
||||
use crate::protocol::PatchApplyStatus;
|
||||
use crate::protocol::UserMessageEvent;
|
||||
use crate::protocol::WebSearchEndEvent;
|
||||
use crate::user_input::ByteRange;
|
||||
@@ -20,6 +24,8 @@ use quick_xml::se::to_string as to_xml_string;
|
||||
use schemars::JsonSchema;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use ts_rs::TS;
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
|
||||
@@ -33,6 +39,7 @@ pub enum TurnItem {
|
||||
Reasoning(ReasoningItem),
|
||||
WebSearch(WebSearchItem),
|
||||
ImageGeneration(ImageGenerationItem),
|
||||
FileChange(FileChangeItem),
|
||||
ContextCompaction(ContextCompactionItem),
|
||||
}
|
||||
|
||||
@@ -127,6 +134,24 @@ pub struct ImageGenerationItem {
|
||||
pub saved_path: Option<AbsolutePathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema, PartialEq)]
|
||||
pub struct FileChangeItem {
|
||||
pub id: String,
|
||||
pub changes: HashMap<PathBuf, FileChange>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub status: Option<PatchApplyStatus>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub auto_approved: Option<bool>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub stdout: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub stderr: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
|
||||
pub struct ContextCompactionItem {
|
||||
pub id: String,
|
||||
@@ -381,6 +406,30 @@ impl ImageGenerationItem {
|
||||
}
|
||||
}
|
||||
|
||||
impl FileChangeItem {
|
||||
pub fn as_legacy_begin_event(&self, turn_id: String) -> EventMsg {
|
||||
EventMsg::PatchApplyBegin(PatchApplyBeginEvent {
|
||||
call_id: self.id.clone(),
|
||||
turn_id,
|
||||
auto_approved: self.auto_approved.unwrap_or(false),
|
||||
changes: self.changes.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
pub fn as_legacy_end_event(&self, turn_id: String) -> Option<EventMsg> {
|
||||
let status = self.status.clone()?;
|
||||
Some(EventMsg::PatchApplyEnd(PatchApplyEndEvent {
|
||||
call_id: self.id.clone(),
|
||||
turn_id,
|
||||
stdout: self.stdout.clone().unwrap_or_default(),
|
||||
stderr: self.stderr.clone().unwrap_or_default(),
|
||||
success: status == PatchApplyStatus::Completed,
|
||||
changes: self.changes.clone(),
|
||||
status,
|
||||
}))
|
||||
}
|
||||
}
|
||||
|
||||
impl TurnItem {
|
||||
pub fn id(&self) -> String {
|
||||
match self {
|
||||
@@ -391,6 +440,7 @@ impl TurnItem {
|
||||
TurnItem::Reasoning(item) => item.id.clone(),
|
||||
TurnItem::WebSearch(item) => item.id.clone(),
|
||||
TurnItem::ImageGeneration(item) => item.id.clone(),
|
||||
TurnItem::FileChange(item) => item.id.clone(),
|
||||
TurnItem::ContextCompaction(item) => item.id.clone(),
|
||||
}
|
||||
}
|
||||
@@ -403,6 +453,10 @@ impl TurnItem {
|
||||
TurnItem::Plan(_) => Vec::new(),
|
||||
TurnItem::WebSearch(item) => vec![item.as_legacy_event()],
|
||||
TurnItem::ImageGeneration(item) => vec![item.as_legacy_event()],
|
||||
TurnItem::FileChange(item) => item
|
||||
.as_legacy_end_event(String::new())
|
||||
.into_iter()
|
||||
.collect(),
|
||||
TurnItem::Reasoning(item) => item.as_legacy_events(show_raw_agent_reasoning),
|
||||
TurnItem::ContextCompaction(item) => vec![item.as_legacy_event()],
|
||||
}
|
||||
|
||||
@@ -1841,6 +1841,7 @@ impl HasLegacyEvent for ItemStartedEvent {
|
||||
call_id: item.id.clone(),
|
||||
})]
|
||||
}
|
||||
TurnItem::FileChange(item) => vec![item.as_legacy_begin_event(self.turn_id.clone())],
|
||||
_ => Vec::new(),
|
||||
}
|
||||
}
|
||||
@@ -1859,7 +1860,13 @@ pub trait HasLegacyEvent {
|
||||
|
||||
impl HasLegacyEvent for ItemCompletedEvent {
|
||||
fn as_legacy_events(&self, show_raw_agent_reasoning: bool) -> Vec<EventMsg> {
|
||||
self.item.as_legacy_events(show_raw_agent_reasoning)
|
||||
match &self.item {
|
||||
TurnItem::FileChange(item) => item
|
||||
.as_legacy_end_event(self.turn_id.clone())
|
||||
.into_iter()
|
||||
.collect(),
|
||||
_ => self.item.as_legacy_events(show_raw_agent_reasoning),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3928,6 +3935,7 @@ pub struct CollabResumeEndEvent {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::items::FileChangeItem;
|
||||
use crate::items::ImageGenerationItem;
|
||||
use crate::items::UserMessageItem;
|
||||
use crate::items::WebSearchItem;
|
||||
@@ -4630,6 +4638,41 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_started_event_from_file_change_emits_patch_begin_event() {
|
||||
let event = ItemStartedEvent {
|
||||
thread_id: ThreadId::new(),
|
||||
turn_id: "turn-1".into(),
|
||||
item: TurnItem::FileChange(FileChangeItem {
|
||||
id: "patch-1".into(),
|
||||
changes: [(
|
||||
PathBuf::from("new.txt"),
|
||||
FileChange::Add {
|
||||
content: "hello".into(),
|
||||
},
|
||||
)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
status: None,
|
||||
auto_approved: Some(true),
|
||||
stdout: None,
|
||||
stderr: None,
|
||||
}),
|
||||
};
|
||||
|
||||
let legacy_events = event.as_legacy_events(/*show_raw_agent_reasoning*/ false);
|
||||
assert_eq!(legacy_events.len(), 1);
|
||||
match &legacy_events[0] {
|
||||
EventMsg::PatchApplyBegin(event) => {
|
||||
assert_eq!(event.call_id, "patch-1");
|
||||
assert_eq!(event.turn_id, "turn-1");
|
||||
assert!(event.auto_approved);
|
||||
assert!(event.changes.contains_key(&PathBuf::from("new.txt")));
|
||||
}
|
||||
_ => panic!("expected PatchApplyBegin event"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_completed_event_from_image_generation_emits_end_event() {
|
||||
let event = ItemCompletedEvent {
|
||||
@@ -4661,6 +4704,43 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_completed_event_from_file_change_emits_patch_end_event() {
|
||||
let event = ItemCompletedEvent {
|
||||
thread_id: ThreadId::new(),
|
||||
turn_id: "turn-1".into(),
|
||||
item: TurnItem::FileChange(FileChangeItem {
|
||||
id: "patch-1".into(),
|
||||
changes: [(
|
||||
PathBuf::from("new.txt"),
|
||||
FileChange::Add {
|
||||
content: "hello".into(),
|
||||
},
|
||||
)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
status: Some(PatchApplyStatus::Completed),
|
||||
auto_approved: None,
|
||||
stdout: Some("Done!".into()),
|
||||
stderr: Some(String::new()),
|
||||
}),
|
||||
};
|
||||
|
||||
let legacy_events = event.as_legacy_events(/*show_raw_agent_reasoning*/ false);
|
||||
assert_eq!(legacy_events.len(), 1);
|
||||
match &legacy_events[0] {
|
||||
EventMsg::PatchApplyEnd(event) => {
|
||||
assert_eq!(event.call_id, "patch-1");
|
||||
assert_eq!(event.turn_id, "turn-1");
|
||||
assert_eq!(event.stdout, "Done!");
|
||||
assert!(event.success);
|
||||
assert_eq!(event.status, PatchApplyStatus::Completed);
|
||||
assert!(event.changes.contains_key(&PathBuf::from("new.txt")));
|
||||
}
|
||||
_ => panic!("expected PatchApplyEnd event"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rollback_failed_error_does_not_affect_turn_status() {
|
||||
let event = ErrorEvent {
|
||||
|
||||
Reference in New Issue
Block a user