Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,31 @@ go build -o test-output ./cmd/server && rm test-output # Verify compile (REQUIRE
- Use logrus structured logging; avoid leaking secrets/tokens in logs
- Avoid panics in HTTP handlers; prefer logged errors and meaningful HTTP status codes
- Timeouts are allowed only during credential acquisition; after an upstream connection is established, do not set timeouts for any subsequent network behavior. Intentional exceptions that must remain allowed are the Codex websocket liveness deadlines in `internal/runtime/executor/codex_websockets_executor.go`, the wsrelay session deadlines in `internal/wsrelay/session.go`, the management APICall timeout in `internal/api/handlers/management/api_tools.go`, and the `cmd/fetch_antigravity_models` utility timeouts

### [Testing] Baseline full-suite failures can be unrelated to the current patch
Detailed description:
Running `go test ./...` on `fix/preserve-reasoning-content` surfaced existing failures outside the Responses reasoning follow-up work:
- `internal/registry`: `TestCodexFreeModelsExcludeGPT55`
- `internal/runtime/executor`: `TestEnsureAccessToken_WarmTokenLoadsCreditsHint`
- `internal/runtime/executor`: `TestUpdateAntigravityCreditsBalance_LoadCodeAssistUserAgent`
These failures can block "green full suite" expectations even when the modified package under review is passing.

Impact scope:
AI agents reviewing or preparing commits for narrowly scoped translator/request fixes may incorrectly assume their patch caused unrelated red tests, delaying or broadening the change unnecessarily.

Suggested solutions:
- Record both the full-suite result and the package-scoped result when reporting verification.
- For Responses reasoning fixes, verify at minimum `go test ./internal/translator/openai/openai/responses` and `go build -o test-output ./cmd/server`.
- Treat unrelated full-suite failures as baseline noise unless the diff touches the failing package.

### [Change Scope] Do not mix unverified local executor refactors into Responses-only fixes
Detailed description:
The working tree may contain extra local edits under `internal/runtime/executor/` that are not required for a Responses translator issue. In this session, `internal/runtime/executor/reasoning_preserve.go` included a separate strategy change that rebuilds the entire `messages` array after patching reasoning fields. That implementation detail is broader than the Responses follow-up fix and needs its own dedicated validation before inclusion.

Impact scope:
If an agent stages all modified files blindly, a small Responses bugfix commit can accidentally absorb executor behavior changes that were not part of the same root cause or acceptance scope.

Suggested solutions:
- Stage only files directly tied to the issue being fixed.
- When executor-side reasoning preservation logic changes independently, add focused tests for the specific reconstruction strategy before committing it.
- Call out excluded local files explicitly in the handoff or commit summary.
28 changes: 21 additions & 7 deletions internal/runtime/executor/openai_compat_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,10 @@ func (e *OpenAICompatExecutor) Execute(ctx context.Context, auth *cliproxyauth.A
to = sdktranslator.FromString("openai-response")
endpoint = "/responses/compact"
}
originalPayloadSource := req.Payload
originalPayload := req.Payload
if len(opts.OriginalRequest) > 0 {
originalPayloadSource = opts.OriginalRequest
originalPayload = opts.OriginalRequest
}
originalPayload := originalPayloadSource
originalTranslated := sdktranslator.TranslateRequest(from, to, baseModel, originalPayload, opts.Stream)
translated := sdktranslator.TranslateRequest(from, to, baseModel, req.Payload, opts.Stream)

