Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 8 of 13: management handlers and small translators - #20
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. |
🛑 9 blocking findings🔴 Critical · Nil Devin handler now panics before its guard 🔴 Critical · Clone OAuth configuration under the handler lock 🟡 Medium · Direct Responses conversion discards translation failures 🟡 Medium · Evaluate dropped attachments across the merged user turn 🟡 Medium · 🟡 Medium · Honor the credential proxy during SIWC binding 🟡 Medium · Per-session proxy makes optional project lookup fatal 🔵 Low · Limit plugin default metadata to v8 responses 🔵 Low · Keep plugin model fallback exclusive to v8 WalkthroughThis is a review-only slice (part 8 of 13) of the upstream
Advisories (1)
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.
🛑 9 blocking findings
🔴 Critical · Nil Devin handler now panics before its guard
🔴 Critical · Clone OAuth configuration under the handler lock
🟡 Medium · Direct Responses conversion discards translation failures
🟡 Medium · Evaluate dropped attachments across the merged user turn
🟡 Medium · `tool_choice: none` leaves native tools enabled
🟡 Medium · Honor the credential proxy during SIWC binding
🟡 Medium · Per-session proxy makes optional project lookup fatal
🔵 Low · Limit plugin default metadata to v8 responses
🔵 Low · Keep plugin model fallback exclusive to v8
Check
| out = append(out, pluginConfigFieldInfo{ | ||
| Name: htmlsanitize.String(field.Name), | ||
| Type: htmlsanitize.String(string(field.Type)), | ||
| Default: htmlsanitize.JSONValue(field.Default), |
There was a problem hiding this comment.
🔵 Low · Limit plugin default metadata to v8 responses
This shared mapping is used by ListPlugins at GET /v0/management/plugins, so adding Default also changes the deprecated v0 response, including nested metadata.config_fields. AGENTS.md prohibits feature additions to /v0/management unless required for compilation; exposing plugin defaults is not a compilation repair. Keep the new response field confined to v8. Also reported for this defect: - Keep plugin defaults out of the legacy response schema (internal/api/handlers/management/plugins.go:54): ListPlugins serializes this shared struct for both API versions, so the unconditional default key, populated at line 473, extends GET /v0/management/plugins as well as v8. AGENTS.md explicitly forbids such legacy-endpoint feature changes except compilation fixes. Project defaults only into the v8 response while preserving the v0 schema.
Also affects:
| Stream: stream, | ||
| }) | ||
| return req.Body | ||
| return req.Body, nil |
There was a problem hiding this comment.
🟡 Medium · Direct Responses conversion discards translation failures
If envelope conversion sets req.Err, such as when an unsupported attachment empties a user turn, this wrapper still returns the translated body with a nil error. Direct callers therefore cannot reject the request and may send the attachment-stripped body upstream despite the new request-transform error contract. Also reported for this defect: - Return translation refusals from the Responses wrapper (internal/translator/antigravity/openai/responses/antigravity_openai-responses_request.go:136): Calling this converter with a user turn containing only an input_file file_id produces an UnsupportedPartError in req.Err, but this return discards it. The exported function consequently reports success with request.contents empty, unlike its envelope counterpart and the other error-returning translators. A direct-call probe reproduced the lost refusal; return req.Err alongside req.Body. - Return the envelope's translation error (internal/translator/antigravity/openai/responses/antigravity_openai-responses_request.go:136): For a user turn containing only an input_file with a file_id, the envelope converter sets UnsupportedPartError, but this wrapper returns nil instead. Callers using the newly error-returning function therefore see success and receive a payload with the user content removed, bypassing the request-scoped refusal. Return req.Err alongside req.Body.
| models := registry.GetStaticModelDefinitionsByChannel(channel) | ||
| if models == nil { | ||
| if len(models) == 0 { | ||
| models = h.pluginModelDefinitionsByChannel(channel) |
There was a problem hiding this comment.
🔵 Low · Keep plugin model fallback exclusive to v8
This handler also serves GET /v0/management/model-definitions/:channel. For a plugin-only channel, the new unconditional fallback changes that legacy endpoint from an unknown-channel error to a dynamic model response. AGENTS.md forbids feature changes under /v0/management unless required for compilation; the new fallback needs to be restricted to v8. Also reported for this defect: - Keep dynamic model discovery off the deprecated v0 endpoint (internal/api/handlers/management/model_definitions.go:27): GetStaticModelDefinitions is still registered at GET /v0/management/model-definitions/:channel, so this unconditional fallback adds plugin/runtime model discovery to that deprecated endpoint. AGENTS.md expressly prohibits feature changes under /v0/management except compilation repairs. Scope the new discovery behavior to the v8 endpoint while preserving the v0 contract.
|
|
||
| if originalRole == "user" { | ||
| // Whitespace-only text is forwarded but never keeps an emptied turn alive. | ||
| drops.EndTurn(translatorcommon.CountSendableGeminiParts(partItems)) |
There was a problem hiding this comment.
🟡 Medium · Evaluate dropped attachments across the merged user turn
With two adjacent Claude user messages—one containing text and the next containing an unsupported document file_id—this call records an emptied turn and returns UnsupportedPartError. Later, MergeAdjacentGeminiContents combines those messages into one nonempty user turn. The request consequently receives HTTP 400 even though the shared policy permits dropping an attachment when that translated turn still contains sendable text.
| // http://127.0.0.1:<port>/callback (http protocol, 127.0.0.1 host, and /callback path). | ||
| func (h *Handler) devinCallbackURL() (string, error) { | ||
| if h == nil || h.cfg == nil || h.cfg.Port <= 0 { | ||
| cfg := h.configSnapshot() |
There was a problem hiding this comment.
🔴 Critical · Nil Devin handler now panics before its guard
When RequestDevinToken is invoked with a nil *Handler, devinCallbackURL now calls h.configSnapshot() before checking h == nil. Since configSnapshot dereferences h.mu, this crashes instead of returning the existing "server port is not configured" error.
| @@ -520,9 +529,6 @@ func applyOpenAIToolChoiceToAntigravity(out, rawJSON []byte, functionNameMap map | |||
| } | |||
|
|
|||
| out, _ = sjson.SetBytes(out, "request.toolConfig.functionCallingConfig.mode", mode) | |||
There was a problem hiding this comment.
🟡 Medium · tool_choice: none leaves native tools enabled
The deleted if mode == "NONE" { out, _ = sjson.DeleteBytes(out, "request.tools") } guard leaves googleSearch, codeExecution, and urlContext blocks in the Antigravity request. functionCallingConfig.mode = NONE only controls function calling, so a request that explicitly disables tools can still execute one of these native tools.
| SDKConfig: config.SDKConfig{ProxyURL: proxyURL}, | ||
| } | ||
| } | ||
| cloned := cfg.CloneForRuntime() |
There was a problem hiding this comment.
🔴 Critical · Clone OAuth configuration under the handler lock
Starting OAuth with a nonempty proxy_url while another request updates a configuration map, such as PATCH /v0/management/oauth-excluded-models, can terminate the process with "fatal error: concurrent map iteration and map write". configSnapshot releases h.mu before returning the shared configuration, so CloneForRuntime iterates its maps without synchronization. This crash was reproduced with concurrent cloning and locked configuration writes. Also reported for this defect: - Clone OAuth configuration under the handler lock (internal/api/handlers/management/handler.go:164): Starting OAuth with a non-empty proxy_url concurrently with PatchOAuthExcludedModels can terminate the entire server. configSnapshot returns the live h.cfg pointer after releasing h.mu, so CloneForRuntime iterates its maps while the other handler mutates them. A concurrency probe reproduced fatal error: concurrent map iteration and map write. Capture the clone while holding h.mu.
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to initialize SIWC authorization"}) | ||
| return | ||
| } | ||
| var service siwcOAuthService = codex.NewCodexAuth(cfg) |
There was a problem hiding this comment.
🟡 Medium · Honor the credential proxy during SIWC binding
For an existing Codex credential reachable only through target.ProxyURL, binding creates its OAuth client using only the global proxy configuration. The authorization URL is returned successfully, but callback token exchange attempts the wrong network route and fails. Normal Codex refresh already honors auth.ProxyURL through NewCodexAuthWithProxyURL; the new binding path needs the same credential-specific proxy selection.
| if sessionProxyURL != "" { | ||
| log.Errorf("antigravity: failed to fetch project ID: %s", redactSessionProxyInString(errProject.Error(), sessionProxyURL)) | ||
| SetOAuthSessionError(state, "Failed to fetch project ID") | ||
| return |
There was a problem hiding this comment.
🟡 Medium · Per-session proxy makes optional project lookup fatal
When Antigravity token exchange and user-info lookup succeed but FetchProjectID fails, supplying a per-session proxy now aborts the entire OAuth flow. The same lookup failure without a session proxy remains non-fatal and the credential format already supports an empty project ID, so users whose proxy cannot reach that optional endpoint lose an otherwise valid credential.
|
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.