Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 7 of 13: sdk handlers and api server - #19
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. |
🛑 4 blocking findings🔴 Critical · Bound decompressed WebSocket messages before enabling compression 🟡 Medium · Preserve registered speech-model prefixes when routing 🟡 Medium · Match local interrupts to the active response 🟡 Medium · Propagate the Responses conversion refusal WalkthroughThis review-only slice of the upstream
Advisories (2)
Since the last review
Repair promptRead the Details
First review of this pull request. @unfret review · @unfret status · @unfret help |
There was a problem hiding this comment.
🛑 4 blocking findings
🔴 Critical · Bound decompressed WebSocket messages before enabling compression
🟡 Medium · Preserve registered speech-model prefixes when routing
🟡 Medium · Match local interrupts to the active response
🟡 Medium · Propagate the Responses conversion refusal
Check
| return defaultXAISpeechModel, true | ||
| case xaiSpeechVoiceModel: | ||
| return xaiSpeechVoiceModel, true |
There was a problem hiding this comment.
🟡 Medium · Preserve registered speech-model prefixes when routing
With an xAI credential prefix of xai and force-model-prefix: true, model registration publishes xai/grok-tts without the bare model. Requesting that advertised ID passes validation, but this return replaces it with grok-tts, causing a 400 model_not_found without executing the credential. The voice-model branch has the same problem. Preserve the registered routing ID when normalizing speech aliases. Also reported for this defect: - Preserve registered speech routing prefixes (sdk/api/handlers/openai/openai_speech_handlers.go:107-109): With force-model-prefix enabled and an xAI credential prefix of xai, the registry publishes xai/grok-tts rather than grok-tts. speechModelBase strips the prefix, and these returns route the request using the unregistered bare ID. Posting the advertised xai/grok-tts model therefore returns 400 'unknown provider for model grok-tts' without executing speech synthesis. The same problem affects grok-voice-tts-1.0. Preserve the registered routing ID separately from upstream model normalization.
| case s.frames <- bytes.Clone(payload): | ||
| default: | ||
| } | ||
| return true |
There was a problem hiding this comment.
🟡 Medium · Match local interrupts to the active response
The local interrupt queue checks only whether a turn is active, not whether response_id identifies that turn. During an HTTP-backed response r-current, an interrupt naming an unrelated or previous HTTP response is accepted here. completeResponsesWebsocketLocalInterrupt then cancels r-current and emits response.incomplete under the supplied, incorrect ID, which also becomes the continuation ID. This is reproducible with a mismatched response_id; local cancellation needs response-scoped identity checks. Also reported for this defect: - Match local interrupts to the active response ID (sdk/api/handlers/openai/openai_responses_websocket_input.go:66): During an HTTP fallback turn streaming response r-http, an interrupt naming a different response is accepted because deliver checks only active. The forwarder then cancels r-http and emits response.incomplete using the unrelated supplied ID. Consequently, a delayed interrupt from an earlier turn can terminate the current generation. Validate the target response ID before enqueueing the interrupt.
Also affects:
| upgrader.EnableCompression = responsesWebsocketCompressionEnabled.Load() | ||
| conn, err := upgrader.Upgrade(c.Writer, c.Request, websocketUpgradeHeaders(c.Request)) |
There was a problem hiding this comment.
🔴 Critical · Bound decompressed WebSocket messages before enabling compression
Enabling permessage-deflate while the reader still uses unbounded conn.ReadMessage() introduces a decompression-amplified memory-exhaustion path. An authenticated client can send a small compressed message that allocates arbitrarily large buffers before turn authorization or validation. A reproduction expanded roughly 17 KB on the wire into 16 MiB despite a configured 1 MiB request-body limit; HTTP MaxBytesReader does not cover messages after upgrade. Larger or repeated messages can exhaust the heap and terminate the proxy.
| if shouldTreatAsResponsesFormat(rawJSON) { | ||
| modelName := gjson.GetBytes(rawJSON, "model").String() | ||
| rawJSON = responsesconverter.ConvertOpenAIResponsesRequestToOpenAIChatCompletions(modelName, rawJSON, stream) | ||
| rawJSON, _ = responsesconverter.ConvertOpenAIResponsesRequestToOpenAIChatCompletions(modelName, rawJSON, stream) |
There was a problem hiding this comment.
🟡 Medium · Propagate the Responses conversion refusal
The converter now returns UnsupportedPartError (HTTP 400) when translation would empty a user turn, but this call discards that error. For example, a Responses-format request containing instructions and a user turn consisting solely of input_file.file_url reaches the executor with content:[] instead of being rejected. A reproduction invoked the upstream and returned 200 despite the conversion refusal. Handle the error before entering either execution path.
|
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.