Repository navigation
fix(chat): finish with length when a Responses upstream reports incomplete reason "length" - #7539
Conversation
…plete reason "length"
…at incomplete vocabulary Move the regression into tests/responses/ (where the Chat converter suites live), register it in scripts/test-layout/layout.json and tests/fixtures/test-layout-expected.json, add explicit streamed SSE wire assertions and handleChatCompletions cases over an openai-responses upstream (SSE and JSON, stream true/false), and record in structure/transports/responses-wire-shapes.md and structure/data-planes/inbound-compat.md that the Chat writer classifies through truncationReasonFor while proxy-synthesized reasons stay errors. Co-authored-by: hedging8563 <45883076+hedging8563@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: mapping a Responses upstream incomplete reason of "length" to the Chat finish reason "length". Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
✅ Deterministic PR hygiene checks passed. |
… shard scripts/test-layout/layout.json sits at 1999 lines on dev, so the inline explicit entry took it to 2000 and failed the file-size ratchet as NEW_OVERSIZED. Move the entry into a new explicitShards file, scripts/test-layout/layout-chat.json, listed on the existing shard line. Co-authored-by: hedging8563 <45883076+hedging8563@users.noreply.github.com>
Summary
Carries #7392 by @hedging8563 onto the current
devtip and addresses the CHANGES_REQUESTED review.A Responses upstream that ends a capped turn with
status: "incomplete"andincomplete_details.reason: "length"(Volcengine Ark Coding Plan/responses; OpenAI sendsmax_output_tokens) was classified by the Chat writer as an unsupported boundary.chatCompletionsIncompleteOutcomeandresponsesJsonToChatCompletioninsrc/chat/outbound.tsaccepted onlymax_output_tokensandcontent_filter, so the converter producedupstream stream ended early (length)/upstream_incompleteinstead offinish_reason: "length".chatCompletionsIncompleteOutcomenow reads the reason through the sharedtruncationReasonFortable (src/responses/truncated-stop-reason.ts).responsesJsonToChatCompletioncalls that function instead of its own two-value check, so the SSE converter, its buffered collector, the direct encoder (src/protocols/encoders/chat.ts) and the JSON projection share one classification.adapter_eof,upstream_stall_timeout,missing_terminal_event,max_messages,steered, missing) are not in the table and still fail.Scope note for reviewers: on current
dev,handleChatCompletionsover anopenai-responsesprovider already receives the normalized reason, because #7218 madesrc/server/responses/passthrough-delivery.tsapplyrewriteResponsesIncompleteReasonJsonto every upstream body. The new handler cases pass with and without this change. The fix therefore makes the Chat writer correct on its own instead of relying on every producer normalizing first; the converter cases are the red→green proof. This matches what normalized producers already deliver (for examplepause_turnandrefusalmap the same way on both sides).Review follow-ups:
tests/responses/chat-incomplete-length-reason.test.ts(the domain the other Chat converter suites use;tests/chat/does not exist) and is registered in both maps: the newexplicitShardsfilescripts/test-layout/layout-chat.json(listed on the existing shard line ofscripts/test-layout/layout.json) andtests/fixtures/test-layout-expected.json.layout.jsonis at 1999 lines ondev, and an inline entry made it 2000, which the file-size ratchet rejects asNEW_OVERSIZED(first CI run, test 2/4). The shard follows thelayout-agent-assertion.jsonprecedent from fix(auth): refuse Codex runtime AgentAssertion auth and scope dual_header assertions to Direct #7470.structure/transports/responses-wire-shapes.md#adapter-incomplete-reasonsnow states the Chat writer's vocabulary and failure boundary, andstructure/data-planes/inbound-compat.mdlinks "known incomplete limits" to it. The doc stays at its 600-line budget.max_messages/steered. Explicit streamed-wire assertions were added: the converter's SSE text and the handler'sstream:trueresponse must carry exactly onefinish_reason: "length"and end withdata: [DONE]. The handler also covers SSE and JSON upstreams for bothstream:trueandstream:falseclients.CodeRabbit reported no actionable comments, and the original PR had no inline review threads.
Not changed here:
anthropicIncompleteOutcomeinsrc/claude/outbound.tsstill compares againstmax_output_tokensdirectly, so the Messages writer still depends on upstream normalization. That writer is outside this Chat lane.Co-authored-by: hedging8563 45883076+hedging8563@users.noreply.github.com
Verification
Run on base
299e8b68b2(currentorigin/dev) with isolatedHOME/CLAUDE_CONFIG_DIR/OPENCODEX_HOME(mktemp) and test runner Bun 1.4.3.bun test tests/responses/chat-incomplete-length-reason.test.ts: 9 pass / 0 fail. Withsrc/chat/outbound.tsreverted todev, 4 converter cases fail and 5 pass (the 5 are the negative case and the 4 handler cases, which pass because of the upstream normalization described above).tests/test-layout.test.ts,tests/test-layout-tooling.test.ts, the new file,tests/responses/chat-json-sse-fallback.test.ts,tests/responses/chat-completions-endpoint.test.ts,tests/responses/protocol-direct-encoders-chat.test.ts,tests/responses/incomplete-reason-normalization.test.ts,tests/adapters/bridge-nonstreaming-terminal.test.ts): 412 pass / 0 fail.bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts tests/responses/chat-incomplete-length-reason.test.ts: 45 pass / 0 fail;layout.jsonback at 1999 lines.bun run test:changed(before the JSON-only shard move, which changes no import graph): 16397 pass / 34 skip / 0 fail across 668 files, exit 0.bun run typecheck: exit 0.bun run privacy:scan: passed.bun run structure:check: passed.windowspath filter does not match, and no added line undersrc/ortests/contains a marker fromscripts/ci/windows-sensitive-diff.sh, so nolane=alldispatch was needed.bun run testlocally. This Mac is under elevated load and shared with other lanes; the import-graphtest:changedset above covers the changed module's dependents, and PR CI covers the Ubuntu suite.Checklist
Summary by CodeRabbit
lengthnow return the Chat Completions finish reasonlengthacross streamed and non-streamed responses, including JSON and SSE formats.content_filtercontinue to return that finish reason; unrecognized incomplete reasons remain errors. This keeps Chat completion outcomes consistent across supported response paths.