feat: normalize provider-bound Responses state - #5410
Conversation
Signed-off-by: huangruiteng <huangrt01@163.com>
…istory-sse-normalizers Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
Signed-off-by: huangruiteng <huangrt01@163.com>
…phan-recovery # Conflicts: # sdk/cliproxy/auth/conductor_stream.go
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 235af07cd0
ℹ️ 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".
| if committed { | ||
| opts.EnsureMetadata()[cliproxyexecutor.ProviderOutputCommittedMetadataKey] = true |
There was a problem hiding this comment.
When a stream delivers one payload and then fails, this goroutine writes into the same metadata map originally passed by the handler because Options was only shallow-copied. After the unbuffered first-chunk send completes, the handler may still be iterating or serializing opts.Metadata for a stream interceptor while this code processes the next error chunk, causing a data race and potentially a fatal concurrent-map panic. Give the wrapper private metadata before making this asynchronous write.
AGENTS.md reference: AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6f1c08c. wrapStreamResult now clones request metadata before starting the goroutine, so committed/error bookkeeping cannot mutate the handler-owned map. Added a regression that drains a payload-then-error stream and verifies the caller map remains unchanged; focused race and full-suite validation pass.
| func providerHistorySessionIDs(headers http.Header, payload []byte, metadata map[string]any) []string { | ||
| primaryID, fallbackID := extractSessionIDs(headers, payload, metadata) | ||
| aliases := mergeSessionAliases(nil, primaryID, fallbackID) |
There was a problem hiding this comment.
When a Responses request has no explicit session header, prompt-cache key, or other stable identifier, extractSessionIDs falls back to a hash of the first user text. That heuristic is acceptable for affinity but not for credential-ownership enforcement: two unrelated conversations whose current input is the same common text (for example, continue) become one session, so after one uses credential A, a separate request using its own previous_response_id under credential B is rejected as a foreign continuation. Provider-history safety decisions should use only identifiers that uniquely establish the conversation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6f1c08c. Provider-history ownership now uses only explicit client/execution identities plus the stable prompt-cache alias; the affinity-only first-message hash fallback is excluded. Added a regression proving a common message such as continue produces no ownership key.
| for key, item := range typed { | ||
| switch strings.ToLower(strings.TrimSpace(key)) { | ||
| case "vector_store_id", "vector_store_ids": | ||
| if hasSemanticHistoryValue(item) { | ||
| return true |
There was a problem hiding this comment.
On a foreign-history replay, this recursive scan treats any semantically nonempty value beneath a key named vector_store_id or vector_store_ids as an actual hosted resource. A normal custom-function schema such as properties.vector_store_id: {"type":"string"} therefore triggers foreign_tool_resource_requires_rehydration, even though it merely declares an argument and supplies no credential-bound ID. Restrict this check to concrete hosted-tool configuration values rather than recursively traversing arbitrary tool schemas.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6f1c08c. Bound-resource detection now inspects only concrete file_search tool configuration fields. It no longer recursively traverses arbitrary function parameter schemas; a custom function property named vector_store_id is covered by a new normalization regression.
|
@codex review Replacement for auto-closed #5220. The refreshed compare contains only the intended SDK files. This head also handles the App-host orphan |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 235af07cd0
ℹ️ 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".
| case "response.reasoning_summary_text.delta", "response.reasoning_text.delta": | ||
| delta, _ := event["delta"].(string) | ||
| s.active.summaries[summaryIndex] += delta | ||
| case "response.reasoning_summary_text.done", "response.reasoning_text.done": |
There was a problem hiding this comment.
When a compatible provider omits response.output_item.done but emits response.reasoning_text.delta or .done, these cases accumulate the text as a summary; closeActive consequently serializes it under item.summary as summary_text and emits no content entry of type reasoning_text. This changes the Responses schema semantics and causes clients replaying the synthesized completed item to lose the actual reasoning-content shape, so raw reasoning text should be tracked separately and emitted in item.content.
Useful? React with 👍 / 👎.
|
@codex review Addressed all current review findings in 6f1c08c: private stream metadata ownership, explicit-only provider-history identity, and concrete file_search resource detection. Added negative regressions and reran focused/race/full-suite/build/diff validation; exact-scope quality receipt cqr_a3048a2fe9640eb5b484 passes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1c08c360
ℹ️ 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".
| if itemType == "reasoning" && !hasSemanticHistoryValue(item["summary"]) { | ||
| result.Changed = true | ||
| result.DroppedItems++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
When a foreign-history reasoning item has an empty summary but portable text in content entries of type reasoning_text, this condition drops the entire item after removing its provider-bound fields. That is a supported Responses shape—the request translators explicitly read content[].reasoning_text when no summary exists—so a credential handoff silently removes useful reasoning context. Treat semantic reasoning content as sufficient to retain the item.
Useful? React with 👍 / 👎.
Signed-off-by: huangruiteng <huangrt01@163.com>
|
Correction after live same-task validation: dropping a confirmed orphan host output is syntactically valid but semantically unsafe. In Codex App, the orphan The updated PR head (
Validation:
This supersedes the earlier wording in this comment that described the orphan output as safe to drop. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 952e2f587f
ℹ️ 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".
| if len(s.pending) > 0 && !bytes.HasSuffix(s.pending, []byte("\n")) && !bytes.HasPrefix(chunk, []byte("\n")) && responsesSSEChunkStartsField(chunk) { | ||
| s.pending = append(s.pending, '\n') | ||
| } | ||
| s.pending = append(s.pending, chunk...) |
There was a problem hiding this comment.
When a compatible Responses model uses OpenAICompatExecutor, each translated chunk is emitted without a trailing blank-line delimiter (openai_compat_executor.go constructs streamLine as data: <payload>, and common.SSEEventData likewise omits trailing newlines). This state therefore buffers the first complete event, inserts only one newline before the next field, and never finds \n\n; clients receive no events and Finish() ultimately reports an incomplete stream as a 502. Treat a complete pending data frame followed by a new data:/event: field as a frame boundary rather than merely inserting one newline.
Useful? React with 👍 / 👎.
Signed-off-by: huangruiteng <huangrt01@163.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 566da3b430
ℹ️ 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".
| itemType, _ := items[0]["type"].(string) | ||
| if itemType == "message" { | ||
| return true | ||
| } | ||
| _, hasRole := items[0]["role"].(string) | ||
| return itemType == "" && hasRole |
There was a problem hiding this comment.
When a session is tainted and remains on the same credential with previous_response_id, any single object whose type is message is classified as provider-neutral without inspecting its fields. A client carrying one replayed message with id/provider_item_id, or an input_file containing a foreign file_id, therefore bypasses normalizeProviderBoundResponseHistory and sends credential-bound state unchanged. Require the message to contain only neutral incremental fields, or run the existing bound-field/resource checks before taking this bypass.
Useful? React with 👍 / 👎.
| func providerHistorySessionIDs(headers http.Header, payload []byte, metadata map[string]any) []string { | ||
| primaryID, fallbackID := extractExplicitSessionIDs(headers, payload, metadata) | ||
| aliases := mergeSessionAliases(nil, primaryID, fallbackID) |
There was a problem hiding this comment.
extractExplicitSessionIDs can return a child or subagent as the primary ID and its parent as the fallback, but merging both into one ownership alias group makes independent branches overwrite each other. If two child sessions sharing a parent commit under different credentials, SessionCache.SetAliases merges the second child with the parent's existing group and rebinds the first child's primary ID; that first child is then treated as foreign and its own previous_response_id is rejected. Parent fallback IDs should not be ownership aliases for child/subagent sessions, matching the separation already used by session affinity.
Useful? React with 👍 / 👎.
1 participant
Supersedes #5220, which was auto-closed after its stale base snapshot incorrectly treated newer upstream
devfiles as part of the pull request.Summary
previous_response_idrequests while continuing to quarantine mixed transcripts;automation_updateoutput, while unknown or potentially paired missing-call_idoutputs still fail closed;Validation
gofmton changed Go filesgo build -o test-output ./cmd/servergo test ./...go test -racefor provider-history, SSE, and websocket pathsgit diff --check origin/devThe replacement compare contains only the intended SDK files and no
AGENTS.mdorinternal/translatorchanges.