diff --git a/backend/internal/service/openai_codex_transform.go b/backend/internal/service/openai_codex_transform.go index 01b5d3e526..4bb29167f9 100644 --- a/backend/internal/service/openai_codex_transform.go +++ b/backend/internal/service/openai_codex_transform.go @@ -1157,11 +1157,41 @@ func filterCodexInputWithOptions(input []any, opts codexInputFilterOptions) []an } typ, _ := m["type"].(string) - // chatgpt.com codex backend (OAuth path) does not persist reasoning - // items because applyCodexOAuthTransform forces store=false. Any rs_* - // reference replayed in input is guaranteed to 404 upstream - // ("Item with id 'rs_...' not found"). Drop reasoning items entirely. + // chatgpt.com codex (OAuth path) runs with store=false (forced by + // applyCodexOAuthTransform). Replaying a reasoning item with its rs_* + // id but no encrypted_content 404s upstream ("Item with id 'rs_...' + // not found") — the 404 is triggered by the id lookup, not by the + // reasoning item itself. So strip the id (always, independent of + // PreserveReferences) yet keep the item: under store=false + // encrypted_content is the official channel for carrying reasoning + // context across turns, and dropping the whole item silently degrades + // multi-turn agent reasoning. Preserve encrypted_content/content/ + // summary and every other field verbatim. Upstream additionally + // requires a summary field — a missing one is rejected with 400 + // "Missing required parameter 'input[N].summary'" — so backfill an + // empty array when it is absent. Contracts verified end-to-end against + // chatgpt.com codex (gpt-5.5); see issue #1957. + // compaction_summary items (cmp_*) are the other encrypted_content + // carrier. Verified against the live backend: they require + // encrypted_content (a missing one is rejected with 400), and with it + // present the cmp_* id does not 404 whether kept or stripped. Being + // neither reasoning nor tool calls, they flow through the generic path + // below (id stripped when !PreserveReferences, encrypted_content + // preserved either way), which is safe and needs no special-casing. if typ == "reasoning" { + newItem := make(map[string]any, len(m)) + for key, value := range m { + if key == "id" { + // rs_* id replayed under store=false 404s; strip it. + continue + } + newItem[key] = value + } + if summary, ok := newItem["summary"]; !ok || summary == nil { + // Upstream requires a summary field; an empty array satisfies it. + newItem["summary"] = []any{} + } + filtered = append(filtered, newItem) continue } diff --git a/backend/internal/service/openai_codex_transform_test.go b/backend/internal/service/openai_codex_transform_test.go index e9157377f6..d3bdc0a52e 100644 --- a/backend/internal/service/openai_codex_transform_test.go +++ b/backend/internal/service/openai_codex_transform_test.go @@ -1343,23 +1343,24 @@ func TestIsInstructionsEmpty(t *testing.T) { } } -func TestFilterCodexInput_DropsReasoningItemsRegardlessOfPreserveReferences(t *testing.T) { - // Reasoning items in input[] reference rs_* IDs that were emitted by - // chatgpt.com under store=false (forced by applyCodexOAuthTransform). - // They are never persisted upstream, so forwarding them produces a - // guaranteed 404 ("Item with id 'rs_...' not found"). Drop them - // regardless of preserveReferences. See: Wei-Shaw/sub2api issue #1957. - +// TestFilterCodexInput_PreservesReasoningStripsID covers the core OAuth-path +// reasoning contract (replaces the earlier "drops reasoning" test, whose +// premise was wrong). A reasoning item carrying encrypted_content is the +// official channel for replaying reasoning context across turns under +// store=false, so it must survive the filter with encrypted_content intact; +// only its rs_* id is stripped (always, independent of PreserveReferences) +// because a bare rs_* id replayed under store=false 404s upstream. Contracts +// 1/2/3, verified end-to-end against chatgpt.com codex (gpt-5.5). See issue +// #1957. +func TestFilterCodexInput_PreservesReasoningStripsID(t *testing.T) { build := func() []any { return []any{ - map[string]any{"type": "message", "id": "msg_0", "role": "user", "content": "hi"}, map[string]any{ - "type": "reasoning", - "id": "rs_0672f12450da0b9c0169f07220a6c08198b68c2455ced99344", - "summary": []any{}, + "type": "reasoning", + "id": "rs_0672f12450da0b9c0169f07220a6c08198b68c2455ced99344", + "encrypted_content": "gAAAAAB-enc-payload", + "summary": []any{}, }, - map[string]any{"type": "function_call", "id": "fc_1", "call_id": "call_1", "name": "tool"}, - map[string]any{"type": "function_call_output", "call_id": "call_1", "output": "{}"}, } } @@ -1367,31 +1368,175 @@ func TestFilterCodexInput_DropsReasoningItemsRegardlessOfPreserveReferences(t *t preserve := preserve t.Run(fmt.Sprintf("preserveReferences=%v", preserve), func(t *testing.T) { filtered := filterCodexInput(build(), preserve) + require.Len(t, filtered, 1) + item, ok := filtered[0].(map[string]any) + require.True(t, ok) + // Contract 2: the reasoning item survives the filter. + require.Equal(t, "reasoning", item["type"]) + // Contract 2: encrypted_content (cross-turn channel) preserved verbatim. + require.Equal(t, "gAAAAAB-enc-payload", item["encrypted_content"]) + // Contract 1/3: rs_* id stripped unconditionally, even when + // PreserveReferences=true (id lookup, not the item, triggers the 404). + _, hasID := item["id"] + require.False(t, hasID) + // summary passed through untouched. + summary, ok := item["summary"].([]any) + require.True(t, ok) + require.Len(t, summary, 0) + }) + } +} + +// TestFilterCodexInput_BareReasoningStripsIDBackfillsSummary covers contract 1 +// plus 5: a reasoning item carrying only an rs_* id (no encrypted_content) is +// kept as an empty shell with the id stripped, and a missing summary is +// backfilled to [] so upstream does not reject it with 400 "Missing required +// parameter 'input[N].summary'". Verified against chatgpt.com codex (gpt-5.5). +func TestFilterCodexInput_BareReasoningStripsIDBackfillsSummary(t *testing.T) { + input := []any{ + map[string]any{ + "type": "reasoning", + "id": "rs_0672f12450da0b9c0169f07220a6c08198b68c2455ced99344", + }, + } + + filtered := filterCodexInput(input, false) + require.Len(t, filtered, 1) + + item, ok := filtered[0].(map[string]any) + require.True(t, ok) + require.Equal(t, "reasoning", item["type"]) + // Contract 1: id stripped. + _, hasID := item["id"] + require.False(t, hasID) + // Contract 5: summary backfilled to an empty array. + summary, ok := item["summary"].([]any) + require.True(t, ok) + require.Len(t, summary, 0) +} + +// TestFilterCodexInput_ReasoningBackfillsMissingSummary isolates contract 5: +// even when a reasoning item carries other content (here encrypted_content), +// a missing summary field is always added as [] before forwarding upstream. +func TestFilterCodexInput_ReasoningBackfillsMissingSummary(t *testing.T) { + input := []any{ + map[string]any{ + "type": "reasoning", + "id": "rs_abc", + "encrypted_content": "gAAAAAB-enc", + }, + } + + filtered := filterCodexInput(input, false) + require.Len(t, filtered, 1) + + item, ok := filtered[0].(map[string]any) + require.True(t, ok) + summary, ok := item["summary"].([]any) + require.True(t, ok) + require.Len(t, summary, 0) + // encrypted_content still preserved alongside the backfilled summary. + require.Equal(t, "gAAAAAB-enc", item["encrypted_content"]) +} + +// TestFilterCodexInput_PreservesReasoningSummaryAndContent verifies that a +// non-empty summary is not overwritten and that arbitrary reasoning fields +// (e.g. content) survive verbatim — only the id is removed. +func TestFilterCodexInput_PreservesReasoningSummaryAndContent(t *testing.T) { + summary := []any{ + map[string]any{"type": "summary_text", "text": "Considered the options."}, + } + content := []any{ + map[string]any{"type": "reasoning_text", "text": "internal chain"}, + } + input := []any{ + map[string]any{ + "type": "reasoning", + "id": "rs_abc", + "summary": summary, + "content": content, + "encrypted_content": "gAAAAAB-enc", + }, + } + + filtered := filterCodexInput(input, false) + require.Len(t, filtered, 1) + + item, ok := filtered[0].(map[string]any) + require.True(t, ok) + // Non-empty summary preserved verbatim (not replaced with []). + require.Equal(t, summary, item["summary"]) + // content preserved verbatim. + require.Equal(t, content, item["content"]) + require.Equal(t, "gAAAAAB-enc", item["encrypted_content"]) + _, hasID := item["id"] + require.False(t, hasID) +} + +// TestFilterCodexInput_PreservesReasoningInMixedInput exercises contract 7: +// reasoning items are stripped of their rs_* ids but kept (with +// encrypted_content) while message / function_call / function_call_output +// items flow through unchanged, with tool-call pairing (call_id) intact. +func TestFilterCodexInput_PreservesReasoningInMixedInput(t *testing.T) { + build := func() []any { + return []any{ + map[string]any{"type": "message", "id": "msg_0", "role": "user", "content": "hi"}, + map[string]any{ + "type": "reasoning", + "id": "rs_1", + "encrypted_content": "gAAAAAB-enc-1", + "summary": []any{}, + }, + map[string]any{ + "type": "reasoning", + "id": "rs_2", + "summary": []any{}, + }, + // call_id already in fc_ form so the unrelated call_->fc_ + // normalization does not obscure the pairing assertion. + map[string]any{"type": "function_call", "id": "fc_1", "call_id": "fc_1", "name": "tool", "arguments": "{}"}, + map[string]any{"type": "function_call_output", "call_id": "fc_1", "output": "{}"}, + } + } + + for _, preserve := range []bool{true, false} { + preserve := preserve + t.Run(fmt.Sprintf("preserveReferences=%v", preserve), func(t *testing.T) { + filtered := filterCodexInput(build(), preserve) + // Nothing is dropped: both reasoning items are now preserved. + require.Len(t, filtered, 5) + + byType := make(map[string][]map[string]any) for _, raw := range filtered { item, ok := raw.(map[string]any) require.True(t, ok) - require.NotEqual(t, "reasoning", item["type"], - "reasoning items must be dropped from input on the OAuth path") + typ, _ := item["type"].(string) + byType[typ] = append(byType[typ], item) + // No surviving item may carry an rs_* id. if id, ok := item["id"].(string); ok { require.False(t, strings.HasPrefix(id, "rs_"), "no item carrying an rs_* id should survive the filter") } } - // Sanity check: the non-reasoning items should still be present. - gotTypes := make(map[string]int) - for _, raw := range filtered { - item, ok := raw.(map[string]any) - require.True(t, ok) - typ, ok := item["type"].(string) - require.True(t, ok) - gotTypes[typ]++ + // Both reasoning items kept, ids stripped, summary present. + require.Len(t, byType["reasoning"], 2) + for _, r := range byType["reasoning"] { + _, hasID := r["id"] + require.False(t, hasID) + _, hasSummary := r["summary"] + require.True(t, hasSummary) } - require.Equal(t, 1, gotTypes["message"]) - require.Equal(t, 1, gotTypes["function_call"]) - require.Equal(t, 1, gotTypes["function_call_output"]) - require.Equal(t, 0, gotTypes["reasoning"]) + require.Equal(t, "gAAAAAB-enc-1", byType["reasoning"][0]["encrypted_content"]) + + // message / function_call(+output) untouched by reasoning handling. + require.Len(t, byType["message"], 1) + // Contract 7: tool-call pairing by call_id is unaffected. + require.Len(t, byType["function_call"], 1) + require.Equal(t, "fc_1", byType["function_call"][0]["call_id"]) + require.Len(t, byType["function_call_output"], 1) + require.Equal(t, "fc_1", byType["function_call_output"][0]["call_id"]) }) } }