mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
mark Feature::RemoteControl as removed (#22386)
## Why `remote_control` can appear in `config.toml`, CLI feature overrides, and the app-server config APIs. Before this PR, app-server startup treated `config.features.enabled(Feature::RemoteControl)` as the signal to start remote control ([base code](https://github.com/openai/codex/blob/5e3ee5eddfa5333f2e0b011880abf0cbf92bd295/codex-rs/app-server/src/lib.rs#L678-L680)). That meant a user with: ```toml [features] remote_control = true ``` would accidentally opt every app-server process into remote control. Remote-control startup should instead be a per-process launch decision made by CLI flags. ## What Changed - Marks `Feature::RemoteControl` as `Stage::Removed`, keeping `remote_control` as a known compatibility key while making it config-inert. - Adds a hidden `--remote-control` process flag to `codex app-server` and standalone `codex-app-server`. - Plumbs that flag through `AppServerRuntimeOptions.remote_control_enabled` and makes app-server startup use only that runtime option to decide whether to start remote control. - Removes the app-server config mutation hook that reloaded config and toggled remote control at runtime. - Updates managed daemon spawning to use `codex app-server --remote-control --listen unix://` instead of `--enable remote_control`. Config APIs can still list, read, write, and set `remote_control`; those operations just no longer affect remote-control process enrollment.
This commit is contained in:
committed by
GitHub
Unverified
parent
1ae9867296
commit
2237a13cf1
@@ -349,13 +349,7 @@ impl PidBackend {
|
||||
match self.command_kind {
|
||||
PidCommandKind::AppServer {
|
||||
remote_control_enabled: true,
|
||||
} => vec![
|
||||
"--enable",
|
||||
"remote_control",
|
||||
"app-server",
|
||||
"--listen",
|
||||
"unix://",
|
||||
],
|
||||
} => vec!["app-server", "--remote-control", "--listen", "unix://"],
|
||||
PidCommandKind::AppServer {
|
||||
remote_control_enabled: false,
|
||||
} => vec!["app-server", "--listen", "unix://"],
|
||||
|
||||
@@ -430,7 +430,6 @@ async fn start_uninitialized(args: InProcessStartArgs) -> IoResult<InProcessClie
|
||||
auth_manager,
|
||||
installation_id,
|
||||
rpc_transport: AppServerRpcTransport::InProcess,
|
||||
remote_control_handle: None,
|
||||
plugin_startup_tasks: crate::PluginStartupTasks::Start,
|
||||
}));
|
||||
let mut thread_created_rx = processor.thread_created_receiver();
|
||||
|
||||
@@ -8,7 +8,6 @@ use codex_config::RemoteThreadConfigLoader;
|
||||
use codex_config::ThreadConfigLoader;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::resolve_installation_id;
|
||||
use codex_features::Feature;
|
||||
use codex_login::AuthManager;
|
||||
use codex_utils_cli::CliConfigOverrides;
|
||||
use std::collections::HashMap;
|
||||
@@ -392,12 +391,14 @@ pub enum PluginStartupTasks {
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub struct AppServerRuntimeOptions {
|
||||
pub plugin_startup_tasks: PluginStartupTasks,
|
||||
pub remote_control_enabled: bool,
|
||||
}
|
||||
|
||||
impl Default for AppServerRuntimeOptions {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
plugin_startup_tasks: PluginStartupTasks::Start,
|
||||
remote_control_enabled: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -675,15 +676,15 @@ pub async fn run_main_with_transport_options(
|
||||
let auth_manager =
|
||||
AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await;
|
||||
|
||||
let remote_control_config_enabled = config.features.enabled(Feature::RemoteControl);
|
||||
let remote_control_enabled = remote_control_config_enabled && state_db.is_some();
|
||||
if remote_control_config_enabled && state_db.is_none() {
|
||||
let remote_control_requested = runtime_options.remote_control_enabled;
|
||||
let remote_control_enabled = remote_control_requested && state_db.is_some();
|
||||
if remote_control_requested && state_db.is_none() {
|
||||
error!("remote control disabled because sqlite state db is unavailable");
|
||||
}
|
||||
if transport_accept_handles.is_empty() && !remote_control_enabled {
|
||||
return Err(std::io::Error::new(
|
||||
ErrorKind::InvalidInput,
|
||||
if remote_control_config_enabled && state_db.is_none() {
|
||||
if remote_control_requested && state_db.is_none() {
|
||||
"no transport configured; remote control disabled because sqlite state db is unavailable"
|
||||
} else {
|
||||
"no transport configured; use --listen or enable remote control"
|
||||
@@ -787,7 +788,6 @@ pub async fn run_main_with_transport_options(
|
||||
auth_manager,
|
||||
installation_id,
|
||||
rpc_transport: analytics_rpc_transport(&transport),
|
||||
remote_control_handle: Some(remote_control_handle.clone()),
|
||||
plugin_startup_tasks: runtime_options.plugin_startup_tasks,
|
||||
}));
|
||||
let mut thread_created_rx = processor.thread_created_receiver();
|
||||
|
||||
@@ -40,6 +40,10 @@ struct AppServerArgs {
|
||||
#[cfg(debug_assertions)]
|
||||
#[arg(long = "disable-plugin-startup-tasks-for-tests", hide = true)]
|
||||
disable_plugin_startup_tasks_for_tests: bool,
|
||||
|
||||
/// Enable remote control for this app-server process.
|
||||
#[arg(long = "remote-control", hide = true)]
|
||||
remote_control: bool,
|
||||
}
|
||||
|
||||
fn main() -> anyhow::Result<()> {
|
||||
@@ -59,6 +63,7 @@ fn main() -> anyhow::Result<()> {
|
||||
if args.disable_plugin_startup_tasks_for_tests {
|
||||
runtime_options.plugin_startup_tasks = PluginStartupTasks::Skip;
|
||||
}
|
||||
runtime_options.remote_control_enabled = args.remote_control;
|
||||
|
||||
run_main_with_transport_options(
|
||||
arg0_paths,
|
||||
|
||||
@@ -42,7 +42,6 @@ use crate::skills_watcher::SkillsWatcher;
|
||||
use crate::thread_state::ConnectionCapabilities;
|
||||
use crate::thread_state::ThreadStateManager;
|
||||
use crate::transport::AppServerTransport;
|
||||
use crate::transport::RemoteControlHandle;
|
||||
use async_trait::async_trait;
|
||||
use codex_analytics::AnalyticsEventsClient;
|
||||
use codex_analytics::AppServerRpcTransport;
|
||||
@@ -265,7 +264,6 @@ pub(crate) struct MessageProcessorArgs {
|
||||
pub(crate) auth_manager: Arc<AuthManager>,
|
||||
pub(crate) installation_id: String,
|
||||
pub(crate) rpc_transport: AppServerRpcTransport,
|
||||
pub(crate) remote_control_handle: Option<RemoteControlHandle>,
|
||||
pub(crate) plugin_startup_tasks: crate::PluginStartupTasks,
|
||||
}
|
||||
|
||||
@@ -288,7 +286,6 @@ impl MessageProcessor {
|
||||
auth_manager,
|
||||
installation_id,
|
||||
rpc_transport,
|
||||
remote_control_handle,
|
||||
plugin_startup_tasks,
|
||||
} = args;
|
||||
auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge {
|
||||
@@ -446,7 +443,6 @@ impl MessageProcessor {
|
||||
auth_manager,
|
||||
thread_manager.clone(),
|
||||
analytics_events_client,
|
||||
remote_control_handle,
|
||||
);
|
||||
let external_agent_config_processor = ExternalAgentConfigRequestProcessor::new(
|
||||
outgoing.clone(),
|
||||
|
||||
@@ -264,7 +264,6 @@ async fn build_test_processor(
|
||||
auth_manager,
|
||||
installation_id: "11111111-1111-4111-8111-111111111111".to_string(),
|
||||
rpc_transport: AppServerRpcTransport::Stdio,
|
||||
remote_control_handle: None,
|
||||
plugin_startup_tasks: crate::PluginStartupTasks::Start,
|
||||
}));
|
||||
(processor, outgoing_rx)
|
||||
|
||||
@@ -6,7 +6,6 @@ use crate::error_code::internal_error;
|
||||
use crate::error_code::invalid_request;
|
||||
use crate::outgoing_message::ConnectionRequestId;
|
||||
use crate::outgoing_message::OutgoingMessageSender;
|
||||
use crate::transport::RemoteControlHandle;
|
||||
use codex_analytics::AnalyticsEventsClient;
|
||||
use codex_app_server_protocol::AppListUpdatedNotification;
|
||||
use codex_app_server_protocol::ClientResponsePayload;
|
||||
@@ -39,7 +38,6 @@ use codex_config::MatcherGroup as CoreMatcherGroup;
|
||||
use codex_config::ResidencyRequirement as CoreResidencyRequirement;
|
||||
use codex_config::SandboxModeRequirement as CoreSandboxModeRequirement;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_features::Feature;
|
||||
use codex_features::canonical_feature_for_key;
|
||||
use codex_features::feature_for_key;
|
||||
use codex_login::AuthManager;
|
||||
@@ -67,7 +65,6 @@ pub(crate) struct ConfigRequestProcessor {
|
||||
auth_manager: Arc<AuthManager>,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
remote_control_handle: Option<RemoteControlHandle>,
|
||||
}
|
||||
|
||||
impl ConfigRequestProcessor {
|
||||
@@ -77,7 +74,6 @@ impl ConfigRequestProcessor {
|
||||
auth_manager: Arc<AuthManager>,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
analytics_events_client: AnalyticsEventsClient,
|
||||
remote_control_handle: Option<RemoteControlHandle>,
|
||||
) -> Self {
|
||||
Self {
|
||||
outgoing,
|
||||
@@ -85,7 +81,6 @@ impl ConfigRequestProcessor {
|
||||
auth_manager,
|
||||
thread_manager,
|
||||
analytics_events_client,
|
||||
remote_control_handle,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -187,21 +182,6 @@ impl ConfigRequestProcessor {
|
||||
pub(crate) async fn handle_config_mutation(&self) {
|
||||
self.thread_manager.plugins_manager().clear_cache();
|
||||
self.thread_manager.skills_manager().clear_cache();
|
||||
let Some(remote_control_handle) = &self.remote_control_handle else {
|
||||
return;
|
||||
};
|
||||
|
||||
match self.load_latest_config(/*fallback_cwd*/ None).await {
|
||||
Ok(config) => {
|
||||
remote_control_handle.set_enabled(config.features.enabled(Feature::RemoteControl));
|
||||
}
|
||||
Err(error) => {
|
||||
tracing::warn!(
|
||||
"failed to load config for remote control enablement refresh after config mutation: {}",
|
||||
error.message
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn handle_config_mutation_result<T>(
|
||||
|
||||
@@ -18,7 +18,6 @@ pub(crate) use codex_app_server_transport::ConnectionId;
|
||||
pub(crate) use codex_app_server_transport::ConnectionOrigin;
|
||||
pub(crate) use codex_app_server_transport::OutgoingMessage;
|
||||
pub(crate) use codex_app_server_transport::QueuedOutgoingMessage;
|
||||
pub(crate) use codex_app_server_transport::RemoteControlHandle;
|
||||
pub(crate) use codex_app_server_transport::RemoteControlStartConfig;
|
||||
pub(crate) use codex_app_server_transport::TransportEvent;
|
||||
pub use codex_app_server_transport::app_server_control_socket_path;
|
||||
|
||||
@@ -428,6 +428,10 @@ struct AppServerCommand {
|
||||
)]
|
||||
listen: codex_app_server::AppServerTransport,
|
||||
|
||||
/// Enable remote control for this app-server process.
|
||||
#[arg(long = "remote-control", hide = true)]
|
||||
remote_control: bool,
|
||||
|
||||
/// Controls whether analytics are enabled by default.
|
||||
///
|
||||
/// Analytics are disabled by default for app-server. Users have to explicitly opt in
|
||||
@@ -503,10 +507,10 @@ enum AppServerDaemonSubcommand {
|
||||
/// Restart the local app server daemon.
|
||||
Restart,
|
||||
|
||||
/// Enable remote_control for future starts and a currently running managed daemon.
|
||||
/// Enable remote control for future starts and a currently running managed daemon.
|
||||
EnableRemoteControl,
|
||||
|
||||
/// Disable remote_control for future starts and a currently running managed daemon.
|
||||
/// Disable remote control for future starts and a currently running managed daemon.
|
||||
DisableRemoteControl,
|
||||
|
||||
/// Stop the local app server daemon.
|
||||
@@ -529,7 +533,7 @@ struct AppServerProxyCommand {
|
||||
|
||||
#[derive(Debug, Args)]
|
||||
struct AppServerBootstrapCommand {
|
||||
/// Launch the managed app-server with remote_control enabled.
|
||||
/// Launch the managed app-server with remote control enabled.
|
||||
#[arg(long = "remote-control")]
|
||||
remote_control: bool,
|
||||
}
|
||||
@@ -888,6 +892,7 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> {
|
||||
let AppServerCommand {
|
||||
subcommand,
|
||||
listen,
|
||||
remote_control,
|
||||
analytics_default_enabled,
|
||||
} = app_server_cli;
|
||||
reject_remote_mode_for_app_server_subcommand(
|
||||
@@ -898,13 +903,18 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> {
|
||||
match subcommand {
|
||||
None => {
|
||||
let transport = listen;
|
||||
codex_app_server::run_main_with_transport(
|
||||
let runtime_options = codex_app_server::AppServerRuntimeOptions {
|
||||
remote_control_enabled: remote_control,
|
||||
..Default::default()
|
||||
};
|
||||
codex_app_server::run_main_with_transport_options(
|
||||
arg0_paths.clone(),
|
||||
root_config_overrides,
|
||||
codex_config::LoaderOverrides::default(),
|
||||
analytics_default_enabled,
|
||||
transport,
|
||||
codex_protocol::protocol::SessionSource::VSCode,
|
||||
runtime_options,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
@@ -2379,6 +2389,7 @@ mod tests {
|
||||
fn app_server_analytics_default_disabled_without_flag() {
|
||||
let app_server = app_server_from_args(["codex", "app-server"].as_ref());
|
||||
assert!(!app_server.analytics_default_enabled);
|
||||
assert!(!app_server.remote_control);
|
||||
assert_eq!(
|
||||
app_server.listen,
|
||||
codex_app_server::AppServerTransport::Stdio
|
||||
|
||||
@@ -1118,7 +1118,7 @@ pub const FEATURES: &[FeatureSpec] = &[
|
||||
FeatureSpec {
|
||||
id: Feature::RemoteControl,
|
||||
key: "remote_control",
|
||||
stage: Stage::UnderDevelopment,
|
||||
stage: Stage::Removed,
|
||||
default_enabled: false,
|
||||
},
|
||||
FeatureSpec {
|
||||
|
||||
@@ -287,8 +287,8 @@ fn auth_elicitation_is_under_development() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remote_control_is_under_development() {
|
||||
assert_eq!(Feature::RemoteControl.stage(), Stage::UnderDevelopment);
|
||||
fn remote_control_is_removed_and_disabled_by_default() {
|
||||
assert_eq!(Feature::RemoteControl.stage(), Stage::Removed);
|
||||
assert_eq!(Feature::RemoteControl.default_enabled(), false);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user