providers: surface non-normal terminal turns instead of silent empty completions

[gnanam1990]

Summary

Four provider stream-adapter bugs where a non-normal terminal turn was treated as a clean, empty completion — so a dropped tool call, an upstream failure, a content block, or a refusal looked like the model simply returned nothing. Surfaced by a full-tree audit; each ships a regression test.

Fixes

Adapter	Bug	Fix
Responses (codex)	A tool call with output_index 0 and no item_id had its argument deltas dropped — the tool ran with empty/partial JSON. output_index was an int, so a real 0 was indistinguishable from "absent".	output_index is now a *int; toolCallKey keys on it even when 0.
Responses (codex)	response.failed / response.completed with a nil payload returned with no terminal event — the collector saw a clean empty completion that masked a real failure.	failed emits StreamEventError, completed emits StreamEventDone.
Gemini	Terminal finishReasons (RECITATION, MALFORMED_FUNCTION_CALL, IMAGE_SAFETY, OTHER, LANGUAGE, UNEXPECTED_TOOL_CALL) mapped to a normal stop → silent empty/truncated turn.	Safety reasons → content_filter; remaining non-STOP reasons surface the raw reason so the "ended early (…)" notice fires. Only ""/STOP/FINISH_REASON_UNSPECIFIED are normal.
Anthropic	stop_reason: refusal treated as a normal completion → silent empty turn.	refusal → content_filter.

Tests

TestToolCallKeyOutputIndexZero, TestHandleTerminalResponseNilPayload, TestMapFinishReasonNonNormal, TestMapStopReasonRefusal.

go build ./..., go vet ./..., gofmt, and the full go test ./... are all green.

Summary by CodeRabbit

• Bug Fixes
  • Improved streaming terminal-event handling so missing or incomplete failure details still produce correct error signaling and always stop cleanly.
  • Fixed tool-call correlation for terminal SSE events, especially when output indices include zero.
• Refactor
  • Standardized finish/stop reason mapping to more accurately distinguish normal completions from content-filtered and length-limited outcomes.
• Tests
  • Added/updated unit and integration tests to cover refined reason mappings and terminal-event edge cases.

[coderabbitai]

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Free

Run ID: 223fa3d3-166f-4761-b7d3-f45da52455ef

📥 Commits

Reviewing files that changed from the base of the PR and between 8efb611 and 13a3b45.

📒 Files selected for processing (2)

• internal/providers/openai/codex_responses.go
• internal/providers/openai/codex_terminal_test.go

---

Walkthrough

Three provider-level correctness fixes: Anthropic's mapStopReason is converted to a switch statement with "refusal" explicitly handled. Gemini's mapFinishReason expands the normal-completion set, adds safety variants to the content-filter branch, and surfaces unknown reasons as raw strings. OpenAI Codex fixes OutputIndex zero-value ambiguity via a pointer type, closes nil-dereference risks in handleTerminalResponse, and corrects toolCallKey indexing. Unit tests are added or updated for all three.

Changes

Provider finish/stop reason fixes

Layer / File(s)	Summary
Anthropic mapStopReason switch + test internal/providers/anthropic/provider.go, internal/providers/anthropic/stop_reason_test.go	mapStopReason is converted from an if chain to a switch that explicitly handles "max_tokens" and "refusal". New test covers both mapped constants and all normal stop reasons.
Gemini mapFinishReason expansion + test refactor internal/providers/gemini/provider.go, internal/providers/gemini/finish_reason_test.go	mapFinishReason adds "FINISH_REASON_UNSPECIFIED" to the normal set, adds "RECITATION" and "IMAGE_SAFETY" to the content-filter branch, and returns the raw reason string for unrecognized values instead of silently returning empty. The test file replaces its old streaming integration tests with a focused unit mapping test and a new RECITATION end-to-end assertion.
Codex OutputIndex pointer type and toolCallKey fix internal/providers/openai/codex_responses.go, internal/providers/openai/codex_terminal_test.go	responsesEvent.OutputIndex changes from int to *int to disambiguate a real zero index from an absent one. toolCallKey is updated to check pointer presence (including 0) and drop the prior offset-by-one logic. Tests cover output_index==0, absent index, and item_id priority.
Codex handleTerminalResponse nil payload and missing error safety internal/providers/openai/codex_responses.go, internal/providers/openai/codex_terminal_test.go	handleTerminalResponse now explicitly handles a nil event.Response by emitting StreamEventError for response.failed and StreamEventDone for non-failed terminals. It also treats response.failed events with missing/null error details as an error condition instead of falling through to a clean completion. Tests assert proper error emission for both nil payloads and missing error details.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

