Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 3 of 13: codex executor - #16
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. |
🛑 7 blocking findings🟡 Medium · Use the finalized payload for image-tool usage attribution 🟡 Medium · Synchronize plan attributes when adopting disk credentials 🟡 Medium · Derive routing hints from the finalized model 🟡 Medium · Parse responses according to the finalized stream mode 🟡 Medium · Restrict TLS timeouts to credential acquisition 🟡 Medium · Select the tokenizer using the finalized model 🟡 Medium · Recognize explicit-port Alpha Search URLs 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.
🛑 7 blocking findings
🟡 Medium · Use the finalized payload for image-tool usage attribution
🟡 Medium · Synchronize plan attributes when adopting disk credentials
🟡 Medium · Derive routing hints from the finalized model
🟡 Medium · Parse responses according to the finalized stream mode
🟡 Medium · Restrict TLS timeouts to credential acquisition
🟡 Medium · Select the tokenizer using the finalized model
🟡 Medium · Recognize explicit-port Alpha Search URLs
Check
| httpReq, upstreamBody, err := e.cacheHelper(ctx, from, url, req, body, auth, opts.Headers) | ||
| if err != nil { | ||
| return resp, err | ||
| } | ||
| applyCodexHeaders(httpReq, auth, apiKey, true, e.cfg, opts.Headers) | ||
| applyCodexRoutingHint(ctx, httpReq.Header, auth, baseModel, upstreamBody, opts.Headers) | ||
| applyModelHeaderOverrides(httpReq.Header, baseModel) | ||
| reporter.SetTranslatedReasoningEffort(upstreamBody, to.String()) |
There was a problem hiding this comment.
🟡 Medium · Use the finalized payload for image-tool usage attribution
Payload rules now run inside cacheHelper, but the finalized upstreamBody never replaces body. For a request declaring image_generation with model gpt-image-2 and an override setting tools.0.model to gpt-image-1.5, upstream receives gpt-image-1.5 while publishCodexImageToolUsage reads the old body and attributes its tokens to gpt-image-2. This reproduces in both Execute here and ExecuteStream at codex_executor_stream.go:100. Also reported for this defect: - Use the finalized tool model for image usage (internal/runtime/executor/codex_executor_execute.go:93): After cacheHelper applies payload rules, only upstreamBody contains the configured payload; body remains unconfigured. With an override changing tools.0.model from gpt-image-2 to gpt-image-1.5, the request correctly sends gpt-image-1.5, but publishCodexImageToolUsage still receives body and records the image tokens against gpt-image-2. This reproduces in both Execute and ExecuteStream, including its buffered and unbuffered completion branches, producing incorrect model-specific usage accounting.
Also affects:
| auth.Metadata[key] = val | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Synchronize plan attributes when adopting disk credentials
When another process has rotated the credential file and the new token is no longer due, this branch updates Metadata["plan_type"] but leaves Attributes["plan_type"] unchanged. With an in-memory plus credential and refreshed disk credentials marked free, Refresh succeeds while isCodexFreePlanAuth still returns false, causing subsequent requests to inject image_generation and select the wrong plan capabilities. Disk adoption needs the same plan-attribute synchronization as a successful OAuth refresh. Also reported for this defect: - Synchronize plan attributes after disk rotation (internal/runtime/executor/codex_executor_auth.go:221): Disk reconciliation copies a newly refreshed plan_type only into Metadata, leaving Attributes["plan_type"] stale. For example, after another process persists a plus-to-free plan change, this process can still make isCodexFreePlanAuth return false and inject image-generation tools that the free credential cannot use, causing the next request to be rejected.
Also affects:
| reporter.SetTranslatedReasoningEffort(upstreamBody, to.String()) | ||
| prepareHeaders := func(request *http.Request) { | ||
| applyCodexHeaders(request, auth, apiKey, true, e.cfg, opts.Headers) | ||
| applyCodexRoutingHint(ctx, request.Header, auth, baseModel, upstreamBody, opts.Headers) |
There was a problem hiding this comment.
🟡 Medium · Derive routing hints from the finalized model
Payload model overrides now survive finalization, but the routing hint still uses the original baseModel. For example, overriding a gpt-5.4 request to gpt-5.5 sends body.model=gpt-5.5 alongside X-Codex-Routing-Hint: model=gpt-5.4, so the generated routing metadata names the wrong model. The same mismatch affects executeCompact, ExecuteStream, CodexWebsocketsExecutor.Execute, and prepareCodexWebsocketStream. Derive the model from the final payload while retaining explicit operator-header precedence.
| } | ||
| rawJSON = helps.SanitizeCodexInputItemIDs(rawJSON) | ||
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(rawJSON)) | ||
| rawJSON = helps.FinalizePayload(ctx, rawJSON) |
There was a problem hiding this comment.
🟡 Medium · Parse responses according to the finalized stream mode
Payload rules now run after the built-in stream:true assignment. If a valid payload rule sets stream:false, Execute receives a normal JSON Responses object but still scans exclusively for data: SSE events and returns an incomplete-stream error despite the successful upstream response; the streaming path likewise cannot handle the finalized non-stream mode.
| if timeout <= 0 { | ||
| return ctx, func() {} | ||
| } | ||
| return context.WithTimeout(ctx, timeout) |
There was a problem hiding this comment.
🟡 Medium · Restrict TLS timeouts to credential acquisition
AGENTS.md permits timeouts only during credential acquisition. This helper is invoked by both inference uTLS transports after TCP dialing and proxy CONNECT, and NewUtlsHTTPClient activates it for ordinary Claude/Codex inference whenever TLSHandshakeTimeoutSeconds is positive. A slow handshake is therefore canceled with DeadlineExceeded outside credential acquisition, contrary to the mandate; none of the listed timeout exceptions covers this path. Also reported for this defect: - Keep inference timeouts within the repository mandate (internal/runtime/executor/helps/utls_handshake_timeout.go:74): AGENTS.md permits timeouts only during credential acquisition, apart from its named exceptions. This timeout is used by both inference uTLS paths in utls_client.go after TCP dialing and proxy CONNECT, not during credential acquisition. With requests.tls-handshake-timeout-seconds enabled, an ordinary Claude or Codex inference attempt is terminated when this timer expires even while its caller context remains active. Cancelling the timer after a successful handshake does not satisfy the credential-acquisition-only rule.
| body = helps.SetBoolIfDifferent(body, "stream", false) | ||
| body = normalizeCodexInstructions(body, helps.IsNativeCodexRequest(req.Payload, opts)) | ||
|
|
||
| body = helps.NewPayloadFinalizer(e.cfg, e.Identifier(), baseModel, to.String(), "", originalTranslatedForPayload, req, opts)(body) |
There was a problem hiding this comment.
🟡 Medium · Select the tokenizer using the finalized model
The new finalizer can change body.model, but the following tokenizer selection still uses the original baseModel. A gpt-5.4 request with instructions "こんにちは世界" and a payload override to gpt-3.5-turbo returns 2 input tokens, whereas the configured model's tokenizer returns 4 for the same final content. The token-count endpoint therefore reports a count for a different model than the payload used for generation.
| apiKey, _ := codexCredsForRequest(req.Context(), auth, req.Header) | ||
| if codexUseSIWCForRequest(req.Context(), auth, req.Header) && req.URL != nil && req.URL.Scheme == "https" && | ||
| req.URL.Host == "chatgpt.com" && req.URL.Path == "/backend-api/codex/alpha/search" { | ||
| // ChatGPT Alpha Search requires the original OAuth partition, not SIWC. |
There was a problem hiding this comment.
🟡 Medium · Recognize explicit-port Alpha Search URLs
With SIWC enabled, an Alpha Search request to https://chatgpt.com:443/backend-api/codex/alpha/search misses this branch because URL.Host includes the port; host casing can fail similarly. The request consequently sends the SIWC token instead of the required original OAuth token and receives an authorization error.
|
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.