Repository navigation
Conversation
Reasoning without finalized encrypted content was sent as an item_reference whenever store was true. That only resolves while OpenAI still holds the source response. A reference to reasoning from an unstored response, or from a stored response whose stream was cut, gets a 404 that fails the whole request, so every later turn of the chat fails the same way. Such reasoning is now skipped in both store modes. By default, models before GPT-5.6 ignore reasoning from earlier turns anyway.
Prompt conversion no longer reads store: searches and reasoning go inline or not at all in both modes. Tests that ran once per store value now run once.
|
@codex review
|
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 1 | Last posted: Round 1, 10 findings (1 P2, 3 P3, 4 P4, 2 Nit), COMMENT. Review Finding inventoryFinding inventory: coder/fantasy PR 81Findings
Contested and acknowledgedRound logRound 1Round type: round 1, full panel. Netero first pass: 1 P3. Law not run (effective additions 105). Panel: ging-go, ryosuke, melody (specialists), pariston, mafuuu (core), bisky (tests changed), gon, leorio (once-per-PR floor), razor (wildcard). Size 8 + wildcard, focused Go PR. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
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. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR stops sending reasoning without finalized encrypted content as an item_reference and removes store from prompt conversion. Findings: 1 P2, 3 P3, 4 P4, 2 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
providers/openai/responses_reasoning_replay_test.go:209: the test callsGenerate(context.Background(), ...)instead oft.Context(), so the context is not cancelled when the test ends.providers/openai/responses_web_search_test.go:22: thewantInlinecomment says "when it is not referenced", but no code references a search anymore, so the comment describes a case that does not exist.providers/openai/responses_language_model.go:468:allowUnpairedFunctionCallOutputsis a bare bool, so the caller at line 229 passestruewith no name at the call site.providers/openai/responses_language_model.go:1117: the commentCheck if we added any assistant content parts from this messagerepeats the function name and loop.
Notes:
- No non-test code in
providers/openaibuilds anitem_referenceafter this PR, so removingOfItemReferencefromhasVisibleResponsesAssistantContenthides nothing. - Restoring the
item_referencebranch fails four tests, as the description states (reproduced withgo test ./providers/openai -count=1).
providers/openai/responses_options.go:152
P4 [CRF-7] The exported Finalized doc says unfinalized metadata is not "replayed inline", which implies another replay path; after this PR it is not sent at all. (Leorio)
Callers such as coder/coder read this doc to decide whether old rows need migration. Suggested: // Finalized marks metadata copied from the completed reasoning output item. Only finalized metadata with encrypted content is sent on replay; reasoning from streaming placeholders or saved before this field existed is skipped.
🤖
🤖 This review was automatically generated with Coder Agents.
| @@ -682,13 +681,10 @@ func toResponsesPromptWithValidation(prompt fantasy.Prompt, systemMessageMode st | |||
| } | |||
| if item, ok := responsesReasoningInputItem(meta); ok { | |||
There was a problem hiding this comment.
P2 [CRF-2] With Store: true and no IncludeReasoningEncryptedContent in Include, all reasoning, including reasoning from completed responses, is now dropped from every later request with no warning. (Pariston, Mafuuu, Melody, Razor, Leorio)
Fantasy never adds reasoning.encrypted_content itself (prepareParams only copies openaiOptions.Include, lines 312-313), so finalResponsesReasoningMetadata gives Finalized: true, EncryptedContent: nil; the description's "always has encrypted content" holds for coder/coder only because chatopenai.EnsureResponseIncludes forces the include. Before this PR that item went back as an item_reference to a stored, completed response, which your probe shows returns 200, and the no blob store true case now asserts nothing is sent, so these callers lose reasoning between tool-call steps of a single agent run. Fix one of: add IncludeReasoningEncryptedContent in prepareParams for reasoning models so replay always has a blob; keep the reference for Finalized items when store is true and skip only unfinalized ones; or emit a CallWarning per skipped reasoning item with an ItemID (as dropping web search ... does) and document on Store and Include that reasoning is replayed only with that include. The warning also gives a signal for the pre-coder/coder#30298 rows this PR targets.
🤖
| @@ -682,13 +681,10 @@ func toResponsesPromptWithValidation(prompt fantasy.Prompt, systemMessageMode st | |||
| } | |||
| if item, ok := responsesReasoningInputItem(meta); ok { | |||
There was a problem hiding this comment.
P3 [CRF-1] No test covers an assistant message whose only content is unfinalized reasoning, which this PR changes from kept (as an item_reference) to dropped with the empty-message warning. (Netero)
A scratch test with [user "q", assistant {ReasoningPart{ItemID: "rs_1"}}, user "q2"] returned 2 items and dropping empty assistant message (contains neither user-facing content nor tool calls). The only test asserting that warning, TestResponsesToPrompt_DropsEmptyMessages, uses an assistant message with no parts, so a change that keeps or errors on reasoning-only messages passes the suite. Add a subtest there with a reasoning-only assistant message asserting the drop and the warning.
🤖
There was a problem hiding this comment.
Fixed in 7752a17: added a reasoning-only subtest to TestResponsesToPrompt_DropsEmptyMessages.
🤖
| // content cannot be resolved by the API. | ||
| continue | ||
| } | ||
| // Reasoning that cannot be sent inline is skipped, not |
There was a problem hiding this comment.
P3 [CRF-3] The comment says a reference fails for "an item OpenAI did not store", which implies a reference is safe when store was true. (Gon, Leorio)
The PR's evidence shows references to stored items also return 404: a stored response whose stream was cut, zero data retention, and expired items. That is the reason the else if store branch was removed. Suggested: // Never sent as an item_reference: a reference to an item OpenAI no longer holds fails the whole request with 404.
🤖
There was a problem hiding this comment.
Fixed in a4b7d52, reworded to an item OpenAI does not hold.
🤖
| }) | ||
|
|
||
| t.Run("store true skips reasoning when item id missing", func(t *testing.T) { | ||
| t.Run("skips reasoning when item id missing", func(t *testing.T) { |
There was a problem hiding this comment.
P3 [CRF-4] The subtest skips reasoning when item id missing no longer tests the meta.ItemID == "" check at responses_language_model.go:679, and no other test does. (Ryosuke)
Its fixture has no Finalized and no encrypted content, so responsesReasoningInputItem rejects it with or without the ID check; changing the check to if meta == nil { leaves go test ./providers/openai/ -run 'Reasoning|Responses' passing. The check still matters: responsesReasoningInputItem accepts finalized metadata with encrypted content and an empty ItemID and marshals {"id":"",...,"type":"reasoning"} (API response to an empty id unverified). Move the ItemID == "" check into responsesReasoningInputItem and add a TestResponsesReplay_FinalizedReasoningInput case with finalized encrypted content and an empty ItemID.
🤖
There was a problem hiding this comment.
Fixed in 7752a17: the fixture now has finalized encrypted content, so only the ID check rejects it. Removing the check fails the subtest.
🤖
| t.Parallel() | ||
|
|
||
| input, warnings, err := toResponsesPrompt(prompt, "system", false) | ||
| input, warnings, err := toResponsesPrompt(prompt, "system") |
There was a problem hiding this comment.
P4 [CRF-5] TestResponsesToPrompt_ReasoningWithWebSearchCombined and TestResponsesToPrompt_ReasoningWithFunctionCallCombined now only check that unfinalized reasoning is missing, the same branch skips unfinalized reasoning covers, while their names describe ordering they no longer check. (Bisky)
The skip is a continue at responses_language_model.go:688 that does not read the adjacent parts. Ordering of finalized reasoning before a function call and before a web search is checked in TestResponsesReplay_FinalizedReasoningInput by the store true and store true before web search rows. Delete both Combined tests.
🤖
| name: "no blob store true", | ||
| store: true, | ||
| want: reference, | ||
| }, |
There was a problem hiding this comment.
P4 [CRF-6] The new store true/store false row pairs for no blob and unfinalized build the same input, the duplication the description gives as a reason to remove store. (Bisky, Mafuuu)
Store here only sets the request's store field, which the test does not read. If replay stays independent of store (see the Store: true reasoning finding), keep the store true row of each pair, since store: true is the mode that used to send the reference, and drop the store false rows.
🤖
There was a problem hiding this comment.
Fixed in 7752a17: dropped the unfinalized store false row. The no blob store false row predates this PR, so I left it.
🤖
| encrypted *string | ||
| summary []string | ||
| webSearch bool | ||
| // unfinalized marks metadata saved before Finalized existed. |
There was a problem hiding this comment.
P4 [CRF-8] The unfinalized field comment names only metadata saved before Finalized existed; streaming placeholders (responses_language_model.go:1511) are also unfinalized, and the cut-stream 404 in the description is that case. (Gon)
Suggested: // unfinalized clears Finalized, as in streaming placeholders and metadata saved before Finalized existed.
🤖
|
|
||
| // Without an ItemID we cannot reference the reasoning | ||
| // item. Order: user, assistant text, user. | ||
| // Without an ItemID the reasoning cannot be sent. Order: user, |
There was a problem hiding this comment.
Nit [CRF-9] "Without an ItemID the reasoning cannot be sent." repeats the subtest name. (Gon)
Keep only the order list, // user, assistant text, user., as the sibling subtest does at line 4803.
🤖
| }, | ||
| { | ||
| name: "UnstoredPre62SearchWithoutStatus", | ||
| name: "Pre62SearchWithoutStatus", |
There was a problem hiding this comment.
Nit [CRF-10] The renamed cases Pre62SearchWithoutStatus and Pre62FindInPageWithoutStatus use a PR number as their only description. (Gon)
This diff already renames them, so name them after the data: LegacySearchWithoutStatus and LegacyFindInPageWithoutStatus. The comment at line 604 can keep #62.
🤖
The item ID test now uses finalized encrypted reasoning, so only the ID check rejects it. A reasoning-only assistant message is dropped with the empty-message warning. The two Combined tests only rechecked the skip, and the store false row repeated the store true one.
Stacked on #77.
Reasoning that can't be sent inline, because it has no finalized encrypted content, was sent as an
item_referencewheneverstorewas true. That resolves only while OpenAI still holds the source response. Otherwise the whole request gets a 404, and since a chat resends the same history, every later turn fails the same way.In coder/coder this affects reasoning saved before coder/coder#30298 (2026-10-13), which has no
finalizedflag. Reasoning saved since then always has encrypted content and goes inline. The reference fails when:storeas false;storewas false when the message was saved;Such reasoning is now skipped in both store modes. The model loses it: by default models before GPT-5.6 don't render reasoning from earlier turns, GPT-5.6 and later do (reasoning guide). An assistant message whose only content is such reasoning is now dropped with the existing empty-message warning, as it already was with
store: false.With #77 and this, prompt conversion no longer reads
store, so the parameter is removed.Evidence
item_referenceprobes through the AI Gateway on gpt-5-mini: reasoning from a completed stored response got 200; from astore: falseresponse, 404; from a stored response whose stream was cut after the reasoning started, 404 at 5s and 60s.agent.Streamandagent.Generateon gpt-5-mini, with the turn 1 messages round-tripped through JSON andfinalizedremoved, turn 2 withstore: true. On fix(providers/openai): replay web searches and computer calls without item IDs #77: turn 1store: falsegot 404 "Item with id 'rs_...' not found", turn 1store: truegot 200. With this PR both got 200.Validation
go test ./... -count=1,go vet ./...and golangci-lint v2.15.0 pass. gofumpt only flags two existing blank lines inopenai_test.go.Decisions
storerather than rename it_: revive flags the unused parameter, and the tests that ran once per store value checked the same output twice.