[3 of 4] tui: route feature and memory toggles through app server#22915
Conversation
12fe1d5 to
15dc6d3
Compare
ff17872 to
6a181fa
Compare
d8375c8 to
d5f9063
Compare
## 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](#22913) `[1 of 4]` primary settings writes 2. [#22914](#22914) `[2 of 4]` app and skill enablement 3. [#22915](#22915) `[3 of 4]` feature and memory toggles 4. [#22916](#22916) `[4 of 4]` startup and onboarding bookkeeping
df4e122 to
fd994c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b37f46d600
ℹ️ 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 App and skill toggles are user config mutations too. When the TUI is attached to a remote app server, writing those toggles into the local `config.toml` makes the UI report success without updating the server that actually owns the session. This is **[2 of 4]** in a stacked series that moves TUI-owned config mutations onto app-server APIs. ## What changed - Routed app enable/disable persistence through app-server config batch writes. - Routed skill enable/disable persistence through `skills/config/write`. - Avoided refreshing local config from disk after these writes when the TUI is connected to a remote app server. ## Config keys affected - `apps.<app_id>.enabled` - `apps.<app_id>.disabled_reason` - `[[skills.config]]` entries keyed by `path`, with `enabled = false` used for persisted disables ## Suggested manual validation - Connect the TUI to a remote app server, disable an app, reconnect, and confirm the app remains disabled from remote config rather than local disk state. - Re-enable the same app and confirm both `apps.<app_id>.enabled` and `apps.<app_id>.disabled_reason` are cleared remotely. - Disable a skill in the manage-skills UI and confirm a remote `[[skills.config]]` disable entry appears. - Re-enable that skill and confirm the disable entry is removed and the effective enabled state updates without relying on local config reloads. ## Stack 1. [#22913](#22913) `[1 of 4]` primary settings writes 2. [#22914](#22914) `[2 of 4]` app and skill enablement 3. [#22915](#22915) `[3 of 4]` feature and memory toggles 4. [#22916](#22916) `[4 of 4]` startup and onboarding bookkeeping
f0e5a0e to
61beab3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61beab30bd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7513a9ceb7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 752339266a
ℹ️ 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".
…-server-3-feature-toggles # Conflicts: # codex-rs/tui/src/app/config_persistence.rs # codex-rs/tui/src/config_update.rs
canvrno-oai
left a comment
There was a problem hiding this comment.
I tasked Codex with testing this on a remote app-server, and it did encounter one issue that seems legitimate:
While testing this manually against a remote app-server, I found one remaining override edge case: the
OkOverriddenpath resyncs the effectiveguardian_approvalbit, but then still appliesapprovals_reviewer/approval_policyfrom the effective config unconditionally.If a higher-priority layer forces
guardian_approval = falsewhile companion reviewer/policy writes remain effective, the TUI can show Auto-review disabled while approvals are still routed to the reviewer.
Other that that, everything looks good
Why
Experimental feature toggles and memory settings can update several related config values in one interaction. Keeping those writes local in a remote TUI session is especially dangerous because the UI can diverge from the app-server config while also leaving behind partially stale supporting keys.
This is [3 of 4] in a stacked series that moves TUI-owned config mutations onto app-server APIs.
What changed
Config keys affected
features.<feature_key>profiles.<profile>.features.<feature_key>approval_policysandbox_modeapprovals_reviewerwindows.sandboxfeatures.experimental_windows_sandboxfeatures.elevated_windows_sandboxfeatures.enable_experimental_windows_sandboxprofiles.<profile>.features.*memories.use_memoriesmemories.generate_memoriesprofiles.<profile>.memories.*Suggested manual validation
features.guardian_approval, reviewer state, approval policy, and sandbox mode coherently.false.windows.sandboxis updated while the legacy Windows feature keys are cleared.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