fix(tui): support legacy remote exec approvals#15063
fix(tui): support legacy remote exec approvals#15063fcoury wants to merge 11 commits intoopenai:mainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds backwards-compatibility so the TUI can interoperate with remote servers still sending the legacy ExecCommandApproval request shape, while continuing to use the existing exec-approval UI and returning responses in the matching wire format.
Changes:
- Track pending exec approvals with a response “kind” (legacy v1 vs v2) and serialize the appropriate approval response payload.
- Bridge remote legacy
ExecCommandApprovalrequests intoExecApprovalRequestthread events so the existing approval overlay can render them. - Remove already-resolved exec approvals from the live modal queue and from replay buffers when the server reports a request as resolved.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| codex-rs/tui_app_server/src/chatwidget.rs | Adds a ChatWidget entry point to remove resolved exec approvals and trigger redraw. |
| codex-rs/tui_app_server/src/bottom_pane/mod.rs | Adds propagation/removal of resolved exec approvals from the active bottom-pane view; includes unit test. |
| codex-rs/tui_app_server/src/bottom_pane/bottom_pane_view.rs | Extends the BottomPaneView trait with a hook to remove resolved exec approvals. |
| codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs | Implements pruning resolved exec approvals from the approval overlay queue/current item. |
| codex-rs/tui_app_server/src/app/pending_interactive_replay.rs | Tracks server-resolved exec approvals to prevent replay on thread switches; includes unit test. |
| codex-rs/tui_app_server/src/app/app_server_requests.rs | Stores pending exec approvals with response kind (legacy vs v2) and returns resolved exec approval IDs on notification resolution; adds tests. |
| codex-rs/tui_app_server/src/app/app_server_adapter.rs | Adds bridging for remote legacy exec-approval requests into thread events; updates resolution handling. |
| codex-rs/tui_app_server/src/app.rs | Removes resolved exec approvals from buffered primary events, live UI modals, and replay state; adds async test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb68c4e203
ℹ️ 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
force-pushed
the
fcoury/fix/remote-legacy-exec-approval
branch
from
fb68c4e to
8a3c796
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca09ea47c9
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89efa807e3
ℹ️ 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
force-pushed
the
fcoury/fix/remote-legacy-exec-approval
branch
from
61366f4 to
89efa80
Compare
|
@codex review |
Accept the older remote `ExecCommandApproval` request shape, surface it through the existing exec approval UI, and remember which wire format to use when the user responds. Older remote servers can still rely on the legacy approval mechanism, so rejecting that request shape left the agent blocked even though the TUI already had the right approval experience.
Handle legacy remote exec approvals as fully tracked requests so they clean up correctly when the server resolves them or the UI cannot surface them. This also adds regression coverage for approval-id fallback paths and strengthens the bridge test to assert the mapped approval payload.
Remove buffered legacy exec approval prompts when the app-server resolves those requests before the primary session is configured. This prevents stale approvals from replaying out of `pending_primary_events` and adds regression coverage for the pre-session resolution ordering.
Remove resolved legacy exec approvals from the live modal queue in `tui_app_server` so `serverRequest/resolved` cannot leave a stale, clickable approval prompt behind. This threads resolved approval ids through `App`, `ChatWidget`, and the bottom pane overlay, and adds regression coverage for removing queued and currently visible exec approval modals.
Adapt the rebased legacy exec approval bridge to the current `tui_app_server` buffering model instead of replaying an older `app.rs` shape onto `upstream/main`. This keeps the PR surface focused while preserving the intended behavior: legacy approvals still bridge into the UI, resolved approvals clear buffered and live state, and the rebased branch matches the current upstream request-routing paths.
Remove resolved exec approval requests from the active thread queue in `tui_app_server` so fast `serverRequest/resolved` notifications cannot replay a dead approval prompt from `active_thread_rx`. This keeps the live queue consistent with the buffered, replay, and modal cleanup paths and adds regression coverage for the active-thread race.
Rename the inherent resolved-approval cleanup helper in `ApprovalOverlay` so the `BottomPaneView` trait implementation does not recurse back into itself. This preserves the resolved-approval modal cleanup behavior while removing the stack-overflow path flagged in review.
Use the existing argv escaping helper when bridging legacy exec approval requests so approval prompts keep shell quoting for arguments with spaces or special characters. This also prunes resolved exec approvals across the full bottom-pane modal stack, preventing hidden overlays from resurfacing stale prompts after the server has already resolved them.
Restore modal teardown side effects when removing a resolved exec approval from the active bottom-pane overlay. This resumes the paused status timer when the active approval view is cleared by `serverRequest/resolved`, while leaving hidden overlay pruning behavior unchanged. Add a regression test that exercises the active-view removal path directly.
Restore `HistoryEntryResponse` buffering on the rebased app-server thread pipeline so history lookups still replay and route correctly after moving onto `upstream/main`. Extract the remote legacy exec-approval bridge into a dedicated helper in `app_server_adapter.rs` so the special-case path is isolated from normal server request handling.
fcoury
force-pushed
the
fcoury/fix/remote-legacy-exec-approval
branch
from
20358f7 to
bbbaa81
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbbaa818a0
ℹ️ 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".
Treat legacy `ExecCommandApproval` requests as a compatibility path regardless of transport so older request shapes continue to surface in the existing exec approval UI instead of failing on session type. Restore `active_thread_rx` before reinserting retained thread events and use non-blocking refill logic so resolved-approval cleanup cannot deadlock while the receiver is detached.
fcoury
force-pushed
the
fcoury/fix/remote-legacy-exec-approval
branch
from
af568a7 to
ba3c1fe
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba3c1feeca
ℹ️ 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".
| let Some(thread_id) = self.active_thread_id else { | ||
| return; | ||
| }; | ||
| let Some(mut rx) = self.active_thread_rx.take() else { | ||
| return; |
There was a problem hiding this comment.
remove_resolved_exec_approvals_from_active_queue only filters self.active_thread_rx, but switched-away threads keep their own receiver in ThreadEventChannel (see store_active_thread_receiver/select_agent_thread). If an exec approval is queued, the user switches threads, and the server resolves that request while the thread is inactive, the stale CommandExecutionRequestApproval remains in that inactive receiver and is processed when the thread is reselected, resurfacing an already-resolved approval prompt and potentially re-blocking interaction.
Useful? React with 👍 / 👎.
Summary
Add compatibility for the older
ExecCommandApprovalrequest shape.Problem
Some app-server sessions still use the legacy
ExecCommandApprovalrequest as the way to ask for command approval. The TUI rejected that request as unsupported, so the agent remained blocked even though the UI already had the right approval flow.What this change does
ExecCommandApprovalrequestsResult
Sessions using the older exec approval protocol can interoperate correctly with the TUI's exec approval flow, including approval display, resolution, and cleanup, without falling back to an unsupported-request failure.