mirror of
https://github.com/openai/codex.git
synced 2026-09-28 08:43:01 +08:00
Offer explicit recovery for incompatible background servers (#47318)
## Why Automatic daemon launches could silently fall back to embedded mode when shared feature settings differed from the session's requirements. Changing those settings affects other clients, and restarting the server may interrupt active or queued work. ## What changed - Offer interactive choices to run without the daemon, restart with the required settings, or cancel. Default to cancel and require explicit confirmation for restart. - Allow restart only for managed daemons with a feature mismatch, then recheck compatibility once. Noninteractive required-daemon failures return an error with `--no-daemon` guidance; optional attachment retains its fallback behavior. - Preserve saved feature overrides on startup and require confirmation before applying requested shared-feature disables, including on fresh starts. - Merge confirmed settings under the lifecycle lock, persist changes after the old process stops, and avoid restarting when the saved overrides already match. ## Testing Add recovery-menu snapshots and confirmation tests, compatibility tests for required and optional attachment, daemon ownership and settings-preservation tests, and CLI terminal tests for cancellation, embedded fallback, confirmed restart, and disabling shared features. GitOrigin-RevId: d50a6cf7e476b9647237431e8fc0fc6041ed17db
This commit is contained in:
@@ -5,6 +5,11 @@
|
||||
slow-timeout = { period = "30s", terminate-after = 2 }
|
||||
retries = 1
|
||||
|
||||
[[profile.default.overrides]]
|
||||
# These cases cold-start a daemon before confirming a second launch with new settings.
|
||||
filter = 'package(codex-cli) & binary(daemon_startup) & test(requires_confirmation_to_disable_)'
|
||||
slow-timeout = { period = "1m", terminate-after = 2 }
|
||||
|
||||
[[profile.default.overrides]]
|
||||
# This case validates and copies both the initial and replacement debug package.
|
||||
filter = 'package(codex-cli) & binary(app_server_daemon) & test(=packaged_daemon_start_and_explicit_replacement)'
|
||||
|
||||
@@ -1,13 +1,15 @@
|
||||
//! Invocation feature overrides apply only when starting a missing daemon.
|
||||
//! The lifecycle lock protects their persistence against concurrent starts and updates.
|
||||
//! Invocation overrides apply on fresh starts or explicitly confirmed managed restarts.
|
||||
//! The lifecycle lock serializes persistence against other clients and updates.
|
||||
|
||||
use crate::Daemon;
|
||||
use crate::LifecycleOutput;
|
||||
use crate::ensure_supported_platform;
|
||||
use anyhow::Result;
|
||||
use anyhow::ensure;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
/// Start a missing daemon with these features, or leave a running daemon unchanged.
|
||||
/// Start a missing daemon with these features, preserving other saved overrides,
|
||||
/// or leave a running daemon unchanged.
|
||||
/// Callers must check the running server's configuration before using it.
|
||||
pub async fn start_with_features(features: &BTreeMap<String, bool>) -> Result<LifecycleOutput> {
|
||||
ensure_supported_platform()?;
|
||||
@@ -16,5 +18,44 @@ pub async fn start_with_features(features: &BTreeMap<String, bool>) -> Result<Li
|
||||
let daemon = Daemon::from_environment()?;
|
||||
let _operation_lock = daemon.acquire_operation_lock().await?;
|
||||
let selected = daemon.current_installation()?;
|
||||
Box::pin(selected.start(features)).await
|
||||
let mut overrides = selected.load_settings().await?.feature_overrides;
|
||||
overrides.extend(features.clone());
|
||||
Box::pin(selected.start(&overrides)).await
|
||||
}
|
||||
|
||||
/// Apply confirmed feature settings to an existing managed daemon and restart it.
|
||||
/// Callers must obtain consent for changes to shared services and interrupted work,
|
||||
/// then verify effective server compatibility after this operation completes.
|
||||
pub async fn restart_with_features(features: &BTreeMap<String, bool>) -> Result<LifecycleOutput> {
|
||||
ensure_supported_platform()?;
|
||||
#[cfg(windows)]
|
||||
crate::backend::windows::ensure_not_elevated()?;
|
||||
let daemon = Daemon::from_environment()?;
|
||||
let _operation_lock = daemon.acquire_operation_lock().await?;
|
||||
let selected = daemon.current_installation()?;
|
||||
Box::pin(selected.restart_with_features_locked(features)).await
|
||||
}
|
||||
|
||||
impl Daemon {
|
||||
pub(super) async fn restart(&self) -> Result<LifecycleOutput> {
|
||||
self.restart_with_settings(self.load_settings().await?)
|
||||
.await
|
||||
}
|
||||
|
||||
pub(super) async fn restart_with_features_locked(
|
||||
&self,
|
||||
features: &BTreeMap<String, bool>,
|
||||
) -> Result<LifecycleOutput> {
|
||||
let mut settings = self.load_settings().await?;
|
||||
ensure!(
|
||||
self.running_backend_instance(&settings).await?.is_some(),
|
||||
"no running managed daemon; rerun the command to check the current server"
|
||||
);
|
||||
let previous = settings.feature_overrides.clone();
|
||||
settings.feature_overrides.extend(features.clone());
|
||||
if settings.feature_overrides == previous {
|
||||
return self.start(&previous).await;
|
||||
}
|
||||
self.restart_with_settings(settings).await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ use backend::windows::try_lock_file;
|
||||
mod client;
|
||||
mod install_lock;
|
||||
mod launch;
|
||||
pub use launch::restart_with_features;
|
||||
pub use launch::start_with_features;
|
||||
mod managed_install;
|
||||
mod prepare_install;
|
||||
@@ -437,8 +438,7 @@ impl Daemon {
|
||||
.await)
|
||||
}
|
||||
|
||||
async fn restart(&self) -> Result<LifecycleOutput> {
|
||||
let settings = self.load_settings().await?;
|
||||
async fn restart_with_settings(&self, settings: DaemonSettings) -> Result<LifecycleOutput> {
|
||||
if client::probe(&self.socket_path).await.is_ok()
|
||||
&& self.running_backend(&settings).await?.is_none()
|
||||
{
|
||||
@@ -465,6 +465,11 @@ impl Daemon {
|
||||
.await?;
|
||||
}
|
||||
|
||||
// Persist changed launch settings only after the old process has stopped.
|
||||
// A failed or interrupted drain must not make an unapplied change look current.
|
||||
if self.load_settings().await? != settings {
|
||||
settings.save(&self.settings_file).await?;
|
||||
}
|
||||
let pid = managed.start_managed_backend(&settings).await?;
|
||||
let info = self.wait_until_ready().await?;
|
||||
if let Err(err) = managed.ensure_managed_updater(&settings).await {
|
||||
|
||||
@@ -617,6 +617,74 @@ async fn daemon_start_and_restart_preserve_launch_features() {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn confirmed_feature_restart_preserves_ownership_and_skips_matching_settings() {
|
||||
use crate::LifecycleStatus;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
for managed in [true, false] {
|
||||
let home = TempDir::new().unwrap();
|
||||
let (daemon, _) = manual_update_daemon(&home);
|
||||
std::fs::write(&daemon.settings_file,
|
||||
r#"{"featureOverrides":{"auth_elicitation":true,"api_key_model_discovery":true},"updater":{"autoUpdateEnabled":false},"shutdownGraceSeconds":0}"#
|
||||
).unwrap();
|
||||
let original = daemon.load_settings().await.unwrap();
|
||||
if managed {
|
||||
daemon.start_managed_backend(&original).await.unwrap();
|
||||
}
|
||||
let server = test_control_server(&daemon, home.path()).await;
|
||||
let _lock = daemon.acquire_operation_lock().await.unwrap();
|
||||
let requested = BTreeMap::from([
|
||||
("api_key_model_discovery".to_string(), false),
|
||||
("mcp_oauth_refresh_coordination".to_string(), true),
|
||||
]);
|
||||
if managed {
|
||||
// Hide the selection without removing the script the spawned shell still needs.
|
||||
let selected_package = daemon.managed_codex_bin.parent().unwrap();
|
||||
let saved_package = selected_package.with_extension("saved");
|
||||
std::fs::rename(selected_package, &saved_package).unwrap();
|
||||
let error = daemon
|
||||
.restart_with_features_locked(&requested)
|
||||
.await
|
||||
.unwrap_err();
|
||||
std::fs::rename(saved_package, selected_package).unwrap();
|
||||
assert!(
|
||||
error.to_string().contains("daemon executable not found"),
|
||||
"{error:#}"
|
||||
);
|
||||
assert_eq!(daemon.load_settings().await.unwrap(), original);
|
||||
}
|
||||
let result = daemon.restart_with_features_locked(&requested).await;
|
||||
if managed {
|
||||
assert_eq!(result.unwrap().status, LifecycleStatus::Restarted);
|
||||
let pid = std::fs::read(&daemon.pid_file).unwrap();
|
||||
let mut expected = original;
|
||||
expected.feature_overrides.extend(requested.clone());
|
||||
assert_eq!(daemon.load_settings().await.unwrap(), expected);
|
||||
assert_eq!(
|
||||
daemon
|
||||
.restart_with_features_locked(&requested)
|
||||
.await
|
||||
.unwrap()
|
||||
.status,
|
||||
LifecycleStatus::AlreadyRunning
|
||||
);
|
||||
assert_eq!(std::fs::read(&daemon.pid_file).unwrap(), pid);
|
||||
daemon.stop().await.unwrap();
|
||||
} else {
|
||||
assert!(
|
||||
result
|
||||
.unwrap_err()
|
||||
.to_string()
|
||||
.contains("no running managed daemon")
|
||||
);
|
||||
assert_eq!(daemon.load_settings().await.unwrap(), original);
|
||||
}
|
||||
server.abort();
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn manual_update_restarts_local_daemon_with_automatic_updates_disabled() {
|
||||
|
||||
@@ -14,6 +14,36 @@ async fn auto_daemon_start_attaches_to_shared_server() -> Result<()> {
|
||||
daemon_startup("start").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn incompatible_daemon_can_cancel() -> Result<()> {
|
||||
daemon_startup("mismatch-cancel").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn incompatible_daemon_can_run_embedded() -> Result<()> {
|
||||
daemon_startup("mismatch-embedded").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn incompatible_daemon_can_restart_with_confirmed_settings() -> Result<()> {
|
||||
daemon_startup("mismatch-restart").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn fresh_daemon_requires_confirmation_to_disable_shared_features() -> Result<()> {
|
||||
daemon_startup("mismatch-disable").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg(unix)]
|
||||
async fn stopped_daemon_requires_confirmation_to_disable_persisted_features() -> Result<()> {
|
||||
daemon_startup("mismatch-disable-persisted").await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn daemon_exclusion_survives_resume_picker() -> Result<()> {
|
||||
daemon_startup("resume").await
|
||||
@@ -55,6 +85,10 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
serde_json::to_string(&workspace_path)?,
|
||||
),
|
||||
)?;
|
||||
let mismatch = command.starts_with("mismatch-");
|
||||
let persisted = command == "mismatch-disable-persisted";
|
||||
let disabling = matches!(command, "mismatch-disable" | "mismatch-disable-persisted");
|
||||
let restart = command == "mismatch-restart" || disabling;
|
||||
let bedrock_onboarding = matches!(command, "bedrock" | "bedrock-running");
|
||||
if !bedrock_onboarding {
|
||||
fs::write(
|
||||
@@ -83,7 +117,7 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
env.insert("TERM".into(), "xterm-256color".into());
|
||||
let mut args = vec!["--no-alt-screen".to_string()];
|
||||
let mut steps: VecDeque<(&str, &[u8])> = VecDeque::new();
|
||||
if matches!(command, "start" | "bedrock-running") {
|
||||
if matches!(command, "start" | "bedrock-running") || mismatch {
|
||||
// A selected package with a stopped daemon avoids installing a release.
|
||||
let managed = home
|
||||
.path()
|
||||
@@ -93,12 +127,16 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
fs::create_dir(home.path().join("app-server-daemon"))?;
|
||||
fs::write(
|
||||
home.path().join("app-server-daemon/settings.json"),
|
||||
r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false}}"#,
|
||||
if persisted {
|
||||
r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false},"featureOverrides":{"api_key_model_discovery":true}}"#
|
||||
} else {
|
||||
r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false}}"#
|
||||
},
|
||||
)?;
|
||||
}
|
||||
let pid_file = home.path().join("app-server-daemon/daemon.pid");
|
||||
let result = async {
|
||||
let existing_daemon = if command == "bedrock-running" {
|
||||
let mut existing_daemon = if command == "bedrock-running" || (mismatch && !disabling) {
|
||||
let started = Command::new(&codex)
|
||||
.env_clear()
|
||||
.envs(&env)
|
||||
@@ -118,6 +156,27 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
// The draft header is visible before the session's command composer is ready.
|
||||
steps.push_back(("GPT-5.6-Terra", b"/status\r"));
|
||||
"Server:Localbackgroundserver"
|
||||
} else if mismatch {
|
||||
args.extend(if persisted {
|
||||
["--disable".into(), "api_key_model_discovery".into()]
|
||||
} else if disabling {
|
||||
["--disable".into(), "auth_elicitation".into()]
|
||||
} else {
|
||||
["--enable".into(), "api_key_model_discovery".into()]
|
||||
});
|
||||
let input: &[u8] = match command {
|
||||
"mismatch-cancel" => b"\x03",
|
||||
"mismatch-embedded" => b"1",
|
||||
"mismatch-restart" | "mismatch-disable" | "mismatch-disable-persisted" => b"2\r",
|
||||
_ => unreachable!(),
|
||||
};
|
||||
steps.push_back(("Backgroundserverhasincompatiblefeaturesettings", input));
|
||||
if command == "mismatch-cancel" {
|
||||
"--no-daemon"
|
||||
} else {
|
||||
steps.push_back(("GPT-5.6-Terra", b"/status\r"));
|
||||
if restart { "Server:Localbackgroundserver" } else { "Model:" }
|
||||
}
|
||||
} else if bedrock_onboarding {
|
||||
"UseAmazonBedrock"
|
||||
} else {
|
||||
@@ -139,6 +198,7 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
&[],
|
||||
)
|
||||
.await?;
|
||||
let exit = spawned.exit_rx;
|
||||
let session = spawned.session;
|
||||
let writer = session.writer_sender();
|
||||
let mut stdout = spawned.stdout_rx;
|
||||
@@ -153,7 +213,9 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
("\x1b]10;?", b"\x1b]10;rgb:ffff/ffff/ffff\x1b\\"),
|
||||
("\x1b]11;?", b"\x1b]11;rgb:0000/0000/0000\x1b\\"),
|
||||
];
|
||||
let result = tokio::time::timeout(Duration::from_secs(/*secs*/ 45), async {
|
||||
// A fresh disable request includes both cold daemon startup and its confirmed restart.
|
||||
let timeout = Duration::from_secs(if disabling { 90 } else { 45 });
|
||||
let result = tokio::time::timeout(timeout, async {
|
||||
while let Some(bytes) = stdout.recv().await {
|
||||
output.push_str(&String::from_utf8_lossy(&bytes));
|
||||
screen.process(&bytes);
|
||||
@@ -172,11 +234,29 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
if let Some((ready, input)) = steps.front()
|
||||
&& text.contains(ready)
|
||||
{
|
||||
if disabling && *ready == "Backgroundserverhasincompatiblefeaturesettings" {
|
||||
existing_daemon = Some(fs::read(&pid_file)?);
|
||||
let settings: serde_json::Value = serde_json::from_slice(&fs::read(home.path().join("app-server-daemon/settings.json"))?)?;
|
||||
if persisted {
|
||||
ensure!(settings["featureOverrides"]["api_key_model_discovery"] == true);
|
||||
} else {
|
||||
ensure!(settings["featureOverrides"]["auth_elicitation"].is_null());
|
||||
}
|
||||
}
|
||||
writer.send(input.to_vec()).await?;
|
||||
steps.pop_front();
|
||||
output.clear();
|
||||
} else if steps.is_empty() && text.contains(expected) {
|
||||
if command == "start" {
|
||||
if mismatch {
|
||||
let previous_pid = existing_daemon.as_ref().context("missing original daemon PID")?;
|
||||
ensure!((fs::read(&pid_file)? != *previous_pid) == restart);
|
||||
if command == "mismatch-cancel" {
|
||||
ensure!(text.contains("Cannotusethesharedbackgroundserver:Thissessionrequiresapi_key_model_discoverytobeenabled."));
|
||||
} else {
|
||||
ensure!(text.contains("Server:Localbackgroundserver") == restart);
|
||||
}
|
||||
} else if command == "start" {
|
||||
ensure!(text.contains("Server:Localbackgroundserver"));
|
||||
ensure!(home.path().join("app-server-daemon/daemon.pid").exists());
|
||||
} else if let Some(existing_daemon) = &existing_daemon {
|
||||
ensure!(fs::read(&pid_file)? == *existing_daemon);
|
||||
@@ -192,9 +272,13 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
)
|
||||
})
|
||||
.await;
|
||||
if command == "mismatch-cancel" && matches!(result, Ok(Ok(()))) {
|
||||
let status = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), exit).await??;
|
||||
ensure!(status != 0, "incompatible daemon launch must fail");
|
||||
}
|
||||
Ok::<_, anyhow::Error>((
|
||||
session,
|
||||
result.with_context(|| format!("{command} timed out: {}", screen.screen().contents())),
|
||||
result.with_context(|| format!("{command} timed out waiting for {}: {}", steps.front().map_or(expected, |(ready, _)| *ready), screen.screen().contents())),
|
||||
))
|
||||
}
|
||||
.await;
|
||||
|
||||
@@ -0,0 +1,181 @@
|
||||
//! Explicit recovery from required-daemon incompatibility. Restart is managed-only,
|
||||
//! requires a fresh confirmation, and is followed by one compatibility check, never a loop.
|
||||
|
||||
use crate::AppServerTarget;
|
||||
use crate::app_event::AppEvent;
|
||||
use crate::app_event_sender::AppEventSender;
|
||||
use crate::bottom_pane::BottomPaneView;
|
||||
use crate::bottom_pane::ListSelectionView;
|
||||
use crate::bottom_pane::SelectionItem;
|
||||
use crate::bottom_pane::SelectionViewParams;
|
||||
use crate::daemon_startup;
|
||||
use crate::daemon_startup::CompatibilityError;
|
||||
use crate::keymap::RuntimeKeymap;
|
||||
use crate::legacy_core::config::Config;
|
||||
use crate::render::renderable::ColumnRenderable;
|
||||
use crate::render::renderable::Renderable;
|
||||
use crate::startup_draft::StartupDraft;
|
||||
use crossterm::event::KeyCode;
|
||||
use crossterm::event::KeyEventKind;
|
||||
use crossterm::event::KeyModifiers;
|
||||
use ratatui::style::Stylize;
|
||||
use ratatui::text::Line;
|
||||
use ratatui::widgets::Paragraph;
|
||||
use ratatui::widgets::Wrap;
|
||||
use std::io;
|
||||
use std::io::IsTerminal;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
use tokio_stream::StreamExt;
|
||||
|
||||
pub(super) async fn check(
|
||||
startup: &mut StartupDraft,
|
||||
target: &AppServerTarget,
|
||||
config: &Config,
|
||||
managed_daemon: bool,
|
||||
) -> io::Result<Option<String>> {
|
||||
let issue = match startup
|
||||
.run_until(daemon_startup::compatibility_warning(target, config))
|
||||
.await?
|
||||
{
|
||||
Ok(warning) => return Ok(warning),
|
||||
Err(issue) => issue,
|
||||
};
|
||||
if !(io::stdin().is_terminal() && io::stdout().is_terminal()) {
|
||||
return Err(io::Error::other(issue));
|
||||
}
|
||||
startup.flush_pending_events().await?;
|
||||
let keymap = RuntimeKeymap::from_config(&config.tui_keymap).map_err(io::Error::other)?;
|
||||
let mut view = recovery_view(&issue, managed_daemon, &keymap);
|
||||
let mut chord_matcher = crate::keymap::KeyChordMatcher::default();
|
||||
let tui = startup.tui_mut();
|
||||
tui.discard_pending_input_before_interactive_screen()?;
|
||||
let selection = {
|
||||
let events = tui.event_stream();
|
||||
tokio::pin!(events);
|
||||
loop {
|
||||
let height = view.desired_height(tui.terminal.size()?.width);
|
||||
tui.draw(height, |frame| {
|
||||
view.render(frame.area(), frame.buffer_mut())
|
||||
})?;
|
||||
let Some(event) = events.next().await else {
|
||||
break None;
|
||||
};
|
||||
tui.screen_size_for_event(&event)?;
|
||||
if let crate::tui::TuiEvent::Key(key) = event
|
||||
&& matches!(key.kind, KeyEventKind::Press | KeyEventKind::Repeat)
|
||||
{
|
||||
if key.modifiers.contains(KeyModifiers::CONTROL)
|
||||
&& matches!(key.code, KeyCode::Char('c' | 'd'))
|
||||
{
|
||||
break None;
|
||||
}
|
||||
let key = match chord_matcher.advance(
|
||||
key,
|
||||
&keymap.chords,
|
||||
crate::keymap::KeymapContextSet::new(crate::keymap::KeymapContext::List),
|
||||
) {
|
||||
crate::keymap::KeyChordMatch::PassThrough => key,
|
||||
crate::keymap::KeyChordMatch::Completed(key) => key,
|
||||
crate::keymap::KeyChordMatch::Pending(_)
|
||||
| crate::keymap::KeyChordMatch::Cancelled
|
||||
| crate::keymap::KeyChordMatch::Ignored => continue,
|
||||
};
|
||||
view.handle_key_event(key);
|
||||
}
|
||||
if view.is_complete() {
|
||||
break view.take_last_selected_index();
|
||||
}
|
||||
}
|
||||
};
|
||||
tui.terminal.clear()?;
|
||||
match (selection, issue.restart_features.as_ref()) {
|
||||
(Some(0), _) => Ok(Some(format!(
|
||||
"Running without the shared background server: {}.",
|
||||
issue.reason
|
||||
))),
|
||||
(Some(1), Some(features)) if managed_daemon => {
|
||||
tui.with_restored(|| async {
|
||||
crossterm::terminal::disable_raw_mode()?;
|
||||
codex_app_server_daemon::restart_with_features(features)
|
||||
.await
|
||||
.map_err(|err| {
|
||||
io::Error::other(format!("{err:#}\n{}", daemon_startup::FAILURE_HINT))
|
||||
})
|
||||
})
|
||||
.await?;
|
||||
startup
|
||||
.run_until(daemon_startup::compatibility_warning(target, config))
|
||||
.await?
|
||||
.map_err(io::Error::other)
|
||||
}
|
||||
(Some(_) | None, _) => Err(io::Error::other(issue)),
|
||||
}
|
||||
}
|
||||
|
||||
fn recovery_view(
|
||||
issue: &CompatibilityError,
|
||||
managed_daemon: bool,
|
||||
keymap: &RuntimeKeymap,
|
||||
) -> ListSelectionView {
|
||||
let mut header = ColumnRenderable::new();
|
||||
header.push(Line::from(
|
||||
if issue.restart_features.is_some() {
|
||||
"Background server has incompatible feature settings"
|
||||
} else {
|
||||
"Cannot use the background server"
|
||||
}
|
||||
.bold(),
|
||||
));
|
||||
header.push(Paragraph::new(issue.reason.clone()).wrap(Wrap { trim: false }));
|
||||
if let Some(features) = &issue.restart_features {
|
||||
header.push(Line::from(
|
||||
"Restart will use these shared feature settings:",
|
||||
));
|
||||
for (name, enabled) in features {
|
||||
header.push(Line::from(format!(" {name} = {enabled}").dim()));
|
||||
}
|
||||
header.push(Paragraph::new(
|
||||
"These settings persist and can disable functionality for other clients. Restart may interrupt active or queued work."
|
||||
).wrap(Wrap { trim: false }));
|
||||
}
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
ListSelectionView::new(
|
||||
SelectionViewParams {
|
||||
header: Box::new(header),
|
||||
initial_selected_idx: Some(2),
|
||||
items: vec![
|
||||
SelectionItem {
|
||||
name: "Run without daemon this time".to_string(),
|
||||
dismiss_on_select: true,
|
||||
..Default::default()
|
||||
},
|
||||
SelectionItem {
|
||||
name: "Restart with these settings".to_string(),
|
||||
dismiss_on_select: true,
|
||||
require_explicit_confirmation: true,
|
||||
is_disabled: !managed_daemon || issue.restart_features.is_none(),
|
||||
disabled_reason: if !managed_daemon {
|
||||
Some("This server is not managed by Codex.".to_string())
|
||||
} else if issue.restart_features.is_none() {
|
||||
Some("Restart cannot resolve this compatibility check.".to_string())
|
||||
} else {
|
||||
None
|
||||
},
|
||||
..Default::default()
|
||||
},
|
||||
SelectionItem {
|
||||
name: "Cancel".to_string(),
|
||||
dismiss_on_select: true,
|
||||
..Default::default()
|
||||
},
|
||||
],
|
||||
..SelectionViewParams::picker()
|
||||
},
|
||||
AppEventSender::new(tx),
|
||||
keymap.list.clone(),
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "daemon_recovery_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,67 @@
|
||||
use super::*;
|
||||
use crossterm::event::KeyCode;
|
||||
use crossterm::event::KeyEvent;
|
||||
use crossterm::event::KeyModifiers;
|
||||
use pretty_assertions::assert_eq;
|
||||
use ratatui::buffer::Buffer;
|
||||
use ratatui::layout::Rect;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
#[test]
|
||||
fn daemon_recovery_requires_explicit_restart_and_defaults_to_cancel() {
|
||||
let issue = CompatibilityError {
|
||||
reason: "This session requires api_key_model_discovery to be enabled".to_string(),
|
||||
restart_features: Some(BTreeMap::from([
|
||||
("api_key_model_discovery".to_string(), true),
|
||||
("mcp_oauth_refresh_coordination".to_string(), false),
|
||||
])),
|
||||
};
|
||||
let non_restartable = CompatibilityError {
|
||||
reason: "code-mode host fallback policy requires embedded mode".to_string(),
|
||||
restart_features: None,
|
||||
};
|
||||
for (issue, managed, snapshot) in [
|
||||
(&issue, true, "daemon_recovery_menu"),
|
||||
(&issue, false, "unmanaged_daemon_recovery_menu"),
|
||||
(
|
||||
&non_restartable,
|
||||
true,
|
||||
"non_restartable_daemon_recovery_menu",
|
||||
),
|
||||
] {
|
||||
let mut view = recovery_view(issue, managed, &RuntimeKeymap::defaults());
|
||||
let area = Rect::new(
|
||||
/*x*/ 0,
|
||||
/*y*/ 0,
|
||||
/*width*/ 90,
|
||||
view.desired_height(/*width*/ 90),
|
||||
);
|
||||
let mut buffer = Buffer::empty(area);
|
||||
view.render(area, &mut buffer);
|
||||
let text = buffer
|
||||
.content
|
||||
.chunks(90)
|
||||
.map(|row| {
|
||||
row.iter()
|
||||
.map(ratatui::buffer::Cell::symbol)
|
||||
.collect::<String>()
|
||||
.trim_end()
|
||||
.to_string()
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
insta::assert_snapshot!(snapshot, text);
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
assert_eq!(view.take_last_selected_index(), Some(2));
|
||||
|
||||
let mut view = recovery_view(issue, managed, &RuntimeKeymap::defaults());
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('2'), KeyModifiers::NONE));
|
||||
if managed && issue.restart_features.is_some() {
|
||||
assert!(!view.is_complete());
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
assert_eq!(view.take_last_selected_index(), Some(1));
|
||||
} else {
|
||||
assert_eq!(view.take_last_selected_index(), Some(2));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,6 +1,6 @@
|
||||
//! Local daemon launch policy. Explicit embedded launches never discover or start a daemon;
|
||||
//! incompatible feature settings use embedded mode. Compatible automatic launches
|
||||
//! require a successful shared-server connection.
|
||||
//! optional attachment may fall back to embedded mode, while automatic launches
|
||||
//! require a compatible shared server and a successful connection.
|
||||
|
||||
use super::*;
|
||||
use std::collections::BTreeMap;
|
||||
@@ -14,6 +14,13 @@ const SERVER_FEATURES: [Feature; 4] = [
|
||||
|
||||
pub(super) const FAILURE_HINT: &str = "To work without the background server, rerun the same command with --no-daemon (including resume or fork and its arguments).";
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
#[error("Cannot use the shared background server: {reason}.\n{FAILURE_HINT}")]
|
||||
pub(super) struct CompatibilityError {
|
||||
pub reason: String,
|
||||
pub restart_features: Option<BTreeMap<String, bool>>,
|
||||
}
|
||||
|
||||
pub(super) fn exclusion(
|
||||
cli: &Cli,
|
||||
cli_kv_overrides: &[(String, toml::Value)],
|
||||
@@ -54,7 +61,10 @@ pub(super) fn config_exclusion(
|
||||
.all(|(key, value)| match key.as_str() {
|
||||
"suppress_unstable_features_warning" | "tui.fullscreen_transcript" => value.is_bool(),
|
||||
"tui" => value.as_table().is_some_and(|tui| {
|
||||
tui.len() == 1 && tui.get("fullscreen_transcript").is_some_and(toml::Value::is_bool)
|
||||
tui.len() == 1
|
||||
&& tui
|
||||
.get("fullscreen_transcript")
|
||||
.is_some_and(toml::Value::is_bool)
|
||||
}),
|
||||
"features" => value.as_table().is_some_and(|features| {
|
||||
!features.is_empty()
|
||||
@@ -64,9 +74,6 @@ pub(super) fn config_exclusion(
|
||||
}),
|
||||
_ => key.strip_prefix("features.").is_some_and(allowed_feature) && value.is_bool(),
|
||||
})
|
||||
// Older clients cannot check compatibility before attaching. Do not disable
|
||||
// shared services they may rely on through a new daemon's CLI overrides.
|
||||
|| server_features(cli_kv_overrides).values().any(|enabled| !enabled)
|
||||
{
|
||||
Some("command-line configuration overrides (-c, --enable, --disable, or --search)")
|
||||
} else if !loader_overrides_are_default(loader_overrides) {
|
||||
@@ -111,17 +118,22 @@ pub(super) fn server_features(overrides: &[(String, toml::Value)]) -> BTreeMap<S
|
||||
pub(super) async fn compatibility_warning(
|
||||
target: &AppServerTarget,
|
||||
config: &Config,
|
||||
) -> Option<String> {
|
||||
if !matches!(target, AppServerTarget::LocalDaemon { .. }) {
|
||||
return None;
|
||||
}
|
||||
// The feature-list RPC cannot report this process-scoped structured setting.
|
||||
if !config.features.enabled(Feature::CodeModeHost)
|
||||
&& config.code_mode.disable_in_process_fallback
|
||||
{
|
||||
return Some("Running without the shared background server: code-mode host fallback policy requires embedded mode.".to_string());
|
||||
}
|
||||
) -> Result<Option<String>, CompatibilityError> {
|
||||
let AppServerTarget::LocalDaemon {
|
||||
allow_embedded_fallback,
|
||||
..
|
||||
} = target
|
||||
else {
|
||||
return Ok(None);
|
||||
};
|
||||
let mut restart_features = None;
|
||||
let check = async {
|
||||
// The feature-list RPC cannot report this process-scoped structured setting.
|
||||
if !config.features.enabled(Feature::CodeModeHost)
|
||||
&& config.code_mode.disable_in_process_fallback
|
||||
{
|
||||
return Err("code-mode host fallback policy requires embedded mode".to_string());
|
||||
}
|
||||
let client = app_server_connection::connect(target)
|
||||
.await
|
||||
.map_err(|_| "could not connect to check daemon feature settings".to_string())?;
|
||||
@@ -146,13 +158,29 @@ pub(super) async fn compatibility_warning(
|
||||
.is_some_and(|feature| feature.enabled)
|
||||
!= enabled
|
||||
{
|
||||
return Err(format!("daemon does not report features.{name}={enabled}"));
|
||||
restart_features = Some(
|
||||
SERVER_FEATURES
|
||||
.into_iter()
|
||||
.map(|feature| {
|
||||
(feature.key().to_string(), config.features.enabled(feature))
|
||||
})
|
||||
.collect(),
|
||||
);
|
||||
let state = if enabled { "enabled" } else { "disabled" };
|
||||
return Err(format!("This session requires {name} to be {state}"));
|
||||
}
|
||||
}
|
||||
Ok::<(), String>(())
|
||||
}
|
||||
.await;
|
||||
check
|
||||
.err()
|
||||
.map(|reason| format!("Running without the shared background server: {reason}."))
|
||||
match check {
|
||||
Ok(()) => Ok(None),
|
||||
Err(reason) if *allow_embedded_fallback => Ok(Some(format!(
|
||||
"Running without the shared background server: {reason}."
|
||||
))),
|
||||
Err(reason) => Err(CompatibilityError {
|
||||
reason,
|
||||
restart_features,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,11 +17,11 @@ fn audited_overrides_allow_daemon_without_allowing_arbitrary_config() {
|
||||
("features={worktrees=true}", true),
|
||||
(
|
||||
"features={worktrees=true,api_key_model_discovery=false}",
|
||||
false,
|
||||
true,
|
||||
),
|
||||
("features.auth_elicitation=false", false),
|
||||
("features.code_mode_host=false", false),
|
||||
("features.mcp_oauth_refresh_coordination=false", false),
|
||||
("features.auth_elicitation=false", true),
|
||||
("features.code_mode_host=false", true),
|
||||
("features.mcp_oauth_refresh_coordination=false", true),
|
||||
("suppress_unstable_features_warning=true", true),
|
||||
("suppress_unstable_features_warning='true'", false),
|
||||
("features={worktrees=true,shell_tool=false}", false),
|
||||
@@ -138,6 +138,119 @@ fn daemon_launch_telemetry_records_once_on_connection_or_early_return() {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn daemon_feature_compatibility_respects_required_and_optional_attachment()
|
||||
-> color_eyre::Result<()> {
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use futures::SinkExt;
|
||||
use futures::StreamExt;
|
||||
use serde_json::json;
|
||||
use tokio_tungstenite::tungstenite::Message;
|
||||
|
||||
for allow_embedded_fallback in [false, true] {
|
||||
for scenario in [
|
||||
"matching",
|
||||
"stale overrides",
|
||||
"conflicting client",
|
||||
"unsupported RPC",
|
||||
] {
|
||||
let home = TempDir::new()?;
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.loader_overrides(LoaderOverrides::without_managed_config_for_tests())
|
||||
.build()
|
||||
.await?;
|
||||
config.features.enable(Feature::ApiKeyModelDiscovery)?;
|
||||
if scenario == "conflicting client" {
|
||||
config.features.disable(Feature::ApiKeyModelDiscovery)?;
|
||||
}
|
||||
let features = [
|
||||
Feature::ApiKeyModelDiscovery,
|
||||
Feature::CodeModeHost,
|
||||
Feature::AuthElicitation,
|
||||
Feature::McpOAuthRefreshCoordination,
|
||||
].map(|feature| {
|
||||
let enabled = if feature == Feature::ApiKeyModelDiscovery && scenario == "stale overrides" {
|
||||
false
|
||||
} else if feature == Feature::ApiKeyModelDiscovery && scenario == "conflicting client" {
|
||||
true
|
||||
} else {
|
||||
config.features.enabled(feature)
|
||||
};
|
||||
json!({"name": feature.key(), "stage": "beta", "enabled": enabled, "defaultEnabled": false})
|
||||
});
|
||||
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?;
|
||||
let target = AppServerTarget::LocalDaemon {
|
||||
endpoint: RemoteAppServerEndpoint::WebSocket {
|
||||
websocket_url: format!("ws://{}", listener.local_addr()?),
|
||||
auth_token: None,
|
||||
},
|
||||
allow_embedded_fallback,
|
||||
};
|
||||
let server = tokio::spawn(async move {
|
||||
let (stream, _) = listener.accept().await.unwrap();
|
||||
let mut socket = tokio_tungstenite::accept_async(stream).await.unwrap();
|
||||
while let Some(Ok(Message::Text(text))) = socket.next().await {
|
||||
let JSONRPCMessage::Request(request) = serde_json::from_str(&text).unwrap()
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
let response = if request.method == "initialize" {
|
||||
json!({"id": request.id, "result": {"userAgent": "daemon-test"}})
|
||||
} else {
|
||||
assert_eq!(request.method, "experimentalFeature/list");
|
||||
assert_eq!(request.params.unwrap()["threadId"], json!(null));
|
||||
if scenario == "unsupported RPC" {
|
||||
json!({"id": request.id, "error": {"code": -32601, "message": "method not found"}})
|
||||
} else {
|
||||
json!({"id": request.id, "result": {"data": features, "nextCursor": null}})
|
||||
}
|
||||
};
|
||||
socket
|
||||
.send(Message::Text(response.to_string().into()))
|
||||
.await
|
||||
.unwrap();
|
||||
}
|
||||
});
|
||||
let result = daemon_startup::compatibility_warning(&target, &config).await;
|
||||
server.await?;
|
||||
if scenario == "matching" {
|
||||
assert_eq!(result?, None);
|
||||
continue;
|
||||
}
|
||||
let reason = match scenario {
|
||||
"stale overrides" => "This session requires api_key_model_discovery to be enabled",
|
||||
"conflicting client" => {
|
||||
"This session requires api_key_model_discovery to be disabled"
|
||||
}
|
||||
"unsupported RPC" => "Experimental feature request failed",
|
||||
_ => unreachable!(),
|
||||
};
|
||||
if allow_embedded_fallback {
|
||||
assert_eq!(
|
||||
result?,
|
||||
Some(format!(
|
||||
"Running without the shared background server: {reason}."
|
||||
))
|
||||
);
|
||||
} else {
|
||||
let error = result.unwrap_err().to_string();
|
||||
assert_eq!(
|
||||
error,
|
||||
format!(
|
||||
"Cannot use the shared background server: {reason}.\n{}",
|
||||
daemon_startup::FAILURE_HINT
|
||||
)
|
||||
);
|
||||
if scenario == "stale overrides" {
|
||||
insta::assert_snapshot!("daemon_feature_mismatch_error", error);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn daemon_connection_rejects_unprotected_socket_before_handshake() -> color_eyre::Result<()> {
|
||||
|
||||
@@ -2246,6 +2246,7 @@ fn should_show_bedrock_setup_wizard(
|
||||
.is_login_method_allowed(ForcedLoginMethod::Api)
|
||||
}
|
||||
|
||||
mod daemon_recovery;
|
||||
mod daemon_startup;
|
||||
mod daemon_telemetry;
|
||||
|
||||
|
||||
+17
@@ -0,0 +1,17 @@
|
||||
---
|
||||
source: tui/src/daemon_recovery_tests.rs
|
||||
expression: text
|
||||
---
|
||||
|
||||
Background server has incompatible feature settings
|
||||
This session requires api_key_model_discovery to be enabled
|
||||
Restart will use these shared feature settings:
|
||||
api_key_model_discovery = true
|
||||
mcp_oauth_refresh_coordination = false
|
||||
These settings persist and can disable functionality for other clients. Restart may
|
||||
interrupt active or queued work.
|
||||
|
||||
|
||||
1. Run without daemon this time
|
||||
2. Restart with these settings
|
||||
› 3. Cancel
|
||||
+13
@@ -0,0 +1,13 @@
|
||||
---
|
||||
source: tui/src/daemon_recovery_tests.rs
|
||||
expression: text
|
||||
---
|
||||
|
||||
Cannot use the background server
|
||||
code-mode host fallback policy requires embedded mode
|
||||
|
||||
|
||||
1. Run without daemon this time
|
||||
Restart with these settings (disabled) Restart cannot resolve this compatibility
|
||||
check.
|
||||
› 2. Cancel
|
||||
+17
@@ -0,0 +1,17 @@
|
||||
---
|
||||
source: tui/src/daemon_recovery_tests.rs
|
||||
expression: text
|
||||
---
|
||||
|
||||
Background server has incompatible feature settings
|
||||
This session requires api_key_model_discovery to be enabled
|
||||
Restart will use these shared feature settings:
|
||||
api_key_model_discovery = true
|
||||
mcp_oauth_refresh_coordination = false
|
||||
These settings persist and can disable functionality for other clients. Restart may
|
||||
interrupt active or queued work.
|
||||
|
||||
|
||||
1. Run without daemon this time
|
||||
Restart with these settings (disabled) This server is not managed by Codex.
|
||||
› 2. Cancel
|
||||
+6
@@ -0,0 +1,6 @@
|
||||
---
|
||||
source: tui/src/daemon_startup_tests.rs
|
||||
expression: error
|
||||
---
|
||||
Cannot use the shared background server: This session requires api_key_model_discovery to be enabled.
|
||||
To work without the background server, rerun the same command with --no-daemon (including resume or fork and its arguments).
|
||||
@@ -491,7 +491,10 @@ pub(super) async fn run_main_inner(
|
||||
daemon_exclusion = Some("Bedrock sign-in");
|
||||
app_server_target = AppServerTarget::Embedded;
|
||||
}
|
||||
let daemon_features = daemon_startup::server_features(&cli_kv_overrides);
|
||||
let mut daemon_features = daemon_startup::server_features(&cli_kv_overrides);
|
||||
// Disabling shared services requires confirmation, even on a fresh auto-start.
|
||||
daemon_features.retain(|_, enabled| *enabled);
|
||||
let mut managed_daemon = false;
|
||||
if auto_start_daemon && daemon_exclusion.is_none() {
|
||||
startup_draft.flush_pending_events().await?;
|
||||
let output = startup_draft
|
||||
@@ -506,6 +509,7 @@ pub(super) async fn run_main_inner(
|
||||
})
|
||||
})
|
||||
.await?;
|
||||
managed_daemon = output.backend.is_some();
|
||||
app_server_target = AppServerTarget::LocalDaemon {
|
||||
endpoint: RemoteAppServerEndpoint::UnixSocket {
|
||||
socket_path: AbsolutePathBuf::from_absolute_path_checked(output.socket_path)?,
|
||||
@@ -517,12 +521,13 @@ pub(super) async fn run_main_inner(
|
||||
let compatibility_warning = if cli.agents_overview {
|
||||
None
|
||||
} else {
|
||||
startup_draft
|
||||
.run_until(daemon_startup::compatibility_warning(
|
||||
&app_server_target,
|
||||
&config,
|
||||
))
|
||||
.await?
|
||||
daemon_recovery::check(
|
||||
&mut startup_draft,
|
||||
&app_server_target,
|
||||
&config,
|
||||
managed_daemon,
|
||||
)
|
||||
.await?
|
||||
};
|
||||
if compatibility_warning.is_some() {
|
||||
app_server_target = AppServerTarget::Embedded;
|
||||
|
||||
@@ -71,11 +71,11 @@ async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> R
|
||||
let (snapshot, warning_end) = match scenario {
|
||||
"default" => (
|
||||
"daemon_feature_mismatch",
|
||||
"features.api_key_model_discovery=false.",
|
||||
"api_key_model_discovery to be disabled.",
|
||||
),
|
||||
"explicit" => (
|
||||
"daemon_override_mismatch",
|
||||
"features.api_key_model_discovery=true.",
|
||||
"api_key_model_discovery to be enabled.",
|
||||
),
|
||||
"host policy" => ("daemon_host_policy_mismatch", "requires embedded mode."),
|
||||
_ => unreachable!(),
|
||||
|
||||
+1
-1
@@ -2,4 +2,4 @@
|
||||
source: tui/tests/suite/daemon_compatibility.rs
|
||||
expression: warning
|
||||
---
|
||||
⚠ Running without the shared background server: daemon does not report features.api_key_model_discovery=false.
|
||||
⚠ Running without the shared background server: This session requires api_key_model_discovery to be disabled.
|
||||
|
||||
+1
-1
@@ -2,4 +2,4 @@
|
||||
source: tui/tests/suite/daemon_compatibility.rs
|
||||
expression: warning
|
||||
---
|
||||
⚠ Running without the shared background server: daemon does not report features.api_key_model_discovery=true.
|
||||
⚠ Running without the shared background server: This session requires api_key_model_discovery to be enabled.
|
||||
|
||||
Reference in New Issue
Block a user