mirror of
https://github.com/openai/codex.git
synced 2026-09-09 15:58:47 +00:00
Use server defaults when starting TUI background tasks (#43261)
## Why Background tasks launched from the agents overview could use client model settings instead of the server defaults for the destination directory. ## What changed - Read `config/read` for the task's destination before starting the thread, preserving explicit launch settings and applying managed new-thread defaults. - Use the server's model provider for embedded sessions when no model or provider override is set. When the server clears the model and fast mode is enabled, resolve the fallback model from the server catalog. - Restore the prompt and show an error if settings cannot be loaded. Keep the existing fallback for older servers that do not support `config/read`. ## Testing Add request-level tests for local and remote destinations, model and provider selection, explicit and managed defaults, fast-mode fallback, and prompt recovery after a configuration read failure. GitOrigin-RevId: 50c84fba964463303442dd849e1280bcd5b4f467
This commit is contained in:
@@ -644,12 +644,73 @@ impl App {
|
||||
.add_error_message("Permission profile has different settings.".to_string());
|
||||
}
|
||||
self.apply_runtime_policy_overrides(&mut config, RuntimePolicyOverrideScope::All);
|
||||
let defaults_cwd = match app_server.thread_params_mode() {
|
||||
crate::app_server_session::ThreadParamsMode::Embedded => config.cwd.as_path(),
|
||||
crate::app_server_session::ThreadParamsMode::Remote => remote_cwd
|
||||
.as_deref()
|
||||
.or_else(|| app_server.remote_cwd_override())
|
||||
.unwrap_or(Path::new(".")),
|
||||
};
|
||||
let mut server_model_cleared = false;
|
||||
match super::new_session::read_new_session_defaults(app_server, defaults_cwd).await {
|
||||
Ok(Some(defaults)) => {
|
||||
server_model_cleared = defaults.model.is_none();
|
||||
let use_server_provider = matches!(
|
||||
app_server.thread_params_mode(),
|
||||
crate::app_server_session::ThreadParamsMode::Embedded
|
||||
) && self.harness_overrides.model.is_none()
|
||||
&& !super::new_session::has_launch_setting(
|
||||
&config,
|
||||
&self.cli_kv_overrides,
|
||||
"model",
|
||||
)
|
||||
&& self.harness_overrides.model_provider.is_none()
|
||||
&& !super::new_session::has_launch_setting(
|
||||
&config,
|
||||
&self.cli_kv_overrides,
|
||||
"model_provider",
|
||||
);
|
||||
super::new_session::overlay_new_session_defaults(
|
||||
&mut config,
|
||||
&defaults,
|
||||
&self.cli_kv_overrides,
|
||||
&self.harness_overrides,
|
||||
);
|
||||
// Embedded thread/start sends a provider ID alongside the selected model.
|
||||
if use_server_provider {
|
||||
config.model_provider_id = defaults
|
||||
.model_provider
|
||||
.unwrap_or_else(|| "openai".to_string());
|
||||
}
|
||||
}
|
||||
Ok(None) => {}
|
||||
Err(error) => {
|
||||
self.restore_agents_overview_prompt(prompt);
|
||||
return self.chat_widget.add_error_message(format!(
|
||||
"Failed to load background task settings: {error}"
|
||||
));
|
||||
}
|
||||
}
|
||||
apply_managed_new_thread_defaults(
|
||||
&mut config,
|
||||
app_server.managed_new_thread_defaults(),
|
||||
&self.cli_kv_overrides,
|
||||
&self.harness_overrides,
|
||||
);
|
||||
if server_model_cleared
|
||||
&& config.model.is_none()
|
||||
&& config.features.enabled(Feature::FastMode)
|
||||
{
|
||||
// Bootstrap's fallback model may be seeded from the client. Resolve tiers
|
||||
// against the server catalog when config/read cleared the model.
|
||||
config.model = self
|
||||
.model_catalog
|
||||
.models
|
||||
.iter()
|
||||
.find(|model| model.is_default)
|
||||
.or_else(|| self.model_catalog.models.first())
|
||||
.map(|model| model.model.clone());
|
||||
}
|
||||
match app_server
|
||||
.start_thread_with_session_start_source(
|
||||
&self.local_settings,
|
||||
|
||||
@@ -36,31 +36,36 @@ pub(super) async fn read_new_session_defaults(
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn has_launch_setting(
|
||||
config: &Config,
|
||||
cli_kv_overrides: &[(String, TomlValue)],
|
||||
key: &str,
|
||||
) -> bool {
|
||||
// A remote server cannot resolve this invocation's explicitly selected local profile.
|
||||
cli_kv_overrides.iter().any(|(path, _)| path == key)
|
||||
|| config.config_layer_stack.layers_high_to_low().any(|layer| {
|
||||
layer.disabled_reason.is_none()
|
||||
&& matches!(
|
||||
layer.name,
|
||||
ConfigLayerSource::User {
|
||||
profile: Some(_),
|
||||
..
|
||||
}
|
||||
)
|
||||
&& layer.config.get(key).is_some()
|
||||
})
|
||||
}
|
||||
|
||||
pub(super) fn overlay_new_session_defaults(
|
||||
config: &mut Config,
|
||||
defaults: &codex_app_server_protocol::Config,
|
||||
cli_kv_overrides: &[(String, TomlValue)],
|
||||
harness_overrides: &ConfigOverrides,
|
||||
) {
|
||||
// A remote server cannot resolve this invocation's explicitly selected local profile.
|
||||
let has_launch_setting = |key: &str| {
|
||||
cli_kv_overrides.iter().any(|(path, _)| path == key)
|
||||
|| config.config_layer_stack.layers_high_to_low().any(|layer| {
|
||||
layer.disabled_reason.is_none()
|
||||
&& matches!(
|
||||
layer.name,
|
||||
ConfigLayerSource::User {
|
||||
profile: Some(_),
|
||||
..
|
||||
}
|
||||
)
|
||||
&& layer.config.get(key).is_some()
|
||||
})
|
||||
};
|
||||
if harness_overrides.model.is_none() && !has_launch_setting("model") {
|
||||
if harness_overrides.model.is_none() && !has_launch_setting(config, cli_kv_overrides, "model") {
|
||||
config.model = defaults.model.clone();
|
||||
}
|
||||
if !has_launch_setting("model_reasoning_effort") {
|
||||
if !has_launch_setting(config, cli_kv_overrides, "model_reasoning_effort") {
|
||||
config.model_reasoning_effort = defaults.model_reasoning_effort.clone();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,8 @@ mod backend_banner_recovery_tests;
|
||||
mod backend_banner_startup_tests;
|
||||
#[path = "tests/background_exit_tests.rs"]
|
||||
mod background_exit_tests;
|
||||
#[path = "tests/background_task_defaults_tests.rs"]
|
||||
mod background_task_defaults_tests;
|
||||
#[path = "tests/buffered_replay.rs"]
|
||||
mod buffered_replay;
|
||||
#[path = "tests/connector_policy.rs"]
|
||||
|
||||
369
codex-rs/tui/src/app/tests/background_task_defaults_tests.rs
Normal file
369
codex-rs/tui/src/app/tests/background_task_defaults_tests.rs
Normal file
@@ -0,0 +1,369 @@
|
||||
//! Request-level coverage for background-task server defaults and dispatch recovery.
|
||||
|
||||
use super::*;
|
||||
use crate::app::agents_overview::AGENTS_OVERVIEW_VIEW_ID;
|
||||
use crate::app::tests::session_lifecycle_requests::HistoryCapabilities;
|
||||
use crate::app::tests::session_lifecycle_requests::recorded_params;
|
||||
use crate::app::tests::session_lifecycle_requests::start_recording_app_server_with_history;
|
||||
use crate::test_support::PathBufExt;
|
||||
use codex_state::SqliteConfig;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
async fn background_task_reads_server_defaults_for_actual_destination() -> Result<()> {
|
||||
for (mode, explicit_cwd, launch_override, expected_cwd, expected_model) in [
|
||||
("local", false, false, "launch", "server-model"),
|
||||
("local", true, false, "destination", "destination-model"),
|
||||
("local-cli-provider", false, false, "launch", "server-model"),
|
||||
("local-cli-model", false, false, "launch", "cli-model"),
|
||||
(
|
||||
"local-default-provider",
|
||||
false,
|
||||
false,
|
||||
"launch",
|
||||
"server-model",
|
||||
),
|
||||
("remote", true, true, "destination", "destination-model"),
|
||||
("remote", false, true, "launch", "server-model"),
|
||||
("remote", false, false, ".", "server-model"),
|
||||
("remote-null-fast", false, false, ".", ""),
|
||||
] {
|
||||
let client_home = tempdir()?;
|
||||
let server_home = tempdir()?;
|
||||
let launch = tempdir()?;
|
||||
let destination = tempdir()?;
|
||||
std::fs::write(
|
||||
client_home.path().join("config.toml"),
|
||||
format!(
|
||||
"model = \"{}\"\nmodel_reasoning_effort = \"low\"\n{}",
|
||||
if mode == "remote-null-fast" {
|
||||
"gpt-5.2"
|
||||
} else {
|
||||
"client-model"
|
||||
},
|
||||
if mode == "local-default-provider" {
|
||||
"model_provider = \"ollama\"\n"
|
||||
} else {
|
||||
""
|
||||
}
|
||||
),
|
||||
)?;
|
||||
std::fs::write(
|
||||
server_home.path().join("config.toml"),
|
||||
format!(
|
||||
"{}model_reasoning_effort = \"high\"\n{}",
|
||||
if mode == "remote-null-fast" {
|
||||
""
|
||||
} else {
|
||||
"model = \"server-model\"\n"
|
||||
},
|
||||
if mode == "local" || mode == "local-cli-provider" || mode == "local-cli-model" {
|
||||
"model_provider = \"ollama\"\n"
|
||||
} else {
|
||||
""
|
||||
}
|
||||
),
|
||||
)?;
|
||||
std::fs::create_dir(destination.path().join(".codex"))?;
|
||||
std::fs::write(
|
||||
destination.path().join(".codex/config.toml"),
|
||||
"model = \"destination-model\"\nservice_tier = \"flex\"\n",
|
||||
)?;
|
||||
for home in [client_home.path(), server_home.path()] {
|
||||
crate::legacy_core::config::set_project_trust_level(
|
||||
home,
|
||||
destination.path(),
|
||||
codex_protocol::config_types::TrustLevel::Trusted,
|
||||
)
|
||||
.map_err(|error| color_eyre::eyre::eyre!(error.to_string()))?;
|
||||
}
|
||||
let mut app = make_test_app_with_channels().await.0;
|
||||
if mode == "local" && explicit_cwd {
|
||||
app.harness_overrides.model_provider = Some("openai".into());
|
||||
}
|
||||
if mode == "local-cli-provider" {
|
||||
app.cli_kv_overrides
|
||||
.push(("model_provider".into(), TomlValue::String("openai".into())));
|
||||
}
|
||||
if mode == "local-cli-model" {
|
||||
app.harness_overrides.model = Some("cli-model".into());
|
||||
}
|
||||
app.chat_widget.set_service_tier(Some("priority".into()));
|
||||
app.harness_overrides.cwd = Some(launch.path().to_path_buf());
|
||||
app.config = ConfigBuilder::default()
|
||||
.codex_home(client_home.path().to_path_buf())
|
||||
.loader_overrides(app.loader_overrides.clone())
|
||||
.cli_overrides(app.cli_kv_overrides.clone())
|
||||
.harness_overrides(app.harness_overrides.clone())
|
||||
.build()
|
||||
.await?;
|
||||
if mode == "remote-null-fast" {
|
||||
app.config.features.enable(Feature::FastMode)?;
|
||||
}
|
||||
app.chat_widget
|
||||
.handle_thread_session_quiet(test_thread_session(
|
||||
ThreadId::new(),
|
||||
launch.path().to_path_buf(),
|
||||
));
|
||||
let mut server_config = app.config.clone();
|
||||
server_config.codex_home = server_home.path().to_path_buf().abs();
|
||||
server_config.sqlite = SqliteConfig::new_for_testing(server_home.path().abs());
|
||||
let thread_mode = if mode.starts_with("remote") {
|
||||
crate::app_server_session::ThreadParamsMode::Remote
|
||||
} else {
|
||||
crate::app_server_session::ThreadParamsMode::Embedded
|
||||
};
|
||||
let (mut server, requests, proxy) = start_recording_app_server_with_history(
|
||||
&server_config,
|
||||
HistoryCapabilities::Current,
|
||||
/*blocked_thread_list*/ None,
|
||||
/*failed_thread_name*/ None,
|
||||
thread_mode,
|
||||
LoaderOverrides {
|
||||
user_config_path: Some(server_home.path().join("config.toml").abs()),
|
||||
..LoaderOverrides::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
if launch_override {
|
||||
server = server.with_remote_cwd_override(Some(launch.path().to_path_buf()));
|
||||
}
|
||||
let bootstrap = server.bootstrap(&app.config).await?;
|
||||
let expected_model = if mode == "remote-null-fast" {
|
||||
let default_model = bootstrap
|
||||
.available_models
|
||||
.iter()
|
||||
.find(|model| model.is_default)
|
||||
.or_else(|| bootstrap.available_models.first())
|
||||
.expect("server catalog model")
|
||||
.model
|
||||
.clone();
|
||||
app.model_catalog = Arc::new(ModelCatalog::new(bootstrap.available_models));
|
||||
default_model
|
||||
} else {
|
||||
expected_model.to_string()
|
||||
};
|
||||
app.dispatch_agents_overview_task(
|
||||
&mut server,
|
||||
"background prompt".into(),
|
||||
explicit_cwd.then(|| destination.path().to_path_buf().abs()),
|
||||
)
|
||||
.await;
|
||||
let cwd = match expected_cwd {
|
||||
"launch" => launch.path().display().to_string(),
|
||||
"destination" => destination.path().display().to_string(),
|
||||
"." => ".".to_string(),
|
||||
_ => unreachable!(),
|
||||
};
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "config/read"),
|
||||
vec![serde_json::json!({"cwd": cwd})],
|
||||
"{mode} {expected_cwd}"
|
||||
);
|
||||
let starts = recorded_params(&requests, "thread/start");
|
||||
assert_eq!(starts.len(), 1, "{mode} {expected_cwd}");
|
||||
assert_eq!(
|
||||
(
|
||||
&starts[0]["cwd"],
|
||||
&starts[0]["model"],
|
||||
&starts[0]["modelProvider"],
|
||||
&starts[0]["config"]["model_reasoning_effort"],
|
||||
&starts[0]["serviceTier"],
|
||||
),
|
||||
(
|
||||
&if mode.starts_with("remote") && !explicit_cwd && !launch_override {
|
||||
serde_json::Value::Null
|
||||
} else {
|
||||
serde_json::json!(cwd)
|
||||
},
|
||||
&serde_json::json!(expected_model),
|
||||
&if mode.starts_with("remote") {
|
||||
serde_json::Value::Null
|
||||
} else if mode == "local" && !explicit_cwd {
|
||||
serde_json::json!("ollama")
|
||||
} else {
|
||||
serde_json::json!("openai")
|
||||
},
|
||||
&serde_json::json!("high"),
|
||||
&serde_json::json!(if mode == "local" && explicit_cwd {
|
||||
"flex"
|
||||
} else {
|
||||
"priority"
|
||||
}),
|
||||
),
|
||||
"{mode} {expected_cwd}"
|
||||
);
|
||||
assert_eq!(recorded_params(&requests, "turn/start").len(), 1);
|
||||
server.shutdown().await?;
|
||||
proxy.await??;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn background_task_preserves_explicit_choices_and_managed_defaults() -> Result<()> {
|
||||
for (choice, expected_model, expected_effort) in [
|
||||
("saved", "server-model", "high"),
|
||||
("cli_effort", "server-model", "low"),
|
||||
("profile_model", "profile-model", "high"),
|
||||
("managed", "managed-model", "medium"),
|
||||
] {
|
||||
let client_home = tempdir()?;
|
||||
let server_home = tempdir()?;
|
||||
std::fs::write(
|
||||
client_home.path().join("config.toml"),
|
||||
"model = \"client-model\"\nmodel_reasoning_effort = \"low\"\n",
|
||||
)?;
|
||||
std::fs::write(
|
||||
server_home.path().join("config.toml"),
|
||||
"model = \"server-model\"\nmodel_reasoning_effort = \"high\"\n",
|
||||
)?;
|
||||
if choice == "managed" || choice.starts_with("cli_") {
|
||||
std::fs::write(
|
||||
server_home.path().join("requirements.toml"),
|
||||
"[models.new_thread]\nmodel = \"managed-model\"\nmodel_reasoning_effort = \"medium\"\n",
|
||||
)?;
|
||||
}
|
||||
let mut app = make_test_app_with_channels().await.0;
|
||||
match choice {
|
||||
"cli_effort" => app.cli_kv_overrides.push((
|
||||
"model_reasoning_effort".into(),
|
||||
TomlValue::String("low".into()),
|
||||
)),
|
||||
"profile_model" => {
|
||||
let path = client_home.path().join("work.config.toml");
|
||||
std::fs::write(&path, "model = \"profile-model\"\n")?;
|
||||
app.loader_overrides.user_config_path = Some(path.abs());
|
||||
app.loader_overrides.user_config_profile = Some("work".parse()?);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
app.config = ConfigBuilder::default()
|
||||
.codex_home(client_home.path().to_path_buf())
|
||||
.loader_overrides(app.loader_overrides.clone())
|
||||
.cli_overrides(app.cli_kv_overrides.clone())
|
||||
.harness_overrides(app.harness_overrides.clone())
|
||||
.build()
|
||||
.await?;
|
||||
let mut server_config = app.config.clone();
|
||||
server_config.codex_home = server_home.path().to_path_buf().abs();
|
||||
server_config.sqlite = SqliteConfig::new_for_testing(server_home.path().abs());
|
||||
let (mut server, requests, proxy) = start_recording_app_server_with_history(
|
||||
&server_config,
|
||||
HistoryCapabilities::Current,
|
||||
/*blocked_thread_list*/ None,
|
||||
/*failed_thread_name*/ None,
|
||||
crate::app_server_session::ThreadParamsMode::Remote,
|
||||
LoaderOverrides {
|
||||
user_config_path: Some(server_home.path().join("config.toml").abs()),
|
||||
system_requirements_path: Some(server_home.path().join("requirements.toml")),
|
||||
..LoaderOverrides::default()
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
server.bootstrap(&app.config).await?;
|
||||
app.dispatch_agents_overview_task(
|
||||
&mut server,
|
||||
"background prompt".into(),
|
||||
/*cwd*/ None,
|
||||
)
|
||||
.await;
|
||||
let starts = recorded_params(&requests, "thread/start");
|
||||
assert_eq!(starts.len(), 1, "{choice}");
|
||||
assert_eq!(
|
||||
(
|
||||
&starts[0]["model"],
|
||||
&starts[0]["config"]["model_reasoning_effort"]
|
||||
),
|
||||
(
|
||||
&serde_json::json!(expected_model),
|
||||
&serde_json::json!(expected_effort)
|
||||
),
|
||||
"{choice}"
|
||||
);
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "turn/start").len(),
|
||||
1,
|
||||
"{choice}"
|
||||
);
|
||||
server.shutdown().await?;
|
||||
proxy.await??;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn background_task_read_failure_keeps_prompt_and_does_not_start() -> Result<()> {
|
||||
for capability in [
|
||||
HistoryCapabilities::ConfigReadFails,
|
||||
HistoryCapabilities::ConfigReadUnsupported(-32600),
|
||||
HistoryCapabilities::ConfigReadUnsupported(-32601),
|
||||
] {
|
||||
let (mut app, mut events, _) = make_test_app_with_channels().await;
|
||||
app.harness_overrides.model = Some("local-model".into());
|
||||
let view = app.agents_overview_view(Vec::new(), /*selected_thread_id*/ None);
|
||||
app.chat_widget.show_bottom_pane_view(Box::new(view));
|
||||
let (mut server, requests, proxy) = start_recording_app_server_with_history(
|
||||
&app.config,
|
||||
capability,
|
||||
/*blocked_thread_list*/ None,
|
||||
/*failed_thread_name*/ None,
|
||||
crate::app_server_session::ThreadParamsMode::Embedded,
|
||||
LoaderOverrides::default(),
|
||||
)
|
||||
.await?;
|
||||
app.dispatch_agents_overview_task(
|
||||
&mut server,
|
||||
"retry background prompt".into(),
|
||||
/*cwd*/ None,
|
||||
)
|
||||
.await;
|
||||
let failed = capability == HistoryCapabilities::ConfigReadFails;
|
||||
assert_eq!(recorded_params(&requests, "config/read").len(), 1);
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "thread/start").len(),
|
||||
usize::from(!failed)
|
||||
);
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "turn/start").len(),
|
||||
usize::from(!failed)
|
||||
);
|
||||
if failed {
|
||||
assert!(app.agents_overview.dispatched_requests.is_empty());
|
||||
assert_eq!(
|
||||
app.agents_overview
|
||||
.view_state
|
||||
.lock()
|
||||
.unwrap()
|
||||
.composer
|
||||
.as_ref()
|
||||
.unwrap()
|
||||
.current_text_with_pending(),
|
||||
"retry background prompt"
|
||||
);
|
||||
assert!(
|
||||
app.chat_widget
|
||||
.selected_index_for_present_view(AGENTS_OVERVIEW_VIEW_ID)
|
||||
.is_some()
|
||||
);
|
||||
let error = std::iter::from_fn(|| events.try_recv().ok())
|
||||
.filter_map(|event| match event {
|
||||
AppEvent::InsertHistoryCell(cell) => {
|
||||
Some(lines_to_single_string(&cell.display_lines(/*width*/ 80)))
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.find(|message| message.contains("Failed to load background task settings"))
|
||||
.expect("visible read error");
|
||||
insta::assert_snapshot!(error, @"■ Failed to load background task settings: config/read failed in TUI");
|
||||
} else {
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "thread/start")[0]["model"],
|
||||
"local-model"
|
||||
);
|
||||
}
|
||||
server.shutdown().await?;
|
||||
proxy.await??;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
@@ -99,7 +99,7 @@ async fn same_thread_retry_keeps_subscription_and_restores_draft() -> Result<()>
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
enum HistoryCapabilities {
|
||||
pub(super) enum HistoryCapabilities {
|
||||
Current,
|
||||
LegacyOnly,
|
||||
LegacyOnlyUnsupportedVariant,
|
||||
@@ -158,7 +158,7 @@ pub(super) async fn start_recording_remote_app_server(
|
||||
}
|
||||
|
||||
/// Proxies a real app server while optionally rejecting modern pagination like an older server.
|
||||
async fn start_recording_app_server_with_history(
|
||||
pub(super) async fn start_recording_app_server_with_history(
|
||||
config: &Config,
|
||||
history_capabilities: HistoryCapabilities,
|
||||
mut blocked_thread_list: Option<(ThreadId, oneshot::Sender<()>, oneshot::Receiver<()>)>,
|
||||
|
||||
Reference in New Issue
Block a user