fix(chatcmpl): raise ModelBehaviorError on truncated empty completions - #4513

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 738034b1a8

I filed #4510, so this is the fix for my report. Checked it out and measured rather than reading the diff.

All four empty shapes behave correctly, including the one I would have expected to slip:

finish_reason=length, content=None              -> ResponseOutputRefusal
finish_reason=length, content=""                -> ResponseOutputRefusal
finish_reason=length, content="partial answer"  -> ResponseOutputText     (no synthetic refusal)
finish_reason=length, content=" "               -> ResponseOutputText     (no synthetic refusal)

tests/models/ is 833 passed on the branch.

The third row is the one that mattered most to me and I want to say why, because it is the risk that does not exist for content_filter. Truncation is common and most truncated turns do emit content, so a guard that fired on finish_reason alone would have attached a refusal to a large share of ordinary responses. The state.text_content_index_and_output is None and ... and not state.function_calls condition is what prevents that, and it holds on both paths.

You did more than your comment claims, and it is the right call. You wrote "kept to the reported path only", but the diff also forwards delta-stripped length choices through buffer_tool_call_stream and tracks saw_length in handle_stream. Without the buffering change the handler never sees the finish_reason at all, so the streaming half would have been dead code. Worth correcting the description so a reviewer does not go looking for a gap that is not there.

One thing I would want decided before this lands, not a defect in the diff.

A refusal is not just an output shape here, it is a control-flow signal. turn_resolution.py:953-979 extracts it and raises:

refusal = ItemHelpers.extract_refusal(message_items[-1].raw_item) if message_items else None
if refusal:
    refusal_error = ModelRefusalError(refusal)
    ...
    error_kind="model_refusal"
    raise refusal_error

Measured on this branch:

finish_reason=content_filter  extract_refusal -> "Response withheld by the provider's content filter."
finish_reason=length          extract_refusal -> "Response truncated because the provider's maximum token limit was reached."
                              both -> ModelRefusalError, error_kind="model_refusal"

So a run that hit its token budget now terminates as a model refusal. That is a better outcome than the silent empty turn I reported, and I do not think it should block the PR. But the two events want different handling: a content filter is a policy decision you should not retry, while a zero-token truncation is a budget problem whose usual remedy is a larger max_tokens or a continuation. Anything branching on error_kind == "model_refusal" will now treat the second as the first.

Three ways out, and the choice is the maintainers':

Leave it. An explicit, mislabelled error still beats an invisible empty turn, and the refusal text names the real cause.

Give truncation its own kind, for example a model_truncated alongside model_refusal in RunErrorHandlerKind. Larger, and it touches the public error surface.

Keep the synthesized output but let turn_resolution distinguish the two, so the shape stays uniform and only the raised error differs.

I lean toward the first for this PR and the second as a follow-up, because the finish_reason is the thing ModelResponse still does not carry, which was the other half of what I reported. Option 1 in my issue, surfacing finish_reason, would make that distinction available without guessing from the refusal string.

@seratch flagging the semantics rather than the code, since the diff itself is faithful to the content_filter precedent it copies.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad2e9141e8

Thanks for addressing this. I verified the behavior against real Chat Completions APIs, and the underlying issue is real and provider-agnostic:

• OpenAI gpt-5.4 returned finish_reason="length" with content="" when reasoning consumed the completion budget.
• A few providers on OpenRouter returned finish_reason="length" with content=null.
• In every successful raw response, the provider's refusal field was null.

Because these responses represent token or reasoning-budget exhaustion, converting them into ResponseOutputRefusal misclassifies them as policy refusals. It would also incorrectly route them through model_refusal handlers and potentially raise ModelRefusalError.

Please surface a non-refusal error such as ModelBehaviorError when Chat Completions terminates with finish_reason="length" without final assistant text, a tool call, or an actual refusal. The handling should:

• cover both content is None and content == "";
• behave consistently in streaming and non-streaming paths;
• preserve genuine provider refusals when the upstream refusal field is populated;
• include regression tests confirming that token exhaustion does not produce ResponseOutputRefusal or invoke refusal handling.

The behavior needs fixing, but manufacturing a refusal is not the correct semantic representation.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 527400155a

Thanks for reworking this. The core semantics are now correct: an empty finish_reason="length" terminal represents token exhaustion, so ModelBehaviorError is preferable to ResponseOutputRefusal.

Before merge, please address two remaining points:

1. Gate the new length-error behavior behind an internal ChatCmplStreamHandler option enabled only by OpenAIChatCompletionsModel. The shared handler also serves LiteLLM and AnyLLM, whose non-streaming paths remain unchanged; #4514 was closed because that extension need was not demonstrated.
2. Preserve the available request and token usage on the generation span before raising, for both streaming and non-streaming calls, with focused tracing assertions.

Please also update the title and description so they describe the current non-refusal error behavior rather than the superseded refusal implementation.

Checked fb3e0b14 against all three points. They are addressed, and the gating claim holds structurally rather than only by intent.

I grepped every caller of handle_stream in src/ rather than trusting the description, since "LiteLLM and AnyLLM are unchanged" is exactly the kind of claim that quietly stops being true when a fourth caller appears:

src/agents/extensions/models/any_llm_model.py:752
src/agents/extensions/models/litellm_model.py:430
src/agents/models/openai_chatcompletions.py:476

Three callers, and only the third passes raise_on_length_truncation=True. litellm_model.py calls it with response, stream, model=self.model and nothing else, so it takes the False default and the length terminal keeps producing an empty turn there. That is the behaviour I was worried about breaking.

The usage preservation covers both branches, which was the half I expected to be missed:

if usage is not None:
    response.usage = cls._build_response_usage(usage)
else:
    _mark_request_completed_without_usage(response)

so a provider that reports no usage still gets the request recorded on the span rather than the span being abandoned mid-flight.

One optional nit, take it or leave it. raise_on_length_truncation is positional-or-keyword, since handle_stream has no * in its signature. For a flag whose entire purpose is that exactly one caller may enable it, making the tail keyword-only would enforce that mechanically instead of relying on the docstring. Nothing can hit it accidentally today, because it would take six positional arguments to reach, so this is hygiene rather than a defect.

Nothing else from me. The model_refusal question we set aside is still the right follow-up rather than part of this.