Remove TUI exec-policy core exports - #31179
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be2a538083
ℹ️ 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: 887d2df706
ℹ️ 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".
fcoury-oai
left a comment
There was a problem hiding this comment.
I manually ran smoke-tested te TUI startup with a malformed exec-policy rules file and confirmed Codex displays the expected warning and remains usable instead of exiting during startup.
Also left two issues inline found by Codex.
| /// the runtime is shut down and an `InvalidData` error is returned. | ||
| pub async fn start(args: InProcessStartArgs) -> IoResult<InProcessClientHandle> { | ||
| pub async fn start(mut args: InProcessStartArgs) -> IoResult<InProcessClientHandle> { | ||
| if let Ok(Some(err)) = check_execpolicy_for_warnings(&args.config.config_layer_stack).await { |
There was a problem hiding this comment.
[P1] A parse warning here is unsafe when managed requirements().exec_policy is present. The later thread load installs Policy::empty() before the requirements overlay can be merged, changing the embedded TUI from fail-closed to running without managed prompt/forbidden rules. Please preserve the requirements overlay when dropping malformed custom rules, or keep this case fatal.
There was a problem hiding this comment.
This is addressed separately in #31188. It's an existing issue that affects existing clients like the app, so it should be addressed in the app server layer or core.
| /// the runtime is shut down and an `InvalidData` error is returned. | ||
| pub async fn start(args: InProcessStartArgs) -> IoResult<InProcessClientHandle> { | ||
| pub async fn start(mut args: InProcessStartArgs) -> IoResult<InProcessClientHandle> { | ||
| if let Ok(Some(err)) = check_execpolicy_for_warnings(&args.config.config_layer_stack).await { |
There was a problem hiding this comment.
[P2] The reusable local-daemon path still lacks fresh validation. Its startup warnings are frozen, while each new thread reloads config and rereads .rules. If rules become malformed after daemon startup or come from another project, the thread silently receives an empty policy. Please validate per thread/initialize or otherwise surface a fresh warning.
There was a problem hiding this comment.
This is similarly an existing bug in the app server layer that affects other clients. It should be addressed in the app server or core layer. This PR exposes the problem for the TUI. I'll work on a separate PR to address this.
fcoury-oai
left a comment
There was a problem hiding this comment.
Approved after understanding how the findings will be handled.
## Why App-server initialization captures exec-policy parse warnings only once. Each `thread/start` then reloads config for that thread's cwd and rereads its `.rules` files, so rules that become malformed after initialization—or belong to a different project—are dropped without a fresh warning to the requesting client. ## What changed - Validate exec-policy rules against the freshly loaded per-thread config during `thread/start`. - Send the existing structured `configWarning` only to the connection that requested the thread. - Preserve nonfatal startup and avoid repeating an identical warning already delivered during initialization. This is intentionally separate from the TUI `legacy_core` cleanup in #31179 and the managed-requirements fallback fix in #31188.
Why
The TUI still reached through
codex_app_server_client::legacy_coreto validate exec-policy rules during local startup. The app server already owns this validation and reports parse failures throughconfigWarning, so the duplicate TUI preflight preserved an unnecessary core dependency and could inspect the wrong machine when connected to a remote app server.What changed
legacy_core.