Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce54e4725b
ℹ️ 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".
| self.session | ||
| .update_settings(SessionSettingsUpdate { | ||
| notify_client, | ||
| ..Default::default() | ||
| }) |
There was a problem hiding this comment.
Queue notify-client updates with turn submissions
set_notify_client applies notify_client immediately through update_settings, but turn-starting calls still use submit(...), which only enqueues work. In a multi-connection app-server session, if client A and client B (different clientInfo.name) start turns back-to-back on the same thread, B can overwrite this shared setting before A's queued turn is processed, so A's legacy notify payload is attributed to the wrong client. This should be ordered with the turn operation itself (for example via a queued op/override) so client attribution is per-turn and race-free.
Useful? React with 👍 / 👎.
|
|
||
| const TUI_NOTIFY_CLIENT: &str = "codex-tui"; | ||
|
|
||
| async fn initialize_app_server_client_name(thread: &CodexThread) { |
There was a problem hiding this comment.
nit: this name makes it sound like the TUI is built on top of app server. prob worth adding an inline comment to explain that we are special casing TUI surface
Why
The
notifyhook payload did not identify which Codex client started the turn. That meant downstream notification hooks could not distinguish between completions coming from the TUI and completions coming from app-server clients such as VS Code or Xcode. Now that the Codex App provides its own desktop notifications, it would be nice to be able to filter those out.This change adds that context without changing the existing payload shape for callers that do not know the client name, and keeps the new end-to-end test cross-platform.
What changed
clientfield to the legacynotifyJSON payloadcoreandhooks; the internal session and turn state now carries it asapp_server_client_namecodex-tuifor TUI turnsinitialize.clientInfo.namein the app server and applied it to subsequent turns before dispatching hookspython3script so the test does not rely on Unix shell permissions orbashdocs/config.mdTesting
cargo test -p codex-hookscargo test -p codex-tuicargo test -p codex-app-server suite::v2::initialize::turn_start_notify_payload_includes_initialize_client_name -- --exact --nocapturecargo test -p codex-core(src/lib.rspassed;core/tests/all.rsstill has unrelated existing failures in this environment)Docs
The public config reference on
developers.openai.com/codexshould mention that the legacynotifypayload may include a top-levelclientfield. The TUI reportscodex-tui, and the app server reportsinitialize.clientInfo.namewhen it is available.