Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions codex-rs/app-server/src/config_manager_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,10 +403,47 @@ fn parse_key_path(path: &str) -> Result<Vec<String>, String> {
if path.trim().is_empty() {
return Err("keyPath must not be empty".to_string());
}
Ok(path
.split('.')
.map(std::string::ToString::to_string)
.collect())

let mut segments = Vec::new();
let mut segment = String::new();
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)]
Expand Down
65 changes: 65 additions & 0 deletions codex-rs/app-server/tests/suite/v2/config_rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,71 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> {
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)]
async fn config_batch_write_updates_multiple_desktop_settings() -> Result<()> {
let tmp_dir = TempDir::new()?;
Expand Down
99 changes: 49 additions & 50 deletions codex-rs/tui/src/app/event_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1182,11 +1182,15 @@ impl App {
}
AppEvent::PersistModelSelection { model, effort } => {
let profile = self.active_profile.as_deref();
match ConfigEditsBuilder::for_config(&self.config)
.with_profile(profile)
.set_model(Some(model.as_str()), effort)
.apply()
.await
match crate::config_update::write_config_batch(
app_server.request_handle(),
crate::config_update::build_model_selection_edits(
profile,
model.as_str(),
effort,
),
)
.await
{
Ok(()) => {
let effort_label = effort
Expand Down Expand Up @@ -1260,11 +1264,14 @@ impl App {
}
AppEvent::PersistPersonalitySelection { personality } => {
let profile = self.active_profile.as_deref();
match ConfigEditsBuilder::for_config(&self.config)
.with_profile(profile)
.set_personality(Some(personality))
.apply()
.await
match crate::config_update::write_config_batch(
app_server.request_handle(),
vec![crate::config_update::replace_config_value(
crate::config_update::profile_scoped_key_path(profile, "personality"),
serde_json::json!(personality.to_string()),
)],
)
.await
{
Ok(()) => {
let label = Self::personality_label(personality);
Expand Down Expand Up @@ -1297,14 +1304,16 @@ impl App {
self.refresh_status_line();
let profile = self.active_profile.as_deref();
self.config.service_tier = service_tier.clone();
let mut edits = ConfigEditsBuilder::for_config(&self.config)
.with_profile(profile)
.set_service_tier(service_tier.clone());
let edits = crate::config_update::build_service_tier_selection_edits(
profile,
service_tier.as_deref(),
);
if service_tier.is_none() {
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(()) => {
let mut message = if let Some(service_tier) = service_tier {
format!("Service tier set to {service_tier}")
Expand Down Expand Up @@ -1468,23 +1477,17 @@ impl App {
self.sync_active_thread_permission_settings_to_cached_session()
.await;
let profile = self.active_profile.as_deref();
let segments = if let Some(profile) = profile {
vec![
"profiles".to_string(),
profile.to_string(),
"approvals_reviewer".to_string(),
]
} else {
vec!["approvals_reviewer".to_string()]
};
if let Err(err) = ConfigEditsBuilder::for_config(&self.config)
.with_profile(profile)
.with_edits([ConfigEdit::SetPath {
segments,
value: policy.to_string().into(),
}])
.apply()
.await
if let Err(err) = crate::config_update::write_config_batch(
app_server.request_handle(),
vec![crate::config_update::replace_config_value(
crate::config_update::profile_scoped_key_path(
profile,
"approvals_reviewer",
),
serde_json::json!(policy.to_string()),
)],
)
.await
{
tracing::error!(
error = %err,
Expand Down Expand Up @@ -1575,27 +1578,23 @@ impl App {
}
AppEvent::PersistPlanModeReasoningEffort(effort) => {
let profile = self.active_profile.as_deref();
let segments = if let Some(profile) = profile {
vec![
"profiles".to_string(),
profile.to_string(),
"plan_mode_reasoning_effort".to_string(),
]
} else {
vec!["plan_mode_reasoning_effort".to_string()]
};
let key_path = crate::config_update::profile_scoped_key_path(
profile,
"plan_mode_reasoning_effort",
);
let edit = if let Some(effort) = effort {
ConfigEdit::SetPath {
segments,
value: effort.to_string().into(),
}
crate::config_update::replace_config_value(
key_path,
serde_json::json!(effort.to_string()),
)
} else {
ConfigEdit::ClearPath { segments }
crate::config_update::clear_config_value(key_path)
};
if let Err(err) = ConfigEditsBuilder::for_config(&self.config)
.with_edits([edit])
.apply()
.await
if let Err(err) = crate::config_update::write_config_batch(
app_server.request_handle(),
vec![edit],
)
.await
{
tracing::error!(
error = %err,
Expand Down
124 changes: 124 additions & 0 deletions codex-rs/tui/src/config_update.rs
Original file line number Diff line number Diff line change
@@ -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}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a problem here with the help of Codex. When you write a profile with "a.b" notation, it will write to config.toml as:

[profiles.sample.profile]
model = "..."

The correct way is:

[profiles."sample.profile"]
model = "...""

I created a profile like this:

profile = "team.prod"

[profiles."team.prod"]
model = "gpt-5.3-spark"
model_reasoning_effort = "medium"

And when I changed the model in the TUI I got:

[profiles.team.prod]
model = "gpt-5.5"
model_reasoning_effort = "xhigh"

And sure enough it uses the version with quotes when I start codex:

Image

Suggested fix: reuse the existing segment-based config edit model instead of building profile paths with raw dot concatenation. Core already preserves profile names as one segment via ConfigEdit::SetPath { segments } / ConfigEditsBuilder::with_profile; the lossy part here is the app-server key_path string being parsed with split('.').

A compatible fix would be:

  1. Replace parse_key_path with a small TOML dotted-key parser that supports quoted segments.
  2. Emit quoted profile segments from profile_scoped_key_path, e.g. profiles."team.prod".model.
  3. Add a regression test where both [profiles."team.prod"] and [profiles.team.prod] exist, then assert config/batchWrite updates only the quoted active profile.

That keeps this PR’s app-server write direction while matching the profile resolution behavior used at startup.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find. This appears to be an existing bug in the app server layer. I'm going to explore whether it makes sense to fix as part of this PR or whether we should get a fix in place prior to merging this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to fix the app server bug with a relatively surgical fix (plus some regression tests), so I decided to include it as part of this PR.

} 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"
);
}
}
1 change: 1 addition & 0 deletions codex-rs/tui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ mod clipboard_copy;
mod clipboard_paste;
mod collaboration_modes;
mod color;
mod config_update;
pub(crate) mod custom_terminal;
mod pets;
pub use custom_terminal::Terminal;
Expand Down
Loading