Handle rejected encrypted reasoning retries#3537
Conversation
|
This pull request targeted The base branch has been automatically changed to |
|
ok |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism across multiple executors to handle cases where upstream providers reject encrypted_content, supported by a new utility for detecting and stripping invalid content. Additionally, the Dockerfile was updated to use internal image registries, and thinking configuration validation was modified to allow clamping of unsupported levels across all provider families. Review feedback highlights significant code duplication in the retry implementation and recommends refactoring this logic into a shared helper function to improve maintainability.
| if isInvalidResponsesEncryptedContentError(httpResp.StatusCode, b) { | ||
| if strippedBody, changed := stripInvalidEncryptedContentFromResponsesBody(body); changed { | ||
| helps.LogWithRequestID(ctx).Warn("codex executor: upstream rejected encrypted_content; retrying once without encrypted reasoning context") | ||
| retryReq, retryBuildErr := e.cacheHelper(ctx, from, url, req, strippedBody) | ||
| if retryBuildErr != nil { | ||
| err = retryBuildErr | ||
| return resp, err | ||
| } | ||
| applyCodexHeaders(retryReq, auth, apiKey, true, e.cfg) | ||
| helps.RecordAPIRequest(ctx, e.cfg, helps.UpstreamRequestLog{ | ||
| URL: url, | ||
| Method: http.MethodPost, | ||
| Headers: retryReq.Header.Clone(), | ||
| Body: strippedBody, | ||
| Provider: e.Identifier(), | ||
| AuthID: authID, | ||
| AuthLabel: authLabel, | ||
| AuthType: authType, | ||
| AuthValue: authValue, | ||
| }) | ||
| retryResp, retryErr := httpClient.Do(retryReq) | ||
| if retryErr != nil { | ||
| helps.RecordAPIResponseError(ctx, e.cfg, retryErr) | ||
| err = retryErr | ||
| return resp, err | ||
| } | ||
| defer func(body io.Closer) { | ||
| if errClose := body.Close(); errClose != nil { | ||
| log.Errorf("codex executor: close retry response body error: %v", errClose) | ||
| } | ||
| }(retryResp.Body) | ||
| helps.RecordAPIResponseMetadata(ctx, e.cfg, retryResp.StatusCode, retryResp.Header.Clone()) | ||
| if retryResp.StatusCode < 200 || retryResp.StatusCode >= 300 { | ||
| b, _ = io.ReadAll(retryResp.Body) | ||
| helps.AppendAPIResponseChunk(ctx, e.cfg, b) | ||
| helps.LogWithRequestID(ctx).Debugf("request retry error, error status: %d, error message: %s", retryResp.StatusCode, helps.SummarizeErrorBody(retryResp.Header.Get("Content-Type"), b)) | ||
| err = newCodexStatusErr(retryResp.StatusCode, b) | ||
| return resp, err | ||
| } | ||
| body = strippedBody | ||
| httpResp = retryResp | ||
| } else { | ||
| err = newCodexStatusErr(httpResp.StatusCode, b) | ||
| return resp, err | ||
| } | ||
| } else { | ||
| err = newCodexStatusErr(httpResp.StatusCode, b) | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
The retry logic for handling rejected encrypted content is duplicated across Execute, executeCompact, and ExecuteStream. This duplication increases the risk of bugs and makes maintenance harder. Consider refactoring the common parts of this logic (detection, stripping, and the retry flow) into a shared helper method within the CodexExecutor or a utility function in the helps package.
| if opts.Alt == "responses/compact" && isInvalidResponsesEncryptedContentError(httpResp.StatusCode, b) { | ||
| if strippedTranslated, changed := stripInvalidEncryptedContentFromResponsesBody(translated); changed { | ||
| helps.LogWithRequestID(ctx).Warn("openai compat executor: upstream rejected encrypted_content; retrying once without encrypted reasoning context") | ||
| retryReq, retryBuildErr := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(strippedTranslated)) | ||
| if retryBuildErr != nil { | ||
| err = retryBuildErr | ||
| return resp, err | ||
| } | ||
| retryReq.Header.Set("Content-Type", "application/json") | ||
| if apiKey != "" { | ||
| retryReq.Header.Set("Authorization", "Bearer "+apiKey) | ||
| } | ||
| retryReq.Header.Set("User-Agent", "cli-proxy-openai-compat") | ||
| util.ApplyCustomHeadersFromAttrs(retryReq, attrs) | ||
| helps.RecordAPIRequest(ctx, e.cfg, helps.UpstreamRequestLog{ | ||
| URL: url, | ||
| Method: http.MethodPost, | ||
| Headers: retryReq.Header.Clone(), | ||
| Body: strippedTranslated, | ||
| Provider: e.Identifier(), | ||
| AuthID: authID, | ||
| AuthLabel: authLabel, | ||
| AuthType: authType, | ||
| AuthValue: authValue, | ||
| }) | ||
| retryResp, retryErr := httpClient.Do(retryReq) | ||
| if retryErr != nil { | ||
| helps.RecordAPIResponseError(ctx, e.cfg, retryErr) | ||
| err = retryErr | ||
| return resp, err | ||
| } | ||
| defer func(body io.Closer) { | ||
| if errClose := body.Close(); errClose != nil { | ||
| log.Errorf("openai compat executor: close retry response body error: %v", errClose) | ||
| } | ||
| }(retryResp.Body) | ||
| helps.RecordAPIResponseMetadata(ctx, e.cfg, retryResp.StatusCode, retryResp.Header.Clone()) | ||
| if retryResp.StatusCode < 200 || retryResp.StatusCode >= 300 { | ||
| b, _ = io.ReadAll(retryResp.Body) | ||
| helps.AppendAPIResponseChunk(ctx, e.cfg, b) | ||
| helps.LogWithRequestID(ctx).Debugf("request retry error, error status: %d, error message: %s", retryResp.StatusCode, helps.SummarizeErrorBody(retryResp.Header.Get("Content-Type"), b)) | ||
| err = statusErr{code: retryResp.StatusCode, msg: string(b)} | ||
| return resp, err | ||
| } | ||
| translated = strippedTranslated | ||
| httpResp = retryResp | ||
| } else { | ||
| err = statusErr{code: httpResp.StatusCode, msg: string(b)} | ||
| return resp, err | ||
| } | ||
| } else { | ||
| err = statusErr{code: httpResp.StatusCode, msg: string(b)} | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
The retry logic here is nearly identical to the logic implemented in CodexExecutor. To improve maintainability and reduce code duplication, consider abstracting the retry flow into a reusable component or helper function that can be shared between different executors.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0184f30c50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f410489efd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if handlers.StreamingKeepAliveInterval(h.Cfg) > 0 { | ||
| setSSEHeaders() | ||
| handlers.WriteUpstreamHeaders(c.Writer.Header(), upstreamHeaders) | ||
| _, _ = c.Writer.Write([]byte(": keep-alive\n\n")) | ||
| flusher.Flush() | ||
| h.forwardResponsesStream(c, flusher, func(err error) { cliCancel(err) }, dataChan, errChan, framer) |
There was a problem hiding this comment.
When StreamingKeepAliveInterval(h.Cfg) > 0, this branch immediately writes and flushes SSE headers plus a keep-alive comment before reading errChan, which commits a 200 response even if the upstream fails during stream bootstrap (for example 401, 429, or 5xx). In that case forwardResponsesStream can only emit an in-band SSE error event and cannot restore the correct HTTP status, so clients that depend on status codes for auth/retry/error handling will mis-handle failed requests. The non-keepalive path below explicitly peeks for immediate errors first, so this is a behavior regression specific to keepalive-enabled deployments.
Useful? React with 👍 / 👎.
qa19971109
force-pushed
the
cpa-encrypted-reasoning-fix-20260525
branch
from
f410489 to
77dce3e
Compare
1 participant
Summary
Tests