mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
feat(app-server): update remote control APIs for better UX (#22877)
## Why To help improve `codex remote-control` CLI UX which I plan to do in a followup, this PR adds `server-name` to the various remote control APIs: - `remoteControl/enable` - `remoteControl/disable` - `remoteControl/status/changed` Also, add a `remoteControl/status/read` API. This will be helpful in the Codex App.
This commit is contained in:
1 parent
98129fb9c5
commit
6a331a66eb
17 files changed
+276
-33
No files matched your search
@@ -6,7 +6,6 @@ use codex_api::SharedAuthProvider;
|
||||
use codex_login::default_client::build_reqwest_client;
|
||||
use codex_state::RemoteControlEnrollmentRecord;
|
||||
use codex_state::StateRuntime;
|
||||
use gethostname::gethostname;
|
||||
use std::io;
|
||||
use std::io::ErrorKind;
|
||||
use tracing::info;
|
||||
@@ -195,11 +194,11 @@ pub(super) async fn enroll_remote_control_server(
|
||||
remote_control_target: &RemoteControlTarget,
|
||||
auth: &RemoteControlConnectionAuth,
|
||||
installation_id: &str,
|
||||
server_name: &str,
|
||||
) -> io::Result<RemoteControlEnrollment> {
|
||||
let enroll_url = &remote_control_target.enroll_url;
|
||||
let server_name = gethostname().to_string_lossy().trim().to_string();
|
||||
let request = EnrollRemoteServerRequest {
|
||||
name: server_name.clone(),
|
||||
name: server_name.to_string(),
|
||||
os: std::env::consts::OS,
|
||||
arch: std::env::consts::ARCH,
|
||||
app_server_version: env!("CARGO_PKG_VERSION"),
|
||||
@@ -255,7 +254,7 @@ pub(super) async fn enroll_remote_control_server(
|
||||
account_id: auth.account_id.clone(),
|
||||
environment_id: enrollment.environment_id,
|
||||
server_id: enrollment.server_id,
|
||||
server_name,
|
||||
server_name: server_name.to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -464,6 +463,7 @@ mod tests {
|
||||
account_id: "account_id".to_string(),
|
||||
},
|
||||
"11111111-1111-4111-8111-111111111111",
|
||||
"test-server",
|
||||
)
|
||||
.await
|
||||
.expect_err("invalid response should fail to parse");
|
||||
|
||||
@@ -19,6 +19,7 @@ use codex_app_server_protocol::RemoteControlConnectionStatus;
|
||||
use codex_app_server_protocol::RemoteControlStatusChangedNotification;
|
||||
use codex_login::AuthManager;
|
||||
use codex_state::StateRuntime;
|
||||
use gethostname::gethostname;
|
||||
use std::error::Error;
|
||||
use std::fmt;
|
||||
use std::io;
|
||||
@@ -112,15 +113,8 @@ impl RemoteControlHandle {
|
||||
connection_status: RemoteControlConnectionStatus,
|
||||
) -> RemoteControlStatusChangedNotification {
|
||||
self.status_tx.send_if_modified(|status| {
|
||||
let next_status = RemoteControlStatusChangedNotification {
|
||||
status: connection_status,
|
||||
installation_id: status.installation_id.clone(),
|
||||
environment_id: if connection_status == RemoteControlConnectionStatus::Disabled {
|
||||
None
|
||||
} else {
|
||||
status.environment_id.clone()
|
||||
},
|
||||
};
|
||||
let next_status =
|
||||
remote_control_status_with_connection_status(status, connection_status);
|
||||
if *status == next_status {
|
||||
return false;
|
||||
}
|
||||
@@ -132,6 +126,22 @@ impl RemoteControlHandle {
|
||||
}
|
||||
}
|
||||
|
||||
fn remote_control_status_with_connection_status(
|
||||
status: &RemoteControlStatusChangedNotification,
|
||||
connection_status: RemoteControlConnectionStatus,
|
||||
) -> RemoteControlStatusChangedNotification {
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: connection_status,
|
||||
server_name: status.server_name.clone(),
|
||||
installation_id: status.installation_id.clone(),
|
||||
environment_id: if connection_status == RemoteControlConnectionStatus::Disabled {
|
||||
None
|
||||
} else {
|
||||
status.environment_id.clone()
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn start_remote_control(
|
||||
config: RemoteControlStartConfig,
|
||||
state_db: Option<Arc<StateRuntime>>,
|
||||
@@ -154,12 +164,14 @@ pub async fn start_remote_control(
|
||||
};
|
||||
|
||||
let (enabled_tx, enabled_rx) = watch::channel(initial_enabled);
|
||||
let server_name = gethostname().to_string_lossy().trim().to_string();
|
||||
let initial_status = RemoteControlStatusChangedNotification {
|
||||
status: if initial_enabled {
|
||||
RemoteControlConnectionStatus::Connecting
|
||||
} else {
|
||||
RemoteControlConnectionStatus::Disabled
|
||||
},
|
||||
server_name: server_name.clone(),
|
||||
installation_id: config.installation_id.clone(),
|
||||
environment_id: None,
|
||||
};
|
||||
@@ -171,6 +183,7 @@ pub async fn start_remote_control(
|
||||
remote_control_url: config.remote_control_url,
|
||||
installation_id: config.installation_id,
|
||||
remote_control_target,
|
||||
server_name,
|
||||
},
|
||||
state_db,
|
||||
auth_manager,
|
||||
|
||||
@@ -121,6 +121,10 @@ fn remote_control_url_for_listener(listener: &TcpListener) -> String {
|
||||
format!("http://{addr}/backend-api/")
|
||||
}
|
||||
|
||||
fn test_server_name() -> String {
|
||||
gethostname().to_string_lossy().trim().to_string()
|
||||
}
|
||||
|
||||
async fn expect_remote_control_status(
|
||||
status_rx: &mut watch::Receiver<RemoteControlStatusChangedNotification>,
|
||||
expected_status: Option<RemoteControlConnectionStatus>,
|
||||
@@ -134,6 +138,7 @@ async fn expect_remote_control_status(
|
||||
if let Some(expected_status) = expected_status {
|
||||
assert_eq!(status.status, expected_status);
|
||||
}
|
||||
assert_eq!(status.server_name, test_server_name());
|
||||
assert_eq!(status.installation_id, TEST_INSTALLATION_ID);
|
||||
assert_eq!(status.environment_id.as_deref(), expected_environment_id);
|
||||
}
|
||||
@@ -630,6 +635,7 @@ async fn remote_control_start_reports_missing_state_db_as_disabled_when_enabled(
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Disabled,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
@@ -698,6 +704,7 @@ async fn remote_control_handle_enable_disable_stops_and_restarts_connections() {
|
||||
&mut status_rx,
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connected,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: Some("env_test".to_string()),
|
||||
},
|
||||
@@ -708,6 +715,7 @@ async fn remote_control_handle_enable_disable_stops_and_restarts_connections() {
|
||||
remote_handle.disable(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Disabled,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
@@ -716,6 +724,7 @@ async fn remote_control_handle_enable_disable_stops_and_restarts_connections() {
|
||||
&mut status_rx,
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Disabled,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
},
|
||||
@@ -732,6 +741,7 @@ async fn remote_control_handle_enable_disable_stops_and_restarts_connections() {
|
||||
remote_handle.enable().expect("enable should succeed"),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
@@ -740,6 +750,7 @@ async fn remote_control_handle_enable_disable_stops_and_restarts_connections() {
|
||||
&mut status_rx,
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: test_server_name(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
},
|
||||
|
||||
@@ -15,6 +15,7 @@ use super::protocol::ClientId;
|
||||
use super::protocol::RemoteControlTarget;
|
||||
use super::protocol::ServerEnvelope;
|
||||
use super::protocol::StreamId;
|
||||
use super::remote_control_status_with_connection_status;
|
||||
use super::segment::ClientSegmentObservation;
|
||||
use super::segment::ClientSegmentReassembler;
|
||||
use super::segment::REMOTE_CONTROL_SEGMENT_MAX_BYTES;
|
||||
@@ -216,6 +217,7 @@ impl WebsocketState {
|
||||
pub(crate) struct RemoteControlWebsocket {
|
||||
remote_control_url: String,
|
||||
installation_id: String,
|
||||
server_name: String,
|
||||
remote_control_target: Option<RemoteControlTarget>,
|
||||
state_db: Option<Arc<StateRuntime>>,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
@@ -235,6 +237,7 @@ pub(crate) struct RemoteControlWebsocketConfig {
|
||||
pub(crate) remote_control_url: String,
|
||||
pub(crate) installation_id: String,
|
||||
pub(crate) remote_control_target: Option<RemoteControlTarget>,
|
||||
pub(crate) server_name: String,
|
||||
}
|
||||
|
||||
enum ConnectOutcome {
|
||||
@@ -260,15 +263,8 @@ impl RemoteControlStatusPublisher {
|
||||
|
||||
fn publish_status(&self, connection_status: RemoteControlConnectionStatus) {
|
||||
self.tx.send_if_modified(|status| {
|
||||
let next_status = RemoteControlStatusChangedNotification {
|
||||
status: connection_status,
|
||||
installation_id: status.installation_id.clone(),
|
||||
environment_id: if connection_status == RemoteControlConnectionStatus::Disabled {
|
||||
None
|
||||
} else {
|
||||
status.environment_id.clone()
|
||||
},
|
||||
};
|
||||
let next_status =
|
||||
remote_control_status_with_connection_status(status, connection_status);
|
||||
if *status == next_status {
|
||||
return false;
|
||||
}
|
||||
@@ -285,6 +281,7 @@ impl RemoteControlStatusPublisher {
|
||||
}
|
||||
let next_status = RemoteControlStatusChangedNotification {
|
||||
status: status.status,
|
||||
server_name: status.server_name.clone(),
|
||||
installation_id: status.installation_id.clone(),
|
||||
environment_id,
|
||||
};
|
||||
@@ -301,6 +298,7 @@ impl RemoteControlStatusPublisher {
|
||||
#[derive(Clone, Copy)]
|
||||
pub(super) struct RemoteControlConnectOptions<'a> {
|
||||
installation_id: &'a str,
|
||||
server_name: &'a str,
|
||||
subscribe_cursor: Option<&'a str>,
|
||||
app_server_client_name: Option<&'a str>,
|
||||
}
|
||||
@@ -327,6 +325,7 @@ impl RemoteControlWebsocket {
|
||||
Self {
|
||||
remote_control_url: config.remote_control_url,
|
||||
installation_id: config.installation_id,
|
||||
server_name: config.server_name,
|
||||
remote_control_target: config.remote_control_target,
|
||||
state_db,
|
||||
auth_manager,
|
||||
@@ -454,6 +453,7 @@ impl RemoteControlWebsocket {
|
||||
let subscribe_cursor = self.state.lock().await.subscribe_cursor.clone();
|
||||
let connect_options = RemoteControlConnectOptions {
|
||||
installation_id: &self.installation_id,
|
||||
server_name: &self.server_name,
|
||||
subscribe_cursor: subscribe_cursor.as_deref(),
|
||||
app_server_client_name,
|
||||
};
|
||||
@@ -1076,7 +1076,10 @@ pub(super) async fn connect_remote_control_websocket(
|
||||
if let Some(loaded_enrollment) = loaded_enrollment.as_ref() {
|
||||
status_publisher.publish_environment_id(Some(loaded_enrollment.environment_id.clone()));
|
||||
}
|
||||
*enrollment = loaded_enrollment;
|
||||
*enrollment = loaded_enrollment.map(|mut enrollment| {
|
||||
enrollment.server_name = connect_options.server_name.to_string();
|
||||
enrollment
|
||||
});
|
||||
}
|
||||
|
||||
if enrollment.is_none() {
|
||||
@@ -1088,6 +1091,7 @@ pub(super) async fn connect_remote_control_websocket(
|
||||
remote_control_target,
|
||||
&auth,
|
||||
connect_options.installation_id,
|
||||
connect_options.server_name,
|
||||
)
|
||||
.await
|
||||
{
|
||||
@@ -1279,6 +1283,7 @@ mod tests {
|
||||
) {
|
||||
let (status_tx, status_rx) = watch::channel(RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
});
|
||||
@@ -1386,6 +1391,7 @@ mod tests {
|
||||
&mut enrollment,
|
||||
RemoteControlConnectOptions {
|
||||
installation_id: TEST_INSTALLATION_ID,
|
||||
server_name: "test-server",
|
||||
subscribe_cursor: None,
|
||||
app_server_client_name: None,
|
||||
},
|
||||
@@ -1403,6 +1409,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: Some("env_test".to_string()),
|
||||
}
|
||||
@@ -1464,6 +1471,7 @@ mod tests {
|
||||
&mut enrollment,
|
||||
RemoteControlConnectOptions {
|
||||
installation_id: TEST_INSTALLATION_ID,
|
||||
server_name: "test-server",
|
||||
subscribe_cursor: None,
|
||||
app_server_client_name: None,
|
||||
},
|
||||
@@ -1477,6 +1485,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: Some("env_test".to_string()),
|
||||
}
|
||||
@@ -1546,6 +1555,7 @@ mod tests {
|
||||
&mut enrollment,
|
||||
RemoteControlConnectOptions {
|
||||
installation_id: TEST_INSTALLATION_ID,
|
||||
server_name: "test-server",
|
||||
subscribe_cursor: None,
|
||||
app_server_client_name: None,
|
||||
},
|
||||
@@ -1599,6 +1609,7 @@ mod tests {
|
||||
&mut enrollment,
|
||||
RemoteControlConnectOptions {
|
||||
installation_id: TEST_INSTALLATION_ID,
|
||||
server_name: "test-server",
|
||||
subscribe_cursor: None,
|
||||
app_server_client_name: None,
|
||||
},
|
||||
@@ -1647,6 +1658,7 @@ mod tests {
|
||||
&mut enrollment,
|
||||
RemoteControlConnectOptions {
|
||||
installation_id: TEST_INSTALLATION_ID,
|
||||
server_name: "test-server",
|
||||
subscribe_cursor: None,
|
||||
app_server_client_name: None,
|
||||
},
|
||||
@@ -1665,6 +1677,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
@@ -1694,6 +1707,7 @@ mod tests {
|
||||
remote_control_url,
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
remote_control_target: Some(remote_control_target),
|
||||
server_name: "test-server".to_string(),
|
||||
},
|
||||
/*state_db*/ None,
|
||||
remote_control_auth_manager(),
|
||||
@@ -1738,6 +1752,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connecting,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: Some("env_first".to_string()),
|
||||
}
|
||||
@@ -1759,6 +1774,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connected,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: Some("env_first".to_string()),
|
||||
}
|
||||
@@ -1773,6 +1789,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Connected,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
@@ -1788,6 +1805,7 @@ mod tests {
|
||||
status_rx.borrow().clone(),
|
||||
RemoteControlStatusChangedNotification {
|
||||
status: RemoteControlConnectionStatus::Disabled,
|
||||
server_name: "test-server".to_string(),
|
||||
installation_id: TEST_INSTALLATION_ID.to_string(),
|
||||
environment_id: None,
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user