refactor(tui): move skills loading onto the app-server by fcoury · Pull Request #14615 · openai/codex

fcoury · 2026-03-13T18:11:22Z

Problem

The TUI was still loading skills through a TUI/core-specific path even though the app-server already owns the skills API surface used by other clients. That left the TUI with duplicated request plumbing, duplicated event handling, and a separate in-memory skills model that had to be kept in sync with app-server behavior.

What this change does

This PR moves TUI skills loading onto the app-server path and makes the TUI consume app-server protocol types directly.

Concretely:

skills/list requests are issued as app-server RPCs from App
SkillsListLoaded now carries codex_app_server_protocol::SkillsListResponse
the TUI stores and renders codex_app_server_protocol::SkillMetadata directly instead of adapting it into legacy core/protocol skill structs
the old app-server-to-core conversion layer for skills is removed
live skills refresh in the TUI is now driven by app-server ServerNotification::SkillsChanged, not the legacy EventMsg::SkillsUpdateAvailable path
session logging now summarizes SkillsListLoaded instead of dumping the full skills payload

Mental model

There is now one TUI skills-loading path:

ChatWidget requests Op::ListSkills
App::run intercepts that op and calls the app-server skills/list RPC
the RPC result is returned as AppEvent::SkillsListLoaded
App applies the response to the chat widget and refreshes plugin mentions

For on-disk changes:

core’s file watcher still detects skill-file changes internally
app-server translates that into ServerNotification::SkillsChanged
the TUI app-server adapter turns that notification into AppEvent::RefreshSkillsList
App reissues skills/list

Non-goals

Routing unrelated TUI operations through the app-server in this PR
Reworking core’s internal file-watcher ownership
Changing the app-server protocol shape to match legacy TUI/core types

Tradeoffs

The TUI still has a small transitional app-server adapter module to translate app-server notifications into app events.
Skills refresh on disk still originates from core internally, but the TUI no longer depends on the legacy core event to consume it.
per_cwd_extra_user_roots remains None; this PR does not add per-CWD extra-root support.

Files of interest

codex-rs/tui/src/app.rs
codex-rs/tui/src/app/skills.rs
codex-rs/tui/src/app/app_server_adapter.rs
codex-rs/tui/src/app_event.rs
codex-rs/tui/src/chatwidget.rs
codex-rs/tui/src/chatwidget/skills.rs
codex-rs/tui/src/session_log.rs
codex-rs/app-server-client/src/lib.rs

Tests

Updated and added coverage includes:

app_server_skills_changed_notification_emits_refresh_skills_list
trigger_skills_list_request_uses_current_cwd_and_emits_result
effective_skills_list_cwds_uses_active_chat_widget_cwd_when_request_empty
effective_skills_list_cwds_preserves_explicit_cwds
stale-response tests for SkillsListLoaded
generated_id_typed_request_roundtrip_works in codex-app-server-client

Observability

CODEX_TUI_RECORD_SESSION no longer logs the full skills payload for SkillsListLoaded; it records a compact summary instead.

Copilot

Pull request overview

This PR begins migrating the TUI to use the embedded app-server as a backend (gated via --app-server for the first migrated operation: ListSkills), and adds supporting in-process client/request plumbing plus adapter/event-loop wiring.

Changes:

Added --app-server CLI flag and routed ListSkills through the App event loop to an embedded app-server RPC when enabled.
Introduced an embedded app-server startup path in the TUI and an adapter to drain/handle app-server events during the hybrid migration.
Extended codex-app-server-client with a cloneable requester + typed request helpers and shared core manager accessors for migration bootstrapping.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
codex-rs/tui/src/main.rs	Updates `run_main` invocation to pass loader overrides.
codex-rs/tui/src/lib.rs	Adds embedded app-server startup helpers and threads new args through app startup.
codex-rs/tui/src/cli.rs	Adds `--app-server` flag plus parsing test.
codex-rs/tui/src/chatwidget/tests.rs	Adds routing tests ensuring `ListSkills` uses correct channel based on flag.
codex-rs/tui/src/chatwidget/skills.rs	Adds test-only helper to inspect loaded skills.
codex-rs/tui/src/chatwidget.rs	Adds `use_app_server` plumbing and routes `ListSkills` via `request_skills_list`.
codex-rs/tui/src/app_event.rs	Adds `SkillsListLoaded` event for app-server-proxied skills responses.
codex-rs/tui/src/app/skills.rs	Implements app-server `skills/list` RPC + protocol-to-core conversions.
codex-rs/tui/src/app/app_server_adapter.rs	Adds temporary adapter to drain app-server event stream and reject server requests.
codex-rs/tui/src/app.rs	Wires select-loop interception for app-server skills path; adds generation tracking + tests.
codex-rs/tui/Cargo.toml	Adds dependency on `codex-app-server-client`.
codex-rs/core/src/auth.rs	Adds `clear_external_auth_refresher` to break retained runtime references on shutdown.
codex-rs/cli/src/main.rs	Updates call into TUI `run_main` signature.
codex-rs/app-server/src/message_processor/tracing_tests.rs	Updates tests for new optional manager args.
codex-rs/app-server/src/message_processor.rs	Allows reusing provided auth/thread managers; adds runtime reference clearing.
codex-rs/app-server/src/lib.rs	Threads new optional manager args into `MessageProcessorArgs`.
codex-rs/app-server/src/in_process.rs	Threads optional manager args; clears runtime references during shutdown.
codex-rs/app-server-client/src/lib.rs	Adds shared-manager retention, requester handle, and typed-request helpers.
codex-rs/Cargo.lock	Locks new dependency inclusion.

