mirror of
https://github.com/openai/codex.git
synced 2026-11-02 16:20:35 +00:00
[1 of 4] tui: route primary settings writes through app server (#22913)
## Why The TUI can run against a remote app server, but several high-traffic settings still persisted by editing the local config file. That sends remote sessions' preference writes to the wrong machine and lets local disk state drift from the app-server-owned config. This is **[1 of 4]** in a stacked series that moves TUI-owned config mutations onto app-server APIs. ## What changed - Added a small TUI helper for typed app-server config writes. - Routed primary interactive preference writes through `config/batchWrite`. - Preserved existing profile scoping for settings that already support `profiles.<profile>.*` overrides. ## Config keys affected - `model` - `model_reasoning_effort` - `personality` - `service_tier` - `plan_mode_reasoning_effort` - `approvals_reviewer` - `notice.fast_default_opt_out` - Profile-scoped equivalents under `profiles.<profile>.*` ## Suggested manual validation - Connect the TUI to a remote app server, change `model` and `model_reasoning_effort`, reconnect, and confirm the remote config retained both values while the local `config.toml` did not change. - Change `personality`, `plan_mode_reasoning_effort`, and the explicit auto-review selection, then reconnect and confirm those choices persist through the app server. - Clear the service tier back to default and confirm `service_tier` is cleared while `notice.fast_default_opt_out = true` is persisted remotely. - Repeat one setting change with an active profile and confirm the write lands under `profiles.<profile>.*`. ## Stack 1. [#22913](https://github.com/openai/codex/pull/22913) `[1 of 4]` primary settings writes 2. [#22914](https://github.com/openai/codex/pull/22914) `[2 of 4]` app and skill enablement 3. [#22915](https://github.com/openai/codex/pull/22915) `[3 of 4]` feature and memory toggles 4. [#22916](https://github.com/openai/codex/pull/22916) `[4 of 4]` startup and onboarding bookkeeping
This commit is contained in:
1 parent
061a614d85
commit
0445b290fe
5 files changed
+280
-54
No files matched your search
@@ -403,10 +403,47 @@ fn parse_key_path(path: &str) -> Result<Vec<String>, String> {
|
|||||||
if path.trim().is_empty() {
|
if path.trim().is_empty() {
|
||||||
return Err("keyPath must not be empty".to_string());
|
return Err("keyPath must not be empty".to_string());
|
||||||
}
|
}
|
||||||
Ok(path
|
|
||||||
.split('.')
|
let mut segments = Vec::new();
|
||||||
.map(std::string::ToString::to_string)
|
let mut segment = String::new();
|
||||||
.collect())
|
let mut chars = path.chars();
|
||||||
|
let mut quoted = false;
|
||||||
|
|
||||||
|
// Split on dots unless they appear inside a quoted segment. Bare segments
|
||||||
|
// intentionally stay permissive so existing paths like `sample@catalog`
|
||||||
|
// remain valid.
|
||||||
|
while let Some(ch) = chars.next() {
|
||||||
|
match ch {
|
||||||
|
'"' if segment.is_empty() && !quoted => quoted = true,
|
||||||
|
'"' if quoted => quoted = false,
|
||||||
|
'\\' if quoted => {
|
||||||
|
// Quoted segments may escape punctuation that would otherwise
|
||||||
|
// participate in parsing, such as `.` or `"`.
|
||||||
|
let Some(escaped) = chars.next() else {
|
||||||
|
return Err("unterminated escape in keyPath".to_string());
|
||||||
|
};
|
||||||
|
segment.push(escaped);
|
||||||
|
}
|
||||||
|
'.' if !quoted => {
|
||||||
|
if segment.is_empty() {
|
||||||
|
return Err("keyPath segments must not be empty".to_string());
|
||||||
|
}
|
||||||
|
segments.push(std::mem::take(&mut segment));
|
||||||
|
}
|
||||||
|
'"' => return Err("invalid quoted keyPath segment".to_string()),
|
||||||
|
_ => segment.push(ch),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if quoted {
|
||||||
|
return Err("unterminated quoted keyPath segment".to_string());
|
||||||
|
}
|
||||||
|
if segment.is_empty() {
|
||||||
|
return Err("keyPath segments must not be empty".to_string());
|
||||||
|
}
|
||||||
|
|
||||||
|
segments.push(segment);
|
||||||
|
Ok(segments)
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
|
|||||||
@@ -893,6 +893,71 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn config_batch_write_preserves_dotted_profile_names() -> Result<()> {
|
||||||
|
let tmp_dir = TempDir::new()?;
|
||||||
|
let codex_home = tmp_dir.path().canonicalize()?;
|
||||||
|
write_config(
|
||||||
|
&tmp_dir,
|
||||||
|
r#"
|
||||||
|
profile = "team.prod"
|
||||||
|
|
||||||
|
[profiles."team.prod"]
|
||||||
|
model = "gpt-5.3-spark"
|
||||||
|
|
||||||
|
[profiles.team.prod]
|
||||||
|
model = "should-stay-put"
|
||||||
|
"#,
|
||||||
|
)?;
|
||||||
|
|
||||||
|
let mut mcp = McpProcess::new(&codex_home).await?;
|
||||||
|
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||||
|
|
||||||
|
let batch_id = mcp
|
||||||
|
.send_config_batch_write_request(ConfigBatchWriteParams {
|
||||||
|
file_path: Some(codex_home.join("config.toml").display().to_string()),
|
||||||
|
edits: vec![
|
||||||
|
ConfigEdit {
|
||||||
|
key_path: "profiles.\"team.prod\".model".to_string(),
|
||||||
|
value: json!("gpt-5.5"),
|
||||||
|
merge_strategy: MergeStrategy::Replace,
|
||||||
|
},
|
||||||
|
ConfigEdit {
|
||||||
|
key_path: "items.sample@catalog.enabled".to_string(),
|
||||||
|
value: json!(true),
|
||||||
|
merge_strategy: MergeStrategy::Replace,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
expected_version: None,
|
||||||
|
reload_user_config: false,
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
let batch_resp: JSONRPCResponse = timeout(
|
||||||
|
DEFAULT_READ_TIMEOUT,
|
||||||
|
mcp.read_stream_until_response_message(RequestId::Integer(batch_id)),
|
||||||
|
)
|
||||||
|
.await??;
|
||||||
|
let batch_write: ConfigWriteResponse = to_response(batch_resp)?;
|
||||||
|
assert_eq!(batch_write.status, WriteStatus::Ok);
|
||||||
|
|
||||||
|
let config: toml::Value =
|
||||||
|
toml::from_str(&std::fs::read_to_string(codex_home.join("config.toml"))?)?;
|
||||||
|
assert_eq!(
|
||||||
|
config["profiles"]["team.prod"]["model"].as_str(),
|
||||||
|
Some("gpt-5.5")
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
config["profiles"]["team"]["prod"]["model"].as_str(),
|
||||||
|
Some("should-stay-put")
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
config["items"]["sample@catalog"]["enabled"].as_bool(),
|
||||||
|
Some(true)
|
||||||
|
);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn config_batch_write_updates_multiple_desktop_settings() -> Result<()> {
|
async fn config_batch_write_updates_multiple_desktop_settings() -> Result<()> {
|
||||||
let tmp_dir = TempDir::new()?;
|
let tmp_dir = TempDir::new()?;
|
||||||
|
|||||||
@@ -1183,11 +1183,15 @@ impl App {
|
|||||||
}
|
}
|
||||||
AppEvent::PersistModelSelection { model, effort } => {
|
AppEvent::PersistModelSelection { model, effort } => {
|
||||||
let profile = self.active_profile.as_deref();
|
let profile = self.active_profile.as_deref();
|
||||||
match ConfigEditsBuilder::for_config(&self.config)
|
match crate::config_update::write_config_batch(
|
||||||
.with_profile(profile)
|
app_server.request_handle(),
|
||||||
.set_model(Some(model.as_str()), effort)
|
crate::config_update::build_model_selection_edits(
|
||||||
.apply()
|
profile,
|
||||||
.await
|
model.as_str(),
|
||||||
|
effort,
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.await
|
||||||
{
|
{
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
let effort_label = effort
|
let effort_label = effort
|
||||||
@@ -1261,11 +1265,14 @@ impl App {
|
|||||||
}
|
}
|
||||||
AppEvent::PersistPersonalitySelection { personality } => {
|
AppEvent::PersistPersonalitySelection { personality } => {
|
||||||
let profile = self.active_profile.as_deref();
|
let profile = self.active_profile.as_deref();
|
||||||
match ConfigEditsBuilder::for_config(&self.config)
|
match crate::config_update::write_config_batch(
|
||||||
.with_profile(profile)
|
app_server.request_handle(),
|
||||||
.set_personality(Some(personality))
|
vec![crate::config_update::replace_config_value(
|
||||||
.apply()
|
crate::config_update::profile_scoped_key_path(profile, "personality"),
|
||||||
.await
|
serde_json::json!(personality.to_string()),
|
||||||
|
)],
|
||||||
|
)
|
||||||
|
.await
|
||||||
{
|
{
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
let label = Self::personality_label(personality);
|
let label = Self::personality_label(personality);
|
||||||
@@ -1298,14 +1305,16 @@ impl App {
|
|||||||
self.refresh_status_line();
|
self.refresh_status_line();
|
||||||
let profile = self.active_profile.as_deref();
|
let profile = self.active_profile.as_deref();
|
||||||
self.config.service_tier = service_tier.clone();
|
self.config.service_tier = service_tier.clone();
|
||||||
let mut edits = ConfigEditsBuilder::for_config(&self.config)
|
let edits = crate::config_update::build_service_tier_selection_edits(
|
||||||
.with_profile(profile)
|
profile,
|
||||||
.set_service_tier(service_tier.clone());
|
service_tier.as_deref(),
|
||||||
|
);
|
||||||
if service_tier.is_none() {
|
if service_tier.is_none() {
|
||||||
self.config.notices.fast_default_opt_out = Some(true);
|
self.config.notices.fast_default_opt_out = Some(true);
|
||||||
edits = edits.set_fast_default_opt_out(/*opted_out*/ true);
|
|
||||||
}
|
}
|
||||||
match edits.apply().await {
|
match crate::config_update::write_config_batch(app_server.request_handle(), edits)
|
||||||
|
.await
|
||||||
|
{
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
let mut message = if let Some(service_tier) = service_tier {
|
let mut message = if let Some(service_tier) = service_tier {
|
||||||
format!("Service tier set to {service_tier}")
|
format!("Service tier set to {service_tier}")
|
||||||
@@ -1476,23 +1485,17 @@ impl App {
|
|||||||
self.sync_active_thread_permission_settings_to_cached_session()
|
self.sync_active_thread_permission_settings_to_cached_session()
|
||||||
.await;
|
.await;
|
||||||
let profile = self.active_profile.as_deref();
|
let profile = self.active_profile.as_deref();
|
||||||
let segments = if let Some(profile) = profile {
|
if let Err(err) = crate::config_update::write_config_batch(
|
||||||
vec![
|
app_server.request_handle(),
|
||||||
"profiles".to_string(),
|
vec![crate::config_update::replace_config_value(
|
||||||
profile.to_string(),
|
crate::config_update::profile_scoped_key_path(
|
||||||
"approvals_reviewer".to_string(),
|
profile,
|
||||||
]
|
"approvals_reviewer",
|
||||||
} else {
|
),
|
||||||
vec!["approvals_reviewer".to_string()]
|
serde_json::json!(policy.to_string()),
|
||||||
};
|
)],
|
||||||
if let Err(err) = ConfigEditsBuilder::for_config(&self.config)
|
)
|
||||||
.with_profile(profile)
|
.await
|
||||||
.with_edits([ConfigEdit::SetPath {
|
|
||||||
segments,
|
|
||||||
value: policy.to_string().into(),
|
|
||||||
}])
|
|
||||||
.apply()
|
|
||||||
.await
|
|
||||||
{
|
{
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
error = %err,
|
error = %err,
|
||||||
@@ -1583,27 +1586,23 @@ impl App {
|
|||||||
}
|
}
|
||||||
AppEvent::PersistPlanModeReasoningEffort(effort) => {
|
AppEvent::PersistPlanModeReasoningEffort(effort) => {
|
||||||
let profile = self.active_profile.as_deref();
|
let profile = self.active_profile.as_deref();
|
||||||
let segments = if let Some(profile) = profile {
|
let key_path = crate::config_update::profile_scoped_key_path(
|
||||||
vec![
|
profile,
|
||||||
"profiles".to_string(),
|
"plan_mode_reasoning_effort",
|
||||||
profile.to_string(),
|
);
|
||||||
"plan_mode_reasoning_effort".to_string(),
|
|
||||||
]
|
|
||||||
} else {
|
|
||||||
vec!["plan_mode_reasoning_effort".to_string()]
|
|
||||||
};
|
|
||||||
let edit = if let Some(effort) = effort {
|
let edit = if let Some(effort) = effort {
|
||||||
ConfigEdit::SetPath {
|
crate::config_update::replace_config_value(
|
||||||
segments,
|
key_path,
|
||||||
value: effort.to_string().into(),
|
serde_json::json!(effort.to_string()),
|
||||||
}
|
)
|
||||||
} else {
|
} else {
|
||||||
ConfigEdit::ClearPath { segments }
|
crate::config_update::clear_config_value(key_path)
|
||||||
};
|
};
|
||||||
if let Err(err) = ConfigEditsBuilder::for_config(&self.config)
|
if let Err(err) = crate::config_update::write_config_batch(
|
||||||
.with_edits([edit])
|
app_server.request_handle(),
|
||||||
.apply()
|
vec![edit],
|
||||||
.await
|
)
|
||||||
|
.await
|
||||||
{
|
{
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
error = %err,
|
error = %err,
|
||||||
|
|||||||
@@ -0,0 +1,124 @@
|
|||||||
|
//! App-server-backed config update helpers for the TUI.
|
||||||
|
//!
|
||||||
|
//! This module centralizes the small typed update helpers the TUI uses
|
||||||
|
//! when a config mutation must be owned by the app server rather than written
|
||||||
|
//! to the local `config.toml` directly.
|
||||||
|
|
||||||
|
use codex_app_server_client::AppServerRequestHandle;
|
||||||
|
use codex_app_server_protocol::ClientRequest;
|
||||||
|
use codex_app_server_protocol::ConfigBatchWriteParams;
|
||||||
|
use codex_app_server_protocol::ConfigEdit;
|
||||||
|
use codex_app_server_protocol::ConfigWriteResponse;
|
||||||
|
use codex_app_server_protocol::MergeStrategy;
|
||||||
|
use codex_app_server_protocol::RequestId;
|
||||||
|
use color_eyre::eyre::Result;
|
||||||
|
use color_eyre::eyre::WrapErr;
|
||||||
|
use serde_json::Value as JsonValue;
|
||||||
|
use uuid::Uuid;
|
||||||
|
|
||||||
|
pub(crate) fn replace_config_value(key_path: impl Into<String>, value: JsonValue) -> ConfigEdit {
|
||||||
|
ConfigEdit {
|
||||||
|
key_path: key_path.into(),
|
||||||
|
value,
|
||||||
|
merge_strategy: MergeStrategy::Replace,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(crate) fn clear_config_value(key_path: impl Into<String>) -> ConfigEdit {
|
||||||
|
replace_config_value(key_path, JsonValue::Null)
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(crate) fn profile_scoped_key_path(profile: Option<&str>, key_path: &str) -> String {
|
||||||
|
if let Some(profile) = profile {
|
||||||
|
let profile = serde_json::Value::String(profile.to_string()).to_string();
|
||||||
|
format!("profiles.{profile}.{key_path}")
|
||||||
|
} else {
|
||||||
|
key_path.to_string()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(crate) fn build_model_selection_edits(
|
||||||
|
profile: Option<&str>,
|
||||||
|
model: &str,
|
||||||
|
effort: Option<impl ToString>,
|
||||||
|
) -> Vec<ConfigEdit> {
|
||||||
|
let effort_edit = effort.map_or_else(
|
||||||
|
|| clear_config_value(profile_scoped_key_path(profile, "model_reasoning_effort")),
|
||||||
|
|effort| {
|
||||||
|
replace_config_value(
|
||||||
|
profile_scoped_key_path(profile, "model_reasoning_effort"),
|
||||||
|
serde_json::json!(effort.to_string()),
|
||||||
|
)
|
||||||
|
},
|
||||||
|
);
|
||||||
|
vec![
|
||||||
|
replace_config_value(
|
||||||
|
profile_scoped_key_path(profile, "model"),
|
||||||
|
serde_json::json!(model),
|
||||||
|
),
|
||||||
|
effort_edit,
|
||||||
|
]
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(crate) fn build_service_tier_selection_edits(
|
||||||
|
profile: Option<&str>,
|
||||||
|
service_tier: Option<&str>,
|
||||||
|
) -> Vec<ConfigEdit> {
|
||||||
|
let service_tier_edit = service_tier.map_or_else(
|
||||||
|
|| clear_config_value(profile_scoped_key_path(profile, "service_tier")),
|
||||||
|
|service_tier| {
|
||||||
|
let config_value =
|
||||||
|
match codex_protocol::config_types::ServiceTier::from_request_value(service_tier) {
|
||||||
|
Some(codex_protocol::config_types::ServiceTier::Fast) => "fast",
|
||||||
|
Some(codex_protocol::config_types::ServiceTier::Flex) => "flex",
|
||||||
|
None => service_tier,
|
||||||
|
};
|
||||||
|
replace_config_value(
|
||||||
|
profile_scoped_key_path(profile, "service_tier"),
|
||||||
|
serde_json::json!(config_value),
|
||||||
|
)
|
||||||
|
},
|
||||||
|
);
|
||||||
|
let mut edits = vec![service_tier_edit];
|
||||||
|
if service_tier.is_none() {
|
||||||
|
edits.push(replace_config_value(
|
||||||
|
"notice.fast_default_opt_out",
|
||||||
|
serde_json::json!(true),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
edits
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(crate) async fn write_config_batch(
|
||||||
|
request_handle: AppServerRequestHandle,
|
||||||
|
edits: Vec<ConfigEdit>,
|
||||||
|
) -> Result<()> {
|
||||||
|
let request_id = RequestId::String(format!("tui-config-write-{}", Uuid::new_v4()));
|
||||||
|
let _: ConfigWriteResponse = request_handle
|
||||||
|
.request_typed(ClientRequest::ConfigBatchWrite {
|
||||||
|
request_id,
|
||||||
|
params: ConfigBatchWriteParams {
|
||||||
|
edits,
|
||||||
|
file_path: None,
|
||||||
|
expected_version: None,
|
||||||
|
reload_user_config: true,
|
||||||
|
},
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.wrap_err("config/batchWrite failed in TUI")?;
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::*;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn profile_scoped_key_path_quotes_dotted_profile_names() {
|
||||||
|
assert_eq!(
|
||||||
|
profile_scoped_key_path(Some("team.prod"), "model"),
|
||||||
|
"profiles.\"team.prod\".model"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -112,6 +112,7 @@ mod clipboard_copy;
|
|||||||
mod clipboard_paste;
|
mod clipboard_paste;
|
||||||
mod collaboration_modes;
|
mod collaboration_modes;
|
||||||
mod color;
|
mod color;
|
||||||
|
mod config_update;
|
||||||
pub(crate) mod custom_terminal;
|
pub(crate) mod custom_terminal;
|
||||||
mod pets;
|
mod pets;
|
||||||
pub use custom_terminal::Terminal;
|
pub use custom_terminal::Terminal;
|
||||||
|
|||||||
Reference in new issue
Block a user