mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Finish moving codex exec to app-server (#15424)
This PR completes the conversion of non-interactive `codex exec` to use app server rather than directly using core events and methods. ### Summary - move `codex-exec` off exec-owned `AuthManager` and `ThreadManager` state - route exec bootstrap, resume, and auth refresh through existing app-server paths - replace legacy `codex/event/*` decoding in exec with typed app-server notification handling - update human and JSONL exec output adapters to translate existing app-server notifications only - clean up "app server client" layer by eliminating support for legacy notifications; this is no longer needed - remove exposure of `authManager` and `threadManager` from "app server client" layer ### Testing - `exec` has pretty extensive unit and integration tests already, and these all pass - In addition, I asked Codex to put together a comprehensive manual set of tests to cover all of the `codex exec` functionality (including command-line options), and it successfully generated and ran these tests
This commit is contained in:
committed by
GitHub
Unverified
parent
1db6cb9789
commit
45f68843b8
@@ -12,7 +12,6 @@ use crate::models::supported_models;
|
||||
use crate::outgoing_message::ConnectionId;
|
||||
use crate::outgoing_message::ConnectionRequestId;
|
||||
use crate::outgoing_message::OutgoingMessageSender;
|
||||
use crate::outgoing_message::OutgoingNotification;
|
||||
use crate::outgoing_message::RequestContext;
|
||||
use crate::outgoing_message::ThreadScopedOutgoingMessageSender;
|
||||
use crate::thread_status::ThreadWatchManager;
|
||||
@@ -6831,43 +6830,9 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
};
|
||||
|
||||
// For now, we send a notification for every event,
|
||||
// Legacy `codex/event/*` notifications are still
|
||||
// produced here because the in-process app-server lane
|
||||
// (`codex exec` and other in-process consumers) still
|
||||
// depends on them. External transports now drop
|
||||
// `OutgoingMessage::Notification` in `transport.rs`,
|
||||
// so stdio/websocket clients only observe the typed
|
||||
// `ServerNotification` translations emitted below.
|
||||
//
|
||||
// TODO: remove this raw legacy-notification emission
|
||||
// entirely once the remaining in-process consumers are
|
||||
// migrated off `codex/event/*`.
|
||||
let event_formatted = match &event.msg {
|
||||
EventMsg::TurnStarted(_) => "task_started",
|
||||
EventMsg::TurnComplete(_) => "task_complete",
|
||||
_ => &event.msg.to_string(),
|
||||
};
|
||||
let request_event_name = format!("codex/event/{event_formatted}");
|
||||
tracing::trace!(
|
||||
conversation_id = %conversation_id,
|
||||
"app-server event: {request_event_name}"
|
||||
);
|
||||
let mut params = match serde_json::to_value(event.clone()) {
|
||||
Ok(serde_json::Value::Object(map)) => map,
|
||||
Ok(_) => {
|
||||
error!("event did not serialize to an object");
|
||||
continue;
|
||||
}
|
||||
Err(err) => {
|
||||
error!("failed to serialize event: {err}");
|
||||
continue;
|
||||
}
|
||||
};
|
||||
params.insert(
|
||||
"conversationId".to_string(),
|
||||
conversation_id.to_string().into(),
|
||||
);
|
||||
// Track the event before emitting any typed
|
||||
// translations so thread-local state such as raw event
|
||||
// opt-in stays synchronized with the conversation.
|
||||
let raw_events_enabled = {
|
||||
let mut thread_state = thread_state.lock().await;
|
||||
thread_state.track_current_turn_event(&event.msg);
|
||||
@@ -6880,18 +6845,6 @@ impl CodexMessageProcessor {
|
||||
continue;
|
||||
}
|
||||
|
||||
if !subscribed_connection_ids.is_empty() {
|
||||
outgoing_for_task
|
||||
.send_notification_to_connections(
|
||||
&subscribed_connection_ids,
|
||||
OutgoingNotification {
|
||||
method: request_event_name,
|
||||
params: Some(params.into()),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
let thread_outgoing = ThreadScopedOutgoingMessageSender::new(
|
||||
outgoing_for_task.clone(),
|
||||
subscribed_connection_ids,
|
||||
|
||||
@@ -68,14 +68,11 @@ use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ConfigWarningNotification;
|
||||
use codex_app_server_protocol::InitializeParams;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::JSONRPCNotification;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::Result;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_core::AuthManager;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config_loader::CloudRequirementsLoader;
|
||||
use codex_core::config_loader::LoaderOverrides;
|
||||
@@ -98,16 +95,6 @@ fn server_notification_requires_delivery(notification: &ServerNotification) -> b
|
||||
matches!(notification, ServerNotification::TurnCompleted(_))
|
||||
}
|
||||
|
||||
fn legacy_notification_requires_delivery(notification: &JSONRPCNotification) -> bool {
|
||||
matches!(
|
||||
notification
|
||||
.method
|
||||
.strip_prefix("codex/event/")
|
||||
.unwrap_or(¬ification.method),
|
||||
"task_complete" | "turn_aborted" | "shutdown_complete"
|
||||
)
|
||||
}
|
||||
|
||||
/// Input needed to start an in-process app-server runtime.
|
||||
///
|
||||
/// These fields mirror the pieces of ambient process state that stdio and
|
||||
@@ -124,10 +111,6 @@ pub struct InProcessStartArgs {
|
||||
pub loader_overrides: LoaderOverrides,
|
||||
/// Preloaded cloud requirements provider.
|
||||
pub cloud_requirements: CloudRequirementsLoader,
|
||||
/// Optional prebuilt auth manager reused by an embedding caller.
|
||||
pub auth_manager: Option<Arc<AuthManager>>,
|
||||
/// Optional prebuilt thread manager reused by an embedding caller.
|
||||
pub thread_manager: Option<Arc<ThreadManager>>,
|
||||
/// Feedback sink used by app-server/core telemetry and logs.
|
||||
pub feedback: CodexFeedback,
|
||||
/// Startup warnings emitted after initialize succeeds.
|
||||
@@ -144,11 +127,6 @@ pub struct InProcessStartArgs {
|
||||
|
||||
/// Event emitted from the app-server to the in-process client.
|
||||
///
|
||||
/// The stream carries three event families because CLI surfaces are mid-migration
|
||||
/// from the legacy `codex_protocol::Event` model to the typed app-server
|
||||
/// notification model. Once all surfaces consume only [`ServerNotification`],
|
||||
/// [`LegacyNotification`](Self::LegacyNotification) can be removed.
|
||||
///
|
||||
/// [`Lagged`](Self::Lagged) is a transport health marker, not an application
|
||||
/// event — it signals that the consumer fell behind and some events were dropped.
|
||||
#[derive(Debug, Clone)]
|
||||
@@ -157,8 +135,6 @@ pub enum InProcessServerEvent {
|
||||
ServerRequest(ServerRequest),
|
||||
/// App-server notification directed to the embedded client.
|
||||
ServerNotification(ServerNotification),
|
||||
/// Legacy JSON-RPC notification from core event bridge.
|
||||
LegacyNotification(JSONRPCNotification),
|
||||
/// Indicates one or more events were dropped due to backpressure.
|
||||
Lagged { skipped: usize },
|
||||
}
|
||||
@@ -390,7 +366,6 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle {
|
||||
Arc::clone(&outbound_initialized),
|
||||
Arc::clone(&outbound_experimental_api_enabled),
|
||||
Arc::clone(&outbound_opted_out_notification_methods),
|
||||
/*allow_legacy_notifications*/ true,
|
||||
/*disconnect_sender*/ None,
|
||||
),
|
||||
);
|
||||
@@ -410,8 +385,6 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle {
|
||||
cli_overrides: args.cli_overrides,
|
||||
loader_overrides: args.loader_overrides,
|
||||
cloud_requirements: args.cloud_requirements,
|
||||
auth_manager: args.auth_manager,
|
||||
thread_manager: args.thread_manager,
|
||||
feedback: args.feedback,
|
||||
log_db: None,
|
||||
config_warnings: args.config_warnings,
|
||||
@@ -655,32 +628,6 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle {
|
||||
}
|
||||
}
|
||||
}
|
||||
OutgoingMessage::Notification(notification) => {
|
||||
let notification = JSONRPCNotification {
|
||||
method: notification.method,
|
||||
params: notification.params,
|
||||
};
|
||||
if legacy_notification_requires_delivery(¬ification) {
|
||||
if event_tx
|
||||
.send(InProcessServerEvent::LegacyNotification(notification))
|
||||
.await
|
||||
.is_err()
|
||||
{
|
||||
break;
|
||||
}
|
||||
} else if let Err(send_error) =
|
||||
event_tx.try_send(InProcessServerEvent::LegacyNotification(notification))
|
||||
{
|
||||
match send_error {
|
||||
mpsc::error::TrySendError::Full(_) => {
|
||||
warn!("dropping in-process legacy notification (queue full)");
|
||||
}
|
||||
mpsc::error::TrySendError::Closed(_) => {
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -759,8 +706,6 @@ mod tests {
|
||||
cli_overrides: Vec::new(),
|
||||
loader_overrides: LoaderOverrides::default(),
|
||||
cloud_requirements: CloudRequirementsLoader::default(),
|
||||
auth_manager: None,
|
||||
thread_manager: None,
|
||||
feedback: CodexFeedback::new(),
|
||||
config_warnings: Vec::new(),
|
||||
session_source,
|
||||
@@ -858,7 +803,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guaranteed_delivery_helpers_cover_terminal_notifications() {
|
||||
fn guaranteed_delivery_helpers_cover_terminal_server_notifications() {
|
||||
assert!(server_notification_requires_delivery(
|
||||
&ServerNotification::TurnCompleted(TurnCompletedNotification {
|
||||
thread_id: "thread-1".to_string(),
|
||||
@@ -870,30 +815,5 @@ mod tests {
|
||||
},
|
||||
})
|
||||
));
|
||||
|
||||
assert!(legacy_notification_requires_delivery(
|
||||
&JSONRPCNotification {
|
||||
method: "codex/event/task_complete".to_string(),
|
||||
params: None,
|
||||
}
|
||||
));
|
||||
assert!(legacy_notification_requires_delivery(
|
||||
&JSONRPCNotification {
|
||||
method: "codex/event/turn_aborted".to_string(),
|
||||
params: None,
|
||||
}
|
||||
));
|
||||
assert!(legacy_notification_requires_delivery(
|
||||
&JSONRPCNotification {
|
||||
method: "codex/event/shutdown_complete".to_string(),
|
||||
params: None,
|
||||
}
|
||||
));
|
||||
assert!(!legacy_notification_requires_delivery(
|
||||
&JSONRPCNotification {
|
||||
method: "codex/event/item_started".to_string(),
|
||||
params: None,
|
||||
}
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -104,8 +104,6 @@ enum OutboundControlEvent {
|
||||
Opened {
|
||||
connection_id: ConnectionId,
|
||||
writer: mpsc::Sender<crate::outgoing_message::OutgoingMessage>,
|
||||
// Allow codex/event/* notifications to be emitted.
|
||||
allow_legacy_notifications: bool,
|
||||
disconnect_sender: Option<CancellationToken>,
|
||||
initialized: Arc<AtomicBool>,
|
||||
experimental_api_enabled: Arc<AtomicBool>,
|
||||
@@ -562,7 +560,6 @@ pub async fn run_main_with_transport(
|
||||
OutboundControlEvent::Opened {
|
||||
connection_id,
|
||||
writer,
|
||||
allow_legacy_notifications,
|
||||
disconnect_sender,
|
||||
initialized,
|
||||
experimental_api_enabled,
|
||||
@@ -575,7 +572,6 @@ pub async fn run_main_with_transport(
|
||||
initialized,
|
||||
experimental_api_enabled,
|
||||
opted_out_notification_methods,
|
||||
allow_legacy_notifications,
|
||||
disconnect_sender,
|
||||
),
|
||||
);
|
||||
@@ -618,8 +614,6 @@ pub async fn run_main_with_transport(
|
||||
cli_overrides,
|
||||
loader_overrides,
|
||||
cloud_requirements: cloud_requirements.clone(),
|
||||
auth_manager: None,
|
||||
thread_manager: None,
|
||||
feedback: feedback.clone(),
|
||||
log_db,
|
||||
config_warnings,
|
||||
@@ -675,7 +669,6 @@ pub async fn run_main_with_transport(
|
||||
TransportEvent::ConnectionOpened {
|
||||
connection_id,
|
||||
writer,
|
||||
allow_legacy_notifications,
|
||||
disconnect_sender,
|
||||
} => {
|
||||
let outbound_initialized = Arc::new(AtomicBool::new(false));
|
||||
@@ -687,7 +680,6 @@ pub async fn run_main_with_transport(
|
||||
.send(OutboundControlEvent::Opened {
|
||||
connection_id,
|
||||
writer,
|
||||
allow_legacy_notifications,
|
||||
disconnect_sender,
|
||||
initialized: Arc::clone(&outbound_initialized),
|
||||
experimental_api_enabled: Arc::clone(
|
||||
|
||||
@@ -172,8 +172,6 @@ pub(crate) struct MessageProcessorArgs {
|
||||
pub(crate) cli_overrides: Vec<(String, TomlValue)>,
|
||||
pub(crate) loader_overrides: LoaderOverrides,
|
||||
pub(crate) cloud_requirements: CloudRequirementsLoader,
|
||||
pub(crate) auth_manager: Option<Arc<AuthManager>>,
|
||||
pub(crate) thread_manager: Option<Arc<ThreadManager>>,
|
||||
pub(crate) feedback: CodexFeedback,
|
||||
pub(crate) log_db: Option<LogDbLayer>,
|
||||
pub(crate) config_warnings: Vec<ConfigWarningNotification>,
|
||||
@@ -192,36 +190,27 @@ impl MessageProcessor {
|
||||
cli_overrides,
|
||||
loader_overrides,
|
||||
cloud_requirements,
|
||||
auth_manager,
|
||||
thread_manager,
|
||||
feedback,
|
||||
log_db,
|
||||
config_warnings,
|
||||
session_source,
|
||||
enable_codex_api_key_env,
|
||||
} = args;
|
||||
let (auth_manager, thread_manager) = match (auth_manager, thread_manager) {
|
||||
(Some(auth_manager), Some(thread_manager)) => (auth_manager, thread_manager),
|
||||
(None, None) => {
|
||||
let auth_manager = AuthManager::shared(
|
||||
config.codex_home.clone(),
|
||||
enable_codex_api_key_env,
|
||||
config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.as_ref(),
|
||||
auth_manager.clone(),
|
||||
session_source,
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
.enabled(Feature::DefaultModeRequestUserInput),
|
||||
},
|
||||
));
|
||||
(auth_manager, thread_manager)
|
||||
}
|
||||
_ => panic!("MessageProcessorArgs must provide both auth_manager and thread_manager"),
|
||||
};
|
||||
let auth_manager = AuthManager::shared(
|
||||
config.codex_home.clone(),
|
||||
enable_codex_api_key_env,
|
||||
config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.as_ref(),
|
||||
auth_manager.clone(),
|
||||
session_source,
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
.enabled(Feature::DefaultModeRequestUserInput),
|
||||
},
|
||||
));
|
||||
auth_manager.set_forced_chatgpt_workspace_id(config.forced_chatgpt_workspace_id.clone());
|
||||
auth_manager.set_external_auth_refresher(Arc::new(ExternalAuthRefreshBridge {
|
||||
outgoing: outgoing.clone(),
|
||||
|
||||
@@ -239,8 +239,6 @@ fn build_test_processor(
|
||||
cli_overrides: Vec::new(),
|
||||
loader_overrides: LoaderOverrides::default(),
|
||||
cloud_requirements: CloudRequirementsLoader::default(),
|
||||
auth_manager: None,
|
||||
thread_manager: None,
|
||||
feedback: CodexFeedback::new(),
|
||||
log_db: None,
|
||||
config_warnings: Vec::new(),
|
||||
|
||||
@@ -527,38 +527,6 @@ impl OutgoingMessageSender {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn send_notification_to_connections(
|
||||
&self,
|
||||
connection_ids: &[ConnectionId],
|
||||
notification: OutgoingNotification,
|
||||
) {
|
||||
let outgoing_message = OutgoingMessage::Notification(notification);
|
||||
if connection_ids.is_empty() {
|
||||
if let Err(err) = self
|
||||
.sender
|
||||
.send(OutgoingEnvelope::Broadcast {
|
||||
message: outgoing_message,
|
||||
})
|
||||
.await
|
||||
{
|
||||
warn!("failed to send notification to client: {err:?}");
|
||||
}
|
||||
return;
|
||||
}
|
||||
for connection_id in connection_ids {
|
||||
if let Err(err) = self
|
||||
.sender
|
||||
.send(OutgoingEnvelope::ToConnection {
|
||||
connection_id: *connection_id,
|
||||
message: outgoing_message.clone(),
|
||||
})
|
||||
.await
|
||||
{
|
||||
warn!("failed to send notification to client: {err:?}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn send_error(
|
||||
&self,
|
||||
request_id: ConnectionRequestId,
|
||||
@@ -616,7 +584,6 @@ impl OutgoingMessageSender {
|
||||
#[serde(untagged)]
|
||||
pub(crate) enum OutgoingMessage {
|
||||
Request(ServerRequest),
|
||||
Notification(OutgoingNotification),
|
||||
/// AppServerNotification is specific to the case where this is run as an
|
||||
/// "app server" as opposed to an MCP server.
|
||||
AppServerNotification(ServerNotification),
|
||||
@@ -624,13 +591,6 @@ pub(crate) enum OutgoingMessage {
|
||||
Error(OutgoingError),
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize)]
|
||||
pub(crate) struct OutgoingNotification {
|
||||
pub method: String,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub params: Option<serde_json::Value>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize)]
|
||||
pub(crate) struct OutgoingResponse {
|
||||
pub id: RequestId,
|
||||
|
||||
@@ -188,7 +188,6 @@ pub(crate) enum TransportEvent {
|
||||
ConnectionOpened {
|
||||
connection_id: ConnectionId,
|
||||
writer: mpsc::Sender<OutgoingMessage>,
|
||||
allow_legacy_notifications: bool,
|
||||
disconnect_sender: Option<CancellationToken>,
|
||||
},
|
||||
ConnectionClosed {
|
||||
@@ -226,7 +225,6 @@ pub(crate) struct OutboundConnectionState {
|
||||
pub(crate) initialized: Arc<AtomicBool>,
|
||||
pub(crate) experimental_api_enabled: Arc<AtomicBool>,
|
||||
pub(crate) opted_out_notification_methods: Arc<RwLock<HashSet<String>>>,
|
||||
pub(crate) allow_legacy_notifications: bool,
|
||||
pub(crate) writer: mpsc::Sender<OutgoingMessage>,
|
||||
disconnect_sender: Option<CancellationToken>,
|
||||
}
|
||||
@@ -237,14 +235,12 @@ impl OutboundConnectionState {
|
||||
initialized: Arc<AtomicBool>,
|
||||
experimental_api_enabled: Arc<AtomicBool>,
|
||||
opted_out_notification_methods: Arc<RwLock<HashSet<String>>>,
|
||||
allow_legacy_notifications: bool,
|
||||
disconnect_sender: Option<CancellationToken>,
|
||||
) -> Self {
|
||||
Self {
|
||||
initialized,
|
||||
experimental_api_enabled,
|
||||
opted_out_notification_methods,
|
||||
allow_legacy_notifications,
|
||||
writer,
|
||||
disconnect_sender,
|
||||
}
|
||||
@@ -272,7 +268,6 @@ pub(crate) async fn start_stdio_connection(
|
||||
.send(TransportEvent::ConnectionOpened {
|
||||
connection_id,
|
||||
writer: writer_tx,
|
||||
allow_legacy_notifications: false,
|
||||
disconnect_sender: None,
|
||||
})
|
||||
.await
|
||||
@@ -376,7 +371,6 @@ async fn run_websocket_connection(
|
||||
.send(TransportEvent::ConnectionOpened {
|
||||
connection_id,
|
||||
writer: writer_tx,
|
||||
allow_legacy_notifications: false,
|
||||
disconnect_sender: Some(disconnect_token.clone()),
|
||||
})
|
||||
.await
|
||||
@@ -584,16 +578,6 @@ fn should_skip_notification_for_connection(
|
||||
connection_state: &OutboundConnectionState,
|
||||
message: &OutgoingMessage,
|
||||
) -> bool {
|
||||
if !connection_state.allow_legacy_notifications
|
||||
&& matches!(message, OutgoingMessage::Notification(_))
|
||||
{
|
||||
// Raw legacy `codex/event/*` notifications are still emitted upstream
|
||||
// for in-process compatibility, but they are no longer part of the
|
||||
// external app-server contract. Keep dropping them here until the
|
||||
// producer path can be deleted entirely.
|
||||
return true;
|
||||
}
|
||||
|
||||
let Ok(opted_out_notification_methods) = connection_state.opted_out_notification_methods.read()
|
||||
else {
|
||||
warn!("failed to read outbound opted-out notifications");
|
||||
@@ -604,9 +588,6 @@ fn should_skip_notification_for_connection(
|
||||
let method = notification.to_string();
|
||||
opted_out_notification_methods.contains(method.as_str())
|
||||
}
|
||||
OutgoingMessage::Notification(notification) => {
|
||||
opted_out_notification_methods.contains(notification.method.as_str())
|
||||
}
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
@@ -719,6 +700,8 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::error_code::OVERLOADED_ERROR_CODE;
|
||||
use codex_app_server_protocol::CommandExecutionRequestApprovalSkillMetadata;
|
||||
use codex_app_server_protocol::ConfigWarningNotification;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
@@ -921,11 +904,13 @@ mod tests {
|
||||
.expect("transport queue should accept first message");
|
||||
|
||||
writer_tx
|
||||
.send(OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "queued".to_string(),
|
||||
params: None,
|
||||
},
|
||||
.send(OutgoingMessage::AppServerNotification(
|
||||
ServerNotification::ConfigWarning(ConfigWarningNotification {
|
||||
summary: "queued".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
}),
|
||||
))
|
||||
.await
|
||||
.expect("writer queue should accept first message");
|
||||
@@ -950,7 +935,16 @@ mod tests {
|
||||
.await
|
||||
.expect("writer queue should still contain original message");
|
||||
let queued_json = serde_json::to_value(queued_outgoing).expect("serialize queued message");
|
||||
assert_eq!(queued_json, json!({ "method": "queued" }));
|
||||
assert_eq!(
|
||||
queued_json,
|
||||
json!({
|
||||
"method": "configWarning",
|
||||
"params": {
|
||||
"summary": "queued",
|
||||
"details": null,
|
||||
},
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -958,9 +952,8 @@ mod tests {
|
||||
let connection_id = ConnectionId(7);
|
||||
let (writer_tx, mut writer_rx) = mpsc::channel(1);
|
||||
let initialized = Arc::new(AtomicBool::new(true));
|
||||
let opted_out_notification_methods = Arc::new(RwLock::new(HashSet::from([
|
||||
"codex/event/task_started".to_string(),
|
||||
])));
|
||||
let opted_out_notification_methods =
|
||||
Arc::new(RwLock::new(HashSet::from(["configWarning".to_string()])));
|
||||
|
||||
let mut connections = HashMap::new();
|
||||
connections.insert(
|
||||
@@ -970,7 +963,6 @@ mod tests {
|
||||
initialized,
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
opted_out_notification_methods,
|
||||
false,
|
||||
None,
|
||||
),
|
||||
);
|
||||
@@ -979,12 +971,14 @@ mod tests {
|
||||
&mut connections,
|
||||
OutgoingEnvelope::ToConnection {
|
||||
connection_id,
|
||||
message: OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "codex/event/task_started".to_string(),
|
||||
params: None,
|
||||
message: OutgoingMessage::AppServerNotification(ServerNotification::ConfigWarning(
|
||||
ConfigWarningNotification {
|
||||
summary: "task_started".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
},
|
||||
),
|
||||
)),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
@@ -995,89 +989,6 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn to_connection_legacy_notifications_are_dropped_for_external_clients() {
|
||||
let connection_id = ConnectionId(10);
|
||||
let (writer_tx, mut writer_rx) = mpsc::channel(1);
|
||||
|
||||
let mut connections = HashMap::new();
|
||||
connections.insert(
|
||||
connection_id,
|
||||
OutboundConnectionState::new(
|
||||
writer_tx,
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
None,
|
||||
),
|
||||
);
|
||||
|
||||
route_outgoing_envelope(
|
||||
&mut connections,
|
||||
OutgoingEnvelope::ToConnection {
|
||||
connection_id,
|
||||
message: OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "codex/event/task_started".to_string(),
|
||||
params: None,
|
||||
},
|
||||
),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
writer_rx.try_recv().is_err(),
|
||||
"legacy notifications should not reach external clients"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn to_connection_legacy_notifications_are_preserved_for_in_process_clients() {
|
||||
let connection_id = ConnectionId(11);
|
||||
let (writer_tx, mut writer_rx) = mpsc::channel(1);
|
||||
|
||||
let mut connections = HashMap::new();
|
||||
connections.insert(
|
||||
connection_id,
|
||||
OutboundConnectionState::new(
|
||||
writer_tx,
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
true,
|
||||
None,
|
||||
),
|
||||
);
|
||||
|
||||
route_outgoing_envelope(
|
||||
&mut connections,
|
||||
OutgoingEnvelope::ToConnection {
|
||||
connection_id,
|
||||
message: OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "codex/event/task_started".to_string(),
|
||||
params: None,
|
||||
},
|
||||
),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
let message = writer_rx
|
||||
.recv()
|
||||
.await
|
||||
.expect("legacy notification should reach in-process clients");
|
||||
assert!(matches!(
|
||||
message,
|
||||
OutgoingMessage::Notification(crate::outgoing_message::OutgoingNotification {
|
||||
method,
|
||||
params: None,
|
||||
}) if method == "codex/event/task_started"
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn command_execution_request_approval_strips_experimental_fields_without_capability() {
|
||||
let connection_id = ConnectionId(8);
|
||||
@@ -1091,7 +1002,6 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(false)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
None,
|
||||
),
|
||||
);
|
||||
@@ -1158,7 +1068,6 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
None,
|
||||
),
|
||||
);
|
||||
@@ -1246,7 +1155,6 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
Some(fast_disconnect_token.clone()),
|
||||
),
|
||||
);
|
||||
@@ -1257,25 +1165,30 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
Some(slow_disconnect_token.clone()),
|
||||
),
|
||||
);
|
||||
|
||||
let queued_message =
|
||||
OutgoingMessage::Notification(crate::outgoing_message::OutgoingNotification {
|
||||
method: "codex/event/already-buffered".to_string(),
|
||||
params: None,
|
||||
});
|
||||
let queued_message = OutgoingMessage::AppServerNotification(
|
||||
ServerNotification::ConfigWarning(ConfigWarningNotification {
|
||||
summary: "already-buffered".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
}),
|
||||
);
|
||||
slow_writer_tx
|
||||
.try_send(queued_message)
|
||||
.expect("channel should have room");
|
||||
|
||||
let broadcast_message =
|
||||
OutgoingMessage::Notification(crate::outgoing_message::OutgoingNotification {
|
||||
method: "codex/event/test".to_string(),
|
||||
params: None,
|
||||
});
|
||||
let broadcast_message = OutgoingMessage::AppServerNotification(
|
||||
ServerNotification::ConfigWarning(ConfigWarningNotification {
|
||||
summary: "test".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
}),
|
||||
);
|
||||
timeout(
|
||||
Duration::from_millis(100),
|
||||
route_outgoing_envelope(
|
||||
@@ -1286,24 +1199,28 @@ mod tests {
|
||||
),
|
||||
)
|
||||
.await
|
||||
.expect("broadcast should return even when legacy notifications are dropped");
|
||||
assert!(connections.contains_key(&slow_connection_id));
|
||||
assert!(!slow_disconnect_token.is_cancelled());
|
||||
.expect("broadcast should return even when one connection is slow");
|
||||
assert!(!connections.contains_key(&slow_connection_id));
|
||||
assert!(slow_disconnect_token.is_cancelled());
|
||||
assert!(!fast_disconnect_token.is_cancelled());
|
||||
assert!(
|
||||
fast_writer_rx.try_recv().is_err(),
|
||||
"broadcast legacy notification should be dropped for fast connections"
|
||||
);
|
||||
let fast_message = fast_writer_rx
|
||||
.try_recv()
|
||||
.expect("fast connection should receive the broadcast notification");
|
||||
assert!(matches!(
|
||||
fast_message,
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::ConfigWarning(
|
||||
ConfigWarningNotification { summary, .. }
|
||||
)) if summary == "test"
|
||||
));
|
||||
|
||||
let slow_message = slow_writer_rx
|
||||
.try_recv()
|
||||
.expect("slow connection should retain its original buffered message");
|
||||
assert!(matches!(
|
||||
slow_message,
|
||||
OutgoingMessage::Notification(crate::outgoing_message::OutgoingNotification {
|
||||
method,
|
||||
params: None,
|
||||
}) if method == "codex/event/already-buffered"
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::ConfigWarning(
|
||||
ConfigWarningNotification { summary, .. }
|
||||
)) if summary == "already-buffered"
|
||||
));
|
||||
}
|
||||
|
||||
@@ -1312,11 +1229,13 @@ mod tests {
|
||||
let connection_id = ConnectionId(3);
|
||||
let (writer_tx, mut writer_rx) = mpsc::channel(1);
|
||||
writer_tx
|
||||
.send(OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "queued".to_string(),
|
||||
params: None,
|
||||
},
|
||||
.send(OutgoingMessage::AppServerNotification(
|
||||
ServerNotification::ConfigWarning(ConfigWarningNotification {
|
||||
summary: "queued".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
}),
|
||||
))
|
||||
.await
|
||||
.expect("channel should accept the first queued message");
|
||||
@@ -1329,7 +1248,6 @@ mod tests {
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(AtomicBool::new(true)),
|
||||
Arc::new(RwLock::new(HashSet::new())),
|
||||
false,
|
||||
None,
|
||||
),
|
||||
);
|
||||
@@ -1339,11 +1257,13 @@ mod tests {
|
||||
&mut connections,
|
||||
OutgoingEnvelope::ToConnection {
|
||||
connection_id,
|
||||
message: OutgoingMessage::Notification(
|
||||
crate::outgoing_message::OutgoingNotification {
|
||||
method: "second".to_string(),
|
||||
params: None,
|
||||
},
|
||||
message: OutgoingMessage::AppServerNotification(
|
||||
ServerNotification::ConfigWarning(ConfigWarningNotification {
|
||||
summary: "second".to_string(),
|
||||
details: None,
|
||||
path: None,
|
||||
range: None,
|
||||
}),
|
||||
),
|
||||
},
|
||||
)
|
||||
@@ -1356,20 +1276,23 @@ mod tests {
|
||||
.expect("first queued message should exist");
|
||||
timeout(Duration::from_millis(100), route_task)
|
||||
.await
|
||||
.expect("routing should finish immediately when legacy notifications are dropped")
|
||||
.expect("routing should finish after the first queued message is drained")
|
||||
.expect("routing task should succeed");
|
||||
|
||||
assert!(matches!(
|
||||
first,
|
||||
OutgoingMessage::Notification(crate::outgoing_message::OutgoingNotification {
|
||||
method,
|
||||
params: None,
|
||||
}) if method == "queued"
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::ConfigWarning(
|
||||
ConfigWarningNotification { summary, .. }
|
||||
)) if summary == "queued"
|
||||
));
|
||||
let second = writer_rx
|
||||
.try_recv()
|
||||
.expect("second notification should be delivered once the queue has room");
|
||||
assert!(matches!(
|
||||
writer_rx.try_recv(),
|
||||
Err(tokio::sync::mpsc::error::TryRecvError::Empty)
|
||||
| Err(tokio::sync::mpsc::error::TryRecvError::Disconnected)
|
||||
second,
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::ConfigWarning(
|
||||
ConfigWarningNotification { summary, .. }
|
||||
)) if summary == "second"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user