Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 5 of 13: antigravity, gemini, openai-compat executors - #18
Review only: HOUSE-79 upstream v8.0.44, part 5 of 13: antigravity, gemini, openai-compat executors#18ericlitman 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. |
🛑 6 blocking findings🟡 Medium · Close stream output before waiting for delivery acknowledgment 🟡 Medium · Aliased non-GPT reasoning signatures are discarded 🟡 Medium · Ignore null error fields in successful stream frames 🟡 Medium · Do not cache continuations for unstored interactions 🟡 Medium · Token counting uses the pre-finalization model 🟡 Medium · Bound incomplete JSON buffering to one SSE event 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.
🛑 6 blocking findings
🟡 Medium · Close stream output before waiting for delivery acknowledgment
🟡 Medium · Aliased non-GPT reasoning signatures are discarded
🟡 Medium · Ignore null error fields in successful stream frames
🟡 Medium · Do not cache continuations for unstored interactions
🟡 Medium · Token counting uses the pre-finalization model
🟡 Medium · Bound incomplete JSON buffering to one SSE event
Check
| if deliveryErr, tracked := usage.WaitStreamDelivery(ctx); tracked && deliveryErr == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Close stream output before waiting for delivery acknowledgment
When a consumer uses WithStreamDelivery and acknowledges delivery after draining Chunks, cancellation while the scanner awaits another upstream chunk reaches this wait before close(out). The producer waits for acknowledgment while the consumer waits for EOF, deadlocking the canceled stream and retaining its response body. The stream-delivery contract explicitly requires producers to close their output before waiting.
| } | ||
| requested := thinking.ParseSuffix(helps.PayloadRequestedModel(opts, req.Model)).ModelName | ||
| switch signature.SignatureProviderFromModelName(requested) { | ||
| case signature.SignatureProviderMeta, signature.SignatureProviderGrok, signature.SignatureProviderGPT: |
There was a problem hiding this comment.
🟡 Medium · Aliased non-GPT reasoning signatures are discarded
When an opaque resolved model is an alias for a requested Claude, Gemini, Kimi, or SWE model, this switch does not fall back to the requested model even though the sanitizer explicitly supports those providers. On /responses/compact, a valid provider signature is consequently classified as foreign and removed, potentially along with its item ID, so the upstream continuation loses its required reasoning state or is rejected.
| if errorResult := gjson.GetBytes(payload, "error"); errorResult.Exists() { | ||
| statusCode := int(errorResult.Get("code").Int()) | ||
| if statusCode < http.StatusBadRequest || statusCode > 599 { | ||
| statusCode = http.StatusBadGateway | ||
| } | ||
| streamErr = newAntigravityStatusErr(statusCode, payload) |
There was a problem hiding this comment.
🟡 Medium · Ignore null error fields in successful stream frames
A configured Antigravity-compatible relay can include an optional "error":null alongside valid response candidates. GJSON reports that field as existing, so this condition converts the successful frame into a 502 error and terminates the stream before delivering its answer. Check for a non-null error value rather than field presence alone. Also reported for this defect: - Exclude JSON null from the stream error check (internal/runtime/executor/antigravity_executor_stream.go:299): For a successful HTTP-200 frame containing both "error":null and valid response.candidates, GJSON's Exists() returns true for the null field. This branch therefore discards the successful answer and emits a 502 before translation. A local executor reproduction confirms the failure; without this new check, the same frame delivers its answer. Require a non-null error node before taking the error path.
Also affects:
| if callKey == "" { | ||
| return | ||
| } | ||
| antigravityInteractionsSessions.Put(s.key+":"+callKey, cache.InteractionsContinuation{ID: s.id, Environment: s.environment}) |
There was a problem hiding this comment.
🟡 Medium · Do not cache continuations for unstored interactions
A native Interactions request with store:false can return an ID and requires_action, but that ID cannot be used for subsequent continuation. This unconditional cache write ignores the outgoing storage policy. On the next matching tool-result turn, PrepareAntigravityInteractions injects the unstored ID and removes the full history, causing upstream to reject a request that previously worked statelessly. Only cache continuation IDs for interactions actually stored upstream.
| return cliproxyexecutor.Response{}, err | ||
| } | ||
|
|
||
| translated = helps.NewPayloadFinalizer(e.cfg, e.Identifier(), baseModel, to.String(), "", originalTranslatedForPayload, req, opts)(translated) |
There was a problem hiding this comment.
🟡 Medium · Token counting uses the pre-finalization model
This finalizer can apply a payload rule that changes the final model, but modelForCounting was already fixed to baseModel and the tokenizer is selected from that old value. For example, a rule rewriting a GPT-3.5 request to GPT-4o counts the finalized messages with the wrong encoding and returns an incorrect token total.
| if len(trimmedLine) > 0 { | ||
| pendingJSON = append(pendingJSON, '\n') | ||
| pendingJSON = append(pendingJSON, trimmedLine...) | ||
| } | ||
| if !gjson.ValidBytes(pendingJSON) { | ||
| continue |
There was a problem hiding this comment.
🟡 Medium · Bound incomplete JSON buffering to one SSE event
An incomplete event such as data: {\n\n leaves pendingJSON populated across the blank-line event boundary. Every subsequent valid content or terminal event is appended to that invalid buffer and skipped indefinitely. A reproduction with a valid answer following that event returns no answer and no StreamChunk.Err; the parser must reject or reset the incomplete event rather than consume the rest of the stream. Also reported for this defect: - Do not let malformed JSON consume subsequent SSE frames (internal/runtime/executor/antigravity_executor_stream.go:287-288): Once pendingJSON contains malformed data, this continue also skips SSE frame boundaries and appends every later event to the invalid prefix. For example, data: {"unused": followed by a blank line and a valid completion frame loses the entire answer and closes Chunks without an error. The base parser still delivered the later valid frame. Reject or isolate the malformed frame instead of silently swallowing subsequent output.
Also affects:
|
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.