Comments suppressed due to low confidence (1)

codex-rs/tui/src/lib.rs:1021

start_embedded_app_server(..) is invoked unconditionally and any startup error aborts the TUI (even when --app-server is false). That makes the legacy (direct codex-core op channel) path dependent on app-server startup and can break the default flow if the embedded server fails to start. Consider only starting the embedded app-server when cli.app_server is true, or falling back to the legacy core-only managers when --app-server is false (and only treat startup failure as fatal when the flag is enabled).

    let embedded_app_server = match start_embedded_app_server(
        arg0_paths,
        config.clone(),
        cli_kv_overrides.clone(),
        loader_overrides,
        cloud_requirements.clone(),
        feedback.clone(),
    )
    .await
    {
        Ok(app_server) => app_server,
        Err(err) => {
            restore();
            session_log::log_session_end();
            return Err(err);
        }
    };

💡 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.

codex-rs/tui/src/app.rs

etraut-openai

First round of review comments

codex-rs/tui/src/app/skills.rs

codex-rs/tui/src/chatwidget/skills.rs

codex-rs/tui/src/chatwidget/tests.rs

codex-rs/tui/src/chatwidget.rs

codex-rs/tui/src/lib.rs

fcoury · 2026-03-13T21:38:43Z

@codex review

chatgpt-codex-connector · 2026-03-13T21:53:05Z

Codex Review: Didn't find any major issues. Nice work!

ℹ️ 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 · 2026-03-13T22:22:45Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 998997f31a

ℹ️ 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-rs/tui/src/app_event.rs

fcoury · 2026-03-13T23:54:00Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0020a26474

ℹ️ 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".

chatgpt-codex-connector · 2026-03-14T00:02:42Z

codex-rs/tui/src/app.rs

