Skip to content

fix(providers/openai): skip reasoning that can't go inline #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mafredri wants to merge 6 commits into from
+118 −303
Draft

fix(providers/openai): skip reasoning that can't go inline #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1788a2
fix(providers/openai): skip reasoning that can't go inline
mafredri Oct 29, 2026
2f7f243
refactor(providers/openai): drop store from prompt conversion
mafredri Oct 29, 2026
8a6fe75
test(providers/openai): name the reasoning skip test for what it checks
mafredri Oct 29, 2026
13e36ee
refactor(providers/openai): drop unreachable item_reference check
mafredri Oct 29, 2026
a4b7d52
docs(providers/openai): say reasoning without replay data is skipped
mafredri Oct 29, 2026
7752a17
test(providers/openai): cover the reasoning skip directly
mafredri Oct 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
293 changes: 59 additions & 234 deletions providers/openai/openai_test.go
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 10 additions & 13 deletions providers/openai/responses_language_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,13 @@ func (o responsesLanguageModel) prepareParams(call fantasy.Call) (*responses.Res
allowPreviousResponseToolOutputs = true
}

storeEnabled := openaiOptions != nil && openaiOptions.Store != nil && *openaiOptions.Store
var input responses.ResponseInputParam
var inputWarnings []fantasy.CallWarning
var err error
if allowPreviousResponseToolOutputs {
input, inputWarnings, err = toResponsesPromptWithValidation(call.Prompt, modelConfig.systemMessageMode, storeEnabled, true)
input, inputWarnings, err = toResponsesPromptWithValidation(call.Prompt, modelConfig.systemMessageMode, true)
} else {
input, inputWarnings, err = toResponsesPrompt(call.Prompt, modelConfig.systemMessageMode, storeEnabled)
input, inputWarnings, err = toResponsesPrompt(call.Prompt, modelConfig.systemMessageMode)
}
warnings = append(warnings, inputWarnings...)
if err != nil {
Expand Down Expand Up @@ -462,11 +461,11 @@ func responsesUsage(resp responses.Response) fantasy.Usage {
return usage
}

func toResponsesPrompt(prompt fantasy.Prompt, systemMessageMode string, store bool) (responses.ResponseInputParam, []fantasy.CallWarning, error) {
return toResponsesPromptWithValidation(prompt, systemMessageMode, store, false)
func toResponsesPrompt(prompt fantasy.Prompt, systemMessageMode string) (responses.ResponseInputParam, []fantasy.CallWarning, error) {
return toResponsesPromptWithValidation(prompt, systemMessageMode, false)
}

func toResponsesPromptWithValidation(prompt fantasy.Prompt, systemMessageMode string, store bool, allowUnpairedFunctionCallOutputs bool) (responses.ResponseInputParam, []fantasy.CallWarning, error) {
func toResponsesPromptWithValidation(prompt fantasy.Prompt, systemMessageMode string, allowUnpairedFunctionCallOutputs bool) (responses.ResponseInputParam, []fantasy.CallWarning, error) {
var input responses.ResponseInputParam
var warnings []fantasy.CallWarning

Expand Down Expand Up @@ -682,13 +681,11 @@ func toResponsesPromptWithValidation(prompt fantasy.Prompt, systemMessageMode st
}
if item, ok := responsesReasoningInputItem(meta); ok {

@coder-agents-review coder-agents-review Bot Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

🤖

@coder-agents-review coder-agents-review Bot Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

🤖

@mafredri mafredri Oct 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 7752a17: added a reasoning-only subtest to TestResponsesToPrompt_DropsEmptyMessages.

🤖

input = append(input, item)
} else if store {
input = append(input, responses.ResponseInputItemParamOfItemReference(meta.ItemID))
} else {
// Unstored reasoning without finalized encrypted
// content cannot be resolved by the API.
continue
}
// Reasoning that cannot be sent inline is skipped, not

@coder-agents-review coder-agents-review Bot Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

🤖

@mafredri mafredri Oct 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in a4b7d52, reworded to an item OpenAI does not hold.

🤖

// referenced: a reference to an item OpenAI does not hold,
// such as one from an unstored response, fails the whole
// request.
continue
case fantasy.ContentTypeToolResult:
toolResultPart, ok := fantasy.AsContentType[fantasy.ToolResultPart](c)
Expand Down Expand Up @@ -1120,7 +1117,7 @@ func hasVisibleResponsesUserContent(content responses.ResponseInputMessageConten
func hasVisibleResponsesAssistantContent(items []responses.ResponseInputItemUnionParam, startIdx int) bool {
// Check if we added any assistant content parts from this message
for i := startIdx; i < len(items); i++ {
if items[i].OfMessage != nil || items[i].OfFunctionCall != nil || items[i].OfItemReference != nil || items[i].OfReasoning != nil || items[i].OfComputerCall != nil || items[i].OfWebSearchCall != nil {
if items[i].OfMessage != nil || items[i].OfFunctionCall != nil || items[i].OfReasoning != nil || items[i].OfComputerCall != nil || items[i].OfWebSearchCall != nil {
return true
}
}
Expand Down
5 changes: 3 additions & 2 deletions providers/openai/responses_options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,9 @@ type ResponsesReasoningMetadata struct {
EncryptedContent *string `json:"encrypted_content"`
Summary []string `json:"summary"`
// Finalized marks metadata copied from the completed reasoning output
// item. Only finalized encrypted content is replayed inline; streaming
// placeholders and metadata persisted before this field existed are not.
// item. Only finalized metadata with encrypted content is replayed;
// other reasoning, such as streaming placeholders and metadata persisted
// before this field existed, is skipped.
Finalized bool `json:"finalized,omitempty"`
}

Expand Down
2 changes: 1 addition & 1 deletion providers/openai/responses_params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ func TestPrepareParams_ValidatesFunctionCallOutputPairing(t *testing.T) {

input, warnings, err := toResponsesPrompt(fantasy.Prompt{
testResponsesProviderToolResultMessage("ws_01"),
}, "system", false)
}, "system")
require.NoError(t, err)
require.Equal(t, []fantasy.CallWarning{{
Type: fantasy.CallWarningTypeOther,
Expand Down
14 changes: 10 additions & 4 deletions providers/openai/responses_reasoning_replay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ func TestResponsesGenerate_ReasoningMetadataIsFinalized(t *testing.T) {
func TestResponsesReplay_FinalizedReasoningInput(t *testing.T) {
t.Parallel()

// The SDK marshals item references without a type field.
reference := map[string]any{"id": "rs_1"}
fullItem := func(summary ...any) map[string]any {
return map[string]any{
"type": "reasoning",
Expand All @@ -126,6 +124,9 @@ func TestResponsesReplay_FinalizedReasoningInput(t *testing.T) {
encrypted *string
summary []string
webSearch bool
// unfinalized clears Finalized, as in streaming placeholders and
// metadata saved before Finalized existed.
unfinalized bool
// want is the reasoning input item; nil means none is sent.
want map[string]any
}{
Expand Down Expand Up @@ -154,11 +155,16 @@ func TestResponsesReplay_FinalizedReasoningInput(t *testing.T) {
{
name: "no blob store true",
store: true,
want: reference,
},

@coder-agents-review coder-agents-review Bot Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

🤖

@mafredri mafredri Oct 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 7752a17: dropped the unfinalized store false row. The no blob store false row predates this PR, so I left it.

🤖

{
name: "no blob store false",
},
{
name: "unfinalized store true",
store: true,
encrypted: new("enc-final"),
unfinalized: true,
},
{
name: "store true before web search",
store: true,
Expand All @@ -185,7 +191,7 @@ func TestResponsesReplay_FinalizedReasoningInput(t *testing.T) {
ItemID: "rs_1",
EncryptedContent: tc.encrypted,
Summary: tc.summary,
Finalized: true,
Finalized: !tc.unfinalized,
}}},
next,
fantasy.TextPart{Text: "a"},
Expand Down
32 changes: 13 additions & 19 deletions providers/openai/responses_text_phase_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,24 +123,18 @@ func TestResponsesToPrompt_ReplaysTextPhase(t *testing.T) {
},
}

for _, store := range []bool{true, false} {
t.Run(fmt.Sprintf("store %t", store), func(t *testing.T) {
t.Parallel()

input, warnings, err := toResponsesPrompt(prompt, "system", store)
require.NoError(t, err)
require.Empty(t, warnings)
require.Len(t, input, 5)

var phases []any
for _, item := range input[1:] {
raw, err := json.Marshal(item)
require.NoError(t, err)
var fields map[string]any
require.NoError(t, json.Unmarshal(raw, &fields))
phases = append(phases, fields["phase"])
}
require.Equal(t, []any{"commentary", "final_answer", nil, nil}, phases)
})
input, warnings, err := toResponsesPrompt(prompt, "system")
require.NoError(t, err)
require.Empty(t, warnings)
require.Len(t, input, 5)

var phases []any
for _, item := range input[1:] {
raw, err := json.Marshal(item)
require.NoError(t, err)
var fields map[string]any
require.NoError(t, json.Unmarshal(raw, &fields))
phases = append(phases, fields["phase"])
}
require.Equal(t, []any{"commentary", "final_answer", nil, nil}, phases)
}
48 changes: 20 additions & 28 deletions providers/openai/responses_web_search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,18 +459,18 @@ func withoutItemID(meta *WebSearchCallMetadata) *WebSearchCallMetadata {
return &copied
}

func requireResponsesInputJSON(t *testing.T, prompt fantasy.Prompt, store bool, want string) []fantasy.CallWarning {
func requireResponsesInputJSON(t *testing.T, prompt fantasy.Prompt, want string) []fantasy.CallWarning {
t.Helper()

input, warnings, err := toResponsesPrompt(prompt, "system", store)
input, warnings, err := toResponsesPrompt(prompt, "system")
require.NoError(t, err)
encoded, err := json.Marshal(input)
require.NoError(t, err)
require.JSONEq(t, want, string(encoded))
return warnings
}

func TestResponsesToPrompt_UnstoredWebSearchCallAction(t *testing.T) {
func TestResponsesToPrompt_WebSearchCallAction(t *testing.T) {
t.Parallel()

for _, tc := range webSearchCallActionCases {
Expand All @@ -484,7 +484,7 @@ func TestResponsesToPrompt_UnstoredWebSearchCallAction(t *testing.T) {
fantasy.TextPart{Text: "Answer."},
)},
}
warnings := requireResponsesInputJSON(t, prompt, false, `[`+
warnings := requireResponsesInputJSON(t, prompt, `[`+
`{"role":"user","content":[{"type":"input_text","text":"Search."}]},`+
tc.wantInline+`,`+
`{"role":"assistant","content":"Answer."}]`)
Expand Down Expand Up @@ -537,7 +537,6 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {

tests := []struct {
name string
store bool
content []fantasy.MessagePart
laterContent []fantasy.MessagePart
// toolContentBefore and toolContentAfter are tool messages before
Expand All @@ -548,27 +547,20 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
wantWarning string
}{
{
name: "UnstoredInterleavedWithReasoning",
name: "InterleavedWithReasoning",
content: interleaved,
want: `[` + userJSON + `,` + searchJSON + `,` + reasoningJSON + `,` + openPageJSON + `,` + answerJSON + `]`,
},
{
// A stored search is sent inline too, never referenced.
name: "StoredInterleavedWithReasoning",
store: true,
content: interleaved,
want: `[` + userJSON + `,` + searchJSON + `,` + reasoningJSON + `,` + openPageJSON + `,` + answerJSON + `]`,
},
{
name: "UnstoredRepeatedParts",
name: "RepeatedParts",
content: slices.Concat(
webSearchCallParts("ws_01", search),
webSearchCallParts("ws_01", search),
),
want: `[` + userJSON + `,` + searchJSON + `]`,
},
{
name: "UnstoredWithoutIDs",
name: "WithoutIDs",
content: []fantasy.MessagePart{
webSearchCallParts("", withoutItemID(search))[1],
webSearchCallParts("", withoutItemID(openPage))[1],
Expand All @@ -577,7 +569,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
},
{
// Without a call ID, the metadata's item ID identifies the search.
name: "UnstoredRepeatedResultWithoutCallID",
name: "RepeatedResultWithoutCallID",
content: []fantasy.MessagePart{
webSearchCallParts("", search)[1],
webSearchCallParts("", search)[1],
Expand All @@ -587,18 +579,18 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
{
// No warning: the assistant message after the tool message
// replays the search.
name: "UnstoredResultInEarlierToolMessage",
name: "ResultInEarlierToolMessage",
toolContentBefore: []fantasy.MessagePart{webSearchCallParts("ws_01", search)[1]},
content: []fantasy.MessagePart{webSearchCallParts("ws_01", search)[1], answer},
want: `[` + userJSON + `,` + searchJSON + `,` + answerJSON + `]`,
},
{
name: "UnstoredSearchOnly",
name: "SearchOnly",
content: webSearchCallParts("ws_01", search),
want: `[` + userJSON + `,` + searchJSON + `]`,
},
{
name: "UnstoredPre62SearchWithoutStatus",
name: "LegacySearchWithoutStatus",
content: append(
webSearchCallParts("ws_01", &WebSearchCallMetadata{
ItemID: "ws_01",
Expand All @@ -611,7 +603,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
{
// Before #62 only Type and Query were saved. The API accepts a
// find_in_page action without url and pattern.
name: "UnstoredPre62FindInPageWithoutStatus",
name: "LegacyFindInPageWithoutStatus",
content: append(
webSearchCallParts("ws_01", &WebSearchCallMetadata{
ItemID: "ws_01",
Expand All @@ -622,7 +614,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
want: `[` + userJSON + `,{"type":"web_search_call","action":{"type":"find_in_page"}},` + answerJSON + `]`,
},
{
name: "UnstoredWithoutAction",
name: "WithoutAction",
content: append(
webSearchCallParts("ws_01", &WebSearchCallMetadata{ItemID: "ws_01", Status: "completed"}),
answer,
Expand All @@ -631,15 +623,15 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
wantWarning: `dropping web search "ws_01": metadata has no action`,
},
{
name: "UnstoredWithoutMetadata",
name: "WithoutMetadata",
content: append(webSearchCallParts("ws_01", nil), answer),
want: `[` + userJSON + `,` + answerJSON + `]`,
wantWarning: `dropping web search "ws_01": it has no web search metadata`,
},
{
// A part that can't replay a search doesn't warn when another
// part replays it.
name: "UnstoredUnreplayableThenReplayable",
name: "UnreplayableThenReplayable",
content: []fantasy.MessagePart{
webSearchCallParts("ws_01", &WebSearchCallMetadata{ItemID: "ws_01", Status: "completed"})[1],
answer,
Expand All @@ -648,7 +640,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
want: `[` + userJSON + `,` + answerJSON + `,` + searchJSON + `]`,
},
{
name: "UnstoredRepeatedWithoutMetadata",
name: "RepeatedWithoutMetadata",
content: []fantasy.MessagePart{
webSearchCallParts("ws_01", nil)[1],
webSearchCallParts("ws_01", nil)[1],
Expand All @@ -658,7 +650,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
wantWarning: `dropping web search "ws_01": it has no web search metadata`,
},
{
name: "UnstoredRepeatedInToolMessages",
name: "RepeatedInToolMessages",
toolContentBefore: []fantasy.MessagePart{webSearchCallParts("ws_01", search)[1]},
content: []fantasy.MessagePart{answer},
toolContentAfter: []fantasy.MessagePart{webSearchCallParts("ws_01", search)[1]},
Expand All @@ -667,14 +659,14 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
},
{
// The assistant message's reason wins over the tool message's.
name: "UnstoredWithoutMetadataAlsoInToolMessage",
name: "WithoutMetadataAlsoInToolMessage",
toolContentBefore: []fantasy.MessagePart{webSearchCallParts("ws_01", search)[1]},
content: []fantasy.MessagePart{webSearchCallParts("ws_01", nil)[1], answer},
want: `[` + userJSON + `,` + answerJSON + `]`,
wantWarning: `dropping web search "ws_01": it has no web search metadata`,
},
{
name: "UnstoredUnknownActionType",
name: "UnknownActionType",
content: append(
webSearchCallParts("ws_01", &WebSearchCallMetadata{
ItemID: "ws_01",
Expand Down Expand Up @@ -702,7 +694,7 @@ func TestResponsesToPrompt_WebSearchCallReplay(t *testing.T) {
if tc.toolContentAfter != nil {
prompt = append(prompt, fantasy.Message{Role: fantasy.MessageRoleTool, Content: tc.toolContentAfter})
}
warnings := requireResponsesInputJSON(t, prompt, tc.store, tc.want)
warnings := requireResponsesInputJSON(t, prompt, tc.want)
var messages []string
for _, warning := range warnings {
messages = append(messages, warning.Message)
Expand Down
4 changes: 2 additions & 2 deletions providers/openai/tool_result_media_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ func TestToResponsesPrompt_MediaToolResult_ImagePNG(t *testing.T) {
},
}

input, warnings, err := toResponsesPrompt(prompt, "system", false)
input, warnings, err := toResponsesPrompt(prompt, "system")
require.NoError(t, err)
require.Empty(t, warnings)
// Assistant function call + function_call_output + synthetic user image
Expand Down Expand Up @@ -373,7 +373,7 @@ func TestToResponsesPrompt_MediaToolResult_UnsupportedMediaType(t *testing.T) {
},
}

input, warnings, err := toResponsesPrompt(prompt, "system", false)
input, warnings, err := toResponsesPrompt(prompt, "system")
require.NoError(t, err)

// Assistant function call + function_call_output, but no synthetic user
Expand Down
Loading