[codex] Assign response item IDs when recording history#28814
Conversation
7acd80a to
bd68179
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd681791d8
ℹ️ 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".
2cf87aa to
4966ca6
Compare
4966ca6 to
009bd87
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 009bd870cd
ℹ️ 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".
| ResponseItem::LocalShellCall { .. } => "lsh", | ||
| ResponseItem::FunctionCall { .. } => "fc", | ||
| ResponseItem::ToolSearchCall { .. } => "tsc", | ||
| ResponseItem::FunctionCallOutput { .. } => "fco", |
There was a problem hiding this comment.
Use the Responses prefix for function output IDs
When item_ids is enabled and any function/tool result is recorded, this branch generates IDs like fco_<uuid> for function_call_output items. The Responses API uses the fc_... item-id shape for function tool call outputs and validates item IDs by type, so the next request after a tool call can be rejected solely because Codex minted an invalid output item ID; use the provider-accepted prefix here instead of the invented fco prefix.
Useful? React with 👍 / 👎.
6d33e10 to
009bd87
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949475bda2
ℹ️ 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".
| if !include_item_ids { | ||
| strip_response_item_ids(&mut body); |
There was a problem hiding this comment.
Preserve Azure store item IDs by default
When using the Azure Responses endpoint, ModelClient::build_responses_request still sets store to true for every request, but include_item_ids is false unless the new under-development item_ids feature is enabled. This unconditional strip therefore removes IDs from the default Azure stored-history path; before this change the request.store && is_azure_responses_endpoint() branch specifically re-attached those IDs, so Azure continuation/store requests lose the item identifiers unless users opt into an unrelated rollout flag.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
|
||
| /// Sets the Responses API item ID for variants that carry one. | ||
| pub fn set_id(&mut self, new_id: String) { | ||
| /// Sets or clears the Responses API item ID for variants that carry one. |
There was a problem hiding this comment.
whats the point of ever stripping all the items on a request (by calling set_id None?)
There was a problem hiding this comment.
i guess it doesn't really matter though, the functionality is fine to have (and maybe some providers don't support the ID?)
There was a problem hiding this comment.
I want to re-enable this for everyone but we had it disabled for a bit (because having IDs cause Responses API to try and load the item).
Why
Client-created response items enter history without IDs, so their identity is lost across rollout persistence and resume. IDs should be assigned once at the history-recording boundary, while IDs returned by the server must remain unchanged.
The Responses API validates item IDs using type-specific prefixes. Locally generated IDs therefore use the matching prefix plus a hyphenated UUIDv7, keeping them valid while distinguishable from server-generated IDs. Because this changes persisted history and provider request shapes, the behavior is opt-in behind the under-development
item_idsfeature. Compaction triggers remain request controls whose API shape does not accept an ID.What changed
item_idsfeature and expose it inconfig.schema.json.ResponseItemIDs serializable and expose them in the generated app-server schemas.item_idsis enabled, assign an ID during conversation-history preparation if an item has no ID.CompactionTriggerand document why it has no ID.prepare_conversation_items_for_historyis the single response-item ID allocation boundary.Test plan
just test -p codex-featuresjust test -p codex-core response_item_ids_persist_across_resume_and_preserve_server_idsjust test -p codex-core non_openai_responses_requests_omit_item_turn_metadatajust test -p codex-core resize_all_images_prepares_failures_before_history_insertionjust test -p codex-protocoljust test -p codex-app-server-protocoljust test -p codex-api azure_default_store_attaches_ids_and_headers