+        generation: u64,
+        result: std::result::Result<SkillsListResponse, String>,
+    ) -> Result<()> {
+        let event = result.map_err(|err| color_eyre::eyre::eyre!(err))?;


Check staleness before failing skills/list errors

handle_skills_list_loaded unwraps result immediately, so any failed skills/list response returns an error before the requested_cwds/generation stale-response checks run. In practice, a delayed failure from an older request (for example after a cwd switch or overlapping refresh) will still bubble out of handle_event and tear down the app loop, even though stale successes are intentionally ignored; the stale guards should be applied before treating the response as fatal.

Useful? React with 👍 / 👎.

Add a TUI `--app-server` flag and use it to route `ListSkills` through the embedded app-server while leaving the existing core path unchanged when the flag is off. Keep the change surface small by preserving the base `AppEvent` model, intercepting only `AppEvent::CodexOp(Op::ListSkills { .. })` in the app loop, and normalizing the app-server response back into the existing skills UI update path.

Clarify the intent behind the --app-server flag, the type conversion helpers, and the dual-channel routing in request_skills_list.

Route app-server `skills/list` refreshes through a detached request handle so the main TUI event loop stays responsive while scans run. The completed result is fed back through an app event, preserving the existing UI update flow. Expand empty skills cwd requests against the active chat session cwd instead of the embedded app-server startup cwd. This keeps skill discovery aligned with the visible thread when users switch to sessions rooted in other directories.

Restore the CLI `app_server` flag flow after the rebase by passing the embedded app-server client and the boolean flag as distinct values into `App::run`. Drop stale imports from `tui/src/app.rs` now that app-server event handling lives in the adapter module. This keeps the rebased branch building cleanly.

Generate a fresh app-server request ID for each detached `skills/list` RPC issued by the TUI when `--app-server` is enabled. This avoids duplicate in-flight JSON-RPC request IDs during overlapping skills refreshes, which could otherwise cause the embedded app-server to reject a request and surface an avoidable TUI error.

Carry the requested cwd set through `SkillsListLoaded` and only apply a `skills/list` response when it still matches the active chat cwd. This prevents a delayed app-server response for an old session cwd from clearing or replacing the visible skills after the user switches threads. Add a regression test covering the stale-response case.

Track a per-cwd `skills/list` generation in the TUI and carry that context through `SkillsListLoaded` completions from the embedded app-server path. This prevents delayed same-cwd or old-cwd responses from overwriting newer skill state after overlapping refreshes, and adds regressions for both stale-response cases.

Move the temporary app-server `skills/list` request and response adaptation helpers out of `tui/src/app.rs` into `tui/src/app/skills.rs`. This keeps the main app loop focused on orchestration, makes the hybrid migration code easier to isolate, and removes a stale `ListSkills`-specific comment from the `use_app_server` flag docs.

Drop an unnecessary `current_cwd.clone()` from the stale skills response regression test in `tui/src/app.rs`. This matches Clippy's `redundant_clone` lint on CI and keeps the branch green without changing runtime behavior.

Route TUI skill loading and skill UI state through `codex_app_server_protocol` types instead of adapting app-server responses back into the legacy core/protocol skill structs. This removes the extra conversion layer, aligns the runtime and test fixtures with the app-server skills flow, and simplifies the remaining skills handling in the TUI.

Route TUI skills loading, storage, and mention handling through `codex_app_server_protocol` types instead of adapting app-server responses back into legacy core/protocol skill structs. This removes the extra conversion layer, cleans up stale split-path skills tests, and aligns app-server naming with the unconditional skills routing in the TUI.

Add `request_typed_with_generated_id` on `InProcessAppServerRequester` so the TUI skills path no longer owns transport-level JSON-RPC request ID allocation. This keeps `tui/src/app/skills.rs` focused on skills behavior, aligns the request plumbing with the app-server client boundary, and preserves the recent `app_server` naming cleanup in the TUI tests and startup helpers.

…nerated_id

Handle app-server `SkillsChanged` notifications in the TUI and route them through `AppEvent::RefreshSkillsList` instead of relying on the legacy core `SkillsUpdateAvailable` event path. This keeps skills reload orchestration in `App`, removes the stale chatwidget routing test, and adds app-level coverage for the new notification-driven flow.

Add a dedicated `SkillsListLoaded` logging path in `tui/src/session_log.rs` so session recording does not serialize the entire skills response payload on each refresh. This keeps `CODEX_TUI_RECORD_SESSION` logs compact by recording only summary fields for skills reload events.

Replace the single-pattern `match` in the app-server adapter with `if let` so the skills-changed notification path conforms to the workspace clippy settings. This keeps the notification handling behavior unchanged while avoiding a local lint allow for a temporary shape.

Update the remaining `SessionConfiguredEvent` test fixtures in `codex-rs/tui/src/app.rs` to include `approvals_reviewer`. This matches the rebased `upstream/main` protocol shape and clears the post-rebase `cargo test -p codex-tui` failure seen in CI.

Check `skills/list` response staleness before treating a failed response as fatal in `App::handle_skills_list_loaded`. This keeps delayed failures from older generations or cwd requests from tearing down the app loop, and adds regression coverage for the stale error case.

Copilot AI review requested due to automatic review settings March 13, 2026 18:11

Copilot started reviewing on behalf of fcoury March 13, 2026 18:11 View session

fcoury force-pushed the feat/tui-gated-app-server branch from 8c47bb3 to ea5c62d Compare March 13, 2026 18:15

Copilot AI reviewed Mar 13, 2026

View reviewed changes

codex-rs/tui/src/app.rs Outdated Show resolved Hide resolved

etraut-openai reviewed Mar 13, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 13, 2026

View reviewed changes

codex-rs/tui/src/app_event.rs Show resolved Hide resolved

fcoury changed the title ~~feat(tui): start wiring app-server backend behind --app-server~~ refactor(tui): move skills loading onto the app-server Mar 13, 2026

chatgpt-codex-connector bot reviewed Mar 14, 2026

View reviewed changes

fcoury force-pushed the feat/tui-gated-app-server branch from 0020a26 to fb6cea0 Compare March 14, 2026 00:09

fcoury added 17 commits March 14, 2026 18:17

docs: add doc comments for app-server gating logic

457cd54

Clarify the intent behind the --app-server flag, the type conversion helpers, and the dual-channel routing in request_skills_list.

fix(tui): remove redundant clone in skills test

f815d5e

Drop an unnecessary `current_cwd.clone()` from the stale skills response regression test in `tui/src/app.rs`. This matches Clippy's `redundant_clone` lint on CI and keeps the branch green without changing runtime behavior.

feat(tui): removed --app-server TUI flag

081e49f

test(app-server-client): add roundtrip test for request_typed_with_ge…

2a91afe

…nerated_id

fcoury added 2 commits March 14, 2026 18:17

fcoury force-pushed the feat/tui-gated-app-server branch from 18ed29b to ce5dfe3 Compare March 14, 2026 21:18

github-actions bot mentioned this pull request Mar 15, 2026

📊 Bản tin hàng ngày công cụ AI CLI 2026-03-15 compasify/agents-radar#45

Open

Conversation

fcoury commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

What this change does

Mental model

Non-goals

Tradeoffs

Files of interest

Tests

Observability

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

etraut-openai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fcoury commented Mar 13, 2026

Uh oh!

chatgpt-codex-connector bot commented Mar 13, 2026

Uh oh!

fcoury commented Mar 13, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

fcoury commented Mar 13, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fcoury commented Mar 13, 2026 •

edited

Loading