[codex] Store pending response items directly#24865
Conversation
![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: 128194f473
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.codex | ||
| .session | ||
| .record_conversation_items(turn_context.as_ref(), &items) | ||
| .inject_no_new_turn(items, Some(turn_context.as_ref())) | ||
| .await; |
There was a problem hiding this comment.
When CodexThread::inject_response_items is called while a turn is active, this now routes through inject_no_new_turn, whose inject_if_running success path only appends the items to the active turn's pending-input queue and returns without recording them. The method still returns Ok after flush_rollout, but the items are not in history/rollout until a later pending-input drain; if the active turn is interrupted or aborted first, abort_all_tasks clears that pending input, so an API call that previously durably appended raw items can silently lose them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Temporary cost of unification for an advanced-use method.
| Ok(ResponseEvent::OutputItemDone(item)) => { | ||
| sess.record_into_history(std::slice::from_ref(&item), turn_context) | ||
| sess.record_conversation_items(turn_context, std::slice::from_ref(&item)) | ||
| .await; |
There was a problem hiding this comment.
In local compaction, drain_to_completed explicitly treats a stream that closes before response.completed as an error, but this call now persists every OutputItemDone to rollout before that completion is observed. If the compact model emits the summary item and then the stream disconnects before response.completed, the compaction fails/retries without installing replace_compacted_history, yet the unfinished compact output is already durable and will pollute reconstructed history after a restart; the old record_into_history path did not write these provisional items to rollout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
replace_compacted_history "will" wipe these extra items anyway.
pakrym-oai
force-pushed
the
pakrym/responseitem-pending-input
branch
6 times, most recently
from
0379ef7 to
7aa4197
Compare
pakrym-oai
force-pushed
the
pakrym/responseitem-pending-input
branch
from
7aa4197 to
0a683cc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a37149b24
ℹ️ 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 Err(items) = self.inject_if_running(items).await else { | ||
| return; |
There was a problem hiding this comment.
When thread/inject_items is called while a turn is active, this early return queues the items as pending turn input instead of recording them immediately. That violates the app-server contract in app-server/README.md that injected items are persisted to rollout and included in subsequent model requests without starting a user turn: the JSON-RPC response can return before the items are durably written, and the active turn will treat them as pending input and issue an extra follow-up model request even if the model was otherwise done. CodexThread::inject_response_items passes a current_turn_context specifically for appending raw history; this path should record to that context rather than always preferring inject_if_running.
Useful? React with 👍 / 👎.
| sess.record_conversation_items(turn_context, std::slice::from_ref(&item)) | ||
| .await; |
There was a problem hiding this comment.
For local compaction, this now persists and emits each compaction model OutputItemDone, whereas the previous record_into_history call kept these synthesized summary outputs only in memory until replace_compacted_history runs after response.completed. If the compaction stream emits an output item and then fails or is interrupted before completion, the rollout now contains that partial compaction response as ordinary conversation history, so a resumed session can include a summary the compaction never successfully installed.
Useful? React with 👍 / 👎.
| self.record_conversation_items( | ||
| task.turn_context.as_ref(), | ||
| std::slice::from_ref(&marker), | ||
| ) |
There was a problem hiding this comment.
For interrupted turns, this changed the internal <turn_aborted> marker from history+rollout-only persistence to record_conversation_items, which also sends a RawResponseItem before TurnAborted. That marker is model-visible recovery context, not model output; clients that subscribe to raw response items can now render or process an internal control message for every interrupted regular/review turn, whereas the previous path deliberately persisted it without a separate client-visible raw item.
Useful? React with 👍 / 👎.
2 participants
Why
Pending injected items were stored as
ResponseInputItemeven though session history and raw item injection already operate onResponseItem. That forced record-time conversions and split injection behavior across several caller-specific paths.What Changed
ResponseItemSessioninjection behavior intocore/src/session/inject.rs, with explicit helpers for active-only, starts-turn, and no-new-turn deliveryrecord_conversation_itemsand update interrupt-marker expectations for its raw-item notificationValidation
just fix -p codex-corejust test -p codex-core session::tests::interrupting_regular_turn_waiting_on_startup_prewarm_emits_turn_abortedjust test -p codex-core; before the final expectation follow-up, the run failed primarily in MCP/code-mode cases that could not locatetarget/debug/test_stdio_server, plus one timeout. The affected startup-prewarm expectation was corrected and passed in isolation.