Expand All @@ -105,6 +104,12 @@ func (e *OpenAICompatExecutor) Execute(ctx context.Context, auth *cliproxyauth.A
requestedModel := helps.PayloadRequestedModel(opts, req.Model)
requestPath := helps.PayloadRequestPath(opts)
translated = helps.ApplyPayloadConfigWithRoot(e.cfg, baseModel, to.String(), "", translated, originalTranslated, requestedModel, requestPath)

translated, err = preserveReasoningContent(originalTranslated, translated)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect payload filters for reasoning_content

When a user configures payload.filter or an override to remove/replace messages.N.reasoning_content for an OpenAI-compatible backend, this second preserveReasoningContent call runs after ApplyPayloadConfigWithRoot and unconditionally re-adds the original value. That makes the explicit payload config ineffective (the same ordering exists in the streaming path), so a provider that rejects or needs sanitized reasoning content can no longer be worked around via config.

Useful? React with 👍 / 👎.

if err != nil {
return resp, err
}

if opts.Alt == "responses/compact" {
if updated, errDelete := sjson.DeleteBytes(translated, "stream"); errDelete == nil {
translated = updated
Expand Down Expand Up @@ -193,11 +198,15 @@ func (e *OpenAICompatExecutor) ExecuteStream(ctx context.Context, auth *cliproxy

from := opts.SourceFormat
to := sdktranslator.FromString("openai")
originalPayloadSource := req.Payload
endpoint := "/chat/completions"
if opts.Alt == "responses/compact" {
to = sdktranslator.FromString("openai-response")
endpoint = "/responses/compact"
}
originalPayload := req.Payload
if len(opts.OriginalRequest) > 0 {
originalPayloadSource = opts.OriginalRequest
originalPayload = opts.OriginalRequest
}
originalPayload := originalPayloadSource
originalTranslated := sdktranslator.TranslateRequest(from, to, baseModel, originalPayload, true)
translated := sdktranslator.TranslateRequest(from, to, baseModel, req.Payload, true)

Expand All @@ -210,11 +219,16 @@ func (e *OpenAICompatExecutor) ExecuteStream(ctx context.Context, auth *cliproxy
requestPath := helps.PayloadRequestPath(opts)
translated = helps.ApplyPayloadConfigWithRoot(e.cfg, baseModel, to.String(), "", translated, originalTranslated, requestedModel, requestPath)

translated, err = preserveReasoningContent(originalTranslated, translated)
if err != nil {
return nil, err
}

// Request usage data in the final streaming chunk so that token statistics
// are captured even when the upstream is an OpenAI-compatible provider.
translated, _ = sjson.SetBytes(translated, "stream_options.include_usage", true)

url := strings.TrimSuffix(baseURL, "/") + "/chat/completions"
url := strings.TrimSuffix(baseURL, "/") + endpoint
httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(translated))
if err != nil {
return nil, err
Expand Down
96 changes: 96 additions & 0 deletions internal/runtime/executor/reasoning_preserve.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package executor

import (
"fmt"
"strings"

"github.com/tidwall/gjson"
"github.com/tidwall/sjson"
)

// preserveReasoningContent ensures assistant messages in the translated OpenAI-format
// payload retain reasoning_content from the original source payload.
//
// DeepSeek and other providers that support thinking mode require reasoning_content
// to be passed back verbatim in multi-turn conversations. Without this, the API returns
// a 400 error: "The reasoning_content in the thinking mode must be passed back to the API."
//
// Matching strategy: instead of requiring identical message counts (which breaks when
// translation inserts/splits messages like Claude tool_result → tool role), we match
// assistant messages by their ordinal position within the assistant-only sequence.
// This is robust because translation never reorders or drops assistant messages —
// it only inserts non-assistant messages (tool, system) around them.
//
// When the translated payload already carries reasoning_content at a given assistant
// ordinal (e.g. from a payload override or from translation), that value is preserved —
// the user or translator has explicitly set it and their intent takes precedence.
// Only when reasoning_content is missing do we fall back to the original value.
//
// Error contract: on sjson.SetBytes failure, the function discards any partial writes
// and returns the unmodified translated input along with the error, so the caller never
// receives a partially-patched payload.
func preserveReasoningContent(original, translated []byte) ([]byte, error) {
if len(original) == 0 || len(translated) == 0 {
return translated, nil
}
if !gjson.ValidBytes(original) || !gjson.ValidBytes(translated) {
return translated, nil
}

origMsgs := gjson.GetBytes(original, "messages")
if !origMsgs.Exists() || !origMsgs.IsArray() {
return translated, nil
}
origMsgArr := origMsgs.Array()

transMsgs := gjson.GetBytes(translated, "messages")
if !transMsgs.Exists() || !transMsgs.IsArray() {
return translated, nil
}
transMsgArr := transMsgs.Array()

origReasoning := collectAssistantReasoning(origMsgArr)
if len(origReasoning) == 0 {
return translated, nil
}

out := translated
assistantOrdinal := 0
for i, msg := range transMsgArr {
if strings.TrimSpace(msg.Get("role").String()) != "assistant" {
continue
}

origText, origOK := origReasoning[assistantOrdinal]
transRC := msg.Get("reasoning_content")
if origOK && !transRC.Exists() {
path := fmt.Sprintf("messages.%d.reasoning_content", i)
next, err := sjson.SetBytes(out, path, origText)
if err != nil {
return translated, fmt.Errorf("preserveReasoningContent: failed to set reasoning_content at index %d: %w", i, err)
}
out = next
}
assistantOrdinal++
}

return out, nil
}

// collectAssistantReasoning extracts reasoning_content from assistant messages,
// keyed by their ordinal position in the assistant-only sequence (0, 1, 2, ...).
// Empty-string reasoning_content is preserved because DeepSeek requires it.
func collectAssistantReasoning(messages []gjson.Result) map[int]string {
reasoning := make(map[int]string)
ordinal := 0
for _, msg := range messages {
if strings.TrimSpace(msg.Get("role").String()) != "assistant" {
continue
}
if rc := msg.Get("reasoning_content"); rc.Exists() {
reasoning[ordinal] = rc.String()
}
ordinal++
}
return reasoning
}
Loading
Loading