---

Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: 13a3b4525160
Changed files (6): internal/providers/anthropic/provider.go, internal/providers/anthropic/stop_reason_test.go, internal/providers/gemini/finish_reason_test.go, internal/providers/gemini/provider.go, internal/providers/openai/codex_responses.go, internal/providers/openai/codex_terminal_test.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[Vasanthdev2004]

Review: providers — surface non-normal terminal turns instead of silent empty completions

Verdict direction

The core fixes are sound and well-motivated. Three real bugs are addressed:

• M1 (Codex): OutputIndex was a plain int defaulting to 0, so a no-item_id call with output_index: 0 was dropped (the > 0 guard rejected it). Switching to *int distinguishes "absent" from a real first-output zero — correct fix, and toolCallKey now keys on it. Good unit coverage in TestToolCallKeyOutputIndexZero.
• M2 (Codex): handleTerminalResponse returned silently when event.Response == nil, so a response.failed with no payload became a clean-looking empty completion. Now it emits StreamEventError for failed and a clean Done for completed. Correct, and the test in TestHandleTerminalResponseNilPayload covers both branches.
• M3/M4 (Gemini/Anthropic): previously unmapped non-normal reasons (RECITATION, IMAGE_SAFETY for Gemini; refusal for Anthropic) were collapsed to "" (clean completion). Now Gemini surfaces the raw reason for anything not in the normal set, and Anthropic maps refusal to content_filter. Both are correct and align with TruncationNotice's "Response ended early ()" rendering.

Findings

P1 — None. No correctness, security, or data-loss issues in the diff.

P2

