Skip to content

fix(llm): split OpenAI reasoning summary blocks - #29000

Merged
rekram1-node merged 12 commits into
devfrom
fix-openai-reasoning
May 24, 2026
Merged

rekram1-node merged 12 commits into
devfrom
fix-openai-reasoning

Conversation

@rekram1-node

@rekram1-node rekram1-node commented May 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • preserve OpenAI Responses reasoning summary boundaries by translating summary_index into distinct native reasoning block IDs
  • mirror @ai-sdk/openai lifecycle handling for summary-part completion and encrypted continuation metadata
  • automatically request encrypted reasoning state for stateless OpenAI reasoning models and replay it on follow-up requests
  • send stored OpenAI reasoning items back as item_reference values, matching SDK behavior when store: true

Testing

  • bun test test/provider/openai-responses.test.ts && bun typecheck from packages/llm
  • bun test && bun typecheck from packages/llm
  • bun test test/session/llm-native.test.ts && bun typecheck from packages/opencode
  • git diff --check
  • bun run lint packages/llm/src/protocols/openai-responses.ts packages/llm/src/protocols/utils/openai-options.ts packages/llm/test/provider/openai-responses.test.ts packages/opencode/test/session/llm-native.test.ts

- Mirror official ResponseIncludable union from openai-node SDK as the
  single source of truth for OpenAIResponseIncludables. Export a typed
  Schema and a string Set for filtering.
- Remove the duplicate includeEncryptedReasoning boolean alias. Callers
  pass include: ReadonlyArray<OpenAIResponseIncludable> directly.
- Tighten the wire schema in openai-responses.ts to the full SDK union
  instead of a single hand-coded literal.
- Re-export the type from providers/openai-options.ts so existing
  facade consumers keep the same import surface.
- Update internal test scenarios to use the canonical include array.

No behavioral change at the wire level; the only previously-accepted
input shape (the boolean alias) is now unsupported in favor of the
schema-aligned array.
…ptions

The native @opencode-ai/llm SDK keeps two ways to request encrypted
reasoning state, which serve different audiences:

- include: ReadonlyArray<OpenAIResponseIncludable> is the canonical
  OpenAI Responses wire field. Mirrors the official SDK union and is
  what opencode emits via ProviderTransform.options when routing
  through the native runtime.
- includeEncryptedReasoning: boolean is a native-SDK-only convenience
  for direct @opencode-ai/llm callers that only need the encrypted
  reasoning category. Explicit include wins when both are set.

OpenAIOptions.include() filters incoming arrays against the official
ResponseIncludable union so a typo in upstream config silently drops
the invalid entry instead of poisoning the request.

Re-export OpenAIResponseIncludable from the OpenAI provider facade so
external consumers can spell the union without reaching into protocol
internals.

Document the AI SDK -> native pass-through in native-runtime.ts so the
boundary is visible: provider options share field names with the
official OpenAI wire shape, so this is a pass-through, not a
translation. Tests cover the union, filtering, the boolean alias, and
the explicit-include precedence.
- Require non-empty itemId in OpenAI lowerReasoning so the store-true
  and store-false branches behave consistently.
- Hoist ["reasoning.encrypted_content"] into a single INCLUDE_ENCRYPTED_REASONING
  constant in opencode's transform.ts; five branches used to repeat the
  literal.
- Document the OpenAI Responses reasoning-event ordering assumption above
  the handler set so the implicit contract is visible.
- Replace Object.fromEntries(entries.filter(...)) with a destructure-rest
  in onOutputItemDone reasoning cleanup.
@rekram1-node

Copy link
Copy Markdown
Collaborator Author

/review

…ning for GPT-5

Address two PR review findings:

1. Include precedence: `OpenAIOptions.include` previously fell through to
   the `includeEncryptedReasoning` alias when an explicit `include` array
   filtered to empty. That made `{ include: [], includeEncryptedReasoning:
   true }` and `{ include: ["bogus"], includeEncryptedReasoning: true }`
   silently re-enable encrypted reasoning the caller had explicitly opted
   out of. Treat any explicit `include` (including empty or all-invalid)
   as opting out of the alias.

