Skip to content

Move item event mapping into app-server-protocol#20299

Merged
pakrym-oai merged 11 commits into
mainfrom
pakrym/item-event-mapping
Apr 30, 2026
Merged

Move item event mapping into app-server-protocol#20299
pakrym-oai merged 11 commits into
mainfrom
pakrym/item-event-mapping

Conversation

@pakrym-oai
Copy link
Copy Markdown
Collaborator

@pakrym-oai pakrym-oai commented Apr 30, 2026

Why

Follow-up to #20291.

The v2 item-event-to-notification translation had been embedded in app-server/src/bespoke_event_handling.rs, which made it hard to reuse anywhere else. This PR moves that stateless mapping into shared protocol code so other entry points can produce the same ServerNotification payloads without copying app-server logic.

That also lets thread-manager-sample demonstrate the same notification surface that the app server exposes, instead of only printing the final assistant message.

What changed

  • move item_event_to_server_notification into codex-app-server-protocol::protocol::event_mapping
  • keep the mapper tests next to the shared implementation in codex-app-server-protocol
  • re-export the mapper from codex-core-api so lightweight consumers can use it without reaching into app-server-protocol directly
  • simplify app-server/src/bespoke_event_handling.rs so it delegates the stateless event-to-notification projection to the shared helper
  • update thread-manager-sample to:
    • print mapped notifications as newline-delimited JSON
    • use the shared mapper through codex-core-api
    • enable the default feature set so the sample exposes the normal tool surface
    • use a read_only permission profile so shell commands can run in the sample without widening permissions

Testing

  • cargo test -p codex-app-server-protocol
  • cargo test -p codex-core-api
  • cargo test -p codex-app-server bespoke_event_handling::tests
  • cargo test -p codex-thread-manager-sample
  • cargo run -p codex-thread-manager-sample -- "briefly explore the repo with pwd and ls, then summarize it"
Base automatically changed from pakrym/remove-bespoke-api-version to main April 30, 2026 01:55
@pakrym-oai pakrym-oai changed the title app-server-protocol: move item event mapping out of bespoke_event_handling Add a helper to map item event to ServerNotifications Apr 30, 2026
@pakrym-oai pakrym-oai changed the title Add a helper to map item event to ServerNotifications Move item event mapping into app-server-protocol Apr 30, 2026
@@ -0,0 +1,807 @@
use crate::protocol::common::ServerNotification;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like protocol crates would ideally be "types only," but I guess that ship has sailed...

@starr-openai
Copy link
Copy Markdown
Contributor

starr-openai commented Apr 30, 2026

not sure if you were planning on doing this in a follow-up / intended behavior, but for the new thread-manager-sample code:

codex-rs/thread-manager-sample/src/main.rs on the PR branch, codex-rs/thread-manager-sample/src/main.rs:317: the sample always calls item_event_to_server_notification(..., /*is_file_change_output*/ false).
  - But the extracted mapper in codex-rs/app-server-protocol/src/protocol/event_mapping.rs, codex-rs/app-server-protocol/src/protocol/event_mapping.rs:477, uses that flag to decide whether an ExecCommandOutputDelta becomes
    FileChangeOutputDelta or CommandExecutionOutputDelta.
  - The app-server callsite preserves the old behavior by computing is_file_change from turn state before calling the helper, codex-rs/app-server/src/bespoke_event_handling.rs:1174.

  So: the extracted conversion itself looks semantically faithful to the old app-server logic, including the old MCP/collab helper paths. I did not find a mismatch in the actual moved mapping code. The bug is that the new
  sample does not preserve the app-server’s apply_patch/file-change discrimination, so if a turn emits PatchApplyBegin followed by ExecCommandOutputDelta, the sample will report the output as command output instead of file-
  change output.

  If your question is specifically “is the app-server conversion still correct after extraction?”, my answer is yes. If it’s “does every new caller now get the same surface as app-server?”, then no: thread-manager-sample
  currently doesn’t.
@pakrym-oai
Copy link
Copy Markdown
Collaborator Author

not sure if you were planning on doing this in a follow-up / intended behavior, but for the new thread-manager-sample code:

Yep, deleting that flag in #20471

@pakrym-oai pakrym-oai merged commit 5cc5f12 into main Apr 30, 2026
24 of 25 checks passed
@pakrym-oai pakrym-oai deleted the pakrym/item-event-mapping branch April 30, 2026 18:02
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

3 participants