Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 9 of 13: openai and claude translators - #21
ericlitman wants to merge 1 commit into
Conversation
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
🛑 5 blocking findings🔴 High · Emit document URLs under the Interactions 🟡 Medium · Reuse existing call IDs for sparse indexless deltas 🟡 Medium · Final finish chunks can lose Anthropic terminal events 🟡 Medium · Reserve historical identities when naming the shell bridge 🟡 Medium · Preserve the model role before applying drop validation WalkthroughThis review-only slice of the upstream
Since the last review
Repair promptRead the
Details
First review of this pull request. @unfret review · @unfret status · @unfret help |
![unfret[bot]](https://avatars.githubusercontent.com/in/4933690?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛑 5 blocking findings
🔴 High · Emit document URLs under the Interactions `uri` field
🟡 Medium · Reuse existing call IDs for sparse indexless deltas
🟡 Medium · Final finish chunks can lose Anthropic terminal events
🟡 Medium · Reserve historical identities when naming the shell bridge
🟡 Medium · Preserve the model role before applying drop validation
Check
| if existing, exists := param.ToolCallsAccumulator[index]; exists && existing != nil && incomingID != "" && existing.ID != "" && existing.ID != incomingID { | ||
| nextIndex := index + 1 | ||
| for k := range param.ToolCallsAccumulator { | ||
| if k >= nextIndex { | ||
| nextIndex = k + 1 | ||
| } | ||
| } | ||
| index = nextIndex |
There was a problem hiding this comment.
🟡 Medium · Reuse existing call IDs for sparse indexless deltas
When an indexless chunk initially announces calls A and B, a later indexless chunk containing only B's arguments lands at array position zero. This branch compares B against A and allocates a third accumulator instead of finding B's existing accumulator. Reproducing this with two no-argument calls followed by B's arguments="{}" emits three tool_use blocks, including a duplicate B ID with the synthesized name tool_2; the base emits only A and B.
| (param.FinishReason != "" || param.SawToolCall || param.TextContentBlockStarted || param.ThinkingContentBlockStarted || param.ContentAccumulator.Len() > 0 || len(param.InterleavedContentChunks) > 0) | ||
|
|
||
| if !param.MessageDeltaSent && (param.FinishReason != "" || isTrailingUsageChunk) && hasUsage { | ||
| if !param.MessageDeltaSent && isTrailingUsageChunk { |
There was a problem hiding this comment.
🟡 Medium · Final finish chunks can lose Anthropic terminal events
The deleted param.FinishReason != "" branch terminated streams whose final choice chunk contained both finish_reason and usage. This replacement only terminates on a separate usage-only chunk, so an OpenAI-compatible upstream that closes after that final choice chunk without [DONE] now emits neither message_delta nor message_stop, leaving Anthropic clients with an unterminated response; no EOF finalizer for this state is visible here. Also reported for this defect: - message_stop now depends on [DONE] when usage rides on the finish chunk (internal/translator/openai/claude/openai_claude_response.go:454): Before this diff, message_delta and message_stop were emitted on the finish_reason chunk whenever that chunk carried usage ((FinishReason != "" || isTrailingUsageChunk) && hasUsage). The new condition at this line fires only for a usage-only chunk with no choices. Otherwise emission waits for convertOpenAIDoneToAnthropic. Failure scenario: an OpenAI-compatible upstream sends its last chunk with finish_reason and usage, then closes the stream without data: [DONE]. The Claude client now receives content blocks but no message_delta (no stop_reason, no output usage) and no message_stop. Claude Code then sees a null stop_reason and does not execute the emitted tool_use calls, so the agent turn stalls. Nothing in the visible diff synthesizes a [DONE] at EOF for ConvertOpenAIResponseToAnthropicParams. Such synthesis may exist in the executor outside this diff. - Keep EOF finalization compatible with bare terminators (internal/translator/openai/claude/openai_claude_response.go:454): Removing the param.FinishReason != "" completion case breaks Kimi execution with SourceFormat=OpenAI and ResponseFormat=Claude when upstream ends after a finish-reason-plus-usage chunk. Kimi's EOF finalizer supplies bare [DONE], which this translator rejects at its data: prefix check. Execution now closes successfully without either message_delta or message_stop; the same sequence emitted both terminal events in the base.
| out, _ = sjson.SetBytes(out, "file_url", fileURL) | ||
| hasContent = true |
There was a problem hiding this comment.
🔴 High · Emit document URLs under the Interactions uri field
For an input_file containing only file_url, this mapper emits a document with file_url and reports it as sendable. Native Interactions DocumentContent uses uri, not file_url; the Gemini Interactions executors forward this payload without remapping that field. The resulting document has neither inline data nor a recognized URI, so a valid URL attachment produces an unusable upstream request. Also reported for this defect: - Translate document URLs to the Interactions uri field (internal/translator/openai/interactions/responses/interactions_openai_responses_request.go:428): An input_file containing only file_url becomes a document part with file_url, and this helper reports it as sendable. Native Gemini Interactions expects the remote location in uri, not file_url. No downstream normalization rewrites this key, leaving the document without either data or uri and causing an upstream validation failure. This also breaks otherwise valid requests containing text alongside the URL attachment.
Also affects:
| for _, d := range declarations { | ||
| if !d.shell { | ||
| reserved[d.localName] = true | ||
| reserved[d.chatName] = true | ||
| reserved[rawResponsesNamespaceQualifiedName(d.namespace, d.localName)] = true |
There was a problem hiding this comment.
🟡 Medium · Reserve historical identities when naming the shell bridge
This reservation scans current declarations only. If history contains a now-withdrawn user function named __cpa_local_shell and the current request adds a local shell tool, the bridge takes that same name. Request translation then emits both the historical non-shell call and rewritten shell calls with identical function.name values, attributing unrelated calls and arguments to one tool identity. The replay-only naming path already reserves historical calls, but this declared-shell path does not.
| } | ||
| wrapped, _ = sjson.SetRawBytes(wrapped, "content", []byte(step.Get("parts").Raw)) | ||
| appendInteractionsStepToClaude(accumulator, gjson.ParseBytes(wrapped), "user") | ||
| appendInteractionsStepToClaude(accumulator, gjson.ParseBytes(wrapped), "user", translatorcommon.IsInteractionsInstructionStep(step, false), run) |
There was a problem hiding this comment.
🟡 Medium · Preserve the model role before applying drop validation
Native history items shaped as {role:"model",parts:[...]} are wrapped as model_output but passed here with defaultRole="user"; the wrapped object has no role to override it. The newly added drop tracking therefore classifies assistant audio as an emptied user turn. For example, model audio followed by another model_output and a normal user question now produces UnsupportedPartError/400. The equivalent model_output/content shape correctly retains the assistant-media placeholder. Also reported for this defect: - Preserve the model role when tracking dropped media (internal/translator/claude/interactions/interactions_claude_request.go:199): A {role:"model",parts:[audio]} history item is wrapped as model_output but still passed here with default role user. The new drop accounting therefore treats assistant media as an emptied user turn. After a preceding model_output has closed the user run, this item causes UnsupportedPartError at the next model step or input end, rejecting the request with HTTP 400. The base preserved a placeholder instead of rejecting this assistant history.
|
Closing: the review scope changed. Eric (2026-10-27): "i don't want to fix anything that's an issue in upstream - our only goal here is to validate our changes". Upstream code is out of scope. Our fork's own changes on v8.0.44 get one review PR instead. Findings from this part are recorded in the HOUSE-79 adjudication record. None was a defect in our code. |
Review only. Do not merge. This PR is one part of the review of #13, the merge of upstream
v8.0.44into the fork. That PR's diff (966 files) is too large for one review round.c88d701f7bc0894cde517ca7f931f5c05749cd74). The base is that tree with only this part's files restored tomain(c265779f), so the diff is this part's files alone and the surrounding code is the real final code.go.sumare in no part's diff.Linear: HOUSE-79
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.