2. GPT-5 default include: the native OpenAI facade configures GPT-5
   reasoning models stateless (`store: false`) with
   `reasoningSummary: "auto"`, but did not set `include:
   ["reasoning.encrypted_content"]` by default. Direct
   `@opencode-ai/llm` callers were getting reasoning summaries with no
   encrypted state, which cannot be replayed on a follow-up turn. Set
   the include automatically in `gpt5DefaultOptions`; callers can still
   override with their own `include` value (including `[]` to opt out).

Tests cover the empty/invalid include opt-out, the alias semantics, and
the GPT-5 default. Re-recorded the two affected golden cassettes
(`openai-responses-gpt-5-5-reasoning`, `openai-responses-gpt-5-5-image-tool-result`,
and the touched `openai-responses-gpt-5-5-reasoning-continuation`) so
they now carry the auto-included `reasoning.encrypted_content` in their
request bodies.
…the only API

The native OpenAI options accepted two ways to request encrypted
reasoning (`include` array and `includeEncryptedReasoning` boolean),
which forced a precedence rule and surfaced edge cases like `include:
[]` clashing with `includeEncryptedReasoning: true`. There should
either be one option with a translation layer between AI SDK options
and native LLM options, or both sides should share the same array
shape with the translation layer being identity. Choose the latter:

- Remove `includeEncryptedReasoning` from OpenAIOptionsInput entirely.
- `OpenAIOptions.include` is now a pure filter over the official
  ResponseIncludable union; an empty or all-invalid array returns
  undefined.
- Update tests and docs accordingly. The AI SDK -> native boundary in
  native-runtime.ts is identity (both surfaces use OpenAI's wire field
  names verbatim); if a future field needs translation, that's the
  one place it belongs.
The field was removed from OpenAIOptionsInput in the previous commit,
but openAIProviderOptions still read `options?.includeEncryptedReasoning`
into the emitted providerOptions.openai map. The index signature on
OpenAIOptionsInput meant this compiled silently — a caller upgrading
with a stale { includeEncryptedReasoning: true } would forward the
value to a key nothing reads. Delete the line.

Also tighten summaryParts to Record<number, ReasoningSummaryStatus>
since every write uses event.summary_index (a number).
@rekram1-node

Copy link
Copy Markdown
Collaborator Author

/review

openaiMetadata({ itemId: item.id, reasoningEncryptedContent: item.encrypted_content ?? null })

// OpenAI Responses streams reasoning items in a stable order:
// `output_item.added` (reasoning) →

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion only: this added comment introduces a non-ASCII arrow, while the repo style guide defaults edits to ASCII unless there is a clear reason. The human reviewer can decide, but consider using -> here to keep the comment within the style guide.

// keys via OpenAIOptions.* (store, reasoningEffort, reasoningSummary,
// include, textVerbosity, promptCacheKey). Both sides intentionally use
// OpenAI's official wire field names, so this is identity, not translation
// — if a field ever needs to differ between the two surfaces, the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion only: this added comment introduces an em dash, while the repo style guide defaults edits to ASCII unless there is a clear reason. The human reviewer can decide, but replacing it with a plain sentence like If a field ever needs... would keep the comment within the style guide.

…y drop

Two narrow changes prompted by the latest review pass — both verified
against the surrounding code, not added arbitrarily:

- reasoningStart now calls stepStart unconditionally before checking
  whether the reasoning id is already in state. textDelta, textEnd,
  and reasoningEnd already do this; reasoningStart was the outlier.
  stepStart is idempotent so the change is a no-op for all existing
  callers, but it prevents a hypothetical future protocol whose first
  emitted event is a reasoningDelta from being missing a step-start.
  This is internal helper hygiene; it does not change anything on the
  wire and adds no OpenAI-imposed constraint.

- Add a regression test asserting that a reasoning part with an itemId
  but no encrypted state is dropped under store:false. This codifies
  behaviour the protocol already enforces and that the surrounding
  comment in lowerMessages documents: "With store:false, OpenAI only
  accepts previous reasoning items when the encrypted state is
  present." Complements the existing reasoningEncryptedContent: null
  case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant