fix(channel-test): use global Responses API policy instead of model name heuristic - #2963
Closed
0-don wants to merge 1 commit into
Closed
fix(channel-test): use global Responses API policy instead of model name heuristic#29630-don wants to merge 1 commit into
0-don wants to merge 1 commit into
Conversation
…ame heuristic The channel test hardcoded that any model containing "codex" in the name should be tested via /v1/responses (Responses API). This caused test failures for channels where the upstream doesn't support the Responses API for codex models, even though the models work fine via /v1/chat/completions. Replace the model-name-based heuristic with ShouldChatCompletionsUseResponsesGlobal() in both the request path selection and the test request body construction, matching what the production relay pipeline uses. Also fix playground custom body mode not injecting the user's new message into the custom payload's messages array before sending.
Contributor
WalkthroughThis PR modifies two files: it replaces a heuristic-based decision (checking for "codex" in model names) with a policy-driven approach in the channel test logic, and adds message injection into custom request payloads in the Playground component. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/pages/Playground/index.jsx (1)
249-270:⚠️ Potential issue | 🟠 MajorSide-effect mutation inside a React state updater will cause double send and duplicate messages in Strict Mode
customPayloadis mutated (customPayload.messages = [...]) andsendRequestis invoked all inside thesetMessageupdater callback. React 18 Strict Mode intentionally invokes state updaters twice to surface impure updaters. On the second invocation:
customPayload.messagesalready contains the injected user message → the spread produces a doubled user message.sendRequestfires a second API request with the duplicated payload.Move the injection and the
sendRequestcall outside the updater, keeping only the pure state derivation inside:🐛 Proposed fix – hoist side-effects out of the updater
if (customRequestMode && customRequestBody) { try { const customPayload = JSON.parse(customRequestBody); - setMessage((prevMessage) => { - const newMessages = [...prevMessage, userMessage, loadingMessage]; - - // Inject the user's new message into the custom payload's messages array - if (Array.isArray(customPayload.messages)) { - customPayload.messages = [ - ...customPayload.messages, - { role: MESSAGE_ROLES.USER, content }, - ]; - } - - // 发送自定义请求体 - sendRequest(customPayload, customPayload.stream !== false); - - // 发送消息后保存，传入新消息列表 - setTimeout(() => saveMessagesImmediately(newMessages), 0); - - return newMessages; - }); + // Inject the user's new message into the custom payload's messages array + if (Array.isArray(customPayload.messages)) { + customPayload.messages = [ + ...customPayload.messages, + { role: MESSAGE_ROLES.USER, content }, + ]; + } + + // 发送自定义请求体 + sendRequest(customPayload, customPayload.stream !== false); + + setMessage((prevMessage) => { + const newMessages = [...prevMessage, userMessage, loadingMessage]; + // 发送消息后保存，传入新消息列表 + setTimeout(() => saveMessagesImmediately(newMessages), 0); + return newMessages; + }); return; } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Playground/index.jsx` around lines 249 - 270, The updater passed to setMessage is performing side effects and mutating customPayload which causes duplicate messages/requests in React Strict Mode; to fix, parse customRequestBody into customPayload and build a newPayload (do not mutate customPayload) by appending { role: MESSAGE_ROLES.USER, content } to its messages array, call sendRequest(newPayload, newPayload.stream !== false) and schedule saveMessagesImmediately(newMessages) outside the setMessage updater, and keep the setMessage callback pure by only computing and returning the newMessages array (using userMessage and loadingMessage) without touching customPayload or calling sendRequest; ensure you reference customRequestBody, customPayload/newPayload, setMessage, sendRequest, saveMessagesImmediately, userMessage, loadingMessage and MESSAGE_ROLES.USER when applying the change.controller/channel-test.go (1)
688-695:⚠️ Potential issue | 🟡 MinorConsolidate policy evaluation to eliminate TOCTOU window
ShouldChatCompletionsUseResponsesGlobalreads from mutable global state (model_setting.GetGlobalSettings().ChatCompletionsToResponsesPolicy). The two independent calls at line 125 (testChannel) and line 689 (buildTestRequest) create a TOCTOU window: if the config toggles between them,requestPathand the request body type will disagree. The RelayMode switch at lines 319–328 would then fail the type assertion.Pass the policy decision as a parameter to
buildTestRequest:🔒 Proposed fix – single policy evaluation
-func buildTestRequest(model string, endpointType string, channel *model.Channel, isStream bool) dto.Request { +func buildTestRequest(model string, endpointType string, channel *model.Channel, isStream bool, useResponsesAPI bool) dto.Request {- // Use Responses API if the global policy says this channel+model should use it - if channel != nil && service.ShouldChatCompletionsUseResponsesGlobal(channel.Id, channel.Type, model) { + // Use Responses API if the caller determined the policy already + if useResponsesAPI {In
testChannel, evaluate once and pass through:+ useResponsesAPI := service.ShouldChatCompletionsUseResponsesGlobal(channel.Id, channel.Type, testModel) // Use Responses API if the global policy says this channel+model should use it - if service.ShouldChatCompletionsUseResponsesGlobal(channel.Id, channel.Type, testModel) { + if useResponsesAPI { requestPath = "/v1/responses" }- request := buildTestRequest(testModel, endpointType, channel, isStream) + request := buildTestRequest(testModel, endpointType, channel, isStream, useResponsesAPI)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel-test.go` around lines 688 - 695, The TOCTOU happens because ShouldChatCompletionsUseResponsesGlobal is called twice (in testChannel and inside buildTestRequest), causing requestPath and request body types to possibly disagree; fix this by evaluating the policy once in testChannel and passing the boolean decision into buildTestRequest (add a parameter like useResponses bool to buildTestRequest and update all callers), remove the internal call to ShouldChatCompletionsUseResponsesGlobal from buildTestRequest, and make buildTestRequest use the passed useResponses to choose whether to return an OpenAIResponsesRequest or ChatCompletions request and to set requestPath so the RelayMode switch (which asserts the request type) always sees a consistent body/type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@controller/channel-test.go`:
- Around line 688-695: The TOCTOU happens because
ShouldChatCompletionsUseResponsesGlobal is called twice (in testChannel and
inside buildTestRequest), causing requestPath and request body types to possibly
disagree; fix this by evaluating the policy once in testChannel and passing the
boolean decision into buildTestRequest (add a parameter like useResponses bool
to buildTestRequest and update all callers), remove the internal call to
ShouldChatCompletionsUseResponsesGlobal from buildTestRequest, and make
buildTestRequest use the passed useResponses to choose whether to return an
OpenAIResponsesRequest or ChatCompletions request and to set requestPath so the
RelayMode switch (which asserts the request type) always sees a consistent
body/type.
In `@web/src/pages/Playground/index.jsx`:
- Around line 249-270: The updater passed to setMessage is performing side
effects and mutating customPayload which causes duplicate messages/requests in
React Strict Mode; to fix, parse customRequestBody into customPayload and build
a newPayload (do not mutate customPayload) by appending { role:
MESSAGE_ROLES.USER, content } to its messages array, call
sendRequest(newPayload, newPayload.stream !== false) and schedule
saveMessagesImmediately(newMessages) outside the setMessage updater, and keep
the setMessage callback pure by only computing and returning the newMessages
array (using userMessage and loadingMessage) without touching customPayload or
calling sendRequest; ensure you reference customRequestBody,
customPayload/newPayload, setMessage, sendRequest, saveMessagesImmediately,
userMessage, loadingMessage and MESSAGE_ROLES.USER when applying the change.
Collaborator
|
这个你用来替换的方法不是这种用途的，目前的端点选择预测就是单纯的只按照模型名的 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ShouldChatCompletionsUseResponsesGlobal()instead of hardcoded codex name checkSummary by CodeRabbit
Bug Fixes
Improvements