fix(openai): handle done-only/content-part responses

[aShanki]

Summary

• harden OpenAI Codex response stream parsing for done-only/content-part event shapes
• emit assistant text fallback when only done/completed payloads contain text
• emit function-call fallback for done-only tool call outputs
• add regression tests for done-only/content-part and tool-call fallback paths

Why

Some Codex/Spark responses can omit expected delta events, which caused:
Unexpected API Response: The language model did not provide any assistant messages.

Testing

• pnpm vitest run api/providers/__tests__/openai-codex-native-tool-calls.spec.ts (in src)
• pnpm vitest run api/providers/__tests__/openai-codex.spec.ts api/providers/__tests__/openai-codex-native-tool-calls.spec.ts (in src)

Start a new Roo Code Cloud session on this branch

[ghost]

PR Review: fix(openai-codex): handle done-only/content-part responses

Overall Assessment

Is this PR sound? Mostly yes. The core problem is well-defined: some Codex/Spark API variants omit expected delta events and only provide assistant text in response.output_item.done or response.completed payloads, causing the "no assistant messages" error. The fallback approach using sawTextOutputInCurrentResponse to conditionally emit text from done events is reasonable and the guard against duplicate tool calls via streamedToolCallIds is well thought out. Tests cover the new code paths. However, there are a few issues worth addressing.

---

Findings

P1 (High) -- content_part.added/done handlers risk double-emission

In both openai-codex.ts and openai-native.ts, the new response.content_part.added and response.content_part.done handlers emit text unconditionally -- they do not check !this.sawTextOutputInCurrentResponse before yielding.

Compare the response.text.done / response.output_text.done handler, which correctly guards:

if (!this.sawTextOutputInCurrentResponse && doneText) { ... }

But response.content_part.added just emits:

if (partText) {
    this.sawTextOutputInCurrentResponse = true
    yield { type: "text", text: partText }
}

If a model sends both streaming deltas AND a content_part.added event, text will be emitted twice. This should have the same !this.sawTextOutputInCurrentResponse guard unless there is an explicit reason content_part events are always unique (if so, that reasoning should be documented in a comment).

---

P2 (Medium) -- Scope creep: PR modifies openai-native.ts despite the title targeting only openai-codex

The PR title is fix(openai-codex) but identical changes (+119/-9) are applied to openai-native.ts. The body references "Codex/Spark" but the native handler serves all standard OpenAI API key users (GPT-4o, o1, etc.). If the intent is to also harden the native handler, the title/scope should reflect that (e.g., fix(openai): handle done-only/content-part responses). If the native handler changes are precautionary, they deserve a brief note explaining why they are needed there too.

---

P2 (Medium) -- Raw stream fallback in openai-native.ts still treats new event types as no-ops

Lines ~1054-1061 in openai-native.ts still classify response.output_item.done, response.content_part.added, and response.content_part.done as "Status events - no action needed" in the SSE/raw-stream fallback path. The new fallback logic only lives in processEvent() (SDK path). If a model that needs these fallbacks is accessed through the raw stream path, the fix would not apply. Either the raw-stream path should get matching fallback logic, or a comment should explain why it is not needed there.

---

P2 (Medium) -- Significant code duplication between the two handlers

The processEvent() methods in openai-codex.ts and openai-native.ts are nearly identical. This PR adds ~100 lines of identical fallback logic to both. This is a substantial DRY concern that increases the maintenance burden and the risk of the two implementations drifting. Consider extracting shared event-processing logic into a common base or utility. (Not necessarily blocking for this PR, but worth flagging.)

---

P3 (Low) -- Missing changeset

No .changeset/ entry is included. This is a user-facing bug fix that should appear in the changelog.

---

P3 (Low) -- Tests do not cover the normal (non-fallback) path to verify no regression

The new tests all exercise the fallback paths (done-only, completed-only, content_part). There are no new tests verifying that the normal streaming delta path still works correctly with sawTextOutputInCurrentResponse set -- i.e., confirming that done events do NOT double-emit when deltas were already streamed. A regression test for the happy path would strengthen confidence.

---

Summary

Severity	Count	Key Issue
P1	1	content_part handlers missing double-emission guard
P2	3	Scope creep to native handler; raw-stream path inconsistency; code duplication
P3	2	Missing changeset; no regression test for happy path

The fix addresses a real user-facing bug and the approach is sound. The P1 regarding content_part double-emission should be addressed before merge. The P2 items are worth discussion but are not blockers if the team is comfortable with the current scope.

[aShanki]

Addressed review feedback in ae6589e:\n\n- Added duplicate guards for content_part fallback text in both providers by treating content_part as fallback-only when text deltas were already observed (sawTextDeltaInCurrentResponse).\n- Added regression tests in both provider suites for non-fallback streaming paths to ensure no double-emission when deltas and done/content_part events are both present.\n- Clarified raw SSE fallback flow in openai-native and removed stale no-op status classification for output_item.done / content_part.* (those events are now delegated through coreHandledEventTypes to processEvent).\n- Added a changeset for the user-facing bugfix.\n- Updated PR title to reflect scope (ix(openai)).\n\nI left the shared-event-parser extraction as follow-up work to keep this bugfix PR surgical.

[ghost]

Re-review: All P0/P1 issues resolved

The latest commit addresses the findings from the initial review:

P1 (content_part double-emission) -- Fixed. A new sawTextDeltaInCurrentResponse flag precisely tracks whether text arrived through streaming deltas. The content_part.added/done handlers now correctly guard with !this.sawTextDeltaInCurrentResponse before emitting, preventing double-emission when both deltas and content_part events are sent by a model. This is a clean separation of concerns -- sawTextDeltaInCurrentResponse guards content_part fallbacks, while sawTextOutputInCurrentResponse guards done-event and completed-event fallbacks.

P2 (raw-stream no-op) -- Fixed. response.output_item.done, response.content_part.added, and response.content_part.done have been removed from the "Status events - no action needed" block in the SSE/raw-stream fallback path. These event types are now in coreHandledEventTypes and routed to processEvent(), so the fallback logic applies consistently across both SDK and raw-stream paths.

P3 (changeset) -- Fixed. A changeset entry (.changeset/sly-candles-hide.md) is now included.

P3 (regression tests) -- Fixed. New tests verify that text is NOT duplicated when both delta and content_part events are emitted (in both codex and native handlers).

The remaining P2 items (scope naming in PR title, code duplication between handlers) are noted but not blocking. Approving.

[hannesrudolph]

Reviewed by Rooviewer (Task #72955). The P1 double-emission bug is successfully fixed with the sawTextDeltaInCurrentResponse guard. Tests cover the regression path. Approving.