[1 of 4] tui: route primary settings writes through app server#22913
Conversation
fcoury-oai
left a comment
There was a problem hiding this comment.
Check the issue Codex found. I was able to replicate it.
|
|
||
| pub(crate) fn profile_scoped_key_path(profile: Option<&str>, key_path: &str) -> String { | ||
| if let Some(profile) = profile { | ||
| format!("profiles.{profile}.{key_path}") |
There was a problem hiding this comment.
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:
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:
- Replace
parse_key_pathwith a small TOML dotted-key parser that supports quoted segments. - Emit quoted profile segments from
profile_scoped_key_path, e.g.profiles."team.prod".model. - Add a regression test where both
[profiles."team.prod"]and[profiles.team.prod]exist, then assertconfig/batchWriteupdates only the quoted active profile.
That keeps this PR’s app-server write direction while matching the profile resolution behavior used at startup.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b74ce8d478
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
config/batchWrite.profiles.<profile>.*overrides.Config keys affected
modelmodel_reasoning_effortpersonalityservice_tierplan_mode_reasoning_effortapprovals_reviewernotice.fast_default_opt_outprofiles.<profile>.*Suggested manual validation
modelandmodel_reasoning_effort, reconnect, and confirm the remote config retained both values while the localconfig.tomldid not change.personality,plan_mode_reasoning_effort, and the explicit auto-review selection, then reconnect and confirm those choices persist through the app server.service_tieris cleared whilenotice.fast_default_opt_out = trueis persisted remotely.profiles.<profile>.*.Stack
[1 of 4]primary settings writes[2 of 4]app and skill enablement[3 of 4]feature and memory toggles[4 of 4]startup and onboarding bookkeeping