1. gofmt violation in responsesEvent struct — internal/providers/openai/codex_responses.go:118-121. The PR changes OutputIndex from int to *int, which shifts the alignment column, but ItemID's tag was not re-aligned. gofmt -d flags this:

   -    ItemID string `json:"item_id,omitempty"`
   +    ItemID      string       `json:"item_id,omitempty"`
   

   This will fail gofmt -l / lint in CI. The other struct lines (OutputIndex, Item) are already misaligned in the same way in the PR snippet, so the whole block needs re-running through gofmt -w. (I verified by reconstructing the changed struct and running gofmt -d — it wants ItemID string with the extra padding to match the new widest type *int/*itemPayload.)

P3

2. Gemini: significant end-to-end test coverage is deleted and not replaced — internal/providers/gemini/finish_reason_test.go. The PR replaces 145 lines with a single unit test (TestMapFinishReasonNonNormal) of mapFinishReason only. This deletes:

   • TestStreamCompletionSurfacesMaxTokensFinishReason
   • TestStreamCompletionSurfacesSafetyFinishReason
   • TestStreamCompletionNormalFinishReasonHasNoReason
   • TestStreamCompletionEmitsDroppedOnNamelessFunctionCallPart
   • TestStreamCompletionEmitsDroppedOnNamelessTopLevelFunctionCall
   • TestStreamCompletionDoesNotDropValidFunctionCall

   The new unit test covers mapFinishReason in isolation, but the deleted tests verified the full SSE → streamState.finishReason → Done event wiring. The M3 change widens mapFinishReason's default branch to return the raw reason, and that new behavior is exercised by the unit test — but the integration path (does a RECITATION candidate actually reach the Done event's FinishReason?) is now untested. The three ToolCallDropped tests are orthogonal to this PR's goal and their deletion removes the only coverage for the nameless-functionCall drop behavior in the Gemini provider (the production code in provider.go:248 and provider.go:265 is unchanged, so this is a net loss of coverage rather than a behavior regression). Recommendation: keep TestMapFinishReasonNonNormal and restore at least the three dropped-tool-call tests (they belong to a different feature and shouldn't ride along with this PR); consider restoring the finish-reason integration tests too, or moving them to a streaming-test file if the file name was the issue.

3. Anthropic: mapStopReason comment lists pause_turn as normal without verification — internal/providers/anthropic/provider.go:607. The new default-branch comment says pause_turn (and "") are normal. The test asserts this, but there's no citation to Anthropic's API docs that pause_turn is a non-terminal/normal reason. If pause_turn is actually an early-stop signal (e.g. for the new pause-and-resume feature), collapsing it to "" would re-introduce the silent-empty-completion bug this PR is fixing. Worth a quick doc/link confirmation, or dropping it from the "normal" list until confirmed.

4. Codex M2: response.incomplete is not a terminal event here — internal/providers/openai/codex_responses.go:494 routes only response.completed/response.failed to handleTerminalResponse; response.incomplete falls through to the default (ignored) branch. That's pre-existing behavior, but the PR's M2 comment frames the nil-payload fix as closing the "silent empty completion" hole — response.incomplete with a nil Response payload would still be silently ignored. Out of scope for this PR, but worth a follow-up note since the PR title is broad ("non-normal terminal turns").

Suggested actions

• Run gofmt -w internal/providers/openai/codex_responses.go and re-check the struct alignment (P2 #1).
• Restore the three Gemini ToolCallDropped tests (P3 #2) — they're unrelated to this PR's goal and shouldn't be deleted.
• Add a one-line citation for pause_turn being normal, or drop it from the comment (P3 #3).

Overall this is a good, focused fix; the blocking item is the gofmt issue, the rest are nits/coverage observations.

[gnanam1990]

Thanks for the thorough review — addressed in 8efb611:

P2 (gofmt — ItemID alignment). No change needed: gofmt -l internal/providers/ is clean on the current head. The ItemID field is followed by a comment line, which breaks gofmt's struct-tag alignment run, so ItemID is its own group and isn't padded to match the *int/*itemPayload widths (those two form their own aligned group). The reconstruction in the review didn't account for the comment breaking the run. Confirmed clean in CI-equivalent gofmt -l.

P3 #2 (deleted Gemini coverage) — restored. Brought back all six tests the refactor dropped, alongside the mapFinishReason unit test:

• finish-reason wiring: TestStreamCompletionSurfacesMaxTokensFinishReason, ...SurfacesSafetyFinishReason, ...NormalFinishReasonHasNoReason
• nameless-functionCall drop: ...EmitsDroppedOnNamelessFunctionCallPart, ...EmitsDroppedOnNamelessTopLevelFunctionCall, ...DoesNotDropValidFunctionCall

And added TestStreamCompletionSurfacesRecitationFinishReason so the M3 widening is exercised end-to-end through the SSE → done-event path (RECITATION → content_filter), not just in mapFinishReason.

P3 #3 (pause_turn) — justified in code. Added a comment explaining it's Anthropic's long-running-turn pause (server-side tools), which the client resumes by sending the response back — not a truncation/refusal. Mapping it to "" is correct; classifying it as non-normal would fire a spurious truncation notice. Behavior unchanged.

P3 #4 (response.incomplete) — acknowledged as a follow-up. Agreed it's pre-existing and out of scope for this PR; leaving it for a separate change so this one stays focused on the terminal completed/failed paths.

gofmt/vet/build clean; gemini/anthropic/openai suites green.

[gnanam1990]

Follow-up hardening from a self-review pass (13a3b45): found a symmetric gap to the M2 nil-payload fix — a response.failed carrying a non-nil Response whose error object is null/omitted (the backend can report the failure via status alone) fell through to a clean StreamEventDone, i.e. a real failure reported as a successful turn. Now branches on the failed terminal (event type / Response.Status) before the clean-done fallthrough and emits StreamEventError, with a regression test (TestHandleTerminalResponseFailedPayloadWithoutError) verified to fail without the fix.

[Vasanthdev2004]

Re-reviewed after 8efb611 ("address review: restore Gemini tests; justify pause_turn"). All three items from my prior CHANGES_REQUESTED are resolved, and a new-issue scan found nothing blocking. Approving.

Prior items — all resolved

1. Blocking gofmt (responsesEvent struct alignment) — fixed. gofmt -l is clean on internal/providers/openai/codex_responses.go; the doc comment on OutputIndex correctly breaks the alignment block so OutputIndex *int / Item *itemPayload align as their own group.
2. Restore the deleted Gemini tests — fixed. All six are back, and the net diff vs main is now purely additive: the original 6 plus 3 new (TestMapFinishReasonNonNormal, TestStreamCompletionSurfacesRecitationFinishReason, and the SSE-wired RECITATION integration test). Coverage is strictly better than main.
3. Justify "pause_turn is normal" — fixed. anthropic/provider.go now explains pause_turn is Anthropic's long-running-turn pause for server-side tools (client resumes by sending the response back — not a truncation/refusal), so mapping it to "" correctly avoids a spurious truncation notice. TestMapStopReasonRefusal pins pause_turn → "".

New-issue scan — clean

• The OutputIndex int → *int change is internally consistent: toolCallKey keys on *event.OutputIndex directly and drops the old +1 offset (the offset only existed to reserve 0 for "absent", which nil now expresses). TestToolCallKeyOutputIndexZero covers 0 / 2 / absent / item_id-precedence.
• Gemini's widened default: return reason is safe end-to-end — it sets state.finishReason → Done event → Result.TruncationNotice(), surfaced in exec + TUI; no raw enum leaks improperly.
• Codex nil-payload handling correctly splits response.failed (emits StreamEventError) from response.completed (clean Done); TestHandleTerminalResponseNilPayload covers both.

Thanks for the thorough turnaround on the feedback — this is a clean, well-tested fix.